* [PATCH 0/2] HWMON compatibility layer for power supplies @ 2019-05-29 7:11 Andrey Smirnov 2019-05-29 7:11 ` [PATCH 1/2] power: supply: Add HWMON compatibility layer Andrey Smirnov 2019-05-29 7:11 ` [PATCH 2/2] power: supply: ucs1002: Add HWMON interface Andrey Smirnov 0 siblings, 2 replies; 6+ messages in thread From: Andrey Smirnov @ 2019-05-29 7:11 UTC (permalink / raw) To: linux-pm Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Fabio Estevam, Guenter Roeck, Sebastian Reichel, linux-kernel Everyone: This small series contains the code I wrote to expose various power supply sensors via HWMON layer in order to be able to access all of the sensors in the system with libsensors. Not sure if this is an something that can be accepted upstream, so I am hoping to get some quick feedback. Thanks, Andrey Smirnov Andrey Smirnov (2): power: supply: Add HWMON compatibility layer power: supply: ucs1002: Add HWMON interface drivers/power/supply/Kconfig | 14 + drivers/power/supply/Makefile | 1 + drivers/power/supply/power_supply_hwmon.c | 329 ++++++++++++++++++++++ drivers/power/supply/ucs1002_power.c | 9 +- include/linux/power_supply.h | 11 + 5 files changed, 363 insertions(+), 1 deletion(-) create mode 100644 drivers/power/supply/power_supply_hwmon.c -- 2.21.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] power: supply: Add HWMON compatibility layer 2019-05-29 7:11 [PATCH 0/2] HWMON compatibility layer for power supplies Andrey Smirnov @ 2019-05-29 7:11 ` Andrey Smirnov 2019-05-29 12:40 ` Guenter Roeck 2019-05-29 7:11 ` [PATCH 2/2] power: supply: ucs1002: Add HWMON interface Andrey Smirnov 1 sibling, 1 reply; 6+ messages in thread From: Andrey Smirnov @ 2019-05-29 7:11 UTC (permalink / raw) To: linux-pm Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Fabio Estevam, Guenter Roeck, Sebastian Reichel, linux-kernel Add code implementing HWMON adapter/compatibility layer to allow expositing various sensors present on power supply devices via HWMON subsystem. This is done in order to allow userspace to use single ABI/library(libsensors) to access/manipulate all of the sensors of the system. Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: Chris Healy <cphealy@gmail.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Fabio Estevam <fabio.estevam@nxp.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Sebastian Reichel <sre@kernel.org> Cc: linux-kernel@vger.kernel.org Cc: linux-pm@vger.kernel.org --- drivers/power/supply/Kconfig | 14 + drivers/power/supply/Makefile | 1 + drivers/power/supply/power_supply_hwmon.c | 329 ++++++++++++++++++++++ include/linux/power_supply.h | 11 + 4 files changed, 355 insertions(+) create mode 100644 drivers/power/supply/power_supply_hwmon.c diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index 26dacdab03cc..1f2252cb95fd 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -14,6 +14,20 @@ config POWER_SUPPLY_DEBUG Say Y here to enable debugging messages for power supply class and drivers. +config POWER_SUPPLY_HWMON + bool + prompt "Expose power supply sensors as hwmon device" + depends on HWMON=y || HWMON=POWER_SUPPLY + default y + help + This options enables API that allows sensors found on a + power supply device (current, voltage, temperature) to be + exposed as a hwmon device. + + Say 'Y' here if you want power supplies to + have hwmon sysfs interface too. + + config PDA_POWER tristate "Generic PDA/phone power driver" depends on !S390 diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile index f208273f9686..c47e88ba16b9 100644 --- a/drivers/power/supply/Makefile +++ b/drivers/power/supply/Makefile @@ -6,6 +6,7 @@ power_supply-$(CONFIG_SYSFS) += power_supply_sysfs.o power_supply-$(CONFIG_LEDS_TRIGGERS) += power_supply_leds.o obj-$(CONFIG_POWER_SUPPLY) += power_supply.o +obj-$(CONFIG_POWER_SUPPLY_HWMON) += power_supply_hwmon.o obj-$(CONFIG_GENERIC_ADC_BATTERY) += generic-adc-battery.o obj-$(CONFIG_PDA_POWER) += pda_power.o diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c new file mode 100644 index 000000000000..dca7f79fae6e --- /dev/null +++ b/drivers/power/supply/power_supply_hwmon.c @@ -0,0 +1,329 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * power_supply_hwmon.c - power supply hwmon support. + */ + +#include <linux/hwmon.h> +#include <linux/slab.h> +#include <linux/err.h> +#include <linux/power_supply.h> + +struct power_supply_hwmon { + struct power_supply *psy; + unsigned long *props; +}; + +static int power_supply_hwmon_in_to_property(u32 attr) +{ + switch (attr) { + case hwmon_in_average: + return POWER_SUPPLY_PROP_VOLTAGE_AVG; + case hwmon_in_min: + return POWER_SUPPLY_PROP_VOLTAGE_MIN; + case hwmon_in_max: + return POWER_SUPPLY_PROP_VOLTAGE_MAX; + case hwmon_in_input: + return POWER_SUPPLY_PROP_VOLTAGE_NOW; + default: + break; + } + + return -EINVAL; +} + +static int power_supply_hwmon_curr_to_property(u32 attr) +{ + switch (attr) { + case hwmon_curr_average: + return POWER_SUPPLY_PROP_CURRENT_AVG; + case hwmon_curr_max: + return POWER_SUPPLY_PROP_CURRENT_MAX; + case hwmon_curr_input: + return POWER_SUPPLY_PROP_CURRENT_NOW; + default: + break; + } + + return -EINVAL; +} + +static int power_supply_hwmon_temp_to_property(u32 attr, int channel) +{ + if (channel) { + switch (attr) { + case hwmon_temp_input: + return POWER_SUPPLY_PROP_TEMP_AMBIENT; + case hwmon_temp_min_alarm: + return POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN; + case hwmon_temp_max_alarm: + return POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX; + default: + break; + } + } else { + switch (attr) { + case hwmon_temp_input: + return POWER_SUPPLY_PROP_TEMP; + case hwmon_temp_max: + return POWER_SUPPLY_PROP_TEMP_MAX; + case hwmon_temp_min: + return POWER_SUPPLY_PROP_TEMP_MIN; + case hwmon_temp_min_alarm: + return POWER_SUPPLY_PROP_TEMP_ALERT_MIN; + case hwmon_temp_max_alarm: + return POWER_SUPPLY_PROP_TEMP_ALERT_MAX; + default: + break; + } + } + + return -EINVAL; +} + +static int +power_supply_hwmon_to_property(enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + case hwmon_in: + return power_supply_hwmon_in_to_property(attr); + case hwmon_curr: + return power_supply_hwmon_curr_to_property(attr); + case hwmon_temp: + return power_supply_hwmon_temp_to_property(attr, channel); + default: + break; + } + + return -EINVAL; +} + +static bool power_supply_hwmon_is_a_label(enum hwmon_sensor_types type, + u32 attr) +{ + return type == hwmon_temp && attr == hwmon_temp_label; +} + +static umode_t power_supply_hwmon_is_visible(const void *data, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + const struct power_supply_hwmon *psyhw = data; + int prop, is_writable; + + if (power_supply_hwmon_is_a_label(type, attr)) + return 0444; + + prop = power_supply_hwmon_to_property(type, attr, channel); + if (prop < 0 || !test_bit(prop, psyhw->props)) + return 0; + + is_writable = power_supply_property_is_writeable(psyhw->psy, prop); + + return (is_writable <= 0) ? 0444 : 0644; +} + +static int power_supply_hwmon_read_string(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, + const char **str) +{ + *str = channel ? "temp" : "temp ambient"; + return 0; +} + +static int +power_supply_hwmon_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + struct power_supply_hwmon *psyhw = dev_get_drvdata(dev); + struct power_supply *psy = psyhw->psy; + union power_supply_propval pspval; + int ret, prop; + + prop = power_supply_hwmon_to_property(type, attr, channel); + if (prop < 0) + return prop; + + ret = power_supply_get_property(psy, prop, &pspval); + if (ret) + return ret; + + switch (type) { + /* + * Both voltage and current is reported in units of + * microvolts/microamps, so we need to adjust it to + * milliamps(volts) + */ + case hwmon_curr: + case hwmon_in: + pspval.intval /= 1000; + break; + /* + * Temp needs to be converted from 1/10 C to milli-C + */ + case hwmon_temp: + pspval.intval *= 100; + break; + default: + break; + } + + *val = pspval.intval; + + return 0; +} + +static int +power_supply_hwmon_write(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long val) +{ + struct power_supply_hwmon *psyhw = dev_get_drvdata(dev); + struct power_supply *psy = psyhw->psy; + union power_supply_propval pspval; + int prop; + + prop = power_supply_hwmon_to_property(type, attr, channel); + if (prop < 0) + return prop; + + pspval.intval = val; + + switch (type) { + /* + * Both voltage and current is reported in units of + * microvolts/microamps, so we need to adjust it to + * milliamps(volts) + */ + case hwmon_curr: + case hwmon_in: + pspval.intval *= 1000; + break; + /* + * Temp needs to be converted from 1/10 C to milli-C + */ + case hwmon_temp: + pspval.intval /= 100; + break; + default: + break; + } + + return power_supply_set_property(psy, prop, &pspval); +} + +static const struct hwmon_ops power_supply_hwmon_ops = { + .is_visible = power_supply_hwmon_is_visible, + .read = power_supply_hwmon_read, + .write = power_supply_hwmon_write, + .read_string = power_supply_hwmon_read_string, +}; + +static const struct hwmon_channel_info *power_supply_hwmon_info[] = { + HWMON_CHANNEL_INFO(temp, + HWMON_T_LABEL | + HWMON_T_INPUT | + HWMON_T_MAX | + HWMON_T_MIN | + HWMON_T_MIN_ALARM | + HWMON_T_MIN_ALARM, + + HWMON_T_LABEL | + HWMON_T_INPUT | + HWMON_T_MIN_ALARM | + HWMON_T_LABEL | + HWMON_T_MAX_ALARM), + + HWMON_CHANNEL_INFO(curr, + HWMON_C_AVERAGE | + HWMON_C_MAX | + HWMON_C_INPUT), + + HWMON_CHANNEL_INFO(in, + HWMON_I_AVERAGE | + HWMON_I_MIN | + HWMON_I_MAX | + HWMON_I_INPUT), + NULL +}; + +static const struct hwmon_chip_info power_supply_hwmon_chip_info = { + .ops = &power_supply_hwmon_ops, + .info = power_supply_hwmon_info, +}; + +static void power_supply_hwmon_bitmap_free(void *data) +{ + bitmap_free(data); +} + +struct device *devm_power_supply_add_hwmon_sysfs(struct power_supply *psy) +{ + const struct power_supply_desc *desc = psy->desc; + struct power_supply_hwmon *psyhw; + struct device *dev = &psy->dev; + struct device *hwmon; + int ret, i; + + if (!devres_open_group(dev, NULL, GFP_KERNEL)) + return ERR_PTR(-ENOMEM); + + psyhw = devm_kzalloc(dev, sizeof(*psyhw), GFP_KERNEL); + if (!psyhw) { + ret = -ENOMEM; + goto error; + } + + psyhw->psy = psy; + psyhw->props = bitmap_zalloc(POWER_SUPPLY_PROP_TIME_TO_FULL_AVG + 1, + GFP_KERNEL); + if (!psyhw->props) { + ret = -ENOMEM; + goto error; + } + + ret = devm_add_action(dev, power_supply_hwmon_bitmap_free, + psyhw->props); + if (ret) + goto error; + + for (i = 0; i < desc->num_properties; i++) { + const enum power_supply_property prop = desc->properties[i]; + + switch (prop) { + case POWER_SUPPLY_PROP_CURRENT_AVG: + case POWER_SUPPLY_PROP_CURRENT_MAX: + case POWER_SUPPLY_PROP_CURRENT_NOW: + case POWER_SUPPLY_PROP_TEMP: + case POWER_SUPPLY_PROP_TEMP_MAX: + case POWER_SUPPLY_PROP_TEMP_MIN: + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN: + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: + case POWER_SUPPLY_PROP_TEMP_AMBIENT: + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN: + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX: + case POWER_SUPPLY_PROP_VOLTAGE_AVG: + case POWER_SUPPLY_PROP_VOLTAGE_MIN: + case POWER_SUPPLY_PROP_VOLTAGE_MAX: + case POWER_SUPPLY_PROP_VOLTAGE_NOW: + set_bit(prop, psyhw->props); + break; + default: + break; + } + } + + hwmon = devm_hwmon_device_register_with_info(dev, psy->desc->name, + psyhw, + &power_supply_hwmon_chip_info, + NULL); + ret = PTR_ERR_OR_ZERO(hwmon); + if (ret) + goto error; + + devres_remove_group(dev, NULL); + return hwmon; +error: + devres_release_group(dev, NULL); + return ERR_PTR(ret); +} diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h index d9c0c094f8a0..839abfa2c640 100644 --- a/include/linux/power_supply.h +++ b/include/linux/power_supply.h @@ -481,4 +481,15 @@ static inline bool power_supply_is_watt_property(enum power_supply_property psp) return 0; } +#ifdef CONFIG_POWER_SUPPLY_HWMON +extern struct device * +devm_power_supply_add_hwmon_sysfs(struct power_supply *psy); +#else +static struct device * +devm_power_supply_add_hwmon_sysfs(struct power_supply *psy) +{ + return 0; +} +#endif + #endif /* __LINUX_POWER_SUPPLY_H__ */ -- 2.21.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] power: supply: Add HWMON compatibility layer 2019-05-29 7:11 ` [PATCH 1/2] power: supply: Add HWMON compatibility layer Andrey Smirnov @ 2019-05-29 12:40 ` Guenter Roeck 2019-05-30 20:08 ` Andrey Smirnov 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2019-05-29 12:40 UTC (permalink / raw) To: Andrey Smirnov, linux-pm Cc: Chris Healy, Lucas Stach, Fabio Estevam, Sebastian Reichel, linux-kernel On 5/29/19 12:11 AM, Andrey Smirnov wrote: > Add code implementing HWMON adapter/compatibility layer to allow > expositing various sensors present on power supply devices via HWMON > subsystem. This is done in order to allow userspace to use single > ABI/library(libsensors) to access/manipulate all of the sensors of the > system. > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > Cc: Chris Healy <cphealy@gmail.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Sebastian Reichel <sre@kernel.org> > Cc: linux-kernel@vger.kernel.org > Cc: linux-pm@vger.kernel.org > --- > drivers/power/supply/Kconfig | 14 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/power_supply_hwmon.c | 329 ++++++++++++++++++++++ > include/linux/power_supply.h | 11 + > 4 files changed, 355 insertions(+) > create mode 100644 drivers/power/supply/power_supply_hwmon.c > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index 26dacdab03cc..1f2252cb95fd 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -14,6 +14,20 @@ config POWER_SUPPLY_DEBUG > Say Y here to enable debugging messages for power supply class > and drivers. > > +config POWER_SUPPLY_HWMON > + bool > + prompt "Expose power supply sensors as hwmon device" > + depends on HWMON=y || HWMON=POWER_SUPPLY > + default y Not sure if you want to enable that by default. > + help > + This options enables API that allows sensors found on a > + power supply device (current, voltage, temperature) to be > + exposed as a hwmon device. > + > + Say 'Y' here if you want power supplies to > + have hwmon sysfs interface too. > + > + > config PDA_POWER > tristate "Generic PDA/phone power driver" > depends on !S390 > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > index f208273f9686..c47e88ba16b9 100644 > --- a/drivers/power/supply/Makefile > +++ b/drivers/power/supply/Makefile > @@ -6,6 +6,7 @@ power_supply-$(CONFIG_SYSFS) += power_supply_sysfs.o > power_supply-$(CONFIG_LEDS_TRIGGERS) += power_supply_leds.o > > obj-$(CONFIG_POWER_SUPPLY) += power_supply.o > +obj-$(CONFIG_POWER_SUPPLY_HWMON) += power_supply_hwmon.o > obj-$(CONFIG_GENERIC_ADC_BATTERY) += generic-adc-battery.o > > obj-$(CONFIG_PDA_POWER) += pda_power.o > diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c > new file mode 100644 > index 000000000000..dca7f79fae6e > --- /dev/null > +++ b/drivers/power/supply/power_supply_hwmon.c > @@ -0,0 +1,329 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * power_supply_hwmon.c - power supply hwmon support. > + */ > + > +#include <linux/hwmon.h> > +#include <linux/slab.h> > +#include <linux/err.h> > +#include <linux/power_supply.h> > + Alphabetic order ? > +struct power_supply_hwmon { > + struct power_supply *psy; > + unsigned long *props; > +}; > + > +static int power_supply_hwmon_in_to_property(u32 attr) > +{ > + switch (attr) { > + case hwmon_in_average: > + return POWER_SUPPLY_PROP_VOLTAGE_AVG; > + case hwmon_in_min: > + return POWER_SUPPLY_PROP_VOLTAGE_MIN; > + case hwmon_in_max: > + return POWER_SUPPLY_PROP_VOLTAGE_MAX; > + case hwmon_in_input: > + return POWER_SUPPLY_PROP_VOLTAGE_NOW; > + default: > + break; > + } > + > + return -EINVAL; > +} > + > +static int power_supply_hwmon_curr_to_property(u32 attr) > +{ > + switch (attr) { > + case hwmon_curr_average: > + return POWER_SUPPLY_PROP_CURRENT_AVG; > + case hwmon_curr_max: > + return POWER_SUPPLY_PROP_CURRENT_MAX; > + case hwmon_curr_input: > + return POWER_SUPPLY_PROP_CURRENT_NOW; > + default: > + break; > + } > + > + return -EINVAL; > +} > + > +static int power_supply_hwmon_temp_to_property(u32 attr, int channel) > +{ > + if (channel) { > + switch (attr) { > + case hwmon_temp_input: > + return POWER_SUPPLY_PROP_TEMP_AMBIENT; > + case hwmon_temp_min_alarm: > + return POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN; > + case hwmon_temp_max_alarm: > + return POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX; > + default: > + break; > + } > + } else { > + switch (attr) { > + case hwmon_temp_input: > + return POWER_SUPPLY_PROP_TEMP; > + case hwmon_temp_max: > + return POWER_SUPPLY_PROP_TEMP_MAX; > + case hwmon_temp_min: > + return POWER_SUPPLY_PROP_TEMP_MIN; > + case hwmon_temp_min_alarm: > + return POWER_SUPPLY_PROP_TEMP_ALERT_MIN; > + case hwmon_temp_max_alarm: > + return POWER_SUPPLY_PROP_TEMP_ALERT_MAX; > + default: > + break; > + } > + } > + > + return -EINVAL; > +} > + > +static int > +power_supply_hwmon_to_property(enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + switch (type) { > + case hwmon_in: > + return power_supply_hwmon_in_to_property(attr); > + case hwmon_curr: > + return power_supply_hwmon_curr_to_property(attr); > + case hwmon_temp: > + return power_supply_hwmon_temp_to_property(attr, channel); > + default: > + break; > + } > + > + return -EINVAL; > +} > + > +static bool power_supply_hwmon_is_a_label(enum hwmon_sensor_types type, > + u32 attr) > +{ > + return type == hwmon_temp && attr == hwmon_temp_label; > +} > + > +static umode_t power_supply_hwmon_is_visible(const void *data, > + enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + const struct power_supply_hwmon *psyhw = data; > + int prop, is_writable; > + > + if (power_supply_hwmon_is_a_label(type, attr)) > + return 0444; > + > + prop = power_supply_hwmon_to_property(type, attr, channel); > + if (prop < 0 || !test_bit(prop, psyhw->props)) > + return 0; > + > + is_writable = power_supply_property_is_writeable(psyhw->psy, prop); > + > + return (is_writable <= 0) ? 0444 : 0644; I a a bit concerned that this can result in writeable voltage/current/temp values (not just for limits), which would be very undesirable. It might be better to add another check to ensure that this does not happen. > +} > + > +static int power_supply_hwmon_read_string(struct device *dev, > + enum hwmon_sensor_types type, > + u32 attr, int channel, > + const char **str) > +{ > + *str = channel ? "temp" : "temp ambient"; > + return 0; > +} > + > +static int > +power_supply_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct power_supply_hwmon *psyhw = dev_get_drvdata(dev); > + struct power_supply *psy = psyhw->psy; > + union power_supply_propval pspval; > + int ret, prop; > + > + prop = power_supply_hwmon_to_property(type, attr, channel); > + if (prop < 0) > + return prop; > + > + ret = power_supply_get_property(psy, prop, &pspval); > + if (ret) > + return ret; > + > + switch (type) { > + /* > + * Both voltage and current is reported in units of > + * microvolts/microamps, so we need to adjust it to > + * milliamps(volts) > + */ > + case hwmon_curr: > + case hwmon_in: > + pspval.intval /= 1000; DIV_ROUND_CLOSEST() ? > + break; > + /* > + * Temp needs to be converted from 1/10 C to milli-C > + */ > + case hwmon_temp: > + pspval.intval *= 100; intval is an int, so theoretically this could result in an overflow. > + break; > + default: Wouldn't this be an error ? Personally I prefer direct value assignments. I don't see value in updating pspval.intval first only to assign it to *val later. > + break; > + } > + > + *val = pspval.intval; > + > + return 0; > +} > + > +static int > +power_supply_hwmon_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + struct power_supply_hwmon *psyhw = dev_get_drvdata(dev); > + struct power_supply *psy = psyhw->psy; > + union power_supply_propval pspval; > + int prop; > + > + prop = power_supply_hwmon_to_property(type, attr, channel); > + if (prop < 0) > + return prop; > + > + pspval.intval = val; > + > + switch (type) { > + /* > + * Both voltage and current is reported in units of > + * microvolts/microamps, so we need to adjust it to > + * milliamps(volts) > + */ > + case hwmon_curr: > + case hwmon_in: > + pspval.intval *= 1000; Range checks ? This can result in an overflow. > + break; > + /* > + * Temp needs to be converted from 1/10 C to milli-C > + */ > + case hwmon_temp: > + pspval.intval /= 100; > + break; > + default: > + break; Wouldn't this be an error ? > + } > + > + return power_supply_set_property(psy, prop, &pspval); > +} > + > +static const struct hwmon_ops power_supply_hwmon_ops = { > + .is_visible = power_supply_hwmon_is_visible, > + .read = power_supply_hwmon_read, > + .write = power_supply_hwmon_write, > + .read_string = power_supply_hwmon_read_string, > +}; > + > +static const struct hwmon_channel_info *power_supply_hwmon_info[] = { > + HWMON_CHANNEL_INFO(temp, > + HWMON_T_LABEL | > + HWMON_T_INPUT | > + HWMON_T_MAX | > + HWMON_T_MIN | > + HWMON_T_MIN_ALARM | > + HWMON_T_MIN_ALARM, > + > + HWMON_T_LABEL | > + HWMON_T_INPUT | > + HWMON_T_MIN_ALARM | > + HWMON_T_LABEL | > + HWMON_T_MAX_ALARM), > + > + HWMON_CHANNEL_INFO(curr, > + HWMON_C_AVERAGE | > + HWMON_C_MAX | > + HWMON_C_INPUT), > + > + HWMON_CHANNEL_INFO(in, > + HWMON_I_AVERAGE | > + HWMON_I_MIN | > + HWMON_I_MAX | > + HWMON_I_INPUT), > + NULL > +}; > + > +static const struct hwmon_chip_info power_supply_hwmon_chip_info = { > + .ops = &power_supply_hwmon_ops, > + .info = power_supply_hwmon_info, > +}; > + > +static void power_supply_hwmon_bitmap_free(void *data) > +{ > + bitmap_free(data); > +} > + > +struct device *devm_power_supply_add_hwmon_sysfs(struct power_supply *psy) > +{ > + const struct power_supply_desc *desc = psy->desc; > + struct power_supply_hwmon *psyhw; > + struct device *dev = &psy->dev; > + struct device *hwmon; > + int ret, i; > + > + if (!devres_open_group(dev, NULL, GFP_KERNEL)) > + return ERR_PTR(-ENOMEM); > + > + psyhw = devm_kzalloc(dev, sizeof(*psyhw), GFP_KERNEL); > + if (!psyhw) { > + ret = -ENOMEM; > + goto error; > + } > + > + psyhw->psy = psy; > + psyhw->props = bitmap_zalloc(POWER_SUPPLY_PROP_TIME_TO_FULL_AVG + 1, > + GFP_KERNEL); > + if (!psyhw->props) { > + ret = -ENOMEM; > + goto error; > + } > + > + ret = devm_add_action(dev, power_supply_hwmon_bitmap_free, > + psyhw->props); > + if (ret) > + goto error; > + > + for (i = 0; i < desc->num_properties; i++) { > + const enum power_supply_property prop = desc->properties[i]; > + > + switch (prop) { > + case POWER_SUPPLY_PROP_CURRENT_AVG: > + case POWER_SUPPLY_PROP_CURRENT_MAX: > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + case POWER_SUPPLY_PROP_TEMP: > + case POWER_SUPPLY_PROP_TEMP_MAX: > + case POWER_SUPPLY_PROP_TEMP_MIN: > + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN: > + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: > + case POWER_SUPPLY_PROP_TEMP_AMBIENT: > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN: > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX: > + case POWER_SUPPLY_PROP_VOLTAGE_AVG: > + case POWER_SUPPLY_PROP_VOLTAGE_MIN: > + case POWER_SUPPLY_PROP_VOLTAGE_MAX: > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + set_bit(prop, psyhw->props); > + break; > + default: > + break; > + } > + } > + > + hwmon = devm_hwmon_device_register_with_info(dev, psy->desc->name, > + psyhw, > + &power_supply_hwmon_chip_info, > + NULL); > + ret = PTR_ERR_OR_ZERO(hwmon); > + if (ret) > + goto error; > + > + devres_remove_group(dev, NULL); Why devres_remove_group() and not devres_close_group() ? > + return hwmon; Why return a pointer to the hwmon device and not just an error code ? > +error: > + devres_release_group(dev, NULL); > + return ERR_PTR(ret); > +} > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index d9c0c094f8a0..839abfa2c640 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -481,4 +481,15 @@ static inline bool power_supply_is_watt_property(enum power_supply_property psp) > return 0; > } > > +#ifdef CONFIG_POWER_SUPPLY_HWMON > +extern struct device * > +devm_power_supply_add_hwmon_sysfs(struct power_supply *psy); > +#else > +static struct device * > +devm_power_supply_add_hwmon_sysfs(struct power_supply *psy) > +{ > + return 0; You might consider making the return code an int. Otherwise this gets difficult for the caller, who would have to check for IS_ERR() and NULL (if the device pointer is supposed to be used for anything). > +} > +#endif > + > #endif /* __LINUX_POWER_SUPPLY_H__ */ > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] power: supply: Add HWMON compatibility layer 2019-05-29 12:40 ` Guenter Roeck @ 2019-05-30 20:08 ` Andrey Smirnov 2019-05-30 20:32 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Andrey Smirnov @ 2019-05-30 20:08 UTC (permalink / raw) To: Guenter Roeck Cc: Linux PM list, Chris Healy, Lucas Stach, Fabio Estevam, Sebastian Reichel, linux-kernel On Wed, May 29, 2019 at 5:40 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 5/29/19 12:11 AM, Andrey Smirnov wrote: > > Add code implementing HWMON adapter/compatibility layer to allow > > expositing various sensors present on power supply devices via HWMON > > subsystem. This is done in order to allow userspace to use single > > ABI/library(libsensors) to access/manipulate all of the sensors of the > > system. > > > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > Cc: Chris Healy <cphealy@gmail.com> > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Cc: Fabio Estevam <fabio.estevam@nxp.com> > > Cc: Guenter Roeck <linux@roeck-us.net> > > Cc: Sebastian Reichel <sre@kernel.org> > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-pm@vger.kernel.org > > --- > > drivers/power/supply/Kconfig | 14 + > > drivers/power/supply/Makefile | 1 + > > drivers/power/supply/power_supply_hwmon.c | 329 ++++++++++++++++++++++ > > include/linux/power_supply.h | 11 + > > 4 files changed, 355 insertions(+) > > create mode 100644 drivers/power/supply/power_supply_hwmon.c > > > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > > index 26dacdab03cc..1f2252cb95fd 100644 > > --- a/drivers/power/supply/Kconfig > > +++ b/drivers/power/supply/Kconfig > > @@ -14,6 +14,20 @@ config POWER_SUPPLY_DEBUG > > Say Y here to enable debugging messages for power supply class > > and drivers. > > > > +config POWER_SUPPLY_HWMON > > + bool > > + prompt "Expose power supply sensors as hwmon device" > > + depends on HWMON=y || HWMON=POWER_SUPPLY > > + default y > > Not sure if you want to enable that by default. > That's what THERMAL_HWMON does which seems pretty analogous to this code. > > + help > > + This options enables API that allows sensors found on a > > + power supply device (current, voltage, temperature) to be > > + exposed as a hwmon device. > > + > > + Say 'Y' here if you want power supplies to > > + have hwmon sysfs interface too. > > + > > + > > config PDA_POWER > > tristate "Generic PDA/phone power driver" > > depends on !S390 > > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > > index f208273f9686..c47e88ba16b9 100644 > > --- a/drivers/power/supply/Makefile > > +++ b/drivers/power/supply/Makefile > > @@ -6,6 +6,7 @@ power_supply-$(CONFIG_SYSFS) += power_supply_sysfs.o > > power_supply-$(CONFIG_LEDS_TRIGGERS) += power_supply_leds.o > > > > obj-$(CONFIG_POWER_SUPPLY) += power_supply.o > > +obj-$(CONFIG_POWER_SUPPLY_HWMON) += power_supply_hwmon.o > > obj-$(CONFIG_GENERIC_ADC_BATTERY) += generic-adc-battery.o > > > > obj-$(CONFIG_PDA_POWER) += pda_power.o > > diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c > > new file mode 100644 > > index 000000000000..dca7f79fae6e > > --- /dev/null > > +++ b/drivers/power/supply/power_supply_hwmon.c > > @@ -0,0 +1,329 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * power_supply_hwmon.c - power supply hwmon support. > > + */ > > + > > +#include <linux/hwmon.h> > > +#include <linux/slab.h> > > +#include <linux/err.h> > > +#include <linux/power_supply.h> > > + > Alphabetic order ? > Sure, will do in v2. > > +struct power_supply_hwmon { > > + struct power_supply *psy; > > + unsigned long *props; > > +}; > > + > > +static int power_supply_hwmon_in_to_property(u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_in_average: > > + return POWER_SUPPLY_PROP_VOLTAGE_AVG; > > + case hwmon_in_min: > > + return POWER_SUPPLY_PROP_VOLTAGE_MIN; > > + case hwmon_in_max: > > + return POWER_SUPPLY_PROP_VOLTAGE_MAX; > > + case hwmon_in_input: > > + return POWER_SUPPLY_PROP_VOLTAGE_NOW; > > + default: > > + break; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int power_supply_hwmon_curr_to_property(u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_curr_average: > > + return POWER_SUPPLY_PROP_CURRENT_AVG; > > + case hwmon_curr_max: > > + return POWER_SUPPLY_PROP_CURRENT_MAX; > > + case hwmon_curr_input: > > + return POWER_SUPPLY_PROP_CURRENT_NOW; > > + default: > > + break; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int power_supply_hwmon_temp_to_property(u32 attr, int channel) > > +{ > > + if (channel) { > > + switch (attr) { > > + case hwmon_temp_input: > > + return POWER_SUPPLY_PROP_TEMP_AMBIENT; > > + case hwmon_temp_min_alarm: > > + return POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN; > > + case hwmon_temp_max_alarm: > > + return POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX; > > + default: > > + break; > > + } > > + } else { > > + switch (attr) { > > + case hwmon_temp_input: > > + return POWER_SUPPLY_PROP_TEMP; > > + case hwmon_temp_max: > > + return POWER_SUPPLY_PROP_TEMP_MAX; > > + case hwmon_temp_min: > > + return POWER_SUPPLY_PROP_TEMP_MIN; > > + case hwmon_temp_min_alarm: > > + return POWER_SUPPLY_PROP_TEMP_ALERT_MIN; > > + case hwmon_temp_max_alarm: > > + return POWER_SUPPLY_PROP_TEMP_ALERT_MAX; > > + default: > > + break; > > + } > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int > > +power_supply_hwmon_to_property(enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + switch (type) { > > + case hwmon_in: > > + return power_supply_hwmon_in_to_property(attr); > > + case hwmon_curr: > > + return power_supply_hwmon_curr_to_property(attr); > > + case hwmon_temp: > > + return power_supply_hwmon_temp_to_property(attr, channel); > > + default: > > + break; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static bool power_supply_hwmon_is_a_label(enum hwmon_sensor_types type, > > + u32 attr) > > +{ > > + return type == hwmon_temp && attr == hwmon_temp_label; > > +} > > + > > +static umode_t power_supply_hwmon_is_visible(const void *data, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + const struct power_supply_hwmon *psyhw = data; > > + int prop, is_writable; > > + > > + if (power_supply_hwmon_is_a_label(type, attr)) > > + return 0444; > > + > > + prop = power_supply_hwmon_to_property(type, attr, channel); > > + if (prop < 0 || !test_bit(prop, psyhw->props)) > > + return 0; > > + > > + is_writable = power_supply_property_is_writeable(psyhw->psy, prop); > > + > > + return (is_writable <= 0) ? 0444 : 0644; > > I a a bit concerned that this can result in writeable voltage/current/temp > values (not just for limits), which would be very undesirable. It might be > better to add another check to ensure that this does not happen. > OK, will do. > > +} > > + > > +static int power_supply_hwmon_read_string(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, > > + const char **str) > > +{ > > + *str = channel ? "temp" : "temp ambient"; > > + return 0; > > +} > > + > > +static int > > +power_supply_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + struct power_supply_hwmon *psyhw = dev_get_drvdata(dev); > > + struct power_supply *psy = psyhw->psy; > > + union power_supply_propval pspval; > > + int ret, prop; > > + > > + prop = power_supply_hwmon_to_property(type, attr, channel); > > + if (prop < 0) > > + return prop; > > + > > + ret = power_supply_get_property(psy, prop, &pspval); > > + if (ret) > > + return ret; > > + > > + switch (type) { > > + /* > > + * Both voltage and current is reported in units of > > + * microvolts/microamps, so we need to adjust it to > > + * milliamps(volts) > > + */ > > + case hwmon_curr: > > + case hwmon_in: > > + pspval.intval /= 1000; > > DIV_ROUND_CLOSEST() ? > Yeah, good idea, will do. > > + break; > > + /* > > + * Temp needs to be converted from 1/10 C to milli-C > > + */ > > + case hwmon_temp: > > + pspval.intval *= 100; > > intval is an int, so theoretically this could result in an overflow. > Sure, will change the code to use check_mul_overflow() > > > + break; > > + default: > > Wouldn't this be an error ? > Shouldn't happen, but adding a error path here won't hurt. > Personally I prefer direct value assignments. I don't see value in > updating pspval.intval first only to assign it to *val later. > This won't work once I convert the code to use check_mul_overflow() since it expects its argument to have the same type and "*val" is long. > > + break; > > + } > > + > > + *val = pspval.intval; > > + > > + return 0; > > +} > > + > > +static int > > +power_supply_hwmon_write(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long val) > > +{ > > + struct power_supply_hwmon *psyhw = dev_get_drvdata(dev); > > + struct power_supply *psy = psyhw->psy; > > + union power_supply_propval pspval; > > + int prop; > > + > > + prop = power_supply_hwmon_to_property(type, attr, channel); > > + if (prop < 0) > > + return prop; > > + > > + pspval.intval = val; > > + > > + switch (type) { > > + /* > > + * Both voltage and current is reported in units of > > + * microvolts/microamps, so we need to adjust it to > > + * milliamps(volts) > > + */ > > + case hwmon_curr: > > + case hwmon_in: > > + pspval.intval *= 1000; > > Range checks ? This can result in an overflow. > Will add check_mul_overflow() here as well. > > + break; > > + /* > > + * Temp needs to be converted from 1/10 C to milli-C > > + */ > > + case hwmon_temp: > > + pspval.intval /= 100; > > + break; > > + default: > > + break; > > Wouldn't this be an error ? > Will add an error path in v2. > > + } > > + > > + return power_supply_set_property(psy, prop, &pspval); > > +} > > + > > +static const struct hwmon_ops power_supply_hwmon_ops = { > > + .is_visible = power_supply_hwmon_is_visible, > > + .read = power_supply_hwmon_read, > > + .write = power_supply_hwmon_write, > > + .read_string = power_supply_hwmon_read_string, > > +}; > > + > > +static const struct hwmon_channel_info *power_supply_hwmon_info[] = { > > + HWMON_CHANNEL_INFO(temp, > > + HWMON_T_LABEL | > > + HWMON_T_INPUT | > > + HWMON_T_MAX | > > + HWMON_T_MIN | > > + HWMON_T_MIN_ALARM | > > + HWMON_T_MIN_ALARM, > > + > > + HWMON_T_LABEL | > > + HWMON_T_INPUT | > > + HWMON_T_MIN_ALARM | > > + HWMON_T_LABEL | > > + HWMON_T_MAX_ALARM), > > + > > + HWMON_CHANNEL_INFO(curr, > > + HWMON_C_AVERAGE | > > + HWMON_C_MAX | > > + HWMON_C_INPUT), > > + > > + HWMON_CHANNEL_INFO(in, > > + HWMON_I_AVERAGE | > > + HWMON_I_MIN | > > + HWMON_I_MAX | > > + HWMON_I_INPUT), > > + NULL > > +}; > > + > > +static const struct hwmon_chip_info power_supply_hwmon_chip_info = { > > + .ops = &power_supply_hwmon_ops, > > + .info = power_supply_hwmon_info, > > +}; > > + > > +static void power_supply_hwmon_bitmap_free(void *data) > > +{ > > + bitmap_free(data); > > +} > > + > > +struct device *devm_power_supply_add_hwmon_sysfs(struct power_supply *psy) > > +{ > > + const struct power_supply_desc *desc = psy->desc; > > + struct power_supply_hwmon *psyhw; > > + struct device *dev = &psy->dev; > > + struct device *hwmon; > > + int ret, i; > > + > > + if (!devres_open_group(dev, NULL, GFP_KERNEL)) > > + return ERR_PTR(-ENOMEM); > > + > > + psyhw = devm_kzalloc(dev, sizeof(*psyhw), GFP_KERNEL); > > + if (!psyhw) { > > + ret = -ENOMEM; > > + goto error; > > + } > > + > > + psyhw->psy = psy; > > + psyhw->props = bitmap_zalloc(POWER_SUPPLY_PROP_TIME_TO_FULL_AVG + 1, > > + GFP_KERNEL); > > + if (!psyhw->props) { > > + ret = -ENOMEM; > > + goto error; > > + } > > + > > + ret = devm_add_action(dev, power_supply_hwmon_bitmap_free, > > + psyhw->props); > > + if (ret) > > + goto error; > > + > > + for (i = 0; i < desc->num_properties; i++) { > > + const enum power_supply_property prop = desc->properties[i]; > > + > > + switch (prop) { > > + case POWER_SUPPLY_PROP_CURRENT_AVG: > > + case POWER_SUPPLY_PROP_CURRENT_MAX: > > + case POWER_SUPPLY_PROP_CURRENT_NOW: > > + case POWER_SUPPLY_PROP_TEMP: > > + case POWER_SUPPLY_PROP_TEMP_MAX: > > + case POWER_SUPPLY_PROP_TEMP_MIN: > > + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN: > > + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: > > + case POWER_SUPPLY_PROP_TEMP_AMBIENT: > > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN: > > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX: > > + case POWER_SUPPLY_PROP_VOLTAGE_AVG: > > + case POWER_SUPPLY_PROP_VOLTAGE_MIN: > > + case POWER_SUPPLY_PROP_VOLTAGE_MAX: > > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > > + set_bit(prop, psyhw->props); > > + break; > > + default: > > + break; > > + } > > + } > > + > > + hwmon = devm_hwmon_device_register_with_info(dev, psy->desc->name, > > + psyhw, > > + &power_supply_hwmon_chip_info, > > + NULL); > > + ret = PTR_ERR_OR_ZERO(hwmon); > > + if (ret) > > + goto error; > > + > > + devres_remove_group(dev, NULL); > > Why devres_remove_group() and not devres_close_group() ? > That's id-less group that'll never be used again, closing it and keeping it around isn't really particularly useful to me. > > + return hwmon; > > Why return a pointer to the hwmon device and not just an error code ? > Just to expose created hwmon device. > > +error: > > + devres_release_group(dev, NULL); > > + return ERR_PTR(ret); > > +} > > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > > index d9c0c094f8a0..839abfa2c640 100644 > > --- a/include/linux/power_supply.h > > +++ b/include/linux/power_supply.h > > @@ -481,4 +481,15 @@ static inline bool power_supply_is_watt_property(enum power_supply_property psp) > > return 0; > > } > > > > +#ifdef CONFIG_POWER_SUPPLY_HWMON > > +extern struct device * > > +devm_power_supply_add_hwmon_sysfs(struct power_supply *psy); > > +#else > > +static struct device * > > +devm_power_supply_add_hwmon_sysfs(struct power_supply *psy) > > +{ > > + return 0; > > You might consider making the return code an int. Otherwise this gets difficult > for the caller, who would have to check for IS_ERR() and NULL (if the device > pointer is supposed to be used for anything). > Sure, if removing access to created hwmon device is OK, I am more than happy to convert this to return an int. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] power: supply: Add HWMON compatibility layer 2019-05-30 20:08 ` Andrey Smirnov @ 2019-05-30 20:32 ` Guenter Roeck 0 siblings, 0 replies; 6+ messages in thread From: Guenter Roeck @ 2019-05-30 20:32 UTC (permalink / raw) To: Andrey Smirnov Cc: Linux PM list, Chris Healy, Lucas Stach, Fabio Estevam, Sebastian Reichel, linux-kernel On Thu, May 30, 2019 at 01:08:35PM -0700, Andrey Smirnov wrote: > On Wed, May 29, 2019 at 5:40 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On 5/29/19 12:11 AM, Andrey Smirnov wrote: > > > Add code implementing HWMON adapter/compatibility layer to allow > > > expositing various sensors present on power supply devices via HWMON > > > subsystem. This is done in order to allow userspace to use single > > > ABI/library(libsensors) to access/manipulate all of the sensors of the > > > system. > > > > > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > > Cc: Chris Healy <cphealy@gmail.com> > > > Cc: Lucas Stach <l.stach@pengutronix.de> > > > Cc: Fabio Estevam <fabio.estevam@nxp.com> > > > Cc: Guenter Roeck <linux@roeck-us.net> > > > Cc: Sebastian Reichel <sre@kernel.org> > > > Cc: linux-kernel@vger.kernel.org > > > Cc: linux-pm@vger.kernel.org > > > --- > > > drivers/power/supply/Kconfig | 14 + > > > drivers/power/supply/Makefile | 1 + > > > drivers/power/supply/power_supply_hwmon.c | 329 ++++++++++++++++++++++ > > > include/linux/power_supply.h | 11 + > > > 4 files changed, 355 insertions(+) > > > create mode 100644 drivers/power/supply/power_supply_hwmon.c > > > > > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > > > index 26dacdab03cc..1f2252cb95fd 100644 > > > --- a/drivers/power/supply/Kconfig > > > +++ b/drivers/power/supply/Kconfig > > > @@ -14,6 +14,20 @@ config POWER_SUPPLY_DEBUG > > > Say Y here to enable debugging messages for power supply class > > > and drivers. > > > > > > +config POWER_SUPPLY_HWMON > > > + bool > > > + prompt "Expose power supply sensors as hwmon device" > > > + depends on HWMON=y || HWMON=POWER_SUPPLY > > > + default y > > > > Not sure if you want to enable that by default. > > > > That's what THERMAL_HWMON does which seems pretty analogous to this code. > maintainer call to make. > > > + help > > > + This options enables API that allows sensors found on a > > > + power supply device (current, voltage, temperature) to be > > > + exposed as a hwmon device. > > > + > > > + Say 'Y' here if you want power supplies to > > > + have hwmon sysfs interface too. > > > + > > > + > > > config PDA_POWER > > > tristate "Generic PDA/phone power driver" > > > depends on !S390 > > > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > > > index f208273f9686..c47e88ba16b9 100644 > > > --- a/drivers/power/supply/Makefile > > > +++ b/drivers/power/supply/Makefile > > > @@ -6,6 +6,7 @@ power_supply-$(CONFIG_SYSFS) += power_supply_sysfs.o > > > power_supply-$(CONFIG_LEDS_TRIGGERS) += power_supply_leds.o > > > > > > obj-$(CONFIG_POWER_SUPPLY) += power_supply.o > > > +obj-$(CONFIG_POWER_SUPPLY_HWMON) += power_supply_hwmon.o > > > obj-$(CONFIG_GENERIC_ADC_BATTERY) += generic-adc-battery.o > > > > > > obj-$(CONFIG_PDA_POWER) += pda_power.o > > > diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c > > > new file mode 100644 > > > index 000000000000..dca7f79fae6e > > > --- /dev/null > > > +++ b/drivers/power/supply/power_supply_hwmon.c > > > @@ -0,0 +1,329 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * power_supply_hwmon.c - power supply hwmon support. > > > + */ > > > + > > > +#include <linux/hwmon.h> > > > +#include <linux/slab.h> > > > +#include <linux/err.h> > > > +#include <linux/power_supply.h> > > > + > > Alphabetic order ? > > > > Sure, will do in v2. > > > > +struct power_supply_hwmon { > > > + struct power_supply *psy; > > > + unsigned long *props; > > > +}; > > > + > > > +static int power_supply_hwmon_in_to_property(u32 attr) > > > +{ > > > + switch (attr) { > > > + case hwmon_in_average: > > > + return POWER_SUPPLY_PROP_VOLTAGE_AVG; > > > + case hwmon_in_min: > > > + return POWER_SUPPLY_PROP_VOLTAGE_MIN; > > > + case hwmon_in_max: > > > + return POWER_SUPPLY_PROP_VOLTAGE_MAX; > > > + case hwmon_in_input: > > > + return POWER_SUPPLY_PROP_VOLTAGE_NOW; > > > + default: > > > + break; > > > + } > > > + > > > + return -EINVAL; > > > +} > > > + > > > +static int power_supply_hwmon_curr_to_property(u32 attr) > > > +{ > > > + switch (attr) { > > > + case hwmon_curr_average: > > > + return POWER_SUPPLY_PROP_CURRENT_AVG; > > > + case hwmon_curr_max: > > > + return POWER_SUPPLY_PROP_CURRENT_MAX; > > > + case hwmon_curr_input: > > > + return POWER_SUPPLY_PROP_CURRENT_NOW; > > > + default: > > > + break; > > > + } > > > + > > > + return -EINVAL; > > > +} > > > + > > > +static int power_supply_hwmon_temp_to_property(u32 attr, int channel) > > > +{ > > > + if (channel) { > > > + switch (attr) { > > > + case hwmon_temp_input: > > > + return POWER_SUPPLY_PROP_TEMP_AMBIENT; > > > + case hwmon_temp_min_alarm: > > > + return POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN; > > > + case hwmon_temp_max_alarm: > > > + return POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX; > > > + default: > > > + break; > > > + } > > > + } else { > > > + switch (attr) { > > > + case hwmon_temp_input: > > > + return POWER_SUPPLY_PROP_TEMP; > > > + case hwmon_temp_max: > > > + return POWER_SUPPLY_PROP_TEMP_MAX; > > > + case hwmon_temp_min: > > > + return POWER_SUPPLY_PROP_TEMP_MIN; > > > + case hwmon_temp_min_alarm: > > > + return POWER_SUPPLY_PROP_TEMP_ALERT_MIN; > > > + case hwmon_temp_max_alarm: > > > + return POWER_SUPPLY_PROP_TEMP_ALERT_MAX; > > > + default: > > > + break; > > > + } > > > + } > > > + > > > + return -EINVAL; > > > +} > > > + > > > +static int > > > +power_supply_hwmon_to_property(enum hwmon_sensor_types type, > > > + u32 attr, int channel) > > > +{ > > > + switch (type) { > > > + case hwmon_in: > > > + return power_supply_hwmon_in_to_property(attr); > > > + case hwmon_curr: > > > + return power_supply_hwmon_curr_to_property(attr); > > > + case hwmon_temp: > > > + return power_supply_hwmon_temp_to_property(attr, channel); > > > + default: > > > + break; > > > + } > > > + > > > + return -EINVAL; > > > +} > > > + > > > +static bool power_supply_hwmon_is_a_label(enum hwmon_sensor_types type, > > > + u32 attr) > > > +{ > > > + return type == hwmon_temp && attr == hwmon_temp_label; > > > +} > > > + > > > +static umode_t power_supply_hwmon_is_visible(const void *data, > > > + enum hwmon_sensor_types type, > > > + u32 attr, int channel) > > > +{ > > > + const struct power_supply_hwmon *psyhw = data; > > > + int prop, is_writable; > > > + > > > + if (power_supply_hwmon_is_a_label(type, attr)) > > > + return 0444; > > > + > > > + prop = power_supply_hwmon_to_property(type, attr, channel); > > > + if (prop < 0 || !test_bit(prop, psyhw->props)) > > > + return 0; > > > + > > > + is_writable = power_supply_property_is_writeable(psyhw->psy, prop); > > > + > > > + return (is_writable <= 0) ? 0444 : 0644; > > > > I a a bit concerned that this can result in writeable voltage/current/temp > > values (not just for limits), which would be very undesirable. It might be > > better to add another check to ensure that this does not happen. > > > > OK, will do. > > > > +} > > > + > > > +static int power_supply_hwmon_read_string(struct device *dev, > > > + enum hwmon_sensor_types type, > > > + u32 attr, int channel, > > > + const char **str) > > > +{ > > > + *str = channel ? "temp" : "temp ambient"; > > > + return 0; > > > +} > > > + > > > +static int > > > +power_supply_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > > > + u32 attr, int channel, long *val) > > > +{ > > > + struct power_supply_hwmon *psyhw = dev_get_drvdata(dev); > > > + struct power_supply *psy = psyhw->psy; > > > + union power_supply_propval pspval; > > > + int ret, prop; > > > + > > > + prop = power_supply_hwmon_to_property(type, attr, channel); > > > + if (prop < 0) > > > + return prop; > > > + > > > + ret = power_supply_get_property(psy, prop, &pspval); > > > + if (ret) > > > + return ret; > > > + > > > + switch (type) { > > > + /* > > > + * Both voltage and current is reported in units of > > > + * microvolts/microamps, so we need to adjust it to > > > + * milliamps(volts) > > > + */ > > > + case hwmon_curr: > > > + case hwmon_in: > > > + pspval.intval /= 1000; > > > > DIV_ROUND_CLOSEST() ? > > > > Yeah, good idea, will do. > > > > + break; > > > + /* > > > + * Temp needs to be converted from 1/10 C to milli-C > > > + */ > > > + case hwmon_temp: > > > + pspval.intval *= 100; > > > > intval is an int, so theoretically this could result in an overflow. > > > > Sure, will change the code to use check_mul_overflow() > > > > > > + break; > > > + default: > > > > Wouldn't this be an error ? > > > > Shouldn't happen, but adding a error path here won't hurt. > > > Personally I prefer direct value assignments. I don't see value in > > updating pspval.intval first only to assign it to *val later. > > > > This won't work once I convert the code to use check_mul_overflow() > since it expects its argument to have the same type and "*val" is > long. > > > > + break; > > > + } > > > + > > > + *val = pspval.intval; > > > + > > > + return 0; > > > +} > > > + > > > +static int > > > +power_supply_hwmon_write(struct device *dev, enum hwmon_sensor_types type, > > > + u32 attr, int channel, long val) > > > +{ > > > + struct power_supply_hwmon *psyhw = dev_get_drvdata(dev); > > > + struct power_supply *psy = psyhw->psy; > > > + union power_supply_propval pspval; > > > + int prop; > > > + > > > + prop = power_supply_hwmon_to_property(type, attr, channel); > > > + if (prop < 0) > > > + return prop; > > > + > > > + pspval.intval = val; > > > + > > > + switch (type) { > > > + /* > > > + * Both voltage and current is reported in units of > > > + * microvolts/microamps, so we need to adjust it to > > > + * milliamps(volts) > > > + */ > > > + case hwmon_curr: > > > + case hwmon_in: > > > + pspval.intval *= 1000; > > > > Range checks ? This can result in an overflow. > > > > Will add check_mul_overflow() here as well. > > > > + break; > > > + /* > > > + * Temp needs to be converted from 1/10 C to milli-C > > > + */ > > > + case hwmon_temp: > > > + pspval.intval /= 100; > > > + break; > > > + default: > > > + break; > > > > Wouldn't this be an error ? > > > > Will add an error path in v2. > > > > + } > > > + > > > + return power_supply_set_property(psy, prop, &pspval); > > > +} > > > + > > > +static const struct hwmon_ops power_supply_hwmon_ops = { > > > + .is_visible = power_supply_hwmon_is_visible, > > > + .read = power_supply_hwmon_read, > > > + .write = power_supply_hwmon_write, > > > + .read_string = power_supply_hwmon_read_string, > > > +}; > > > + > > > +static const struct hwmon_channel_info *power_supply_hwmon_info[] = { > > > + HWMON_CHANNEL_INFO(temp, > > > + HWMON_T_LABEL | > > > + HWMON_T_INPUT | > > > + HWMON_T_MAX | > > > + HWMON_T_MIN | > > > + HWMON_T_MIN_ALARM | > > > + HWMON_T_MIN_ALARM, > > > + > > > + HWMON_T_LABEL | > > > + HWMON_T_INPUT | > > > + HWMON_T_MIN_ALARM | > > > + HWMON_T_LABEL | > > > + HWMON_T_MAX_ALARM), > > > + > > > + HWMON_CHANNEL_INFO(curr, > > > + HWMON_C_AVERAGE | > > > + HWMON_C_MAX | > > > + HWMON_C_INPUT), > > > + > > > + HWMON_CHANNEL_INFO(in, > > > + HWMON_I_AVERAGE | > > > + HWMON_I_MIN | > > > + HWMON_I_MAX | > > > + HWMON_I_INPUT), > > > + NULL > > > +}; > > > + > > > +static const struct hwmon_chip_info power_supply_hwmon_chip_info = { > > > + .ops = &power_supply_hwmon_ops, > > > + .info = power_supply_hwmon_info, > > > +}; > > > + > > > +static void power_supply_hwmon_bitmap_free(void *data) > > > +{ > > > + bitmap_free(data); > > > +} > > > + > > > +struct device *devm_power_supply_add_hwmon_sysfs(struct power_supply *psy) > > > +{ > > > + const struct power_supply_desc *desc = psy->desc; > > > + struct power_supply_hwmon *psyhw; > > > + struct device *dev = &psy->dev; > > > + struct device *hwmon; > > > + int ret, i; > > > + > > > + if (!devres_open_group(dev, NULL, GFP_KERNEL)) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + psyhw = devm_kzalloc(dev, sizeof(*psyhw), GFP_KERNEL); > > > + if (!psyhw) { > > > + ret = -ENOMEM; > > > + goto error; > > > + } > > > + > > > + psyhw->psy = psy; > > > + psyhw->props = bitmap_zalloc(POWER_SUPPLY_PROP_TIME_TO_FULL_AVG + 1, > > > + GFP_KERNEL); > > > + if (!psyhw->props) { > > > + ret = -ENOMEM; > > > + goto error; > > > + } > > > + > > > + ret = devm_add_action(dev, power_supply_hwmon_bitmap_free, > > > + psyhw->props); > > > + if (ret) > > > + goto error; > > > + > > > + for (i = 0; i < desc->num_properties; i++) { > > > + const enum power_supply_property prop = desc->properties[i]; > > > + > > > + switch (prop) { > > > + case POWER_SUPPLY_PROP_CURRENT_AVG: > > > + case POWER_SUPPLY_PROP_CURRENT_MAX: > > > + case POWER_SUPPLY_PROP_CURRENT_NOW: > > > + case POWER_SUPPLY_PROP_TEMP: > > > + case POWER_SUPPLY_PROP_TEMP_MAX: > > > + case POWER_SUPPLY_PROP_TEMP_MIN: > > > + case POWER_SUPPLY_PROP_TEMP_ALERT_MIN: > > > + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: > > > + case POWER_SUPPLY_PROP_TEMP_AMBIENT: > > > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN: > > > + case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX: > > > + case POWER_SUPPLY_PROP_VOLTAGE_AVG: > > > + case POWER_SUPPLY_PROP_VOLTAGE_MIN: > > > + case POWER_SUPPLY_PROP_VOLTAGE_MAX: > > > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > > > + set_bit(prop, psyhw->props); > > > + break; > > > + default: > > > + break; > > > + } > > > + } > > > + > > > + hwmon = devm_hwmon_device_register_with_info(dev, psy->desc->name, > > > + psyhw, > > > + &power_supply_hwmon_chip_info, > > > + NULL); > > > + ret = PTR_ERR_OR_ZERO(hwmon); > > > + if (ret) > > > + goto error; > > > + > > > + devres_remove_group(dev, NULL); > > > > Why devres_remove_group() and not devres_close_group() ? > > > > That's id-less group that'll never be used again, closing it and > keeping it around isn't really particularly useful to me. > Ok, fine with me if it works. I have no idea what the semantics of devres groups is, much less what the difference between devres_remove_group() and devres_close_group() is. > > > + return hwmon; > > > > Why return a pointer to the hwmon device and not just an error code ? > > > > Just to expose created hwmon device. > Sure, but what for ? > > > +error: > > > + devres_release_group(dev, NULL); > > > + return ERR_PTR(ret); > > > +} > > > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > > > index d9c0c094f8a0..839abfa2c640 100644 > > > --- a/include/linux/power_supply.h > > > +++ b/include/linux/power_supply.h > > > @@ -481,4 +481,15 @@ static inline bool power_supply_is_watt_property(enum power_supply_property psp) > > > return 0; > > > } > > > > > > +#ifdef CONFIG_POWER_SUPPLY_HWMON > > > +extern struct device * > > > +devm_power_supply_add_hwmon_sysfs(struct power_supply *psy); > > > +#else > > > +static struct device * > > > +devm_power_supply_add_hwmon_sysfs(struct power_supply *psy) > > > +{ > > > + return 0; > > > > You might consider making the return code an int. Otherwise this gets difficult > > for the caller, who would have to check for IS_ERR() and NULL (if the device > > pointer is supposed to be used for anything). > > > > Sure, if removing access to created hwmon device is OK, I am more than > happy to convert this to return an int. > Again, the question is the opposite: You should not return a pointer unless you have a use case. "to expose created hwmon device" is not a use case. Thanks, Guenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] power: supply: ucs1002: Add HWMON interface 2019-05-29 7:11 [PATCH 0/2] HWMON compatibility layer for power supplies Andrey Smirnov 2019-05-29 7:11 ` [PATCH 1/2] power: supply: Add HWMON compatibility layer Andrey Smirnov @ 2019-05-29 7:11 ` Andrey Smirnov 1 sibling, 0 replies; 6+ messages in thread From: Andrey Smirnov @ 2019-05-29 7:11 UTC (permalink / raw) To: linux-pm Cc: Andrey Smirnov, Chris Healy, Lucas Stach, Fabio Estevam, Guenter Roeck, Sebastian Reichel, linux-kernel Expose current sensors found on UCS1002 via HWMON. Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: Chris Healy <cphealy@gmail.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Fabio Estevam <fabio.estevam@nxp.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Sebastian Reichel <sre@kernel.org> Cc: linux-kernel@vger.kernel.org Cc: linux-pm@vger.kernel.org --- drivers/power/supply/ucs1002_power.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/power/supply/ucs1002_power.c b/drivers/power/supply/ucs1002_power.c index 1c89d030c045..1faf2ef7d3f0 100644 --- a/drivers/power/supply/ucs1002_power.c +++ b/drivers/power/supply/ucs1002_power.c @@ -491,7 +491,7 @@ static const struct regulator_desc ucs1002_regulator_descriptor = { static int ucs1002_probe(struct i2c_client *client, const struct i2c_device_id *dev_id) { - struct device *dev = &client->dev; + struct device *hwmon, *dev = &client->dev; struct power_supply_config charger_config = {}; const struct regmap_config regmap_config = { .reg_bits = 8, @@ -571,6 +571,13 @@ static int ucs1002_probe(struct i2c_client *client, return ret; } + hwmon = devm_power_supply_add_hwmon_sysfs(info->charger); + ret = PTR_ERR_OR_ZERO(hwmon); + if (ret) { + dev_err(dev, "Failed to add hmwon attributes: %d\n", ret); + return ret; + } + ret = regmap_read(info->regmap, UCS1002_REG_PIN_STATUS, ®val); if (ret) { dev_err(dev, "Failed to read pin status: %d\n", ret); -- 2.21.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-30 20:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-05-29 7:11 [PATCH 0/2] HWMON compatibility layer for power supplies Andrey Smirnov 2019-05-29 7:11 ` [PATCH 1/2] power: supply: Add HWMON compatibility layer Andrey Smirnov 2019-05-29 12:40 ` Guenter Roeck 2019-05-30 20:08 ` Andrey Smirnov 2019-05-30 20:32 ` Guenter Roeck 2019-05-29 7:11 ` [PATCH 2/2] power: supply: ucs1002: Add HWMON interface Andrey Smirnov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox