Re: [PATCH 1/2] hwmon: (core) Add hwmon_mode structure and mode sysfs node
Hi Nicolin,
On 10/09/2018 09:33 PM, Nicolin Chen wrote:
There are a few hwmon sensors support different operating modes,
for example, one-shot and continuous modes. So it's probably not
a bad idea to abstract a mode sysfs node as a common feature in
the hwmon core.
Right beside the hwmon device name, this patch adds a new sysfs
attribute named "mode" and "available_modes" for user to check
and configure the operating mode. For hwmon device drivers that
implemented the _with_info API, the change also adds an optional
hwmon_mode structure in hwmon_chip_info structure so that those
drivers can pass mode related information.
Signed-off-by: Nicolin Chen
---
Documentation/hwmon/sysfs-interface | 15 +
drivers/hwmon/hwmon.c | 87 ++---
include/linux/hwmon.h | 24
3 files changed, 119 insertions(+), 7 deletions(-)
diff --git a/Documentation/hwmon/sysfs-interface
b/Documentation/hwmon/sysfs-interface
index 2b9e1005d88b..48d6ca6b9bd4 100644
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
@@ -92,6 +92,21 @@ name The chip name.
I2C devices get this attribute created automatically.
RO
+available_modes The available operating modes of the chip.
+ This should be short, lowercase string, not containing
+ whitespace, or the wildcard character '*'.
+ This attribute shows all the available of the operating modes,
+ for example, "power-down" "one-shot" and "continuous".
+ RO
+
+mode The current operating mode of the chip.
+ This should be short, lowercase string, not containing
+ whitespace, or the wildcard character '*'.
+ This attribute shows the current operating mode of the chip.
+ Writing a valid string from the list of available_modes will
+ configure the chip to the corresponding operating mode.
+ RW
+
No, sorry.
This is not a well defined ABI: The modes would be under full and arbitrary
control by drivers, and be completely driver dependent. It isn't just the sysfs
attribute that makes the ABI, it is also the contents.
Also, being able to set the mode itself (for whatever definition of mode)
is of questionable value. This is not only for the modes suggested here, but
for other possible modes such as comparator mode vs. interrupt mode (which,
if configurable, should be via platform data or devicetree node entries).
For the modes suggested here, more in the other patch.
In short, NACK. I am open to enhancing the ABI, but I don't see the value
of this attribute.
Guenter
update_interval The interval at which the chip will update readings.
Unit: millisecond
RW
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 975c95169884..5a33b616284b 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -72,25 +72,88 @@ name_show(struct device *dev, struct device_attribute
*attr, char *buf)
}
static DEVICE_ATTR_RO(name);
+static ssize_t available_modes_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct hwmon_device *hwdev = to_hwmon_device(dev);
+ const struct hwmon_chip_info *chip = hwdev->chip;
+ const struct hwmon_mode *mode = chip->mode;
+ int i;
+
+ for (i = 0; i < mode->list_size; i++)
+ snprintf(buf, PAGE_SIZE, "%s%s ", buf, mode->names[i]);
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", buf);
+}
+static DEVICE_ATTR_RO(available_modes);
+
+static ssize_t mode_show(struct device *dev,
+struct device_attribute *attr, char *buf)
+{
+ struct hwmon_device *hwdev = to_hwmon_device(dev);
+ const struct hwmon_chip_info *chip = hwdev->chip;
+ const struct hwmon_mode *mode = chip->mode;
+ unsigned int index;
+ int ret;
+
+ ret = mode->get_index(dev, &index);
+ if (ret)
+ return ret;
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", mode->names[index]);
+}
+
+static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct hwmon_device *hwdev = to_hwmon_device(dev);
+ const struct hwmon_chip_info *chip = hwdev->chip;
+ const struct hwmon_mode *mode = chip->mode;
+ const char **names = mode->names;
+ unsigned int index;
+ int ret;
+
+ /* Get the corresponding mode index */
+ for (index = 0; index < mode->list_size; index++) {
+ if (!strncmp(buf, names[index], strlen(names[index])))
+ break;
+ }
+
+ if (index >= mode->list_size)
+ return -EINVAL;
+
+ ret = mode->set_index(dev, index);
+ if (ret)
+ return ret;
+
+ return count;
+}
+static
Re: [PATCH 2/2] hwmon: (ina3221) Add operating mode support
Hi Nicolin,
On 10/09/2018 09:33 PM, Nicolin Chen wrote:
The hwmon core now has a new optional mode interface. So this patch
just implements this mode support so that user space can check and
configure via sysfs node its operating modes: power-down, one-shot,
and continuous modes.
One-shot mode on its own does not make sense or add value: It would require
explicit driver support to trigger a reading, wait for the result, and
report it back to the user. If the intent here is to have the user write the
mode (which triggers the one-shot reading), wait a bit, and then read the
results, that doesn't make sense because standard userspace applications
won't know that. Also, that would be unsynchronized - one has to read the
CVRF bit in the mask/enable register to know if the reading is complete.
The effort to do all this using CPU cycles would in most if not all cases
outweigh any perceived power savings. As such, I just don't see the
practical use case.
power-down mode effectively reinvents runtime power management (runtime
suspend/resume support) and is thus simply unacceptable.
I am open to help explore adding support for runtime power management
to the hwmon subsystem, but that would be less than straightforward and
require an actual use case to warrant the effort.
As such, NACK, sorry.
Thanks,
Guenter
Signed-off-by: Nicolin Chen
---
drivers/hwmon/ina3221.c | 64 +
1 file changed, 64 insertions(+)
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index d61688f04594..5218fd85506d 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -77,6 +77,28 @@ enum ina3221_channels {
INA3221_NUM_CHANNELS
};
+enum ina3221_modes {
+ INA3221_MODE_POWERDOWN,
+ INA3221_MODE_ONESHOT,
+ INA3221_MODE_CONTINUOUS,
+ INA3221_NUM_MODES,
+};
+
+static const char *ina3221_mode_names[INA3221_NUM_MODES] = {
+ [INA3221_MODE_POWERDOWN] = "power-down",
+ [INA3221_MODE_ONESHOT] = "one-shot",
+ [INA3221_MODE_CONTINUOUS] = "continuous",
+};
+
+static const u16 ina3221_mode_val[] = {
+ [INA3221_MODE_POWERDOWN] = INA3221_CONFIG_MODE_POWERDOWN,
+ [INA3221_MODE_ONESHOT] = INA3221_CONFIG_MODE_SHUNT |
+INA3221_CONFIG_MODE_BUS,
+ [INA3221_MODE_CONTINUOUS] = INA3221_CONFIG_MODE_CONTINUOUS |
+ INA3221_CONFIG_MODE_SHUNT |
+ INA3221_CONFIG_MODE_BUS,
+};
+
/**
* struct ina3221_input - channel input source specific information
* @label: label of channel input source
@@ -386,9 +408,51 @@ static const struct hwmon_ops ina3221_hwmon_ops = {
.write = ina3221_write,
};
+static int ina3221_mode_get_index(struct device *dev, unsigned int *index)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ u16 mode = ina->reg_config & INA3221_CONFIG_MODE_MASK;
+
+ if (mode == INA3221_CONFIG_MODE_POWERDOWN)
+ *index = INA3221_MODE_POWERDOWN;
+ if (mode & INA3221_CONFIG_MODE_CONTINUOUS)
+ *index = INA3221_MODE_CONTINUOUS;
+ else
+ *index = INA3221_MODE_ONESHOT;
+
+ return 0;
+}
+
+static int ina3221_mode_set_index(struct device *dev, unsigned int index)
+{
+ struct ina3221_data *ina = dev_get_drvdata(dev);
+ int ret;
+
+ ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
+INA3221_CONFIG_MODE_MASK,
+ina3221_mode_val[index]);
+ if (ret)
+ return ret;
+
+ /* Cache the latest config register value */
+ ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct hwmon_mode ina3221_hwmon_mode = {
+ .names = ina3221_mode_names,
+ .list_size = INA3221_NUM_MODES,
+ .get_index = ina3221_mode_get_index,
+ .set_index = ina3221_mode_set_index,
+};
+
static const struct hwmon_chip_info ina3221_chip_info = {
.ops = &ina3221_hwmon_ops,
.info = ina3221_info,
+ .mode = &ina3221_hwmon_mode,
};
/* Extra attribute groups */
[PATCH] hwmon: (pmbus) remove redundant 'default n' from Kconfig
'default n' is the default value for any bool or tristate Kconfig
setting so there is no need to write it explicitly.
Also since commit f467c5640c29 ("kconfig: only write '# CONFIG_FOO
is not set' for visible symbols") the Kconfig behavior is the same
regardless of 'default n' being present or not:
...
One side effect of (and the main motivation for) this change is making
the following two definitions behave exactly the same:
config FOO
bool
config FOO
bool
default n
With this change, neither of these will generate a
'# CONFIG_FOO is not set' line (assuming FOO isn't selected/implied).
That might make it clearer to people that a bare 'default n' is
redundant.
...
Signed-off-by: Bartlomiej Zolnierkiewicz
---
drivers/hwmon/pmbus/Kconfig | 15 ---
1 file changed, 15 deletions(-)
Index: b/drivers/hwmon/pmbus/Kconfig
===
--- a/drivers/hwmon/pmbus/Kconfig 2018-10-09 15:58:39.067122941 +0200
+++ b/drivers/hwmon/pmbus/Kconfig 2018-10-10 16:31:57.671888538 +0200
@@ -5,7 +5,6 @@
menuconfig PMBUS
tristate "PMBus support"
depends on I2C
- default n
help
Say yes here if you want to enable PMBus support.
@@ -28,7 +27,6 @@ config SENSORS_PMBUS
config SENSORS_ADM1275
tristate "Analog Devices ADM1275 and compatibles"
- default n
help
If you say yes here you get hardware monitoring support for Analog
Devices ADM1075, ADM1272, ADM1275, ADM1276, ADM1278, ADM1293,
@@ -49,7 +47,6 @@ config SENSORS_IBM_CFFPS
config SENSORS_IR35221
tristate "Infineon IR35221"
- default n
help
If you say yes here you get hardware monitoring support for the
Infineon IR35221 controller.
@@ -59,7 +56,6 @@ config SENSORS_IR35221
config SENSORS_LM25066
tristate "National Semiconductor LM25066 and compatibles"
- default n
help
If you say yes here you get hardware monitoring support for National
Semiconductor LM25056, LM25066, LM5064, and LM5066.
@@ -69,7 +65,6 @@ config SENSORS_LM25066
config SENSORS_LTC2978
tristate "Linear Technologies LTC2978 and compatibles"
- default n
help
If you say yes here you get hardware monitoring support for Linear
Technology LTC2974, LTC2975, LTC2977, LTC2978, LTC2980, LTC3880,
@@ -88,7 +83,6 @@ config SENSORS_LTC2978_REGULATOR
config SENSORS_LTC3815
tristate "Linear Technologies LTC3815"
- default n
help
If you say yes here you get hardware monitoring support for Linear
Technology LTC3815.
@@ -98,7 +92,6 @@ config SENSORS_LTC3815
config SENSORS_MAX16064
tristate "Maxim MAX16064"
- default n
help
If you say yes here you get hardware monitoring support for Maxim
MAX16064.
@@ -108,7 +101,6 @@ config SENSORS_MAX16064
config SENSORS_MAX20751
tristate "Maxim MAX20751"
- default n
help
If you say yes here you get hardware monitoring support for Maxim
MAX20751.
@@ -118,7 +110,6 @@ config SENSORS_MAX20751
config SENSORS_MAX31785
tristate "Maxim MAX31785 and compatibles"
- default n
help
If you say yes here you get hardware monitoring support for Maxim
MAX31785.
@@ -128,7 +119,6 @@ config SENSORS_MAX31785
config SENSORS_MAX34440
tristate "Maxim MAX34440 and compatibles"
- default n
help
If you say yes here you get hardware monitoring support for Maxim
MAX34440, MAX34441, MAX34446, MAX34451, MAX34460, and MAX34461.
@@ -138,7 +128,6 @@ config SENSORS_MAX34440
config SENSORS_MAX8688
tristate "Maxim MAX8688"
- default n
help
If you say yes here you get hardware monitoring support for Maxim
MAX8688.
@@ -148,7 +137,6 @@ config SENSORS_MAX8688
config SENSORS_TPS40422
tristate "TI TPS40422"
- default n
help
If you say yes here you get hardware monitoring support for TI
TPS40422.
@@ -167,7 +155,6 @@ config SENSORS_TPS53679
config SENSORS_UCD9000
tristate "TI UCD90120, UCD90124, UCD90160, UCD9090, UCD90910"
- default n
help
If you say yes here you get hardware monitoring support for TI
UCD90120, UCD90124, UCD90160, UCD9090, UCD90910, Sequencer and System
@@ -178,7 +165,6 @@ config SENSORS_UCD9000
config SENSORS_UCD9200
tristate "TI UCD9220, UCD9222, UCD9224, UCD9240, UCD9244, UCD9246,
UCD9248"
- default n
help
If you say yes here you get hardware monitoring support for TI
UCD9220, UCD9222, UCD9224, UCD9240, UCD9244, UCD9246, and UCD9248
@@ -189,7 +175,6 @@ config SENSORS_UCD9200
config SENSORS_ZL6100
tristate "
Re: [PATCH 1/2] hwmon: (core) Add hwmon_mode structure and mode sysfs node
Hi Guenter,
On Wed, Oct 10, 2018 at 06:08:30AM -0700, Guenter Roeck wrote:
> > +available_modes The available operating modes of the chip.
> > + This should be short, lowercase string, not containing
> > + whitespace, or the wildcard character '*'.
> > + This attribute shows all the available of the operating modes,
> > + for example, "power-down" "one-shot" and "continuous".
> > + RO
> > +
> > +mode The current operating mode of the chip.
> > + This should be short, lowercase string, not containing
> > + whitespace, or the wildcard character '*'.
> > + This attribute shows the current operating mode of the chip.
> > + Writing a valid string from the list of available_modes will
> > + configure the chip to the corresponding operating mode.
> > + RW
> > +
> This is not a well defined ABI: The modes would be under full and arbitrary
> control by drivers, and be completely driver dependent. It isn't just the
> sysfs
> attribute that makes the ABI, it is also the contents.
> Also, being able to set the mode itself (for whatever definition of mode)
> is of questionable value. This is not only for the modes suggested here, but
> for other possible modes such as comparator mode vs. interrupt mode (which,
> if configurable, should be via platform data or devicetree node entries).
> For the modes suggested here, more in the other patch.
I could foresee an objection here but still wrote the change after
seeing quite a few drivers (especially TI's chips) share the same
pattern for operating modes: power-down, one-shot and continuous.
For example, I could add it to ina3221 driver instead of touching
the core code, but later I would do the same for the ina2xx driver
(just received a board having ina230/226.)
Although I don't mind doing this and will put it to ina3221 driver
in v2, yet maybe we could think about a better way to abstract it?
Thank you
Nicolin
--
> In short, NACK. I am open to enhancing the ABI, but I don't see the value
> of this attribute.
>
> Guenter
>
>
> > update_interval The interval at which the chip will update readings.
> > Unit: millisecond
> > RW
> > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> > index 975c95169884..5a33b616284b 100644
> > --- a/drivers/hwmon/hwmon.c
> > +++ b/drivers/hwmon/hwmon.c
> > @@ -72,25 +72,88 @@ name_show(struct device *dev, struct device_attribute
> > *attr, char *buf)
> > }
> > static DEVICE_ATTR_RO(name);
> > +static ssize_t available_modes_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct hwmon_device *hwdev = to_hwmon_device(dev);
> > + const struct hwmon_chip_info *chip = hwdev->chip;
> > + const struct hwmon_mode *mode = chip->mode;
> > + int i;
> > +
> > + for (i = 0; i < mode->list_size; i++)
> > + snprintf(buf, PAGE_SIZE, "%s%s ", buf, mode->names[i]);
> > +
> > + return snprintf(buf, PAGE_SIZE, "%s\n", buf);
> > +}
> > +static DEVICE_ATTR_RO(available_modes);
> > +
> > +static ssize_t mode_show(struct device *dev,
> > +struct device_attribute *attr, char *buf)
> > +{
> > + struct hwmon_device *hwdev = to_hwmon_device(dev);
> > + const struct hwmon_chip_info *chip = hwdev->chip;
> > + const struct hwmon_mode *mode = chip->mode;
> > + unsigned int index;
> > + int ret;
> > +
> > + ret = mode->get_index(dev, &index);
> > + if (ret)
> > + return ret;
> > +
> > + return snprintf(buf, PAGE_SIZE, "%s\n", mode->names[index]);
> > +}
> > +
> > +static ssize_t mode_store(struct device *dev, struct device_attribute
> > *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct hwmon_device *hwdev = to_hwmon_device(dev);
> > + const struct hwmon_chip_info *chip = hwdev->chip;
> > + const struct hwmon_mode *mode = chip->mode;
> > + const char **names = mode->names;
> > + unsigned int index;
> > + int ret;
> > +
> > + /* Get the corresponding mode index */
> > + for (index = 0; index < mode->list_size; index++) {
> > + if (!strncmp(buf, names[index], strlen(names[index])))
> > + break;
> > + }
> > +
> > + if (index >= mode->list_size)
> > + return -EINVAL;
> > +
> > + ret = mode->set_index(dev, index);
> > + if (ret)
> > + return ret;
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_RW(mode);
> > +
> > static struct attribute *hwmon_dev_attrs[] = {
> > - &dev_attr_name.attr,
> > + &dev_attr_name.attr,/* index = 0 */
> > + &dev_attr_available_modes.attr, /* index = 1 */
> > + &dev_attr_mode.attr,/* index = 2 */
> > NULL
> > };
> > -static umode_t hwmon_dev_name_is_visible(struct kobject *kobj,
> > -struct attribute *attr, int n)
> > +static umode_t hwmon_dev_is_visible
Re: [PATCH 1/2] hwmon: (core) Add hwmon_mode structure and mode sysfs node
Hi Nicolin, On Wed, Oct 10, 2018 at 02:13:57PM -0700, Nicolin Chen wrote: > Hi Guenter, > > On Wed, Oct 10, 2018 at 06:08:30AM -0700, Guenter Roeck wrote: > > > +available_modes The available operating modes of the chip. > > > + This should be short, lowercase string, not containing > > > + whitespace, or the wildcard character '*'. > > > + This attribute shows all the available of the operating modes, > > > + for example, "power-down" "one-shot" and "continuous". > > > + RO > > > + > > > +mode The current operating mode of the chip. > > > + This should be short, lowercase string, not containing > > > + whitespace, or the wildcard character '*'. > > > + This attribute shows the current operating mode of the chip. > > > + Writing a valid string from the list of available_modes will > > > + configure the chip to the corresponding operating mode. > > > + RW > > > + > > > This is not a well defined ABI: The modes would be under full and arbitrary > > control by drivers, and be completely driver dependent. It isn't just the > > sysfs > > attribute that makes the ABI, it is also the contents. > > > Also, being able to set the mode itself (for whatever definition of mode) > > is of questionable value. This is not only for the modes suggested here, but > > for other possible modes such as comparator mode vs. interrupt mode (which, > > if configurable, should be via platform data or devicetree node entries). > > For the modes suggested here, more in the other patch. > > I could foresee an objection here but still wrote the change after > seeing quite a few drivers (especially TI's chips) share the same > pattern for operating modes: power-down, one-shot and continuous. > For example, I could add it to ina3221 driver instead of touching > the core code, but later I would do the same for the ina2xx driver > (just received a board having ina230/226.) > Most hardware monitoring chips have the functionality. That doesn't mean that it makes sense to use/expose it. > Although I don't mind doing this and will put it to ina3221 driver > in v2, yet maybe we could think about a better way to abstract it? > My comments to patch 2/2 still apply. Powerdown duplicates existing and standardized functionality, one-shot mode is not as simple as just enabling the mode, and I find it quite unlikely to one-shot mode would actually save any energy. Thanks, Guenter
Re: [PATCH 2/2] hwmon: (ina3221) Add operating mode support
Hello Guenter,
On Wed, Oct 10, 2018 at 06:22:39AM -0700, Guenter Roeck wrote:
> > The hwmon core now has a new optional mode interface. So this patch
> > just implements this mode support so that user space can check and
> > configure via sysfs node its operating modes: power-down, one-shot,
> > and continuous modes.
> One-shot mode on its own does not make sense or add value: It would require
> explicit driver support to trigger a reading, wait for the result, and
> report it back to the user. If the intent here is to have the user write the
> mode (which triggers the one-shot reading), wait a bit, and then read the
> results, that doesn't make sense because standard userspace applications
> won't know that. Also, that would be unsynchronized - one has to read the
> CVRF bit in the mask/enable register to know if the reading is complete.
I think I oversimplified the one-shot mode here and you are right:
there should be a one-shot reading routine; the conversion time in
the configuration register also needs to be taken care of.
> The effort to do all this using CPU cycles would in most if not all cases
> outweigh any perceived power savings. As such, I just don't see the
> practical use case.
It really depends on the use case and how often the one-shot gets
triggered. For battery-powered devices, running in the continuous
mode does consume considerable power based on the measurement from
our power folks. If a system is running in a power sensitive mode,
while it still needs to occasionally check the inputs, it could be
a use case for one-shot mode, though it's purely a user decision.
> power-down mode effectively reinvents runtime power management (runtime
> suspend/resume support) and is thus simply unacceptable.
Similar to one-shot, if a system is in a low power mode where it
doesn't want to check the inputs anymore, I feel the user space
could at least make the decision to turn on/off the chips, I am
not quite sure if the generic runtime PM system already has this
kind of support though.
> I am open to help explore adding support for runtime power management
> to the hwmon subsystem, but that would be less than straightforward and
> require an actual use case to warrant the effort.
Is there any feasible solution from your point of view?
Thanks
Nicolin
>
> As such, NACK, sorry.
>
> Thanks,
> Guenter
>
> > Signed-off-by: Nicolin Chen
> > ---
> > drivers/hwmon/ina3221.c | 64 +
> > 1 file changed, 64 insertions(+)
> >
> > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> > index d61688f04594..5218fd85506d 100644
> > --- a/drivers/hwmon/ina3221.c
> > +++ b/drivers/hwmon/ina3221.c
> > @@ -77,6 +77,28 @@ enum ina3221_channels {
> > INA3221_NUM_CHANNELS
> > };
> > +enum ina3221_modes {
> > + INA3221_MODE_POWERDOWN,
> > + INA3221_MODE_ONESHOT,
> > + INA3221_MODE_CONTINUOUS,
> > + INA3221_NUM_MODES,
> > +};
> > +
> > +static const char *ina3221_mode_names[INA3221_NUM_MODES] = {
> > + [INA3221_MODE_POWERDOWN] = "power-down",
> > + [INA3221_MODE_ONESHOT] = "one-shot",
> > + [INA3221_MODE_CONTINUOUS] = "continuous",
> > +};
> > +
> > +static const u16 ina3221_mode_val[] = {
> > + [INA3221_MODE_POWERDOWN] = INA3221_CONFIG_MODE_POWERDOWN,
> > + [INA3221_MODE_ONESHOT] = INA3221_CONFIG_MODE_SHUNT |
> > +INA3221_CONFIG_MODE_BUS,
> > + [INA3221_MODE_CONTINUOUS] = INA3221_CONFIG_MODE_CONTINUOUS |
> > + INA3221_CONFIG_MODE_SHUNT |
> > + INA3221_CONFIG_MODE_BUS,
> > +};
> > +
> > /**
> >* struct ina3221_input - channel input source specific information
> >* @label: label of channel input source
> > @@ -386,9 +408,51 @@ static const struct hwmon_ops ina3221_hwmon_ops = {
> > .write = ina3221_write,
> > };
> > +static int ina3221_mode_get_index(struct device *dev, unsigned int *index)
> > +{
> > + struct ina3221_data *ina = dev_get_drvdata(dev);
> > + u16 mode = ina->reg_config & INA3221_CONFIG_MODE_MASK;
> > +
> > + if (mode == INA3221_CONFIG_MODE_POWERDOWN)
> > + *index = INA3221_MODE_POWERDOWN;
> > + if (mode & INA3221_CONFIG_MODE_CONTINUOUS)
> > + *index = INA3221_MODE_CONTINUOUS;
> > + else
> > + *index = INA3221_MODE_ONESHOT;
> > +
> > + return 0;
> > +}
> > +
> > +static int ina3221_mode_set_index(struct device *dev, unsigned int index)
> > +{
> > + struct ina3221_data *ina = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = regmap_update_bits(ina->regmap, INA3221_CONFIG,
> > +INA3221_CONFIG_MODE_MASK,
> > +ina3221_mode_val[index]);
> > + if (ret)
> > + return ret;
> > +
> > + /* Cache the latest config register value */
> > + ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static co
Re: [PATCH 2/2] hwmon: (ina3221) Add operating mode support
Hi Nicolin, On Wed, Oct 10, 2018 at 04:09:07PM -0700, Nicolin Chen wrote: > Hello Guenter, > > On Wed, Oct 10, 2018 at 06:22:39AM -0700, Guenter Roeck wrote: > > > > The hwmon core now has a new optional mode interface. So this patch > > > just implements this mode support so that user space can check and > > > configure via sysfs node its operating modes: power-down, one-shot, > > > and continuous modes. > > > One-shot mode on its own does not make sense or add value: It would require > > explicit driver support to trigger a reading, wait for the result, and > > report it back to the user. If the intent here is to have the user write the > > mode (which triggers the one-shot reading), wait a bit, and then read the > > results, that doesn't make sense because standard userspace applications > > won't know that. Also, that would be unsynchronized - one has to read the > > CVRF bit in the mask/enable register to know if the reading is complete. > > I think I oversimplified the one-shot mode here and you are right: > there should be a one-shot reading routine; the conversion time in > the configuration register also needs to be taken care of. > > > The effort to do all this using CPU cycles would in most if not all cases > > outweigh any perceived power savings. As such, I just don't see the > > practical use case. > > It really depends on the use case and how often the one-shot gets > triggered. For battery-powered devices, running in the continuous > mode does consume considerable power based on the measurement from > our power folks. If a system is running in a power sensitive mode, > while it still needs to occasionally check the inputs, it could be > a use case for one-shot mode, though it's purely a user decision. > That would actually be a use case for runtime power management. The power used by a modern sensor chip is miniscule compared to the power consumed by other components in the system, which may explain why no one bothered looking into runtime power management for sensors. This is also an argument against any device or subsystem specific solution: Users will want to be able to control power consumption for all devices in the system, not just for sensors. A device specific power control mechanism would, from user space perspective, be a nightmare. > > power-down mode effectively reinvents runtime power management (runtime > > suspend/resume support) and is thus simply unacceptable. > > Similar to one-shot, if a system is in a low power mode where it > doesn't want to check the inputs anymore, I feel the user space > could at least make the decision to turn on/off the chips, I am > not quite sure if the generic runtime PM system already has this > kind of support though. > Please look up "runtime power management". It provides the basic mechanism to turn off / disable a device if it is not used. The point here is that the basic mechanism is there, even though it may not be perfect. If it is not perfect, it needs to be improved. Implementing a per-subsystem alternate method would be the wrong approach. > > I am open to help explore adding support for runtime power management > > to the hwmon subsystem, but that would be less than straightforward and > > require an actual use case to warrant the effort. > > Is there any feasible solution from your point of view? > You mean to implement runtime suspend/resume for hwmon drivers, or some other approach ? As for implementing it in hwmon drivers, I don't know; I don't recall this ever coming up, and never thought about it. This is where the use case comes in - if done, it has to be done properly, which will require some thinking and a substantial amount of time. It simply does not make sense to spend time on that effort if there is no actual use case. Implementing an alternate mechanism is simply a no-go. Guenter
Re: [PATCH 2/2] hwmon: (ina3221) Add operating mode support
Hi Guenter, On Wed, Oct 10, 2018 at 04:43:00PM -0700, Guenter Roeck wrote: > > > The effort to do all this using CPU cycles would in most if not all cases > > > outweigh any perceived power savings. As such, I just don't see the > > > practical use case. > > > > It really depends on the use case and how often the one-shot gets > > triggered. For battery-powered devices, running in the continuous > > mode does consume considerable power based on the measurement from > > our power folks. If a system is running in a power sensitive mode, > > while it still needs to occasionally check the inputs, it could be > > a use case for one-shot mode, though it's purely a user decision. > > > That would actually be a use case for runtime power management. > The power used by a modern sensor chip is miniscule compared > to the power consumed by other components in the system, > which may explain why no one bothered looking into runtime > power management for sensors. > > This is also an argument against any device or subsystem specific solution: > Users will want to be able to control power consumption for all devices > in the system, not just for sensors. A device specific power control > mechanism would, from user space perspective, be a nightmare. That makes sense to me. Actually we wanted a solution more on the driver side so that user space doesn't need to be involved. But our downstream solution (switching different modes when certain number of CPUs get hot plugged in/out) wasn't accepted years ago by other subsystem maintainers, being commented that it's a user decision, IIRC. So I thought that having a sysfs node might be a generic way for "user decision". But I guess I should have tried to seek for a solution in the kernel like runtime PM as you said. > > > power-down mode effectively reinvents runtime power management (runtime > > > suspend/resume support) and is thus simply unacceptable. > > > > Similar to one-shot, if a system is in a low power mode where it > > doesn't want to check the inputs anymore, I feel the user space > > could at least make the decision to turn on/off the chips, I am > > not quite sure if the generic runtime PM system already has this > > kind of support though. > > > Please look up "runtime power management". It provides the basic > mechanism to turn off / disable a device if it is not used. The point > here is that the basic mechanism is there, even though it may not > be perfect. If it is not perfect, it needs to be improved. > Implementing a per-subsystem alternate method would be the wrong > approach. I have implemented runtime PM suspend/resume in other drivers but what I know is that it relies on someone to call get_sync and put functions accordingly and it doesn't seem to have built-in system level low-power or power-sensitive mode for those use cases that I mentioned previously. But I agree with your point and will take a closer look. One more question here, and this might sound a bit abuse of using the existing hwmon ABI: would it sound plausible to you that the driver powers down the chip when all three channels get disabled via in[123]_enable nodes? :) Thanks Nicolin
