[PATCH v3 00/11] Add per channel properies support in tmp421

[PATCH v3 00/11] Add per channel properies support in tmp421

[Krzysztof Adamski]

Hi,

This series adds support for defining per-channel properies (like
n-factor and label) to the TMP421 driver. It starts by adding the
missing DT binding for tmp421, in the form that was there before any of
my changes. Then I do the changes to the driver and finally adjust the
bindings to my changes.

The precedence for this case is:
[PATCH v9 2/2] hwmon: (ina3221) Read channel input source info from DT
Which can be found here:
https://lkml.org/lkml/2018/10/2/136

My patches does similar thing but to tmp422 - we need a way to define
the labels for specific channels as well as to define the n-factor (that
is board specific as it depends on the diodes used for remote sensing).
A possibility to disable unused channels seems like a good idea too.

Here comes v3. Changes compared to v2:
- fixed the $id path in DT
- moved the tmp421_enable_channels() call inside of tmp421_init_client()
- fixed some unneeded line brakes
- added "ignore non input related DT nodes" patch for skipping all
  subnodes that are not input@X without raising an error. I'm not
  completly convinced this is what we should do so I made it a separate
  patch so that you can easily skip it, if you decide to

Changes compared to v1:
- fixed sparse warnings about making function declarations static
- changed the policy for broken DT - in case of errors, the probe will
  return an error instead of continuing
- in addition to disabling the channels specified in DT, the probe
  function will also enable all the others in case they were disabled by
  some other code earlier
  NOTE: this may be a backwards incompatible change - if some channels
  were disabled by some bootloader code previously the channels would
  stay like that and now they would be enabled during probe, even if
  nothing is specified in DT. Is this what we want?
- added support for HWMON_T_ENABLE
- updated documentation
- NOTE: I haven't changed anything related to DT as the discussion has
  no clear conclusion yet.

Krzysztof Adamski (11):
  dt-bindings: hwmon: add missing tmp421 binding
  hwmon: (tmp421) introduce MAX_CHANNELS define
  hwmon: (tmp421) introduce a channel struct
  hwmon: (tmp421) add support for defining labels from DT
  hwmon: (tmp421) support disabling channels from DT
  hwmon: (tmp421) support specifying n-factor via DT
  hwmon: (tmp421) really disable channels
  hwmon: (tmp421) support HWMON_T_ENABLE
  hwmon: (tmp421) update documentation
  hwmon: (tmp421) ignore non input related DT nodes
  dt-bindings: hwmon: allow specifying channels for tmp421

 .../devicetree/bindings/hwmon/ti,tmp421.yaml  | 109 +++++++++++
 Documentation/hwmon/tmp421.rst                |  10 +
 drivers/hwmon/tmp421.c                        | 175 ++++++++++++++++--
 3 files changed, 281 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml

-- 
2.31.1

[PATCH v3 01/11] dt-bindings: hwmon: add missing tmp421 binding

[Krzysztof Adamski]

Add basic description of the tmp421 driver DT bindings.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 .../devicetree/bindings/hwmon/ti,tmp421.yaml  | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
new file mode 100644
index 000000000000..47040ace4f73
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ti,tmp421.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TMP42x/TMP44x temperature sensor
+
+maintainers:
+  - Guenter Roeck <linux@roeck-us.net>
+
+description: |
+  ±1°C Remote and Local temperature sensor
+  https://www.ti.com/lit/ds/symlink/tmp422.pdf
+
+properties:
+  compatible:
+    enum:
+      - ti,tmp421
+      - ti,tmp422
+      - ti,tmp423
+      - ti,tmp441
+      - ti,tmp442
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      sensor@4c {
+        compatible = "ti,tmp422";
+        reg = <0x4c>;
+      };
+    };
-- 
2.31.1

Re: [PATCH v3 01/11] dt-bindings: hwmon: add missing tmp421 binding

[Rob Herring]

On Thu, 30 Sep 2021 08:57:52 +0200, Krzysztof Adamski wrote:
> Add basic description of the tmp421 driver DT bindings.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
>  .../devicetree/bindings/hwmon/ti,tmp421.yaml  | 43 +++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v3 02/11] hwmon: (tmp421) introduce MAX_CHANNELS define

[Krzysztof Adamski]

