[PATCH] hwmon (ina3221) Add single-shot mode support

[PATCH] hwmon (ina3221) Add single-shot mode support

[Nicolin Chen]

INA3221 supports both continuous and single-shot modes. When
running in the continuous mode, it keeps measuring the inputs
and converting them to the data register even if there are no
users reading the data out. In this use case, this could be a
power waste.

So this patch adds a single-shot mode support so that ina3221
could do measurement and conversion only if users trigger it,
depending on the use case where it only needs to poll data in
a lower frequency.

The change also exposes "mode" and "available_modes" nodes to
allow users to switch between two operating modes.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 Documentation/hwmon/ina3221 |   3 +
 drivers/hwmon/ina3221.c     | 109 ++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index 4b82cbfb551c..b03f4ad901ee 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -35,3 +35,6 @@ curr[123]_max           Warning alert current(mA) setting, activates the
                           average is above this value.
 curr[123]_max_alarm     Warning alert current limit exceeded
 in[456]_input           Shunt voltage(uV) for channels 1, 2, and 3 respectively
+available_modes         Available operating modes of the chip:
+                          continuous mode; single-shot mode
+mode                    Current operating mode of the chip
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 17a57dbc0424..8f7da3f75d53 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -91,6 +91,17 @@ enum ina3221_channels {
 	INA3221_NUM_CHANNELS
 };
 
+enum ina3221_modes {
+	INA3221_MODE_SINGLE_SHOT,
+	INA3221_MODE_CONTINUOUS,
+	INA3221_NUM_MODES,
+};
+
+static const char * const ina3221_mode_names[] = {
+	[INA3221_MODE_SINGLE_SHOT] = "single-shot",
+	[INA3221_MODE_CONTINUOUS] = "continuous",
+};
+
 /**
  * struct ina3221_input - channel input source specific information
  * @label: label of channel input source
@@ -127,6 +138,11 @@ static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
 	       (ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
 }
 
+static inline bool ina3221_is_singleshot_mode(struct ina3221_data *ina)
+{
+	return !(ina->reg_config & INA3221_CONFIG_MODE_CONTINUOUS);
+}
+
 /* Lookup table for Bus and Shunt conversion times in usec */
 static const u16 ina3221_conv_time[] = {
 	140, 204, 332, 588, 1100, 2116, 4156, 8244,
@@ -188,6 +204,11 @@ static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val)
 		if (!ina3221_is_enabled(ina, channel))
 			return -ENODATA;
 
+		/* Write CONFIG register to trigger a single-shot measurement */
+		if (ina3221_is_singleshot_mode(ina))
+			regmap_write(ina->regmap, INA3221_CONFIG,
+				     ina->reg_config);
+
 		ret = ina3221_wait_for_data(ina);
 		if (ret)
 			return ret;
@@ -232,6 +253,11 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
 		if (!ina3221_is_enabled(ina, channel))
 			return -ENODATA;
 
+		/* Write CONFIG register to trigger a single-shot measurement */
+		if (ina3221_is_singleshot_mode(ina))
+			regmap_write(ina->regmap, INA3221_CONFIG,
+				     ina->reg_config);
+
 		ret = ina3221_wait_for_data(ina);
 		if (ret)
 			return ret;
@@ -532,6 +558,82 @@ static ssize_t ina3221_set_shunt(struct device *dev,
 	return count;
 }
 
