[PATCH 0/2] hwmon: Add operating mode support for core and ina3221

Add operating mode support in the core and ina3221.

Nicolin Chen (2):
  hwmon: (core) Add hwmon_mode structure and mode sysfs node
  hwmon: (ina3221) Add operating mode support

 Documentation/hwmon/sysfs-interface | 15 +++++
 drivers/hwmon/hwmon.c               | 87 ++++++++++++++++++++++++++---
 drivers/hwmon/ina3221.c             | 64 +++++++++++++++++++++
 include/linux/hwmon.h               | 24 ++++++++
 4 files changed, 183 insertions(+), 7 deletions(-)

-- 
2.17.1

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 <nicoleotsuka@gmail.com>
---
 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
+
 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(struct kobject *kobj,
+				    struct attribute *attr, int n)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
+	struct hwmon_device *hwdev = to_hwmon_device(dev);
 
-	if (to_hwmon_device(dev)->name == NULL)
-		return 0;
+	if (n == 0 && hwdev->name)
+		return attr->mode;
+	else if (n <= 2 && hwdev->chip->mode)
+		return attr->mode;
 
-	return attr->mode;
+	return 0;
 }
 
 static const struct attribute_group hwmon_dev_attr_group = {
 	.attrs		= hwmon_dev_attrs,
-	.is_visible	= hwmon_dev_name_is_visible,
+	.is_visible	= hwmon_dev_is_visible,
 };
 
 static const struct attribute_group *hwmon_dev_attr_groups[] = {

@@ -591,6 +654,16 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 		struct attribute **attrs;
 		int ngroups = 2; /* terminating NULL plus &hwdev->groups */
 
+		/* Validate optional hwmon_mode */
+		if (chip->mode) {
+			/* Check mandatory properties */
+			if (!chip->mode->names || !chip->mode->list_size ||
+			    !chip->mode->get_index || !chip->mode->set_index) {
+				err = -EINVAL;
+				goto free_hwmon;
+			}
+		}
+
 		if (groups)
 			for (i = 0; groups[i]; i++)
 				ngroups++;

diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 99e0c1b0b5fb..06c1940ca98b 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h

@@ -365,14 +365,38 @@ struct hwmon_channel_info {
 	const u32 *config;
 };
 
+/**
+ * Chip operating mode information
+ * @names:	A list of available operating mode names.
+ * @list_size:	The total number of available operating mode names.
+ * @get_index:	Callback to get current operating mode index. Mandatory.
+ *		Parameters are:
+ *		@dev:	Pointer to hardware monitoring device
+ *		@index:	Pointer to returned mode index
+ *		The function returns 0 on success or a negative error number.
+ * @set_index:	Callback to set operating mode using the index. Mandatory.
+ *		Parameters are:
+ *		@dev:	Pointer to hardware monitoring device
+ *		@index:	Mode index in the mode list
+ *		The function returns 0 on success or a negative error number.
+ */
+struct hwmon_mode {
+	const char **names;
+	unsigned int list_size;
+	int (*get_index)(struct device *dev, unsigned int *index);
+	int (*set_index)(struct device *dev, unsigned int index);
+};
+
 /**
  * Chip configuration
  * @ops:	Pointer to hwmon operations.
  * @info:	Null-terminated list of channel information.
+ * @mode:	Pointer to hwmon operating mode (optional).
  */
 struct hwmon_chip_info {
 	const struct hwmon_ops *ops;
 	const struct hwmon_channel_info **info;
+	const struct hwmon_mode *mode;
 };
 
 /* hwmon_device_register() is deprecated */

-- 
2.17.1

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.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 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 */

-- 
2.17.1

Hi Nicolin,

On 10/09/2018 09:33 PM, Nicolin Chen wrote:

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

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

Hi Guenter,

On Wed, Oct 10, 2018 at 06:08:30AM -0700, Guenter Roeck wrote:

quoted

+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

quoted

  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(struct kobject *kobj,
+				    struct attribute *attr, int n)
  {
  	struct device *dev = container_of(kobj, struct device, kobj);
+	struct hwmon_device *hwdev = to_hwmon_device(dev);
-	if (to_hwmon_device(dev)->name == NULL)
-		return 0;
+	if (n == 0 && hwdev->name)
+		return attr->mode;
+	else if (n <= 2 && hwdev->chip->mode)
+		return attr->mode;
-	return attr->mode;
+	return 0;
  }
  static const struct attribute_group hwmon_dev_attr_group = {
  	.attrs		= hwmon_dev_attrs,
-	.is_visible	= hwmon_dev_name_is_visible,
+	.is_visible	= hwmon_dev_is_visible,
  };
  static const struct attribute_group *hwmon_dev_attr_groups[] = {

@@ -591,6 +654,16 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
  		struct attribute **attrs;
  		int ngroups = 2; /* terminating NULL plus &hwdev->groups */
+		/* Validate optional hwmon_mode */
+		if (chip->mode) {
+			/* Check mandatory properties */
+			if (!chip->mode->names || !chip->mode->list_size ||
+			    !chip->mode->get_index || !chip->mode->set_index) {
+				err = -EINVAL;
+				goto free_hwmon;
+			}
+		}
+
  		if (groups)
  			for (i = 0; groups[i]; i++)
  				ngroups++;

diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 99e0c1b0b5fb..06c1940ca98b 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h

@@ -365,14 +365,38 @@ struct hwmon_channel_info {
  	const u32 *config;
  };
+/**
+ * Chip operating mode information
+ * @names:	A list of available operating mode names.
+ * @list_size:	The total number of available operating mode names.
+ * @get_index:	Callback to get current operating mode index. Mandatory.
+ *		Parameters are:
+ *		@dev:	Pointer to hardware monitoring device
+ *		@index:	Pointer to returned mode index
+ *		The function returns 0 on success or a negative error number.
+ * @set_index:	Callback to set operating mode using the index. Mandatory.
+ *		Parameters are:
+ *		@dev:	Pointer to hardware monitoring device
+ *		@index:	Mode index in the mode list
+ *		The function returns 0 on success or a negative error number.
+ */
+struct hwmon_mode {
+	const char **names;
+	unsigned int list_size;
+	int (*get_index)(struct device *dev, unsigned int *index);
+	int (*set_index)(struct device *dev, unsigned int index);
+};
+
  /**
   * Chip configuration
   * @ops:	Pointer to hwmon operations.
   * @info:	Null-terminated list of channel information.
+ * @mode:	Pointer to hwmon operating mode (optional).
   */
  struct hwmon_chip_info {
  	const struct hwmon_ops *ops;
  	const struct hwmon_channel_info **info;
+	const struct hwmon_mode *mode;
  };
  /* hwmon_device_register() is deprecated */

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:

quoted

quoted

+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
+

quoted

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.

quoted

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

Hello Guenter,

On Wed, Oct 10, 2018 at 06:22:39AM -0700, Guenter Roeck wrote:

quoted

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

quoted

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
  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 */

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:

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

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

Hi Guenter,

On Wed, Oct 10, 2018 at 04:43:00PM -0700, Guenter Roeck wrote:

quoted

quoted

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.

quoted

quoted

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

On Wed, Oct 10, 2018 at 05:24:12PM -0700, Nicolin Chen wrote:

Hi Guenter,

On Wed, Oct 10, 2018 at 04:43:00PM -0700, Guenter Roeck wrote:

quoted

quoted

quoted

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.

quoted

quoted

quoted

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? :)

I would not call that an abuse, no.

Guenter

On Thu, Oct 11, 2018 at 12:31:52PM -0700, Guenter Roeck wrote:

quoted

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? :)

