[PATCH v4 0/1] HWMON compatibility layer for power supplies

[PATCH v4 0/1] HWMON compatibility layer for power supplies

[Andrey Smirnov]

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.

Changes since [v3]:

  - Patch converted to have HWMON be registered in
    power_supply_register() and free'd in power_supply_unregister().

  - Collected Tested-by from Chris

Changes since [v2]:

  - Added missing static specified to devm_power_supply_add_hwmon_sysfs()
  
  - Collected Reviewed-by from Guenter

Changes since [v1]:

  - All multiplications converted to use check_mul_overflow()

  - All divisions converted to use DIV_ROUND_CLOSEST()

  - Places that were ignoring errors now don't

  - Alphabetized include list

[v3] lkml.kernel.org/r/20190605072323.21990-1-andrew.smirnov@gmail.com
[v2] lkml.kernel.org/r/20190531011620.9383-1-andrew.smirnov@gmail.com
[v1] lkml.kernel.org/r/20190529071112.16849-1-andrew.smirnov@gmail.com

Thanks,
Andrey Smirnov

Andrey Smirnov (1):
  power: supply: Add HWMON compatibility layer

 drivers/power/supply/Kconfig              |  14 +
 drivers/power/supply/Makefile             |   1 +
 drivers/power/supply/power_supply_core.c  |   7 +
 drivers/power/supply/power_supply_hwmon.c | 355 ++++++++++++++++++++++
 include/linux/power_supply.h              |  13 +
 5 files changed, 390 insertions(+)
 create mode 100644 drivers/power/supply/power_supply_hwmon.c

-- 
2.21.0

[PATCH v4 1/1] power: supply: Add HWMON compatibility layer

[Andrey Smirnov]

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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Chris Healy <cphealy@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Cory Tusar <cory.tusar@zii.aero>
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_core.c  |   7 +
 drivers/power/supply/power_supply_hwmon.c | 355 ++++++++++++++++++++++
 include/linux/power_supply.h              |  13 +
 5 files changed, 390 insertions(+)
 create mode 100644 drivers/power/supply/power_supply_hwmon.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index c614c8a196f3..0550cedb53c9 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 c56803a9e4fe..0a87cfe49b21 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_core.c b/drivers/power/supply/power_supply_core.c
index c917a8b43b2b..67e78538bb9c 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1062,6 +1062,10 @@ __power_supply_register(struct device *parent,
 	if (rc)
 		goto create_triggers_failed;
 
+	rc = power_supply_add_hwmon_sysfs(psy);
+	if (rc)
+		goto add_hwmon_sysfs_failed;
+
 	/*
 	 * Update use_cnt after any uevents (most notably from device_add()).
 	 * We are here still during driver's probe but
@@ -1080,6 +1084,8 @@ __power_supply_register(struct device *parent,
 
 	return psy;
 
+add_hwmon_sysfs_failed:
+	power_supply_remove_triggers(psy);
 create_triggers_failed:
 	psy_unregister_cooler(psy);
 register_cooler_failed:
@@ -1232,6 +1238,7 @@ void power_supply_unregister(struct power_supply *psy)
 	cancel_work_sync(&psy->changed_work);
 	cancel_delayed_work_sync(&psy->deferred_register_work);
 	sysfs_remove_link(&psy->dev.kobj, "powers");
+	power_supply_remove_hwmon_sysfs(psy);
 	power_supply_remove_triggers(psy);
 	psy_unregister_cooler(psy);
 	psy_unregister_thermal(psy);
diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
new file mode 100644
index 000000000000..51fe60440d12
--- /dev/null
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  power_supply_hwmon.c - power supply hwmon support.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/power_supply.h>
+#include <linux/slab.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:
+		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:
+		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:
+		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 bool power_supply_hwmon_is_writable(enum hwmon_sensor_types type,
+					   u32 attr)
+{
+	switch (type) {
+	case hwmon_in:
+		return attr == hwmon_in_min ||
+		       attr == hwmon_in_max;
+	case hwmon_curr:
+		return attr == hwmon_curr_max;
+	case hwmon_temp:
+		return attr == hwmon_temp_max ||
+		       attr == hwmon_temp_min ||
+		       attr == hwmon_temp_min_alarm ||
+		       attr == hwmon_temp_max_alarm;
+	default:
+		return false;
+	}
+}
+
+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;
+
+
+	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;
+
+	if (power_supply_property_is_writeable(psyhw->psy, prop) > 0 &&
+	    power_supply_hwmon_is_writable(type, attr))
+		return 0644;
+
+	return 0444;
+}
+
+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 = DIV_ROUND_CLOSEST(pspval.intval, 1000);
+		break;
+	/*
+	 * Temp needs to be converted from 1/10 C to milli-C
+	 */
+	case hwmon_temp:
+		if (check_mul_overflow(pspval.intval, 100,
+				       &pspval.intval))
+			return -EOVERFLOW;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*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:
+		if (check_mul_overflow(pspval.intval, 1000,
+				       &pspval.intval))
+			return -EOVERFLOW;
+		break;
+	/*
+	 * Temp needs to be converted from 1/10 C to milli-C
+	 */
+	case hwmon_temp:
+		pspval.intval = DIV_ROUND_CLOSEST(pspval.intval, 100);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	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);
+}
+
+int 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, power_supply_add_hwmon_sysfs,
+			       GFP_KERNEL))
+		return -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_close_group(dev, power_supply_add_hwmon_sysfs);
+	return 0;
+error:
+	devres_release_group(dev, NULL);
+	return ret;
+}
+
+void power_supply_remove_hwmon_sysfs(struct power_supply *psy)
+{
+	devres_release_group(&psy->dev, power_supply_add_hwmon_sysfs);
+}
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index bdab14c7ca4d..f850d4110935 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -475,4 +475,17 @@ static inline bool power_supply_is_watt_property(enum power_supply_property psp)
 	return 0;
 }
 
