[PATCH 0/8] Add per channel properies support in tmp421

[PATCH 0/8] Add per channel properies support in tmp421

[Krzysztof Adamski]

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.

Krzysztof Adamski (8):
  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
  dt-bindings: hwmon: allow specifying channels for tmp421

 .../devicetree/bindings/hwmon/tmp421.yaml     | 109 ++++++++++++++
 drivers/hwmon/tmp421.c                        | 141 ++++++++++++++++--
 2 files changed, 234 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/tmp421.yaml

-- 
2.31.1

[PATCH 1/8] 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/tmp421.yaml     | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/tmp421.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
new file mode 100644
index 000000000000..53940e146ee6
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/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/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 1/8] dt-bindings: hwmon: add missing tmp421 binding

[Rob Herring]

On Tue, 07 Sep 2021 15:42:24 +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/tmp421.yaml     | 43 +++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/tmp421.yaml
> 

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

[PATCH 2/8] 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 ede66ea6a730..2c9ba5fe5f2a 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_s16(s16 reg)
-- 
2.31.1

[PATCH 3/8] 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 | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 2c9ba5fe5f2a..1068fe59df0b 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_s16(s16 reg)
@@ -134,9 +138,9 @@ static struct tmp421_data *tmp421_update_device(struct device *dev)
 			TMP421_CONFIG_REG_1);
 
 		for (i = 0; i < data->channels; i++) {
-			data->temp[i] = i2c_smbus_read_byte_data(client,
+			data->channel[i].temp = i2c_smbus_read_byte_data(client,
 				TMP421_TEMP_MSB[i]) << 8;
-			data->temp[i] |= i2c_smbus_read_byte_data(client,
+			data->channel[i].temp |= i2c_smbus_read_byte_data(client,
 				TMP421_TEMP_LSB[i]);
 		}
 		data->last_updated = jiffies;
@@ -156,16 +160,16 @@ static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
 	switch (attr) {
 	case hwmon_temp_input:
 		if (tmp421->config & TMP421_CONFIG_RANGE)
-			*val = temp_from_u16(tmp421->temp[channel]);
+			*val = temp_from_u16(tmp421->channel[channel].temp);
 		else
-			*val = temp_from_s16(tmp421->temp[channel]);
+			*val = temp_from_s16(tmp421->channel[channel].temp);
 		return 0;
 	case hwmon_temp_fault:
 		/*
 		 * The OPEN bit signals a fault. This is bit 0 of the temperature
 		 * register (low byte).
 		 */
-		*val = tmp421->temp[channel] & 0x01;
+		*val = tmp421->channel[channel].temp & 0x01;
 		return 0;
 	default:
 		return -EOPNOTSUPP;
-- 
2.31.1

[PATCH 4/8] 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 | 51 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 1068fe59df0b..a1dba1d405ee 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;
 };
 
@@ -177,6 +178,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)
 {
@@ -187,6 +198,8 @@ static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type,
 		return 0444;
 	case hwmon_temp_input:
 		return 0444;
+	case hwmon_temp_label:
+		return 0444;
 	default:
 		return 0;
 	}
@@ -279,9 +292,45 @@ static int tmp421_detect(struct i2c_client *client,
 	return 0;
 }
 
+void 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;
+	} else if (i > MAX_CHANNELS) {
+		dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
+		return;
+	}
+
+	of_property_read_string(child, "label", &data->channel[i].label);
+	if (data->channel[i].label)
+		data->temp_config[i] |= HWMON_T_LABEL;
+
+}
+
+void 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;
+
+	for_each_child_of_node(np, child) {
+		tmp421_probe_child_from_dt(client, child, data);
+	}
+}
+
 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)
@@ -310,6 +359,8 @@ 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;
 
+	tmp421_probe_from_dt(client, data);
+
 	data->chip.ops = &tmp421_ops;
 	data->chip.info = data->info;
 
-- 
2.31.1

Re: [PATCH 4/8] hwmon: (tmp421) add support for defining labels from DT

[Guenter Roeck]

On 9/7/21 6:43 AM, 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 | 51 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> index 1068fe59df0b..a1dba1d405ee 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;
>   };
>   
> @@ -177,6 +178,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)
>   {
> @@ -187,6 +198,8 @@ static umode_t tmp421_is_visible(const void *data, enum hwmon_sensor_types type,
>   		return 0444;
>   	case hwmon_temp_input:
>   		return 0444;
> +	case hwmon_temp_label:
> +		return 0444;
>   	default:
>   		return 0;
>   	}
> @@ -279,9 +292,45 @@ static int tmp421_detect(struct i2c_client *client,
>   	return 0;
>   }
>   
> +void 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;

Report to caller

> +	} else if (i > MAX_CHANNELS) {

else after return is pointless.

> +		dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
> +		return;

Again report to caller.

> +	}
> +
> +	of_property_read_string(child, "label", &data->channel[i].label);
> +	if (data->channel[i].label)
> +		data->temp_config[i] |= HWMON_T_LABEL;
> +
> +}
> +
> +void 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;
> +
> +	for_each_child_of_node(np, child) {
> +		tmp421_probe_child_from_dt(client, child, data);
> +	}
> +}
> +
>   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)
> @@ -310,6 +359,8 @@ 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;
>   
> +	tmp421_probe_from_dt(client, data);
> +
>   	data->chip.ops = &tmp421_ops;
>   	data->chip.info = data->info;
>   
> 

Re: [PATCH 4/8] hwmon: (tmp421) add support for defining labels from DT

[Krzysztof Adamski]

Dnia Tue, Sep 07, 2021 at 08:46:22AM -0700, Guenter Roeck napisał(a):
>>+void 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;
>
>Report to caller
>

My idea was to make those errors in DT non-critical. I.e. if one of the
child nodes is not well structured, I just skip it but continue the
probing. Do you think it should be considered critical and I should
abort the whole probe function as soon as I detect such DT errors,
instead?

Krzysztof

Re: [PATCH 4/8] hwmon: (tmp421) add support for defining labels from DT

[Guenter Roeck]

On Tue, Sep 07, 2021 at 07:49:36PM +0200, Krzysztof Adamski wrote:
> Dnia Tue, Sep 07, 2021 at 08:46:22AM -0700, Guenter Roeck napisał(a):
> > > +void 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;
> > 
> > Report to caller
> > 
> 
> My idea was to make those errors in DT non-critical. I.e. if one of the
> child nodes is not well structured, I just skip it but continue the
> probing. Do you think it should be considered critical and I should
> abort the whole probe function as soon as I detect such DT errors,
> instead?
> 
Yes, I do think so. Otherwise people will just generate bad DT files and
never fix them or even on purpose ignore the error messages. I don't want
to see such messages in a production system.

On a side note, I do not accept dev_err/pr_err if the error is ignored.
Either it is an error, or it isn't.

Thanks,
Guenter

Re: [PATCH 4/8] hwmon: (tmp421) add support for defining labels from DT

[Krzysztof Adamski]

Dnia Tue, Sep 07, 2021 at 10:55:52AM -0700, Guenter Roeck napisał(a):
>On Tue, Sep 07, 2021 at 07:49:36PM +0200, Krzysztof Adamski wrote:
>> Dnia Tue, Sep 07, 2021 at 08:46:22AM -0700, Guenter Roeck napisał(a):
>> > > +void 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;
>> >
>> > Report to caller
>> >
>>
>> My idea was to make those errors in DT non-critical. I.e. if one of the
>> child nodes is not well structured, I just skip it but continue the
>> probing. Do you think it should be considered critical and I should
>> abort the whole probe function as soon as I detect such DT errors,
>> instead?
>>
>Yes, I do think so. Otherwise people will just generate bad DT files and
>never fix them or even on purpose ignore the error messages. I don't want
>to see such messages in a production system.
>
>On a side note, I do not accept dev_err/pr_err if the error is ignored.
>Either it is an error, or it isn't.

Makes sense - if it's an error, it should be reported as such, otherwise
it's just a warning.
I'll change the code to fail on DT errors in v2.

Thank you,
Krzysztof

Re: [PATCH 4/8] hwmon: (tmp421) add support for defining labels from DT

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3806 bytes --]

Hi Krzysztof,

