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

[PATCH v5 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
v4->v5:
 * Changed two property names of child node (PATCH-1)
 * Changed the driver accordingly and simplified is_visible (PATCH-2)
v3->v4:
 * Fixed one place in child DT node bindings (PATCH-1)
 * Changed the driver accordingly and added is_visible (PATCH-2)
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     |  49 ++++++
 Documentation/hwmon/ina3221                   |   1 +
 drivers/hwmon/ina3221.c                       | 161 ++++++++++++++++--
 3 files changed, 197 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt

-- 
2.17.1

[PATCH v5 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
v4->v5:
 * Replaced "input-id" with "reg" and added address-cells and size-cells
 * Replaced "input-label" with "label"
 * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms"
v3->v4:
 * Removed the attempt of putting labels in the node names
 * Added a new optional label property in the child node
 * Updated examples accordingly
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     | 49 +++++++++++++++++++
 1 file changed, 49 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..e17a897f4803
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt
@@ -0,0 +1,49 @@
+Texas Instruments INA3221 Device Tree Bindings
+
+1) ina3221 node
+  Required properties:
+  - compatible: Must be "ti,ina3221"
+  - reg: I2C address
+
+  Optional properties:
+  = The node contains optional child nodes for three channels =
+  = Each child node describes the information of input source =
+
+  - #address-cells: Required only if a child node is present. Must be 1.
+  - #size-cells: Required only if a child node is present. Must be 0.
+
+  Example:
+
+  ina3221@40 {
+          compatible = "ti,ina3221";
+          reg = <0x40>;
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          [ child node definitions... ]
+  };
+
+2) child nodes
+  Required properties:
+  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
+
+  Optional properties:
+  - label: Name of the input source
+  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
+  - status: Should be "disabled" if no input source
+
+  Example:
+
+  input@0 {
+          reg = <0x0>;
+          status = "disabled";
+  };
+  input@1 {
+          reg = <0x1>;
+          shunt-resistor-micro-ohms = <5000>;
+  };
+  input@2 {
+          reg = <0x2>;
+          label = "VDD_5V";
+          shunt-resistor-micro-ohms = <5000>;
+  };
-- 
2.17.1

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

[Nicolin Chen]

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 if available and also disables those channels
where there's no input source being connected.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
Changelog
v4->v5:
 * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms"
 * Replaced "input-label" with "label"
 * Replaced "input-id" with "reg"
 * Simplified is_visible() by using index of the attr
 * Moved inX_label to index-0 and added comments for safety
v3->v4:
 * Added is_visible callback function to hide sysfs nodes
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     | 161 ++++++++++++++++++++++++++++++++----
 2 files changed, 148 insertions(+), 14 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..25fa8c05a3bf 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,18 +87,41 @@ 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 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 int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
 			      int *val)
 {
@@ -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;
-
-	resistance_uo = ina->shunt_resistors[channel];
+	struct ina3221_input *input = &ina->inputs[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;
 }
@@ -253,6 +278,14 @@ static ssize_t ina3221_show_alert(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", regval);
 }
 
+/* input 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);
+
 /* bus voltage */
 static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO,
 		ina3221_show_bus_voltage, NULL, INA3221_BUS1);
