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 [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
[v2] lkml.kernel.org/r/[email protected]
[v1] lkml.kernel.org/r/[email protected]
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 | 349 ++++++++++++++++++++++
drivers/power/supply/ucs1002_power.c | 6 +
include/linux/power_supply.h | 9 +
5 files changed, 379 insertions(+)
create mode 100644 drivers/power/supply/power_supply_hwmon.c
--
2.21.0
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 <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Cory Tusar <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Sebastian Reichel <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/power/supply/Kconfig | 14 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/power_supply_hwmon.c | 349 ++++++++++++++++++++++
include/linux/power_supply.h | 9 +
4 files changed, 373 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_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
new file mode 100644
index 000000000000..3451b997367d
--- /dev/null
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -0,0 +1,349 @@
+// 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 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 -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 0;
+error:
+ devres_release_group(dev, NULL);
+ return ret;
+}
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index bdab14c7ca4d..39343bc4a3f8 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -475,4 +475,13 @@ static inline bool power_supply_is_watt_property(enum power_supply_property psp)
return 0;
}
+#ifdef CONFIG_POWER_SUPPLY_HWMON
+int devm_power_supply_add_hwmon_sysfs(struct power_supply *psy);
+#else
+static int devm_power_supply_add_hwmon_sysfs(struct power_supply *psy)
+{
+ return 0;
+}
+#endif
+
#endif /* __LINUX_POWER_SUPPLY_H__ */
--
2.21.0
Expose current sensors found on UCS1002 via HWMON.
Signed-off-by: Andrey Smirnov <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Cory Tusar <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Sebastian Reichel <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/power/supply/ucs1002_power.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/power/supply/ucs1002_power.c b/drivers/power/supply/ucs1002_power.c
index 677f20a4d76f..a4b6b37549cf 100644
--- a/drivers/power/supply/ucs1002_power.c
+++ b/drivers/power/supply/ucs1002_power.c
@@ -571,6 +571,12 @@ static int ucs1002_probe(struct i2c_client *client,
return ret;
}
+ ret = devm_power_supply_add_hwmon_sysfs(info->charger);
+ 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
On 6/5/19 3:23 AM, Andrey Smirnov wrote:
> Expose current sensors found on UCS1002 via HWMON.
>
> Signed-off-by: Andrey Smirnov <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
> Cc: Chris Healy <[email protected]>
> Cc: Cory Tusar <[email protected]>
> Cc: Lucas Stach <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Sebastian Reichel <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/power/supply/ucs1002_power.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/power/supply/ucs1002_power.c b/drivers/power/supply/ucs1002_power.c
> index 677f20a4d76f..a4b6b37549cf 100644
> --- a/drivers/power/supply/ucs1002_power.c
> +++ b/drivers/power/supply/ucs1002_power.c
> @@ -571,6 +571,12 @@ static int ucs1002_probe(struct i2c_client *client,
> return ret;
> }
>
> + ret = devm_power_supply_add_hwmon_sysfs(info->charger);
Could this be added to the core power supply framework on registering so
all devices get this, vs each driver having to add this line?
Andrew
> + 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);
>
On Wed, Jun 5, 2019 at 9:26 AM Andrew F. Davis <[email protected]> wrote:
>
> On 6/5/19 3:23 AM, Andrey Smirnov wrote:
> > Expose current sensors found on UCS1002 via HWMON.
> >
> > Signed-off-by: Andrey Smirnov <[email protected]>
> > Reviewed-by: Guenter Roeck <[email protected]>
> > Cc: Chris Healy <[email protected]>
> > Cc: Cory Tusar <[email protected]>
> > Cc: Lucas Stach <[email protected]>
> > Cc: Fabio Estevam <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > Cc: Sebastian Reichel <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > drivers/power/supply/ucs1002_power.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/power/supply/ucs1002_power.c b/drivers/power/supply/ucs1002_power.c
> > index 677f20a4d76f..a4b6b37549cf 100644
> > --- a/drivers/power/supply/ucs1002_power.c
> > +++ b/drivers/power/supply/ucs1002_power.c
> > @@ -571,6 +571,12 @@ static int ucs1002_probe(struct i2c_client *client,
> > return ret;
> > }
> >
> > + ret = devm_power_supply_add_hwmon_sysfs(info->charger);
>
> Could this be added to the core power supply framework on registering so
> all devices get this, vs each driver having to add this line?
>
I'd say it is up to Sebastian to decide if this should be opt-out
rather than opt-in. I have no objections to either approach.
Thanks,
Andrey Smirnov
On Wed, Jun 05, 2019 at 10:47:25AM -0700, Andrey Smirnov wrote:
> On Wed, Jun 5, 2019 at 9:26 AM Andrew F. Davis <[email protected]> wrote:
> >
> > On 6/5/19 3:23 AM, Andrey Smirnov wrote:
> > > Expose current sensors found on UCS1002 via HWMON.
> > >
> > > Signed-off-by: Andrey Smirnov <[email protected]>
> > > Reviewed-by: Guenter Roeck <[email protected]>
> > > Cc: Chris Healy <[email protected]>
> > > Cc: Cory Tusar <[email protected]>
> > > Cc: Lucas Stach <[email protected]>
> > > Cc: Fabio Estevam <[email protected]>
> > > Cc: Guenter Roeck <[email protected]>
> > > Cc: Sebastian Reichel <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > ---
> > > drivers/power/supply/ucs1002_power.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/power/supply/ucs1002_power.c b/drivers/power/supply/ucs1002_power.c
> > > index 677f20a4d76f..a4b6b37549cf 100644
> > > --- a/drivers/power/supply/ucs1002_power.c
> > > +++ b/drivers/power/supply/ucs1002_power.c
> > > @@ -571,6 +571,12 @@ static int ucs1002_probe(struct i2c_client *client,
> > > return ret;
> > > }
> > >
> > > + ret = devm_power_supply_add_hwmon_sysfs(info->charger);
> >
> > Could this be added to the core power supply framework on registering so
> > all devices get this, vs each driver having to add this line?
> >
>
> I'd say it is up to Sebastian to decide if this should be opt-out
> rather than opt-in. I have no objections to either approach.
>
Same here, and agreed.
Guenter
> 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 [v2]:
Full series is tested using the UCS1002.
Tested-by: [email protected] <[email protected]>
Hi,
On Wed, Jun 05, 2019 at 11:39:45AM -0700, Guenter Roeck wrote:
> On Wed, Jun 05, 2019 at 10:47:25AM -0700, Andrey Smirnov wrote:
> > On Wed, Jun 5, 2019 at 9:26 AM Andrew F. Davis <[email protected]> wrote:
> > >
> > > On 6/5/19 3:23 AM, Andrey Smirnov wrote:
> > > > Expose current sensors found on UCS1002 via HWMON.
> > > >
> > > > Signed-off-by: Andrey Smirnov <[email protected]>
> > > > Reviewed-by: Guenter Roeck <[email protected]>
> > > > Cc: Chris Healy <[email protected]>
> > > > Cc: Cory Tusar <[email protected]>
> > > > Cc: Lucas Stach <[email protected]>
> > > > Cc: Fabio Estevam <[email protected]>
> > > > Cc: Guenter Roeck <[email protected]>
> > > > Cc: Sebastian Reichel <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > ---
> > > > drivers/power/supply/ucs1002_power.c | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/power/supply/ucs1002_power.c b/drivers/power/supply/ucs1002_power.c
> > > > index 677f20a4d76f..a4b6b37549cf 100644
> > > > --- a/drivers/power/supply/ucs1002_power.c
> > > > +++ b/drivers/power/supply/ucs1002_power.c
> > > > @@ -571,6 +571,12 @@ static int ucs1002_probe(struct i2c_client *client,
> > > > return ret;
> > > > }
> > > >
> > > > + ret = devm_power_supply_add_hwmon_sysfs(info->charger);
> > >
> > > Could this be added to the core power supply framework on registering so
> > > all devices get this, vs each driver having to add this line?
> > >
> >
> > I'd say it is up to Sebastian to decide if this should be opt-out
> > rather than opt-in. I have no objections to either approach.
> >
>
> Same here, and agreed.
I think this should be registered in power_supply_register() and
free'd in power_supply_unregister(). It's not device specific at
all and the functionality can be configured via Kconfig.
-- Sebastian
On Tue, Jun 11, 2019 at 12:46 PM Sebastian Reichel <[email protected]> wrote:
>
> Hi,
>
> On Wed, Jun 05, 2019 at 11:39:45AM -0700, Guenter Roeck wrote:
> > On Wed, Jun 05, 2019 at 10:47:25AM -0700, Andrey Smirnov wrote:
> > > On Wed, Jun 5, 2019 at 9:26 AM Andrew F. Davis <[email protected]> wrote:
> > > >
> > > > On 6/5/19 3:23 AM, Andrey Smirnov wrote:
> > > > > Expose current sensors found on UCS1002 via HWMON.
> > > > >
> > > > > Signed-off-by: Andrey Smirnov <[email protected]>
> > > > > Reviewed-by: Guenter Roeck <[email protected]>
> > > > > Cc: Chris Healy <[email protected]>
> > > > > Cc: Cory Tusar <[email protected]>
> > > > > Cc: Lucas Stach <[email protected]>
> > > > > Cc: Fabio Estevam <[email protected]>
> > > > > Cc: Guenter Roeck <[email protected]>
> > > > > Cc: Sebastian Reichel <[email protected]>
> > > > > Cc: [email protected]
> > > > > Cc: [email protected]
> > > > > ---
> > > > > drivers/power/supply/ucs1002_power.c | 6 ++++++
> > > > > 1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/power/supply/ucs1002_power.c b/drivers/power/supply/ucs1002_power.c
> > > > > index 677f20a4d76f..a4b6b37549cf 100644
> > > > > --- a/drivers/power/supply/ucs1002_power.c
> > > > > +++ b/drivers/power/supply/ucs1002_power.c
> > > > > @@ -571,6 +571,12 @@ static int ucs1002_probe(struct i2c_client *client,
> > > > > return ret;
> > > > > }
> > > > >
> > > > > + ret = devm_power_supply_add_hwmon_sysfs(info->charger);
> > > >
> > > > Could this be added to the core power supply framework on registering so
> > > > all devices get this, vs each driver having to add this line?
> > > >
> > >
> > > I'd say it is up to Sebastian to decide if this should be opt-out
> > > rather than opt-in. I have no objections to either approach.
> > >
> >
> > Same here, and agreed.
>
> I think this should be registered in power_supply_register() and
> free'd in power_supply_unregister(). It's not device specific at
> all and the functionality can be configured via Kconfig.
>
Sure, will do in v4.
Thanks,
Andrey Smirnov