I love your patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on robh/for-next v5.14 next-20210907]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: mips-randconfig-c004-20210907 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9c476172b93367d2cb88d7d3f4b1b5b456fa6020)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/50e1eb6bf222a2fb0df304033f2aaed075fd76b2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724
        git checkout 50e1eb6bf222a2fb0df304033f2aaed075fd76b2
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/hwmon/tmp421.c:295:6: warning: no previous prototype for function 'tmp421_probe_child_from_dt' [-Wmissing-prototypes]
   void tmp421_probe_child_from_dt(struct i2c_client *client,
        ^
   drivers/hwmon/tmp421.c:295:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void tmp421_probe_child_from_dt(struct i2c_client *client,
   ^
   static 
>> drivers/hwmon/tmp421.c:319:6: warning: no previous prototype for function 'tmp421_probe_from_dt' [-Wmissing-prototypes]
   void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
        ^
   drivers/hwmon/tmp421.c:319:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
   ^
   static 
   2 warnings generated.


vim +/tmp421_probe_child_from_dt +295 drivers/hwmon/tmp421.c

   294	
 > 295	void tmp421_probe_child_from_dt(struct i2c_client *client,
   296					struct device_node *child,
   297					struct tmp421_data *data)
   298	
   299	{
   300		struct device *dev = &client->dev;
   301		u32 i;
   302		int err;
   303	
   304		err = of_property_read_u32(child, "reg", &i);
   305		if (err) {
   306			dev_err(dev, "missing reg property of %pOFn\n", child);
   307			return;
   308		} else if (i > MAX_CHANNELS) {
   309			dev_err(dev, "invalid reg %d of %pOFn\n", i, child);
   310			return;
   311		}
   312	
   313		of_property_read_string(child, "label", &data->channel[i].label);
   314		if (data->channel[i].label)
   315			data->temp_config[i] |= HWMON_T_LABEL;
   316	
   317	}
   318	
 > 319	void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
   320	{
   321		struct device *dev = &client->dev;
   322		const struct device_node *np = dev->of_node;
   323		struct device_node *child;
   324	
   325		for_each_child_of_node(np, child) {
   326			tmp421_probe_child_from_dt(client, child, data);
   327		}
   328	}
   329	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32497 bytes --]

Re: [PATCH 4/8] hwmon: (tmp421) add support for defining labels from DT

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]

Hi Krzysztof,

I love your patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on robh/for-next v5.14 next-20210909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-s001-20210908 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/50e1eb6bf222a2fb0df304033f2aaed075fd76b2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724
        git checkout 50e1eb6bf222a2fb0df304033f2aaed075fd76b2
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hwmon/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/hwmon/tmp421.c:295:6: sparse: sparse: symbol 'tmp421_probe_child_from_dt' was not declared. Should it be static?
>> drivers/hwmon/tmp421.c:319:6: sparse: sparse: symbol 'tmp421_probe_from_dt' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27564 bytes --]

[RFC PATCH] hwmon: tmp421_probe_child_from_dt() can be static

[kernel test robot]

drivers/hwmon/tmp421.c:295:6: warning: symbol 'tmp421_probe_child_from_dt' was not declared. Should it be static?
drivers/hwmon/tmp421.c:319:6: warning: symbol 'tmp421_probe_from_dt' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 tmp421.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index a1dba1d405ee8..5f1f3ec9f51c1 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -292,7 +292,7 @@ static int tmp421_detect(struct i2c_client *client,
 	return 0;
 }
 
-void tmp421_probe_child_from_dt(struct i2c_client *client,
+static void tmp421_probe_child_from_dt(struct i2c_client *client,
 				struct device_node *child,
 				struct tmp421_data *data)
 
@@ -316,7 +316,7 @@ void tmp421_probe_child_from_dt(struct i2c_client *client,
 
 }
 
-void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
+static void 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;

[PATCH 5/8] 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 | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index a1dba1d405ee..a41d7935acb9 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 disabled;
 	s16 temp;
 };
 
@@ -125,9 +126,8 @@ static int temp_from_u16(u16 reg)
 	return (temp * 1000 + 128) / 256;
 }
 
-static struct tmp421_data *tmp421_update_device(struct device *dev)
+static void tmp421_update_device(struct tmp421_data *data)
 {
-	struct tmp421_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
 	int i;
 
@@ -149,14 +149,17 @@ static struct tmp421_data *tmp421_update_device(struct device *dev)
 	}
 
 	mutex_unlock(&data->update_lock);
