Re: [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery charger
From: Andy Shevchenko
Date: Sun Mar 22 2020 - 19:26:25 EST
On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@xxxxxxxxx> 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