| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9e105e70-a572-aff4-df91-a620e82d3436@gmail.com>
Date: Mon, 23 Mar 2020 13:29:37 +0100
From: saravanan sekar <sravanhome@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Lee Jones <lee.jones@...aro.org>, Rob Herring <robh+dt@...nel.org>,
Jonathan Cameron <jic23@...nel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald <pmeerw@...erw.net>,
Sebastian Reichel <sre@...nel.org>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-iio <linux-iio@...r.kernel.org>,
Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v4 4/5] power: supply: Add support for mps mp2629 battery
charger
Hi Andy,
On 23/03/20 12:26 am, Andy Shevchenko wrote:
> On Mon, Mar 23, 2020 at 12:47 AM Saravanan Sekar <sravanhome@...il.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?
I have to pass regmap (address) from mfd driver to its children, as per
review comment previous patch, mfd driver structure should not be
directly accessed in children.
Please suggest if any other better way
>> + 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
Thanks,
Saravanan
Powered by blists - more mailing lists