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