@@ -318,7 +351,8 @@ static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
 		ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3);
 
 static struct attribute *ina3221_attrs[] = {
-	/* channel 1 */
+	/* channel 1 -- make sure label at first */
+	&sensor_dev_attr_in1_label.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,
@@ -328,7 +362,8 @@ static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in4_input.dev_attr.attr,
 
-	/* channel 2 */
+	/* channel 2 -- make sure label at first */
+	&sensor_dev_attr_in2_label.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,
@@ -338,7 +373,8 @@ static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr2_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in5_input.dev_attr.attr,
 
-	/* channel 3 */
+	/* channel 3 -- make sure label at first */
+	&sensor_dev_attr_in3_label.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,
@@ -350,7 +386,34 @@ static struct attribute *ina3221_attrs[] = {
 
 	NULL,
 };
-ATTRIBUTE_GROUPS(ina3221);
+
+static umode_t ina3221_attr_is_visible(struct kobject *kobj,
+				       struct attribute *attr, int n)
+{
+	const int max_attrs = ARRAY_SIZE(ina3221_attrs) - 1;
+	const int num_attrs = max_attrs / INA3221_NUM_CHANNELS;
+	struct device *dev = kobj_to_dev(kobj);
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	enum ina3221_channels channel = n / num_attrs;
+	struct ina3221_input *input = &ina->inputs[channel];
+	int index = n % num_attrs;
+
+	/* Hide if channel input source is disconnected */
+	if (input->disconnected)
+		return 0;
+
+	/* Hide label node if label is not provided */
+	if (index == 0 && !input->label)
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group ina3221_group = {
+	.is_visible = ina3221_attr_is_visible,
+	.attrs = ina3221_attrs,
+};
+__ATTRIBUTE_GROUPS(ina3221);
 
 static const struct regmap_range ina3221_yes_ranges[] = {
 	regmap_reg_range(INA3221_SHUNT1, INA3221_BUS3),
@@ -370,6 +433,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, "reg", &val);
+	if (ret) {
+		dev_err(dev, "missing reg property of %s\n", child->name);
+		return ret;
+	} else if (val > INA3221_CHANNEL3) {
+		dev_err(dev, "invalid reg %d of %s\n", val, child->name);
+		return ret;
+	}
+
+	input = &ina->inputs[val];
+
+	/* Log the disconnected channel input */
+	if (!of_device_is_available(child)) {
+		input->disconnected = true;
+		return 0;
+	}
+
+	/* Save the connected input label if available */
+	of_property_read_string(child, "label", &input->label);
+
+	/* Overwrite default shunt resistor value optionally */
+	if (!of_property_read_u32(child, "shunt-resistor-micro-ohms", &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 +494,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 +517,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 +531,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 v5 1/2] dt-bindings: hwmon: Add ina3221 documentation

[Guenter Roeck]

Hi Nicolin,

On 09/25/2018 03:59 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
> v4->v5:
>   * Replaced "input-id" with "reg" and added address-cells and size-cells
>   * Replaced "input-label" with "label"
>   * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms"
> v3->v4:
>   * Removed the attempt of putting labels in the node names
>   * Added a new optional label property in the child node
>   * Updated examples accordingly
> 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     | 49 +++++++++++++++++++
>   1 file changed, 49 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..e17a897f4803
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt
> @@ -0,0 +1,49 @@
> +Texas Instruments INA3221 Device Tree Bindings
> +
> +1) ina3221 node
> +  Required properties:
> +  - compatible: Must be "ti,ina3221"
> +  - reg: I2C address
> +
> +  Optional properties:
> +  = The node contains optional child nodes for three channels =
> +  = Each child node describes the information of input source =
> +
> +  - #address-cells: Required only if a child node is present. Must be 1.
> +  - #size-cells: Required only if a child node is present. Must be 0.
> +
> +  Example:
> +
> +  ina3221@40 {
> +          compatible = "ti,ina3221";
> +          reg = <0x40>;
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          [ child node definitions... ]
> +  };
> +
> +2) child nodes
> +  Required properties:
> +  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
> +
> +  Optional properties:
> +  - label: Name of the input source
> +  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
> +  - status: Should be "disabled" if no input source
> +
> +  Example:
> +
> +  input@0 {
> +          reg = <0x0>;
> +          status = "disabled";

I kind of feel embarrassed that I asked for the reg change ... especially while
saying at the same time that I would like to see this work for other chips
as well.

Other chips have different kinds of sensors. Voltage, current, power, temperature,
and others. Whatever we come up with should support that.

I see two possibilities right now. We can stick with reg and add a "type" property,
or we can make the index something like
	{voltage,current,power,temperature,humidity}-{id,index}

I personally prefer "type", but I don't really have a strong opinion.
What do you think ? Or maybe we should really wait for feedback from Rob.

Thanks,
Guenter

> +  };
> +  input@1 {
> +          reg = <0x1>;
> +          shunt-resistor-micro-ohms = <5000>;
> +  };
> +  input@2 {
> +          reg = <0x2>;
> +          label = "VDD_5V";
> +          shunt-resistor-micro-ohms = <5000>;
> +  };
> 

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

[Nicolin Chen]

Hello Guenter,

On Tue, Sep 25, 2018 at 06:52:29PM -0700, Guenter Roeck wrote:

> > +2) child nodes
> > +  Required properties:
> > +  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
> > +
> > +  Optional properties:
> > +  - label: Name of the input source
> > +  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
> > +  - status: Should be "disabled" if no input source
> > +
> > +  Example:
> > +
> > +  input@0 {
> > +          reg = <0x0>;
> > +          status = "disabled";
> 
> I kind of feel embarrassed that I asked for the reg change ... especially while
> saying at the same time that I would like to see this work for other chips
> as well.

Well, though I didn't mention it, yet I changed it to "reg" is more
likely an agreement than a compromise: I searched in the mail list
and then found this mail (a year ago though):
    https://www.spinics.net/lists/kernel/msg2455439.html

I feel it very similar to my case. So rather than betting Rob won't
tell me the same, changing to "reg" may reduce a turnaround time :)