I would not call that an abuse, no.

Hmm..do you mean that you aren't in favor of powering down the chip
after all channels get disabled?

I was thinking about having pm_runtime_get_sync()/put() for channel
enabling/disabling routine of in[123]_enable.

Thanks
Nicolin

Hi Nicolin,

On Thu, Oct 11, 2018 at 12:36:59PM -0700, Nicolin Chen wrote:

On Thu, Oct 11, 2018 at 12:31:52PM -0700, Guenter Roeck wrote:

quoted

quoted

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? :)

I would not call that an abuse, no.

Hmm..do you mean that you aren't in favor of powering down the chip
after all channels get disabled?

No, I was trying to say that I would be ok with powering down the chip.

I was thinking about having pm_runtime_get_sync()/put() for channel
enabling/disabling routine of in[123]_enable.

Not sure if that would work. It might end up waking the chip when a
sysfs attribute is accessed. It might be worth a try, though.

It might also be possible to utilize userspace runtime attributes,
like setting runtime_enabled and setting the idle time before the sensor
shuts down. It would probably be necessary to implement not only
activating the sensor, though - we would also need to to ensure that
the first reading after activation waits until the first read is
complete.

Guenter

On Thu, Oct 11, 2018 at 12:50:25PM -0700, Guenter Roeck wrote:

Hi Nicolin,

On Thu, Oct 11, 2018 at 12:36:59PM -0700, Nicolin Chen wrote:

quoted

On Thu, Oct 11, 2018 at 12:31:52PM -0700, Guenter Roeck wrote:

quoted

quoted

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? :)

I would not call that an abuse, no.

Hmm..do you mean that you aren't in favor of powering down the chip
after all channels get disabled?

No, I was trying to say that I would be ok with powering down the chip.

Great!

quoted

I was thinking about having pm_runtime_get_sync()/put() for channel
enabling/disabling routine of in[123]_enable.

Not sure if that would work. It might end up waking the chip when a
sysfs attribute is accessed. It might be worth a try, though.

It might also be possible to utilize userspace runtime attributes,
like setting runtime_enabled and setting the idle time before the sensor
shuts down. It would probably be necessary to implement not only
activating the sensor, though - we would also need to to ensure that
the first reading after activation waits until the first read is
complete.

That's true. Thanks for the input!

Nicolin