Extend the device-tree binding for lm90

Extend the device-tree binding for lm90

[Slawomir Stepien]

From: Slawomir Stepien <slawomir.stepien@nokia.com>

This series extends the device-tree binding for lm90.
Support for channel's label and channel's temperature offset has been added.

In lm90.c support for 2nd remote channel's temperature offset has been added (it is useful for
ADT7481 device) along side the needed changes for new bindings.

Note: this series has been rebased on hwmon/hwmon-lm90.

[PATCH 1/7] dt-bindings: hwmon: Add compatible string for ADT7481 in lm90

[Slawomir Stepien]

From: Slawomir Stepien <slawomir.stepien@nokia.com>

This will allow binding the driver with the device from the device tree.

This device can work in extended temperature measurement mode, so add it
also to the list of devices that support 'ti,extended-range-enable'.

Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
 Documentation/devicetree/bindings/hwmon/national,lm90.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
index b04657849852..82fce96498c7 100644
--- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
+++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
@@ -16,6 +16,7 @@ properties:
       - adi,adm1032
       - adi,adt7461
       - adi,adt7461a
+      - adi,adt7481
       - dallas,max6646
       - dallas,max6647
       - dallas,max6649
@@ -70,6 +71,7 @@ allOf:
               enum:
                 - adi,adt7461
                 - adi,adt7461a
+                - adi,adt7481
                 - ti,tmp451
                 - ti,tmp461
     then:
-- 
2.36.1

[PATCH 2/7] dt-bindings: hwmon: Allow specifying channels for lm90

[Slawomir Stepien]

From: Slawomir Stepien <slawomir.stepien@nokia.com>

Add binding description for temperature channels. Currently, support for
label and temperature-offset-millicelsius is implemented.

Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
 .../bindings/hwmon/national,lm90.yaml         | 129 ++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
index 82fce96498c7..e1719839faf0 100644
--- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
+++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
@@ -51,6 +51,12 @@ properties:
   "#thermal-sensor-cells":
     const: 1
 
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
   vcc-supply:
     description: phandle to the regulator that provides the +VCC supply
 
@@ -62,6 +68,29 @@ required:
   - compatible
   - reg
 
+patternProperties:
+  "^channel@([0-2])$":
+    type: object
+    description: Represents channels of the device and their specific configuration.
+
+    properties:
+      reg:
+        description: The channel number. 0 is local channel, 1-2 are remote channels.
+        items:
+          minimum: 0
+          maximum: 2
+
+      label:
+        description: A descriptive name for this channel, like "ambient" or "psu".
+
+      temperature-offset-millicelsius:
+        description: Temperature offset to be added to or subtracted from remote temperature measurements.
+
+    required:
+      - reg
+
+    additionalProperties: false
+
 allOf:
   - if:
       not:
@@ -78,6 +107,77 @@ allOf:
       properties:
         ti,extended-range-enable: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - dallas,max6646
+              - dallas,max6647
+              - dallas,max6649
+              - dallas,max6657
+              - dallas,max6658
+              - dallas,max6659
+              - dallas,max6695
+              - dallas,max6696
+    then:
+      patternProperties:
+        "^channel@([0-2])$":
+          properties:
+            temperature-offset-millicelsius: false
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,adt7461
+              - adi,adt7461a
+              - adi,adt7481
+              - onnn,nct1008
+    then:
+      patternProperties:
+        "^channel@([0-2])$":
+          properties:
+            temperature-offset-millicelsius:
+              maximum: 127750
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,adm1032
+              - dallas,max6680
+              - dallas,max6681
+              - gmt,g781
+              - national,lm86
+              - national,lm89
+              - national,lm90
+              - national,lm99
+              - nxp,sa56004
+              - winbond,w83l771
+    then:
+      patternProperties:
+        "^channel@([0-2])$":
+          properties:
+            temperature-offset-millicelsius:
+              maximum: 127875
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - ti,tmp451
+              - ti,tmp461
+    then:
+      patternProperties:
+        "^channel@([0-2])$":
+          properties:
+            temperature-offset-millicelsius:
+              maximum: 127937
+
 additionalProperties: false
 
 examples:
@@ -96,3 +196,32 @@ examples:
             #thermal-sensor-cells = <1>;
         };
     };
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      sensor@4c {
+        compatible = "adi,adt7481";
+        reg = <0x4c>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+          reg = <0x0>;
+          label = "local";
+        };
+
+        channel@1 {
+          reg = <0x1>;
+          label = "front";
+          temperature-offset-millicelsius = <4000>;
+        };
+
+        channel@2 {
+          reg = <0x2>;
+          label = "back";
+          temperature-offset-millicelsius = <750>;
+        };
+      };
+    };
-- 
2.36.1

[PATCH 3/7] hwmon: (lm90) Add compatible entry for adt7481

[Slawomir Stepien]

From: Slawomir Stepien <slawomir.stepien@nokia.com>

This will allow binding the driver with the device from the device tree.

Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
 drivers/hwmon/lm90.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 4c25c9ffdfe9..02b211a4e571 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -294,6 +294,10 @@ static const struct of_device_id __maybe_unused lm90_of_match[] = {
 		.compatible = "adi,adt7461a",
 		.data = (void *)adt7461a
 	},
+	{
+		.compatible = "adi,adt7481",
+		.data = (void *)adt7481
+	},
 	{
 		.compatible = "gmt,g781",
 		.data = (void *)g781
-- 
2.36.1

[PATCH 4/7] hwmon: (lm90) Add support for 2nd remote channel's offset register

[Slawomir Stepien]

From: Slawomir Stepien <slawomir.stepien@nokia.com>

The ADT7461 supports offset register for both remote channels it has.
Both registers have the same bit width (resolution).

In the code, this device has LM90_HAVE_TEMP3 and LM90_HAVE_OFFSET flags,
but the support of second remote channel's offset is missing. Add that
implementation.

Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
 drivers/hwmon/lm90.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 02b211a4e571..d226f1dea2ba 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -153,6 +153,8 @@ enum chips { adm1023, adm1032, adt7461, adt7461a, adt7481,
 #define LM90_REG_REMOTE_TEMPL		0x10
 #define LM90_REG_REMOTE_OFFSH		0x11
 #define LM90_REG_REMOTE_OFFSL		0x12
+#define LM90_REG_REMOTE2_OFFSH		0x34
+#define LM90_REG_REMOTE2_OFFSL		0x35
 #define LM90_REG_REMOTE_HIGHH		0x07
 #define LM90_REG_REMOTE_HIGHL		0x13
 #define LM90_REG_REMOTE_LOWH		0x08
@@ -669,6 +671,7 @@ enum lm90_temp_reg_index {
 	REMOTE2_TEMP,	/* max6695/96 only */
 	REMOTE2_LOW,	/* max6695/96 only */
 	REMOTE2_HIGH,	/* max6695/96 only */
+	REMOTE2_OFFSET,
 
 	TEMP_REG_NUM
 };
@@ -1024,6 +1027,14 @@ static int lm90_update_limits(struct device *dev)
 			return val;
 		data->temp[REMOTE2_HIGH] = val << 8;
 
+		if (data->flags & LM90_HAVE_OFFSET) {
+			val = lm90_read16(client, LM90_REG_REMOTE2_OFFSH,
+					  LM90_REG_REMOTE2_OFFSL, false);
+			if (val < 0)
+				return val;
+			data->temp[REMOTE2_OFFSET] = val;
+		}
+
 		lm90_select_remote_channel(data, false);
 	}
 
@@ -1294,6 +1305,7 @@ static int lm90_temp_get_resolution(struct lm90_data *data, int index)
 			return data->resolution;
 		return 8;
 	case REMOTE_OFFSET:
+	case REMOTE2_OFFSET:
 	case REMOTE2_TEMP:
 		return data->resolution;
 	case LOCAL_TEMP:
@@ -1515,8 +1527,13 @@ static int lm90_temp_read(struct device *dev, u32 attr, int channel, long *val)
 		*val = lm90_get_temphyst(data, lm90_temp_emerg_index[channel], channel);
 		break;
 	case hwmon_temp_offset:
-		*val = lm90_temp_from_reg(0, data->temp[REMOTE_OFFSET],
-					  lm90_temp_get_resolution(data, REMOTE_OFFSET));
+		/* Both offset registers have the same resolution */
+		int res = lm90_temp_get_resolution(data, REMOTE_OFFSET);
+
+		if (channel == 1)
+			*val = lm90_temp_from_reg(0, data->temp[REMOTE_OFFSET], res);
+		else
+			*val = lm90_temp_from_reg(0, data->temp[REMOTE2_OFFSET], res);
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -1556,11 +1573,19 @@ static int lm90_temp_write(struct device *dev, u32 attr, int channel, long val)
 				    channel, val);
 		break;
 	case hwmon_temp_offset:
+		/* Both offset registers have the same resolution */
 		val = lm90_temp_to_reg(0, val,
 				       lm90_temp_get_resolution(data, REMOTE_OFFSET));
-		data->temp[REMOTE_OFFSET] = val;
-		err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
-				   LM90_REG_REMOTE_OFFSL, val);
+
+		if (channel == 1) {
+			data->temp[REMOTE_OFFSET] = val;
+			err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
+					   LM90_REG_REMOTE_OFFSL, val);
+		} else {
+			data->temp[REMOTE2_OFFSET] = val;
+			err = lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
+					   LM90_REG_REMOTE2_OFFSL, val);
+		}
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -2733,6 +2758,8 @@ static int lm90_probe(struct i2c_client *client)
 		}
 		if (data->flags & LM90_HAVE_EMERGENCY_ALARM)
 			data->channel_config[2] |= HWMON_T_EMERGENCY_ALARM;
+		if (data->flags & LM90_HAVE_OFFSET)
+			data->channel_config[2] |= HWMON_T_OFFSET;
 	}
 
 	data->faultqueue_mask = lm90_params[data->kind].faultqueue_mask;
-- 
2.36.1

[PATCH 5/7] hwmon: (lm90) Define maximum number of channels that are supported

[Slawomir Stepien]

From: Slawomir Stepien <slawomir.stepien@nokia.com>

Use this define in all the places where literal '3' was used in this
context.

Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
 drivers/hwmon/lm90.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index d226f1dea2ba..82b020ffd490 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -110,6 +110,9 @@
 #include <linux/slab.h>
 #include <linux/workqueue.h>
 
+/* The maximum number of channels currently supported */
+#define MAX_CHANNELS	3
+
 /*
  * Addresses to scan
  * Address is fully defined internally and cannot be changed except for
@@ -684,7 +687,7 @@ struct lm90_data {
 	struct i2c_client *client;
 	struct device *hwmon_dev;
 	u32 chip_config[2];
-	u32 channel_config[4];
+	u32 channel_config[MAX_CHANNELS + 1];
 	struct hwmon_channel_info chip_info;
 	struct hwmon_channel_info temp_info;
 	const struct hwmon_channel_info *info[3];
@@ -1436,32 +1439,32 @@ static int lm90_set_temphyst(struct lm90_data *data, long val)
 	return lm90_write_reg(data->client, LM90_REG_TCRIT_HYST, data->temp_hyst);
 }
 
-static const u8 lm90_temp_index[3] = {
+static const u8 lm90_temp_index[MAX_CHANNELS] = {
 	LOCAL_TEMP, REMOTE_TEMP, REMOTE2_TEMP
 };
 
-static const u8 lm90_temp_min_index[3] = {
+static const u8 lm90_temp_min_index[MAX_CHANNELS] = {
 	LOCAL_LOW, REMOTE_LOW, REMOTE2_LOW
 };
 
-static const u8 lm90_temp_max_index[3] = {
+static const u8 lm90_temp_max_index[MAX_CHANNELS] = {
 	LOCAL_HIGH, REMOTE_HIGH, REMOTE2_HIGH
 };
 
-static const u8 lm90_temp_crit_index[3] = {
+static const u8 lm90_temp_crit_index[MAX_CHANNELS] = {
 	LOCAL_CRIT, REMOTE_CRIT, REMOTE2_CRIT
 };
 
-static const u8 lm90_temp_emerg_index[3] = {
+static const u8 lm90_temp_emerg_index[MAX_CHANNELS] = {
 	LOCAL_EMERG, REMOTE_EMERG, REMOTE2_EMERG
 };
 
-static const u16 lm90_min_alarm_bits[3] = { BIT(5), BIT(3), BIT(11) };
-static const u16 lm90_max_alarm_bits[3] = { BIT(6), BIT(4), BIT(12) };
-static const u16 lm90_crit_alarm_bits[3] = { BIT(0), BIT(1), BIT(9) };
-static const u16 lm90_crit_alarm_bits_swapped[3] = { BIT(1), BIT(0), BIT(9) };
-static const u16 lm90_emergency_alarm_bits[3] = { BIT(15), BIT(13), BIT(14) };
-static const u16 lm90_fault_bits[3] = { BIT(0), BIT(2), BIT(10) };
+static const u16 lm90_min_alarm_bits[MAX_CHANNELS] = { BIT(5), BIT(3), BIT(11) };
+static const u16 lm90_max_alarm_bits[MAX_CHANNELS] = { BIT(6), BIT(4), BIT(12) };
+static const u16 lm90_crit_alarm_bits[MAX_CHANNELS] = { BIT(0), BIT(1), BIT(9) };
+static const u16 lm90_crit_alarm_bits_swapped[MAX_CHANNELS] = { BIT(1), BIT(0), BIT(9) };
+static const u16 lm90_emergency_alarm_bits[MAX_CHANNELS] = { BIT(15), BIT(13), BIT(14) };
+static const u16 lm90_fault_bits[MAX_CHANNELS] = { BIT(0), BIT(2), BIT(10) };
 
 static int lm90_temp_read(struct device *dev, u32 attr, int channel, long *val)
 {
-- 
2.36.1

[PATCH 6/7] hwmon: (lm90) Read the channel's label from device-tree

[Slawomir Stepien]

From: Slawomir Stepien <slawomir.stepien@nokia.com>

Try to read the channel's label from device-tree. Having label in
device-tree node is not mandatory.

Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
 drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 82b020ffd490..3837c4ab5833 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -688,6 +688,7 @@ struct lm90_data {
 	struct device *hwmon_dev;
 	u32 chip_config[2];
 	u32 channel_config[MAX_CHANNELS + 1];
+	const char *channel_label[MAX_CHANNELS];
 	struct hwmon_channel_info chip_info;
 	struct hwmon_channel_info temp_info;
 	const struct hwmon_channel_info *info[3];
@@ -1610,6 +1611,7 @@ static umode_t lm90_temp_is_visible(const void *data, u32 attr, int channel)
 	case hwmon_temp_emergency_alarm:
 	case hwmon_temp_emergency_hyst:
 	case hwmon_temp_fault:
+	case hwmon_temp_label:
 		return 0444;
 	case hwmon_temp_min:
 	case hwmon_temp_max:
@@ -1729,6 +1731,16 @@ static int lm90_read(struct device *dev, enum hwmon_sensor_types type,
 	}
 }
 
+static int lm90_read_string(struct device *dev, enum hwmon_sensor_types type,
+			    u32 attr, int channel, const char **str)
+{
+	struct lm90_data *data = dev_get_drvdata(dev);
+
+	*str = data->channel_label[channel];
+
+	return 0;
+}
+
 static int lm90_write(struct device *dev, enum hwmon_sensor_types type,
 		      u32 attr, int channel, long val)
 {
@@ -2634,10 +2646,63 @@ static void lm90_regulator_disable(void *regulator)
 	regulator_disable(regulator);
 }
 
+static int lm90_probe_channel_from_dt(struct i2c_client *client,
+				      struct device_node *child,
+				      struct lm90_data *data)
+{
+	u32 id;
+	int err;
+	struct device *dev = &client->dev;
+
+	err = of_property_read_u32(child, "reg", &id);
+	if (err) {
+		dev_err(dev, "missing reg property of %pOFn\n", child);
+		return err;
+	}
+
+	if (id >= MAX_CHANNELS) {
+		dev_err(dev, "invalid reg property value %d in %pOFn\n", id, child);
+		return -EINVAL;
+	}
+
+	err = of_property_read_string(child, "label", &data->channel_label[id]);
+	if (err == -ENODATA || err == -EILSEQ) {
+		dev_err(dev, "invalid label property in %pOFn\n", child);
+		return err;
+	}
+
+	if (data->channel_label[id])
+		data->channel_config[id] |= HWMON_T_LABEL;
+
+	return 0;
+}
+
+static int lm90_parse_dt_channel_info(struct i2c_client *client,
+				      struct lm90_data *data)
+{
+	int err;
+	struct device_node *child;
+	struct device *dev = &client->dev;
+	const struct device_node *np = dev->of_node;
+
+	for_each_child_of_node(np, child) {
+		if (strcmp(child->name, "channel"))
+			continue;
+
+		err = lm90_probe_channel_from_dt(client, child, data);
+		if (err) {
+			of_node_put(child);
+			return err;
+		}
+	}
+
+	return 0;
+}
 
 static const struct hwmon_ops lm90_ops = {
 	.is_visible = lm90_is_visible,
 	.read = lm90_read,
+	.read_string = lm90_read_string,
 	.write = lm90_write,
 };
 
@@ -2775,6 +2840,13 @@ static int lm90_probe(struct i2c_client *client)
 	/* Set maximum conversion rate */
 	data->max_convrate = lm90_params[data->kind].max_convrate;
 
