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

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

[Nicolin Chen]

This series adds a initial DT binding doc for ina3221. It adds
channel names (which should be according to board schematics)
and implements the corresponding actions (disabling channels)
in the driver.

Nicolin Chen (2):
  dt-bindings: hwmon: Add ina3221 documentation
  hwmon: ina3221: Get channel names from DT node

 .../devicetree/bindings/hwmon/ina3221.txt     | 23 +++++
 drivers/hwmon/ina3221.c                       | 88 +++++++++++++++++--
 2 files changed, 103 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt

-- 
2.17.1

[PATCH 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>
---
 .../devicetree/bindings/hwmon/ina3221.txt     | 23 +++++++++++++++++++
 1 file changed, 23 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..266c9586c9b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt
@@ -0,0 +1,23 @@
+ina3221 properties
+
+Required properties:
+- compatible: Must be "ti,ina3221"
+- reg: I2C address
+
+Optional properties:
+
+- ti,channel1-name:
+- ti,channel2-name:
+- ti,channel3-name:
+	The names of the input sources (described in the schematics)
+	Set the names with "NC" to indicate not-connected channels
+
+Example:
+
+ina3221@40 {
+	compatible = "ti,ina3221";
+	reg = <0x40>;
+	ti,channel1-name = "NC";
+	ti,channel2-name = "VDD_5V0_EXT";
+	ti,channel3-name = "VDD_19V";
+};
-- 
2.17.1

[PATCH 2/2] hwmon: ina3221: Get channel names from DT node

[Nicolin Chen]

The connection of channels are usually descirbed in the
schematics, which then should be indicated in DT binding
as well, and further should get exposed to sysfs so as
to help driver users understand what channels are really
monitoring respectively.

Meanwhile, channels could be left unconnected based on
the hardware design. So the channel name should support
NC so the driver could disable the channel accordingly.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 drivers/hwmon/ina3221.c | 88 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index e6b49500c52a..5d487e205260 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -41,9 +41,12 @@
 #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
 
+#define INA3221_NOT_CONNECTED		"NC"
+
 enum ina3221_fields {
 	/* Configuration */
 	F_RST,
@@ -91,11 +94,20 @@ static const unsigned int register_channel[] = {
  * @regmap: Register map of the device
  * @fields: Register fields of the device
  * @shunt_resistors: Array of resistor values per channel
+ * @attr_group: attribute groups for sysfs node
+ *              (leave one space at the end for NULL termination)
+ * @channel_name: channel names
+ * @enable: enable or disable channels
  */
 struct ina3221_data {
 	struct regmap *regmap;
 	struct regmap_field *fields[F_MAX_FIELDS];
 	int shunt_resistors[INA3221_NUM_CHANNELS];
+
+	const struct attribute_group *attr_group[INA3221_NUM_CHANNELS + 1];
+	const char *channel_name[INA3221_NUM_CHANNELS];
+
+	bool enable[INA3221_NUM_CHANNELS];
 };
 
 static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
@@ -253,6 +265,24 @@ static ssize_t ina3221_show_alert(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", regval);
 }
 
+static ssize_t ina3221_show_name(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, "%s\n", ina->channel_name[channel]);
+}
+
+/* channel name */
+static SENSOR_DEVICE_ATTR(name1_input, 0444,
+		ina3221_show_name, NULL, INA3221_CHANNEL1);
+static SENSOR_DEVICE_ATTR(name2_input, 0444,
+		ina3221_show_name, NULL, INA3221_CHANNEL2);
+static SENSOR_DEVICE_ATTR(name3_input, 0444,
+		ina3221_show_name, NULL, INA3221_CHANNEL3);
+
 /* bus voltage */
 static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO,
 		ina3221_show_bus_voltage, NULL, INA3221_BUS1);
@@ -317,8 +347,8 @@ static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO,
 static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
 		ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3);
 