-
-	return data;
 }
 
 static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
 		       u32 attr, int channel, long *val)
 {
-	struct tmp421_data *tmp421 = tmp421_update_device(dev);
+	struct tmp421_data *tmp421 = dev_get_drvdata(dev);
+
+	if (tmp421->channel[channel].disabled)
+		return -ENODATA;
+
+	tmp421_update_device(tmp421);
 
 	switch (attr) {
 	case hwmon_temp_input:
@@ -314,6 +317,10 @@ void tmp421_probe_child_from_dt(struct i2c_client *client,
 	if (data->channel[i].label)
 		data->temp_config[i] |= HWMON_T_LABEL;
 
+	if (!of_device_is_available(child)) {
+		data->channel[i].disabled = true;
+		return;
+	}
 }
 
 void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
-- 
2.31.1

Re: [PATCH 5/8] hwmon: (tmp421) support disabling channels from DT

[Guenter Roeck]

On 9/7/21 6:43 AM, Krzysztof Adamski wrote:
> 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.
> 

If channels can be disabled via DT, the respective sysfs attribute (tempX_enable)
should also be supported.

> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
>   drivers/hwmon/tmp421.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> index a1dba1d405ee..a41d7935acb9 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 disabled;
>   	s16 temp;
>   };
>   
> @@ -125,9 +126,8 @@ static int temp_from_u16(u16 reg)
>   	return (temp * 1000 + 128) / 256;
>   }
>   
> -static struct tmp421_data *tmp421_update_device(struct device *dev)
> +static void tmp421_update_device(struct tmp421_data *data)
>   {
> -	struct tmp421_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
>   	int i;
>   
> @@ -149,14 +149,17 @@ static struct tmp421_data *tmp421_update_device(struct device *dev)
>   	}
>   
>   	mutex_unlock(&data->update_lock);
> -
> -	return data;
>   }
>   
>   static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
>   		       u32 attr, int channel, long *val)
>   {
> -	struct tmp421_data *tmp421 = tmp421_update_device(dev);
> +	struct tmp421_data *tmp421 = dev_get_drvdata(dev);
> +
> +	if (tmp421->channel[channel].disabled)
> +		return -ENODATA;
> +
> +	tmp421_update_device(tmp421);
>   
>   	switch (attr) {
>   	case hwmon_temp_input:
> @@ -314,6 +317,10 @@ void tmp421_probe_child_from_dt(struct i2c_client *client,
>   	if (data->channel[i].label)
>   		data->temp_config[i] |= HWMON_T_LABEL;
>   
> +	if (!of_device_is_available(child)) {
> +		data->channel[i].disabled = true;
> +		return;
> +	}
>   }
>   
>   void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
> 

[PATCH 6/8] 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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index a41d7935acb9..90c6b094785e 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
 
@@ -302,6 +303,7 @@ void 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);
@@ -321,6 +323,21 @@ void tmp421_probe_child_from_dt(struct i2c_client *client,
 		data->channel[i].disabled = true;
 		return;
 	}
+
+	if (i == 0)
+		return; /* 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);
+		else
+			i2c_smbus_write_byte_data(client,
+						  TMP421_N_FACTOR_REG_1 + i - 1,
+						  val);
+	}
+
 }
 
 void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
-- 
2.31.1

Re: [PATCH 6/8] hwmon: (tmp421) support specifying n-factor via DT

[Guenter Roeck]

On 9/7/21 6:45 AM, Krzysztof Adamski wrote:
> 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 | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> index a41d7935acb9..90c6b094785e 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
>   
> @@ -302,6 +303,7 @@ void 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);
> @@ -321,6 +323,21 @@ void tmp421_probe_child_from_dt(struct i2c_client *client,
>   		data->channel[i].disabled = true;
>   		return;
>   	}
> +
> +	if (i == 0)
> +		return; /* 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);

This should report an error to the caller.

Guenter

> +		else
> +			i2c_smbus_write_byte_data(client,
> +						  TMP421_N_FACTOR_REG_1 + i - 1,
> +						  val);
> +	}
> +
>   }
>   
>   void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
> 

[PATCH 7/8] 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 | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index 90c6b094785e..cec25fb1c771 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -33,6 +33,8 @@ 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_CONVERSION_RATE_REG		0x0B
 #define TMP421_N_FACTOR_REG_1			0x21
 #define TMP421_MANUFACTURER_ID_REG		0xFE
@@ -351,6 +353,25 @@ void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
 	}
 }
 
+void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
+{
+	int err;
+	int cfg = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_2);
+
+	if (cfg < 0) {
+		dev_err(&client->dev,
+			"error reading register, can't disable channels\n");
+		return;
+	}
+
+	cfg &= ~mask;
+
+	err = i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_2, cfg);
+	if (err < 0)
+		dev_err(&client->dev,
+			"error writing register, can't disable channels\n");
+}
+
 static const struct hwmon_ops tmp421_ops = {
 	.is_visible = tmp421_is_visible,
 	.read = tmp421_read,
@@ -363,6 +384,7 @@ static int tmp421_probe(struct i2c_client *client)
 	struct device *hwmon_dev;
 	struct tmp421_data *data;
 	int i, err;
+	u8 disable = 0;
 
 	data = devm_kzalloc(dev, sizeof(struct tmp421_data), GFP_KERNEL);
 	if (!data)
@@ -380,11 +402,18 @@ static int tmp421_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
-	for (i = 0; i < data->channels; i++)
-		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
-
 	tmp421_probe_from_dt(client, data);
 
+	for (i = 0; i < data->channels; i++) {
+		data->temp_config[i] |= HWMON_T_INPUT | HWMON_T_FAULT;
+		if (data->channel[i].disabled)
+			disable |= TMP421_CONFIG_REG_REN(i);
+
+	}
+
+	if (disable)
+		tmp421_disable_channels(client, disable);
+
 	data->chip.ops = &tmp421_ops;
 	data->chip.info = data->info;
 
-- 
2.31.1

Re: [PATCH 7/8] hwmon: (tmp421) really disable channels

[Guenter Roeck]

On 9/7/21 6:46 AM, Krzysztof Adamski wrote:
> 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 | 35 ++++++++++++++++++++++++++++++++---
>   1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> index 90c6b094785e..cec25fb1c771 100644
> --- a/drivers/hwmon/tmp421.c
> +++ b/drivers/hwmon/tmp421.c
> @@ -33,6 +33,8 @@ 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_CONVERSION_RATE_REG		0x0B
>   #define TMP421_N_FACTOR_REG_1			0x21
>   #define TMP421_MANUFACTURER_ID_REG		0xFE
> @@ -351,6 +353,25 @@ void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
>   	}
>   }
>   
> +void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
> +{
> +	int err;
> +	int cfg = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_2);
> +
> +	if (cfg < 0) {
> +		dev_err(&client->dev,
> +			"error reading register, can't disable channels\n");
> +		return;
> +	}
> +
> +	cfg &= ~mask;
> +
> +	err = i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_2, cfg);
> +	if (err < 0)
> +		dev_err(&client->dev,
> +			"error writing register, can't disable channels\n");
> +}
> +
>   static const struct hwmon_ops tmp421_ops = {
>   	.is_visible = tmp421_is_visible,
>   	.read = tmp421_read,
> @@ -363,6 +384,7 @@ static int tmp421_probe(struct i2c_client *client)
>   	struct device *hwmon_dev;
>   	struct tmp421_data *data;
>   	int i, err;
> +	u8 disable = 0;
>   
>   	data = devm_kzalloc(dev, sizeof(struct tmp421_data), GFP_KERNEL);
>   	if (!data)
> @@ -380,11 +402,18 @@ static int tmp421_probe(struct i2c_client *client)
>   	if (err)
>   		return err;
>   
> -	for (i = 0; i < data->channels; i++)
> -		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
> -
>   	tmp421_probe_from_dt(client, data);
>   
> +	for (i = 0; i < data->channels; i++) {
> +		data->temp_config[i] |= HWMON_T_INPUT | HWMON_T_FAULT;
> +		if (data->channel[i].disabled)
> +			disable |= TMP421_CONFIG_REG_REN(i);
> +
> +	}
> +
> +	if (disable)
> +		tmp421_disable_channels(client, disable);
> +

This doesn't take into account that channels may already be disabled
by the BIOS/ROMMON. The code will have to set the channel status explicitly
for all channels if configured through dt, or read it from the chip
otherwise. Also, as mentioned, the sysfs attribute should be supported
as well (meaning it should also be possible to enable a channel).

Guenter

>   	data->chip.ops = &tmp421_ops;
>   	data->chip.info = data->info;
>   
> 

Re: [PATCH 7/8] hwmon: (tmp421) really disable channels

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3728 bytes --]

Hi Krzysztof,

I love your patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on robh/for-next v5.14 next-20210907]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: mips-randconfig-c004-20210907 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9c476172b93367d2cb88d7d3f4b1b5b456fa6020)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/56fe3c49ca67e9ea9c4f8a40438b5adc58c417f1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724
        git checkout 56fe3c49ca67e9ea9c4f8a40438b5adc58c417f1
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/hwmon/tmp421.c:301:6: warning: no previous prototype for function 'tmp421_probe_child_from_dt' [-Wmissing-prototypes]
   void tmp421_probe_child_from_dt(struct i2c_client *client,
        ^
   drivers/hwmon/tmp421.c:301:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void tmp421_probe_child_from_dt(struct i2c_client *client,
   ^
   static 
   drivers/hwmon/tmp421.c:345:6: warning: no previous prototype for function 'tmp421_probe_from_dt' [-Wmissing-prototypes]
   void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
        ^
   drivers/hwmon/tmp421.c:345:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
   ^
   static 
>> drivers/hwmon/tmp421.c:356:6: warning: no previous prototype for function 'tmp421_disable_channels' [-Wmissing-prototypes]
   void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
        ^
   drivers/hwmon/tmp421.c:356:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
   ^
   static 
   3 warnings generated.


vim +/tmp421_disable_channels +356 drivers/hwmon/tmp421.c

   355	
 > 356	void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
   357	{
   358		int err;
   359		int cfg = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_2);
   360	
   361		if (cfg < 0) {
   362			dev_err(&client->dev,
   363				"error reading register, can't disable channels\n");
   364			return;
   365		}
   366	
   367		cfg &= ~mask;
   368	
   369		err = i2c_smbus_write_byte_data(client, TMP421_CONFIG_REG_2, cfg);
   370		if (err < 0)
   371			dev_err(&client->dev,
   372				"error writing register, can't disable channels\n");
   373	}
   374	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32497 bytes --]

Re: [PATCH 7/8] hwmon: (tmp421) really disable channels

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

Hi Krzysztof,

I love your patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on robh/for-next v5.14 next-20210909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-s001-20210908 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/56fe3c49ca67e9ea9c4f8a40438b5adc58c417f1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Adamski/Add-per-channel-properies-support-in-tmp421/20210907-214724
        git checkout 56fe3c49ca67e9ea9c4f8a40438b5adc58c417f1
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hwmon/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
   drivers/hwmon/tmp421.c:301:6: sparse: sparse: symbol 'tmp421_probe_child_from_dt' was not declared. Should it be static?
   drivers/hwmon/tmp421.c:345:6: sparse: sparse: symbol 'tmp421_probe_from_dt' was not declared. Should it be static?
>> drivers/hwmon/tmp421.c:356:6: sparse: sparse: symbol 'tmp421_disable_channels' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27564 bytes --]

[RFC PATCH] hwmon: tmp421_disable_channels() can be static

[kernel test robot]

drivers/hwmon/tmp421.c:356:6: warning: symbol 'tmp421_disable_channels' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 tmp421.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
index cec25fb1c7719..162798f7491d9 100644
--- a/drivers/hwmon/tmp421.c
+++ b/drivers/hwmon/tmp421.c
@@ -353,7 +353,7 @@ void tmp421_probe_from_dt(struct i2c_client *client, struct tmp421_data *data)
 	}
 }
 
-void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
+static void tmp421_disable_channels(struct i2c_client *client, uint8_t mask)
 {
 	int err;
 	int cfg = i2c_smbus_read_byte_data(client, TMP421_CONFIG_REG_2);

[PATCH 8/8] 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/tmp421.yaml     | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
index 53940e146ee6..56085fdf1b57 100644
--- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml
+++ b/Documentation/devicetree/bindings/hwmon/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 8/8] dt-bindings: hwmon: allow specifying channels for tmp421

[Guenter Roeck]

On 9/7/21 6:46 AM, 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>

Up to Rob to decide, but it seems to me that can be squashed with the other
dt patch in the series (which on its own doesn't really add much value).

Guenter

> ---
>   .../devicetree/bindings/hwmon/tmp421.yaml     | 66 +++++++++++++++++++
>   1 file changed, 66 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> index 53940e146ee6..56085fdf1b57 100644
> --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/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";
> +        };
> +      };
> +    };
> 

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

[Krzysztof Adamski]

Dnia Tue, Sep 07, 2021 at 08:46:53AM -0700, Guenter Roeck napisał(a):
>On 9/7/21 6:46 AM, 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>
>
>Up to Rob to decide, but it seems to me that can be squashed with the other
>dt patch in the series (which on its own doesn't really add much value).
>

My intention was, I guess, to clearly differentiate between what is the
existing state of bindings and what I propose as an addition, so that it
is not required to read the rest of the patches to review my proposal
from DT perspective.
I can squash the two patches, though, so let me know about the decision,
Rob.

Krzysztof

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

[Rob Herring]

On Tue, Sep 07, 2021 at 03:46:14PM +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/tmp421.yaml     | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)

I'd keep this separate...

> 
> diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> index 53940e146ee6..56085fdf1b57 100644
> --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/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:

ti,n-factor

Needs a type reference too.

> +        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 8/8] dt-bindings: hwmon: allow specifying channels for tmp421

[Guenter Roeck]

On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote:
> On Tue, Sep 07, 2021 at 03:46:14PM +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/tmp421.yaml     | 66 +++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> 
> I'd keep this separate...
> 
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > index 53940e146ee6..56085fdf1b57 100644
> > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > +++ b/Documentation/devicetree/bindings/hwmon/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:
> 
> ti,n-factor

n-factor isn't just supported by TI sensors, though it isn't always called
n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
also refer to the factor as "N" in the datasheet.

So question is if we make this ti,n-factor and maxim,n-factor, or if we make
it generic and define some kind of generic units. Thoughts ? My personal
preference would be a generic definition, but is not a strong preference.

In regard to units, the n-factor is, as the name says, a factor. Default
value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
it is 0.706542 to 1.747977. So the scondary question is if the value
written should be the register value (as proposed here) or the absolute
factor (eg in micro-units).

> 
> Needs a type reference too.
> 
> > +        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";
> > +        };

In the context or other sensors, question here is if we can make the
bindings generic. We have been discussing this for NCT7802Y. The main
question for me is how to handle different sensor types. TMP421 is
easy because it only has one type of sensors, but there are other
devices which also have, for example, voltage and/or current sensors.
NCT7802 is an example for that. We just had a set of bindings for that
chip proposed at
https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/

Would it be possible to determine a generic scheme that works for all
chips ? I can see two problems:
- How to express sensor types. The NCT7802 submission proposes another level
  of indirection, ie

  temperature-sensors {
> > +
> > +        input@1 {
> > +          reg = <0x1>;
> > +          n-factor = <0x0>;
> > +          label = "somelabel";
> > +        };
> > +
> > +        input@2 {
> > +          reg = <0x2>;
> > +          status = "disabled";
> > +        };
> > +      };
> > +    };
    };

The second question is how to express sensor index. One option is the solution
suggested here, ie to use reg=<> as sensor index. The second is the solution
suggested in the 7802 bindings, where the (chip specific) name is used as
sensor index.

+            temperature-sensors {
+                ltd {
+                  status = "disabled";
+                };
+
+                rtd1 {
+                  status = "okay";
+                  type = <4> /* thermistor */;
+                };
+            };

