[PATCH 0/2] hwmon: ina3221: Add power and enable sysfs nodes

[PATCH 0/2] hwmon: ina3221: Add power and enable sysfs nodes

[Nicolin Chen]

This series of patch is to add three sets of sysfs nodes for ina3221
hwmon driver. PATCH-1 adds two power sysfs nodes while PATCH-2 adds
an enable node.

Note: both two patches are rebased upon the following patch:
        "hwmon: ina3221: Read channel input source info from DT"
      As this patch is still under review (waiting for an ack from DT
      binding side), these two patches can only go through review at
      this point -- needs to wait for the patch above getting merged
      first.

Nicolin Chen (2):
  hwmon: ina3221: Add power sysfs nodes
  hwmon: ina3221: Add enable sysfs nodes

 Documentation/hwmon/ina3221 |   5 +
 drivers/hwmon/ina3221.c     | 228 +++++++++++++++++++++++++++++++++---
 2 files changed, 218 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH 1/2] hwmon: ina3221: Add power sysfs nodes

[Nicolin Chen]

The hwmon sysfs ABI supports powerX_input and powerX_crit. This
can ease user space programs who care more about power in total
than voltage or current individually.

So this patch adds these two sysfs nodes for INA3221 driver.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 Documentation/hwmon/ina3221 |   4 +
 drivers/hwmon/ina3221.c     | 145 ++++++++++++++++++++++++++++++++----
 2 files changed, 133 insertions(+), 16 deletions(-)

diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index 2726038be5bd..6be64b553cd0 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -34,3 +34,7 @@ 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
+power[123]_input        Power(uW) measurement channels
+power[123]_crit         Critical alert power(uW) setting, activates the
+                          corresponding alarm when the respective power
+                          is above this value
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index ce4d1f55e9cd..5890a2da3bfe 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -38,6 +38,8 @@
 #define INA3221_WARN3			0x0c
 #define INA3221_MASK_ENABLE		0x0f
 
+#define INA3221_BUS(x)			(INA3221_BUS1 + ((x) << 1))
+
 #define INA3221_CONFIG_MODE_SHUNT	BIT(1)
 #define INA3221_CONFIG_MODE_BUS		BIT(2)
 #define INA3221_CONFIG_MODE_CONTINUOUS	BIT(3)
@@ -172,43 +174,50 @@ static ssize_t ina3221_show_shunt_voltage(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", voltage_uv);
 }
 
-static ssize_t ina3221_show_current(struct device *dev,
-				    struct device_attribute *attr, char *buf)
+static int __ina3221_show_current(struct ina3221_data *ina,
+				  unsigned int reg, int *current_ma)
 {
-	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
-	struct ina3221_data *ina = dev_get_drvdata(dev);
-	unsigned int reg = sd_attr->index;
 	unsigned int channel = register_channel[reg];
 	struct ina3221_input *input = &ina->inputs[channel];
 	int resistance_uo = input->shunt_resistor;
-	int val, current_ma, voltage_nv, ret;
+	int val, voltage_nv, ret;
 
+	/* Read bus shunt voltage */
 	ret = ina3221_read_value(ina, reg, &val);
 	if (ret)
 		return ret;
+
+	/* LSB (4th bit) is 40 uV (40000 nV) */
 	voltage_nv = val * 40000;
 
-	current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
+	*current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", current_ma);
+	return 0;
 }
 