+static ssize_t available_modes_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ina3221_mode_names); i++)
+		snprintf(buf, PAGE_SIZE, "%s%s ", buf, ina3221_mode_names[i]);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", buf);
+}
+
+static ssize_t mode_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	int mode;
+
+	if (ina->reg_config & INA3221_CONFIG_MODE_CONTINUOUS)
+		mode = INA3221_MODE_CONTINUOUS;
+	else
+		mode = INA3221_MODE_SINGLE_SHOT;
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", ina3221_mode_names[mode]);
+}
+
+static ssize_t mode_store(struct device *dev,
+			  struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	u16 mask = INA3221_CONFIG_MODE_CONTINUOUS;
+	u16 continuous;
+	int mode, ret;
+	char name[32];
+
+	mutex_lock(&ina->lock);
+
+	snprintf(name, sizeof(name), "%s", buf);
+	strim(name);
+
+	for (mode = 0; mode < INA3221_NUM_MODES; mode++) {
+		if (!strcmp(name, ina3221_mode_names[mode]))
+			break;
+	}
+
+	switch (mode) {
+	case INA3221_MODE_SINGLE_SHOT:
+		continuous = 0;
+		break;
+	case INA3221_MODE_CONTINUOUS:
+		continuous = INA3221_CONFIG_MODE_CONTINUOUS;
+		break;
+	default:
+		mutex_unlock(&ina->lock);
+		return -EINVAL;
+	}
+
+	/* Set register to configure single-shot or continuous mode */
+	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, continuous);
+	if (ret) {
+		mutex_unlock(&ina->lock);
+		return ret;
+	}
+
+	/* Cache the latest config register value */
+	ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
+	if (ret) {
+		mutex_unlock(&ina->lock);
+		return ret;
+	}
+
+	mutex_unlock(&ina->lock);
+
+	return count;
+}
+
 /* shunt resistance */
 static SENSOR_DEVICE_ATTR(shunt1_resistor, S_IRUGO | S_IWUSR,
 		ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL1);