I personally don't have a strong opinion either way, but I would like to see
a single solution for all sensor chips.

Rob, do you have a preference ?

Thanks,
Guenter

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

[Rob Herring]

On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote:
> > On Tue, Sep 07, 2021 at 03:46:14PM +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/tmp421.yaml     | 66 +++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> >
> > I'd keep this separate...
> >
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > index 53940e146ee6..56085fdf1b57 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > +++ b/Documentation/devicetree/bindings/hwmon/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:
> >
> > ti,n-factor
>
> n-factor isn't just supported by TI sensors, though it isn't always called
> n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
> also refer to the factor as "N" in the datasheet.
>
> So question is if we make this ti,n-factor and maxim,n-factor, or if we make
> it generic and define some kind of generic units. Thoughts ? My personal
> preference would be a generic definition, but is not a strong preference.

generic if the units are generic. Though if the register value is
opaque to s/w, then maybe register value is fine.

> In regard to units, the n-factor is, as the name says, a factor. Default
> value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
> it is 0.706542 to 1.747977. So the scondary question is if the value
> written should be the register value (as proposed here) or the absolute
> factor (eg in micro-units).

A range, but the register value can only be 0 or 1?

>
> >
> > Needs a type reference too.
> >
> > > +        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";
> > > +        };
>
> In the context or other sensors, question here is if we can make the
> bindings generic. We have been discussing this for NCT7802Y. The main
> question for me is how to handle different sensor types. TMP421 is
> easy because it only has one type of sensors, but there are other
> devices which also have, for example, voltage and/or current sensors.
> NCT7802 is an example for that. We just had a set of bindings for that
> chip proposed at
> https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/
>
> Would it be possible to determine a generic scheme that works for all
> chips ? I can see two problems:
> - How to express sensor types. The NCT7802 submission proposes another level
>   of indirection, ie
>
>   temperature-sensors {
> > > +
> > > +        input@1 {
> > > +          reg = <0x1>;
> > > +          n-factor = <0x0>;
> > > +          label = "somelabel";
> > > +        };
> > > +
> > > +        input@2 {
> > > +          reg = <0x2>;
> > > +          status = "disabled";
> > > +        };
> > > +      };
> > > +    };
>     };

I think the function should be within the node. Otherwise, the
addressing becomes weird (e.g. input@3 is under current-sensors or
something) with seemingly separate address spaces.

> The second question is how to express sensor index. One option is the solution
> suggested here, ie to use reg=<> as sensor index. The second is the solution
> suggested in the 7802 bindings, where the (chip specific) name is used as
> sensor index.
>
> +            temperature-sensors {
> +                ltd {
> +                  status = "disabled";
> +                };
> +
> +                rtd1 {
> +                  status = "okay";
> +                  type = <4> /* thermistor */;

'type' is a bit generic. We don't want the same property name to
possibly have multiple definitions.

> +                };
> +            };
>
> I personally don't have a strong opinion either way, but I would like to see
> a single solution for all sensor chips.
>
> Rob, do you have a preference ?

If it is how you address an instance of something which seems to be
the case here, then 'reg' should be used.

Rob

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

[Guenter Roeck]

On Tue, Sep 21, 2021 at 02:06:18PM -0500, Rob Herring wrote:
> On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote:
> > > On Tue, Sep 07, 2021 at 03:46:14PM +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/tmp421.yaml     | 66 +++++++++++++++++++
> > > >  1 file changed, 66 insertions(+)
> > >
> > > I'd keep this separate...
> > >
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > > index 53940e146ee6..56085fdf1b57 100644
> > > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > > +++ b/Documentation/devicetree/bindings/hwmon/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:
> > >
> > > ti,n-factor
> >
> > n-factor isn't just supported by TI sensors, though it isn't always called
> > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
> > also refer to the factor as "N" in the datasheet.
> >
> > So question is if we make this ti,n-factor and maxim,n-factor, or if we make
> > it generic and define some kind of generic units. Thoughts ? My personal
> > preference would be a generic definition, but is not a strong preference.
> 
> generic if the units are generic. Though if the register value is
> opaque to s/w, then maybe register value is fine.
> 
> > In regard to units, the n-factor is, as the name says, a factor. Default
> > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
> > it is 0.706542 to 1.747977. So the scondary question is if the value
> > written should be the register value (as proposed here) or the absolute
> > factor (eg in micro-units).
> 
> A range, but the register value can only be 0 or 1?
> 
No, register values are 0x0 .. 0x1f for MAX6581, and 0x0 .. 0xff for TMP421.

> >
> > >
> > > Needs a type reference too.
> > >
> > > > +        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";
> > > > +        };
> >
> > In the context or other sensors, question here is if we can make the
> > bindings generic. We have been discussing this for NCT7802Y. The main
> > question for me is how to handle different sensor types. TMP421 is
> > easy because it only has one type of sensors, but there are other
> > devices which also have, for example, voltage and/or current sensors.
> > NCT7802 is an example for that. We just had a set of bindings for that
> > chip proposed at
> > https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/
> >
> > Would it be possible to determine a generic scheme that works for all
> > chips ? I can see two problems:
> > - How to express sensor types. The NCT7802 submission proposes another level
> >   of indirection, ie
> >
> >   temperature-sensors {
> > > > +
> > > > +        input@1 {
> > > > +          reg = <0x1>;
> > > > +          n-factor = <0x0>;
> > > > +          label = "somelabel";
> > > > +        };
> > > > +
> > > > +        input@2 {
> > > > +          reg = <0x2>;
> > > > +          status = "disabled";
> > > > +        };
> > > > +      };
> > > > +    };
> >     };
> 
> I think the function should be within the node. Otherwise, the
> addressing becomes weird (e.g. input@3 is under current-sensors or
> something) with seemingly separate address spaces.
> 