+	/* Parse device-tree channel information */
+	if (client->dev.of_node) {
+		err = lm90_parse_dt_channel_info(client, data);
+		if (err)
+			return err;
+	}
+
 	/* Initialize the LM90 chip */
 	err = lm90_init_client(client, data);
 	if (err < 0) {
-- 
2.36.1

[PATCH 7/7] hwmon: (lm90) Read the channel's temperature offset from device-tree

[Slawomir Stepien]

From: Slawomir Stepien <slawomir.stepien@nokia.com>

Try to read the channel's temperature offset from device-tree. Having
offset in device-tree node is not mandatory. The offset can only be set
for remote channels.

Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
 drivers/hwmon/lm90.c | 48 ++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 3837c4ab5833..12e8e874f1b9 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -1440,6 +1440,24 @@ static int lm90_set_temphyst(struct lm90_data *data, long val)
 	return lm90_write_reg(data->client, LM90_REG_TCRIT_HYST, data->temp_hyst);
 }
 
+static int lm90_set_temp_offset(struct lm90_data *data, int channel, long val)
+{
+	/* Both offset registers have the same resolution */
+	int res = lm90_temp_get_resolution(data, REMOTE_OFFSET);
+
+	val = lm90_temp_to_reg(0, val, res);
+
+	if (channel == 1) {
+		data->temp[REMOTE_OFFSET] = val;
+		return lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
+				    LM90_REG_REMOTE_OFFSL, val);
+	}
+
+	data->temp[REMOTE2_OFFSET] = val;
+	return lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
+			    LM90_REG_REMOTE2_OFFSL, val);
+}
+
 static const u8 lm90_temp_index[MAX_CHANNELS] = {
 	LOCAL_TEMP, REMOTE_TEMP, REMOTE2_TEMP
 };
@@ -1577,19 +1595,7 @@ static int lm90_temp_write(struct device *dev, u32 attr, int channel, long val)
 				    channel, val);
 		break;
 	case hwmon_temp_offset:
-		/* Both offset registers have the same resolution */
-		val = lm90_temp_to_reg(0, val,
-				       lm90_temp_get_resolution(data, REMOTE_OFFSET));
-
-		if (channel == 1) {
-			data->temp[REMOTE_OFFSET] = val;
-			err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
-					   LM90_REG_REMOTE_OFFSL, val);
-		} else {
-			data->temp[REMOTE2_OFFSET] = val;
-			err = lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
-					   LM90_REG_REMOTE2_OFFSL, val);
-		}
+		err = lm90_set_temp_offset(data, channel, val);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -2651,6 +2657,7 @@ static int lm90_probe_channel_from_dt(struct i2c_client *client,
 				      struct lm90_data *data)
 {
 	u32 id;
+	s32 val;
 	int err;
 	struct device *dev = &client->dev;
 
@@ -2674,6 +2681,21 @@ static int lm90_probe_channel_from_dt(struct i2c_client *client,
 	if (data->channel_label[id])
 		data->channel_config[id] |= HWMON_T_LABEL;
 
+	err = of_property_read_s32(child, "temperature-offset-millicelsius", &val);
+	if (!err) {
+		if (id == 0) {
+			dev_err(dev, "offset can't be set for internal channel\n");
+			return -EINVAL;
+		}
+
+		err = lm90_set_temp_offset(data, id, val);
+		if (err) {
+			dev_err(dev, "can't set offset %d for channel %d (%d)\n",
+				val, id, err);
+			return err;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.36.1

Re: [PATCH 1/7] dt-bindings: hwmon: Add compatible string for ADT7481 in lm90

[Rob Herring]

On Wed, 25 May 2022 09:36:51 +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> This will allow binding the driver with the device from the device tree.
> 
> This device can work in extended temperature measurement mode, so add it
> also to the list of devices that support 'ti,extended-range-enable'.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> ---
>  Documentation/devicetree/bindings/hwmon/national,lm90.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

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

Re: [PATCH 2/7] dt-bindings: hwmon: Allow specifying channels for lm90

[Rob Herring]

On Wed, 25 May 2022 09:36:52 +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Add binding description for temperature channels. Currently, support for
> label and temperature-offset-millicelsius is implemented.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> ---
>  .../bindings/hwmon/national,lm90.yaml         | 129 ++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 

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

Re: [PATCH 1/7] dt-bindings: hwmon: Add compatible string for ADT7481 in lm90

[Guenter Roeck]

On Wed, May 25, 2022 at 09:36:51AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> This will allow binding the driver with the device from the device tree.
> 
> This device can work in extended temperature measurement mode, so add it
> also to the list of devices that support 'ti,extended-range-enable'.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> Acked-by: Rob Herring <robh@kernel.org>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  Documentation/devicetree/bindings/hwmon/national,lm90.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> index b04657849852..82fce96498c7 100644
> --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> @@ -16,6 +16,7 @@ properties:
>        - adi,adm1032
>        - adi,adt7461
>        - adi,adt7461a
> +      - adi,adt7481
>        - dallas,max6646
>        - dallas,max6647
>        - dallas,max6649
> @@ -70,6 +71,7 @@ allOf:
>                enum:
>                  - adi,adt7461
>                  - adi,adt7461a
> +                - adi,adt7481
>                  - ti,tmp451
>                  - ti,tmp461
>      then:

Re: [PATCH 2/7] dt-bindings: hwmon: Allow specifying channels for lm90

[Guenter Roeck]

On Wed, May 25, 2022 at 09:36:52AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Add binding description for temperature channels. Currently, support for
> label and temperature-offset-millicelsius is implemented.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  .../bindings/hwmon/national,lm90.yaml         | 129 ++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> index 82fce96498c7..e1719839faf0 100644
> --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> @@ -51,6 +51,12 @@ properties:
>    "#thermal-sensor-cells":
>      const: 1
>  
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
>    vcc-supply:
>      description: phandle to the regulator that provides the +VCC supply
>  
> @@ -62,6 +68,29 @@ required:
>    - compatible
>    - reg
>  
> +patternProperties:
> +  "^channel@([0-2])$":
> +    type: object
> +    description: Represents channels of the device and their specific configuration.
> +
> +    properties:
> +      reg:
> +        description: The channel number. 0 is local channel, 1-2 are remote channels.
> +        items:
> +          minimum: 0
> +          maximum: 2
> +
> +      label:
> +        description: A descriptive name for this channel, like "ambient" or "psu".
> +
> +      temperature-offset-millicelsius:
> +        description: Temperature offset to be added to or subtracted from remote temperature measurements.
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
>  allOf:
>    - if:
>        not:
> @@ -78,6 +107,77 @@ allOf:
>        properties:
>          ti,extended-range-enable: false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - dallas,max6646
> +              - dallas,max6647
> +              - dallas,max6649
> +              - dallas,max6657
> +              - dallas,max6658
> +              - dallas,max6659
> +              - dallas,max6695
> +              - dallas,max6696
> +    then:
> +      patternProperties:
> +        "^channel@([0-2])$":
> +          properties:
> +            temperature-offset-millicelsius: false
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,adt7461
> +              - adi,adt7461a
> +              - adi,adt7481
> +              - onnn,nct1008
> +    then:
> +      patternProperties:
> +        "^channel@([0-2])$":
> +          properties:
> +            temperature-offset-millicelsius:
> +              maximum: 127750
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,adm1032
> +              - dallas,max6680
> +              - dallas,max6681
> +              - gmt,g781
> +              - national,lm86
> +              - national,lm89
> +              - national,lm90
> +              - national,lm99
> +              - nxp,sa56004
> +              - winbond,w83l771
> +    then:
> +      patternProperties:
> +        "^channel@([0-2])$":
> +          properties:
> +            temperature-offset-millicelsius:
> +              maximum: 127875
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - ti,tmp451
> +              - ti,tmp461
> +    then:
> +      patternProperties:
> +        "^channel@([0-2])$":
> +          properties:
> +            temperature-offset-millicelsius:
> +              maximum: 127937
> +
>  additionalProperties: false
>  
>  examples:
> @@ -96,3 +196,32 @@ examples:
>              #thermal-sensor-cells = <1>;
>          };
>      };
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      sensor@4c {
> +        compatible = "adi,adt7481";
> +        reg = <0x4c>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        channel@0 {
> +          reg = <0x0>;
> +          label = "local";
> +        };
> +
> +        channel@1 {
> +          reg = <0x1>;
> +          label = "front";
> +          temperature-offset-millicelsius = <4000>;
> +        };
> +
> +        channel@2 {
> +          reg = <0x2>;
> +          label = "back";
> +          temperature-offset-millicelsius = <750>;
> +        };
> +      };
> +    };

Re: [PATCH 3/7] hwmon: (lm90) Add compatible entry for adt7481

[Guenter Roeck]

On Wed, May 25, 2022 at 09:36:53AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> This will allow binding the driver with the device from the device tree.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/lm90.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 4c25c9ffdfe9..02b211a4e571 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -294,6 +294,10 @@ static const struct of_device_id __maybe_unused lm90_of_match[] = {
>  		.compatible = "adi,adt7461a",
>  		.data = (void *)adt7461a
>  	},
> +	{
> +		.compatible = "adi,adt7481",
> +		.data = (void *)adt7481
> +	},
>  	{
>  		.compatible = "gmt,g781",
>  		.data = (void *)g781

Re: [PATCH 4/7] hwmon: (lm90) Add support for 2nd remote channel's offset register

[Guenter Roeck]

On Wed, May 25, 2022 at 09:36:54AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> The ADT7461 supports offset register for both remote channels it has.

ADT7481

> Both registers have the same bit width (resolution).
> 
> In the code, this device has LM90_HAVE_TEMP3 and LM90_HAVE_OFFSET flags,
> but the support of second remote channel's offset is missing. Add that
> implementation.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> ---
>  drivers/hwmon/lm90.c | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 02b211a4e571..d226f1dea2ba 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -153,6 +153,8 @@ enum chips { adm1023, adm1032, adt7461, adt7461a, adt7481,
>  #define LM90_REG_REMOTE_TEMPL		0x10
>  #define LM90_REG_REMOTE_OFFSH		0x11
>  #define LM90_REG_REMOTE_OFFSL		0x12
> +#define LM90_REG_REMOTE2_OFFSH		0x34
> +#define LM90_REG_REMOTE2_OFFSL		0x35

I don't think those are needed.

>  #define LM90_REG_REMOTE_HIGHH		0x07
>  #define LM90_REG_REMOTE_HIGHL		0x13
>  #define LM90_REG_REMOTE_LOWH		0x08
> @@ -669,6 +671,7 @@ enum lm90_temp_reg_index {
>  	REMOTE2_TEMP,	/* max6695/96 only */
>  	REMOTE2_LOW,	/* max6695/96 only */
>  	REMOTE2_HIGH,	/* max6695/96 only */
> +	REMOTE2_OFFSET,
>  
>  	TEMP_REG_NUM
>  };
> @@ -1024,6 +1027,14 @@ static int lm90_update_limits(struct device *dev)
>  			return val;
>  		data->temp[REMOTE2_HIGH] = val << 8;
>  
> +		if (data->flags & LM90_HAVE_OFFSET) {
> +			val = lm90_read16(client, LM90_REG_REMOTE2_OFFSH,
> +					  LM90_REG_REMOTE2_OFFSL, false);

Why not use LM90_REG_REMOTE_OFFSH and LM90_REG_REMOTE_OFFSL ?
The remove channel is selected here, after all.

> +			if (val < 0)
> +				return val;
> +			data->temp[REMOTE2_OFFSET] = val;
> +		}
> +
>  		lm90_select_remote_channel(data, false);
>  	}
>  
> @@ -1294,6 +1305,7 @@ static int lm90_temp_get_resolution(struct lm90_data *data, int index)
>  			return data->resolution;
>  		return 8;
>  	case REMOTE_OFFSET:
> +	case REMOTE2_OFFSET:
>  	case REMOTE2_TEMP:
>  		return data->resolution;
>  	case LOCAL_TEMP:
> @@ -1515,8 +1527,13 @@ static int lm90_temp_read(struct device *dev, u32 attr, int channel, long *val)
>  		*val = lm90_get_temphyst(data, lm90_temp_emerg_index[channel], channel);
>  		break;
>  	case hwmon_temp_offset:
> -		*val = lm90_temp_from_reg(0, data->temp[REMOTE_OFFSET],
> -					  lm90_temp_get_resolution(data, REMOTE_OFFSET));
> +		/* Both offset registers have the same resolution */
> +		int res = lm90_temp_get_resolution(data, REMOTE_OFFSET);
> +
> +		if (channel == 1)
> +			*val = lm90_temp_from_reg(0, data->temp[REMOTE_OFFSET], res);
> +		else
> +			*val = lm90_temp_from_reg(0, data->temp[REMOTE2_OFFSET], res);

I would prefer the use of lm90_temp_offset_index[] as implemented for the
other attributes.

>  		break;
>  	default:
>  		return -EOPNOTSUPP;
> @@ -1556,11 +1573,19 @@ static int lm90_temp_write(struct device *dev, u32 attr, int channel, long val)
>  				    channel, val);
>  		break;
>  	case hwmon_temp_offset:
> +		/* Both offset registers have the same resolution */
>  		val = lm90_temp_to_reg(0, val,
>  				       lm90_temp_get_resolution(data, REMOTE_OFFSET));
> -		data->temp[REMOTE_OFFSET] = val;
> -		err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> -				   LM90_REG_REMOTE_OFFSL, val);
> +
> +		if (channel == 1) {
> +			data->temp[REMOTE_OFFSET] = val;
> +			err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> +					   LM90_REG_REMOTE_OFFSL, val);
> +		} else {
> +			data->temp[REMOTE2_OFFSET] = val;
> +			err = lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
> +					   LM90_REG_REMOTE2_OFFSL, val);
> +		}