@@ -540,10 +642,17 @@ static SENSOR_DEVICE_ATTR(shunt2_resistor, S_IRUGO | S_IWUSR,
 static SENSOR_DEVICE_ATTR(shunt3_resistor, S_IRUGO | S_IWUSR,
 		ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL3);
 
+/* operating mode */
+static DEVICE_ATTR_RW(mode);
+static DEVICE_ATTR_RO(available_modes);
+
 static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_shunt1_resistor.dev_attr.attr,
 	&sensor_dev_attr_shunt2_resistor.dev_attr.attr,
 	&sensor_dev_attr_shunt3_resistor.dev_attr.attr,
+	/* common attributes */
+	&dev_attr_mode.attr,
+	&dev_attr_available_modes.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(ina3221);
-- 
2.17.1

Re: [PATCH] hwmon (ina3221) Add single-shot mode support

[Guenter Roeck]

On Mon, Nov 12, 2018 at 08:23:53PM -0800, Nicolin Chen wrote:
> INA3221 supports both continuous and single-shot modes. When
> running in the continuous mode, it keeps measuring the inputs
> and converting them to the data register even if there are no
> users reading the data out. In this use case, this could be a
> power waste.
> 
> So this patch adds a single-shot mode support so that ina3221
> could do measurement and conversion only if users trigger it,
> depending on the use case where it only needs to poll data in
> a lower frequency.
> 
> The change also exposes "mode" and "available_modes" nodes to
> allow users to switch between two operating modes.
> 
Lots and lots of complexity for little gain. Sorry, I don't see
the point of this change.

Guenter

> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>  Documentation/hwmon/ina3221 |   3 +
>  drivers/hwmon/ina3221.c     | 109 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
> index 4b82cbfb551c..b03f4ad901ee 100644
> --- a/Documentation/hwmon/ina3221
> +++ b/Documentation/hwmon/ina3221
> @@ -35,3 +35,6 @@ curr[123]_max           Warning alert current(mA) setting, activates the
>                            average is above this value.
>  curr[123]_max_alarm     Warning alert current limit exceeded
>  in[456]_input           Shunt voltage(uV) for channels 1, 2, and 3 respectively
> +available_modes         Available operating modes of the chip:
> +                          continuous mode; single-shot mode
> +mode                    Current operating mode of the chip
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 17a57dbc0424..8f7da3f75d53 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -91,6 +91,17 @@ enum ina3221_channels {
>  	INA3221_NUM_CHANNELS
>  };
>  
> +enum ina3221_modes {
> +	INA3221_MODE_SINGLE_SHOT,
> +	INA3221_MODE_CONTINUOUS,
> +	INA3221_NUM_MODES,
> +};
> +
> +static const char * const ina3221_mode_names[] = {
> +	[INA3221_MODE_SINGLE_SHOT] = "single-shot",
> +	[INA3221_MODE_CONTINUOUS] = "continuous",
> +};
> +
>  /**
>   * struct ina3221_input - channel input source specific information
>   * @label: label of channel input source
> @@ -127,6 +138,11 @@ static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
>  	       (ina->reg_config & INA3221_CONFIG_CHx_EN(channel));
>  }
>  
> +static inline bool ina3221_is_singleshot_mode(struct ina3221_data *ina)
> +{
> +	return !(ina->reg_config & INA3221_CONFIG_MODE_CONTINUOUS);
> +}
> +
>  /* Lookup table for Bus and Shunt conversion times in usec */
>  static const u16 ina3221_conv_time[] = {
>  	140, 204, 332, 588, 1100, 2116, 4156, 8244,
> @@ -188,6 +204,11 @@ static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val)
>  		if (!ina3221_is_enabled(ina, channel))
>  			return -ENODATA;
>  
> +		/* Write CONFIG register to trigger a single-shot measurement */
> +		if (ina3221_is_singleshot_mode(ina))
> +			regmap_write(ina->regmap, INA3221_CONFIG,
> +				     ina->reg_config);
> +
>  		ret = ina3221_wait_for_data(ina);
>  		if (ret)
>  			return ret;
> @@ -232,6 +253,11 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
>  		if (!ina3221_is_enabled(ina, channel))
>  			return -ENODATA;
>  
> +		/* Write CONFIG register to trigger a single-shot measurement */
> +		if (ina3221_is_singleshot_mode(ina))
> +			regmap_write(ina->regmap, INA3221_CONFIG,
> +				     ina->reg_config);
> +
>  		ret = ina3221_wait_for_data(ina);
>  		if (ret)
>  			return ret;
> @@ -532,6 +558,82 @@ static ssize_t ina3221_set_shunt(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t available_modes_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ina3221_mode_names); i++)
> +		snprintf(buf, PAGE_SIZE, "%s%s ", buf, ina3221_mode_names[i]);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", buf);
> +}
> +
> +static ssize_t mode_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	int mode;
> +
> +	if (ina->reg_config & INA3221_CONFIG_MODE_CONTINUOUS)
> +		mode = INA3221_MODE_CONTINUOUS;
> +	else
> +		mode = INA3221_MODE_SINGLE_SHOT;
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", ina3221_mode_names[mode]);
> +}
> +
> +static ssize_t mode_store(struct device *dev,
> +			  struct device_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	u16 mask = INA3221_CONFIG_MODE_CONTINUOUS;
> +	u16 continuous;
> +	int mode, ret;
> +	char name[32];
> +
> +	mutex_lock(&ina->lock);
> +
> +	snprintf(name, sizeof(name), "%s", buf);
> +	strim(name);
> +
> +	for (mode = 0; mode < INA3221_NUM_MODES; mode++) {
> +		if (!strcmp(name, ina3221_mode_names[mode]))
> +			break;
> +	}
> +
> +	switch (mode) {
> +	case INA3221_MODE_SINGLE_SHOT:
> +		continuous = 0;
> +		break;
> +	case INA3221_MODE_CONTINUOUS:
> +		continuous = INA3221_CONFIG_MODE_CONTINUOUS;
> +		break;
> +	default:
> +		mutex_unlock(&ina->lock);
> +		return -EINVAL;
> +	}
> +
> +	/* Set register to configure single-shot or continuous mode */
> +	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, continuous);
> +	if (ret) {
> +		mutex_unlock(&ina->lock);
> +		return ret;
> +	}
> +
> +	/* Cache the latest config register value */
> +	ret = regmap_read(ina->regmap, INA3221_CONFIG, &ina->reg_config);
> +	if (ret) {
> +		mutex_unlock(&ina->lock);
> +		return ret;
> +	}
> +
> +	mutex_unlock(&ina->lock);
> +
> +	return count;
> +}
> +
>  /* shunt resistance */
>  static SENSOR_DEVICE_ATTR(shunt1_resistor, S_IRUGO | S_IWUSR,
>  		ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL1);
> @@ -540,10 +642,17 @@ static SENSOR_DEVICE_ATTR(shunt2_resistor, S_IRUGO | S_IWUSR,
>  static SENSOR_DEVICE_ATTR(shunt3_resistor, S_IRUGO | S_IWUSR,
>  		ina3221_show_shunt, ina3221_set_shunt, INA3221_CHANNEL3);
>  
> +/* operating mode */
> +static DEVICE_ATTR_RW(mode);
> +static DEVICE_ATTR_RO(available_modes);
> +
>  static struct attribute *ina3221_attrs[] = {
>  	&sensor_dev_attr_shunt1_resistor.dev_attr.attr,
>  	&sensor_dev_attr_shunt2_resistor.dev_attr.attr,
>  	&sensor_dev_attr_shunt3_resistor.dev_attr.attr,
> +	/* common attributes */
> +	&dev_attr_mode.attr,
> +	&dev_attr_available_modes.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(ina3221);
> -- 
> 2.17.1
> 

Re: [PATCH] hwmon (ina3221) Add single-shot mode support

[Nicolin Chen]

Hi Guenter,

On Mon, Nov 12, 2018 at 08:32:48PM -0800, Guenter Roeck wrote:
> On Mon, Nov 12, 2018 at 08:23:53PM -0800, Nicolin Chen wrote:
> > INA3221 supports both continuous and single-shot modes. When
> > running in the continuous mode, it keeps measuring the inputs
> > and converting them to the data register even if there are no
> > users reading the data out. In this use case, this could be a
> > power waste.
> > 
> > So this patch adds a single-shot mode support so that ina3221
> > could do measurement and conversion only if users trigger it,
> > depending on the use case where it only needs to poll data in
> > a lower frequency.
> > 
> > The change also exposes "mode" and "available_modes" nodes to
> > allow users to switch between two operating modes.
> > 
> Lots and lots of complexity for little gain. Sorry, I don't see
> the point of this change.

The chip is causing considerable power waste on battery-powered
devices so we typically use it running in the single-shot mode.

Although the chip now can be powered down, but we still need to
occasionally poll it for power measurement and critical alerts,
so single-shot mode is the best choice for us, considering that
the power-down-and-up routine would be way heavier.

I could understand that you don't really like it, but it's some
feature that we truly need. Do you have any suggestion to write
the code that can make it more convincing to you?

Thanks
Nicolin

Re: [PATCH] hwmon (ina3221) Add single-shot mode support

[Guenter Roeck]

On Mon, Nov 12, 2018 at 08:58:24PM -0800, Nicolin Chen wrote:
> Hi Guenter,
> 
> On Mon, Nov 12, 2018 at 08:32:48PM -0800, Guenter Roeck wrote:
> > On Mon, Nov 12, 2018 at 08:23:53PM -0800, Nicolin Chen wrote:
> > > INA3221 supports both continuous and single-shot modes. When
> > > running in the continuous mode, it keeps measuring the inputs
> > > and converting them to the data register even if there are no
> > > users reading the data out. In this use case, this could be a
> > > power waste.
> > > 
> > > So this patch adds a single-shot mode support so that ina3221
> > > could do measurement and conversion only if users trigger it,
> > > depending on the use case where it only needs to poll data in
> > > a lower frequency.
> > > 
> > > The change also exposes "mode" and "available_modes" nodes to
> > > allow users to switch between two operating modes.
> > > 
> > Lots and lots of complexity for little gain. Sorry, I don't see
> > the point of this change.
> 
> The chip is causing considerable power waste on battery-powered
> devices so we typically use it running in the single-shot mode.
> 

And you need to be able to do that with a sysfs attribute ?
Are you planning to have some code switching back and forth
between the modes ?

You'll need to provide a good rationale why this needs to be
runtime configurable.

Guenter

> Although the chip now can be powered down, but we still need to
> occasionally poll it for power measurement and critical alerts,
> so single-shot mode is the best choice for us, considering that
> the power-down-and-up routine would be way heavier.
> 
> I could understand that you don't really like it, but it's some
> feature that we truly need. Do you have any suggestion to write
> the code that can make it more convincing to you?
> 
> Thanks
> Nicolin

Re: [PATCH] hwmon (ina3221) Add single-shot mode support

[Nicolin Chen]

Hi Guenter,

On Tue, Nov 13, 2018 at 09:21:02AM -0800, Guenter Roeck wrote:
> > > > INA3221 supports both continuous and single-shot modes. When
> > > > running in the continuous mode, it keeps measuring the inputs
> > > > and converting them to the data register even if there are no
> > > > users reading the data out. In this use case, this could be a
> > > > power waste.
> > > > 
> > > > So this patch adds a single-shot mode support so that ina3221
> > > > could do measurement and conversion only if users trigger it,
> > > > depending on the use case where it only needs to poll data in
> > > > a lower frequency.
> > > > 
> > > > The change also exposes "mode" and "available_modes" nodes to
> > > > allow users to switch between two operating modes.
> > > > 
> > > Lots and lots of complexity for little gain. Sorry, I don't see
> > > the point of this change.
> > 
> > The chip is causing considerable power waste on battery-powered
> > devices so we typically use it running in the single-shot mode.
> 
> And you need to be able to do that with a sysfs attribute ?
> Are you planning to have some code switching back and forth
> between the modes ?
>
> You'll need to provide a good rationale why this needs to be
> runtime configurable.

Honestly, our old downstream driver didn't expose it via sysfs.
Instead, it had a built-in "governor" to switch modes based on
the CPU hotplug state and cpufreq. However, the interface used
to register a CPU hotplug notification was already deprecated.
And I don't feel this governor is generic enough to be present
in the mainline code.

For me, it's not that necessary to be a sysfs attribute. I try
to add it merely because I cannot find a good criteria for the
mode switching in a hwmon driver. So having an open sysfs node
may allow user space power daemon to decide its operating mode,
since it knows which power mode the system is running at: full
speed (charging/charged) or power saving (on-battery), and it
knows how often this exact service will poll the sensor data.

An alternative way (without the sysfs node), after looking at
other hwmon code, could be to have a timed polling thread and
read data using an update_interval value from ABI. This might
turn out to be more complicated as it'll also involve settings
of hardware averaging and conversion time. Above all, I cannot
figure out a good threshold of update_interval to switch modes.

If you can give some advice of a better implementation, that'd
be great.

Thanks
Nicolin

Re: [PATCH] hwmon (ina3221) Add single-shot mode support

[Guenter Roeck]

On Tue, Nov 13, 2018 at 04:11:42PM -0800, Nicolin Chen wrote:
> Hi Guenter,
> 
> On Tue, Nov 13, 2018 at 09:21:02AM -0800, Guenter Roeck wrote:
> > > > > INA3221 supports both continuous and single-shot modes. When
> > > > > running in the continuous mode, it keeps measuring the inputs
> > > > > and converting them to the data register even if there are no
> > > > > users reading the data out. In this use case, this could be a
> > > > > power waste.
> > > > > 
> > > > > So this patch adds a single-shot mode support so that ina3221
> > > > > could do measurement and conversion only if users trigger it,
> > > > > depending on the use case where it only needs to poll data in
> > > > > a lower frequency.
> > > > > 
> > > > > The change also exposes "mode" and "available_modes" nodes to
> > > > > allow users to switch between two operating modes.
> > > > > 
> > > > Lots and lots of complexity for little gain. Sorry, I don't see
> > > > the point of this change.
> > > 
> > > The chip is causing considerable power waste on battery-powered
> > > devices so we typically use it running in the single-shot mode.
> > 
> > And you need to be able to do that with a sysfs attribute ?
> > Are you planning to have some code switching back and forth
> > between the modes ?
> >
> > You'll need to provide a good rationale why this needs to be
> > runtime configurable.
> 
> Honestly, our old downstream driver didn't expose it via sysfs.
> Instead, it had a built-in "governor" to switch modes based on
> the CPU hotplug state and cpufreq. However, the interface used
> to register a CPU hotplug notification was already deprecated.
> And I don't feel this governor is generic enough to be present
> in the mainline code.
> 
> For me, it's not that necessary to be a sysfs attribute. I try
> to add it merely because I cannot find a good criteria for the
> mode switching in a hwmon driver. So having an open sysfs node
> may allow user space power daemon to decide its operating mode,
> since it knows which power mode the system is running at: full
> speed (charging/charged) or power saving (on-battery), and it
> knows how often this exact service will poll the sensor data.
> 
That is bad, because it is not a generic implementation.
Userspace would have to account for each individual driver
one by one.

> An alternative way (without the sysfs node), after looking at
> other hwmon code, could be to have a timed polling thread and
> read data using an update_interval value from ABI. This might
> turn out to be more complicated as it'll also involve settings
> of hardware averaging and conversion time. Above all, I cannot
> figure out a good threshold of update_interval to switch modes.
> 
update_interval should only be used if it can be configured
into hardware, not to trigger a polling thread. It should only
be used in the driver to determine caching intervals.

> If you can give some advice of a better implementation, that'd
> be great.
> 
From your description, the only real feasible solution would be a
generic one, with a well defined interface either to userspace or
to the kernel. The best I can think of would be an in-kernel API
setting the power operational mode via callbacks. Alternative would
be a generic ability to set power operational mode from userspace,
using an ABI which applies to all drivers, not just one.

I don't know if any of those interfaces exists. If not, this will
require a discussion with upstream kernel maintainers, maybe with
a strawman proposal (set of patches). We can't just implement a
driver specific solution.

Guenter

Re: [PATCH] hwmon (ina3221) Add single-shot mode support

[Nicolin Chen]

Hello Guenter,

On Wed, Nov 14, 2018 at 09:23:30AM -0800, Guenter Roeck wrote:
> > An alternative way (without the sysfs node), after looking at
> > other hwmon code, could be to have a timed polling thread and
> > read data using an update_interval value from ABI. This might
> > turn out to be more complicated as it'll also involve settings
> > of hardware averaging and conversion time. Above all, I cannot
> > figure out a good threshold of update_interval to switch modes.
> > 
> update_interval should only be used if it can be configured
> into hardware, not to trigger a polling thread. It should only
> be used in the driver to determine caching intervals.

I see..so it's more like the conversion time settings instead.

> > If you can give some advice of a better implementation, that'd
> > be great.
> > 
> From your description, the only real feasible solution would be a
> generic one, with a well defined interface either to userspace or
> to the kernel. The best I can think of would be an in-kernel API
> setting the power operational mode via callbacks. Alternative would
> be a generic ability to set power operational mode from userspace,
> using an ABI which applies to all drivers, not just one.

Hmm. That would make the situation really complicated, I could
understand your concern though.

I searched a little and found that, while the initial ina3221
hwmon driver was under review, Laxman submitted an IIO version
where Jonathan had a similar comments against its "mode" node:
https://www.spinics.net/lists/linux-hwmon/msg00219.html

Quote from his comments {
 * There is a lot of ABI in here concerned with oneshot vs continuous.
 This seems to me to be more than it should be. We wouldn't expect to
 see stuff changing as a result of switching between these modes other
 than wrt to when the data shows up.  So I'd expect to not see this
 directly exposed at all - but rather sit in oneshot unless either:
 1) Buffered mode is running (not currently supported)
 2) Alerts are on - which I think requires it to be in continuous mode.
}

