[PATCH v3 0/2] Add an initial DT binding doc for ina3221

[PATCH v3 0/2] Add an initial DT binding doc for ina3221

[Nicolin Chen]

This series adds a initial DT binding doc for ina3221. It defines
a child node to describe the input source of each ina3221 channel.
Then it changes the driver to handle the information properly.

Changelog
v2->v3:
 * Fixed two places in DT bindings (PATCH-1)
v1->v2:
 * Redefined DT bindings (detail in PATCH-1)
 * Changed the driver code accordingly (detail in PATCH-2)

Nicolin Chen (2):
  dt-bindings: hwmon: Add ina3221 documentation
  hwmon: ina3221: Read channel input source info from DT

 .../devicetree/bindings/hwmon/ina3221.txt     |  38 ++++++
 Documentation/hwmon/ina3221                   |   1 +
 drivers/hwmon/ina3221.c                       | 126 ++++++++++++++++--
 3 files changed, 155 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt

-- 
2.17.1

[PATCH v3 1/2] dt-bindings: hwmon: Add ina3221 documentation

[Nicolin Chen]

Texas Instruments INA3221 is a triple-channel shunt and bus
voltage monitor. This patch adds a DT binding doc for it.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
Changelog
v2->v3:
 * Added a simple subject in the line 1
 * Fixed the shunt resistor value in the example
v1->v2:
 * Dropped channel name properties
 * Added child node definitions.
 * * Added shunt resistor property in the child node
 * * Added status property to indicate connection status
 * * Changed to use child node name as the label of input source

 .../devicetree/bindings/hwmon/ina3221.txt     | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt

diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt
new file mode 100644
index 000000000000..bcfd5b9c697b
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt
@@ -0,0 +1,38 @@
+Texas Instruments INA3221 Device Tree Bindings
+
+1) ina3221 node
+  Required properties:
+  - compatible: Must be "ti,ina3221"
+  - reg: I2C address
+
+  = The node contains optional child nodes for three channels =
+  = Each child node describes the information of input source =
+
+  Example:
+
+  ina3221@40 {
+          compatible = "ti,ina3221";
+          reg = <0x40>;
+          [ child node definitions... ]
+  };
+
+2) child nodes
+  The names of child nodes should indicate input source names
+
+  Required properties:
+  - input-id: Must be 1, 2 or 3
+
+  Optional properties:
+  - shunt-resistor: Shunt resistor value in micro-Ohm
+  - status: Should be "disabled" if no input source
+
+  Example:
+
+  input1 {
+          input-id = <0x1>;
+          status = "disabled";
+  };
+  VDD_GPU {
+          input-id = <0x2>;
+          shunt-resistor = <5000>;
+  };
-- 
2.17.1

Re: [PATCH v3 1/2] dt-bindings: hwmon: Add ina3221 documentation

[Guenter Roeck]

Hi,

On 09/21/2018 03:32 PM, Nicolin Chen wrote:
> Texas Instruments INA3221 is a triple-channel shunt and bus
> voltage monitor. This patch adds a DT binding doc for it.
> 
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
> Changelog
> v2->v3:
>   * Added a simple subject in the line 1
>   * Fixed the shunt resistor value in the example
> v1->v2:
>   * Dropped channel name properties
>   * Added child node definitions.
>   * * Added shunt resistor property in the child node
>   * * Added status property to indicate connection status
>   * * Changed to use child node name as the label of input source
> 
>   .../devicetree/bindings/hwmon/ina3221.txt     | 38 +++++++++++++++++++
>   1 file changed, 38 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt
> new file mode 100644
> index 000000000000..bcfd5b9c697b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt
> @@ -0,0 +1,38 @@
> +Texas Instruments INA3221 Device Tree Bindings
> +
> +1) ina3221 node
> +  Required properties:
> +  - compatible: Must be "ti,ina3221"
> +  - reg: I2C address
> +
> +  = The node contains optional child nodes for three channels =
> +  = Each child node describes the information of input source =
> +
> +  Example:
> +
> +  ina3221@40 {
> +          compatible = "ti,ina3221";
> +          reg = <0x40>;
> +          [ child node definitions... ]
> +  };
> +
> +2) child nodes
> +  The names of child nodes should indicate input source names
> +
> +  Required properties:
> +  - input-id: Must be 1, 2 or 3
> +
> +  Optional properties:
> +  - shunt-resistor: Shunt resistor value in micro-Ohm
> +  - status: Should be "disabled" if no input source
> +
> +  Example:
> +
> +  input1 {
> +          input-id = <0x1>;
> +          status = "disabled";
> +  };
> +  VDD_GPU {
> +          input-id = <0x2>;
> +          shunt-resistor = <5000>;
> +  };
> 