Same as above.

Thanks,
Guenter

>  		break;
>  	default:
>  		err = -EOPNOTSUPP;
> @@ -2733,6 +2758,8 @@ static int lm90_probe(struct i2c_client *client)
>  		}
>  		if (data->flags & LM90_HAVE_EMERGENCY_ALARM)
>  			data->channel_config[2] |= HWMON_T_EMERGENCY_ALARM;
> +		if (data->flags & LM90_HAVE_OFFSET)
> +			data->channel_config[2] |= HWMON_T_OFFSET;
>  	}
>  
>  	data->faultqueue_mask = lm90_params[data->kind].faultqueue_mask;

Re: [PATCH 5/7] hwmon: (lm90) Define maximum number of channels that are supported

[Guenter Roeck]

On Wed, May 25, 2022 at 09:36:55AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Use this define in all the places where literal '3' was used in this
> context.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>

Applied to hwmon-next.

Thanks,
Guenter

Re: [PATCH 6/7] hwmon: (lm90) Read the channel's label from device-tree

[Guenter Roeck]

On Wed, May 25, 2022 at 09:36:56AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Try to read the channel's label from device-tree. Having label in
> device-tree node is not mandatory.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/lm90.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 82b020ffd490..3837c4ab5833 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -688,6 +688,7 @@ struct lm90_data {
>  	struct device *hwmon_dev;
>  	u32 chip_config[2];
>  	u32 channel_config[MAX_CHANNELS + 1];
> +	const char *channel_label[MAX_CHANNELS];
>  	struct hwmon_channel_info chip_info;
>  	struct hwmon_channel_info temp_info;
>  	const struct hwmon_channel_info *info[3];
> @@ -1610,6 +1611,7 @@ static umode_t lm90_temp_is_visible(const void *data, u32 attr, int channel)
>  	case hwmon_temp_emergency_alarm:
>  	case hwmon_temp_emergency_hyst:
>  	case hwmon_temp_fault:
> +	case hwmon_temp_label:
>  		return 0444;
>  	case hwmon_temp_min:
>  	case hwmon_temp_max:
> @@ -1729,6 +1731,16 @@ static int lm90_read(struct device *dev, enum hwmon_sensor_types type,
>  	}
>  }
>  
> +static int lm90_read_string(struct device *dev, enum hwmon_sensor_types type,
> +			    u32 attr, int channel, const char **str)
> +{
> +	struct lm90_data *data = dev_get_drvdata(dev);
> +
> +	*str = data->channel_label[channel];
> +
> +	return 0;
> +}
> +
>  static int lm90_write(struct device *dev, enum hwmon_sensor_types type,
>  		      u32 attr, int channel, long val)
>  {
> @@ -2634,10 +2646,63 @@ static void lm90_regulator_disable(void *regulator)
>  	regulator_disable(regulator);
>  }
>  
> +static int lm90_probe_channel_from_dt(struct i2c_client *client,
> +				      struct device_node *child,
> +				      struct lm90_data *data)
> +{
> +	u32 id;
> +	int err;
> +	struct device *dev = &client->dev;
> +
> +	err = of_property_read_u32(child, "reg", &id);
> +	if (err) {
> +		dev_err(dev, "missing reg property of %pOFn\n", child);
> +		return err;
> +	}
> +
> +	if (id >= MAX_CHANNELS) {
> +		dev_err(dev, "invalid reg property value %d in %pOFn\n", id, child);
> +		return -EINVAL;
> +	}
> +
> +	err = of_property_read_string(child, "label", &data->channel_label[id]);
> +	if (err == -ENODATA || err == -EILSEQ) {
> +		dev_err(dev, "invalid label property in %pOFn\n", child);
> +		return err;
> +	}
> +
> +	if (data->channel_label[id])
> +		data->channel_config[id] |= HWMON_T_LABEL;
> +
> +	return 0;
> +}
> +
> +static int lm90_parse_dt_channel_info(struct i2c_client *client,
> +				      struct lm90_data *data)
> +{
> +	int err;
> +	struct device_node *child;
> +	struct device *dev = &client->dev;
> +	const struct device_node *np = dev->of_node;
> +
> +	for_each_child_of_node(np, child) {
> +		if (strcmp(child->name, "channel"))
> +			continue;
> +
> +		err = lm90_probe_channel_from_dt(client, child, data);
> +		if (err) {
> +			of_node_put(child);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
>  
>  static const struct hwmon_ops lm90_ops = {
>  	.is_visible = lm90_is_visible,
>  	.read = lm90_read,
> +	.read_string = lm90_read_string,
>  	.write = lm90_write,
>  };
>  
> @@ -2775,6 +2840,13 @@ static int lm90_probe(struct i2c_client *client)
>  	/* Set maximum conversion rate */
>  	data->max_convrate = lm90_params[data->kind].max_convrate;
>  
> +	/* Parse device-tree channel information */
> +	if (client->dev.of_node) {
> +		err = lm90_parse_dt_channel_info(client, data);
> +		if (err)
> +			return err;
> +	}
> +
>  	/* Initialize the LM90 chip */
>  	err = lm90_init_client(client, data);
>  	if (err < 0) {

Re: [PATCH 7/7] hwmon: (lm90) Read the channel's temperature offset from device-tree

[Guenter Roeck]

On Wed, May 25, 2022 at 09:36:57AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Try to read the channel's temperature offset from device-tree. Having
> offset in device-tree node is not mandatory. The offset can only be set
> for remote channels.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> ---
>  drivers/hwmon/lm90.c | 48 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 3837c4ab5833..12e8e874f1b9 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -1440,6 +1440,24 @@ static int lm90_set_temphyst(struct lm90_data *data, long val)
>  	return lm90_write_reg(data->client, LM90_REG_TCRIT_HYST, data->temp_hyst);
>  }
>  
> +static int lm90_set_temp_offset(struct lm90_data *data, int channel, long val)
> +{
> +	/* Both offset registers have the same resolution */
> +	int res = lm90_temp_get_resolution(data, REMOTE_OFFSET);
> +
> +	val = lm90_temp_to_reg(0, val, res);
> +
> +	if (channel == 1) {
> +		data->temp[REMOTE_OFFSET] = val;
> +		return lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> +				    LM90_REG_REMOTE_OFFSL, val);
> +	}
> +
> +	data->temp[REMOTE2_OFFSET] = val;
> +	return lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
> +			    LM90_REG_REMOTE2_OFFSL, val);
> +}
> +
>  static const u8 lm90_temp_index[MAX_CHANNELS] = {
>  	LOCAL_TEMP, REMOTE_TEMP, REMOTE2_TEMP
>  };
> @@ -1577,19 +1595,7 @@ static int lm90_temp_write(struct device *dev, u32 attr, int channel, long val)
>  				    channel, val);
>  		break;
>  	case hwmon_temp_offset:
> -		/* Both offset registers have the same resolution */
> -		val = lm90_temp_to_reg(0, val,
> -				       lm90_temp_get_resolution(data, REMOTE_OFFSET));
> -
> -		if (channel == 1) {
> -			data->temp[REMOTE_OFFSET] = val;
> -			err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> -					   LM90_REG_REMOTE_OFFSL, val);
> -		} else {
> -			data->temp[REMOTE2_OFFSET] = val;
> -			err = lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
> -					   LM90_REG_REMOTE2_OFFSL, val);
> -		}
> +		err = lm90_set_temp_offset(data, channel, val);

Any chance to come up with more unified handling of both channels ?

Thanks,
Guenter

