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?