-static ssize_t ina3221_set_current(struct device *dev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t count)
+static ssize_t ina3221_show_current(struct device *dev,
+				    struct device_attribute *attr, char *buf)
 {
 	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	unsigned int reg = sd_attr->index;
-	unsigned int channel = register_channel[reg];
-	struct ina3221_input *input = &ina->inputs[channel];
-	int resistance_uo = input->shunt_resistor;
-	int val, current_ma, voltage_uv, ret;
+	int current_ma, ret;
 
-	ret = kstrtoint(buf, 0, &current_ma);
+	ret = __ina3221_show_current(ina, reg, &current_ma);
 	if (ret)
 		return ret;
 
+	return snprintf(buf, PAGE_SIZE, "%d\n", current_ma);
+}
+
+static int __ina3221_set_current(struct ina3221_data *ina,
+				 unsigned int reg, int current_ma)
+{
+	unsigned int channel = register_channel[reg];
+	struct ina3221_input *input = &ina->inputs[channel];
+	int resistance_uo = input->shunt_resistor;
+	int val, voltage_uv, ret;
+
 	/* clamp current */
 	current_ma = clamp_val(current_ma,
 			       INT_MIN / resistance_uo,
@@ -226,6 +235,26 @@ static ssize_t ina3221_set_current(struct device *dev,
 	if (ret)
 		return ret;
 
+	return 0;
+}
+
+static ssize_t ina3221_set_current(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	unsigned int reg = sd_attr->index;
+	int current_ma, ret;
+
+	ret = kstrtoint(buf, 0, &current_ma);
+	if (ret)
+		return ret;
+
+	ret = __ina3221_set_current(ina, reg, current_ma);
+	if (ret)
+		return ret;
+
 	return count;
 }
 
@@ -278,6 +307,68 @@ static ssize_t ina3221_show_alert(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", regval);
 }
 
+static ssize_t ina3221_show_power(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	unsigned int reg = sd_attr->index;
+	unsigned int channel = register_channel[reg];
+	unsigned int reg_bus = INA3221_BUS(channel);
+	int val, current_ma, voltage_mv, ret;
+	s64 power_uw;
+
+	/* Read bus voltage */
+	ret = ina3221_read_value(ina, reg_bus, &val);
+	if (ret)
+		return ret;
+
+	/* LSB (4th bit) is 8 mV */
+	voltage_mv = val * 8;
+
+	/* Read calculated current */
+	ret = __ina3221_show_current(ina, reg, &current_ma);
+	if (ret)
+		return ret;
+
+	power_uw = (s64)voltage_mv * current_ma;
+
+	return snprintf(buf, PAGE_SIZE, "%lld\n", power_uw);
+}
+
+static ssize_t ina3221_set_power(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	unsigned int reg = sd_attr->index;
+	unsigned int channel = register_channel[reg];
+	unsigned int reg_bus = INA3221_BUS(channel);
+	int val, current_ma, voltage_mv, ret;
+	s64 power_uw;
+
+	ret = kstrtos64(buf, 0, &power_uw);
+	if (ret)
+		return ret;
+
+	/* Read bus voltage */
+	ret = ina3221_read_value(ina, reg_bus, &val);
+	if (ret)
+		return ret;
+
+	/* LSB (4th bit) is 8 mV */
+	voltage_mv = val * 8;
+
+	current_ma = DIV_ROUND_CLOSEST(power_uw, voltage_mv);
+
+	ret = __ina3221_set_current(ina, reg, current_ma);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
 /* input channel label */
 static SENSOR_DEVICE_ATTR(in1_label, 0444,
 		ina3221_show_label, NULL, INA3221_CHANNEL1);
@@ -350,6 +441,22 @@ static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO,
 static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
 		ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3);
 
+/* calculated power */
+static SENSOR_DEVICE_ATTR(power1_input, 0444,
+		ina3221_show_power, NULL, INA3221_SHUNT1);
+static SENSOR_DEVICE_ATTR(power2_input, 0444,
+		ina3221_show_power, NULL, INA3221_SHUNT2);
+static SENSOR_DEVICE_ATTR(power3_input, 0444,
+		ina3221_show_power, NULL, INA3221_SHUNT3);
+
+/* critical power */
+static SENSOR_DEVICE_ATTR(power1_crit, 0644,
+		ina3221_show_power, ina3221_set_power, INA3221_CRIT1);
+static SENSOR_DEVICE_ATTR(power2_crit, 0644,
+		ina3221_show_power, ina3221_set_power, INA3221_CRIT2);
+static SENSOR_DEVICE_ATTR(power3_crit, 0644,
+		ina3221_show_power, ina3221_set_power, INA3221_CRIT3);
+
 static struct attribute *ina3221_attrs[] = {
 	/* channel 1 -- make sure label at first */
 	&sensor_dev_attr_in1_label.dev_attr.attr,
@@ -361,6 +468,8 @@ static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr1_max.dev_attr.attr,
 	&sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in4_input.dev_attr.attr,
+	&sensor_dev_attr_power1_input.dev_attr.attr,
+	&sensor_dev_attr_power1_crit.dev_attr.attr,
 
 	/* channel 2 -- make sure label at first */
 	&sensor_dev_attr_in2_label.dev_attr.attr,
@@ -372,6 +481,8 @@ static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr2_max.dev_attr.attr,
 	&sensor_dev_attr_curr2_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in5_input.dev_attr.attr,
+	&sensor_dev_attr_power2_input.dev_attr.attr,
+	&sensor_dev_attr_power2_crit.dev_attr.attr,
 
 	/* channel 3 -- make sure label at first */
 	&sensor_dev_attr_in3_label.dev_attr.attr,
@@ -383,6 +494,8 @@ static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr3_max.dev_attr.attr,
 	&sensor_dev_attr_curr3_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in6_input.dev_attr.attr,
+	&sensor_dev_attr_power3_input.dev_attr.attr,
+	&sensor_dev_attr_power3_crit.dev_attr.attr,
 
 	NULL,
 };
-- 
2.17.1

Re: [PATCH 1/2] hwmon: ina3221: Add power sysfs nodes

[Guenter Roeck]

Hi Nicolin,

On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> The hwmon sysfs ABI supports powerX_input and powerX_crit. This
> can ease user space programs who care more about power in total
> than voltage or current individually.
> 
> So this patch adds these two sysfs nodes for INA3221 driver.
> 

Ah, sorry, we can't do that. The sysfs nodes are for chips providing power
registers, not for kernel drivers to provide calculations based on voltage
and current measurements. Basic guideline is that we report what is there,
not some  calculation based on it.

This is even more true for power limits: We can not assume that the power limit
is (max voltage * max current). or (current voltage * max_current), or anything
else. We simply don't have the knowledge to make that assumption.

Thanks,
Guenter

> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>   Documentation/hwmon/ina3221 |   4 +
>   drivers/hwmon/ina3221.c     | 145 ++++++++++++++++++++++++++++++++----
>   2 files changed, 133 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
> index 2726038be5bd..6be64b553cd0 100644
> --- a/Documentation/hwmon/ina3221
> +++ b/Documentation/hwmon/ina3221
> @@ -34,3 +34,7 @@ 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
> +power[123]_input        Power(uW) measurement channels
> +power[123]_crit         Critical alert power(uW) setting, activates the
> +                          corresponding alarm when the respective power
> +                          is above this value
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index ce4d1f55e9cd..5890a2da3bfe 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -38,6 +38,8 @@
>   #define INA3221_WARN3			0x0c
>   #define INA3221_MASK_ENABLE		0x0f
>   
> +#define INA3221_BUS(x)			(INA3221_BUS1 + ((x) << 1))
> +
>   #define INA3221_CONFIG_MODE_SHUNT	BIT(1)
>   #define INA3221_CONFIG_MODE_BUS		BIT(2)
>   #define INA3221_CONFIG_MODE_CONTINUOUS	BIT(3)
> @@ -172,43 +174,50 @@ static ssize_t ina3221_show_shunt_voltage(struct device *dev,
>   	return snprintf(buf, PAGE_SIZE, "%d\n", voltage_uv);
>   }
>   
> -static ssize_t ina3221_show_current(struct device *dev,
> -				    struct device_attribute *attr, char *buf)
> +static int __ina3221_show_current(struct ina3221_data *ina,
> +				  unsigned int reg, int *current_ma)
>   {
> -	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> -	struct ina3221_data *ina = dev_get_drvdata(dev);
> -	unsigned int reg = sd_attr->index;
>   	unsigned int channel = register_channel[reg];
>   	struct ina3221_input *input = &ina->inputs[channel];
>   	int resistance_uo = input->shunt_resistor;
> -	int val, current_ma, voltage_nv, ret;
> +	int val, voltage_nv, ret;
>   
> +	/* Read bus shunt voltage */
>   	ret = ina3221_read_value(ina, reg, &val);
>   	if (ret)
>   		return ret;
> +
> +	/* LSB (4th bit) is 40 uV (40000 nV) */
>   	voltage_nv = val * 40000;
>   
> -	current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
> +	*current_ma = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
>   
> -	return snprintf(buf, PAGE_SIZE, "%d\n", current_ma);
> +	return 0;
>   }
>   
> -static ssize_t ina3221_set_current(struct device *dev,
> -				   struct device_attribute *attr,
> -				   const char *buf, size_t count)
> +static ssize_t ina3221_show_current(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
>   {
>   	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>   	struct ina3221_data *ina = dev_get_drvdata(dev);
>   	unsigned int reg = sd_attr->index;
> -	unsigned int channel = register_channel[reg];
> -	struct ina3221_input *input = &ina->inputs[channel];
> -	int resistance_uo = input->shunt_resistor;
> -	int val, current_ma, voltage_uv, ret;
> +	int current_ma, ret;
>   
> -	ret = kstrtoint(buf, 0, &current_ma);
> +	ret = __ina3221_show_current(ina, reg, &current_ma);
>   	if (ret)
>   		return ret;
>   
> +	return snprintf(buf, PAGE_SIZE, "%d\n", current_ma);
> +}
> +
> +static int __ina3221_set_current(struct ina3221_data *ina,
> +				 unsigned int reg, int current_ma)
> +{
> +	unsigned int channel = register_channel[reg];
> +	struct ina3221_input *input = &ina->inputs[channel];
> +	int resistance_uo = input->shunt_resistor;
> +	int val, voltage_uv, ret;
> +
>   	/* clamp current */
>   	current_ma = clamp_val(current_ma,
>   			       INT_MIN / resistance_uo,
> @@ -226,6 +235,26 @@ static ssize_t ina3221_set_current(struct device *dev,
>   	if (ret)
>   		return ret;
>   
> +	return 0;
> +}
> +
> +static ssize_t ina3221_set_current(struct device *dev,
> +				   struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	unsigned int reg = sd_attr->index;
> +	int current_ma, ret;
> +
> +	ret = kstrtoint(buf, 0, &current_ma);
> +	if (ret)
> +		return ret;
> +
> +	ret = __ina3221_set_current(ina, reg, current_ma);
> +	if (ret)
> +		return ret;
> +
>   	return count;
>   }
>   
> @@ -278,6 +307,68 @@ static ssize_t ina3221_show_alert(struct device *dev,
>   	return snprintf(buf, PAGE_SIZE, "%d\n", regval);
>   }
>   
> +static ssize_t ina3221_show_power(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	unsigned int reg = sd_attr->index;
> +	unsigned int channel = register_channel[reg];
> +	unsigned int reg_bus = INA3221_BUS(channel);
> +	int val, current_ma, voltage_mv, ret;
> +	s64 power_uw;
> +
> +	/* Read bus voltage */
> +	ret = ina3221_read_value(ina, reg_bus, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* LSB (4th bit) is 8 mV */
> +	voltage_mv = val * 8;
> +
> +	/* Read calculated current */
> +	ret = __ina3221_show_current(ina, reg, &current_ma);
> +	if (ret)
> +		return ret;
> +
> +	power_uw = (s64)voltage_mv * current_ma;
> +
> +	return snprintf(buf, PAGE_SIZE, "%lld\n", power_uw);
> +}
> +
> +static ssize_t ina3221_set_power(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	unsigned int reg = sd_attr->index;
> +	unsigned int channel = register_channel[reg];
> +	unsigned int reg_bus = INA3221_BUS(channel);
> +	int val, current_ma, voltage_mv, ret;
> +	s64 power_uw;
> +
> +	ret = kstrtos64(buf, 0, &power_uw);
> +	if (ret)
> +		return ret;
> +
> +	/* Read bus voltage */
> +	ret = ina3221_read_value(ina, reg_bus, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* LSB (4th bit) is 8 mV */
> +	voltage_mv = val * 8;
> +
> +	current_ma = DIV_ROUND_CLOSEST(power_uw, voltage_mv);
> +
> +	ret = __ina3221_set_current(ina, reg, current_ma);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
>   /* input channel label */
>   static SENSOR_DEVICE_ATTR(in1_label, 0444,
>   		ina3221_show_label, NULL, INA3221_CHANNEL1);
> @@ -350,6 +441,22 @@ static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO,
>   static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
>   		ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3);
>   
> +/* calculated power */
> +static SENSOR_DEVICE_ATTR(power1_input, 0444,
> +		ina3221_show_power, NULL, INA3221_SHUNT1);
> +static SENSOR_DEVICE_ATTR(power2_input, 0444,
> +		ina3221_show_power, NULL, INA3221_SHUNT2);
> +static SENSOR_DEVICE_ATTR(power3_input, 0444,
> +		ina3221_show_power, NULL, INA3221_SHUNT3);
> +
> +/* critical power */
> +static SENSOR_DEVICE_ATTR(power1_crit, 0644,
> +		ina3221_show_power, ina3221_set_power, INA3221_CRIT1);
> +static SENSOR_DEVICE_ATTR(power2_crit, 0644,
> +		ina3221_show_power, ina3221_set_power, INA3221_CRIT2);
> +static SENSOR_DEVICE_ATTR(power3_crit, 0644,
> +		ina3221_show_power, ina3221_set_power, INA3221_CRIT3);
> +
>   static struct attribute *ina3221_attrs[] = {
>   	/* channel 1 -- make sure label at first */
>   	&sensor_dev_attr_in1_label.dev_attr.attr,
> @@ -361,6 +468,8 @@ static struct attribute *ina3221_attrs[] = {
>   	&sensor_dev_attr_curr1_max.dev_attr.attr,
>   	&sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
>   	&sensor_dev_attr_in4_input.dev_attr.attr,
> +	&sensor_dev_attr_power1_input.dev_attr.attr,
> +	&sensor_dev_attr_power1_crit.dev_attr.attr,
>   
>   	/* channel 2 -- make sure label at first */
>   	&sensor_dev_attr_in2_label.dev_attr.attr,
> @@ -372,6 +481,8 @@ static struct attribute *ina3221_attrs[] = {
>   	&sensor_dev_attr_curr2_max.dev_attr.attr,
>   	&sensor_dev_attr_curr2_max_alarm.dev_attr.attr,
>   	&sensor_dev_attr_in5_input.dev_attr.attr,
> +	&sensor_dev_attr_power2_input.dev_attr.attr,
> +	&sensor_dev_attr_power2_crit.dev_attr.attr,
>   
>   	/* channel 3 -- make sure label at first */
>   	&sensor_dev_attr_in3_label.dev_attr.attr,
> @@ -383,6 +494,8 @@ static struct attribute *ina3221_attrs[] = {
>   	&sensor_dev_attr_curr3_max.dev_attr.attr,
>   	&sensor_dev_attr_curr3_max_alarm.dev_attr.attr,
>   	&sensor_dev_attr_in6_input.dev_attr.attr,
> +	&sensor_dev_attr_power3_input.dev_attr.attr,
> +	&sensor_dev_attr_power3_crit.dev_attr.attr,
>   
>   	NULL,
>   };
> 

Re: [PATCH 1/2] hwmon: ina3221: Add power sysfs nodes

[Nicolin Chen]

On Wed, Sep 26, 2018 at 05:34:53AM -0700, Guenter Roeck wrote:
> Hi Nicolin,
> 
> On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > The hwmon sysfs ABI supports powerX_input and powerX_crit. This
> > can ease user space programs who care more about power in total
> > than voltage or current individually.
> > 
> > So this patch adds these two sysfs nodes for INA3221 driver.

> Ah, sorry, we can't do that. The sysfs nodes are for chips providing power
> registers, not for kernel drivers to provide calculations based on voltage
> and current measurements.

Hmm..I saw ina2xx.c and ltc4215.c are doing similar calculations...

> Basic guideline is that we report what is there, not some calculation based
> on it.

I could feel the back thoughts behind the guideline, but this does
give user space programs some trouble -- I have a few programs that
were used to read ina2xx driver which provides power nodes, but now
those programs will have to implement another function to read the
voltage and current separately to do further calculations.

Do you know any better solution for this situation?

> This is even more true for power limits: We can not assume that the power limit
> is (max voltage * max current). or (current voltage * max_current), or anything
> else. We simply don't have the knowledge to make that assumption.

I agree that power limit is a bit tricky here as the voltage could
change depending on the user space, Yes, I assumed that users who
set_power() should be aware of it (whether fixed or dynamical) so
as to decide to configure power limit or just current limit.

Thanks
Nicolin

Re: [PATCH 1/2] hwmon: ina3221: Add power sysfs nodes

[Guenter Roeck]

Hi Nicolin,

On Wed, Sep 26, 2018 at 11:20:06AM -0700, Nicolin Chen wrote:
> On Wed, Sep 26, 2018 at 05:34:53AM -0700, Guenter Roeck wrote:
> > Hi Nicolin,
> > 
> > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > The hwmon sysfs ABI supports powerX_input and powerX_crit. This
> > > can ease user space programs who care more about power in total
> > > than voltage or current individually.
> > > 
> > > So this patch adds these two sysfs nodes for INA3221 driver.
> 
> > Ah, sorry, we can't do that. The sysfs nodes are for chips providing power
> > registers, not for kernel drivers to provide calculations based on voltage
> > and current measurements.
> 
> Hmm..I saw ina2xx.c and ltc4215.c are doing similar calculations...
> 

ina2xx.c doesn't; the chips supported by the driver do have a register
reporting the power (0x03). ltc4215.c was not reviewed by a hwmon
maintainer. I think I mentioned before that you can find anything you want
in the Linux kernel. That doesn't make it right.

> > Basic guideline is that we report what is there, not some calculation based
> > on it.
> 
> I could feel the back thoughts behind the guideline, but this does
> give user space programs some trouble -- I have a few programs that
> were used to read ina2xx driver which provides power nodes, but now
> those programs will have to implement another function to read the
> voltage and current separately to do further calculations.
> 
> Do you know any better solution for this situation?
> 

Userspace simply must not assume that power attributes exist and calculate
it from voltage and current attributes if needed.

> > This is even more true for power limits: We can not assume that the power limit
> > is (max voltage * max current). or (current voltage * max_current), or anything
> > else. We simply don't have the knowledge to make that assumption.
> 
> I agree that power limit is a bit tricky here as the voltage could
> change depending on the user space, Yes, I assumed that users who
> set_power() should be aware of it (whether fixed or dynamical) so
> as to decide to configure power limit or just current limit.
> 

You just can not make any such assumptions, sorry. Limit attributes
absolutely must be reflected in hardware. 

Thanks,
Guenter

Re: [PATCH 1/2] hwmon: ina3221: Add power sysfs nodes

[Nicolin Chen]

Hello Guenter,

On Wed, Sep 26, 2018 at 12:45:34PM -0700, Guenter Roeck wrote:
> Hi Nicolin,
> 
> On Wed, Sep 26, 2018 at 11:20:06AM -0700, Nicolin Chen wrote:
> > On Wed, Sep 26, 2018 at 05:34:53AM -0700, Guenter Roeck wrote:
> > > Hi Nicolin,
> > > 
> > > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > > The hwmon sysfs ABI supports powerX_input and powerX_crit. This
> > > > can ease user space programs who care more about power in total
> > > > than voltage or current individually.
> > > > 
> > > > So this patch adds these two sysfs nodes for INA3221 driver.
> > 
> > > Ah, sorry, we can't do that. The sysfs nodes are for chips providing power
> > > registers, not for kernel drivers to provide calculations based on voltage
> > > and current measurements.
> > 
> > Hmm..I saw ina2xx.c and ltc4215.c are doing similar calculations...
> > 
> 
> ina2xx.c doesn't; the chips supported by the driver do have a register
> reporting the power (0x03). ltc4215.c was not reviewed by a hwmon
> maintainer. I think I mentioned before that you can find anything you want
> in the Linux kernel. That doesn't make it right.

OK. In that case, I am dropping this change.

Thanks
Nicolin

[PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Nicolin Chen]

The inX_enable interface allows user space to enable or disable
the corresponding channel. Meanwhile, according to hwmon ABI, a
disabled channel/sensor should return -ENODATA as a read result.

However, there're configurable nodes sharing the same __show()
functions. So this change also adds to check if the attribute is
read-only to make sure it's not reading a configuration but the
sensor data.

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

diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index 6be64b553cd0..1aa32366d8aa 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -23,6 +23,7 @@ Sysfs entries
 
 in[123]_input           Bus voltage(mV) channels
 in[123]_label           Voltage channel labels
+in[123]_enable          Voltage channel enable controls
 curr[123]_input         Current(mA) measurement channels
 shunt[123]_resistor     Shunt resistance(uOhm) channels
 curr[123]_crit          Critical alert current(mA) setting, activates the
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 5890a2da3bfe..1be2d062a19e 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -78,6 +78,9 @@ enum ina3221_channels {
 };
 
 static const unsigned int register_channel[] = {
+	[INA3221_BUS1] = INA3221_CHANNEL1,
+	[INA3221_BUS2] = INA3221_CHANNEL2,
+	[INA3221_BUS3] = INA3221_CHANNEL3,
 	[INA3221_SHUNT1] = INA3221_CHANNEL1,
 	[INA3221_SHUNT2] = INA3221_CHANNEL2,
 	[INA3221_SHUNT3] = INA3221_CHANNEL3,
@@ -113,6 +116,19 @@ struct ina3221_data {
 	struct ina3221_input inputs[INA3221_NUM_CHANNELS];
 };
 
+static inline bool ina3221_is_enable(struct ina3221_data *ina, int channel)
+{
+	unsigned int config;
+	int ret;
+
+	/* Return false to reject further read */
+	ret = regmap_read(ina->regmap, INA3221_CONFIG, &config);
+	if (ret)
+		return false;
+
+	return (config & INA3221_CONFIG_CHx_EN(channel)) > 0;
+}
+
 static ssize_t ina3221_show_label(struct device *dev,
 				  struct device_attribute *attr, char *buf)
 {
@@ -124,6 +140,45 @@ static ssize_t ina3221_show_label(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%s\n", input->label);
 }
 
+static ssize_t ina3221_show_enable(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	unsigned int channel = sd_attr->index;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			ina3221_is_enable(ina, channel));
+}
+
+static ssize_t ina3221_set_enable(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	unsigned int channel = sd_attr->index;
+	unsigned int mask = INA3221_CONFIG_CHx_EN(channel);
+	unsigned int config;
+	int val, ret;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	/* inX_enable only accepts 1 for enabling or 0 for disabling */
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
+	config = val ? mask : 0;
+
+	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
 static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
 			      int *val)
 {
@@ -146,8 +201,13 @@ static ssize_t ina3221_show_bus_voltage(struct device *dev,
 	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	unsigned int reg = sd_attr->index;
+	unsigned int channel = register_channel[reg];
 	int val, voltage_mv, ret;
 
+	/* No data for read-only attribute if channel is disabled */
+	if (!attr->store && !ina3221_is_enable(ina, channel))
+		return -ENODATA;
+
 	ret = ina3221_read_value(ina, reg, &val);
 	if (ret)
 		return ret;
@@ -164,8 +224,13 @@ static ssize_t ina3221_show_shunt_voltage(struct device *dev,
 	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	unsigned int reg = sd_attr->index;
+	unsigned int channel = register_channel[reg];
 	int val, voltage_uv, ret;
 
+	/* No data for read-only attribute if channel is disabled */
+	if (!attr->store && !ina3221_is_enable(ina, channel))
+		return -ENODATA;
+
 	ret = ina3221_read_value(ina, reg, &val);
 	if (ret)
 		return ret;
@@ -201,8 +266,13 @@ static ssize_t ina3221_show_current(struct device *dev,
 	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	unsigned int reg = sd_attr->index;
+	unsigned int channel = register_channel[reg];
 	int current_ma, ret;
 
+	/* No data for read-only attribute if channel is disabled */
+	if (!attr->store && !ina3221_is_enable(ina, channel))
+		return -ENODATA;
+
 	ret = __ina3221_show_current(ina, reg, &current_ma);
 	if (ret)
 		return ret;
@@ -318,6 +388,10 @@ static ssize_t ina3221_show_power(struct device *dev,
 	int val, current_ma, voltage_mv, ret;
 	s64 power_uw;
 
+	/* No data for read-only attribute if channel is disabled */
+	if (!attr->store && !ina3221_is_enable(ina, channel))
+		return -ENODATA;
+
 	/* Read bus voltage */
 	ret = ina3221_read_value(ina, reg_bus, &val);
 	if (ret)
@@ -377,6 +451,14 @@ static SENSOR_DEVICE_ATTR(in2_label, 0444,
 static SENSOR_DEVICE_ATTR(in3_label, 0444,
 		ina3221_show_label, NULL, INA3221_CHANNEL3);
 
+/* voltage channel enable */
+static SENSOR_DEVICE_ATTR(in1_enable, 0644,
+		ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL1);
+static SENSOR_DEVICE_ATTR(in2_enable, 0644,
+		ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL2);
+static SENSOR_DEVICE_ATTR(in3_enable, 0644,
+		ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL3);
+
 /* bus voltage */
 static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO,
 		ina3221_show_bus_voltage, NULL, INA3221_BUS1);
@@ -460,6 +542,7 @@ static SENSOR_DEVICE_ATTR(power3_crit, 0644,
 static struct attribute *ina3221_attrs[] = {
 	/* channel 1 -- make sure label at first */
 	&sensor_dev_attr_in1_label.dev_attr.attr,
+	&sensor_dev_attr_in1_enable.dev_attr.attr,
 	&sensor_dev_attr_in1_input.dev_attr.attr,
 	&sensor_dev_attr_curr1_input.dev_attr.attr,
 	&sensor_dev_attr_shunt1_resistor.dev_attr.attr,
@@ -473,6 +556,7 @@ static struct attribute *ina3221_attrs[] = {
 
 	/* channel 2 -- make sure label at first */
 	&sensor_dev_attr_in2_label.dev_attr.attr,
+	&sensor_dev_attr_in2_enable.dev_attr.attr,
 	&sensor_dev_attr_in2_input.dev_attr.attr,
 	&sensor_dev_attr_curr2_input.dev_attr.attr,
 	&sensor_dev_attr_shunt2_resistor.dev_attr.attr,
@@ -486,6 +570,7 @@ static struct attribute *ina3221_attrs[] = {
 
 	/* channel 3 -- make sure label at first */
 	&sensor_dev_attr_in3_label.dev_attr.attr,
+	&sensor_dev_attr_in3_enable.dev_attr.attr,
 	&sensor_dev_attr_in3_input.dev_attr.attr,
 	&sensor_dev_attr_curr3_input.dev_attr.attr,
 	&sensor_dev_attr_shunt3_resistor.dev_attr.attr,
-- 
2.17.1

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Guenter Roeck]

Hi Nicolin,

On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> The inX_enable interface allows user space to enable or disable
> the corresponding channel. Meanwhile, according to hwmon ABI, a
> disabled channel/sensor should return -ENODATA as a read result.
> 
> However, there're configurable nodes sharing the same __show()
> functions. So this change also adds to check if the attribute is
> read-only to make sure it's not reading a configuration but the
> sensor data.
> 

One necessary high level change I don't see below: With this change,
we should no longer drop a channel entirely if it is disabled from
devicetree. All channels should be visible but report -ENODATA if
disabled. In other words, it should be possible for the 'enable' flag
to override settings in DT.

> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>   Documentation/hwmon/ina3221 |  1 +
>   drivers/hwmon/ina3221.c     | 85 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 86 insertions(+)
> 
> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
> index 6be64b553cd0..1aa32366d8aa 100644
> --- a/Documentation/hwmon/ina3221
> +++ b/Documentation/hwmon/ina3221
> @@ -23,6 +23,7 @@ Sysfs entries
>   
>   in[123]_input           Bus voltage(mV) channels
>   in[123]_label           Voltage channel labels
> +in[123]_enable          Voltage channel enable controls
>   curr[123]_input         Current(mA) measurement channels
>   shunt[123]_resistor     Shunt resistance(uOhm) channels
>   curr[123]_crit          Critical alert current(mA) setting, activates the
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 5890a2da3bfe..1be2d062a19e 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -78,6 +78,9 @@ enum ina3221_channels {
>   };
>   
>   static const unsigned int register_channel[] = {
> +	[INA3221_BUS1] = INA3221_CHANNEL1,
> +	[INA3221_BUS2] = INA3221_CHANNEL2,
> +	[INA3221_BUS3] = INA3221_CHANNEL3,
>   	[INA3221_SHUNT1] = INA3221_CHANNEL1,
>   	[INA3221_SHUNT2] = INA3221_CHANNEL2,
>   	[INA3221_SHUNT3] = INA3221_CHANNEL3,
> @@ -113,6 +116,19 @@ struct ina3221_data {
>   	struct ina3221_input inputs[INA3221_NUM_CHANNELS];
>   };
>   
> +static inline bool ina3221_is_enable(struct ina3221_data *ina, int channel)

s/is_enable/is_enabled/, maybe ?

> +{
> +	unsigned int config;
> +	int ret;
> +
> +	/* Return false to reject further read */
> +	ret = regmap_read(ina->regmap, INA3221_CONFIG, &config);
> +	if (ret)
> +		return false;
> +
> +	return (config & INA3221_CONFIG_CHx_EN(channel)) > 0;

The "> 0" is unnecessary. Conversion to bool is automatic. If you want to make
it explicit, please use

	return !!(config & INA3221_CONFIG_CHx_EN(channel));

It should not be necessary to re-read the value from the chip all the time.
I would suggest to cache the value in the 'disabled' variable.

> +}
> +
>   static ssize_t ina3221_show_label(struct device *dev,
>   				  struct device_attribute *attr, char *buf)
>   {
> @@ -124,6 +140,45 @@ static ssize_t ina3221_show_label(struct device *dev,
>   	return snprintf(buf, PAGE_SIZE, "%s\n", input->label);
>   }
>   
> +static ssize_t ina3221_show_enable(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	unsigned int channel = sd_attr->index;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			ina3221_is_enable(ina, channel));
> +}
> +
> +static ssize_t ina3221_set_enable(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +	unsigned int channel = sd_attr->index;
> +	unsigned int mask = INA3221_CONFIG_CHx_EN(channel);
> +	unsigned int config;
> +	int val, ret;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* inX_enable only accepts 1 for enabling or 0 for disabling */
> +	if (val != 0 && val != 1)
> +		return -EINVAL;
> +
Just use kstrtobool().

> +	config = val ? mask : 0;
> +
> +	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, config);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
>   static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
>   			      int *val)
>   {
> @@ -146,8 +201,13 @@ static ssize_t ina3221_show_bus_voltage(struct device *dev,
>   	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>   	struct ina3221_data *ina = dev_get_drvdata(dev);
>   	unsigned int reg = sd_attr->index;
> +	unsigned int channel = register_channel[reg];
>   	int val, voltage_mv, ret;
>   
> +	/* No data for read-only attribute if channel is disabled */
> +	if (!attr->store && !ina3221_is_enable(ina, channel))
> +		return -ENODATA;
> +
>   	ret = ina3221_read_value(ina, reg, &val);
>   	if (ret)
>   		return ret;
> @@ -164,8 +224,13 @@ static ssize_t ina3221_show_shunt_voltage(struct device *dev,
>   	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>   	struct ina3221_data *ina = dev_get_drvdata(dev);
>   	unsigned int reg = sd_attr->index;
> +	unsigned int channel = register_channel[reg];
>   	int val, voltage_uv, ret;
>   
> +	/* No data for read-only attribute if channel is disabled */
> +	if (!attr->store && !ina3221_is_enable(ina, channel))
> +		return -ENODATA;
> +
>   	ret = ina3221_read_value(ina, reg, &val);
>   	if (ret)
>   		return ret;
> @@ -201,8 +266,13 @@ static ssize_t ina3221_show_current(struct device *dev,
>   	struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>   	struct ina3221_data *ina = dev_get_drvdata(dev);
>   	unsigned int reg = sd_attr->index;
> +	unsigned int channel = register_channel[reg];
>   	int current_ma, ret;
>   
> +	/* No data for read-only attribute if channel is disabled */
> +	if (!attr->store && !ina3221_is_enable(ina, channel))
> +		return -ENODATA;
> +
>   	ret = __ina3221_show_current(ina, reg, &current_ma);
>   	if (ret)
>   		return ret;
> @@ -318,6 +388,10 @@ static ssize_t ina3221_show_power(struct device *dev,
>   	int val, current_ma, voltage_mv, ret;
>   	s64 power_uw;
>   
> +	/* No data for read-only attribute if channel is disabled */
> +	if (!attr->store && !ina3221_is_enable(ina, channel))
> +		return -ENODATA;
> +
>   	/* Read bus voltage */
>   	ret = ina3221_read_value(ina, reg_bus, &val);
>   	if (ret)
> @@ -377,6 +451,14 @@ static SENSOR_DEVICE_ATTR(in2_label, 0444,
>   static SENSOR_DEVICE_ATTR(in3_label, 0444,
>   		ina3221_show_label, NULL, INA3221_CHANNEL3);
>   
> +/* voltage channel enable */
> +static SENSOR_DEVICE_ATTR(in1_enable, 0644,
> +		ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL1);
> +static SENSOR_DEVICE_ATTR(in2_enable, 0644,
> +		ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL2);
> +static SENSOR_DEVICE_ATTR(in3_enable, 0644,
> +		ina3221_show_enable, ina3221_set_enable, INA3221_CHANNEL3);
> +
>   /* bus voltage */
>   static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO,
>   		ina3221_show_bus_voltage, NULL, INA3221_BUS1);
> @@ -460,6 +542,7 @@ static SENSOR_DEVICE_ATTR(power3_crit, 0644,
>   static struct attribute *ina3221_attrs[] = {
>   	/* channel 1 -- make sure label at first */
>   	&sensor_dev_attr_in1_label.dev_attr.attr,
> +	&sensor_dev_attr_in1_enable.dev_attr.attr,
>   	&sensor_dev_attr_in1_input.dev_attr.attr,
>   	&sensor_dev_attr_curr1_input.dev_attr.attr,
>   	&sensor_dev_attr_shunt1_resistor.dev_attr.attr,
> @@ -473,6 +556,7 @@ static struct attribute *ina3221_attrs[] = {
>   
>   	/* channel 2 -- make sure label at first */
>   	&sensor_dev_attr_in2_label.dev_attr.attr,
> +	&sensor_dev_attr_in2_enable.dev_attr.attr,
>   	&sensor_dev_attr_in2_input.dev_attr.attr,
>   	&sensor_dev_attr_curr2_input.dev_attr.attr,
>   	&sensor_dev_attr_shunt2_resistor.dev_attr.attr,
> @@ -486,6 +570,7 @@ static struct attribute *ina3221_attrs[] = {
>   
>   	/* channel 3 -- make sure label at first */
>   	&sensor_dev_attr_in3_label.dev_attr.attr,
> +	&sensor_dev_attr_in3_enable.dev_attr.attr,
>   	&sensor_dev_attr_in3_input.dev_attr.attr,
>   	&sensor_dev_attr_curr3_input.dev_attr.attr,
>   	&sensor_dev_attr_shunt3_resistor.dev_attr.attr,
> 

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Nicolin Chen]

On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote:
> On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > The inX_enable interface allows user space to enable or disable
> > the corresponding channel. Meanwhile, according to hwmon ABI, a
> > disabled channel/sensor should return -ENODATA as a read result.
> > 
> > However, there're configurable nodes sharing the same __show()
> > functions. So this change also adds to check if the attribute is
> > read-only to make sure it's not reading a configuration but the
> > sensor data.
 
> One necessary high level change I don't see below: With this change,
> we should no longer drop a channel entirely if it is disabled from
> devicetree. All channels should be visible but report -ENODATA if
> disabled. In other words, it should be possible for the 'enable' flag
> to override settings in DT.

Hmm...I don't feel so convinced here. The status in DT binding isn't
exactly a setting but a physical status: if a hardware design leaves
a channel to be disconnected, I don't really see a point in enabling
it in the runtime. Or maybe you can shed some light on it?

Meanwhile, I believe the enable nodes are necessary in either way as
users could decide to disable the connected channels, based on their
use cases, to save power.

Thanks
Nicolin

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Guenter Roeck]

Nicolin,

On Wed, Sep 26, 2018 at 11:02:44AM -0700, Nicolin Chen wrote:
> On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote:
> > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > The inX_enable interface allows user space to enable or disable
> > > the corresponding channel. Meanwhile, according to hwmon ABI, a
> > > disabled channel/sensor should return -ENODATA as a read result.
> > > 
> > > However, there're configurable nodes sharing the same __show()
> > > functions. So this change also adds to check if the attribute is
> > > read-only to make sure it's not reading a configuration but the
> > > sensor data.
>  
> > One necessary high level change I don't see below: With this change,
> > we should no longer drop a channel entirely if it is disabled from
> > devicetree. All channels should be visible but report -ENODATA if
> > disabled. In other words, it should be possible for the 'enable' flag
> > to override settings in DT.
> 
> Hmm...I don't feel so convinced here. The status in DT binding isn't
> exactly a setting but a physical status: if a hardware design leaves
> a channel to be disconnected, I don't really see a point in enabling
> it in the runtime. Or maybe you can shed some light on it?
> 

You are making an assumption from your use case. It might as well be that
some or all channels are disabled in DT by default to conserve power,
not because they are disconnected.

> Meanwhile, I believe the enable nodes are necessary in either way as
> users could decide to disable the connected channels, based on their
> use cases, to save power.
> 
Agreed, though I would not say "necessary". "Useful" seems to be more
appropriate.

Thanks,
Guenter

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Nicolin Chen]

Hello,

On Wed, Sep 26, 2018 at 12:58:17PM -0700, Guenter Roeck wrote:
> On Wed, Sep 26, 2018 at 11:02:44AM -0700, Nicolin Chen wrote:
> > On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote:
> > > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > > The inX_enable interface allows user space to enable or disable
> > > > the corresponding channel. Meanwhile, according to hwmon ABI, a
> > > > disabled channel/sensor should return -ENODATA as a read result.
> > > > 
> > > > However, there're configurable nodes sharing the same __show()
> > > > functions. So this change also adds to check if the attribute is
> > > > read-only to make sure it's not reading a configuration but the
> > > > sensor data.
> >  
> > > One necessary high level change I don't see below: With this change,
> > > we should no longer drop a channel entirely if it is disabled from
> > > devicetree. All channels should be visible but report -ENODATA if
> > > disabled. In other words, it should be possible for the 'enable' flag
> > > to override settings in DT.
> > 
> > Hmm...I don't feel so convinced here. The status in DT binding isn't
> > exactly a setting but a physical status: if a hardware design leaves
> > a channel to be disconnected, I don't really see a point in enabling
> > it in the runtime. Or maybe you can shed some light on it?
> > 
> 
> You are making an assumption from your use case. It might as well be that
> some or all channels are disabled in DT by default to conserve power,
> not because they are disconnected.

I think I probably should update my DT binding somehow to say it
explicitly that the property should be only used in cases of the
physical disconnections, although I feel the current binding "no
input source" already has the same meaning.

In my opinion, disabling channels in DT to save power isn't very
plausible, because it sounds more likely a user decision, while,
as we know, DT merely describes the hardware design.

Otherwise, if we want something like a setting for this purpose,
we should probably use a different property for DT binding, bool
type "disable-on-boot" for example.

> > Meanwhile, I believe the enable nodes are necessary in either way as
> > users could decide to disable the connected channels, based on their
> > use cases, to save power.
> > 
> Agreed, though I would not say "necessary". "Useful" seems to be more
> appropriate.

Yea..

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Guenter Roeck]

On Wed, Sep 26, 2018 at 01:25:20PM -0700, Nicolin Chen wrote:
> Hello,
> 
> On Wed, Sep 26, 2018 at 12:58:17PM -0700, Guenter Roeck wrote:
> > On Wed, Sep 26, 2018 at 11:02:44AM -0700, Nicolin Chen wrote:
> > > On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote:
> > > > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > > > The inX_enable interface allows user space to enable or disable
> > > > > the corresponding channel. Meanwhile, according to hwmon ABI, a
> > > > > disabled channel/sensor should return -ENODATA as a read result.
> > > > > 
> > > > > However, there're configurable nodes sharing the same __show()
> > > > > functions. So this change also adds to check if the attribute is
> > > > > read-only to make sure it's not reading a configuration but the
> > > > > sensor data.
> > >  
> > > > One necessary high level change I don't see below: With this change,
> > > > we should no longer drop a channel entirely if it is disabled from
> > > > devicetree. All channels should be visible but report -ENODATA if
> > > > disabled. In other words, it should be possible for the 'enable' flag
> > > > to override settings in DT.
> > > 
> > > Hmm...I don't feel so convinced here. The status in DT binding isn't
> > > exactly a setting but a physical status: if a hardware design leaves
> > > a channel to be disconnected, I don't really see a point in enabling
> > > it in the runtime. Or maybe you can shed some light on it?
> > > 
> > 
> > You are making an assumption from your use case. It might as well be that
> > some or all channels are disabled in DT by default to conserve power,
> > not because they are disconnected.
> 
> I think I probably should update my DT binding somehow to say it
> explicitly that the property should be only used in cases of the
> physical disconnections, although I feel the current binding "no
> input source" already has the same meaning.
> 
> In my opinion, disabling channels in DT to save power isn't very
> plausible, because it sounds more likely a user decision, while,
> as we know, DT merely describes the hardware design.
> 

I try to avoid making such assumptions. All I know is that I'll have
to deal with the fallout if a single person wants to use the property
differently. This is similar to disabling an entire subsystem in DT
because it isn't used in a specific system - say, a SPI or I2C bus
which has nothing connected on some shipping hardware, even though
the board has a connector for it. Your argument is that one shall
not use the status property do disable loading the driver, and that
one must not remove a set of properties for unused hardware either.
That doesn't sound very realistic to me.

Point is that I don't _know_ how this is going to be used, so I'd
rather keep it flexible.

Guenter

> Otherwise, if we want something like a setting for this purpose,
> we should probably use a different property for DT binding, bool
> type "disable-on-boot" for example.
> 
> > > Meanwhile, I believe the enable nodes are necessary in either way as
> > > users could decide to disable the connected channels, based on their
> > > use cases, to save power.
> > > 
> > Agreed, though I would not say "necessary". "Useful" seems to be more
> > appropriate.
> 
> Yea..

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Nicolin Chen]

On Wed, Sep 26, 2018 at 01:44:55PM -0700, Guenter Roeck wrote:
> On Wed, Sep 26, 2018 at 01:25:20PM -0700, Nicolin Chen wrote:
> > Hello,
> > 
> > On Wed, Sep 26, 2018 at 12:58:17PM -0700, Guenter Roeck wrote:
> > > On Wed, Sep 26, 2018 at 11:02:44AM -0700, Nicolin Chen wrote:
> > > > On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote:
> > > > > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > > > > The inX_enable interface allows user space to enable or disable
> > > > > > the corresponding channel. Meanwhile, according to hwmon ABI, a
> > > > > > disabled channel/sensor should return -ENODATA as a read result.
> > > > > > 
> > > > > > However, there're configurable nodes sharing the same __show()
> > > > > > functions. So this change also adds to check if the attribute is
> > > > > > read-only to make sure it's not reading a configuration but the
> > > > > > sensor data.
> > > >  
> > > > > One necessary high level change I don't see below: With this change,
> > > > > we should no longer drop a channel entirely if it is disabled from
> > > > > devicetree. All channels should be visible but report -ENODATA if
> > > > > disabled. In other words, it should be possible for the 'enable' flag
> > > > > to override settings in DT.
> > > > 
> > > > Hmm...I don't feel so convinced here. The status in DT binding isn't
> > > > exactly a setting but a physical status: if a hardware design leaves
> > > > a channel to be disconnected, I don't really see a point in enabling
> > > > it in the runtime. Or maybe you can shed some light on it?
> > > > 
> > > 
> > > You are making an assumption from your use case. It might as well be that
> > > some or all channels are disabled in DT by default to conserve power,
> > > not because they are disconnected.
> > 
> > I think I probably should update my DT binding somehow to say it
> > explicitly that the property should be only used in cases of the
> > physical disconnections, although I feel the current binding "no
> > input source" already has the same meaning.
> > 
> > In my opinion, disabling channels in DT to save power isn't very
> > plausible, because it sounds more likely a user decision, while,
> > as we know, DT merely describes the hardware design.
> > 
> 
> I try to avoid making such assumptions. All I know is that I'll have
> to deal with the fallout if a single person wants to use the property
> differently.

I can understand your point (or pain lol). But I believe in such
a case, DT maintainers should reject such a DT change. Let's say
if this kinda "default setting" in DT is allowable, other things
such as having a default mode setting between polling or one-shot
modes, and as default critical current settings would be able to
put into DT. But we know that these would be rejected as a reason
of "not being hardware design but a user decision".

> This is similar to disabling an entire subsystem in DT
> because it isn't used in a specific system - say, a SPI or I2C bus
> which has nothing connected on some shipping hardware, even though
> the board has a connector for it. Your argument is that one shall
> not use the status property do disable loading the driver, and that
> one must not remove a set of properties for unused hardware either.
> That doesn't sound very realistic to me.

SPI/I2C is a good example comparing to my case. And you do make
a point. In that case, I think DT overlay is designed for it --
one shall overlay the status property from "disable" (defined in
the DTS of the shipping hardware -- the main board) to "okay" in
the overlay DTS where a connection actually happens.

And even in this case, it makes sense to me to disable both the
status and the driver of SPI/I2C for the main board. Otherwise,
memory could be wasted for standalone main board users at those
unused SPI/I2C buses -- and the memory might be larger than one
could expect depending on where drivers allocate data buffers.

> Point is that I don't _know_ how this is going to be used, so I'd
> rather keep it flexible.

Well, taking one step back, I am okay to follow your way if you
are really firm about it. Just please give me a more reply and
I will merge this change to that v5, dropping the is_visible().

Actually neither the is_visible nor inX_enable is that essential
for me. I am just trying to do what I feel right.

Thank you
Nicolin

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Guenter Roeck]

Hi Nicolin,

On Wed, Sep 26, 2018 at 02:55:06PM -0700, Nicolin Chen wrote:
> On Wed, Sep 26, 2018 at 01:44:55PM -0700, Guenter Roeck wrote:
> > On Wed, Sep 26, 2018 at 01:25:20PM -0700, Nicolin Chen wrote:
> > > Hello,
> > > 
> > > On Wed, Sep 26, 2018 at 12:58:17PM -0700, Guenter Roeck wrote:
> > > > On Wed, Sep 26, 2018 at 11:02:44AM -0700, Nicolin Chen wrote:
> > > > > On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote:
> > > > > > On 09/25/2018 11:42 PM, Nicolin Chen wrote:
> > > > > > > The inX_enable interface allows user space to enable or disable
> > > > > > > the corresponding channel. Meanwhile, according to hwmon ABI, a
> > > > > > > disabled channel/sensor should return -ENODATA as a read result.
> > > > > > > 
> > > > > > > However, there're configurable nodes sharing the same __show()
> > > > > > > functions. So this change also adds to check if the attribute is
> > > > > > > read-only to make sure it's not reading a configuration but the
> > > > > > > sensor data.
> > > > >  
> > > > > > One necessary high level change I don't see below: With this change,
> > > > > > we should no longer drop a channel entirely if it is disabled from
> > > > > > devicetree. All channels should be visible but report -ENODATA if
> > > > > > disabled. In other words, it should be possible for the 'enable' flag
> > > > > > to override settings in DT.
> > > > > 
> > > > > Hmm...I don't feel so convinced here. The status in DT binding isn't
> > > > > exactly a setting but a physical status: if a hardware design leaves
> > > > > a channel to be disconnected, I don't really see a point in enabling
> > > > > it in the runtime. Or maybe you can shed some light on it?
> > > > > 
> > > > 
> > > > You are making an assumption from your use case. It might as well be that
> > > > some or all channels are disabled in DT by default to conserve power,
> > > > not because they are disconnected.
> > > 
> > > I think I probably should update my DT binding somehow to say it
> > > explicitly that the property should be only used in cases of the
> > > physical disconnections, although I feel the current binding "no
> > > input source" already has the same meaning.
> > > 
> > > In my opinion, disabling channels in DT to save power isn't very
> > > plausible, because it sounds more likely a user decision, while,
> > > as we know, DT merely describes the hardware design.
> > > 
> > 
> > I try to avoid making such assumptions. All I know is that I'll have
> > to deal with the fallout if a single person wants to use the property
> > differently.
> 
> I can understand your point (or pain lol). But I believe in such
> a case, DT maintainers should reject such a DT change. Let's say
> if this kinda "default setting" in DT is allowable, other things
> such as having a default mode setting between polling or one-shot
> modes, and as default critical current settings would be able to
> put into DT. But we know that these would be rejected as a reason
> of "not being hardware design but a user decision".
> 
> > This is similar to disabling an entire subsystem in DT
> > because it isn't used in a specific system - say, a SPI or I2C bus
> > which has nothing connected on some shipping hardware, even though
> > the board has a connector for it. Your argument is that one shall
> > not use the status property do disable loading the driver, and that
> > one must not remove a set of properties for unused hardware either.
> > That doesn't sound very realistic to me.
> 
> SPI/I2C is a good example comparing to my case. And you do make
> a point. In that case, I think DT overlay is designed for it --
> one shall overlay the status property from "disable" (defined in
> the DTS of the shipping hardware -- the main board) to "okay" in
> the overlay DTS where a connection actually happens.
> 
> And even in this case, it makes sense to me to disable both the
> status and the driver of SPI/I2C for the main board. Otherwise,
> memory could be wasted for standalone main board users at those
> unused SPI/I2C buses -- and the memory might be larger than one
> could expect depending on where drivers allocate data buffers.
> 
> > Point is that I don't _know_ how this is going to be used, so I'd
> > rather keep it flexible.
> 
> Well, taking one step back, I am okay to follow your way if you
> are really firm about it. Just please give me a more reply and
> I will merge this change to that v5, dropping the is_visible().

Quite obviously we are not converging. Given that, I think it
would be better to shelve the new attributes for now and to wait
for an actual user who actually needs the new attributes.

Thanks,
Guenter

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Nicolin Chen]

Hello Guenter,

On Thu, Sep 27, 2018 at 09:05:09AM -0700, Guenter Roeck wrote:
> > > Point is that I don't _know_ how this is going to be used, so I'd
> > > rather keep it flexible.
> > 
> > Well, taking one step back, I am okay to follow your way if you
> > are really firm about it. Just please give me a more reply and
> > I will merge this change to that v5, dropping the is_visible().
> 
> Quite obviously we are not converging. Given that, I think it
> would be better to shelve the new attributes for now and to wait
> for an actual user who actually needs the new attributes.

I would rather remove the hiding code, if we have to sacrifice
one of them. We both agreed that in[123]_enable is more useful.
And actually we will still keep is_visible() for empty labels.
So even if one day we somehow feel safe to hide a disconnected
channel one day, it'd only need a couple of lines of changes.

I will squash this change into v6, and will send it after I fix
things that Rob pointed out at DT binding.

Thank you
Nicolin

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Nicolin Chen]

On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote:
> > +static inline bool ina3221_is_enable(struct ina3221_data *ina, int channel)
> 
> s/is_enable/is_enabled/, maybe ?

Fixing.

> > +	return (config & INA3221_CONFIG_CHx_EN(channel)) > 0;
> 
> The "> 0" is unnecessary. Conversion to bool is automatic. If you want to make
> it explicit, please use
> 
> 	return !!(config & INA3221_CONFIG_CHx_EN(channel));

Removing "> 0".

> It should not be necessary to re-read the value from the chip all the time.
> I would suggest to cache the value in the 'disabled' variable.

Regarding this part, I added a cache before sending this one. But
I realized if the chip got powered off and rebooted during system
suspend/resume, the cache would not reflect the actual status. As
I mentioned earlier, this was enlightened by your comments about
the BIOS. So I feel it'd be safer to read the register every time
at this point, until I add the suspend/resume feature by syncing
with regcache. What do you think about it?

> > +	ret = kstrtoint(buf, 0, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* inX_enable only accepts 1 for enabling or 0 for disabling */
> > +	if (val != 0 && val != 1)
> > +		return -EINVAL;
> > +
> Just use kstrtobool().

Fixing.

Thanks
Nicolin

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Guenter Roeck]

On Thu, Sep 27, 2018 at 03:26:14PM -0700, Nicolin Chen wrote:
> On Wed, Sep 26, 2018 at 06:06:32AM -0700, Guenter Roeck wrote:
> > > +static inline bool ina3221_is_enable(struct ina3221_data *ina, int channel)
> > 
> > s/is_enable/is_enabled/, maybe ?
> 
> Fixing.
> 
> > > +	return (config & INA3221_CONFIG_CHx_EN(channel)) > 0;
> > 
> > The "> 0" is unnecessary. Conversion to bool is automatic. If you want to make
> > it explicit, please use
> > 
> > 	return !!(config & INA3221_CONFIG_CHx_EN(channel));
> 
> Removing "> 0".
> 
> > It should not be necessary to re-read the value from the chip all the time.
> > I would suggest to cache the value in the 'disabled' variable.
> 
> Regarding this part, I added a cache before sending this one. But
> I realized if the chip got powered off and rebooted during system
> suspend/resume, the cache would not reflect the actual status. As
> I mentioned earlier, this was enlightened by your comments about
> the BIOS. So I feel it'd be safer to read the register every time
> at this point, until I add the suspend/resume feature by syncing
> with regcache. What do you think about it?
> 

The proper fix for this problem would be to add support for suspend /
resume to the driver. At resume time, all channels will have been
re-enabled if the chip was powered off, even if they were explicitly
disabled by devicetree (or via explicit configuration). This means
the driver just behaves badly across suspend/resume, period.
Displaying a raw value instead of a cached one doesn't solve that
problem. By using a cached value, at least the user would not notice
that the chip no longer does what it is supposed to be doing.

I guess we just have different priorities. If I think suspend/resume
is a problem for my use case, I would just go ahead and fix it.
I would not try to write code that doesn't fix the problem causing it,
much less argue for it.

Having said that, I didn't mention that part in my other reply,
meaning I'll accept the code as is.

Thanks,
Guenter

Re: [PATCH 2/2] hwmon: ina3221: Add enable sysfs nodes

[Nicolin Chen]

On Thu, Sep 27, 2018 at 03:52:00PM -0700, Guenter Roeck wrote:

> The proper fix for this problem would be to add support for suspend /
> resume to the driver. At resume time, all channels will have been
> re-enabled if the chip was powered off, even if they were explicitly
> disabled by devicetree (or via explicit configuration). This means
> the driver just behaves badly across suspend/resume, period.
> Displaying a raw value instead of a cached one doesn't solve that
> problem. By using a cached value, at least the user would not notice
> that the chip no longer does what it is supposed to be doing.
>
> I guess we just have different priorities. If I think suspend/resume
> is a problem for my use case, I would just go ahead and fix it.
> I would not try to write code that doesn't fix the problem causing it,
> much less argue for it.

I agree.

> Having said that, I didn't mention that part in my other reply,
> meaning I'll accept the code as is.

Thanks for the generosity. But, since Rob hasn't acked yet, let
me write the patch for suspend and resume first, which shouldn't
take long.

Thanks
Nicolin