Using child nodes is a good idea. However, you are converting the node name into
the hwmon 'label' attribute which I can not accept. First, it is undocumented,
second, it effectively creates an undocumented property (if one wants to configure
the shunt resistor value, one has to configure a child node which is converted
into a label), and third, it violates the hwmon ABI ('input1' is not a "hint
about what this voltage channel is being used for").

Guenter

Re: [PATCH v3 1/2] dt-bindings: hwmon: Add ina3221 documentation

[Nicolin Chen]

> >+2) child nodes
> >+  The names of child nodes should indicate input source names
> >+
> >+  Required properties:
> >+  - input-id: Must be 1, 2 or 3
> >+
> >+  Optional properties:
> >+  - shunt-resistor: Shunt resistor value in micro-Ohm
> >+  - status: Should be "disabled" if no input source
> >+
> >+  Example:
> >+
> >+  input1 {
> >+          input-id = <0x1>;
> >+          status = "disabled";
> >+  };
> >+  VDD_GPU {
> >+          input-id = <0x2>;
> >+          shunt-resistor = <5000>;
> >+  };
> >
> 
> Using child nodes is a good idea. However, you are converting the node name into
> the hwmon 'label' attribute which I can not accept. First, it is undocumented,
> second, it effectively creates an undocumented property (if one wants to configure
> the shunt resistor value, one has to configure a child node which is converted
> into a label), and third, it violates the hwmon ABI ('input1' is not a "hint
> about what this voltage channel is being used for").

Oh. I see the point here now. Then a child name could be just input[123],
and I will add a separate optional child property to indicate the label.

Will fix it in next ver.

Thanks
Nicolin

Re: [PATCH v3 1/2] dt-bindings: hwmon: Add ina3221 documentation

[Guenter Roeck]

On 09/22/2018 11:03 AM, Nicolin Chen wrote:
>>> +2) child nodes
>>> +  The names of child nodes should indicate input source names
>>> +
>>> +  Required properties:
>>> +  - input-id: Must be 1, 2 or 3
>>> +
>>> +  Optional properties:
>>> +  - shunt-resistor: Shunt resistor value in micro-Ohm
>>> +  - status: Should be "disabled" if no input source
>>> +
>>> +  Example:
>>> +
>>> +  input1 {
>>> +          input-id = <0x1>;
>>> +          status = "disabled";
>>> +  };
>>> +  VDD_GPU {
>>> +          input-id = <0x2>;
>>> +          shunt-resistor = <5000>;
>>> +  };
>>>
>>
>> Using child nodes is a good idea. However, you are converting the node name into
>> the hwmon 'label' attribute which I can not accept. First, it is undocumented,
>> second, it effectively creates an undocumented property (if one wants to configure
>> the shunt resistor value, one has to configure a child node which is converted
>> into a label), and third, it violates the hwmon ABI ('input1' is not a "hint
>> about what this voltage channel is being used for").
> 
> Oh. I see the point here now. Then a child name could be just input[123],
> and I will add a separate optional child property to indicate the label.
> 
Exactly. "label" is quite widely used as property name, so that should be acceptable.
I am not sure about index; we'll see if Rob has any comments.

Thanks,
Guenter

[PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Nicolin Chen]

From: Nicolin Chen <nicolinc@nvidia.com>

An ina3221 chip has three input ports. Each port is used
to measure the voltage and current of its input source.

The DT binding now has defined bindings for their input
sources, so the driver should read these information and
handle accordingly.

This patch adds a new structure of input source specific
information including input source label, shunt resistor
value and its connection status. It exposes these labels
via sysfs and also disables those channels where there's
no input source being connected.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
Changelog
v2->v3:
 * N/A
v1->v2:
 * Added a structure for input source corresponding to DT bindings
 * Moved shunt resistor value to the structure
 * Defined in[123]_label sysfs nodes instead of name[123]_input
 * Updated probe() function to get information from DT
 * Updated ina3221 hwmon documentation for the labels
 * Dropped dynamical group definition to keep all groups as they were
 * * Will send an incremental patch later apart from this DT binding series

 Documentation/hwmon/ina3221 |   1 +
 drivers/hwmon/ina3221.c     | 126 +++++++++++++++++++++++++++++++++---
 2 files changed, 117 insertions(+), 10 deletions(-)

diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
index 0ff74854cb2e..2726038be5bd 100644
--- a/Documentation/hwmon/ina3221
+++ b/Documentation/hwmon/ina3221
@@ -22,6 +22,7 @@ Sysfs entries
 -------------
 
 in[123]_input           Bus voltage(mV) channels
+in[123]_label           Voltage channel labels
 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 e6b49500c52a..ba470fc53e27 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -41,6 +41,7 @@
 #define INA3221_CONFIG_MODE_SHUNT	BIT(1)
 #define INA3221_CONFIG_MODE_BUS		BIT(2)
 #define INA3221_CONFIG_MODE_CONTINUOUS	BIT(3)
+#define INA3221_CONFIG_CHx_EN(x)	BIT(14 - (x))
 
 #define INA3221_RSHUNT_DEFAULT		10000
 
@@ -86,16 +87,28 @@ static const unsigned int register_channel[] = {
 	[INA3221_WARN3] = INA3221_CHANNEL3,
 };
 
+/**
+ * struct ina3221_input - channel input source specific information
+ * @label: label of channel input source
+ * @shunt_resistor: shunt resistor value of channel input source
+ * @disconnected: connection status of channel input source
+ */
+struct ina3221_input {
+	const char *label;
+	int shunt_resistor;
+	bool disconnected;
+};
+
 /**
  * struct ina3221_data - device specific information
  * @regmap: Register map of the device
  * @fields: Register fields of the device
- * @shunt_resistors: Array of resistor values per channel
+ * @inputs: Array of channel input source specific structures
  */
 struct ina3221_data {
 	struct regmap *regmap;
 	struct regmap_field *fields[F_MAX_FIELDS];
-	int shunt_resistors[INA3221_NUM_CHANNELS];
+	struct ina3221_input inputs[INA3221_NUM_CHANNELS];
 };
 
 static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
@@ -131,6 +144,17 @@ static ssize_t ina3221_show_bus_voltage(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", voltage_mv);
 }
 
+static ssize_t ina3221_show_label(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;
+	struct ina3221_input *input = &ina->inputs[channel];
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", input->label);
+}
+
 static ssize_t ina3221_show_shunt_voltage(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf)
@@ -155,7 +179,8 @@ static ssize_t ina3221_show_current(struct device *dev,
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	unsigned int reg = sd_attr->index;
 	unsigned int channel = register_channel[reg];
-	int resistance_uo = ina->shunt_resistors[channel];
+	struct ina3221_input *input = &ina->inputs[channel];
+	int resistance_uo = input->shunt_resistor;
 	int val, current_ma, voltage_nv, ret;
 
 	ret = ina3221_read_value(ina, reg, &val);
@@ -176,7 +201,8 @@ static ssize_t ina3221_set_current(struct device *dev,
 	struct ina3221_data *ina = dev_get_drvdata(dev);
 	unsigned int reg = sd_attr->index;
 	unsigned int channel = register_channel[reg];
-	int resistance_uo = ina->shunt_resistors[channel];
+	struct ina3221_input *input = &ina->inputs[channel];
+	int resistance_uo = input->shunt_resistor;
 	int val, current_ma, voltage_uv, ret;
 
 	ret = kstrtoint(buf, 0, &current_ma);
@@ -209,11 +235,9 @@ static ssize_t ina3221_show_shunt(struct device *dev,
 	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 resistance_uo;
+	struct ina3221_input *input = &ina->inputs[channel];
 
-	resistance_uo = ina->shunt_resistors[channel];
-
-	return snprintf(buf, PAGE_SIZE, "%d\n", resistance_uo);
+	return snprintf(buf, PAGE_SIZE, "%d\n", input->shunt_resistor);
 }
 
 static ssize_t ina3221_set_shunt(struct device *dev,
@@ -223,6 +247,7 @@ static ssize_t ina3221_set_shunt(struct device *dev,
 	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;
+	struct ina3221_input *input = &ina->inputs[channel];
 	int val;
 	int ret;
 
@@ -232,7 +257,7 @@ static ssize_t ina3221_set_shunt(struct device *dev,
 
 	val = clamp_val(val, 1, INT_MAX);
 
-	ina->shunt_resistors[channel] = val;
+	input->shunt_resistor = val;
 
 	return count;
 }
@@ -261,6 +286,14 @@ static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO,
 static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO,
 		ina3221_show_bus_voltage, NULL, INA3221_BUS3);
 
+/* voltage channel label */
+static SENSOR_DEVICE_ATTR(in1_label, 0444,
+		ina3221_show_label, NULL, INA3221_CHANNEL1);
+static SENSOR_DEVICE_ATTR(in2_label, 0444,
+		ina3221_show_label, NULL, INA3221_CHANNEL2);
+static SENSOR_DEVICE_ATTR(in3_label, 0444,
+		ina3221_show_label, NULL, INA3221_CHANNEL3);
+
 /* calculated current */
 static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO,
 		ina3221_show_current, NULL, INA3221_SHUNT1);
@@ -320,6 +353,7 @@ static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
 static struct attribute *ina3221_attrs[] = {
 	/* channel 1 */
 	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_in1_label.dev_attr.attr,
 	&sensor_dev_attr_curr1_input.dev_attr.attr,
 	&sensor_dev_attr_shunt1_resistor.dev_attr.attr,
 	&sensor_dev_attr_curr1_crit.dev_attr.attr,
@@ -330,6 +364,7 @@ static struct attribute *ina3221_attrs[] = {
 
 	/* channel 2 */
 	&sensor_dev_attr_in2_input.dev_attr.attr,
+	&sensor_dev_attr_in2_label.dev_attr.attr,
 	&sensor_dev_attr_curr2_input.dev_attr.attr,
 	&sensor_dev_attr_shunt2_resistor.dev_attr.attr,
 	&sensor_dev_attr_curr2_crit.dev_attr.attr,
@@ -340,6 +375,7 @@ static struct attribute *ina3221_attrs[] = {
 
 	/* channel 3 */
 	&sensor_dev_attr_in3_input.dev_attr.attr,
+	&sensor_dev_attr_in3_label.dev_attr.attr,
 	&sensor_dev_attr_curr3_input.dev_attr.attr,
 	&sensor_dev_attr_shunt3_resistor.dev_attr.attr,
 	&sensor_dev_attr_curr3_crit.dev_attr.attr,
@@ -370,6 +406,60 @@ static const struct regmap_config ina3221_regmap_config = {
 	.volatile_table = &ina3221_volatile_table,
 };
 
+static int ina3221_probe_child_from_dt(struct device *dev,
+				       struct device_node *child,
+				       struct ina3221_data *ina)
+{
+	struct ina3221_input *input;
+	u32 val;
+	int ret;
+
+	ret = of_property_read_u32(child, "input-id", &val);
+	if (ret) {
+		dev_err(dev, "missing input-id property of %s\n", child->name);
+		return ret;
+	} else if (val < 1 || val > INA3221_NUM_CHANNELS) {
+		dev_err(dev, "invalid input-id %d of %s\n", val, child->name);
+		return ret;
+	}
+
+	input = &ina->inputs[val - 1];
+
+	/* Log the disconnected channel input */
+	if (!of_device_is_available(child)) {
+		input->disconnected = true;
+		return 0;
+	}
+
+	/* Log the connected channel input */
+	input->label = child->name;
+
+	/* Overwrite default shunt resistor value optionally */
+	if (!of_property_read_u32(child, "shunt-resistor", &val))
+		input->shunt_resistor = val;
+
+	return 0;
+}
+
+static int ina3221_probe_from_dt(struct device *dev, struct ina3221_data *ina)
+{
+	const struct device_node *np = dev->of_node;
+	struct device_node *child;
+	int ret;
+
+	/* Compatible with non-DT platforms */
+	if (!np)
+		return 0;
+
+	for_each_child_of_node(np, child) {
+		ret = ina3221_probe_child_from_dt(dev, child, ina);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int ina3221_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -377,6 +467,7 @@ static int ina3221_probe(struct i2c_client *client,
 	struct ina3221_data *ina;
 	struct device *hwmon_dev;
 	int i, ret;
+	u16 mask;
 
 	ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL);
 	if (!ina)
@@ -399,7 +490,13 @@ static int ina3221_probe(struct i2c_client *client,
 	}
 
 	for (i = 0; i < INA3221_NUM_CHANNELS; i++)
-		ina->shunt_resistors[i] = INA3221_RSHUNT_DEFAULT;
+		ina->inputs[i].shunt_resistor = INA3221_RSHUNT_DEFAULT;
+
+	ret = ina3221_probe_from_dt(dev, ina);
+	if (ret) {
+		dev_err(dev, "Unable to probe from device treee\n");
+		return ret;
+	}
 
 	ret = regmap_field_write(ina->fields[F_RST], true);
 	if (ret) {
@@ -407,6 +504,15 @@ static int ina3221_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	/* Disable channels if their inputs are disconnected */
+	for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
+		if (ina->inputs[i].disconnected)
+			mask |= INA3221_CONFIG_CHx_EN(i);
+	}
+	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, 0);
+	if (ret)
+		return ret;
+
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
 							   client->name,
 							   ina, ina3221_groups);
-- 
2.17.1

Re: [PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Guenter Roeck]

Hi,

On 09/21/2018 03:32 PM, Nicolin Chen wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
> 
> An ina3221 chip has three input ports. Each port is used
> to measure the voltage and current of its input source.
> 
> The DT binding now has defined bindings for their input
> sources, so the driver should read these information and
> handle accordingly.
> 
> This patch adds a new structure of input source specific
> information including input source label, shunt resistor
> value and its connection status. It exposes these labels
> via sysfs and also disables those channels where there's
> no input source being connected.
> 

I see you have decided to just display the disconnected channels.
This is misleading, and I can not accept it. Please either use the
is_visible callback to not display those channels at all, or have
the _input attribute of disabled channels return -ENODATA (see
'in[0-*]_enable' attribute in the ABI). If you implement the latter,
I would suggest to also implement the _enable attribute.

As mentioned in patch 1, I can not accept an implicitly mandatory
label attribute. The property defining the label attribute will
have to be optional and well defined to ensure that it matches
the ABI.

One more comment inline below.

Thanks,
Guenter

> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> Changelog
> v2->v3:
>   * N/A
> v1->v2:
>   * Added a structure for input source corresponding to DT bindings
>   * Moved shunt resistor value to the structure
>   * Defined in[123]_label sysfs nodes instead of name[123]_input
>   * Updated probe() function to get information from DT
>   * Updated ina3221 hwmon documentation for the labels
>   * Dropped dynamical group definition to keep all groups as they were
>   * * Will send an incremental patch later apart from this DT binding series
> 
>   Documentation/hwmon/ina3221 |   1 +
>   drivers/hwmon/ina3221.c     | 126 +++++++++++++++++++++++++++++++++---
>   2 files changed, 117 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
> index 0ff74854cb2e..2726038be5bd 100644
> --- a/Documentation/hwmon/ina3221
> +++ b/Documentation/hwmon/ina3221
> @@ -22,6 +22,7 @@ Sysfs entries
>   -------------
>   
>   in[123]_input           Bus voltage(mV) channels
> +in[123]_label           Voltage channel labels
>   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 e6b49500c52a..ba470fc53e27 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -41,6 +41,7 @@
>   #define INA3221_CONFIG_MODE_SHUNT	BIT(1)
>   #define INA3221_CONFIG_MODE_BUS		BIT(2)
>   #define INA3221_CONFIG_MODE_CONTINUOUS	BIT(3)
> +#define INA3221_CONFIG_CHx_EN(x)	BIT(14 - (x))
>   
>   #define INA3221_RSHUNT_DEFAULT		10000
>   
> @@ -86,16 +87,28 @@ static const unsigned int register_channel[] = {
>   	[INA3221_WARN3] = INA3221_CHANNEL3,
>   };
>   
> +/**
> + * struct ina3221_input - channel input source specific information
> + * @label: label of channel input source
> + * @shunt_resistor: shunt resistor value of channel input source
> + * @disconnected: connection status of channel input source
> + */
> +struct ina3221_input {
> +	const char *label;
> +	int shunt_resistor;
> +	bool disconnected;
> +};
> +
>   /**
>    * struct ina3221_data - device specific information
>    * @regmap: Register map of the device
>    * @fields: Register fields of the device
> - * @shunt_resistors: Array of resistor values per channel
> + * @inputs: Array of channel input source specific structures
>    */
>   struct ina3221_data {
>   	struct regmap *regmap;
>   	struct regmap_field *fields[F_MAX_FIELDS];
> -	int shunt_resistors[INA3221_NUM_CHANNELS];
> +	struct ina3221_input inputs[INA3221_NUM_CHANNELS];
>   };
>   
>   static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
> @@ -131,6 +144,17 @@ static ssize_t ina3221_show_bus_voltage(struct device *dev,
>   	return snprintf(buf, PAGE_SIZE, "%d\n", voltage_mv);
>   }
>   
> +static ssize_t ina3221_show_label(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;
> +	struct ina3221_input *input = &ina->inputs[channel];
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", input->label);
> +}
> +
>   static ssize_t ina3221_show_shunt_voltage(struct device *dev,
>   					  struct device_attribute *attr,
>   					  char *buf)
> @@ -155,7 +179,8 @@ static ssize_t ina3221_show_current(struct device *dev,
>   	struct ina3221_data *ina = dev_get_drvdata(dev);
>   	unsigned int reg = sd_attr->index;
>   	unsigned int channel = register_channel[reg];
> -	int resistance_uo = ina->shunt_resistors[channel];
> +	struct ina3221_input *input = &ina->inputs[channel];
> +	int resistance_uo = input->shunt_resistor;
>   	int val, current_ma, voltage_nv, ret;
>   
>   	ret = ina3221_read_value(ina, reg, &val);
> @@ -176,7 +201,8 @@ static ssize_t ina3221_set_current(struct device *dev,
>   	struct ina3221_data *ina = dev_get_drvdata(dev);
>   	unsigned int reg = sd_attr->index;
>   	unsigned int channel = register_channel[reg];
> -	int resistance_uo = ina->shunt_resistors[channel];
> +	struct ina3221_input *input = &ina->inputs[channel];
> +	int resistance_uo = input->shunt_resistor;
>   	int val, current_ma, voltage_uv, ret;
>   
>   	ret = kstrtoint(buf, 0, &current_ma);
> @@ -209,11 +235,9 @@ static ssize_t ina3221_show_shunt(struct device *dev,
>   	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 resistance_uo;
> +	struct ina3221_input *input = &ina->inputs[channel];
>   
> -	resistance_uo = ina->shunt_resistors[channel];
> -
> -	return snprintf(buf, PAGE_SIZE, "%d\n", resistance_uo);
> +	return snprintf(buf, PAGE_SIZE, "%d\n", input->shunt_resistor);
>   }
>   
>   static ssize_t ina3221_set_shunt(struct device *dev,
> @@ -223,6 +247,7 @@ static ssize_t ina3221_set_shunt(struct device *dev,
>   	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;
> +	struct ina3221_input *input = &ina->inputs[channel];
>   	int val;
>   	int ret;
>   
> @@ -232,7 +257,7 @@ static ssize_t ina3221_set_shunt(struct device *dev,
>   
>   	val = clamp_val(val, 1, INT_MAX);
>   
> -	ina->shunt_resistors[channel] = val;
> +	input->shunt_resistor = val;
>   
>   	return count;
>   }
> @@ -261,6 +286,14 @@ static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO,
>   static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO,
>   		ina3221_show_bus_voltage, NULL, INA3221_BUS3);
>   
> +/* voltage channel label */
> +static SENSOR_DEVICE_ATTR(in1_label, 0444,
> +		ina3221_show_label, NULL, INA3221_CHANNEL1);
> +static SENSOR_DEVICE_ATTR(in2_label, 0444,
> +		ina3221_show_label, NULL, INA3221_CHANNEL2);
> +static SENSOR_DEVICE_ATTR(in3_label, 0444,
> +		ina3221_show_label, NULL, INA3221_CHANNEL3);
> +
>   /* calculated current */
>   static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO,
>   		ina3221_show_current, NULL, INA3221_SHUNT1);
> @@ -320,6 +353,7 @@ static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
>   static struct attribute *ina3221_attrs[] = {
>   	/* channel 1 */
>   	&sensor_dev_attr_in1_input.dev_attr.attr,
> +	&sensor_dev_attr_in1_label.dev_attr.attr,
>   	&sensor_dev_attr_curr1_input.dev_attr.attr,
>   	&sensor_dev_attr_shunt1_resistor.dev_attr.attr,
>   	&sensor_dev_attr_curr1_crit.dev_attr.attr,
> @@ -330,6 +364,7 @@ static struct attribute *ina3221_attrs[] = {
>   
>   	/* channel 2 */
>   	&sensor_dev_attr_in2_input.dev_attr.attr,
> +	&sensor_dev_attr_in2_label.dev_attr.attr,
>   	&sensor_dev_attr_curr2_input.dev_attr.attr,
>   	&sensor_dev_attr_shunt2_resistor.dev_attr.attr,
>   	&sensor_dev_attr_curr2_crit.dev_attr.attr,
> @@ -340,6 +375,7 @@ static struct attribute *ina3221_attrs[] = {
>   
>   	/* channel 3 */
>   	&sensor_dev_attr_in3_input.dev_attr.attr,
> +	&sensor_dev_attr_in3_label.dev_attr.attr,
>   	&sensor_dev_attr_curr3_input.dev_attr.attr,
>   	&sensor_dev_attr_shunt3_resistor.dev_attr.attr,
>   	&sensor_dev_attr_curr3_crit.dev_attr.attr,
> @@ -370,6 +406,60 @@ static const struct regmap_config ina3221_regmap_config = {
>   	.volatile_table = &ina3221_volatile_table,
>   };
>   
> +static int ina3221_probe_child_from_dt(struct device *dev,
> +				       struct device_node *child,
> +				       struct ina3221_data *ina)
> +{
> +	struct ina3221_input *input;
> +	u32 val;
> +	int ret;
> +
> +	ret = of_property_read_u32(child, "input-id", &val);
> +	if (ret) {
> +		dev_err(dev, "missing input-id property of %s\n", child->name);
> +		return ret;
> +	} else if (val < 1 || val > INA3221_NUM_CHANNELS) {
> +		dev_err(dev, "invalid input-id %d of %s\n", val, child->name);
> +		return ret;
> +	}
> +
> +	input = &ina->inputs[val - 1];
> +
> +	/* Log the disconnected channel input */
> +	if (!of_device_is_available(child)) {
> +		input->disconnected = true;
> +		return 0;
> +	}
> +
> +	/* Log the connected channel input */
> +	input->label = child->name;
> +
> +	/* Overwrite default shunt resistor value optionally */
> +	if (!of_property_read_u32(child, "shunt-resistor", &val))
> +		input->shunt_resistor = val;
> +
> +	return 0;
> +}
> +
> +static int ina3221_probe_from_dt(struct device *dev, struct ina3221_data *ina)
> +{
> +	const struct device_node *np = dev->of_node;
> +	struct device_node *child;
> +	int ret;
> +
> +	/* Compatible with non-DT platforms */
> +	if (!np)
> +		return 0;
> +
> +	for_each_child_of_node(np, child) {
> +		ret = ina3221_probe_child_from_dt(dev, child, ina);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   static int ina3221_probe(struct i2c_client *client,
>   			 const struct i2c_device_id *id)
>   {
> @@ -377,6 +467,7 @@ static int ina3221_probe(struct i2c_client *client,
>   	struct ina3221_data *ina;
>   	struct device *hwmon_dev;
>   	int i, ret;
> +	u16 mask;
>   
>   	ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL);
>   	if (!ina)
> @@ -399,7 +490,13 @@ static int ina3221_probe(struct i2c_client *client,
>   	}
>   
>   	for (i = 0; i < INA3221_NUM_CHANNELS; i++)
> -		ina->shunt_resistors[i] = INA3221_RSHUNT_DEFAULT;
> +		ina->inputs[i].shunt_resistor = INA3221_RSHUNT_DEFAULT;
> +
> +	ret = ina3221_probe_from_dt(dev, ina);
> +	if (ret) {
> +		dev_err(dev, "Unable to probe from device treee\n");
> +		return ret;
> +	}
>   
>   	ret = regmap_field_write(ina->fields[F_RST], true);
>   	if (ret) {
> @@ -407,6 +504,15 @@ static int ina3221_probe(struct i2c_client *client,
>   		return ret;
>   	}
>   
> +	/* Disable channels if their inputs are disconnected */
> +	for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
> +		if (ina->inputs[i].disconnected)
> +			mask |= INA3221_CONFIG_CHx_EN(i);
> +	}

Consequently, you should also _enable_ channels which are not explicitly disabled.
This can be tricky since you'll have to distinguish non-DT and DT configuration
and retain the original configuration if no channel configuration data is available
from devicetree.

> +	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, 0);
> +	if (ret)
> +		return ret;
> +
>   	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
>   							   client->name,
>   							   ina, ina3221_groups);
> 

Re: [PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Nicolin Chen]

> >This patch adds a new structure of input source specific
> >information including input source label, shunt resistor
> >value and its connection status. It exposes these labels
> >via sysfs and also disables those channels where there's
> >no input source being connected.
> >
> 
> I see you have decided to just display the disconnected channels.
> This is misleading, and I can not accept it. Please either use the
> is_visible callback to not display those channels at all, or have

I will add is_visible. I have almost finished it while waiting for
the v3's review comments. Will test it and include in the v4.

> the _input attribute of disabled channels return -ENODATA (see
> 'in[0-*]_enable' attribute in the ABI). If you implement the latter,
> I would suggest to also implement the _enable attribute.

I will also add one separate patch for in[0-*]_enable after these
two changes pass the review and get applied.

> As mentioned in patch 1, I can not accept an implicitly mandatory
> label attribute. The property defining the label attribute will
> have to be optional and well defined to ensure that it matches
> the ABI.

I replied this in the PATCH-1. Let's discuss this topic there.

> >+	/* Disable channels if their inputs are disconnected */
> >+	for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
> >+		if (ina->inputs[i].disconnected)
> >+			mask |= INA3221_CONFIG_CHx_EN(i);
> >+	}
> 
> Consequently, you should also _enable_ channels which are not explicitly disabled.

The register has enabled all channels by default. So I felt it'd
be neat to have disabling code only. My v1 actually had enabling
part as well, but I can add it back if you think it'd be better.

> This can be tricky since you'll have to distinguish non-DT and DT configuration
> and retain the original configuration if no channel configuration data is available
> from devicetree.

I don't quite understand this comments. Would you please elaborate
it?

For non-DT configurations, input->disconnected is always false by
default unless someone adds config for it (through platform_data).
If regmap_update_bits only does disabling like this version does,
non-DT configurations will not get affected since mask = 0. Or if
we change it to do both enabling and disabling, regmap_update_bits
will still ignore since there's no register value changed, though
it won't really hurt even if regmap writes correct configurations
to the register.

For DT configurations (without channel input source defined), it's
like the same as non-DT configurations. As we have platforms only
enabled ina3221 via DT while they don't have this new DT binding,
the driver has to be backward compatible, so my change only sets
input->disconnected=true when a status="disabled" is present, i.e.
those platforms are treated as all channels getting enabled until
they update their DTs.

Thanks for the review
Nicolin

Re: [PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Guenter Roeck]

On 09/22/2018 11:46 AM, Nicolin Chen wrote:
>>> This patch adds a new structure of input source specific
>>> information including input source label, shunt resistor
>>> value and its connection status. It exposes these labels
>>> via sysfs and also disables those channels where there's
>>> no input source being connected.
>>>
>>
>> I see you have decided to just display the disconnected channels.
>> This is misleading, and I can not accept it. Please either use the
>> is_visible callback to not display those channels at all, or have
> 
> I will add is_visible. I have almost finished it while waiting for
> the v3's review comments. Will test it and include in the v4.
> 
>> the _input attribute of disabled channels return -ENODATA (see
>> 'in[0-*]_enable' attribute in the ABI). If you implement the latter,
>> I would suggest to also implement the _enable attribute.
> 
> I will also add one separate patch for in[0-*]_enable after these
> two changes pass the review and get applied.
> 
>> As mentioned in patch 1, I can not accept an implicitly mandatory
>> label attribute. The property defining the label attribute will
>> have to be optional and well defined to ensure that it matches
>> the ABI.
> 
> I replied this in the PATCH-1. Let's discuss this topic there.
> 
>>> +	/* Disable channels if their inputs are disconnected */
>>> +	for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
>>> +		if (ina->inputs[i].disconnected)
>>> +			mask |= INA3221_CONFIG_CHx_EN(i);
>>> +	}
>>
>> Consequently, you should also _enable_ channels which are not explicitly disabled.
> 
> The register has enabled all channels by default. So I felt it'd
> be neat to have disabling code only. My v1 actually had enabling
> part as well, but I can add it back if you think it'd be better.
> 
>> This can be tricky since you'll have to distinguish non-DT and DT configuration
>> and retain the original configuration if no channel configuration data is available
>> from devicetree.
> 
> I don't quite understand this comments. Would you please elaborate
> it?
> 
> For non-DT configurations, input->disconnected is always false by
> default unless someone adds config for it (through platform_data).
> If regmap_update_bits only does disabling like this version does,
> non-DT configurations will not get affected since mask = 0. Or if
> we change it to do both enabling and disabling, regmap_update_bits
> will still ignore since there's no register value changed, though
> it won't really hurt even if regmap writes correct configurations
> to the register.
> 
> For DT configurations (without channel input source defined), it's
> like the same as non-DT configurations. As we have platforms only
> enabled ina3221 via DT while they don't have this new DT binding,
> the driver has to be backward compatible, so my change only sets
> input->disconnected=true when a status="disabled" is present, i.e.
> those platforms are treated as all channels getting enabled until
> they update their DTs.
> 

I think your assumption may be that the chip is always in its reset state
when Linux is loaded. This is not necessarily the case; it may be
preconfigured by BIOS or ROMMON, or even by someone using i2cset before
loading the driver. If you add enable/disable functionality, you can
not make an assumption about the original state of the chip at probe time;
you have to read it from the chip itself.

Thanks,
Guenter

Re: [PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Nicolin Chen]

On Sat, Sep 22, 2018 at 04:59:55PM -0700, Guenter Roeck wrote:

> > > > +	/* Disable channels if their inputs are disconnected */
> > > > +	for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
> > > > +		if (ina->inputs[i].disconnected)
> > > > +			mask |= INA3221_CONFIG_CHx_EN(i);
> > > > +	}
> > > 
> > > Consequently, you should also _enable_ channels which are not explicitly disabled.
> > 
> > The register has enabled all channels by default. So I felt it'd
> > be neat to have disabling code only. My v1 actually had enabling
> > part as well, but I can add it back if you think it'd be better.
> > 
> > > This can be tricky since you'll have to distinguish non-DT and DT configuration
> > > and retain the original configuration if no channel configuration data is available
> > > from devicetree.

> > For non-DT configurations, input->disconnected is always false by
> > default unless someone adds config for it (through platform_data).
> > If regmap_update_bits only does disabling like this version does,
> > non-DT configurations will not get affected since mask = 0. Or if
> > we change it to do both enabling and disabling, regmap_update_bits
> > will still ignore since there's no register value changed, though
> > it won't really hurt even if regmap writes correct configurations
> > to the register.
> > 
> > For DT configurations (without channel input source defined), it's
> > like the same as non-DT configurations. As we have platforms only
> > enabled ina3221 via DT while they don't have this new DT binding,
> > the driver has to be backward compatible, so my change only sets
> > input->disconnected=true when a status="disabled" is present, i.e.
> > those platforms are treated as all channels getting enabled until
> > they update their DTs.

> I think your assumption may be that the chip is always in its reset state
> when Linux is loaded. This is not necessarily the case; it may be
> preconfigured by BIOS or ROMMON, or even by someone using i2cset before
> loading the driver. If you add enable/disable functionality, you can
> not make an assumption about the original state of the chip at probe time;
> you have to read it from the chip itself.

I see. That made a point. In that case, I think the simplest way
is probably to do software reset before having configurations.

The "disconnected" here is to describe the physical connection,
not exact the switch control status because a channel could be
connected but disabled. However, a channel would not be enabled
if it's disconnected. So I think it'd be safe to just disable
the disconnected channels here as this version does, meanwhile,
I will add a soft reset to make sure all channels are enabled.

Thanks
Nicolin

Re: [PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Nicolin Chen]

On Sat, Sep 22, 2018 at 05:38:41PM -0700, Nicolin Chen wrote:
> On Sat, Sep 22, 2018 at 04:59:55PM -0700, Guenter Roeck wrote:
> 
> > > > > +	/* Disable channels if their inputs are disconnected */
> > > > > +	for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
> > > > > +		if (ina->inputs[i].disconnected)
> > > > > +			mask |= INA3221_CONFIG_CHx_EN(i);
> > > > > +	}
> > > > 
> > > > Consequently, you should also _enable_ channels which are not explicitly disabled.
> > > 
> > > The register has enabled all channels by default. So I felt it'd
> > > be neat to have disabling code only. My v1 actually had enabling
> > > part as well, but I can add it back if you think it'd be better.
> > > 
> > > > This can be tricky since you'll have to distinguish non-DT and DT configuration
> > > > and retain the original configuration if no channel configuration data is available
> > > > from devicetree.
> 
> > > For non-DT configurations, input->disconnected is always false by
> > > default unless someone adds config for it (through platform_data).
> > > If regmap_update_bits only does disabling like this version does,
> > > non-DT configurations will not get affected since mask = 0. Or if
> > > we change it to do both enabling and disabling, regmap_update_bits
> > > will still ignore since there's no register value changed, though
> > > it won't really hurt even if regmap writes correct configurations
> > > to the register.
> > > 
> > > For DT configurations (without channel input source defined), it's
> > > like the same as non-DT configurations. As we have platforms only
> > > enabled ina3221 via DT while they don't have this new DT binding,
> > > the driver has to be backward compatible, so my change only sets
> > > input->disconnected=true when a status="disabled" is present, i.e.
> > > those platforms are treated as all channels getting enabled until
> > > they update their DTs.
> 
> > I think your assumption may be that the chip is always in its reset state
> > when Linux is loaded. This is not necessarily the case; it may be
> > preconfigured by BIOS or ROMMON, or even by someone using i2cset before
> > loading the driver. If you add enable/disable functionality, you can
> > not make an assumption about the original state of the chip at probe time;
> > you have to read it from the chip itself.
> 
> I see. That made a point. In that case, I think the simplest way
> is probably to do software reset before having configurations.
> 
> The "disconnected" here is to describe the physical connection,
> not exact the switch control status because a channel could be
> connected but disabled. However, a channel would not be enabled
> if it's disconnected. So I think it'd be safe to just disable
> the disconnected channels here as this version does, meanwhile,
> I will add a soft reset to make sure all channels are enabled.

Actually...just a couple of lines before this, the probe() does
soft reset:

537         ret = regmap_field_write(ina->fields[F_RST], true);
538         if (ret) {
539                 dev_err(dev, "Unable to reset device\n");
540                 return ret;
541         }

So I think it'd be okay to just keep disabling disconnected
channels only as this version?

Re: [PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Guenter Roeck]

On 09/22/2018 05:38 PM, Nicolin Chen wrote:
> On Sat, Sep 22, 2018 at 04:59:55PM -0700, Guenter Roeck wrote:
> 
>>>>> +	/* Disable channels if their inputs are disconnected */
>>>>> +	for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
>>>>> +		if (ina->inputs[i].disconnected)
>>>>> +			mask |= INA3221_CONFIG_CHx_EN(i);
>>>>> +	}
>>>>
>>>> Consequently, you should also _enable_ channels which are not explicitly disabled.
>>>
>>> The register has enabled all channels by default. So I felt it'd
>>> be neat to have disabling code only. My v1 actually had enabling
>>> part as well, but I can add it back if you think it'd be better.
>>>
>>>> This can be tricky since you'll have to distinguish non-DT and DT configuration
>>>> and retain the original configuration if no channel configuration data is available
>>>> from devicetree.
> 
>>> For non-DT configurations, input->disconnected is always false by
>>> default unless someone adds config for it (through platform_data).
>>> If regmap_update_bits only does disabling like this version does,
>>> non-DT configurations will not get affected since mask = 0. Or if
>>> we change it to do both enabling and disabling, regmap_update_bits
>>> will still ignore since there's no register value changed, though
>>> it won't really hurt even if regmap writes correct configurations
>>> to the register.
>>>
>>> For DT configurations (without channel input source defined), it's
>>> like the same as non-DT configurations. As we have platforms only
>>> enabled ina3221 via DT while they don't have this new DT binding,
>>> the driver has to be backward compatible, so my change only sets
>>> input->disconnected=true when a status="disabled" is present, i.e.
>>> those platforms are treated as all channels getting enabled until
>>> they update their DTs.
> 
>> I think your assumption may be that the chip is always in its reset state
>> when Linux is loaded. This is not necessarily the case; it may be
>> preconfigured by BIOS or ROMMON, or even by someone using i2cset before
>> loading the driver. If you add enable/disable functionality, you can
>> not make an assumption about the original state of the chip at probe time;
>> you have to read it from the chip itself.
> 
> I see. That made a point. In that case, I think the simplest way
> is probably to do software reset before having configurations.
> 
No. If the chip was configured by the BIOS/ROMMON, it is supposed
to be that way. We can not just override that.

Guenter

> The "disconnected" here is to describe the physical connection,
> not exact the switch control status because a channel could be
> connected but disabled. However, a channel would not be enabled
> if it's disconnected. So I think it'd be safe to just disable
> the disconnected channels here as this version does, meanwhile,
> I will add a soft reset to make sure all channels are enabled.
> 
> Thanks
> Nicolin
> 

Re: [PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Nicolin Chen]

On Sat, Sep 22, 2018 at 07:07:02PM -0700, Guenter Roeck wrote:
> On 09/22/2018 05:38 PM, Nicolin Chen wrote:
> >On Sat, Sep 22, 2018 at 04:59:55PM -0700, Guenter Roeck wrote:
> >
> >>>>>+	/* Disable channels if their inputs are disconnected */
> >>>>>+	for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
> >>>>>+		if (ina->inputs[i].disconnected)
> >>>>>+			mask |= INA3221_CONFIG_CHx_EN(i);
> >>>>>+	}
> >>>>
> >>>>Consequently, you should also _enable_ channels which are not explicitly disabled.
> >>>
> >>>The register has enabled all channels by default. So I felt it'd
> >>>be neat to have disabling code only. My v1 actually had enabling
> >>>part as well, but I can add it back if you think it'd be better.
> >>>
> >>>>This can be tricky since you'll have to distinguish non-DT and DT configuration
> >>>>and retain the original configuration if no channel configuration data is available
> >>>>from devicetree.
> >
> >>>For non-DT configurations, input->disconnected is always false by
> >>>default unless someone adds config for it (through platform_data).
> >>>If regmap_update_bits only does disabling like this version does,
> >>>non-DT configurations will not get affected since mask = 0. Or if
> >>>we change it to do both enabling and disabling, regmap_update_bits
> >>>will still ignore since there's no register value changed, though
> >>>it won't really hurt even if regmap writes correct configurations
> >>>to the register.
> >>>
> >>>For DT configurations (without channel input source defined), it's
> >>>like the same as non-DT configurations. As we have platforms only
> >>>enabled ina3221 via DT while they don't have this new DT binding,
> >>>the driver has to be backward compatible, so my change only sets
> >>>input->disconnected=true when a status="disabled" is present, i.e.
> >>>those platforms are treated as all channels getting enabled until
> >>>they update their DTs.
> >
> >>I think your assumption may be that the chip is always in its reset state
> >>when Linux is loaded. This is not necessarily the case; it may be
> >>preconfigured by BIOS or ROMMON, or even by someone using i2cset before
> >>loading the driver. If you add enable/disable functionality, you can
> >>not make an assumption about the original state of the chip at probe time;
> >>you have to read it from the chip itself.
> >
> >I see. That made a point. In that case, I think the simplest way
> >is probably to do software reset before having configurations.
> >
> No. If the chip was configured by the BIOS/ROMMON, it is supposed
> to be that way. We can not just override that.

For this driver, it does soft reset in the probe() so we're
sure that all channels are enabled at the moment of calling
this regmap_update_bits. So there's no assumption anymore.

But the case that you mentioned is a good one. It does give
me some insight about the use case and the things that will
need to be careful when adding in[123]_enable. Just it'd be
also possible that BIOS could disable a channel that is not
explicitly disabled in the DT -- then the driver should not
enable it.

Therefore, for both cases, it seems that disabling only the
disconnected channels as this version is a safe solution.

And the driver actually won't update input->disconnected as
this is a physical hardware status that won't be changed. I
probably could define the input->disconnected in const type.
For in[123]_enable, it'd be safer to read/write the register
directly.

Thanks
Nicolin

> >The "disconnected" here is to describe the physical connection,
> >not exact the switch control status because a channel could be
> >connected but disabled. However, a channel would not be enabled
> >if it's disconnected. So I think it'd be safe to just disable
> >the disconnected channels here as this version does, meanwhile,
> >I will add a soft reset to make sure all channels are enabled.
> >
> >Thanks
> >Nicolin
> >
> 

Re: [PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Nicolin Chen]

On Sat, Sep 22, 2018 at 08:33:00PM -0700, Nicolin Chen wrote:
> On Sat, Sep 22, 2018 at 07:07:02PM -0700, Guenter Roeck wrote:
> > On 09/22/2018 05:38 PM, Nicolin Chen wrote:
> > >On Sat, Sep 22, 2018 at 04:59:55PM -0700, Guenter Roeck wrote:
> > >
> > >>>>>+	/* Disable channels if their inputs are disconnected */
> > >>>>>+	for (i = 0, mask = 0; i < INA3221_NUM_CHANNELS; i++) {
> > >>>>>+		if (ina->inputs[i].disconnected)
> > >>>>>+			mask |= INA3221_CONFIG_CHx_EN(i);
> > >>>>>+	}
> > >>>>
> > >>>>Consequently, you should also _enable_ channels which are not explicitly disabled.
> > >>>
> > >>>The register has enabled all channels by default. So I felt it'd
> > >>>be neat to have disabling code only. My v1 actually had enabling
> > >>>part as well, but I can add it back if you think it'd be better.
> > >>>
> > >>>>This can be tricky since you'll have to distinguish non-DT and DT configuration
> > >>>>and retain the original configuration if no channel configuration data is available
> > >>>>from devicetree.
> > >
> > >>>For non-DT configurations, input->disconnected is always false by
> > >>>default unless someone adds config for it (through platform_data).
> > >>>If regmap_update_bits only does disabling like this version does,
> > >>>non-DT configurations will not get affected since mask = 0. Or if
> > >>>we change it to do both enabling and disabling, regmap_update_bits
> > >>>will still ignore since there's no register value changed, though
> > >>>it won't really hurt even if regmap writes correct configurations
> > >>>to the register.
> > >>>
> > >>>For DT configurations (without channel input source defined), it's
> > >>>like the same as non-DT configurations. As we have platforms only
> > >>>enabled ina3221 via DT while they don't have this new DT binding,
> > >>>the driver has to be backward compatible, so my change only sets
> > >>>input->disconnected=true when a status="disabled" is present, i.e.
> > >>>those platforms are treated as all channels getting enabled until
> > >>>they update their DTs.
> > >
> > >>I think your assumption may be that the chip is always in its reset state
> > >>when Linux is loaded. This is not necessarily the case; it may be
> > >>preconfigured by BIOS or ROMMON, or even by someone using i2cset before
> > >>loading the driver. If you add enable/disable functionality, you can
> > >>not make an assumption about the original state of the chip at probe time;
> > >>you have to read it from the chip itself.
> > >
> > >I see. That made a point. In that case, I think the simplest way
> > >is probably to do software reset before having configurations.
> > >
> > No. If the chip was configured by the BIOS/ROMMON, it is supposed
> > to be that way. We can not just override that.
> 
> For this driver, it does soft reset in the probe() so we're
> sure that all channels are enabled at the moment of calling
> this regmap_update_bits. So there's no assumption anymore.
> 
> But the case that you mentioned is a good one. It does give
> me some insight about the use case and the things that will
> need to be careful when adding in[123]_enable. Just it'd be
> also possible that BIOS could disable a channel that is not
> explicitly disabled in the DT -- then the driver should not
> enable it.
> 
> Therefore, for both cases, it seems that disabling only the
> disconnected channels as this version is a safe solution.
> 
> And the driver actually won't update input->disconnected as
> this is a physical hardware status that won't be changed. I

[...]
> probably could define the input->disconnected in const type.
[...]

Please ignore this line. It'd be read-only then. I got myself
confused as it's not a pointer like label.

> For in[123]_enable, it'd be safer to read/write the register
> directly.
> 
> Thanks
> Nicolin
> 
> > >The "disconnected" here is to describe the physical connection,
> > >not exact the switch control status because a channel could be
> > >connected but disabled. However, a channel would not be enabled
> > >if it's disconnected. So I think it'd be safe to just disable
> > >the disconnected channels here as this version does, meanwhile,
> > >I will add a soft reset to make sure all channels are enabled.
> > >
> > >Thanks
> > >Nicolin
> > >
> > 

Re: [PATCH v3 2/2] hwmon: ina3221: Read channel input source info from DT

[Guenter Roeck]

On 09/22/2018 08:33 PM, Nicolin Chen wrote:

>> No. If the chip was configured by the BIOS/ROMMON, it is supposed
>> to be that way. We can not just override that.
> 
> For this driver, it does soft reset in the probe() so we're
> sure that all channels are enabled at the moment of calling
> this regmap_update_bits. So there's no assumption anymore.
> 

Good point.

> But the case that you mentioned is a good one. It does give
> me some insight about the use case and the things that will
> need to be careful when adding in[123]_enable. Just it'd be
> also possible that BIOS could disable a channel that is not
> explicitly disabled in the DT -- then the driver should not
> enable it.
> 

Not necessarily. Again, we can not assume that everyone has DT
(or that it and/or the BIOS is correct, for that matter).

Nevertheless, adding the enable attribute is not required at this
point, so it is ok for me to skip it. Please note that I'll accept
a patch adding it, though, if it is ever submitted (with a use case).

Thanks,
Guenter