Sorry, can you translate that for a DT non-expert ? Or, in other words,
how would / should one express a chip with sets of, say, current-sensors,
voltage sensors, and temperature sensors. Each would have a different
number of channels and different parameters.

> > The second question is how to express sensor index. One option is the solution
> > suggested here, ie to use reg=<> as sensor index. The second is the solution
> > suggested in the 7802 bindings, where the (chip specific) name is used as
> > sensor index.
> >
> > +            temperature-sensors {
> > +                ltd {
> > +                  status = "disabled";
> > +                };
> > +
> > +                rtd1 {
> > +                  status = "okay";
> > +                  type = <4> /* thermistor */;
> 
> 'type' is a bit generic. We don't want the same property name to
> possibly have multiple definitions.
> 
How about sensor-type ?

> > +                };
> > +            };
> >
> > I personally don't have a strong opinion either way, but I would like to see
> > a single solution for all sensor chips.
> >
> > Rob, do you have a preference ?
> 
> If it is how you address an instance of something which seems to be
> the case here, then 'reg' should be used.
> 
Ok.

Thanks,
Guenter

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

[Oskar Senft]

Hi

> > > +            temperature-sensors {
> > > +                ltd {
> > > +                  status = "disabled";
> > > +                };
> > > +
> > > +                rtd1 {
> > > +                  status = "okay";
> > > +                  type = <4> /* thermistor */;
> >
> > 'type' is a bit generic. We don't want the same property name to
> > possibly have multiple definitions.
> >
> How about sensor-type ?

In the datasheet this is called "mode". I called it "type" since it's
tempX_type in sysfs and I wanted to stay in sync with that.

> > If it is how you address an instance of something which seems to be
> > the case here, then 'reg' should be used.
> >

The reason I didn't do that is because you'd effectively have to
duplicate the ID in both the name (e.g. sensor@1) and the reg property
(e.g. reg = <1>). But maybe that's just what is is in device tree, I
can live with that.

However, we'd also have to find out whether the "local" sensor ("LTD")
would simply be #4, as it is in sysfs today. I'd also be ok with that
as it would keep sysfs and device tree "in sync" wrt. naming.

Thanks
Oskar.

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

[Oskar Senft]

Resend to Rob's correct email address. I'm sorry.

> > > + temperature-sensors {
> > > + ltd {
> > > + status = "disabled";
> > > + };
> > > +
> > > + rtd1 {
> > > + status = "okay";
> > > + type = <4> /* thermistor */;
> >
> > 'type' is a bit generic. We don't want the same property name to
> > possibly have multiple definitions.
> >
> How about sensor-type ?

In the datasheet this is called "mode". I called it "type" since it's
tempX_type in sysfs and I wanted to stay in sync with that.

> > If it is how you address an instance of something which seems to be
> > the case here, then 'reg' should be used.
> >

The reason I didn't do that is because you'd effectively have to
duplicate the ID in both the name (e.g. sensor@1) and the reg property
(e.g. reg = <1>). But maybe that's just what is is in device tree, I
can live with that.

However, we'd also have to find out whether the "local" sensor ("LTD")
would simply be #4, as it is in sysfs today. I'd also be ok with that
as it would keep sysfs and device tree "in sync" wrt. naming.

Thanks
Oskar.

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

[Rob Herring]

On Tue, Sep 21, 2021 at 3:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Sep 21, 2021 at 02:06:18PM -0500, Rob Herring wrote:
> > On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote:
> > > > On Tue, Sep 07, 2021 at 03:46:14PM +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/tmp421.yaml     | 66 +++++++++++++++++++
> > > > >  1 file changed, 66 insertions(+)
> > > >
> > > > I'd keep this separate...
> > > >
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > > > index 53940e146ee6..56085fdf1b57 100644
> > > > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > > > +++ b/Documentation/devicetree/bindings/hwmon/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:
> > > >
> > > > ti,n-factor
> > >
> > > n-factor isn't just supported by TI sensors, though it isn't always called
> > > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
> > > also refer to the factor as "N" in the datasheet.
> > >
> > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make
> > > it generic and define some kind of generic units. Thoughts ? My personal
> > > preference would be a generic definition, but is not a strong preference.
> >
> > generic if the units are generic. Though if the register value is
> > opaque to s/w, then maybe register value is fine.
> >
> > > In regard to units, the n-factor is, as the name says, a factor. Default
> > > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
> > > it is 0.706542 to 1.747977. So the scondary question is if the value
> > > written should be the register value (as proposed here) or the absolute
> > > factor (eg in micro-units).
> >
> > A range, but the register value can only be 0 or 1?
> >
> No, register values are 0x0 .. 0x1f for MAX6581, and 0x0 .. 0xff for TMP421.

Okay, then the schema below is wrong.

> > > > Needs a type reference too.
> > > >
> > > > > +        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";
> > > > > +        };
> > >
> > > In the context or other sensors, question here is if we can make the
> > > bindings generic. We have been discussing this for NCT7802Y. The main
> > > question for me is how to handle different sensor types. TMP421 is
> > > easy because it only has one type of sensors, but there are other
> > > devices which also have, for example, voltage and/or current sensors.
> > > NCT7802 is an example for that. We just had a set of bindings for that
> > > chip proposed at
> > > https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/
> > >
> > > Would it be possible to determine a generic scheme that works for all
> > > chips ? I can see two problems:
> > > - How to express sensor types. The NCT7802 submission proposes another level
> > >   of indirection, ie
> > >
> > >   temperature-sensors {
> > > > > +
> > > > > +        input@1 {
> > > > > +          reg = <0x1>;
> > > > > +          n-factor = <0x0>;
> > > > > +          label = "somelabel";
> > > > > +        };
> > > > > +
> > > > > +        input@2 {
> > > > > +          reg = <0x2>;
> > > > > +          status = "disabled";
> > > > > +        };
> > > > > +      };
> > > > > +    };
> > >     };
> >
> > I think the function should be within the node. Otherwise, the
> > addressing becomes weird (e.g. input@3 is under current-sensors or
> > something) with seemingly separate address spaces.
> >
>
> Sorry, can you translate that for a DT non-expert ? Or, in other words,
> how would / should one express a chip with sets of, say, current-sensors,
> voltage sensors, and temperature sensors. Each would have a different
> number of channels and different parameters.

If each kind of sensor is a different number space (e.g. 0-2), then
how you have it with 2 levels of nodes is appropriate. If you only
have one set of channel or input numbers, then they should all have
the same parent node. So is it current sensors 0,1,2 and temperature
sensors 0,1,2, or just input channels 0,1,2,3,4,5?

> > > The second question is how to express sensor index. One option is the solution
> > > suggested here, ie to use reg=<> as sensor index. The second is the solution
> > > suggested in the 7802 bindings, where the (chip specific) name is used as
> > > sensor index.
> > >
> > > +            temperature-sensors {
> > > +                ltd {
> > > +                  status = "disabled";
> > > +                };
> > > +
> > > +                rtd1 {
> > > +                  status = "okay";
> > > +                  type = <4> /* thermistor */;
> >
> > 'type' is a bit generic. We don't want the same property name to
> > possibly have multiple definitions.
> >
> How about sensor-type ?

Sure. And you are going to define a common set of type numbers?

Rob

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

[Guenter Roeck]

On Thu, Sep 23, 2021 at 10:30:59AM -0500, Rob Herring wrote:
> On Tue, Sep 21, 2021 at 3:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Tue, Sep 21, 2021 at 02:06:18PM -0500, Rob Herring wrote:
> > > On Tue, Sep 21, 2021 at 7:58 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > > >
> > > > On Mon, Sep 20, 2021 at 05:24:09PM -0500, Rob Herring wrote:
> > > > > On Tue, Sep 07, 2021 at 03:46:14PM +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/tmp421.yaml     | 66 +++++++++++++++++++
> > > > > >  1 file changed, 66 insertions(+)
> > > > >
> > > > > I'd keep this separate...
> > > > >
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/tmp421.yaml b/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > > > > index 53940e146ee6..56085fdf1b57 100644
> > > > > > --- a/Documentation/devicetree/bindings/hwmon/tmp421.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/hwmon/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:
> > > > >
> > > > > ti,n-factor
> > > >
> > > > n-factor isn't just supported by TI sensors, though it isn't always called
> > > > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
> > > > also refer to the factor as "N" in the datasheet.
> > > >
> > > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make
> > > > it generic and define some kind of generic units. Thoughts ? My personal
> > > > preference would be a generic definition, but is not a strong preference.
> > >
> > > generic if the units are generic. Though if the register value is
> > > opaque to s/w, then maybe register value is fine.
> > >
> > > > In regard to units, the n-factor is, as the name says, a factor. Default
> > > > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
> > > > it is 0.706542 to 1.747977. So the scondary question is if the value
> > > > written should be the register value (as proposed here) or the absolute
> > > > factor (eg in micro-units).
> > >
> > > A range, but the register value can only be 0 or 1?
> > >
> > No, register values are 0x0 .. 0x1f for MAX6581, and 0x0 .. 0xff for TMP421.
> 
> Okay, then the schema below is wrong.
> 
> > > > > Needs a type reference too.
> > > > >
> > > > > > +        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";
> > > > > > +        };
> > > >
> > > > In the context or other sensors, question here is if we can make the
> > > > bindings generic. We have been discussing this for NCT7802Y. The main
> > > > question for me is how to handle different sensor types. TMP421 is
> > > > easy because it only has one type of sensors, but there are other
> > > > devices which also have, for example, voltage and/or current sensors.
> > > > NCT7802 is an example for that. We just had a set of bindings for that
> > > > chip proposed at
> > > > https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/
> > > >
> > > > Would it be possible to determine a generic scheme that works for all
> > > > chips ? I can see two problems:
> > > > - How to express sensor types. The NCT7802 submission proposes another level
> > > >   of indirection, ie
> > > >
> > > >   temperature-sensors {
> > > > > > +
> > > > > > +        input@1 {
> > > > > > +          reg = <0x1>;
> > > > > > +          n-factor = <0x0>;
> > > > > > +          label = "somelabel";
> > > > > > +        };
> > > > > > +
> > > > > > +        input@2 {
> > > > > > +          reg = <0x2>;
> > > > > > +          status = "disabled";
> > > > > > +        };
> > > > > > +      };
> > > > > > +    };
> > > >     };
> > >
> > > I think the function should be within the node. Otherwise, the
> > > addressing becomes weird (e.g. input@3 is under current-sensors or
> > > something) with seemingly separate address spaces.
> > >
> >
> > Sorry, can you translate that for a DT non-expert ? Or, in other words,
> > how would / should one express a chip with sets of, say, current-sensors,
> > voltage sensors, and temperature sensors. Each would have a different
> > number of channels and different parameters.
> 
> If each kind of sensor is a different number space (e.g. 0-2), then
> how you have it with 2 levels of nodes is appropriate. If you only
> have one set of channel or input numbers, then they should all have
> the same parent node. So is it current sensors 0,1,2 and temperature
> sensors 0,1,2, or just input channels 0,1,2,3,4,5?
> 