Since hwmon driver doesn't support buffered mode, what do you
think about having the chip run in single-shot mode by default
but changing it if alerts are on? Though the driver also needs
to support to enable warning/critical alert pins.

In short, other than exposing it via a generic ABI to the user
space, how about defining some policy to maintaining it within
the driver?

Thanks
Nicolin

Re: [PATCH] hwmon (ina3221) Add single-shot mode support

[Guenter Roeck]

On Fri, Nov 16, 2018 at 05:51:34PM -0800, Nicolin Chen wrote:
> Hello Guenter,
> 
> On Wed, Nov 14, 2018 at 09:23:30AM -0800, Guenter Roeck wrote:
> > > An alternative way (without the sysfs node), after looking at
> > > other hwmon code, could be to have a timed polling thread and
> > > read data using an update_interval value from ABI. This might
> > > turn out to be more complicated as it'll also involve settings
> > > of hardware averaging and conversion time. Above all, I cannot
> > > figure out a good threshold of update_interval to switch modes.
> > > 
> > update_interval should only be used if it can be configured
> > into hardware, not to trigger a polling thread. It should only
> > be used in the driver to determine caching intervals.
> 
> I see..so it's more like the conversion time settings instead.
> 
> > > If you can give some advice of a better implementation, that'd
> > > be great.
> > > 
> > From your description, the only real feasible solution would be a
> > generic one, with a well defined interface either to userspace or
> > to the kernel. The best I can think of would be an in-kernel API
> > setting the power operational mode via callbacks. Alternative would
> > be a generic ability to set power operational mode from userspace,
> > using an ABI which applies to all drivers, not just one.
> 
> Hmm. That would make the situation really complicated, I could
> understand your concern though.
> 
> I searched a little and found that, while the initial ina3221
> hwmon driver was under review, Laxman submitted an IIO version
> where Jonathan had a similar comments against its "mode" node:
> https://www.spinics.net/lists/linux-hwmon/msg00219.html
> 
> Quote from his comments {
>  * There is a lot of ABI in here concerned with oneshot vs continuous.
>  This seems to me to be more than it should be. We wouldn't expect to
>  see stuff changing as a result of switching between these modes other
>  than wrt to when the data shows up.  So I'd expect to not see this
>  directly exposed at all - but rather sit in oneshot unless either:
>  1) Buffered mode is running (not currently supported)
>  2) Alerts are on - which I think requires it to be in continuous mode.
> }
> 
> Since hwmon driver doesn't support buffered mode, what do you
> think about having the chip run in single-shot mode by default
> but changing it if alerts are on? Though the driver also needs
> to support to enable warning/critical alert pins.
> 
> In short, other than exposing it via a generic ABI to the user
> space, how about defining some policy to maintaining it within
> the driver?
> 
I think that would be a bad idea. It changes timing for everyone
curently using the driver. It also effectively disables monitoring,
which is the main purpose for using this chip (and other hardware
monitoring chips). This is indeed a key difference between iio
and hwmon - the main purpose of chips in the iio subsystem is to
be able report data efficiently to user space, not hardware monitoring.
I do not think it is appropriate to use iio requirements as argument
to change hwmon driver behavior (and vice versa).