>  		break;
>  	default:
>  		err = -EOPNOTSUPP;
> @@ -2651,6 +2657,7 @@ static int lm90_probe_channel_from_dt(struct i2c_client *client,
>  				      struct lm90_data *data)
>  {
>  	u32 id;
> +	s32 val;
>  	int err;
>  	struct device *dev = &client->dev;
>  
> @@ -2674,6 +2681,21 @@ static int lm90_probe_channel_from_dt(struct i2c_client *client,
>  	if (data->channel_label[id])
>  		data->channel_config[id] |= HWMON_T_LABEL;
>  
> +	err = of_property_read_s32(child, "temperature-offset-millicelsius", &val);
> +	if (!err) {
> +		if (id == 0) {
> +			dev_err(dev, "offset can't be set for internal channel\n");
> +			return -EINVAL;
> +		}
> +
> +		err = lm90_set_temp_offset(data, id, val);
> +		if (err) {
> +			dev_err(dev, "can't set offset %d for channel %d (%d)\n",
> +				val, id, err);
> +			return err;
> +		}
> +	}
> +
>  	return 0;
>  }
>  

Re: [PATCH 4/7] hwmon: (lm90) Add support for 2nd remote channel's offset register

[Slawomir Stepien]

On cze 05, 2022 11:03, Guenter Roeck wrote:
> On Wed, May 25, 2022 at 09:36:54AM +0200, Slawomir Stepien wrote:
> > From: Slawomir Stepien <slawomir.stepien@nokia.com>
> > 
> > The ADT7461 supports offset register for both remote channels it has.
> 
> ADT7481

Oops. I will fix that in new version.

> > Both registers have the same bit width (resolution).
> > 
> > In the code, this device has LM90_HAVE_TEMP3 and LM90_HAVE_OFFSET flags,
> > but the support of second remote channel's offset is missing. Add that
> > implementation.
> > 
> > Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> > ---
> >  drivers/hwmon/lm90.c | 37 ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 32 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 02b211a4e571..d226f1dea2ba 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -153,6 +153,8 @@ enum chips { adm1023, adm1032, adt7461, adt7461a, adt7481,
> >  #define LM90_REG_REMOTE_TEMPL		0x10
> >  #define LM90_REG_REMOTE_OFFSH		0x11
> >  #define LM90_REG_REMOTE_OFFSL		0x12
> > +#define LM90_REG_REMOTE2_OFFSH		0x34
> > +#define LM90_REG_REMOTE2_OFFSL		0x35
> 
> I don't think those are needed.

In lm90_temp_write() (unlike in lm90_update_limits()) the remote channel is *not* set. I find
setting it (the remote channel) in lm90_temp_write() a waste of xfers, if we can address the
registers directly. But if you prefer to have just one set of register and setting the remote
channel bit, then sure I can do it like that.

> >  #define LM90_REG_REMOTE_HIGHH		0x07
> >  #define LM90_REG_REMOTE_HIGHL		0x13
> >  #define LM90_REG_REMOTE_LOWH		0x08
> > @@ -669,6 +671,7 @@ enum lm90_temp_reg_index {
> >  	REMOTE2_TEMP,	/* max6695/96 only */
> >  	REMOTE2_LOW,	/* max6695/96 only */
> >  	REMOTE2_HIGH,	/* max6695/96 only */
> > +	REMOTE2_OFFSET,
> >  
> >  	TEMP_REG_NUM
> >  };
> > @@ -1024,6 +1027,14 @@ static int lm90_update_limits(struct device *dev)
> >  			return val;
> >  		data->temp[REMOTE2_HIGH] = val << 8;
> >  
> > +		if (data->flags & LM90_HAVE_OFFSET) {
> > +			val = lm90_read16(client, LM90_REG_REMOTE2_OFFSH,
> > +					  LM90_REG_REMOTE2_OFFSL, false);
> 
> Why not use LM90_REG_REMOTE_OFFSH and LM90_REG_REMOTE_OFFSL ?
> The remove channel is selected here, after all.

Yes, in this case I can use the regs for remote channel 1. I will fix that in new version.

> > +			if (val < 0)
> > +				return val;
> > +			data->temp[REMOTE2_OFFSET] = val;
> > +		}
> > +
> >  		lm90_select_remote_channel(data, false);
> >  	}
> >  
> > @@ -1294,6 +1305,7 @@ static int lm90_temp_get_resolution(struct lm90_data *data, int index)
> >  			return data->resolution;
> >  		return 8;
> >  	case REMOTE_OFFSET:
> > +	case REMOTE2_OFFSET:
> >  	case REMOTE2_TEMP:
> >  		return data->resolution;
> >  	case LOCAL_TEMP:
> > @@ -1515,8 +1527,13 @@ static int lm90_temp_read(struct device *dev, u32 attr, int channel, long *val)
> >  		*val = lm90_get_temphyst(data, lm90_temp_emerg_index[channel], channel);
> >  		break;
> >  	case hwmon_temp_offset:
> > -		*val = lm90_temp_from_reg(0, data->temp[REMOTE_OFFSET],
> > -					  lm90_temp_get_resolution(data, REMOTE_OFFSET));
> > +		/* Both offset registers have the same resolution */
> > +		int res = lm90_temp_get_resolution(data, REMOTE_OFFSET);
> > +
> > +		if (channel == 1)
> > +			*val = lm90_temp_from_reg(0, data->temp[REMOTE_OFFSET], res);
> > +		else
> > +			*val = lm90_temp_from_reg(0, data->temp[REMOTE2_OFFSET], res);
> 
> I would prefer the use of lm90_temp_offset_index[] as implemented for the
> other attributes.

OK, I will try to do it like that in new version.

> >  		break;
> >  	default:
> >  		return -EOPNOTSUPP;
> > @@ -1556,11 +1573,19 @@ static int lm90_temp_write(struct device *dev, u32 attr, int channel, long val)
> >  				    channel, val);
> >  		break;
> >  	case hwmon_temp_offset:
> > +		/* Both offset registers have the same resolution */
> >  		val = lm90_temp_to_reg(0, val,
> >  				       lm90_temp_get_resolution(data, REMOTE_OFFSET));
> > -		data->temp[REMOTE_OFFSET] = val;
> > -		err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> > -				   LM90_REG_REMOTE_OFFSL, val);
> > +
> > +		if (channel == 1) {
> > +			data->temp[REMOTE_OFFSET] = val;
> > +			err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> > +					   LM90_REG_REMOTE_OFFSL, val);
> > +		} else {
> > +			data->temp[REMOTE2_OFFSET] = val;
> > +			err = lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
> > +					   LM90_REG_REMOTE2_OFFSL, val);
> > +		}
> 
> Same as above.

OK!

-- 
Slawomir Stepien

Re: [PATCH 7/7] hwmon: (lm90) Read the channel's temperature offset from device-tree

[Slawomir Stepien]

