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

Andy Shevchenko
2020-03-22T23:26:16Z

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