+#ifdef CONFIG_POWER_SUPPLY_HWMON
+int power_supply_add_hwmon_sysfs(struct power_supply *psy);
+void power_supply_remove_hwmon_sysfs(struct power_supply *psy);
+#else
+static inline int power_supply_add_hwmon_sysfs(struct power_supply *psy)
+{
+	return 0;
+}
+
+static inline
+void power_supply_remove_hwmon_sysfs(struct power_supply *psy) {}
+#endif
+
 #endif /* __LINUX_POWER_SUPPLY_H__ */
-- 
2.21.0

Re: [PATCH v4 1/1] power: supply: Add HWMON compatibility layer

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 14360 bytes --]

Hi,

On Wed, Jun 12, 2019 at 01:44:04AM -0700, 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>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Tested-by: Chris Healy <cphealy@gmail.com>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/Kconfig              |  14 +
>  drivers/power/supply/Makefile             |   1 +
>  drivers/power/supply/power_supply_core.c  |   7 +
>  drivers/power/supply/power_supply_hwmon.c | 355 ++++++++++++++++++++++
>  include/linux/power_supply.h              |  13 +
>  5 files changed, 390 insertions(+)
>  create mode 100644 drivers/power/supply/power_supply_hwmon.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index c614c8a196f3..0550cedb53c9 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 c56803a9e4fe..0a87cfe49b21 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_core.c b/drivers/power/supply/power_supply_core.c
> index c917a8b43b2b..67e78538bb9c 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1062,6 +1062,10 @@ __power_supply_register(struct device *parent,
>  	if (rc)
>  		goto create_triggers_failed;
>  
> +	rc = power_supply_add_hwmon_sysfs(psy);
> +	if (rc)
> +		goto add_hwmon_sysfs_failed;
> +
>  	/*
>  	 * Update use_cnt after any uevents (most notably from device_add()).
>  	 * We are here still during driver's probe but
> @@ -1080,6 +1084,8 @@ __power_supply_register(struct device *parent,
>  
>  	return psy;
>  
> +add_hwmon_sysfs_failed:
> +	power_supply_remove_triggers(psy);
>  create_triggers_failed:
>  	psy_unregister_cooler(psy);
>  register_cooler_failed:
> @@ -1232,6 +1238,7 @@ void power_supply_unregister(struct power_supply *psy)
>  	cancel_work_sync(&psy->changed_work);
>  	cancel_delayed_work_sync(&psy->deferred_register_work);
>  	sysfs_remove_link(&psy->dev.kobj, "powers");
> +	power_supply_remove_hwmon_sysfs(psy);
>  	power_supply_remove_triggers(psy);
>  	psy_unregister_cooler(psy);
>  	psy_unregister_thermal(psy);
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> new file mode 100644
> index 000000000000..51fe60440d12
> --- /dev/null
> +++ b/drivers/power/supply/power_supply_hwmon.c
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  power_supply_hwmon.c - power supply hwmon support.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/power_supply.h>
> +#include <linux/slab.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:
> +		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:
> +		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:
> +		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 bool power_supply_hwmon_is_writable(enum hwmon_sensor_types type,
> +					   u32 attr)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return attr == hwmon_in_min ||
> +		       attr == hwmon_in_max;
> +	case hwmon_curr:
> +		return attr == hwmon_curr_max;
> +	case hwmon_temp:
> +		return attr == hwmon_temp_max ||
> +		       attr == hwmon_temp_min ||
> +		       attr == hwmon_temp_min_alarm ||
> +		       attr == hwmon_temp_max_alarm;
> +	default:
> +		return false;
> +	}
> +}
> +
> +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;
> +
> +
> +	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;
> +
> +	if (power_supply_property_is_writeable(psyhw->psy, prop) > 0 &&
> +	    power_supply_hwmon_is_writable(type, attr))
> +		return 0644;
> +
> +	return 0444;
> +}
> +
> +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 = DIV_ROUND_CLOSEST(pspval.intval, 1000);
> +		break;
> +	/*
> +	 * Temp needs to be converted from 1/10 C to milli-C
> +	 */
> +	case hwmon_temp:
> +		if (check_mul_overflow(pspval.intval, 100,
> +				       &pspval.intval))
> +			return -EOVERFLOW;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	*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:
> +		if (check_mul_overflow(pspval.intval, 1000,
> +				       &pspval.intval))
> +			return -EOVERFLOW;
> +		break;
> +	/*
> +	 * Temp needs to be converted from 1/10 C to milli-C
> +	 */
> +	case hwmon_temp:
> +		pspval.intval = DIV_ROUND_CLOSEST(pspval.intval, 100);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	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);
> +}
> +
> +int 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, power_supply_add_hwmon_sysfs,
> +			       GFP_KERNEL))
> +		return -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_close_group(dev, power_supply_add_hwmon_sysfs);
> +	return 0;
> +error:
> +	devres_release_group(dev, NULL);
> +	return ret;
> +}
> +
> +void power_supply_remove_hwmon_sysfs(struct power_supply *psy)
> +{
> +	devres_release_group(&psy->dev, power_supply_add_hwmon_sysfs);
> +}
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index bdab14c7ca4d..f850d4110935 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -475,4 +475,17 @@ static inline bool power_supply_is_watt_property(enum power_supply_property psp)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_POWER_SUPPLY_HWMON
> +int power_supply_add_hwmon_sysfs(struct power_supply *psy);
> +void power_supply_remove_hwmon_sysfs(struct power_supply *psy);
> +#else
> +static inline int power_supply_add_hwmon_sysfs(struct power_supply *psy)
> +{
> +	return 0;
> +}
> +
> +static inline
> +void power_supply_remove_hwmon_sysfs(struct power_supply *psy) {}
> +#endif
> +
>  #endif /* __LINUX_POWER_SUPPLY_H__ */
> -- 
> 2.21.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 1/1] power: supply: Add HWMON compatibility layer

[Andrey Smirnov]

On Thu, Jun 27, 2019 at 11:27 AM Sebastian Reichel <sre@kernel.org> wrote:
>
> Hi,
>
> On Wed, Jun 12, 2019 at 01:44:04AM -0700, 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>
> > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > Tested-by: Chris Healy <cphealy@gmail.com>
> > ---
>
> Thanks, queued.
>

Hmm, I just realized that power supply exposing POWER_SUPPLY_PROP_TEMP
will have a thermal zone created for it by default, which will expose
said reading via thermal/HWMON integration layer. Do we want to do
anything about this duplication of functionality? Maybe hide it here
if desc->no_thermal is true? Or just drop reporting temperature from
this patch?

Thanks,
Andrey Smirnov