> Other chips have different kinds of sensors. Voltage, current, power, temperature,
> and others. Whatever we come up with should support that.
> 
> I see two possibilities right now. We can stick with reg and add a "type" property,
> or we can make the index something like
> 	{voltage,current,power,temperature,humidity}-{id,index}

One small concern is a case of being multi-type. For example, I saw
ina2xx driver having voltage, current and power at the same time...

> I personally prefer "type", but I don't really have a strong opinion.
> What do you think ?

I also like this over "reg" -- "reg" requires two extra properties,
and itself sounds slightly unnatural to me for situations like this
one where we don't use it as a register address, although I know it
is convenient and common to use.

> Or maybe we should really wait for feedback from Rob.

Personally I don't mind it all to change the doc and code and then
send a v6. But eventually we'll still need the final Acked-by from
Rob right? Then I guess it's the only option.

By the way, I have two ina3221 hwmon patches that rebase upon this
binding series. And I'd like to send them out to go through review
first, but I am not sure if you'd be okay for it -- I don't really
like to change their rebase order as it might mess up something.

Thanks
Nicolin

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

[Guenter Roeck]

On 09/25/2018 08:08 PM, Nicolin Chen wrote:
> Hello Guenter,
> 
> On Tue, Sep 25, 2018 at 06:52:29PM -0700, Guenter Roeck wrote:
> 
>>> +2) child nodes
>>> +  Required properties:
>>> +  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
>>> +
>>> +  Optional properties:
>>> +  - label: Name of the input source
>>> +  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
>>> +  - status: Should be "disabled" if no input source
>>> +
>>> +  Example:
>>> +
>>> +  input@0 {
>>> +          reg = <0x0>;
>>> +          status = "disabled";
>>
>> I kind of feel embarrassed that I asked for the reg change ... especially while
>> saying at the same time that I would like to see this work for other chips
>> as well.
> 
> Well, though I didn't mention it, yet I changed it to "reg" is more
> likely an agreement than a compromise: I searched in the mail list
> and then found this mail (a year ago though):
>      https://www.spinics.net/lists/kernel/msg2455439.html
> 
> I feel it very similar to my case. So rather than betting Rob won't
> tell me the same, changing to "reg" may reduce a turnaround time :)
> 
>> Other chips have different kinds of sensors. Voltage, current, power, temperature,
>> and others. Whatever we come up with should support that.
>>
>> I see two possibilities right now. We can stick with reg and add a "type" property,
>> or we can make the index something like
>> 	{voltage,current,power,temperature,humidity}-{id,index}
> 
> One small concern is a case of being multi-type. For example, I saw
> ina2xx driver having voltage, current and power at the same time...
> 
Yes, with that we would have something like

	voltage@0 {
		type = "voltage";
		reg = <0>;
	};
	current@0 {
		type = "current";
		reg = <0>;
	};
	...

or

	voltage@0 {
		voltage-id = <0>;
	};
	current@0 {
		current-id = <0>;
	};
	...

>> I personally prefer "type", but I don't really have a strong opinion.
>> What do you think ?
> 
> I also like this over "reg" -- "reg" requires two extra properties,
> and itself sounds slightly unnatural to me for situations like this
> one where we don't use it as a register address, although I know it
> is convenient and common to use.
> 

With "type", we would still need two properties.

	reg = <0>;
	type = "voltage";

and type could be optional or not required for a chip only supporting
a single sensor type (like the ina3221).

This would be equivalent to, say,
	voltage-index = <0>;
when using a single property.

With the "reg" approach, we would be ok for now - however, I would like
to get feedback from Rob if introducing a "type" property will be
acceptable when the time comes to do so.

>> Or maybe we should really wait for feedback from Rob.
> 
> Personally I don't mind it all to change the doc and code and then
> send a v6. But eventually we'll still need the final Acked-by from
> Rob right? Then I guess it's the only option.
> 

Yes. At this point I'd rather have input from Rob than moving forward
with v6.