There are few places where the maximal number of channels is used define
the size of arrays but when raw number is used it is not clear that they
really related to this quantity.
This commit introduces MAX_CHANNELS define and uses it those places to
give some context to the number.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/tmp421.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index b963a369c5ab..a4ac6e16d592 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -29,6 +29,7 @@ static const unsigned short normal_i2c[] = { 0x2a, 0x4c, 0x4d, 0x4e, 0x4f,
 
 enum chips { tmp421, tmp422, tmp423, tmp441, tmp442 };
 
+#define MAX_CHANNELS				4
 /* The TMP421 registers */
 #define TMP421_STATUS_REG			0x08
 #define TMP421_CONFIG_REG_1			0x09
@@ -36,8 +37,8 @@ enum chips { tmp421, tmp422, tmp423, tmp441, tmp442 };
 #define TMP421_MANUFACTURER_ID_REG		0xFE
 #define TMP421_DEVICE_ID_REG			0xFF
 
-static const u8 TMP421_TEMP_MSB[4]		= { 0x00, 0x01, 0x02, 0x03 };
-static const u8 TMP421_TEMP_LSB[4]		= { 0x10, 0x11, 0x12, 0x13 };
+static const u8 TMP421_TEMP_MSB[MAX_CHANNELS]	= { 0x00, 0x01, 0x02, 0x03 };
+static const u8 TMP421_TEMP_LSB[MAX_CHANNELS]	= { 0x10, 0x11, 0x12, 0x13 };
 
 /* Flags */
 #define TMP421_CONFIG_SHUTDOWN			0x40
@@ -89,7 +90,7 @@ MODULE_DEVICE_TABLE(of, tmp421_of_match);
 struct tmp421_data {
 	struct i2c_client *client;
 	struct mutex update_lock;
-	u32 temp_config[5];
+	u32 temp_config[MAX_CHANNELS + 1];
 	struct hwmon_channel_info temp_info;
 	const struct hwmon_channel_info *info[2];
 	struct hwmon_chip_info chip;
@@ -97,7 +98,7 @@ struct tmp421_data {
 	unsigned long last_updated;
 	unsigned long channels;
 	u8 config;
-	s16 temp[4];
+	s16 temp[MAX_CHANNELS];
 };
 
 static int temp_from_raw(u16 reg, bool extended)
-- 
2.31.1

[PATCH v3 03/11] hwmon: (tmp421) introduce a channel struct

[Krzysztof Adamski]

This is a preparatory change. Upcoming patches will introduce more
per-channel parameters so it's worth organizing them into a struct.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/tmp421.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index a4ac6e16d592..f3d59ea347e3 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -87,6 +87,10 @@ static const struct of_device_id __maybe_unused tmp421_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, tmp421_of_match);
 
+struct tmp421_channel {
+	s16 temp;
+};
+
 struct tmp421_data {
 	struct i2c_client *client;
 	struct mutex update_lock;
@@ -98,7 +102,7 @@ struct tmp421_data {
 	unsigned long last_updated;
 	unsigned long channels;
 	u8 config;
-	s16 temp[MAX_CHANNELS];
+	struct tmp421_channel channel[MAX_CHANNELS];
 };
 
 static int temp_from_raw(u16 reg, bool extended)
@@ -133,12 +137,12 @@ static int tmp421_update_device(struct tmp421_data *data)
 			ret = i2c_smbus_read_byte_data(client, TMP421_TEMP_MSB[i]);
 			if (ret < 0)
 				goto exit;
-			data->temp[i] = ret << 8;
+			data->channel[i].temp = ret << 8;
 
 			ret = i2c_smbus_read_byte_data(client, TMP421_TEMP_LSB[i]);
 			if (ret < 0)
 				goto exit;
-			data->temp[i] |= ret;
+			data->channel[i].temp |= ret;
 		}
 		data->last_updated = jiffies;
 		data->valid = 1;
@@ -167,7 +171,7 @@ static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
 
 	switch (attr) {
 	case hwmon_temp_input:
-		*val = temp_from_raw(tmp421->temp[channel],
+		*val = temp_from_raw(tmp421->channel[channel].temp,
 				     tmp421->config & TMP421_CONFIG_RANGE);
 		return 0;
 	case hwmon_temp_fault:
@@ -175,7 +179,7 @@ static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
 		 * Any of OPEN or /PVLD bits indicate a hardware mulfunction
 		 * and the conversion result may be incorrect
 		 */
-		*val = !!(tmp421->temp[channel] & 0x03);
+		*val = !!(tmp421->channel[channel].temp & 0x03);
 		return 0;
 	default:
 		return -EOPNOTSUPP;
-- 
2.31.1

[PATCH v3 04/11] hwmon: (tmp421) add support for defining labels from DT

[Krzysztof Adamski]

tmp42x is a multichannel temperature sensor with several external
channels. Since those channels can be used to connect diodes placed
anywhere in the system, their meaning will vary depending on the
project. For this case, the hwmon framework has an idea of labels which
allows us to assign the meaning to each channel.

The similar concept is already implemented in ina3221 - the label for
each channel can be defined via device tree. See commit a9e9dd9c6de5
("hwmon: (ina3221) Read channel input source info from DT")

This patch adds support for similar feature to tmp421.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/tmp421.c | 61 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index f3d59ea347e3..7feef60a592c 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -88,6 +88,7 @@ static const struct of_device_id __maybe_unused tmp421_of_match[] = {
 MODULE_DEVICE_TABLE(of, tmp421_of_match);
 
 struct tmp421_channel {
+	const char *label;
 	s16 temp;
 };
 
@@ -187,6 +188,16 @@ static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
 
 }
 
+static int tmp421_read_string(struct device *dev, enum hwmon_sensor_types type,
+			     u32 attr, int channel, const char **str)
+{
+	struct tmp421_data *data = dev_get_drvdata(dev);
+
+	*str = data->channel[channel].label;
+
+	return 0;
+}
+
 static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type,
 				 u32 attr, int channel)
 {
@@ -194,6 +205,8 @@ static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type,
 	case hwmon_temp_fault:
 	case hwmon_temp_input:
 		return 0444;
+	case hwmon_temp_label:
+		return 0444;
 	default:
 		return 0;
 	}
@@ -286,9 +299,53 @@ static int tmp421_detect(struct i2c_client *client,
 	return 0;
 }
 
+static int tmp421_probe_child_from_dt(struct i2c_client *client,
+				      struct device_node *child,
+				      struct tmp421_data *data)
+
+{
+	struct device *dev = &client->dev;
+	u32 i;
+	int err;
+
+	err = of_property_read_u32(child, "reg", &i);
+	if (err) {
+		dev_err(dev, "missing reg property of %pOFn\n", child);
+		return err;
+	}
+
+	if (i > MAX_CHANNELS) {
+		dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
+		return -EINVAL;
+	}
+
+	of_property_read_string(child, "label", &data->channel[i].label);
+	if (data->channel[i].label)
+		data->temp_config[i] |= HWMON_T_LABEL;
+
+	return 0;
+}
+
+static int tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
+{
+	struct device *dev = &client->dev;
+	const struct device_node *np = dev->of_node;
+	struct device_node *child;
+	int err;
+
+	for_each_child_of_node(np, child) {
+		err = tmp421_probe_child_from_dt(client, child, data);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static const struct hwmon_ops tmp421_ops = {
 	.is_visible = tmp421_is_visible,
 	.read = tmp421_read,
+	.read_string = tmp421_read_string,
 };
 
 static int tmp421_probe(struct i2c_client *client)
@@ -317,6 +374,10 @@ static int tmp421_probe(struct i2c_client *client)
 	for (i = 0; i < data->channels; i++)
 		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
 
+	err = tmp421_probe_from_dt(client, data);
+	if (err)
+		return err;
+
 	data->chip.ops = &tmp421_ops;
 	data->chip.info = data->info;
 
-- 
2.31.1

Re: [PATCH v3 04/11] hwmon: (tmp421) add support for defining labels from DT

[Guenter Roeck]

On Thu, Sep 30, 2021 at 08:59:36AM +0200, Krzysztof Adamski wrote:
> tmp42x is a multichannel temperature sensor with several external
> channels. Since those channels can be used to connect diodes placed
> anywhere in the system, their meaning will vary depending on the
> project. For this case, the hwmon framework has an idea of labels which
> allows us to assign the meaning to each channel.
> 
> The similar concept is already implemented in ina3221 - the label for
> each channel can be defined via device tree. See commit a9e9dd9c6de5
> ("hwmon: (ina3221) Read channel input source info from DT")
> 
> This patch adds support for similar feature to tmp421.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
>  drivers/hwmon/tmp421.c | 61 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> index f3d59ea347e3..7feef60a592c 100644
> --- a/drivers/hwmon/tmp421.c
> +++ b/drivers/hwmon/tmp421.c
> @@ -88,6 +88,7 @@ static const struct of_device_id __maybe_unused tmp421_of_match[] = {
>  MODULE_DEVICE_TABLE(of, tmp421_of_match);
>  
>  struct tmp421_channel {
> +	const char *label;
>  	s16 temp;
>  };
>  
> @@ -187,6 +188,16 @@ static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
>  
>  }
>  
> +static int tmp421_read_string(struct device *dev, enum hwmon_sensor_types type,
> +			     u32 attr, int channel, const char **str)
> +{
> +	struct tmp421_data *data = dev_get_drvdata(dev);
> +
> +	*str = data->channel[channel].label;
> +
> +	return 0;
> +}
> +
>  static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type,
>  				 u32 attr, int channel)
>  {
> @@ -194,6 +205,8 @@ static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type,
>  	case hwmon_temp_fault:
>  	case hwmon_temp_input:
>  		return 0444;
> +	case hwmon_temp_label:
> +		return 0444;
>  	default:
>  		return 0;
>  	}
> @@ -286,9 +299,53 @@ static int tmp421_detect(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static int tmp421_probe_child_from_dt(struct i2c_client *client,
> +				      struct device_node *child,
> +				      struct tmp421_data *data)
> +
> +{
> +	struct device *dev = &client->dev;
> +	u32 i;
> +	int err;
> +
> +	err = of_property_read_u32(child, "reg", &i);
> +	if (err) {
> +		dev_err(dev, "missing reg property of %pOFn\n", child);
> +		return err;
> +	}
> +
> +	if (i > MAX_CHANNELS) {

I think this needs to be >=. MAX_CHANNELS is 4, the array size
is 4, and i == 4 will write after the end of the array.

> +		dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_string(child, "label", &data->channel[i].label);
> +	if (data->channel[i].label)
> +		data->temp_config[i] |= HWMON_T_LABEL;
> +
> +	return 0;
> +}
> +
> +static int tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
> +{
> +	struct device *dev = &client->dev;
> +	const struct device_node *np = dev->of_node;
> +	struct device_node *child;
> +	int err;
> +
> +	for_each_child_of_node(np, child) {
> +		err = tmp421_probe_child_from_dt(client, child, data);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct hwmon_ops tmp421_ops = {
>  	.is_visible = tmp421_is_visible,
>  	.read = tmp421_read,
> +	.read_string = tmp421_read_string,
>  };
>  
>  static int tmp421_probe(struct i2c_client *client)
> @@ -317,6 +374,10 @@ static int tmp421_probe(struct i2c_client *client)
>  	for (i = 0; i < data->channels; i++)
>  		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
>  
> +	err = tmp421_probe_from_dt(client, data);
> +	if (err)
> +		return err;
> +
>  	data->chip.ops = &tmp421_ops;
>  	data->chip.info = data->info;
>  
> -- 
> 2.31.1
> 

Re: [PATCH v3 04/11] hwmon: (tmp421) add support for defining labels from DT

[Krzysztof Adamski]

Dnia Sat, Oct 02, 2021 at 07:08:39AM -0700, Guenter Roeck napisał(a):
>> @@ -286,9 +299,53 @@ static int tmp421_detect(struct i2c_client *client,
>>  	return 0;
>>  }
>>
>> +static int tmp421_probe_child_from_dt(struct i2c_client *client,
>> +				      struct device_node *child,
>> +				      struct tmp421_data *data)
>> +
>> +{
>> +	struct device *dev = &client->dev;
>> +	u32 i;
>> +	int err;
>> +
>> +	err = of_property_read_u32(child, "reg", &i);
>> +	if (err) {
>> +		dev_err(dev, "missing reg property of %pOFn\n", child);
>> +		return err;
>> +	}
>> +
>> +	if (i > MAX_CHANNELS) {
>
>I think this needs to be >=. MAX_CHANNELS is 4, the array size
>is 4, and i == 4 will write after the end of the array.

You are absoluetly right here. Off by one error. I will fix this in v4.

Krzysztof

[PATCH v3 05/11] hwmon: (tmp421) support disabling channels from DT

[Krzysztof Adamski]

The previous patch introduced per channel subnodes in DT that let us
specify some channel specific properties. This built a ground for easily
disabling individual channels of the sensor that may not be connected to
any external diode and thus are not returning any meaningful data.

This patch adds support for parsing the "status" property of channels DT
subnodes and makes sure the -ENODATA is returned when disabled channels
value is read.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/tmp421.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 7feef60a592c..30b601c157fe 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -89,6 +89,7 @@ MODULE_DEVICE_TABLE(of, tmp421_of_match);
 
 struct tmp421_channel {
 	const char *label;
+	bool enabled;
 	s16 temp;
 };
 
@@ -170,6 +171,9 @@ static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
 	if (ret)
 		return ret;
 
+	if (!tmp421->channel[channel].enabled)
+		return -ENODATA;
+
 	switch (attr) {
 	case hwmon_temp_input:
 		*val = temp_from_raw(tmp421->channel[channel].temp,
@@ -323,6 +327,8 @@ static int tmp421_probe_child_from_dt(struct i2c_client *client,
 	if (data->channel[i].label)
 		data->temp_config[i] |= HWMON_T_LABEL;
 
+	data->channel[i].enabled = of_device_is_available(child);
+
 	return 0;
 }
 
@@ -371,8 +377,10 @@ static int tmp421_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
-	for (i = 0; i < data->channels; i++)
+	for (i = 0; i < data->channels; i++) {
 		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
+		data->channel[i].enabled = true;
+	}
 
 	err = tmp421_probe_from_dt(client, data);
 	if (err)
-- 
2.31.1

[PATCH v3 06/11] hwmon: (tmp421) support specifying n-factor via DT

[Krzysztof Adamski]

Previous patches added a way to specify some channel specific parameters
in DT and n-factor is definitely one of them. This calibration mechanism
is board specific as its value depends on the diodes/transistors being
connected to the sensor and thus the DT seems like a right fit for that
information. It is very similar to the value of shunt resistor that some
drivers allows specifying in DT.

This patch adds a possibility to set n-factor for each channel via
"n-factor" DT property in each channel subnode.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/tmp421.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 30b601c157fe..b32fd2b6ed07 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -34,6 +34,7 @@ enum chips { tmp421, tmp422, tmp423, tmp441, tmp442 };
 #define TMP421_STATUS_REG			0x08
 #define TMP421_CONFIG_REG_1			0x09
 #define TMP421_CONVERSION_RATE_REG		0x0B
+#define TMP421_N_FACTOR_REG_1			0x21
 #define TMP421_MANUFACTURER_ID_REG		0xFE
 #define TMP421_DEVICE_ID_REG			0xFF
 
@@ -310,6 +311,7 @@ static int tmp421_probe_child_from_dt(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	u32 i;
+	s32 val;
 	int err;
 
 	err = of_property_read_u32(child, "reg", &i);
@@ -329,6 +331,20 @@ static int tmp421_probe_child_from_dt(struct i2c_client *client,
 
 	data->channel[i].enabled = of_device_is_available(child);
 
+	if (i == 0)
+		return 0; /* input 0 is internal channel */
+
+	err = of_property_read_s32(child, "n-factor", &val);
+	if (!err) {
+		if (val > 127 || val < -128) {
+			dev_err(dev, "n-factor for channel %d invalid (%d)\n",
+				i, val);
+			return -EINVAL;
+		}
+		i2c_smbus_write_byte_data(client, TMP421_N_FACTOR_REG_1 + i - 1,
+						  val);
+	}
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH v3 07/11] hwmon: (tmp421) really disable channels

[Krzysztof Adamski]

Recent patch added possibility to disable selected channels. That would
only make sure that the ENODATA is returned for those channels but would
not configure the actual hardware.

With this patch, the config register is written to make sure the
channels are disabled also at hardware level.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/tmp421.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index b32fd2b6ed07..f9c6f23f1f0f 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -33,6 +33,9 @@ enum chips { tmp421, tmp422, tmp423, tmp441, tmp442 };
 /* The TMP421 registers */
 #define TMP421_STATUS_REG			0x08
 #define TMP421_CONFIG_REG_1			0x09
+#define TMP421_CONFIG_REG_2			0x0A
+#define TMP421_CONFIG_REG_REN(x)		(BIT(3 + (x)))
+#define TMP421_CONFIG_REG_REN_MASK		GENMASK(6, 3)
 #define TMP421_CONVERSION_RATE_REG		0x0B
 #define TMP421_N_FACTOR_REG_1			0x21
 #define TMP421_MANUFACTURER_ID_REG		0xFE
@@ -162,6 +165,31 @@ static int tmp421_update_device(struct tmp421_data *data)
 	return 0;
 }
 
+static int tmp421_enable_channels(struct tmp421_data *data)
+{
+	int err;
+	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
+	int cfg = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_2);
+	int i;
+
+	if (cfg < 0) {
+		dev_err(dev, "error reading register, can't disable channels\n");
+		return err;
+	}
+
+	cfg &= ~TMP421_CONFIG_REG_REN_MASK;
+	for (i = 0; i < data->channels; i++)
+		if (data->channel[i].enabled)
+			cfg |= TMP421_CONFIG_REG_REN(i);
+
+	err = i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_2, cfg);
+	if (err < 0)
+		dev_err(dev, "error writing register, can't disable channels\n");
+
+	return err;
+}
+
 static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
 		       u32 attr, int channel, long *val)
 {
@@ -217,9 +245,10 @@ static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type,
 	}
 }
 
-static int tmp421_init_client(struct i2c_client *client)
+static int tmp421_init_client(struct tmp421_data *data)
 {
 	int config, config_orig;
+	struct i2c_client *client = data->client;
 
 	/* Set the conversion rate to 2 Hz */
 	i2c_smbus_write_byte_data(client, TMP421_CONVERSION_RATE_REG, 0x05);
@@ -240,7 +269,7 @@ static int tmp421_init_client(struct i2c_client *client)
 		i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_1, config);
 	}
 
-	return 0;
+	return tmp421_enable_channels(data);
 }
 
 static int tmp421_detect(struct i2c_client *client,
@@ -389,10 +418,6 @@ static int tmp421_probe(struct i2c_client *client)
 		data->channels = i2c_match_id(tmp421_id, client)->driver_data;
 	data->client = client;
 
-	err = tmp421_init_client(client);
-	if (err)
-		return err;
-
 	for (i = 0; i < data->channels; i++) {
 		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
 		data->channel[i].enabled = true;
@@ -402,6 +427,10 @@ static int tmp421_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
+	err = tmp421_init_client(data);
+	if (err)
+		return err;
+
 	data->chip.ops = &tmp421_ops;
 	data->chip.info = data->info;
 
-- 
2.31.1

[PATCH v3 08/11] hwmon: (tmp421) support HWMON_T_ENABLE

[Krzysztof Adamski]

Since the recent patches added possibility of disabling sensor channels
via DT, it only make sense to allow controlling that from userspace via
HWMON_T_ENABLE mechanism. This patches adds support for that.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/tmp421.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index f9c6f23f1f0f..66ef9c5d9a52 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -200,21 +200,27 @@ static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
 	if (ret)
 		return ret;
 
-	if (!tmp421->channel[channel].enabled)
-		return -ENODATA;
-
 	switch (attr) {
 	case hwmon_temp_input:
+		if (!tmp421->channel[channel].enabled)
+			return -ENODATA;
+
 		*val = temp_from_raw(tmp421->channel[channel].temp,
 				     tmp421->config & TMP421_CONFIG_RANGE);
+
 		return 0;
 	case hwmon_temp_fault:
+		if (!tmp421->channel[channel].enabled)
+			return -ENODATA;
 		/*
 		 * Any of OPEN or /PVLD bits indicate a hardware mulfunction
 		 * and the conversion result may be incorrect
 		 */
 		*val = !!(tmp421->channel[channel].temp & 0x03);
 		return 0;
+	case hwmon_temp_enable:
+		*val = tmp421->channel[channel].enabled;
+		return 0;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -231,6 +237,24 @@ static int tmp421_read_string(struct device *dev, enum hwmon_sensor_types type,
 	return 0;
 }
 
+static int tmp421_write(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long val)
+{
+	struct tmp421_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	switch (type) {
+	case hwmon_temp:
+		data->channel[channel].enabled = val;
+		ret = tmp421_enable_channels(data);
+		break;
+	default:
+	    ret = -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
 static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type,
 				 u32 attr, int channel)
 {
@@ -240,6 +264,8 @@ static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type,
 		return 0444;
 	case hwmon_temp_label:
 		return 0444;
+	case hwmon_temp_enable:
+		return 0644;
 	default:
 		return 0;
 	}
@@ -397,6 +423,7 @@ static const struct hwmon_ops tmp421_ops = {
 	.is_visible = tmp421_is_visible,
 	.read = tmp421_read,
 	.read_string = tmp421_read_string,
+	.write = tmp421_write,
 };
 
 static int tmp421_probe(struct i2c_client *client)
@@ -419,7 +446,7 @@ static int tmp421_probe(struct i2c_client *client)
 	data->client = client;
 
 	for (i = 0; i < data->channels; i++) {
-		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
+		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_ENABLE;
 		data->channel[i].enabled = true;
 	}
 
-- 
2.31.1

[PATCH v3 09/11] hwmon: (tmp421) update documentation

[Krzysztof Adamski]

Sysfs interface of the tmp421 driver was extended in the recent patches
so lets update the documentation to reflect that.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 Documentation/hwmon/tmp421.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/hwmon/tmp421.rst b/Documentation/hwmon/tmp421.rst
index ddcd5159c75d..a3002117bbd7 100644
--- a/Documentation/hwmon/tmp421.rst
+++ b/Documentation/hwmon/tmp421.rst
@@ -64,3 +64,13 @@ the temperature values via the following sysfs files:
 **temp[1-4]_input**
 
 **temp[2-4]_fault**
+
+Each sensor can be individually disabled via Devicetree or from sysfs
+via:
+
+**temp[1-4]_enable**
+
+If labels were specified in Devicetree, additional sysfs files will
+be present:
+
+**temp[1-4]_label**
-- 
2.31.1

[PATCH v3 10/11] hwmon: (tmp421) ignore non input related DT nodes

[Krzysztof Adamski]

In case the DT contains some nodes not describing the input channels,
ignore them instead of exiting with error.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/tmp421.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 66ef9c5d9a52..1b26b022ca69 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -411,6 +411,9 @@ static int tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *d
 	int err;
 
 	for_each_child_of_node(np, child) {
+		if (strcmp(child->name, "input"))
+			continue;
+
 		err = tmp421_probe_child_from_dt(client, child, data);
 		if (err)
 			return err;
-- 
2.31.1

[PATCH v3 11/11] dt-bindings: hwmon: allow specifying channels for tmp421

[Krzysztof Adamski]

Add binding description for the per temperature channel configuration
like labels and n-factor.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 .../devicetree/bindings/hwmon/ti,tmp421.yaml  | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
index 47040ace4f73..0d4ea2209500 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
@@ -24,12 +24,49 @@ properties:
   reg:
     maxItems: 1
 
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
 required:
   - compatible
   - reg
 
 additionalProperties: false
 
+patternProperties:
+  "^input@([0-4])$":
+    type: object
+    description: |
+      Represents channels of the device and their specific configuration.
+
+    properties:
+      reg:
+        description: |
+          The channel number. 0 is local channel, 1-4 are remote channels
+        items:
+          minimum: 0
+          maximum: 4
+
+      label:
+        description: |
+          A descriptive name for this channel, like "ambient" or "psu".
+
+      n-factor:
+        description: |
+          The value (two's complement) to be programmed in the channel specific N correction register.
+          For remote channels only.
+        items:
+          minimum: 0
+          maximum: 1
+
+    required:
+      - reg
+
+    additionalProperties: false
+
 examples:
   - |
     i2c {
@@ -41,3 +78,32 @@ examples:
         reg = <0x4c>;
       };
     };
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      sensor@4c {
+        compatible = "ti,tmp422";
+        reg = <0x4c>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        input@0 {
+          reg = <0x0>;
+          n-factor = <0x1>;
+          label = "local";
+        };
+
+        input@1 {
+          reg = <0x1>;
+          n-factor = <0x0>;
+          label = "somelabel";
+        };
+
+        input@2 {
+          reg = <0x2>;
+          status = "disabled";
+        };
+      };
+    };
-- 
2.31.1

Re: [PATCH v3 11/11] dt-bindings: hwmon: allow specifying channels for tmp421

[Guenter Roeck]

On Thu, Sep 30, 2021 at 09:19:49AM +0200, Krzysztof Adamski wrote:
> Add binding description for the per temperature channel configuration
> like labels and n-factor.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
>  .../devicetree/bindings/hwmon/ti,tmp421.yaml  | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> index 47040ace4f73..0d4ea2209500 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> @@ -24,12 +24,49 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
>  required:
>    - compatible
>    - reg
>  
>  additionalProperties: false
>  
> +patternProperties:
> +  "^input@([0-4])$":

Was there agreement on "input" ? It is a somewhat odd name for a temperature
sensor. If that name can be used to distinguish child sensor types, it might
make sense to have a well defined name to state that this is a temperature
sensor.

> +    type: object
> +    description: |
> +      Represents channels of the device and their specific configuration.
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number. 0 is local channel, 1-4 are remote channels

Which of the supported chips has 4 remote channels ?

> +        items:
> +          minimum: 0
> +          maximum: 4
> +
> +      label:
> +        description: |
> +          A descriptive name for this channel, like "ambient" or "psu".
> +
> +      n-factor:

n-factor or "ti,n-factor" ? The unit is chip specific, after all.

> +        description: |
> +          The value (two's complement) to be programmed in the channel specific N correction register.
> +          For remote channels only.
> +        items:
> +          minimum: 0
> +          maximum: 1

Is this the correct value range ? The value range (in integer form) is
-128 .. 127 (or 0 .. 255 as unsigned), not 0..1.

> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
>  examples:
>    - |
>      i2c {
> @@ -41,3 +78,32 @@ examples:
>          reg = <0x4c>;
>        };
>      };
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      sensor@4c {
> +        compatible = "ti,tmp422";
> +        reg = <0x4c>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        input@0 {
> +          reg = <0x0>;
> +          n-factor = <0x1>;
> +          label = "local";
> +        };
> +
> +        input@1 {
> +          reg = <0x1>;
> +          n-factor = <0x0>;
> +          label = "somelabel";
> +        };
> +
> +        input@2 {
> +          reg = <0x2>;
> +          status = "disabled";
> +        };
> +      };
> +    };
> -- 
> 2.31.1
> 

Re: [PATCH v3 11/11] dt-bindings: hwmon: allow specifying channels for tmp421

[Krzysztof Adamski]

Dnia Sat, Oct 02, 2021 at 07:22:19AM -0700, Guenter Roeck napisał(a):
>On Thu, Sep 30, 2021 at 09:19:49AM +0200, Krzysztof Adamski wrote:
>> Add binding description for the per temperature channel configuration
>> like labels and n-factor.
>>
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>> ---
>>  .../devicetree/bindings/hwmon/ti,tmp421.yaml  | 66 +++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
>> index 47040ace4f73..0d4ea2209500 100644
>> --- a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
>> @@ -24,12 +24,49 @@ properties:
>>    reg:
>>      maxItems: 1
>>
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>>  required:
>>    - compatible
>>    - reg
>>
>>  additionalProperties: false
>>
>> +patternProperties:
>> +  "^input@([0-4])$":
>
>Was there agreement on "input" ? It is a somewhat odd name for a temperature
>sensor. If that name can be used to distinguish child sensor types, it might
>make sense to have a well defined name to state that this is a temperature
>sensor.

Nope, no conclusion on that, yet, thus I did not change that and I was
still using the same approach I had on v1. For me it can be a "channel@X", a
"temperature@X".. whatever you decide.

However I'm in favor of some generic name, like "channel" or "input",
and using some "type property", if required, instead of calling the
nodes "temperatue@X", "voltage@X".

>> +    type: object
>> +    description: |
>> +      Represents channels of the device and their specific configuration.
>> +
>> +    properties:
>> +      reg:
>> +        description: |
>> +          The channel number. 0 is local channel, 1-4 are remote channels
>
>Which of the supported chips has 4 remote channels ?

True, there is no TMP424. I will fix that in v4.

>
>> +        items:
>> +          minimum: 0
>> +          maximum: 4
>> +
>> +      label:
>> +        description: |
>> +          A descriptive name for this channel, like "ambient" or "psu".
>> +
>> +      n-factor:
>
>n-factor or "ti,n-factor" ? The unit is chip specific, after all.

Or ti,nfactor, as used by tmp513? Again, there was no clear conclusion
so I didn't change that. Let me know what is your decision and I will
obey that.

>
>> +        description: |
>> +          The value (two's complement) to be programmed in the channel specific N correction register.
>> +          For remote channels only.
>> +        items:
>> +          minimum: 0
>> +          maximum: 1
>
>Is this the correct value range ? The value range (in integer form) is
>-128 .. 127 (or 0 .. 255 as unsigned), not 0..1.

True, I must have misunderstood this minimum/maximum and confused it
with the number of items or something. Now, since DT does not really
handle signed values and considers everything an unsigned, should I use
0..255 or -128..127?

Krzysztof

Re: [PATCH v3 11/11] dt-bindings: hwmon: allow specifying channels for tmp421

[Guenter Roeck]

On Mon, Oct 04, 2021 at 09:36:23AM +0200, Krzysztof Adamski wrote:
> Dnia Sat, Oct 02, 2021 at 07:22:19AM -0700, Guenter Roeck napisał(a):
> > On Thu, Sep 30, 2021 at 09:19:49AM +0200, Krzysztof Adamski wrote:
> > > Add binding description for the per temperature channel configuration
> > > like labels and n-factor.
> > > 
> > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> > > ---
> > >  .../devicetree/bindings/hwmon/ti,tmp421.yaml  | 66 +++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> > > index 47040ace4f73..0d4ea2209500 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> > > +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> > > @@ -24,12 +24,49 @@ properties:
> > >    reg:
> > >      maxItems: 1
> > > 
> > > +  '#address-cells':
> > > +    const: 1
> > > +
> > > +  '#size-cells':
> > > +    const: 0
> > > +
> > >  required:
> > >    - compatible
> > >    - reg
> > > 
> > >  additionalProperties: false
> > > 
> > > +patternProperties:
> > > +  "^input@([0-4])$":
> > 
> > Was there agreement on "input" ? It is a somewhat odd name for a temperature
> > sensor. If that name can be used to distinguish child sensor types, it might
> > make sense to have a well defined name to state that this is a temperature
> > sensor.
> 
> Nope, no conclusion on that, yet, thus I did not change that and I was
> still using the same approach I had on v1. For me it can be a "channel@X", a
> "temperature@X".. whatever you decide.
> 

My question was more on mandating a single string instead of letting
users decide. I don't care either if it isn't used for anything in
particular, but you specifically mandate "input" as the only valid
string. I am not a DT expert, but it seems to me that mandating the
content of that string and then not using it other than to ensure that
the user really specified "input" doesn't make much sense to me.
Having said that, if this is the DT way of things, it is ok with
me.

> However I'm in favor of some generic name, like "channel" or "input",
> and using some "type property", if required, instead of calling the
> nodes "temperatue@X", "voltage@X".
> 

It does open up a nother dimension for multi-type sensor chips, though,

For a chip with voltage and temperature sensors:

	temperature@0 {
		reg = <0>;
	};

	voltage@0 {
		reg = <0>;
	};

vs:

	temperature-sensors {
		xxx@0 {
			reg = <0>;
		};
	};

	voltage-sensors {
		xxx@0 {
			reg = <0>;
		};
	};

This is way out of my league in terms of what is appropriate,
except that "xxx" isn't always easy to determine if the string is fixed
as you suggest. What should it be for a sensor measuring an output voltage ?

	input@0 {
		reg = <0>;
		label = "output voltage";
	};

Anyway, maybe Rob has an idea how to name this properly.

Guenter

> > > +    type: object
> > > +    description: |
> > > +      Represents channels of the device and their specific configuration.
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description: |
> > > +          The channel number. 0 is local channel, 1-4 are remote channels
> > 
> > Which of the supported chips has 4 remote channels ?
> 
> True, there is no TMP424. I will fix that in v4.
> 
> > 
> > > +        items:
> > > +          minimum: 0
> > > +          maximum: 4
> > > +
> > > +      label:
> > > +        description: |
> > > +          A descriptive name for this channel, like "ambient" or "psu".
> > > +
> > > +      n-factor:
> > 
> > n-factor or "ti,n-factor" ? The unit is chip specific, after all.
> 
> Or ti,nfactor, as used by tmp513? Again, there was no clear conclusion
> so I didn't change that. Let me know what is your decision and I will
> obey that.

Not my call to make about nfactor or n-factor, really. I'll leave that
for Rob to decide.
> 
> > 
> > > +        description: |
> > > +          The value (two's complement) to be programmed in the channel specific N correction register.

[ side note: Since the value is just a register value, "two's complement" seems
  unnecessary and confusing; in the context of the DT description it doesn't
  really matter what the register values actually mean. ]

> > > +          For remote channels only.
> > > +        items:
> > > +          minimum: 0
> > > +          maximum: 1
> > 
> > Is this the correct value range ? The value range (in integer form) is
> > -128 .. 127 (or 0 .. 255 as unsigned), not 0..1.
> 
> True, I must have misunderstood this minimum/maximum and confused it
> with the number of items or something. Now, since DT does not really
> handle signed values and considers everything an unsigned, should I use
> 0..255 or -128..127?
> 

I suspect it should be 0..255. After all, the values reflect register values,
not their meaning. But I don't really know. Rob ?

Guenter

Re: [PATCH v3 11/11] dt-bindings: hwmon: allow specifying channels for tmp421

[Krzysztof Adamski]

Dnia Tue, Oct 05, 2021 at 07:14:57AM -0700, Guenter Roeck napisał(a):
>> > > +patternProperties:
>> > > +  "^input@([0-4])$":
>> >
>> > Was there agreement on "input" ? It is a somewhat odd name for a temperature
>> > sensor. If that name can be used to distinguish child sensor types, it might
>> > make sense to have a well defined name to state that this is a temperature
>> > sensor.
>>
>> Nope, no conclusion on that, yet, thus I did not change that and I was
>> still using the same approach I had on v1. For me it can be a "channel@X", a
>> "temperature@X".. whatever you decide.
>>
>
>My question was more on mandating a single string instead of letting
>users decide. I don't care either if it isn't used for anything in
>particular, but you specifically mandate "input" as the only valid
>string. I am not a DT expert, but it seems to me that mandating the
>content of that string and then not using it other than to ensure that
>the user really specified "input" doesn't make much sense to me.
>Having said that, if this is the DT way of things, it is ok with
>me.

I guess the reason I have used "input@X" was, again, because I based my
idea of how DT bindings should look like on the ina3221 as it seemed
like the most similar to my case. This chip, however, current/voltage
monitor so "input" makes more sense there, indeed.

In general, while I do like consistency, we are not defining a binding
for all hwmon sensors now as each of them, anyways, has its own binding
documentation so some variation is allowed, I think (and inevitable as
we won't change existing bindings).

So, I do mandate that the child nodes for *this* device should be named
accordingly but this doesn't mean other hwmon devices can't use others names
that makes more sense to them. While I could make the code that will
iterate over all child nodes and will ignore their names, I do think it
is better to specify the strict naming convention that everybody will
use (again, for *this* device), instead. I see several advantages of
this:
  - consistency in different DTs
  - easier extensibility - if we introduce something that will require
    adding some other non-channel related nodes, we don't have to worry
    that much about the name clashing
  - it makes verifying the DT correctness easier

>> However I'm in favor of some generic name, like "channel" or "input",
>> and using some "type property", if required, instead of calling the
>> nodes "temperatue@X", "voltage@X".
>>
>
>It does open up a nother dimension for multi-type sensor chips, though,
>
>For a chip with voltage and temperature sensors:
>
>	temperature@0 {
>		reg = <0>;
>	};
>
>	voltage@0 {
>		reg = <0>;
>	};
>
>vs:
>
>	temperature-sensors {
>		xxx@0 {
>			reg = <0>;
>		};
>	};
>
>	voltage-sensors {
>		xxx@0 {
>			reg = <0>;
>		};
>	};

Out of those, I strongly prefer the first one. But, again, we don't have
to define one kind of binding for ALL hwmon sensors (but I do think its
better if we can be generic).
So the biggest question to me is whether temperature and voltage should
have separate "address spaces". If we, indeed, want to have
temperature@0 and voltage@0, then using specific names, instead of
generic, like "channel", does make sense. But then, again, I don't see a
problem with one driver having a binding with "channel@X", while other
having "temperature@X" and "voltage@X".

>
>This is way out of my league in terms of what is appropriate,
>except that "xxx" isn't always easy to determine if the string is fixed
>as you suggest. What should it be for a sensor measuring an output voltage ?
>
>	input@0 {
>		reg = <0>;
>		label = "output voltage";
>	};
>

I think that if all the "channels" of the device are of the same type,
it doesn't matter. If we have some inputs and some outputs, we should
have either:

channel@0 {
	reg = <0>;
	type = "input";   // or something like that, maybe a numeric value
	with defines
}
channel@1 {
	reg = <1>;
	type = "output";   // or something like that, maybe a numeric value
	with defines
}

Or:

input@0 {
  reg = <0>;
}

output@1 {
  reg = <1>;
}

But, again, TMP42X doesn't need any of that anyways :)

>Anyway, maybe Rob has an idea how to name this properly.

Rob? :)

>> > Is this the correct value range ? The value range (in integer form) is
>> > -128 .. 127 (or 0 .. 255 as unsigned), not 0..1.
>>
>> True, I must have misunderstood this minimum/maximum and confused it
>> with the number of items or something. Now, since DT does not really
>> handle signed values and considers everything an unsigned, should I use
>> 0..255 or -128..127?
>>
>
>I suspect it should be 0..255. After all, the values reflect register values,
>not their meaning. But I don't really know. Rob ?

The DT blob will only contain 0..255. DTC has a syntactic sugar that
lets you specify the value as negative and will convert it to 2s
complement for you. So those two are exactly the same:

n-factor = <(-10)>;
n-factor = <0xfffffff6>;

 From the code perspective, however, all 3 most significant bytes are
ignored so 0xfffffff6 is the same as 0xf6.

Krzysztof

Re: [PATCH v3 11/11] dt-bindings: hwmon: allow specifying channels for tmp421

[Rob Herring]

On Tue, Oct 05, 2021 at 07:14:57AM -0700, Guenter Roeck wrote:
> On Mon, Oct 04, 2021 at 09:36:23AM +0200, Krzysztof Adamski wrote:
> > Dnia Sat, Oct 02, 2021 at 07:22:19AM -0700, Guenter Roeck napisał(a):
> > > On Thu, Sep 30, 2021 at 09:19:49AM +0200, Krzysztof Adamski wrote:
> > > > Add binding description for the per temperature channel configuration
> > > > like labels and n-factor.
> > > > 
> > > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> > > > ---
> > > >  .../devicetree/bindings/hwmon/ti,tmp421.yaml  | 66 +++++++++++++++++++
> > > >  1 file changed, 66 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> > > > index 47040ace4f73..0d4ea2209500 100644
> > > > --- a/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> > > > +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp421.yaml
> > > > @@ -24,12 +24,49 @@ properties:
> > > >    reg:
> > > >      maxItems: 1
> > > > 
> > > > +  '#address-cells':
> > > > +    const: 1
> > > > +
> > > > +  '#size-cells':
> > > > +    const: 0
> > > > +
> > > >  required:
> > > >    - compatible
> > > >    - reg
> > > > 
> > > >  additionalProperties: false
> > > > 
> > > > +patternProperties:
> > > > +  "^input@([0-4])$":
> > > 
> > > Was there agreement on "input" ? It is a somewhat odd name for a temperature
> > > sensor. If that name can be used to distinguish child sensor types, it might
> > > make sense to have a well defined name to state that this is a temperature
> > > sensor.
> > 
> > Nope, no conclusion on that, yet, thus I did not change that and I was
> > still using the same approach I had on v1. For me it can be a "channel@X", a
> > "temperature@X".. whatever you decide.
> > 
> 
> My question was more on mandating a single string instead of letting
> users decide. I don't care either if it isn't used for anything in
> particular, but you specifically mandate "input" as the only valid
> string. I am not a DT expert, but it seems to me that mandating the
> content of that string and then not using it other than to ensure that
> the user really specified "input" doesn't make much sense to me.
> Having said that, if this is the DT way of things, it is ok with
> me.

Kind of a catch-22. Node names weren't consistent nor checked, so 
we can use them. 

> > However I'm in favor of some generic name, like "channel" or "input",
> > and using some "type property", if required, instead of calling the
> > nodes "temperatue@X", "voltage@X".
> > 
> 
> It does open up a nother dimension for multi-type sensor chips, though,
> 
> For a chip with voltage and temperature sensors:
> 
> 	temperature@0 {
> 		reg = <0>;
> 	};
> 
> 	voltage@0 {
> 		reg = <0>;
> 	};

Not valid because you have same address twice.

> 
> vs:
> 
> 	temperature-sensors {
> 		xxx@0 {
> 			reg = <0>;
> 		};
> 	};
> 
> 	voltage-sensors {
> 		xxx@0 {
> 			reg = <0>;
> 		};
> 	};
> 

Didn't we already discuss this?

> This is way out of my league in terms of what is appropriate,
> except that "xxx" isn't always easy to determine if the string is fixed
> as you suggest. What should it be for a sensor measuring an output voltage ?

Does measuring a voltage have a direction?

> 
> 	input@0 {
> 		reg = <0>;
> 		label = "output voltage";
> 	};
> 
> Anyway, maybe Rob has an idea how to name this properly.

No, I don't have a sense of the range of h/w...

> 
> Guenter
> 
> > > > +    type: object
> > > > +    description: |
> > > > +      Represents channels of the device and their specific configuration.
> > > > +
> > > > +    properties:
> > > > +      reg:
> > > > +        description: |
> > > > +          The channel number. 0 is local channel, 1-4 are remote channels
> > > 
> > > Which of the supported chips has 4 remote channels ?
> > 
> > True, there is no TMP424. I will fix that in v4.
> > 
> > > 
> > > > +        items:
> > > > +          minimum: 0
> > > > +          maximum: 4
> > > > +
> > > > +      label:
> > > > +        description: |
> > > > +          A descriptive name for this channel, like "ambient" or "psu".
> > > > +
> > > > +      n-factor:
> > > 
> > > n-factor or "ti,n-factor" ? The unit is chip specific, after all.
> > 
> > Or ti,nfactor, as used by tmp513? Again, there was no clear conclusion
> > so I didn't change that. Let me know what is your decision and I will
> > obey that.
> 
> Not my call to make about nfactor or n-factor, really. I'll leave that
> for Rob to decide.

ti,n-factor

> > 
> > > 
> > > > +        description: |
> > > > +          The value (two's complement) to be programmed in the channel specific N correction register.
> 
> [ side note: Since the value is just a register value, "two's complement" seems
>   unnecessary and confusing; in the context of the DT description it doesn't
>   really matter what the register values actually mean. ]
> 
> > > > +          For remote channels only.
> > > > +        items:
> > > > +          minimum: 0
> > > > +          maximum: 1
> > > 
> > > Is this the correct value range ? The value range (in integer form) is
> > > -128 .. 127 (or 0 .. 255 as unsigned), not 0..1.
> > 
> > True, I must have misunderstood this minimum/maximum and confused it
> > with the number of items or something. Now, since DT does not really
> > handle signed values and considers everything an unsigned, should I use
> > 0..255 or -128..127?
> > 
> 
> I suspect it should be 0..255. After all, the values reflect register values,
> not their meaning. But I don't really know. Rob ?

That's fine.

You can define it as a signed type, but the validation there is not 
working due to dts->dt.yaml losing the sign.

Rob

Re: [PATCH v3 11/11] dt-bindings: hwmon: allow specifying channels for tmp421

[Krzysztof Adamski]

Dnia Wed, Oct 06, 2021 at 03:55:46PM -0500, Rob Herring napisał(a):
>>
>> 	input@0 {
>> 		reg = <0>;
>> 		label = "output voltage";
>> 	};
>>
>> Anyway, maybe Rob has an idea how to name this properly.
>
>No, I don't have a sense of the range of h/w...

I feel like we are stuck. Rob does not have a sense of the range of the
h/w and Guenter does not have a sense of the DeviceTree idioms. How can
we solve that?

Could we, maybe, just focus on this typical, simplified, case I have for
now - a sensor with several channels of known, same type (temperature)?
We clearly are unable handle all possible cases here, for now.

Does this look sane for that usecase or what would you, Rob, change?

sensor@4c {
   compatible = "ti,tmp422";
   reg = <0x4c>;
   #address-cells = <1>;
   #size-cells = <0>;

   input@0 {
     reg = <0x0>;
     ti,n-factor = <0x1>;
     label = "local";
   };

   input@1 {
     reg = <0x1>;
     ti,n-factor = <0x0>;
     label = "somelabel";
   };

   input@2 {
     reg = <0x2>;
     status = "disabled";
   };
};

There were some doubts whether "input" makes sense here.  I still think
it doas as even in HWMON subsystem, we have "hwmon_temp_input" and
HWMON_T_INPUT, so a temperature channel _is_ an input.  Of course I can
change it to "temperature" or "channel", just tell me which one is
accepted.

Krzysztof

Re: [PATCH v3 11/11] dt-bindings: hwmon: allow specifying channels for tmp421

[Guenter Roeck]

On 10/7/21 12:51 AM, Krzysztof Adamski wrote:
> Dnia Wed, Oct 06, 2021 at 03:55:46PM -0500, Rob Herring napisał(a):
>>>
>>>     input@0 {
>>>         reg = <0>;
>>>         label = "output voltage";
>>>     };
>>>
>>> Anyway, maybe Rob has an idea how to name this properly.
>>
>> No, I don't have a sense of the range of h/w...
> 
> I feel like we are stuck. Rob does not have a sense of the range of the
> h/w and Guenter does not have a sense of the DeviceTree idioms. How can
> we solve that?
> 

That is why I am asking questions. It doesn't mean we are stuck.

> Could we, maybe, just focus on this typical, simplified, case I have for
> now - a sensor with several channels of known, same type (temperature)?
> We clearly are unable handle all possible cases here, for now.
> 
> Does this look sane for that usecase or what would you, Rob, change?
> 
> sensor@4c {
>    compatible = "ti,tmp422";
>    reg = <0x4c>;
>    #address-cells = <1>;
>    #size-cells = <0>;
> 
>    input@0 {
>      reg = <0x0>;
>      ti,n-factor = <0x1>;
>      label = "local";
>    };
> 
>    input@1 {
>      reg = <0x1>;
>      ti,n-factor = <0x0>;
>      label = "somelabel";
>    };
> 
>    input@2 {
>      reg = <0x2>;
>      status = "disabled";
>    };
> };
> 
> There were some doubts whether "input" makes sense here.  I still think
> it doas as even in HWMON subsystem, we have "hwmon_temp_input" and
> HWMON_T_INPUT, so a temperature channel _is_ an input.  Of course I can
> change it to "temperature" or "channel", just tell me which one is
> accepted.
> 

I'd be fine with "channel" or "sensor". Both would be generic.

Guenter

Re: [PATCH v3 11/11] dt-bindings: hwmon: allow specifying channels for tmp421

[Rob Herring]

On Fri, Oct 8, 2021 at 9:33 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/7/21 12:51 AM, Krzysztof Adamski wrote:
> > Dnia Wed, Oct 06, 2021 at 03:55:46PM -0500, Rob Herring napisał(a):
> >>>
> >>>     input@0 {
> >>>         reg = <0>;
> >>>         label = "output voltage";
> >>>     };
> >>>
> >>> Anyway, maybe Rob has an idea how to name this properly.
> >>
> >> No, I don't have a sense of the range of h/w...
> >
> > I feel like we are stuck. Rob does not have a sense of the range of the
> > h/w and Guenter does not have a sense of the DeviceTree idioms. How can
> > we solve that?
> >
>
> That is why I am asking questions. It doesn't mean we are stuck.
>
> > Could we, maybe, just focus on this typical, simplified, case I have for
> > now - a sensor with several channels of known, same type (temperature)?
> > We clearly are unable handle all possible cases here, for now.
> >
> > Does this look sane for that usecase or what would you, Rob, change?
> >
> > sensor@4c {
> >    compatible = "ti,tmp422";
> >    reg = <0x4c>;
> >    #address-cells = <1>;
> >    #size-cells = <0>;
> >
> >    input@0 {
> >      reg = <0x0>;
> >      ti,n-factor = <0x1>;
> >      label = "local";
> >    };
> >
> >    input@1 {
> >      reg = <0x1>;
> >      ti,n-factor = <0x0>;
> >      label = "somelabel";
> >    };
> >
> >    input@2 {
> >      reg = <0x2>;
> >      status = "disabled";
> >    };
> > };
> >
> > There were some doubts whether "input" makes sense here.  I still think
> > it doas as even in HWMON subsystem, we have "hwmon_temp_input" and
> > HWMON_T_INPUT, so a temperature channel _is_ an input.  Of course I can
> > change it to "temperature" or "channel", just tell me which one is
> > accepted.
> >
>
> I'd be fine with "channel" or "sensor". Both would be generic.

'channel' aligns with multi-channel ADC node naming, so that's fine for me.

Rob