On cze 05, 2022 11:10, Guenter Roeck wrote:
> On Wed, May 25, 2022 at 09:36:57AM +0200, Slawomir Stepien wrote:
> > From: Slawomir Stepien <slawomir.stepien@nokia.com>
> > 
> > Try to read the channel's temperature offset from device-tree. Having
> > offset in device-tree node is not mandatory. The offset can only be set
> > for remote channels.
> > 
> > Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> > ---
> >  drivers/hwmon/lm90.c | 48 ++++++++++++++++++++++++++++++++------------
> >  1 file changed, 35 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 3837c4ab5833..12e8e874f1b9 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -1440,6 +1440,24 @@ static int lm90_set_temphyst(struct lm90_data *data, long val)
> >  	return lm90_write_reg(data->client, LM90_REG_TCRIT_HYST, data->temp_hyst);
> >  }
> >  
> > +static int lm90_set_temp_offset(struct lm90_data *data, int channel, long val)
> > +{
> > +	/* Both offset registers have the same resolution */
> > +	int res = lm90_temp_get_resolution(data, REMOTE_OFFSET);
> > +
> > +	val = lm90_temp_to_reg(0, val, res);
> > +
> > +	if (channel == 1) {
> > +		data->temp[REMOTE_OFFSET] = val;
> > +		return lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> > +				    LM90_REG_REMOTE_OFFSL, val);
> > +	}
> > +
> > +	data->temp[REMOTE2_OFFSET] = val;
> > +	return lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
> > +			    LM90_REG_REMOTE2_OFFSL, val);
> > +}
> > +
> >  static const u8 lm90_temp_index[MAX_CHANNELS] = {
> >  	LOCAL_TEMP, REMOTE_TEMP, REMOTE2_TEMP
> >  };
> > @@ -1577,19 +1595,7 @@ static int lm90_temp_write(struct device *dev, u32 attr, int channel, long val)
> >  				    channel, val);
> >  		break;
> >  	case hwmon_temp_offset:
> > -		/* Both offset registers have the same resolution */
> > -		val = lm90_temp_to_reg(0, val,
> > -				       lm90_temp_get_resolution(data, REMOTE_OFFSET));
> > -
> > -		if (channel == 1) {
> > -			data->temp[REMOTE_OFFSET] = val;
> > -			err = lm90_write16(data->client, LM90_REG_REMOTE_OFFSH,
> > -					   LM90_REG_REMOTE_OFFSL, val);
> > -		} else {
> > -			data->temp[REMOTE2_OFFSET] = val;
> > -			err = lm90_write16(data->client, LM90_REG_REMOTE2_OFFSH,
> > -					   LM90_REG_REMOTE2_OFFSL, val);
> > -		}
> > +		err = lm90_set_temp_offset(data, channel, val);
> 
> Any chance to come up with more unified handling of both channels ?

I will give it a try.

-- 
Slawomir Stepien

Re: [PATCH 4/7] hwmon: (lm90) Add support for 2nd remote channel's offset register

[Guenter Roeck]

