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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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