> By the way, I have two ina3221 hwmon patches that rebase upon this
> binding series. And I'd like to send them out to go through review
> first, but I am not sure if you'd be okay for it -- I don't really
> like to change their rebase order as it might mess up something.
> 
Are those bug fixes or further enhancements ? For enhancements,
it is your call when to send them; I am fine either way. If they are
bug fixes, they should come first so we can apply them to -stable.

Thanks,
Guenter

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

[Nicolin Chen]

On Tue, Sep 25, 2018 at 08:40:59PM -0700, Guenter Roeck wrote:
> >>>+2) child nodes
> >>>+  Required properties:
> >>>+  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221

> >>>+  input@0 {
> >>>+          reg = <0x0>;
> >>>+          status = "disabled";

> >>saying at the same time that I would like to see this work for other chips
> >>as well.

> >>Other chips have different kinds of sensors. Voltage, current, power, temperature,
> >>and others. Whatever we come up with should support that.
> >>
> >>I see two possibilities right now. We can stick with reg and add a "type" property,
> >>or we can make the index something like
> >>	{voltage,current,power,temperature,humidity}-{id,index}
> >
> >One small concern is a case of being multi-type. For example, I saw
> >ina2xx driver having voltage, current and power at the same time...
> >
> Yes, with that we would have something like
> 
> 	voltage@0 {
> 		type = "voltage";
> 		reg = <0>;
> 	};
> 	current@0 {
> 		type = "current";
> 		reg = <0>;
> or
> 	voltage@0 {
> 		voltage-id = <0>;
> 	};
> 	current@0 {
> 		current-id = <0>;

I see the point now. So this could be a good binding for different
types of sensor input sources. Then the hwmon device driver side'd
also get a hint from DT, or potentially have further common helper
functions or structures being defined in the core driver.

Yet I feel that we could still have something more obvious to tell
which exact port that the input source links to. From my point of
view, neither "type-id" nor "type + reg" nor "reg" only perfectly
tells the port connection information. It somehow feels like that
we are assigning an ID or even an address to an input source; Yes,
we describe them in the doc, but by reading the device node itself
without knowing the famous "reg", it's a bit hard to tell. And not
to justify for my way, but both the "input-id" in the v2 and the
"pwm-port" in the aspeed patch sound relatively straightforward.

If we could make something similar to regulator bindings, probably
it would make more sense. I know the standalone voltage node might
be odd though, just trying to express my concern.
	source0: voltage0 {
		type = "voltage";
		lable = "VDD_5V";
		shunt-resistor-micro-ohm = <5000>;
		/* status = "disabled"; */
	};
	ina3221@40 {
		port1 = <&source0>;
		/* port2 = <&source1>; */
	};

> With "type", we would still need two properties.
> 
> 	reg = <0>;
> 	type = "voltage";
> 
> and type could be optional or not required for a chip only supporting
> a single sensor type (like the ina3221).
> 
> This would be equivalent to, say,
> 	voltage-index = <0>;
> when using a single property.
> 
> With the "reg" approach, we would be ok for now - however, I would like
> to get feedback from Rob if introducing a "type" property will be
> acceptable when the time comes to do so.

OK. Let's see how Rob replies. If he strongly feels "reg" is still
essential, at least this patch can be Acked first -- reduces turn
around time :)

> >By the way, I have two ina3221 hwmon patches that rebase upon this
> >binding series. And I'd like to send them out to go through review
> >first, but I am not sure if you'd be okay for it -- I don't really
> >like to change their rebase order as it might mess up something.
> >
> Are those bug fixes or further enhancements ? For enhancements,
> it is your call when to send them; I am fine either way. If they are
> bug fixes, they should come first so we can apply them to -stable.

Understood. They are adding new sysfs nodes, like in[123]_enable
as you suggested during the previous review. I'll send them soon.

Thanks
Nicolin

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

[Rob Herring]

On Tue, Sep 25, 2018 at 06:52:29PM -0700, Guenter Roeck wrote:
> Hi Nicolin,
> 
> On 09/25/2018 03:59 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
> > v4->v5:
> >   * Replaced "input-id" with "reg" and added address-cells and size-cells
> >   * Replaced "input-label" with "label"
> >   * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms"
> > v3->v4:
> >   * Removed the attempt of putting labels in the node names
> >   * Added a new optional label property in the child node
> >   * Updated examples accordingly
> > 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     | 49 +++++++++++++++++++
> >   1 file changed, 49 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..e17a897f4803
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt
> > @@ -0,0 +1,49 @@
> > +Texas Instruments INA3221 Device Tree Bindings
> > +
> > +1) ina3221 node
> > +  Required properties:
> > +  - compatible: Must be "ti,ina3221"
> > +  - reg: I2C address
> > +
> > +  Optional properties:
> > +  = The node contains optional child nodes for three channels =
> > +  = Each child node describes the information of input source =
> > +
> > +  - #address-cells: Required only if a child node is present. Must be 1.
> > +  - #size-cells: Required only if a child node is present. Must be 0.
> > +
> > +  Example:
> > +
> > +  ina3221@40 {
> > +          compatible = "ti,ina3221";
> > +          reg = <0x40>;
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +
> > +          [ child node definitions... ]
> > +  };
> > +
> > +2) child nodes
> > +  Required properties:
> > +  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
> > +
> > +  Optional properties:
> > +  - label: Name of the input source
> > +  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
> > +  - status: Should be "disabled" if no input source
> > +
> > +  Example:
> > +
> > +  input@0 {
> > +          reg = <0x0>;
> > +          status = "disabled";
> 
> I kind of feel embarrassed that I asked for the reg change ... especially while
> saying at the same time that I would like to see this work for other chips
> as well.
> 
> Other chips have different kinds of sensors. Voltage, current, power, temperature,
> and others. Whatever we come up with should support that.
> 
> I see two possibilities right now. We can stick with reg and add a "type" property,
> or we can make the index something like
> 	{voltage,current,power,temperature,humidity}-{id,index}
> 
> I personally prefer "type", but I don't really have a strong opinion.
> What do you think ? Or maybe we should really wait for feedback from Rob.

reg and 'type' is my preference though just 'type' may be too vague. For 
this case I don't think we need it if there's only one possible type as 
the driver should know that.

Rob

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

[Rob Herring]

On Tue, Sep 25, 2018 at 03:59:29PM -0700, 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
> v4->v5:
>  * Replaced "input-id" with "reg" and added address-cells and size-cells
>  * Replaced "input-label" with "label"
>  * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms"
> v3->v4:
>  * Removed the attempt of putting labels in the node names
>  * Added a new optional label property in the child node
>  * Updated examples accordingly
> 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     | 49 +++++++++++++++++++
>  1 file changed, 49 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..e17a897f4803
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt
> @@ -0,0 +1,49 @@
> +Texas Instruments INA3221 Device Tree Bindings
> +
> +1) ina3221 node
> +  Required properties:
> +  - compatible: Must be "ti,ina3221"
> +  - reg: I2C address
> +
> +  Optional properties:
> +  = The node contains optional child nodes for three channels =
> +  = Each child node describes the information of input source =
> +
> +  - #address-cells: Required only if a child node is present. Must be 1.
> +  - #size-cells: Required only if a child node is present. Must be 0.
> +
> +  Example:
> +
> +  ina3221@40 {
> +          compatible = "ti,ina3221";
> +          reg = <0x40>;
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          [ child node definitions... ]
> +  };
> +
> +2) child nodes
> +  Required properties:
> +  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
> +
> +  Optional properties:
> +  - label: Name of the input source
> +  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
> +  - status: Should be "disabled" if no input source