Each sensor type has its own number space.

> > > > The second question is how to express sensor index. One option is the solution
> > > > suggested here, ie to use reg=<> as sensor index. The second is the solution
> > > > suggested in the 7802 bindings, where the (chip specific) name is used as
> > > > sensor index.
> > > >
> > > > +            temperature-sensors {
> > > > +                ltd {
> > > > +                  status = "disabled";
> > > > +                };
> > > > +
> > > > +                rtd1 {
> > > > +                  status = "okay";
> > > > +                  type = <4> /* thermistor */;
> > >
> > > 'type' is a bit generic. We don't want the same property name to
> > > possibly have multiple definitions.
> > >
> > How about sensor-type ?
> 
> Sure. And you are going to define a common set of type numbers?
> 
I guess we would have to.

Guenter

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

[Krzysztof Adamski]

Dnia Thu, Sep 23, 2021 at 05:29:51PM -0700, Guenter Roeck napisał(a):
>> If each kind of sensor is a different number space (e.g. 0-2), then
>> how you have it with 2 levels of nodes is appropriate. If you only
>> have one set of channel or input numbers, then they should all have
>> the same parent node. So is it current sensors 0,1,2 and temperature
>> sensors 0,1,2, or just input channels 0,1,2,3,4,5?
>>
>
>Each sensor type has its own number space.
>

But many sensors will have only one type of channels - like several
temperature sensors and nothing else. Like several temperature channels
on a temperature sensor, or several fans on a fan controller.

In such cases, we already define them with 1-level structure, like:
- npcm750-pwm-fan
- aspeed-pwm-tacho
- ina3221

In many cases the channels are "shared" - we have 3 voltage, 3 current and 3
power sensors but in fact they are not separate sensors but 3 channels
each able to measure 3 different things and they may share some common
properties in each channel (so current, voltage and power may be
calculated bases on the same shunt resistor or correction factor). An
example being adi,ltc2992.  In those cases it doesn't make sense to have
two levels as how would you describe the shared parent? Call it generic
"channels"?

So maybe it makes sense to have 2 levels for complex devices that can
measure several independent entities or for devices which do not have a
clear concept of enumerated "channels" or "inputs", but we could skip it
for most others? After all, what is the benefit of having this
additional level if all we have is something like:

temperature-sensors {
     temperature1 {
	};

	temperature2 {
	};

	temperature3 {
	};
};

For most devices having an "index" or "reg" makes much more sense so:
temperature@1, or channel@1 feels like a more natural way to describe
them.

In any case, I'm quite confused right now on what would be the
conclusion of this discussion. How would you like the DT for TMP421 to
look like, after all?

As a side note, should the description of the tmp421 bindings be in
tmp421.yaml file (as it is in current patchset), or should it be
actually called "ti,tmp421.yaml"?

Krzysztof

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

[Guenter Roeck]

On Fri, Sep 24, 2021 at 09:53:16AM +0200, Krzysztof Adamski wrote:
> Dnia Thu, Sep 23, 2021 at 05:29:51PM -0700, Guenter Roeck napisał(a):
> > > If each kind of sensor is a different number space (e.g. 0-2), then
> > > how you have it with 2 levels of nodes is appropriate. If you only
> > > have one set of channel or input numbers, then they should all have
> > > the same parent node. So is it current sensors 0,1,2 and temperature
> > > sensors 0,1,2, or just input channels 0,1,2,3,4,5?
> > > 
> > 
> > Each sensor type has its own number space.
> > 
> 
> But many sensors will have only one type of channels - like several
> temperature sensors and nothing else. Like several temperature channels
> on a temperature sensor, or several fans on a fan controller.
> 
> In such cases, we already define them with 1-level structure, like:
> - npcm750-pwm-fan
> - aspeed-pwm-tacho
> - ina3221
> 
> In many cases the channels are "shared" - we have 3 voltage, 3 current and 3
> power sensors but in fact they are not separate sensors but 3 channels
> each able to measure 3 different things and they may share some common
> properties in each channel (so current, voltage and power may be
> calculated bases on the same shunt resistor or correction factor). An
> example being adi,ltc2992.  In those cases it doesn't make sense to have
> two levels as how would you describe the shared parent? Call it generic
> "channels"?
> 
> So maybe it makes sense to have 2 levels for complex devices that can
> measure several independent entities or for devices which do not have a
> clear concept of enumerated "channels" or "inputs", but we could skip it
> for most others? After all, what is the benefit of having this
> additional level if all we have is something like:
> 
> temperature-sensors {
>     temperature1 {
> 	};
> 
> 	temperature2 {
> 	};
> 
> 	temperature3 {
> 	};
> };

I see your point. I think it would make sense to only use the two-level
approach for devices with more than one type of sensors.

Thanks,
Guenter

> 
> For most devices having an "index" or "reg" makes much more sense so:
> temperature@1, or channel@1 feels like a more natural way to describe
> them.
> 
> In any case, I'm quite confused right now on what would be the
> conclusion of this discussion. How would you like the DT for TMP421 to
> look like, after all?
> 
> As a side note, should the description of the tmp421 bindings be in
> tmp421.yaml file (as it is in current patchset), or should it be
> actually called "ti,tmp421.yaml"?
> 
> Krzysztof

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

[Oskar Senft]

> > In many cases the channels are "shared" - we have 3 voltage, 3 current and 3
> > power sensors but in fact they are not separate sensors but 3 channels
> > each able to measure 3 different things and they may share some common
> > properties in each channel (so current, voltage and power may be
> > calculated bases on the same shunt resistor or correction factor). An
> > example being adi,ltc2992.  In those cases it doesn't make sense to have
> > two levels as how would you describe the shared parent? Call it generic
> > "channels"?

So in that case (e.g. for the nct7802, see [1]) do we want just
1-level, maybe like this:

nct7802@28 {
    compatible = "nuvoton,nct7802";
    reg = <0x28>;

    sensor@1 { /* RTD1 */
         reg = <0x1>;
         status = "okay";
         mode = "thermistor"; /* Any of "thermistor", "thermal-diode",
"voltage" */
    };

    sensor@2 { /* RTD2 */
         reg = <0x2>;
         status = "okay";
         mode = "thermal-diode"; /* Any of "thermistor",
"thermal-diode", "voltage" */
    };

    sensor@3 { /* RTD3 */
         reg = <0x3>;
         status = "okay";
         mode = "voltage"; /* Any of "thermistor", "voltage" */
    };

    sensor@4 { /* LTD */
        reg = <0x4>; /* using the same number as in sysfs */
        status = "okay";
        /* No mode configuration for LTD */
    };
};

In this example, RTD1, RTD2 and LTD would be temperature sensors and
RTD3 would be a voltage sensor.

Would that make more sense? Is the use of strings acceptable?

Thanks
Oskar.

[1] https://lore.kernel.org/all/20210921004627.2786132-1-osk@google.com/

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

[Guenter Roeck]

On Fri, Sep 24, 2021 at 11:37:00AM -0400, Oskar Senft wrote:
> > > In many cases the channels are "shared" - we have 3 voltage, 3 current and 3
> > > power sensors but in fact they are not separate sensors but 3 channels
> > > each able to measure 3 different things and they may share some common
> > > properties in each channel (so current, voltage and power may be
> > > calculated bases on the same shunt resistor or correction factor). An
> > > example being adi,ltc2992.  In those cases it doesn't make sense to have
> > > two levels as how would you describe the shared parent? Call it generic
> > > "channels"?
> 
> So in that case (e.g. for the nct7802, see [1]) do we want just
> 1-level, maybe like this:
> 
> nct7802@28 {
>     compatible = "nuvoton,nct7802";
>     reg = <0x28>;
> 
>     sensor@1 { /* RTD1 */
>          reg = <0x1>;
>          status = "okay";
>          mode = "thermistor"; /* Any of "thermistor", "thermal-diode",
> "voltage" */
>     };
> 
>     sensor@2 { /* RTD2 */
>          reg = <0x2>;
>          status = "okay";
>          mode = "thermal-diode"; /* Any of "thermistor",
> "thermal-diode", "voltage" */
>     };
> 
>     sensor@3 { /* RTD3 */
>          reg = <0x3>;
>          status = "okay";
>          mode = "voltage"; /* Any of "thermistor", "voltage" */
>     };
> 
>     sensor@4 { /* LTD */
>         reg = <0x4>; /* using the same number as in sysfs */

Numbering in sysfs is not relevant here; the index should always start with 0.

>         status = "okay";
>         /* No mode configuration for LTD */
>     };
> };
> 
> In this example, RTD1, RTD2 and LTD would be temperature sensors and
> RTD3 would be a voltage sensor.
> 
> Would that make more sense? Is the use of strings acceptable?
> 
I don't think so. I am quite sure that rtd3 is still a temperature,
and I am not sure if other sensor types on that chip may need dt
configuration.

I have limited internet acces for the next week or so; I'll look
into this further after I am back.

Guenter

> Thanks
> Oskar.
> 
> [1] https://lore.kernel.org/all/20210921004627.2786132-1-osk@google.com/

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

[Oskar Senft]

Hi Guenter

> Numbering in sysfs is not relevant here; the index should always start with 0.
Ok, in that case, I'll encode LTD as @0 and RTD1..3 as @1..@3.

> > In this example, RTD1, RTD2 and LTD would be temperature sensors and
> > RTD3 would be a voltage sensor.
> >
> > Would that make more sense? Is the use of strings acceptable?
> >
> I don't think so. I am quite sure that rtd3 is still a temperature,
> and I am not sure if other sensor types on that chip may need dt
> configuration.
Reading the existing nct7802_in_is_visible() and
nct7802_temp_is_visible() [1] in I read that RTD3 could either be
voltage or temperature.

I'll go ahead and propose another patch version on [3] for that.

[1] https://github.com/torvalds/linux/blob/master/drivers/hwmon/nct7802.c#L778
[2] https://github.com/torvalds/linux/blob/master/drivers/hwmon/nct7802.c#L679
[3] https://lore.kernel.org/all/20210921004627.2786132-1-osk@google.com/

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

[Guenter Roeck]

On 10/8/21 5:55 AM, Oskar Senft wrote:
> Hi Guenter
> 
>> Numbering in sysfs is not relevant here; the index should always start with 0.
> Ok, in that case, I'll encode LTD as @0 and RTD1..3 as @1..@3.
> 
>>> In this example, RTD1, RTD2 and LTD would be temperature sensors and
>>> RTD3 would be a voltage sensor.
>>>
>>> Would that make more sense? Is the use of strings acceptable?
>>>
>> I don't think so. I am quite sure that rtd3 is still a temperature,
>> and I am not sure if other sensor types on that chip may need dt
>> configuration.
> Reading the existing nct7802_in_is_visible() and
> nct7802_temp_is_visible() [1] in I read that RTD3 could either be
> voltage or temperature.
> 

Ah yes, you are correct. The same applies to RTD1 and RTD2, actually,
Sorry, it has been too long since I wrote the driver. And it really needs
a conversion to the new hwmon API.

Guenter

> I'll go ahead and propose another patch version on [3] for that.
> 
> [1] https://github.com/torvalds/linux/blob/master/drivers/hwmon/nct7802.c#L778
> [2] https://github.com/torvalds/linux/blob/master/drivers/hwmon/nct7802.c#L679
> [3] https://lore.kernel.org/all/20210921004627.2786132-1-osk@google.com/
> 

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

[Krzysztof Adamski]

Dnia Tue, Sep 21, 2021 at 05:58:31AM -0700, Guenter Roeck napisał(a):
>>
>> ti,n-factor
>
>n-factor isn't just supported by TI sensors, though it isn't always called
>n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
>also refer to the factor as "N" in the datasheet.
>
>So question is if we make this ti,n-factor and maxim,n-factor, or if we make
>it generic and define some kind of generic units. Thoughts ? My personal
>preference would be a generic definition, but is not a strong preference.

That was exactly my way of thinking here - many sensors have n-factor
parameter and this is the name I see most often.

That being said, maybe it should be "nfactor" instead of "n-factor", as
this is what tmp513 is using?

>In regard to units, the n-factor is, as the name says, a factor. Default
>value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
>it is 0.706542 to 1.747977. So the scondary question is if the value
>written should be the register value (as proposed here) or the absolute
>factor (eg in micro-units).

Since expressing the fractional values in DT isn't well supported and
(at least here) hardware guys like to think in terms of register values
so this is what I proposed. Also, I just noticed that, for example,
TMP531 is using register values as well.

>> > +    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";
>> > +        };
>
>In the context or other sensors, question here is if we can make the
>bindings generic. We have been discussing this for NCT7802Y. The main
>question for me is how to handle different sensor types. TMP421 is
>easy because it only has one type of sensors, but there are other
>devices which also have, for example, voltage and/or current sensors.
>NCT7802 is an example for that. We just had a set of bindings for that
>chip proposed at
>https://patchwork.kernel.org/project/linux-hwmon/patch/20210921004627.2786132-1-osk@google.com/
>
>Would it be possible to determine a generic scheme that works for all
>chips ? 

Just wanted to note that the bindings I propse were not completely made
up by me. I based them on existing ina3221 bindings so I feel like my
proposition is at least in line with that one.

> I can see two problems:
>- How to express sensor types. The NCT7802 submission proposes another level
>  of indirection, ie
>
>  temperature-sensors {
>> > +
>> > +        input@1 {
>> > +          reg = <0x1>;
>> > +          n-factor = <0x0>;
>> > +          label = "somelabel";
>> > +        };
>> > +
>> > +        input@2 {
>> > +          reg = <0x2>;
>> > +          status = "disabled";
>> > +        };
>> > +      };
>> > +    };
>    };
>
>The second question is how to express sensor index. One option is the solution
>suggested here, ie to use reg=<> as sensor index. The second is the solution
>suggested in the 7802 bindings, where the (chip specific) name is used as
>sensor index.
>
>+            temperature-sensors {
>+                ltd {
>+                  status = "disabled";
>+                };
>+
>+                rtd1 {
>+                  status = "okay";
>+                  type = <4> /* thermistor */;
>+                };
>+            };
>
>I personally don't have a strong opinion either way, but I would like to see
>a single solution for all sensor chips.

