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

Andy Shevchenko
2020-03-12T18:06:58Z

On Thu, Mar 12, 2020 at 06:26:48PM +0100, Saravanan Sekar 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.

...

> drivers/power/supply/Kconfig | 14 +-

Why unrelated changes here?

...

> +#include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/types.h>
> +#include <linux/power_supply.h>
> +#include <linux/workqueue.h>

All of them are needed?

...

> +enum mp2629_source_type {
> + MP2629_SOURCE_TYPE_NO_INPUT,
> + MP2629_SOURCE_TYPE_NON_STD,
> + MP2629_SOURCE_TYPE_SDP,
> + MP2629_SOURCE_TYPE_CDP,
> + MP2629_SOURCE_TYPE_DCP,
> + MP2629_SOURCE_TYPE_OTG = 7

+ comma? (It is not obvious there will be no extension in the future)

> +};
> +
> +enum {
> + INPUT_ILIM,
> + INPUT_VLIM,
> + CHARGE_ILIM,
> + CHARGE_VLIM,
> + PRECHARGE,

> + TERM_CURRENT

Ditto.

> +};

...

> +static enum power_supply_usb_type mp2629_usb_types[] = {
> + POWER_SUPPLY_USB_TYPE_SDP,
> + POWER_SUPPLY_USB_TYPE_DCP,
> + POWER_SUPPLY_USB_TYPE_CDP,
> + POWER_SUPPLY_USB_TYPE_PD_DRP,

> + POWER_SUPPLY_USB_TYPE_UNKNOWN,

Here it seems other way around, i.e. no comma.

> +};
> +
> +static enum power_supply_property mp2629_charger_usb_props[] = {
> + POWER_SUPPLY_PROP_ONLINE,
> + POWER_SUPPLY_PROP_USB_TYPE,
> + POWER_SUPPLY_PROP_VOLTAGE_NOW,
> + POWER_SUPPLY_PROP_CURRENT_NOW,
> + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,

> + POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT

...but here again, it can be extended.

> +};
> +
> +static enum power_supply_property mp2629_charger_bat_props[] = {
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_HEALTH,
> + POWER_SUPPLY_PROP_CHARGE_TYPE,
> + POWER_SUPPLY_PROP_VOLTAGE_NOW,
> + POWER_SUPPLY_PROP_CURRENT_NOW,
> + POWER_SUPPLY_PROP_CAPACITY,
> + POWER_SUPPLY_PROP_PRECHARGE_CURRENT,
> + POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,

> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX

Ditto.

> +};
> +
> +static struct mp2629_prop props[] = {
> + MP2629_PROPS(INPUT_ILIM, 100000, 3250000, 50000, 0),
> + MP2629_PROPS(INPUT_VLIM, 3800000, 5300000, 100000, 0),
> + MP2629_PROPS(CHARGE_ILIM, 320000, 4520000, 40000, 0),
> + MP2629_PROPS(CHARGE_VLIM, 3400000, 4670000, 10000, 1),
> + MP2629_PROPS(PRECHARGE, 120000, 720000, 40000, 4),

> + MP2629_PROPS(TERM_CURRENT, 80000, 680000, 40000, 0)

Ditto.

> +};

...

> +static char *adc_chan_name[] = { "mp2629-batt-volt", "mp2629-system-volt",
> + "mp2629-input-volt", "mp2629-batt-current",
> + "mp2629-input-current" };

One item per line, please. And comma in the last, new compatible hw might have
more channels.

> + ret = mp2629_get_value(map, prop->reg, &rval);
> + if (!ret) {
> + rval = (rval & prop->mask) >> prop->shift;
> + val->intval = (rval * prop->step) + prop->min;
> + }

Sounds like regmap field operation...
Ditto for similar code.

> +
> + return ret;
> +}

...

> + default:
> + ret = -EINVAL;
> + break;

return -EINVAL;

...

> + default:
> + ret = -EINVAL;
> + break;

Ditto.

...

> + default:
> + ret = -EINVAL;
> + break;

Ditto.

...

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

Too many parentheses.

...

> + return (psp == POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT ||
> + psp == POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT);

Ditto.

...

> + if (kstrtol(buf, 10, &val) < 0)
> + return -EINVAL;

Don't shadow the actual error code.

> +
> + if (val < 0 && val > 140)

> + return -EINVAL;

ERANGE is better.

...

> +static DEVICE_ATTR(batt_impedance_compensation, 0644,
> + mp2629_sysfs_impedance_show, mp2629_sysfs_impedance_store);

DEVICE_ATTR_RW()

> +static struct attribute *mp2629_charger_sysfs_attrs[] = {
> + &dev_attr_batt_impedance_compensation.attr,

> + NULL,

No comma.

> +};

...

> +static int mp2629_charger_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mp2629_charger *charger;
> + struct mp2629_info *info;
> + struct power_supply_config psy_cfg = {0};
> + int ret, i;

> + platform_set_drvdata(pdev, charger);

> + info = dev_get_drvdata(dev->parent);

Could be assigned in definition block above.

> + charger->info = info;
> + charger->dev = dev;
> +
> + for (i = 0; i < MP2629_ADC_CHAN_END; i++) {
> + charger->iiochan[i] = iio_channel_get(dev, adc_chan_name[i]);
> + if (IS_ERR(charger->iiochan[i])) {
> + ret = PTR_ERR(charger->iiochan[i]);
> + goto iio_fail;
> + }
> + }

> + return 0;
> +
> +iio_fail:

> + for (i = 0; i < MP2629_ADC_CHAN_END; i++) {
> + if (charger->iiochan[i] && !(IS_ERR(charger->iiochan[i])))
> + iio_channel_release(charger->iiochan[i]);
> + }

while (i--)
iio_channel_release(charger->iiochan[i]);

> +
> + dev_err(dev, "driver register fail: %d\n", ret);
> + return ret;
> +}

...

> + for (i = 0; i < MP2629_ADC_CHAN_END; i++) {

> + if (charger->iiochan[i])

When this is possible?
And shouldn't iio_channel_release() take care of NULL pointer? If it doesn't,
fix it in preparatory patch.

> + iio_channel_release(charger->iiochan[i]);
> + }

...

> +static const struct of_device_id mp2629_charger_of_match[] = {
> + { .compatible = "mps,mp2629_charger"},

> + {},

No comma.

> +};

--
With Best Regards,
Andy Shevchenko