Don't need to explicitly list status.

> +
> +  Example:
> +
> +  input@0 {
> +          reg = <0x0>;
> +          status = "disabled";
> +  };
> +  input@1 {
> +          reg = <0x1>;
> +          shunt-resistor-micro-ohms = <5000>;
> +  };
> +  input@2 {
> +          reg = <0x2>;
> +          label = "VDD_5V";
> +          shunt-resistor-micro-ohms = <5000>;
> +  };

Please combine the examples into one complete example.

Rob

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

[Nicolin Chen]

On Thu, Sep 27, 2018 at 12:44:13PM -0500, Rob Herring wrote:
> > +2) child nodes
> > +  Required properties:
> > +  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
> > +
> > +  Optional properties:
> > +  - label: Name of the input source
> > +  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
> > +  - status: Should be "disabled" if no input source
> 
> Don't need to explicitly list status.

Will remove in v6.

> > +
> > +  Example:
> > +
> > +  input@0 {
> > +          reg = <0x0>;
> > +          status = "disabled";
> > +  };
> > +  input@1 {
> > +          reg = <0x1>;
> > +          shunt-resistor-micro-ohms = <5000>;
> > +  };
> > +  input@2 {
> > +          reg = <0x2>;
> > +          label = "VDD_5V";
> > +          shunt-resistor-micro-ohms = <5000>;
> > +  };
> 
> Please combine the examples into one complete example.

Will merge them in v6.

Thank you
Nicolin