For me, the problem with using this style is that it is sometimes hard
to come up with good names, especially in simple devices where all you
have are channels.. which BTW may be called differently as well. So we
would end up having some device with channel0, channel1, etc, and others
might have input0, input1, etc.
This argument goes the other way around as well - some devies have no
way to easily enumerate their "subdevices" it would be hard to assing
proper numbers to them.

For this reason I think it would make sense to actually use both schemes
- reg for cases where the enumeration makes sesne (when we have
   channels, inputs, etc)
- names otherwise, when there is no natual way to enumerate the devices
   by an index.

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

[Guenter Roeck]

On Wed, Sep 22, 2021 at 09:22:33AM +0200, Krzysztof Adamski wrote:
> Dnia Tue, Sep 21, 2021 at 05:58:31AM -0700, Guenter Roeck napisał(a):
> > > 
> > > ti,n-factor
> > 
> > n-factor isn't just supported by TI sensors, though it isn't always called
> > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
> > also refer to the factor as "N" in the datasheet.
> > 
> > So question is if we make this ti,n-factor and maxim,n-factor, or if we make
> > it generic and define some kind of generic units. Thoughts ? My personal
> > preference would be a generic definition, but is not a strong preference.
> 
> That was exactly my way of thinking here - many sensors have n-factor
> parameter and this is the name I see most often.
> 
> That being said, maybe it should be "nfactor" instead of "n-factor", as
> this is what tmp513 is using?
> 
> > In regard to units, the n-factor is, as the name says, a factor. Default
> > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
> > it is 0.706542 to 1.747977. So the scondary question is if the value
> > written should be the register value (as proposed here) or the absolute
> > factor (eg in micro-units).
> 
> Since expressing the fractional values in DT isn't well supported and
> (at least here) hardware guys like to think in terms of register values
> so this is what I proposed. Also, I just noticed that, for example,
> TMP531 is using register values as well.
> 

