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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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?

> +       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