-static struct attribute *ina3221_attrs[] = {
-	/* channel 1 */
+static struct attribute *ina3221_channel1_attrs[] = {
+	&sensor_dev_attr_name1_input.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 +358,11 @@ static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in4_input.dev_attr.attr,
 
-	/* channel 2 */
+	NULL,
+};
+
+static struct attribute *ina3221_channel2_attrs[] = {
+	&sensor_dev_attr_name2_input.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 +372,11 @@ static struct attribute *ina3221_attrs[] = {
 	&sensor_dev_attr_curr2_max_alarm.dev_attr.attr,
 	&sensor_dev_attr_in5_input.dev_attr.attr,
 
-	/* channel 3 */
+	NULL,
+};
+
+static struct attribute *ina3221_channel3_attrs[] = {
+	&sensor_dev_attr_name3_input.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 +388,12 @@ static struct attribute *ina3221_attrs[] = {
 
 	NULL,
 };
-ATTRIBUTE_GROUPS(ina3221);
+
+static const struct attribute_group ina3221_group[INA3221_NUM_CHANNELS] = {
+	{ .attrs = ina3221_channel1_attrs, },
+	{ .attrs = ina3221_channel2_attrs, },
+	{ .attrs = ina3221_channel3_attrs, },
+};
 
 static const struct regmap_range ina3221_yes_ranges[] = {
 	regmap_reg_range(INA3221_SHUNT1, INA3221_BUS3),
@@ -373,10 +416,14 @@ static const struct regmap_config ina3221_regmap_config = {
 static int ina3221_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
+	const struct device_node *np = client->dev.of_node;
 	struct device *dev = &client->dev;
 	struct ina3221_data *ina;
 	struct device *hwmon_dev;
-	int i, ret;
+	u16 mask = 0, val = 0;
+	const char *str;
+	char prop[32];
+	int i, g, ret;
 
 	ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL);
 	if (!ina)
@@ -407,9 +454,34 @@ static int ina3221_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	/* Fetch hardware information from Device Tree */
+	for (i = 0, g = 0; i < INA3221_NUM_CHANNELS; i++) {
+		/* Fetch the channel name */
+		sprintf(prop, "ti,channel%d-name", i + 1);
+		/* Set a default name on failure */
+		if (of_property_read_string(np, prop, &str))
+			str = "unknown";
+		/* Ignore unconnected channels */
+		if (!strcmp(str, INA3221_NOT_CONNECTED))
+			continue;
+		/* Log connected channels */
+		ina->attr_group[g++] = &ina3221_group[i];
+		ina->channel_name[i] = str;
+		ina->enable[i] = true;
+	}
+
+	/* Disable unconnected channels */
+	for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
+		mask |= INA3221_CONFIG_CHx_EN(i);
+		val |= ina->enable[i] ? INA3221_CONFIG_CHx_EN(i) : 0;
+	}
+	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, val);
+	if (ret)
+		return ret;
+
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
-							   client->name,
-							   ina, ina3221_groups);
+							   client->name, ina,
+							   ina->attr_group);
 	if (IS_ERR(hwmon_dev)) {
 		dev_err(dev, "Unable to register hwmon device\n");
 		return PTR_ERR(hwmon_dev);
-- 
2.17.1

Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node

[Guenter Roeck]

On 09/20/2018 05:07 PM, Nicolin Chen wrote:
> The connection of channels are usually descirbed in the
> schematics, which then should be indicated in DT binding
> as well, and further should get exposed to sysfs so as
> to help driver users understand what channels are really
> monitoring respectively.
> 
> Meanwhile, channels could be left unconnected based on
> the hardware design. So the channel name should support
> NC so the driver could disable the channel accordingly.
> 

I am not in favor of such indirect settings. If a channel is
to be disconnected, define a property for it.

I am personally also not in favor of using devicetree to define
channel names like this, much less so for a single driver.

> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>   drivers/hwmon/ina3221.c | 88 +++++++++++++++++++++++++++++++++++++----
>   1 file changed, 80 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index e6b49500c52a..5d487e205260 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -41,9 +41,12 @@
>   #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
>   
> +#define INA3221_NOT_CONNECTED		"NC"
> +
>   enum ina3221_fields {
>   	/* Configuration */
>   	F_RST,
> @@ -91,11 +94,20 @@ static const unsigned int register_channel[] = {
>    * @regmap: Register map of the device
>    * @fields: Register fields of the device
>    * @shunt_resistors: Array of resistor values per channel
> + * @attr_group: attribute groups for sysfs node
> + *              (leave one space at the end for NULL termination)
> + * @channel_name: channel names
> + * @enable: enable or disable channels
>    */
>   struct ina3221_data {
>   	struct regmap *regmap;
>   	struct regmap_field *fields[F_MAX_FIELDS];
>   	int shunt_resistors[INA3221_NUM_CHANNELS];
> +
> +	const struct attribute_group *attr_group[INA3221_NUM_CHANNELS + 1];
> +	const char *channel_name[INA3221_NUM_CHANNELS];
> +
> +	bool enable[INA3221_NUM_CHANNELS];
>   };
>   
>   static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
> @@ -253,6 +265,24 @@ static ssize_t ina3221_show_alert(struct device *dev,
>   	return snprintf(buf, PAGE_SIZE, "%d\n", regval);
>   }
>   
> +static ssize_t ina3221_show_name(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, "%s\n", ina->channel_name[channel]);
> +}
> +
> +/* channel name */
> +static SENSOR_DEVICE_ATTR(name1_input, 0444,
> +		ina3221_show_name, NULL, INA3221_CHANNEL1);
> +static SENSOR_DEVICE_ATTR(name2_input, 0444,
> +		ina3221_show_name, NULL, INA3221_CHANNEL2);
> +static SENSOR_DEVICE_ATTR(name3_input, 0444,
> +		ina3221_show_name, NULL, INA3221_CHANNEL3);
> +
>   /* bus voltage */
>   static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO,
>   		ina3221_show_bus_voltage, NULL, INA3221_BUS1);
> @@ -317,8 +347,8 @@ static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO,
>   static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO,
>   		ina3221_show_shunt_voltage, NULL, INA3221_SHUNT3);
>   
> -static struct attribute *ina3221_attrs[] = {
> -	/* channel 1 */
> +static struct attribute *ina3221_channel1_attrs[] = {
> +	&sensor_dev_attr_name1_input.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 +358,11 @@ static struct attribute *ina3221_attrs[] = {
>   	&sensor_dev_attr_curr1_max_alarm.dev_attr.attr,
>   	&sensor_dev_attr_in4_input.dev_attr.attr,
>   
> -	/* channel 2 */
> +	NULL,
> +};
> +
> +static struct attribute *ina3221_channel2_attrs[] = {
> +	&sensor_dev_attr_name2_input.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 +372,11 @@ static struct attribute *ina3221_attrs[] = {
>   	&sensor_dev_attr_curr2_max_alarm.dev_attr.attr,
>   	&sensor_dev_attr_in5_input.dev_attr.attr,
>   
> -	/* channel 3 */
> +	NULL,
> +};
> +
> +static struct attribute *ina3221_channel3_attrs[] = {
> +	&sensor_dev_attr_name3_input.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 +388,12 @@ static struct attribute *ina3221_attrs[] = {
>   
>   	NULL,
>   };
> -ATTRIBUTE_GROUPS(ina3221);
> +
> +static const struct attribute_group ina3221_group[INA3221_NUM_CHANNELS] = {
> +	{ .attrs = ina3221_channel1_attrs, },
> +	{ .attrs = ina3221_channel2_attrs, },
> +	{ .attrs = ina3221_channel3_attrs, },
> +};
>   
>   static const struct regmap_range ina3221_yes_ranges[] = {
>   	regmap_reg_range(INA3221_SHUNT1, INA3221_BUS3),
> @@ -373,10 +416,14 @@ static const struct regmap_config ina3221_regmap_config = {
>   static int ina3221_probe(struct i2c_client *client,
>   			 const struct i2c_device_id *id)
>   {
> +	const struct device_node *np = client->dev.of_node;
>   	struct device *dev = &client->dev;
>   	struct ina3221_data *ina;
>   	struct device *hwmon_dev;
> -	int i, ret;
> +	u16 mask = 0, val = 0;
> +	const char *str;
> +	char prop[32];
> +	int i, g, ret;
>   
>   	ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL);
>   	if (!ina)
> @@ -407,9 +454,34 @@ static int ina3221_probe(struct i2c_client *client,
>   		return ret;
>   	}
>   
> +	/* Fetch hardware information from Device Tree */
> +	for (i = 0, g = 0; i < INA3221_NUM_CHANNELS; i++) {
> +		/* Fetch the channel name */
> +		sprintf(prop, "ti,channel%d-name", i + 1);
> +		/* Set a default name on failure */
> +		if (of_property_read_string(np, prop, &str))
> +			str = "unknown";

So if there are no devicetree entries, the user now gets a sequence of
"unknown" sensors ? I don't think so. Please keep in mind that there are
users of this chip who don't have devicetree systems, and other users
may not want to specify any specific name properties (or they use sensors3.conf
to do it).

> +		/* Ignore unconnected channels */
> +		if (!strcmp(str, INA3221_NOT_CONNECTED))
> +			continue;

Sorry, I won't accept this. I am sure we can come up with some useful means
to define in devicetree if individual channels of a hardware monitoring chip
are enabled or not, but a channel name of "NC" won't be it.

> +		/* Log connected channels */
> +		ina->attr_group[g++] = &ina3221_group[i];
> +		ina->channel_name[i] = str;
> +		ina->enable[i] = true;

I also don't see the need for defining the group dynamically. The
is_visible callback is very well suited for handling optional sysfs
attributes.

> +	}
> +
> +	/* Disable unconnected channels */
> +	for (i = 0; i < INA3221_NUM_CHANNELS; i++) {
> +		mask |= INA3221_CONFIG_CHx_EN(i);
> +		val |= ina->enable[i] ? INA3221_CONFIG_CHx_EN(i) : 0;
> +	}
> +	ret = regmap_update_bits(ina->regmap, INA3221_CONFIG, mask, val);
> +	if (ret)
> +		return ret;
> +
>   	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> -							   client->name,
> -							   ina, ina3221_groups);
> +							   client->name, ina,
> +							   ina->attr_group);
>   	if (IS_ERR(hwmon_dev)) {
>   		dev_err(dev, "Unable to register hwmon device\n");
>   		return PTR_ERR(hwmon_dev);
> 

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

[Guenter Roeck]

On 09/20/2018 05:07 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>
> ---
>   .../devicetree/bindings/hwmon/ina3221.txt     | 23 +++++++++++++++++++
>   1 file changed, 23 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..266c9586c9b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt
> @@ -0,0 +1,23 @@
> +ina3221 properties
> +
> +Required properties:
> +- compatible: Must be "ti,ina3221"
> +- reg: I2C address
> +
> +Optional properties:
> +
> +- ti,channel1-name:
> +- ti,channel2-name:
> +- ti,channel3-name:
> +	The names of the input sources (described in the schematics)
> +	Set the names with "NC" to indicate not-connected channels
> +

I don't really think this is a good idea - first to specify sensor
names this way, and much less specifying that "NC" means that a sensor
shall be disconnected/disabled.

Also, if we define devicetree support for this chip, it should include
all configuration options required to configure it. This should at
the very least include shunt resistor values.

Thanks,
Guenter

> +Example:
> +
> +ina3221@40 {
> +	compatible = "ti,ina3221";
> +	reg = <0x40>;
> +	ti,channel1-name = "NC";
> +	ti,channel2-name = "VDD_5V0_EXT";
> +	ti,channel3-name = "VDD_19V";
> +};
> 

Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node

[Nicolin Chen]

Hi Guenter,

Thanks for the review.

On Thu, Sep 20, 2018 at 05:41:48PM -0700, Guenter Roeck wrote:

> > Meanwhile, channels could be left unconnected based on
> > the hardware design. So the channel name should support
> > NC so the driver could disable the channel accordingly.
> > 
> 
> I am not in favor of such indirect settings. If a channel is
> to be disconnected, define a property for it.

OK. I can add a bool property for it instead.

> I am personally also not in favor of using devicetree to define
> channel names like this, much less so for a single driver.

As DT is used to describe hardware, I felt plausible to put
them in since the names are mentioned in the schematics. Do
you have any advise to handle this better?

> > +	/* Fetch hardware information from Device Tree */
> > +	for (i = 0, g = 0; i < INA3221_NUM_CHANNELS; i++) {
> > +		/* Fetch the channel name */
> > +		sprintf(prop, "ti,channel%d-name", i + 1);
> > +		/* Set a default name on failure */
> > +		if (of_property_read_string(np, prop, &str))
> > +			str = "unknown";
> 
> So if there are no devicetree entries, the user now gets a sequence of
> "unknown" sensors ? I don't think so. Please keep in mind that there are
> users of this chip who don't have devicetree systems, and other users
> may not want to specify any specific name properties (or they use sensors3.conf
> to do it).

Being enlightened by your comments below, maybe adding a
separate group for channel names by attaching is_visible
to it could be acceptable? Then, name nodes can hide from
those who don't want to specify.

> > +		/* Ignore unconnected channels */
> > +		if (!strcmp(str, INA3221_NOT_CONNECTED))
> > +			continue;
> 
> Sorry, I won't accept this. I am sure we can come up with some useful means
> to define in devicetree if individual channels of a hardware monitoring chip
> are enabled or not, but a channel name of "NC" won't be it.

I will try the bool property as you mentioned earlier.

> > +		/* Log connected channels */
> > +		ina->attr_group[g++] = &ina3221_group[i];
> > +		ina->channel_name[i] = str;
> > +		ina->enable[i] = true;
> 
> I also don't see the need for defining the group dynamically. The
> is_visible callback is very well suited for handling optional sysfs
> attributes.

I will add an is_visible callback.

Thanks
Nicolin

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

[Nicolin Chen]

On Thu, Sep 20, 2018 at 05:45:07PM -0700, Guenter Roeck wrote:
> On 09/20/2018 05:07 PM, Nicolin Chen wrote:
> > +- ti,channel1-name:
> > +- ti,channel2-name:
> > +- ti,channel3-name:
> > +	The names of the input sources (described in the schematics)
> > +	Set the names with "NC" to indicate not-connected channels
> > +
> 
> I don't really think this is a good idea - first to specify sensor
> names this way, and much less specifying that "NC" means that a sensor
> shall be disconnected/disabled.

I will replace the NC with a bool property for disconnection.

> Also, if we define devicetree support for this chip, it should include
> all configuration options required to configure it. This should at
> the very least include shunt resistor values.

I plan to add them too but am doing it incrementally. I can
try to include them in the next version.

Thanks
Nicolin

Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node

[Nicolin Chen]

Hi,

On Thu, Sep 20, 2018 at 06:20:38PM -0700, Nicolin Chen wrote:
> > So if there are no devicetree entries, the user now gets a sequence of
> > "unknown" sensors ? I don't think so. Please keep in mind that there are
> > users of this chip who don't have devicetree systems, and other users
> > may not want to specify any specific name properties (or they use sensors3.conf
> > to do it).
> 
> Being enlightened by your comments below, maybe adding a
> separate group for channel names by attaching is_visible
> to it could be acceptable? Then, name nodes can hide from
> those who don't want to specify.
 
> > > +		/* Log connected channels */
> > > +		ina->attr_group[g++] = &ina3221_group[i];
> > > +		ina->channel_name[i] = str;
> > > +		ina->enable[i] = true;
> > 
> > I also don't see the need for defining the group dynamically. The
> > is_visible callback is very well suited for handling optional sysfs
> > attributes.

I tried is_visible but it looks like it won't be that neat to
implement as some attributes of this driver don't really pass
the channel index to the store()/show() but some other indexes.

If you are very against the dynamical group, I can drop it to
leave the sysfs node as it was.

And for the name nodes that I was talking about above, I will
add an sysfs store() function so non-DT users can set them,
and I also removed the confusing "unknown" default name.

I have been rewriting the DT binding and it should make a lot
of sense now comparing to this version. Will send v2 tomrrow.

Thanks
Nicolin

Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node

[Guenter Roeck]

On 09/21/2018 02:18 AM, Nicolin Chen wrote:
> Hi,
> 
> On Thu, Sep 20, 2018 at 06:20:38PM -0700, Nicolin Chen wrote:
>>> So if there are no devicetree entries, the user now gets a sequence of
>>> "unknown" sensors ? I don't think so. Please keep in mind that there are
>>> users of this chip who don't have devicetree systems, and other users
>>> may not want to specify any specific name properties (or they use sensors3.conf
>>> to do it).
>>
>> Being enlightened by your comments below, maybe adding a
>> separate group for channel names by attaching is_visible
>> to it could be acceptable? Then, name nodes can hide from
>> those who don't want to specify.
>   
>>>> +		/* Log connected channels */
>>>> +		ina->attr_group[g++] = &ina3221_group[i];
>>>> +		ina->channel_name[i] = str;
>>>> +		ina->enable[i] = true;
>>>
>>> I also don't see the need for defining the group dynamically. The
>>> is_visible callback is very well suited for handling optional sysfs
>>> attributes.
> 
> I tried is_visible but it looks like it won't be that neat to
> implement as some attributes of this driver don't really pass
> the channel index to the store()/show() but some other indexes.
>

The channel index is not the only means that can be used to determine
the channel index. Many drivers use the position in the attrs[] array
to determine the channel index. I don't see why this would not be
possible here.

> If you are very against the dynamical group, I can drop it to
> leave the sysfs node as it was.
> 
> And for the name nodes that I was talking about above, I will
> add an sysfs store() function so non-DT users can set them,
> and I also removed the confusing "unknown" default name.
> 

The label attributes are RO. Please follow the ABI.

temp[1-*]_label Suggested temperature channel label.
                 Text string
                 Should only be created if the driver has hints about what
                 this temperature channel is being used for, and user-space
                 doesn't. In all other cases, the label is provided by
                 user-space.
                 RO

Guenter

Re: [PATCH 2/2] hwmon: ina3221: Get channel names from DT node

[Nicolin Chen]

On Fri, Sep 21, 2018 at 05:56:00AM -0700, Guenter Roeck wrote:

> > I tried is_visible but it looks like it won't be that neat to
> > implement as some attributes of this driver don't really pass
> > the channel index to the store()/show() but some other indexes.
> > 
> 
> The channel index is not the only means that can be used to determine
> the channel index. Many drivers use the position in the attrs[] array
> to determine the channel index. I don't see why this would not be
> possible here.

Hmmm, that should simply work...I didn't mean not possible though.

> > If you are very against the dynamical group, I can drop it to
> > leave the sysfs node as it was.
> > 
> > And for the name nodes that I was talking about above, I will
> > add an sysfs store() function so non-DT users can set them,
> > and I also removed the confusing "unknown" default name.
> > 
> 
> The label attributes are RO. Please follow the ABI.
> 
> temp[1-*]_label Suggested temperature channel label.

Thanks a lot for the hint. I looked up the doc and feel that
this one probably fits my situation more:
    in[0-*]_label   Suggested voltage channel label.

Will follow it in my next version.

Thank you
Nicolin