I never see "someone else does that" as valid argument. Also, DT does
support fractional values, via units. It is perfectly valid to describe
a voltage as micro-volt, for example.

If the agreement is to use raw register values, I think the property name
should be prefixed with the vendor name, since it won't be a standard
property. I'll defer on Rob for that, though.

Thanks,
Guenter

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

[Krzysztof Adamski]

Dnia Wed, Sep 22, 2021 at 05:39:26AM -0700, Guenter Roeck napisał(a):
>On Wed, Sep 22, 2021 at 09:22:33AM +0200, Krzysztof Adamski wrote:
>> Dnia Tue, Sep 21, 2021 at 05:58:31AM -0700, Guenter Roeck napisał(a):
>> > >
>> > > ti,n-factor
>> >
>> > n-factor isn't just supported by TI sensors, though it isn't always called
>> > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
>> > also refer to the factor as "N" in the datasheet.
>> >
>> > So question is if we make this ti,n-factor and maxim,n-factor, or if we make
>> > it generic and define some kind of generic units. Thoughts ? My personal
>> > preference would be a generic definition, but is not a strong preference.
>>
>> That was exactly my way of thinking here - many sensors have n-factor
>> parameter and this is the name I see most often.
>>
>> That being said, maybe it should be "nfactor" instead of "n-factor", as
>> this is what tmp513 is using?
>>
>> > In regard to units, the n-factor is, as the name says, a factor. Default
>> > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
>> > it is 0.706542 to 1.747977. So the scondary question is if the value
>> > written should be the register value (as proposed here) or the absolute
>> > factor (eg in micro-units).
>>
>> Since expressing the fractional values in DT isn't well supported and
>> (at least here) hardware guys like to think in terms of register values
>> so this is what I proposed. Also, I just noticed that, for example,
>> TMP531 is using register values as well.
>>
>
>I never see "someone else does that" as valid argument.

It is not an argument for "so I should be allowed too" but more like "so
it is generic enough to make sense for more than a single case" :)

> Also, DT does support fractional values, via units. It is perfectly
> valid to describe a voltage as micro-volt, for example.

True. But doing so for unit-less values isn't as obvious. For real
fractions we don't even know what the resolution should be and then we
also may have those rounding errors etc (while with register values we
know precisely what we get). As usual, we have some pros and
cons of both approaches. While I agree raw values are not perfect, I
still think it makes more sense so I vote for them. But my vote,
obviously, isn't that important here so I'll let you guys decide.

>If the agreement is to use raw register values, I think the property name
>should be prefixed with the vendor name, since it won't be a standard
>property. I'll defer on Rob for that, though.

Fair enough. If we go that route, we should use "ti,nfactor" (without
dash) to be consistent with ti513?

Krzysztof

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

[Guenter Roeck]

On Wed, Sep 22, 2021 at 08:32:00PM +0200, Krzysztof Adamski wrote:
> Dnia Wed, Sep 22, 2021 at 05:39:26AM -0700, Guenter Roeck napisał(a):
> > On Wed, Sep 22, 2021 at 09:22:33AM +0200, Krzysztof Adamski wrote:
> > > Dnia Tue, Sep 21, 2021 at 05:58:31AM -0700, Guenter Roeck napisał(a):
> > > > >
> > > > > ti,n-factor
> > > >
> > > > n-factor isn't just supported by TI sensors, though it isn't always called
> > > > n-factor. Maxim (eg MAX6581) uses the term "ideality factor", though they
> > > > also refer to the factor as "N" in the datasheet.
> > > >
> > > > So question is if we make this ti,n-factor and maxim,n-factor, or if we make
> > > > it generic and define some kind of generic units. Thoughts ? My personal
> > > > preference would be a generic definition, but is not a strong preference.
> > > 
> > > That was exactly my way of thinking here - many sensors have n-factor
> > > parameter and this is the name I see most often.
> > > 
> > > That being said, maybe it should be "nfactor" instead of "n-factor", as
> > > this is what tmp513 is using?
> > > 
> > > > In regard to units, the n-factor is, as the name says, a factor. Default
> > > > value is 1.008. The value range for MAX6581 is 0.999 to 1.030. For TMP421
> > > > it is 0.706542 to 1.747977. So the scondary question is if the value
> > > > written should be the register value (as proposed here) or the absolute
> > > > factor (eg in micro-units).
> > > 
> > > Since expressing the fractional values in DT isn't well supported and
> > > (at least here) hardware guys like to think in terms of register values
> > > so this is what I proposed. Also, I just noticed that, for example,
> > > TMP531 is using register values as well.
> > > 
> > 
> > I never see "someone else does that" as valid argument.
> 
> It is not an argument for "so I should be allowed too" but more like "so
> it is generic enough to make sense for more than a single case" :)
> 
> > Also, DT does support fractional values, via units. It is perfectly
> > valid to describe a voltage as micro-volt, for example.
> 
> True. But doing so for unit-less values isn't as obvious. For real
> fractions we don't even know what the resolution should be and then we
> also may have those rounding errors etc (while with register values we
> know precisely what we get). As usual, we have some pros and
> cons of both approaches. While I agree raw values are not perfect, I
> still think it makes more sense so I vote for them. But my vote,
> obviously, isn't that important here so I'll let you guys decide.
> 

I really have to pass on this one, and leave it up to Rob to decide.
Personally I really really really dislike raw values, but I understand
that this makes me biased. I do realize that converting from a fractional
value to a register value is inherently complex and open to interpretation.
For example. if we define fractional values, what should 1.007000 translate
to ? It would either be 1.008 or 1.004641. Using the register value (0xff,
or -1 for 1.004641) would definitely be simpler and avoid calculations and
rounding.

Guenter

> > If the agreement is to use raw register values, I think the property name
> > should be prefixed with the vendor name, since it won't be a standard
> > property. I'll defer on Rob for that, though.
> 
> Fair enough. If we go that route, we should use "ti,nfactor" (without
> dash) to be consistent with ti513?
> 
> Krzysztof