| Message ID | 20190212010143.3729-1-virendra.kakade@ni.com |
|---|---|
| Headers | show |
| Series | Add support for Ettus Research E31x devices PMU | expand |
Hi, On Mon, Feb 11, 2019 at 07:01:41PM -0600, Virendra Kakade wrote: > Add support for E31x devices charger which is a sub-device of E31X-PMU. > Enables query of charger properties from device tree. > > Signed-off-by: Virendra Kakade <virendra.kakade@ni.com> > --- > drivers/power/supply/Kconfig | 6 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/e31x-charger.c | 190 ++++++++++++++++++++++++++++ > include/linux/mfd/e31x-pmu.h | 4 + > 4 files changed, 201 insertions(+) > create mode 100644 drivers/power/supply/e31x-charger.c > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index e901b9879e7e..3d043b8fd49c 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -660,4 +660,10 @@ config FUEL_GAUGE_SC27XX > Say Y here to enable support for fuel gauge with SC27XX > PMIC chips. > > +config E31X_CHARGER > + tristate "Ettus Research E31x charger support" > + depends on MFD_E31X_PMU > + help > + This adds support for the battery charger in Ettus Research E31x PMU. > + > endif # POWER_SUPPLY > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > index b731c2a9b695..6f4f8ca5484e 100644 > --- a/drivers/power/supply/Makefile > +++ b/drivers/power/supply/Makefile > @@ -87,3 +87,4 @@ obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o > obj-$(CONFIG_CHARGER_CROS_USBPD) += cros_usbpd-charger.o > obj-$(CONFIG_CHARGER_SC2731) += sc2731_charger.o > obj-$(CONFIG_FUEL_GAUGE_SC27XX) += sc27xx_fuel_gauge.o > +obj-$(CONFIG_E31X_CHARGER) += e31x-charger.o > diff --git a/drivers/power/supply/e31x-charger.c b/drivers/power/supply/e31x-charger.c > new file mode 100644 > index 000000000000..f154482bf95e > --- /dev/null > +++ b/drivers/power/supply/e31x-charger.c > @@ -0,0 +1,190 @@ > +// SPDX-License-Identifier: GPL-2.0 This specifies GPLv2 only, but MODULE_LICENSE("GPL") specifies GPL2 or later. Please fix one of them. > +/* > + * Copyright (c) 2018 National Instruments Corp > + * Author: Virendra Kakade <virendra.kakade@ni.com> > + * > + * Ettus Research E31x charger driver > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/mfd/syscon.h> > +#include <linux/mutex.h> > +#include <linux/regmap.h> > +#include <linux/string.h> > +#include <linux/slab.h> > +#include <linux/power_supply.h> > +#include <linux/delay.h> > +#include <linux/mfd/e31x-pmu.h> Please check the includes. Quite a few seem to be unused. > + > +#define E31X_PMU_CHARGER_HEALTH_MASK GENMASK(1, 0) > +#define E31X_PMU_CHARGER_HEALTH_SHIFT 0 > +#define E31X_PMU_CHARGER_CHARGE_TYPE_MASK GENMASK(4, 3) > +#define E31X_PMU_CHARGER_CHARGE_TYPE_SHIFT 3 > + > +struct e31x_charger_dev { > + struct regmap *regmap; > + struct power_supply *supply; > +}; > + > +static int e31x_charger_get_health(struct e31x_charger_dev *charger, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(charger->regmap, E31X_PMU_REG_CHARGER, &value); > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(CHARGER_HEALTH, value); > + switch (value) { > + case 0x0: > + val->intval = POWER_SUPPLY_HEALTH_GOOD; > + break; > + case 0x1: > + val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE; > + break; > + case 0x2: > + val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE; > + break; > + case 0x3: > + val->intval = POWER_SUPPLY_HEALTH_OVERHEAT; > + break; > + default: > + val->intval = POWER_SUPPLY_HEALTH_UNKNOWN; > + break; > + }; > + > + return 0; > +} > + > +static int e31x_charger_get_type(struct e31x_charger_dev *charger, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(charger->regmap, E31X_PMU_REG_CHARGER, &value); > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(CHARGER_CHARGE_TYPE, value); > + switch (value) { > + case 0x0: > + val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE; > + break; > + case 0x1: > + val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE; > + break; > + case 0x2: > + val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST; > + break; > + default: > + val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE; > + break; > + }; > + return 0; > +} > + > +static int e31x_charger_get_online(struct e31x_charger_dev *charger, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(charger->regmap, E31X_PMU_REG_CHARGER, &value); > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(CHARGER_ONLINE, value); > + val->intval = !!value; > + > + return 0; > +} > + > +static int e31x_charger_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct e31x_charger_dev *charger = power_supply_get_drvdata(psy); > + int err = -EINVAL; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_HEALTH: > + return e31x_charger_get_health(charger, val); > + case POWER_SUPPLY_PROP_CHARGE_TYPE: > + return e31x_charger_get_type(charger, val); > + case POWER_SUPPLY_PROP_ONLINE: > + return e31x_charger_get_online(charger, val); > + case POWER_SUPPLY_PROP_SCOPE: > + val->intval = POWER_SUPPLY_SCOPE_SYSTEM; > + return 0; > + default: > + break; > + } > + > + return err; > +} > + > +static enum power_supply_property e31x_charger_properties[] = { > + POWER_SUPPLY_PROP_HEALTH, > + POWER_SUPPLY_PROP_CHARGE_TYPE, > + POWER_SUPPLY_PROP_ONLINE, > + POWER_SUPPLY_PROP_SCOPE, > +}; > + > +static const struct power_supply_desc e31x_charger_desc = { > + .name = "e31x-charger", > + .type = POWER_SUPPLY_TYPE_MAINS, > + .properties = e31x_charger_properties, > + .num_properties = ARRAY_SIZE(e31x_charger_properties), > + .get_property = e31x_charger_get_property, > +}; > + > +static const struct of_device_id e31x_charger_id[] = { > + { .compatible = "ni,e31x-charger" }, > + { }, > +}; > + > +static int e31x_charger_probe(struct platform_device *pdev) > +{ > + struct power_supply_config psy_cfg = {}; > + struct e31x_charger_dev *charger; > + > + if (!of_device_is_available(pdev->dev.of_node)) > + return -ENODEV; > + > + charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL); > + if (!charger) > + return -ENOMEM; > + > + charger->regmap = syscon_regmap_lookup_by_phandle(\ useless \ > + pdev->dev.parent->of_node, "regmap"); > + > + psy_cfg.of_node = pdev->dev.of_node; > + psy_cfg.drv_data = charger; > + > + charger->supply = devm_power_supply_register(&pdev->dev, > + &e31x_charger_desc, > + &psy_cfg); > + > + if (IS_ERR(charger->supply)) > + return PTR_ERR(charger->supply); > + return 0; > +} > + > +static struct platform_driver e31x_charger_driver = { > + .driver = { > + .name = "e31x-charger", > + .of_match_table = e31x_charger_id, > + }, > + .probe = e31x_charger_probe, > +}; > +module_platform_driver(e31x_charger_driver); > + > +MODULE_AUTHOR("Virendra Kakade <virendra.kakade@ni.com>"); > +MODULE_DESCRIPTION("E31x charger driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/e31x-pmu.h b/include/linux/mfd/e31x-pmu.h > index c57d5a8c1aad..e0649845fe4d 100644 > --- a/include/linux/mfd/e31x-pmu.h > +++ b/include/linux/mfd/e31x-pmu.h > @@ -12,9 +12,13 @@ > #include <linux/bitops.h> > > #define E31X_PMU_REG_MISC 0x04 > +#define E31X_PMU_REG_CHARGER 0x0c > > #define E31X_PMU_GET_FIELD(name, reg) \ > (((reg) & E31X_PMU_## name ##_MASK) >> \ > E31X_PMU_## name ##_SHIFT) > > +#define E31X_PMU_CHARGER_ONLINE_MASK BIT(2) > +#define E31X_PMU_CHARGER_ONLINE_SHIFT 2 > + > #endif /* MFD_E31X_PMU_H */ Otherwise this looks good. -- Sebastian
Hi, On Mon, Feb 11, 2019 at 07:01:43PM -0600, Virendra Kakade wrote: > Add support for E31x devices battery which is a sub-device of E31X-PMU. > Enables query of battery properties from device tree. > > Signed-off-by: Virendra Kakade <virendra.kakade@ni.com> > --- > drivers/power/supply/Kconfig | 6 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/e31x-battery.c | 357 ++++++++++++++++++++++++++++ > include/linux/mfd/e31x-pmu.h | 15 +- > 4 files changed, 377 insertions(+), 2 deletions(-) > create mode 100644 drivers/power/supply/e31x-battery.c > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index 3d043b8fd49c..e05e02dffc49 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -666,4 +666,10 @@ config E31X_CHARGER > help > This adds support for the battery charger in Ettus Research E31x PMU. > > +config E31X_BATTERY > + tristate "Ettus Research E31x battery support" > + depends on MFD_E31X_PMU > + help > + This adds support for the battery in Ettus Research E31x PMU. > + > endif # POWER_SUPPLY > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > index 6f4f8ca5484e..3850d08db942 100644 > --- a/drivers/power/supply/Makefile > +++ b/drivers/power/supply/Makefile > @@ -88,3 +88,4 @@ obj-$(CONFIG_CHARGER_CROS_USBPD) += cros_usbpd-charger.o > obj-$(CONFIG_CHARGER_SC2731) += sc2731_charger.o > obj-$(CONFIG_FUEL_GAUGE_SC27XX) += sc27xx_fuel_gauge.o > obj-$(CONFIG_E31X_CHARGER) += e31x-charger.o > +obj-$(CONFIG_E31X_BATTERY) += e31x-battery.o > diff --git a/drivers/power/supply/e31x-battery.c b/drivers/power/supply/e31x-battery.c > new file mode 100644 > index 000000000000..d44a639e8bc2 > --- /dev/null > +++ b/drivers/power/supply/e31x-battery.c > @@ -0,0 +1,357 @@ > +// SPDX-License-Identifier: GPL-2.0 same issue as in charger driver. > +/* > + * Copyright (c) 2018 National Instruments Corp > + * Author: Virendra Kakade <virendra.kakade@ni.com> > + * > + * Ettus Research E31x battery driver > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/mfd/syscon.h> > +#include <linux/mutex.h> > +#include <linux/regmap.h> > +#include <linux/string.h> > +#include <linux/slab.h> > +#include <linux/power_supply.h> > +#include <linux/delay.h> > +#include <linux/mfd/e31x-pmu.h> I also believe a few of those includes are unused. > +#define E31X_PMU_BATTERY_VOLTAGE_MASK GENMASK(23, 8) > +#define E31X_PMU_BATTERY_VOLTAGE_SHIFT 8 > +#define E31X_PMU_BATTERY_TEMP_ALERT_MASK GENMASK(7, 6) > +#define E31X_PMU_BATTERY_TEMP_ALERT_SHIFT 6 > +#define E31X_PMU_BATTERY_ONLINE_MASK BIT(5) > +#define E31X_PMU_BATTERY_ONLINE_SHIFT 5 > +#define E31X_PMU_BATTERY_HEALTH_MASK GENMASK(4, 2) > +#define E31X_PMU_BATTERY_HEALTH_SHIFT 2 > +#define E31X_PMU_BATTERY_STATUS_MASK GENMASK(1, 0) > +#define E31X_PMU_BATTERY_STATUS_SHIFT 0 > + > +#define E31X_PMU_GAUGE_TEMP_MASK GENMASK(31, 16) > +#define E31X_PMU_GAUGE_TEMP_SHIFT 16 > +#define E31X_PMU_GAUGE_CHARGE_MASK GENMASK(15, 0) > +#define E31X_PMU_GAUGE_CHARGE_SHIFT 0 > + > +#define E31X_PMU_GAUGE_VOLTAGE_MASK GENMASK(15, 0) > +#define E31X_PMU_GAUGE_VOLTAGE_SHIFT 0 > + > +#define E31X_PMU_GAUGE_CHARGE_LAST_FULL_MASK GENMASK(15, 0) > +#define E31X_PMU_GAUGE_CHARGE_LAST_FULL_SHIFT 0 > + > +#define E31X_BATTERY_CHARGE_DESIGN_FULL (3200000) > +#define E31X_PMU_VSENSE (6000) > + > +struct e31x_battery_dev { > + struct regmap *regmap; > + struct power_supply *supply; > +}; > + > +static int e31x_battery_set_prop(struct power_supply *psy, > + enum power_supply_property psp, > + const union power_supply_propval *val) > +{ > + return 0; > +} If you don't have writable properties you do not have to specify any handler. > +static int e31x_battery_get_status(struct e31x_battery_dev *bat, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(bat->regmap, E31X_PMU_REG_CHARGER, &value); > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(CHARGER_ONLINE, value); > + > + /* if charger is offline, we're discharging, period */ > + if (!value) { > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + return 0; > + } > + > + err = regmap_read(bat->regmap, E31X_PMU_REG_BATTERY, &value); > + if (err) > + return err; > + > + value &= E31X_PMU_BATTERY_STATUS_MASK; > + > + switch (value) { > + case 0x0: > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > + break; > + case 0x1: > + val->intval = POWER_SUPPLY_STATUS_CHARGING; > + break; > + case 0x2: > + val->intval = POWER_SUPPLY_STATUS_FULL; > + break; > + case 0x3: > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + break; > + default: > + return -EIO; > + }; > + > + return 0; > +} > + > +static int e31x_battery_get_health(struct e31x_battery_dev *bat, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(bat->regmap, E31X_PMU_REG_BATTERY, &value); > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(BATTERY_HEALTH, value); > + > + switch (value) { > + case 0x00: > + val->intval = POWER_SUPPLY_HEALTH_GOOD; > + break; > + case 0x01: > + val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE; > + break; > + case 0x02: > + val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE; > + break; > + case 0x03: > + val->intval = POWER_SUPPLY_HEALTH_OVERHEAT; > + break; > + case 0x04: > + val->intval = POWER_SUPPLY_HEALTH_COLD; > + break; > + default: > + val->intval = POWER_SUPPLY_HEALTH_UNKNOWN; > + break; > + } > + > + return 0; > +} > + > +static int e31x_battery_get_online(struct e31x_battery_dev *bat, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(bat->regmap, E31X_PMU_REG_BATTERY, &value); > + if (err) > + return err; > + > + val->intval = !!(value & E31X_PMU_BATTERY_ONLINE_MASK); > + > + return 0; > +} > + > +static int e31x_battery_get_voltage_now(struct e31x_battery_dev *bat, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(bat->regmap, E31X_PMU_REG_BATTERY, &value); > + > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(BATTERY_VOLTAGE, value); > + > + val->intval = 10000 * (value * E31X_PMU_VSENSE / GENMASK(15, 0)); > + > + return 0; > +} > + > +#define E31X_PMU_BIN_TO_UAH(x) ((x) * 53) > +#define E31X_PMU_UAH_TO_BIN(x) ((x) / 53) > + > +static int e31x_battery_get_charge_now(struct e31x_battery_dev *bat, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(bat->regmap, E31X_PMU_REG_GAUGE, &value); > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(GAUGE_CHARGE, value); > + val->intval = E31X_PMU_BIN_TO_UAH(value); > + > + return 0; > +} > + > +static int e31x_battery_get_temp(struct e31x_battery_dev *bat, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(bat->regmap, E31X_PMU_REG_GAUGE, &value); > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(GAUGE_TEMP, value); > + val->intval = 10 * (((600 * value) / 0xffff) - 273); > + > + return 0; > +} > + > +static int e31x_battery_get_charge_full(struct e31x_battery_dev *bat, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + > + err = regmap_read(bat->regmap, E31X_PMU_REG_LAST, &value); > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(GAUGE_CHARGE_LAST_FULL, value); > + val->intval = E31X_PMU_BIN_TO_UAH(value); > + > + return 0; > +} > + > +static const int e31x_pmu_temp_values[] = { > + 600, 800, 1000, 1200 > +}; > + > +static int e31x_battery_get_temp_alert_max(struct e31x_battery_dev *bat, > + union power_supply_propval *val) > +{ > + u32 value; > + int err; > + int i; > + > + err = regmap_read(bat->regmap, E31X_PMU_REG_BATTERY, &value); > + if (err) > + return err; > + > + value = E31X_PMU_GET_FIELD(BATTERY_TEMP_ALERT, value); > + for (i = 1; i < ARRAY_SIZE(e31x_pmu_temp_values); i++) > + if (e31x_pmu_temp_values[i] > value) > + break; > + > + val->intval = e31x_pmu_temp_values[i - 1]; > + > + return 0; > +} > + > +static int e31x_battery_get_prop(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct e31x_battery_dev *battery = power_supply_get_drvdata(psy); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_STATUS: > + return e31x_battery_get_status(battery, val); > + case POWER_SUPPLY_PROP_HEALTH: > + return e31x_battery_get_health(battery, val); > + case POWER_SUPPLY_PROP_ONLINE: > + return e31x_battery_get_online(battery, val); > + case POWER_SUPPLY_PROP_TECHNOLOGY: > + val->intval = POWER_SUPPLY_TECHNOLOGY_LION; > + break; > + case POWER_SUPPLY_PROP_TEMP: > + return e31x_battery_get_temp(battery, val); > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + return e31x_battery_get_voltage_now(battery, val); > + case POWER_SUPPLY_PROP_CHARGE_NOW: > + return e31x_battery_get_charge_now(battery, val); > + case POWER_SUPPLY_PROP_CHARGE_FULL: > + return e31x_battery_get_charge_full(battery, val); > + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > + val->intval = E31X_BATTERY_CHARGE_DESIGN_FULL; > + return 0; > + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: > + return e31x_battery_get_temp_alert_max(battery, val); > + default: > + break; > + }; > + > + return 0; > +} > + > +static enum power_supply_property e31x_battery_props[] = { > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_HEALTH, > + POWER_SUPPLY_PROP_ONLINE, > + POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_TEMP, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_CHARGE_NOW, > + POWER_SUPPLY_PROP_CHARGE_FULL, > + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, > + POWER_SUPPLY_PROP_TEMP_ALERT_MAX, > +}; > + > +static int e31x_battery_prop_writeable(struct power_supply *psy, > + enum power_supply_property psp) > +{ > + return psp == POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN || > + psp == POWER_SUPPLY_PROP_SCOPE; > +} You'r e31x_battery_set_prop() is empty and you do not even expose POWER_SUPPLY_PROP_SCOPE. > +static const struct power_supply_desc e31x_battery_desc = { > + .name = "e31x-battery", > + .type = POWER_SUPPLY_TYPE_BATTERY, > + .properties = e31x_battery_props, > + .num_properties = ARRAY_SIZE(e31x_battery_props), > + .property_is_writeable = e31x_battery_prop_writeable, > + .get_property = e31x_battery_get_prop, > + .set_property = e31x_battery_set_prop, > +}; > + > +static const struct of_device_id e31x_battery_id[] = { > + { .compatible = "ni,e31x-battery" }, > + { }, > +}; > + > +static int e31x_battery_probe(struct platform_device *pdev) > +{ > + struct power_supply_config psy_cfg = {}; > + struct e31x_battery_dev *battery; > + > + if (!of_device_is_available(pdev->dev.of_node)) > + return -ENODEV; Why is this needed? I would expect, that the driver is not being probed for a disabled node. > + battery = devm_kzalloc(&pdev->dev, sizeof(*battery), GFP_KERNEL); > + if (!battery) > + return -ENOMEM; > + > + battery->regmap = syscon_regmap_lookup_by_phandle(\ useless \ > + pdev->dev.parent->of_node, "regmap"); > + > + psy_cfg.of_node = pdev->dev.of_node; > + psy_cfg.drv_data = battery; > + > + battery->supply = devm_power_supply_register(&pdev->dev, > + &e31x_battery_desc, > + &psy_cfg); > + if (IS_ERR(battery->supply)) > + return PTR_ERR(battery->supply); > + return 0; > +} > + > +static struct platform_driver e31x_battery_driver = { > + .driver = { > + .name = "e31x-battery", > + .of_match_table = e31x_battery_id, > + }, > + .probe = e31x_battery_probe, > +}; > +module_platform_driver(e31x_battery_driver); > + > +MODULE_AUTHOR("Virendra Kakade <virendra.kakade@ni.com>"); > +MODULE_DESCRIPTION("E31x battery driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/e31x-pmu.h b/include/linux/mfd/e31x-pmu.h > index e0649845fe4d..0441f8f896ca 100644 > --- a/include/linux/mfd/e31x-pmu.h > +++ b/include/linux/mfd/e31x-pmu.h > @@ -11,13 +11,24 @@ > > #include <linux/bitops.h> > > -#define E31X_PMU_REG_MISC 0x04 > -#define E31X_PMU_REG_CHARGER 0x0c > +#define E31X_PMU_REG_MISC 0x04 > +#define E31X_PMU_REG_BATTERY 0x08 > +#define E31X_PMU_REG_CHARGER 0x0c > +#define E31X_PMU_REG_GAUGE 0x10 > +#define E31X_PMU_REG_STATUS 0x14 > +#define E31X_PMU_REG_LAST 0x18 > +#define E31X_PMU_REG_EEPROM 0x1c > > #define E31X_PMU_GET_FIELD(name, reg) \ > (((reg) & E31X_PMU_## name ##_MASK) >> \ > E31X_PMU_## name ##_SHIFT) > > +/* the eeprom register */ > +static const u32 E31X_PMU_EEPROM_AUTOBOOT_MASK = BIT(0); > +static const u32 E31X_PMU_EEPROM_AUTOBOOT_SHIFT; > +static const u32 E31X_PMU_EEPROM_DB_POWER_MASK = BIT(1); > +static const u32 E31X_PMU_EEPROM_DB_POWER_SHIFT = 1; > + > #define E31X_PMU_CHARGER_ONLINE_MASK BIT(2) > #define E31X_PMU_CHARGER_ONLINE_SHIFT 2 Otherwise looks good. -- Sebastian
On Mon, 11 Feb 2019, Virendra Kakade wrote: > Signed-off-by: Virendra Kakade <virendra.kakade@ni.com> > --- > drivers/mfd/Kconfig | 7 +++ > drivers/mfd/Makefile | 2 +- > drivers/mfd/e31x-pmu.c | 89 ++++++++++++++++++++++++++++++++++++ > include/linux/mfd/e31x-pmu.h | 20 ++++++++ > 4 files changed, 117 insertions(+), 1 deletion(-) > create mode 100644 drivers/mfd/e31x-pmu.c > create mode 100644 include/linux/mfd/e31x-pmu.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 8c5dfdce4326..6c4c0747f2a5 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1916,4 +1916,11 @@ config RAVE_SP_CORE > device found on several devices in RAVE line of hardware. > > endmenu > + > +config MFD_E31X_PMU > + tristate "E31X PMU driver" > + select MFD_SYSCON > + help > + Select this to get support for the Ettus Research E31x devices. > + > endif > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 12980a4ad460..e37c2057134b 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -241,4 +241,4 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o > obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o > obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o > obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > - > +obj-$(CONFIG_MFD_E31X_PMU) += e31x-pmu.o > diff --git a/drivers/mfd/e31x-pmu.c b/drivers/mfd/e31x-pmu.c > new file mode 100644 > index 000000000000..383df68a538f > --- /dev/null > +++ b/drivers/mfd/e31x-pmu.c > @@ -0,0 +1,89 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 National Instruments Corp This is out of date. '\n' here. > + * Author: Virendra Kakade<virendra.kakade@ni.com> You need a space after your name. > + * Ettus Research E31x PMU MFD driver Please expand PMU. > + */ > + > +#include <linux/err.h> > +#include <linux/delay.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/mfd/core.h> > +#include <linux/of_device.h> > +#include <linux/mfd/e31x-pmu.h> > +#include <linux/platform_device.h> Alphabetical. > +#define E31X_PMU_MISC_IRQ_MASK BIT(8) > +#define E31X_PMU_MISC_IRQ_SHIFT 8 > +#define E31X_PMU_MISC_VERSION_MIN_MASK GENMASK(3, 0) > +#define E31X_PMU_MISC_VERSION_MIN_SHIFT 0 > +#define E31X_PMU_MISC_VERSION_MAJ_MASK GENMASK(7, 4) > +#define E31X_PMU_MISC_VERSION_MAJ_SHIFT 4 > + > +struct e31x_pmu { > + struct regmap *regmap; > +}; A whole struct for one attribute? > +static int e31x_pmu_check_version(struct e31x_pmu *pmu) > +{ > + int timeout = 100; Where does 100 come from? > + u32 misc, maj; > + int err; 'ret' is more common. > + /* we need to wait a bit for firmware to populate the fields */ "a bit" ? What does the datasheet say? Please use proper grammar. Capital letters. > + while (timeout--) { > + err = regmap_read(pmu->regmap, E31X_PMU_REG_MISC, &misc); > + if (err) > + return err; > + if (misc) > + break; > + > + usleep_range(2500, 5000); Formatting. > + } > + > + /* only firmware versions above 2.0 are supported */ "Only supports firmware version 2.0 and above" > + maj = E31X_PMU_GET_FIELD(MISC_VERSION_MAJ, misc); > + if (maj < 2) > + return -ENOTSUPP; '\n' here. > + return 0; > +} > + > +static int e31x_pmu_probe(struct platform_device *pdev) > +{ > + struct e31x_pmu *pmu; > + > + pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL); > + if (!pmu) > + return -ENOMEM; '\n' here. > + pmu->regmap = syscon_regmap_lookup_by_phandle(\ Why are you storing regmap? Just pass it directly to e31x_pmu_check_version(). > + pdev->dev.of_node, "regmap"); > + Remove this line. > + if (IS_ERR(pmu->regmap)) > + return PTR_ERR(pmu->regmap); '\n' here. > + if (e31x_pmu_check_version(pmu)) Please split out the function from the if. Use a variable 'ret' to feed into and check that. > + return -ENOTSUPP; '\n' here. > + return devm_of_platform_populate(&pdev->dev); > +} > + > +static const struct of_device_id e31x_pmu_id[] = { > + { .compatible = "ni,e31x-pmu" }, > + {}, > +}; > + > +static struct platform_driver e31x_pmu_driver = { > + .driver = { > + .name = "e31x-pmu", > + .of_match_table = e31x_pmu_id, These 2 lines need indenting. > + }, > + .probe = e31x_pmu_probe, > +}; > +module_platform_driver(e31x_pmu_driver); > + > +MODULE_DESCRIPTION("E31x PMU driver"); > +MODULE_AUTHOR("Virendra Kakade <virendra.kakade@ni.com>"); > +MODULE_LICENSE("GPL"); So the whole purpose of this driver is to do a version check. Seems like over-kill! Probably better to do the version check in an inline function stored in a header file, then call it from each of the children's .probe() function. > diff --git a/include/linux/mfd/e31x-pmu.h b/include/linux/mfd/e31x-pmu.h > new file mode 100644 > index 000000000000..c57d5a8c1aad > --- /dev/null > +++ b/include/linux/mfd/e31x-pmu.h > @@ -0,0 +1,20 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 National Instruments Corp Out of date. '\n' here. > + * Author: Virendra Kakade <virendra.kakade@ni.com> > + * > + * Ettus Research E31x PMU constants > + */ > + > +#ifndef MFD_E31X_PMU_H > +#define MFD_E31X_PMU_H > + > +#include <linux/bitops.h> > + > +#define E31X_PMU_REG_MISC 0x04 > + > +#define E31X_PMU_GET_FIELD(name, reg) \ > + (((reg) & E31X_PMU_## name ##_MASK) >> \ > + E31X_PMU_## name ##_SHIFT) > + > +#endif /* MFD_E31X_PMU_H */
Hi Lee, thanks for your feedback! On Thu, Feb 14, 2019 at 1:34 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Mon, 11 Feb 2019, Virendra Kakade wrote: > > > Signed-off-by: Virendra Kakade <virendra.kakade@ni.com> > > --- > > drivers/mfd/Kconfig | 7 +++ > > drivers/mfd/Makefile | 2 +- > > drivers/mfd/e31x-pmu.c | 89 ++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/e31x-pmu.h | 20 ++++++++ > > 4 files changed, 117 insertions(+), 1 deletion(-) > > create mode 100644 drivers/mfd/e31x-pmu.c > > create mode 100644 include/linux/mfd/e31x-pmu.h > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index 8c5dfdce4326..6c4c0747f2a5 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -1916,4 +1916,11 @@ config RAVE_SP_CORE > > device found on several devices in RAVE line of hardware. > > > > endmenu > > + > > +config MFD_E31X_PMU > > + tristate "E31X PMU driver" > > + select MFD_SYSCON > > + help > > + Select this to get support for the Ettus Research E31x devices. > > + > > endif > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 12980a4ad460..e37c2057134b 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -241,4 +241,4 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o > > obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o > > obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o > > obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > > - > > +obj-$(CONFIG_MFD_E31X_PMU) += e31x-pmu.o > > diff --git a/drivers/mfd/e31x-pmu.c b/drivers/mfd/e31x-pmu.c > > new file mode 100644 > > index 000000000000..383df68a538f > > --- /dev/null > > +++ b/drivers/mfd/e31x-pmu.c > > @@ -0,0 +1,89 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 National Instruments Corp > > This is out of date. > > '\n' here. > > > + * Author: Virendra Kakade<virendra.kakade@ni.com> > > You need a space after your name. > > > + * Ettus Research E31x PMU MFD driver > > Please expand PMU. > > > + */ > > + > > +#include <linux/err.h> > > +#include <linux/delay.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/module.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/regmap.h> > > +#include <linux/mfd/core.h> > > +#include <linux/of_device.h> > > +#include <linux/mfd/e31x-pmu.h> > > +#include <linux/platform_device.h> > > Alphabetical. > > > +#define E31X_PMU_MISC_IRQ_MASK BIT(8) > > +#define E31X_PMU_MISC_IRQ_SHIFT 8 > > +#define E31X_PMU_MISC_VERSION_MIN_MASK GENMASK(3, 0) > > +#define E31X_PMU_MISC_VERSION_MIN_SHIFT 0 > > +#define E31X_PMU_MISC_VERSION_MAJ_MASK GENMASK(7, 4) > > +#define E31X_PMU_MISC_VERSION_MAJ_SHIFT 4 > > + > > +struct e31x_pmu { > > + struct regmap *regmap; > > +}; > > A whole struct for one attribute? > > > +static int e31x_pmu_check_version(struct e31x_pmu *pmu) > > +{ > > + int timeout = 100; > > Where does 100 come from? > > > + u32 misc, maj; > > + int err; > > 'ret' is more common. > > > + /* we need to wait a bit for firmware to populate the fields */ > > "a bit" ? > > What does the datasheet say? > > Please use proper grammar. Capital letters. > > > + while (timeout--) { > > + err = regmap_read(pmu->regmap, E31X_PMU_REG_MISC, &misc); > > + if (err) > > + return err; > > + if (misc) > > + break; > > + > > + usleep_range(2500, 5000); > > Formatting. > > > + } > > + > > + /* only firmware versions above 2.0 are supported */ > > "Only supports firmware version 2.0 and above" > > > + maj = E31X_PMU_GET_FIELD(MISC_VERSION_MAJ, misc); > > + if (maj < 2) > > + return -ENOTSUPP; > > '\n' here. > > > + return 0; > > +} > > + > > +static int e31x_pmu_probe(struct platform_device *pdev) > > +{ > > + struct e31x_pmu *pmu; > > + > > + pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL); > > + if (!pmu) > > + return -ENOMEM; > > '\n' here. > > > + pmu->regmap = syscon_regmap_lookup_by_phandle(\ > > Why are you storing regmap? See my comment below. > > Just pass it directly to e31x_pmu_check_version(). > > > + pdev->dev.of_node, "regmap"); > > + > > Remove this line. > > > + if (IS_ERR(pmu->regmap)) > > + return PTR_ERR(pmu->regmap); > > '\n' here. > > > + if (e31x_pmu_check_version(pmu)) > > Please split out the function from the if. > > Use a variable 'ret' to feed into and check that. > > > + return -ENOTSUPP; > > '\n' here. > > > + return devm_of_platform_populate(&pdev->dev); > > +} > > + > > +static const struct of_device_id e31x_pmu_id[] = { > > + { .compatible = "ni,e31x-pmu" }, > > + {}, > > +}; > > + > > +static struct platform_driver e31x_pmu_driver = { > > + .driver = { > > + .name = "e31x-pmu", > > + .of_match_table = e31x_pmu_id, > > These 2 lines need indenting. > > > + }, > > + .probe = e31x_pmu_probe, > > +}; > > +module_platform_driver(e31x_pmu_driver); > > + > > +MODULE_DESCRIPTION("E31x PMU driver"); > > +MODULE_AUTHOR("Virendra Kakade <virendra.kakade@ni.com>"); > > +MODULE_LICENSE("GPL"); > > So the whole purpose of this driver is to do a version check. > > Seems like over-kill! > > Probably better to do the version check in an inline function stored > in a header file, then call it from each of the children's .probe() > function. A bit of context here, this is based on an in-house driver that we had that does a bunch of other stuff (e.g. it controls a flag on whether the device auto-boots on power being plugged in etc.). These functions will use the regmap. I had advised Virendra to submit a base version and follow up with patches that would add these functions one by one as he figures out how to expose them in a proper way to the kernel. Cheers, Moritz
On Sun, 17 Feb 2019, Moritz Fischer wrote: > On Thu, Feb 14, 2019 at 1:34 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Mon, 11 Feb 2019, Virendra Kakade wrote: > > > > > Signed-off-by: Virendra Kakade <virendra.kakade@ni.com> > > > --- > > > drivers/mfd/Kconfig | 7 +++ > > > drivers/mfd/Makefile | 2 +- > > > drivers/mfd/e31x-pmu.c | 89 ++++++++++++++++++++++++++++++++++++ > > > include/linux/mfd/e31x-pmu.h | 20 ++++++++ > > > 4 files changed, 117 insertions(+), 1 deletion(-) > > > create mode 100644 drivers/mfd/e31x-pmu.c > > > create mode 100644 include/linux/mfd/e31x-pmu.h [...] Try not to quote too many unnecessary lines when replying please. > > So the whole purpose of this driver is to do a version check. > > > > Seems like over-kill! > > > > Probably better to do the version check in an inline function stored > > in a header file, then call it from each of the children's .probe() > > function. > > A bit of context here, this is based on an in-house driver that we had that does > a bunch of other stuff (e.g. it controls a flag on whether the device auto-boots > on power being plugged in etc.). These functions will use the regmap. > > I had advised Virendra to submit a base version and follow up with patches > that would add these functions one by one as he figures out how to expose them > in a proper way to the kernel. Probably best to wait until you have something a little more featureful before attempting to upstream then. I couldn't tell you how many times I've had contributors tell me that they plan to follow up with additional patches and (for their own perfectly decent reasons I'm sure) fail to do so. As it stands, this driver over-kill for what you're trying to achieve and therefore isn't overly suitable for upstreaming at the moment.