Re: [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger

[saravanan sekar <sravanhome@gmail.com>]

Hi Andy,

On 23/03/20 12:26 am, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@gmail.com> wrote:
>> The mp2629 provides switching-mode battery charge management for
>> single-cell Li-ion or Li-polymer battery. Driver supports the
>> access/control input source and battery charging parameters.
> ...
>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of_device.h>
> Do you need this one?
>
>> +#include <linux/interrupt.h>
>> +#include <linux/iio/consumer.h>
>> +#include <linux/iio/types.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/regmap.h>
> Perhaps put them in order?
>
>> +#include <linux/mfd/core.h>
> How this is being used?
>
>> +#include <linux/mfd/mp2629.h>
> ...
>
>> +#define MP2629_MASK_INPUT_TYPE         0xe0
>> +#define MP2629_MASK_CHARGE_TYPE                0x18
>> +#define MP2629_MASK_CHARGE_CTRL                0x30
>> +#define MP2629_MASK_WDOG_CTRL          0x30
>> +#define MP2629_MASK_IMPEDANCE          0xf0
> GENMASK()?
>
> ...
>
>> +       struct regmap_field *regmap_fields[TERM_CURRENT + 1];
> Hmm... Why not to have a definition to cover + 1?
>
> ...
>
>> +static int mp2629_get_prop(struct mp2629_charger *charger,
>> +                          enum mp2629_field fld,
>> +                          union power_supply_propval *val)
>> +{
>> +       int ret;
>> +       unsigned int rval;
>> +
>> +       ret = regmap_field_read(charger->regmap_fields[fld], &rval);
>> +       if (!ret)
>> +               val->intval = (rval * props[fld].step) + props[fld].min;
>> +
>> +       return ret;
> Why not to use standard pattern, i.e.
>
>    if (ret)
>      return ret;
>    ...
>    return 0;
>
> ?
>
>> +}
> ...
>
>> +static int mp2629_charger_battery_set_prop(struct power_supply *psy,
>> +                                       enum power_supply_property psp,
>> +                                       const union power_supply_propval *val)
>> +{
>> +       struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
>> +       int ret;
> You may replace it with in-place return statements.
>
>> +
>> +       switch (psp) {
>> +       case POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT:
>> +               ret = mp2629_set_prop(charger, TERM_CURRENT, val);
>> +               break;
>> +
>> +       case POWER_SUPPLY_PROP_PRECHARGE_CURRENT:
>> +               ret = mp2629_set_prop(charger, PRECHARGE, val);
>> +               break;
>> +
>> +       case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>> +               ret = mp2629_set_prop(charger, CHARGE_VLIM, val);
>> +               break;
>> +
>> +       case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>> +               ret = mp2629_set_prop(charger, CHARGE_ILIM, val);
>> +               break;
>> +
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       return ret;
> ...and drop this completely.
>
>> +}
> ...
>
>> +       case POWER_SUPPLY_PROP_ONLINE:
>> +               ret = regmap_read(charger->regmap, MP2629_REG_STATUS, &rval);
>> +               if (!ret)
>> +                       val->intval = !!(rval & MP2629_MASK_INPUT_TYPE);
>> +               break;
> Traditional pattern?
>
> ...
>
>> +static int mp2629_charger_usb_set_prop(struct power_supply *psy,
>> +                               enum power_supply_property psp,
>> +                               const union power_supply_propval *val)
>> +{
>> +       struct mp2629_charger *charger = dev_get_drvdata(psy->dev.parent);
>> +       int ret;
> No need to have it.
>
>> +       switch (psp) {
>> +       case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
>> +               ret = mp2629_set_prop(charger, INPUT_VLIM, val);
>> +               break;
>> +
>> +       case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>> +               ret = mp2629_set_prop(charger, INPUT_ILIM, val);
>> +               break;
>> +
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       return ret;
>> +}
> ...
>
>> +       return (psp == POWER_SUPPLY_PROP_PRECHARGE_CURRENT ||
>> +               psp == POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT ||
>> +               psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
>> +               psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE);
> Redundant parentheses.
> Ditto for similar cases in the driver.
>
> ...
>
>> +static ssize_t batt_impedance_compensation_show(struct device *dev,
>> +                                          struct device_attribute *attr,
>> +                                          char *buf)
>> +{
>> +       struct mp2629_charger *charger = dev_get_drvdata(dev->parent);
>> +       unsigned int rval;
>> +       int ret;
>> +
>> +       ret = regmap_read(charger->regmap, MP2629_REG_IMPEDANCE_COMP, &rval);
>> +       if (ret < 0)
> ' < 0' is not needed.
> Ditto for other cases.
>
>> +               return ret;
>> +
>> +       rval = (rval >> 4) * 10;
>> +
>> +       return scnprintf(buf, PAGE_SIZE, "%d mohm\n", rval);
> Simple sprintf().
>
>> +}
> ...
>
>> +static ssize_t batt_impedance_compensation_store(struct device *dev,
>> +                                           struct device_attribute *attr,
>> +                                           const char *buf,
>> +                                           size_t count)
>> +{
>> +       struct mp2629_charger *charger = dev_get_drvdata(dev->parent);
>> +       long val;
>> +       int ret;
>> +
>> +       ret = kstrtol(buf, 10, &val);
>> +       if (ret < 0)
> No need to check for negative only.
>
>> +               return ret;
>> +
>> +       if (val < 0 && val > 140)
>> +               return -ERANGE;
> And what the point then to use l instead of ul or even uint variant of
> the conversion above?
>
>> +       /* multiples of 10 mohm so round off */
>> +       val = val / 10;
>> +       ret = regmap_update_bits(charger->regmap, MP2629_REG_IMPEDANCE_COMP,
>> +                                       MP2629_MASK_IMPEDANCE, val << 4);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return count;
>> +}
> ...
>
>> +static int mp2629_charger_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       void **pdata = pdev->dev.platform_data;
> Why void?
> Why **?
> Why not to use dev_get_platdata()?
> Why do we need platform data at all?


I have to pass regmap (address) from mfd driver to its children, as per 
review comment previous patch, mfd driver structure should not be 
directly accessed in children.
Please suggest if any other better way

>> +       struct mp2629_charger *charger;
>> +       struct power_supply_config psy_cfg = {0};
>> +       int ret, i;
>> +       charger->regmap = *pdata;
>> +       regmap_update_bits(charger->regmap, MP2629_REG_INTERRUPT,
>> +                               GENMASK(6, 5), (BIT(6) | BIT(5)));
> Too many parentheses.
>
>> +}
> --
> With Best Regards,
> Andy Shevchenko

Thanks,
Saravanan