Guenter

Re: [PATCH] hwmon (ina3221) Add single-shot mode support

[Nicolin Chen]

On Mon, Nov 19, 2018 at 09:45:59AM -0800, Guenter Roeck wrote:
> > In short, other than exposing it via a generic ABI to the user
> > space, how about defining some policy to maintaining it within
> > the driver?

> I think that would be a bad idea. It changes timing for everyone
> curently using the driver. It also effectively disables monitoring,
> which is the main purpose for using this chip (and other hardware
> monitoring chips). This is indeed a key difference between iio
> and hwmon - the main purpose of chips in the iio subsystem is to
> be able report data efficiently to user space, not hardware monitoring.
> I do not think it is appropriate to use iio requirements as argument
> to change hwmon driver behavior (and vice versa).

OK...what about setting a default mode via DT? I didn't expect it
to be possible until I found this existing solution. Though it is
still in an iio driver, I don't think this should be iio specific.

  commit 023e30fb0d3a3b9d6b8dc9e47590aa544d58a22f
  Author: Adriana Reus <adriana.reus@intel.com>
  Date:   Tue Nov 24 12:59:49 2015 +0200

    Documentation: devicetree: Add property for controlling power saving
                   mode for the us5182 als sensor

    Add a property to allow changing the default power-saving mode.
    By default, at read raw the chip will activate and provide
    one measurent, then it will shut itself down. However, the
    chip can also work in "continuous" mode which may be more reliable
    but is also more power consuming.

    Signed-off-by: Adriana Reus <adriana.reus@intel.com>
    Acked-by: Rob Herring <robh@kernel.org>
    Signed-off-by: Jonathan Cameron <jic23@kernel.org>