On 6/5/22 23:30, Slawomir Stepien wrote:
> On cze 05, 2022 11:03, Guenter Roeck wrote:
>> On Wed, May 25, 2022 at 09:36:54AM +0200, Slawomir Stepien wrote:
>>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>>
>>> The ADT7461 supports offset register for both remote channels it has.
>>
>> ADT7481
> 
> Oops. I will fix that in new version.
> 
>>> Both registers have the same bit width (resolution).
>>>
>>> In the code, this device has LM90_HAVE_TEMP3 and LM90_HAVE_OFFSET flags,
>>> but the support of second remote channel's offset is missing. Add that
>>> implementation.
>>>
>>> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
>>> ---
>>>   drivers/hwmon/lm90.c | 37 ++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 32 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>> index 02b211a4e571..d226f1dea2ba 100644
>>> --- a/drivers/hwmon/lm90.c
>>> +++ b/drivers/hwmon/lm90.c
>>> @@ -153,6 +153,8 @@ enum chips { adm1023, adm1032, adt7461, adt7461a, adt7481,
>>>   #define LM90_REG_REMOTE_TEMPL		0x10
>>>   #define LM90_REG_REMOTE_OFFSH		0x11
>>>   #define LM90_REG_REMOTE_OFFSL		0x12
>>> +#define LM90_REG_REMOTE2_OFFSH		0x34
>>> +#define LM90_REG_REMOTE2_OFFSL		0x35
>>
>> I don't think those are needed.
> 
> In lm90_temp_write() (unlike in lm90_update_limits()) the remote channel is *not* set. I find

... unless lm90_set_temp() is used to write the values. If I recall correctly
I didn't do that because selecting the remote channel seemed unnecessary.

> setting it (the remote channel) in lm90_temp_write() a waste of xfers, if we can address the
> registers directly. But if you prefer to have just one set of register and setting the remote
> channel bit, then sure I can do it like that.
> 
It isn't as if setting the offset happens all the time, so I'd prefer
to use lm90_set_temp() if that is possible.

Thanks,
Guenter

Re: [PATCH 4/7] hwmon: (lm90) Add support for 2nd remote channel's offset register

[Slawomir Stepien]

On cze 05, 2022 23:50, Guenter Roeck wrote:
> On 6/5/22 23:30, Slawomir Stepien wrote:
> > On cze 05, 2022 11:03, Guenter Roeck wrote:
> > > On Wed, May 25, 2022 at 09:36:54AM +0200, Slawomir Stepien wrote:
> > > > From: Slawomir Stepien <slawomir.stepien@nokia.com>
> > > > 
> > > > The ADT7461 supports offset register for both remote channels it has.
> > > 
> > > ADT7481
> > 
> > Oops. I will fix that in new version.
> > 
> > > > Both registers have the same bit width (resolution).
> > > > 
> > > > In the code, this device has LM90_HAVE_TEMP3 and LM90_HAVE_OFFSET flags,
> > > > but the support of second remote channel's offset is missing. Add that
> > > > implementation.
> > > > 
> > > > Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> > > > ---
> > > >   drivers/hwmon/lm90.c | 37 ++++++++++++++++++++++++++++++++-----
> > > >   1 file changed, 32 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > > > index 02b211a4e571..d226f1dea2ba 100644
> > > > --- a/drivers/hwmon/lm90.c
> > > > +++ b/drivers/hwmon/lm90.c
> > > > @@ -153,6 +153,8 @@ enum chips { adm1023, adm1032, adt7461, adt7461a, adt7481,
> > > >   #define LM90_REG_REMOTE_TEMPL		0x10
> > > >   #define LM90_REG_REMOTE_OFFSH		0x11
> > > >   #define LM90_REG_REMOTE_OFFSL		0x12
> > > > +#define LM90_REG_REMOTE2_OFFSH		0x34
> > > > +#define LM90_REG_REMOTE2_OFFSL		0x35
> > > 
> > > I don't think those are needed.
> > 
> > In lm90_temp_write() (unlike in lm90_update_limits()) the remote channel is *not* set. I find
> 
> ... unless lm90_set_temp() is used to write the values. If I recall correctly
> I didn't do that because selecting the remote channel seemed unnecessary.
> 
> > setting it (the remote channel) in lm90_temp_write() a waste of xfers, if we can address the
> > registers directly. But if you prefer to have just one set of register and setting the remote
> > channel bit, then sure I can do it like that.
> > 
> It isn't as if setting the offset happens all the time, so I'd prefer
> to use lm90_set_temp() if that is possible.

Good point! So I will use just one set of the registers.

-- 
Slawomir Stepien

Re: [PATCH 4/7] hwmon: (lm90) Add support for 2nd remote channel's offset register

[Slawomir Stepien]

On cze 05, 2022 23:50, Guenter Roeck wrote:
> On 6/5/22 23:30, Slawomir Stepien wrote:
> > On cze 05, 2022 11:03, Guenter Roeck wrote:
> > > On Wed, May 25, 2022 at 09:36:54AM +0200, Slawomir Stepien wrote:
> > > > From: Slawomir Stepien <slawomir.stepien@nokia.com>
> > > > 
> > > > The ADT7461 supports offset register for both remote channels it has.
> > > 
> > > ADT7481
> > 
> > Oops. I will fix that in new version.
> > 
> > > > Both registers have the same bit width (resolution).
> > > > 
> > > > In the code, this device has LM90_HAVE_TEMP3 and LM90_HAVE_OFFSET flags,
> > > > but the support of second remote channel's offset is missing. Add that
> > > > implementation.
> > > > 
> > > > Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> > > > ---
> > > >   drivers/hwmon/lm90.c | 37 ++++++++++++++++++++++++++++++++-----
> > > >   1 file changed, 32 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > > > index 02b211a4e571..d226f1dea2ba 100644
> > > > --- a/drivers/hwmon/lm90.c
> > > > +++ b/drivers/hwmon/lm90.c
> > > > @@ -153,6 +153,8 @@ enum chips { adm1023, adm1032, adt7461, adt7461a, adt7481,
> > > >   #define LM90_REG_REMOTE_TEMPL		0x10
> > > >   #define LM90_REG_REMOTE_OFFSH		0x11
> > > >   #define LM90_REG_REMOTE_OFFSL		0x12
> > > > +#define LM90_REG_REMOTE2_OFFSH		0x34
> > > > +#define LM90_REG_REMOTE2_OFFSL		0x35
> > > 
> > > I don't think those are needed.
> > 
> > In lm90_temp_write() (unlike in lm90_update_limits()) the remote channel is *not* set. I find
> 
> ... unless lm90_set_temp() is used to write the values. If I recall correctly
> I didn't do that because selecting the remote channel seemed unnecessary.

I think that modifying lm90_set_temp() to support offsets is a bit messy:

1. The offset on all supported devices is always on two bytes. Unlike the temperature, where
sometimes it is just on one (but if more than one byte, then we set reg_remote_ext). With this also
'regs' in lm90_set_temp() will be back as 2 dimensional array OR additional high and low indexes for
REMOTE_OFFSET and REMOTE2_OFFSET should be added (that will also caused bits glueing on write/read).

2. For offset the calls lm90_temp_from/to_reg should have 0 as flags (1st argument) - that would be
an additional if in lm90_set_temp() OR clear&restore of the flags before&after the call..

Maybe, Guenter you will be happy with something like this (new functions):

static int lm90_get_temp_offset(struct lm90_data *data, int index)
{
	int res = lm90_temp_get_resolution(data, index);

	return lm90_temp_from_reg(0, data->temp[index], res);
}

static int lm90_set_temp_offset(struct lm90_data *data, int index, int channel, long val)
{
	int err;
	static const u8 regs[][2] = {
		[REMOTE_OFFSET] = {LM90_REG_REMOTE_OFFSH, LM90_REG_REMOTE_OFFSL},
		[REMOTE2_OFFSET] = {LM90_REG_REMOTE_OFFSH, LM90_REG_REMOTE_OFFSL},
	};
	u8 regh = regs[index][0];
	u8 regl = regs[index][1];

	val = lm90_temp_to_reg(0, val, lm90_temp_get_resolution(data, index));

	if (channel > 1)
		lm90_select_remote_channel(data, true);

	err = lm90_write16(data->client, regh, regl, val);

	if (channel > 1)
		lm90_select_remote_channel(data, false);

	if (err)
		return err;

	data->temp[index] = val;

	return 0;
}

And new channel->index translator:

static const s8 lm90_temp_offset_index[MAX_CHANNELS] = {
	-1, REMOTE_OFFSET, REMOTE2_OFFSET
};

Having that, we can use that functions in hwmon's read/write attrs but also while paring the
device-tree channel nodes.

Maybe I missed something and using lm90_set_temp() will not be messy?
What do you think?

-- 
Slawomir Stepien

Re: [PATCH 4/7] hwmon: (lm90) Add support for 2nd remote channel's offset register

[Guenter Roeck]

On 6/6/22 05:14, Slawomir Stepien wrote:
> On cze 05, 2022 23:50, Guenter Roeck wrote:
>> On 6/5/22 23:30, Slawomir Stepien wrote:
>>> On cze 05, 2022 11:03, Guenter Roeck wrote:
>>>> On Wed, May 25, 2022 at 09:36:54AM +0200, Slawomir Stepien wrote:
>>>>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>>>>
>>>>> The ADT7461 supports offset register for both remote channels it has.
>>>>
>>>> ADT7481
>>>
>>> Oops. I will fix that in new version.
>>>
>>>>> Both registers have the same bit width (resolution).
>>>>>
>>>>> In the code, this device has LM90_HAVE_TEMP3 and LM90_HAVE_OFFSET flags,
>>>>> but the support of second remote channel's offset is missing. Add that
>>>>> implementation.
>>>>>
>>>>> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
>>>>> ---
>>>>>    drivers/hwmon/lm90.c | 37 ++++++++++++++++++++++++++++++++-----
>>>>>    1 file changed, 32 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>>>> index 02b211a4e571..d226f1dea2ba 100644
>>>>> --- a/drivers/hwmon/lm90.c
>>>>> +++ b/drivers/hwmon/lm90.c
>>>>> @@ -153,6 +153,8 @@ enum chips { adm1023, adm1032, adt7461, adt7461a, adt7481,
>>>>>    #define LM90_REG_REMOTE_TEMPL		0x10
>>>>>    #define LM90_REG_REMOTE_OFFSH		0x11
>>>>>    #define LM90_REG_REMOTE_OFFSL		0x12
>>>>> +#define LM90_REG_REMOTE2_OFFSH		0x34
>>>>> +#define LM90_REG_REMOTE2_OFFSL		0x35
>>>>
>>>> I don't think those are needed.
>>>
>>> In lm90_temp_write() (unlike in lm90_update_limits()) the remote channel is *not* set. I find
>>
>> ... unless lm90_set_temp() is used to write the values. If I recall correctly
>> I didn't do that because selecting the remote channel seemed unnecessary.
> 
> I think that modifying lm90_set_temp() to support offsets is a bit messy:
> 
> 1. The offset on all supported devices is always on two bytes. Unlike the temperature, where
> sometimes it is just on one (but if more than one byte, then we set reg_remote_ext). With this also
> 'regs' in lm90_set_temp() will be back as 2 dimensional array OR additional high and low indexes for
> REMOTE_OFFSET and REMOTE2_OFFSET should be added (that will also caused bits glueing on write/read).
> 
> 2. For offset the calls lm90_temp_from/to_reg should have 0 as flags (1st argument) - that would be
> an additional if in lm90_set_temp() OR clear&restore of the flags before&after the call..
> 
> Maybe, Guenter you will be happy with something like this (new functions):
> 
> static int lm90_get_temp_offset(struct lm90_data *data, int index)
> {
> 	int res = lm90_temp_get_resolution(data, index);
> 
> 	return lm90_temp_from_reg(0, data->temp[index], res);
> }
> 
> static int lm90_set_temp_offset(struct lm90_data *data, int index, int channel, long val)
> {
> 	int err;
> 	static const u8 regs[][2] = {
> 		[REMOTE_OFFSET] = {LM90_REG_REMOTE_OFFSH, LM90_REG_REMOTE_OFFSL},
> 		[REMOTE2_OFFSET] = {LM90_REG_REMOTE_OFFSH, LM90_REG_REMOTE_OFFSL},
> 	};

That array is unnecessary.
> 	u8 regh = regs[index][0];
> 	u8 regl = regs[index][1];

	regh = regs[0];
	regl = regs[1];

has the same result, meaning you can just use LM90_REG_REMOTE_OFFSH
and LM90_REG_REMOTE_OFFSL directly.

... and then you can just use the direct registers and add a comment stating
that those only work for ADT7481 and not for chips which require channel select.

> 
> 	val = lm90_temp_to_reg(0, val, lm90_temp_get_resolution(data, index));
> 
> 	if (channel > 1)
> 		lm90_select_remote_channel(data, true);
> 
> 	err = lm90_write16(data->client, regh, regl, val);
> 
> 	if (channel > 1)
> 		lm90_select_remote_channel(data, false);
> 
> 	if (err)
> 		return err;
> 
> 	data->temp[index] = val;
> 
> 	return 0;
> }
> 
> And new channel->index translator:
> 
> static const s8 lm90_temp_offset_index[MAX_CHANNELS] = {
> 	-1, REMOTE_OFFSET, REMOTE2_OFFSET
> };
> 
> Having that, we can use that functions in hwmon's read/write attrs but also while paring the
> device-tree channel nodes.
> 
> Maybe I missed something and using lm90_set_temp() will not be messy?
> What do you think?
> 
Using a new (simplified) function to write the registers sounds good.
Go for it.

Thanks,
Guenter