hwmon (ina3221) Add single-shot mode support

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 <[email protected]>

---
 Documentation/hwmon/ina3221 |   3 +
 drivers/hwmon/ina3221.c     | 109 ++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

Documentation/hwmon/ina3221 +3 -0

index 4b82cbfb551c..b03f4ad901ee 100644
@@ -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

drivers/hwmon/ina3221.c +109 -0

index 17a57dbc0424..8f7da3f75d53 100644
@@ -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);

Signature · 1 line

2.17.1

Quoted 11 lines · 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.

Quoted 3 lines

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.

Hi Guenter,

Quoted 12 lines · Guenter Roeck

Quoted 11 lines · 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.

Quoted 5 lines

Quoted 3 lines

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

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Hi Guenter,

Quoted 20 lines · Guenter Roeck

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

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.

Hello Guenter,

Quoted 10 lines · Guenter Roeck

Quoted 7 lines

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.

Quoted 9 lines

Quoted 3 lines

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?

Quoted 3 lines

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).

Quoted 4 lines · Guenter Roeck

Quoted 3 lines

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?

Quoted 8 lines

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 <[email protected]>
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 <[email protected]>

Acked-by Rob Herring <[email protected]>

Signed-off-by Jonathan Cameron <[email protected]>

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.