Would it be possible for me to apply a similar one? Since hwmon
driver uses continuous mode by default, I will add a "one-shot"
property instead of "continuous" -- all existing users won't be
effected unless they place one-shot properties in DT bindings.

Thanks
Nicolin

Re: [PATCH] hwmon (ina3221) Add single-shot mode support

[Guenter Roeck]

On 11/19/18 2:18 PM, Nicolin Chen wrote:
> On Mon, Nov 19, 2018 at 09:45:59AM -0800, Guenter Roeck wrote:
>>> In short, other than exposing it via a generic ABI to the user
>>> space, how about defining some policy to maintaining it within
>>> the driver?
> 
>> I think that would be a bad idea. It changes timing for everyone
>> curently using the driver. It also effectively disables monitoring,
>> which is the main purpose for using this chip (and other hardware
>> monitoring chips). This is indeed a key difference between iio
>> and hwmon - the main purpose of chips in the iio subsystem is to
>> be able report data efficiently to user space, not hardware monitoring.
>> I do not think it is appropriate to use iio requirements as argument
>> to change hwmon driver behavior (and vice versa).
> 
> OK...what about setting a default mode via DT? I didn't expect it
> to be possible until I found this existing solution. Though it is
> still in an iio driver, I don't think this should be iio specific.
> 
>    commit 023e30fb0d3a3b9d6b8dc9e47590aa544d58a22f
>    Author: Adriana Reus <adriana.reus@intel.com>
>    Date:   Tue Nov 24 12:59:49 2015 +0200
> 
>      Documentation: devicetree: Add property for controlling power saving
>                     mode for the us5182 als sensor
> 
>      Add a property to allow changing the default power-saving mode.
>      By default, at read raw the chip will activate and provide
>      one measurent, then it will shut itself down. However, the
>      chip can also work in "continuous" mode which may be more reliable
>      but is also more power consuming.
> 
>      Signed-off-by: Adriana Reus <adriana.reus@intel.com>
>      Acked-by: Rob Herring <robh@kernel.org>
>      Signed-off-by: Jonathan Cameron <jic23@kernel.org>
> 
> Would it be possible for me to apply a similar one? Since hwmon
> driver uses continuous mode by default, I will add a "one-shot"
> property instead of "continuous" -- all existing users won't be
> effected unless they place one-shot properties in DT bindings.
> 

I would accept such a patch, but it would for all practical purposes
disable the monitoring of limits. Make sure that is well documented.

Guenter