[PATCH v7 1/2] dt-bindings: hwmon: add tmp464.yaml

[PATCH v7 1/2] dt-bindings: hwmon: add tmp464.yaml

[Guenter Roeck]

From: Agathe Porte <agathe.porte@nokia.com>

Add basic description of the tmp464 driver DT bindings.

Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v7:
- No change

v6:
- Model ti,n-factor as int32 instead of int8.

v5:
- Dropped ti,n-factor from channel@0 example. Added additional
  channel to examples to show positive ti,n-factor property.

v4:
- No changes

v3:
- Addedd support for TMP468.
- Changed number of channels from 0..3 (which was wrong anyway) to 0..8.
- Changed value range for ti,n-factor to int8, with an example for
  a negative value.
- Added myself as driver maintainer.

 .../devicetree/bindings/hwmon/ti,tmp464.yaml  | 114 ++++++++++++++++++
 MAINTAINERS                                   |   7 ++
 2 files changed, 121 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
new file mode 100644
index 000000000000..801ca9ba7d34
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ti,tmp464.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TMP464 and TMP468 temperature sensors
+
+maintainers:
+  - Agathe Porte <agathe.porte@nokia.com>
+
+description: |
+  ±0.0625°C Remote and Local temperature sensor
+  https://www.ti.com/lit/ds/symlink/tmp464.pdf
+  https://www.ti.com/lit/ds/symlink/tmp468.pdf
+
+properties:
+  compatible:
+    enum:
+      - ti,tmp464
+      - ti,tmp468
+
+  reg:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+patternProperties:
+  "^channel@([0-8])$":
+    type: object
+    description: |
+      Represents channels of the device and their specific configuration.
+
+    properties:
+      reg:
+        description: |
+          The channel number. 0 is local channel, 1-8 are remote channels.
+        items:
+          minimum: 0
+          maximum: 8
+
+      label:
+        description: |
+          A descriptive name for this channel, like "ambient" or "psu".
+
+      ti,n-factor:
+        description: |
+          The value (two's complement) to be programmed in the channel specific N correction register.
+          For remote channels only.
+        $ref: /schemas/types.yaml#/definitions/int32
+        items:
+          minimum: -128
+          maximum: 127
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      sensor@4b {
+        compatible = "ti,tmp464";
+        reg = <0x4b>;
+      };
+    };
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      sensor@4b {
+        compatible = "ti,tmp464";
+        reg = <0x4b>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+          reg = <0x0>;
+          label = "local";
+        };
+
+        channel@1 {
+          reg = <0x1>;
+          ti,n-factor = <(-10)>;
+          label = "external";
+        };
+
+        channel@2 {
+          reg = <0x2>;
+          ti,n-factor = <0x10>;
+          label = "somelabel";
+        };
+
+        channel@3 {
+          reg = <0x3>;
+          status = "disabled";
+        };
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index fca970a46e77..f51bc7c7e439 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19489,6 +19489,13 @@ S:	Maintained
 F:	Documentation/hwmon/tmp401.rst
 F:	drivers/hwmon/tmp401.c
 
+TMP464 HARDWARE MONITOR DRIVER
+M:	Agathe Porte <agathe.porte@nokia.com>
+M:	Guenter Roeck <linux@roeck-us.net>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
+
 TMP513 HARDWARE MONITOR DRIVER
 M:	Eric Tremblay <etremblay@distech-controls.com>
 L:	linux-hwmon@vger.kernel.org
-- 
2.35.1

[PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

[Guenter Roeck]

Add support for Texas Instruments TMP464 and TMP468 temperature sensor
ICs.

TI's TMP464 is an I2C temperature sensor chip. This chip is similar
to TI's TMP421 chip, but with 16bit-wide registers (instead of
8bit-wide registers). The chip has one local sensor and four remote
sensors. TMP468 is similar to TMP464 but has one local and eight
remote sensors.

Originally-from: Agathe Porte <agathe.porte@nokia.com>
Cc: Agathe Porte <agathe.porte@nokia.com>
Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v7:
- Added sysfs tempX_enable attributes.
- Fixed bad return in temperature read function
  which causes a hangup on disabled channels.

v6:
- Found that the channel open status register is cleared on read
  and needs to be cached. Since the cache duration depends on the
  update interval, store the update interval locally and report the
  stored value instead of re-reading it from the chip.
- Reworked read and write functions to always acquire the update
  mutex to streamline the code.
- Read ti,n-factor property as s32.

v5:
- Dropped unused variable

v4:
- Fixed reading n-factor information from devicetree
  [Use of_property_read_u8 instead of of_property_read_s32 to read the
   property value, and write n-factor value into the upper 8 bit of the
   n-factor register]
- Fixed displaying label attributes

v3:
- Added support for TMP468
- Added support for various limits, temperature hysteresis, alarm attributes,
  and update interval
- Use regmap instead of local caching
- Use static chip configuration
- Unlock check if needed when loading driver, and lock it when unloading it
  - Call tmp464_init_client() before calling tmp464_probe_from_dt()
    since the latter changes registers, which requires the chip to be
    unlocked.
- Restore configuration register when unloading driver
- ti,n-factor is optional, so don't fail if the property is not present

Notes:
- Tested with TMP464 and TMP468. Module tested for both chips.
  Devicetree code tested with rudimentary qemu implementation.

 Documentation/hwmon/index.rst  |   1 +
 Documentation/hwmon/tmp464.rst |  73 ++++
 MAINTAINERS                    |   2 +
 drivers/hwmon/Kconfig          |  11 +
 drivers/hwmon/Makefile         |   1 +
 drivers/hwmon/tmp464.c         | 712 +++++++++++++++++++++++++++++++++
 6 files changed, 800 insertions(+)
 create mode 100644 Documentation/hwmon/tmp464.rst
 create mode 100644 drivers/hwmon/tmp464.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index df20022c741f..37590db85e65 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -193,6 +193,7 @@ Hardware Monitoring Kernel Drivers
    tmp108
    tmp401
    tmp421
+   tmp464
    tmp513
    tps23861
    tps40422
diff --git a/Documentation/hwmon/tmp464.rst b/Documentation/hwmon/tmp464.rst
new file mode 100644
index 000000000000..7596e7623d06
--- /dev/null
+++ b/Documentation/hwmon/tmp464.rst
@@ -0,0 +1,73 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver tmp464
+====================
+
+Supported chips:
+
+  * Texas Instruments TMP464
+
+    Prefix: 'tmp464'
+
+    Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
+
+    Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp464.html
+
+  * Texas Instruments TMP468
+
+    Prefix: 'tmp468'
+
+    Addresses scanned: I2C 0x48, 0x49, 0x4a and 0x4b
+
+    Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp468.html
+
+Authors:
+
+	Agathe Porte <agathe.porte@nokia.com>
+	Guenter Roeck <linux@roeck-us.net>
+
+Description
+-----------
+
+This driver implements support for Texas Instruments TMP464 and TMP468
+temperature sensor chips. TMP464 provides one local and four remote
+sensors. TMP468 provides one local and eight remote sensors.
+Temperature is measured in degrees Celsius. The chips are wired over
+I2C/SMBus and specified over a temperature range of -40 to +125 degrees
+Celsius. Resolution for both the local and remote channels is 0.0625
+degree C.
+
+The chips support only temperature measurements. The driver exports
+temperature values, limits, and alarms via the following sysfs files:
+
+**temp[1-9]_input**
+
+**temp[1-9]_max**
+
+**temp[1-9]_max_hyst**
+
+**temp[1-9]_max_alarm**
+
+**temp[1-9]_crit**
+
+**temp[1-9]_crit_alarm**
+
+**temp[1-9]_crit_hyst**
+
+**temp[2-9]_offset**
+
+**temp[2-9]_fault**
+
+Each sensor can be individually disabled via Devicetree or from sysfs
+via:
+
+**temp[1-9]_enable**
+
+If labels were specified in Devicetree, additional sysfs files will
+be present:
+
+**temp[1-9]_label**
+
+The update interval is configurable with the following sysfs attribute.
+
+**update_interval**
diff --git a/MAINTAINERS b/MAINTAINERS
index f51bc7c7e439..9b51bf5ca3b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19495,6 +19495,8 @@ M:	Guenter Roeck <linux@roeck-us.net>
 L:	linux-hwmon@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
+F:	Documentation/hwmon/tmp464.rst
+F:	drivers/hwmon/tmp464.c
 
 TMP513 HARDWARE MONITOR DRIVER
 M:	Eric Tremblay <etremblay@distech-controls.com>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 8df25f1079ba..b84932fd0c13 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1979,6 +1979,17 @@ config SENSORS_TMP421
 	  This driver can also be built as a module. If so, the module
 	  will be called tmp421.
 
+config SENSORS_TMP464
+	tristate "Texas Instruments TMP464 and compatible"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for Texas Instruments TMP464
+	  and TMP468 temperature sensor chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called tmp464.
+
 config SENSORS_TMP513
 	tristate "Texas Instruments TMP513 and compatibles"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 185f946d698b..a1f2d6686227 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_TMP103)	+= tmp103.o
 obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
 obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
 obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
+obj-$(CONFIG_SENSORS_TMP464)	+= tmp464.o
 obj-$(CONFIG_SENSORS_TMP513)	+= tmp513.o
 obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress-hwmon.o
 obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
new file mode 100644
index 000000000000..7814f39bd1a3
--- /dev/null
+++ b/drivers/hwmon/tmp464.c
@@ -0,0 +1,712 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* Driver for the Texas Instruments TMP464 SMBus temperature sensor IC.
+ * Supported models: TMP464, TMP468
+
+ * Copyright (C) 2022 Agathe Porte <agathe.porte@nokia.com>
+ * Preliminary support by:
+ * Lionel Pouliquen <lionel.lp.pouliquen@nokia.com>
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b, I2C_CLIENT_END };
+
+#define TMP464_NUM_CHANNELS		5	/* chan 0 is internal, 1-4 are remote */
+#define TMP468_NUM_CHANNELS		9	/* chan 0 is internal, 1-8 are remote */
+
+#define MAX_CHANNELS			9
+
+#define TMP464_TEMP_REG(channel)	(channel)
+#define TMP464_TEMP_OFFSET_REG(channel)	(0x40 + ((channel) - 1) * 8)
+#define TMP464_N_FACTOR_REG(channel)	(0x41 + ((channel) - 1) * 8)
+
+static const u8 TMP464_THERM_LIMIT[MAX_CHANNELS] = {
+	0x39, 0x42, 0x4A, 0x52, 0x5A, 0x62, 0x6a, 0x72, 0x7a };
+static const u8 TMP464_THERM2_LIMIT[MAX_CHANNELS] = {
+	0x3A, 0x43, 0x4B, 0x53, 0x5B, 0x63, 0x6b, 0x73, 0x7b };
+
+#define TMP464_THERM_STATUS_REG			0x21
+#define TMP464_THERM2_STATUS_REG		0x22
+#define TMP464_REMOTE_OPEN_REG			0x23
+#define TMP464_CONFIG_REG			0x30
+#define TMP464_TEMP_HYST_REG			0x38
+#define TMP464_LOCK_REG				0xc4
+
+/* Identification */
+#define TMP464_MANUFACTURER_ID_REG		0xFE
+#define TMP464_DEVICE_ID_REG			0xFF
+
+/* Flags */
+#define TMP464_CONFIG_SHUTDOWN			BIT(5)
+#define TMP464_CONFIG_RANGE			0x04
+#define TMP464_CONFIG_REG_REN(x)		(BIT(7 + (x)))
+#define TMP464_CONFIG_REG_REN_MASK		GENMASK(15, 7)
+#define TMP464_CONFIG_CONVERSION_RATE_B0	2
+#define TMP464_CONFIG_CONVERSION_RATE_B2	4
+#define TMP464_CONFIG_CONVERSION_RATE_MASK      GENMASK(TMP464_CONFIG_CONVERSION_RATE_B2, \
+							TMP464_CONFIG_CONVERSION_RATE_B0)
+
+#define TMP464_UNLOCK_VAL			0xeb19
+#define TMP464_LOCK_VAL				0x5ca6
+#define TMP464_LOCKED				0x8000
+
+/* Manufacturer / Device ID's */
+#define TMP464_MANUFACTURER_ID			0x5449
+#define TMP464_DEVICE_ID			0x1468
+#define TMP468_DEVICE_ID			0x0468
+
+static const struct i2c_device_id tmp464_id[] = {
+	{ "tmp464", TMP464_NUM_CHANNELS },
+	{ "tmp468", TMP468_NUM_CHANNELS },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tmp464_id);
+
+static const struct of_device_id __maybe_unused tmp464_of_match[] = {
+	{
+		.compatible = "ti,tmp464",
+		.data = (void *)TMP464_NUM_CHANNELS
+	},
+	{
+		.compatible = "ti,tmp468",
+		.data = (void *)TMP468_NUM_CHANNELS
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, tmp464_of_match);
+
+struct tmp464_channel {
+	const char *label;
+	bool enabled;
+};
+
+struct tmp464_data {
+	struct regmap *regmap;
+	struct mutex update_lock;
+	int channels;
+	s16 config_orig;
+	u16 open_reg;
+	unsigned long last_updated;
+	bool valid;
+	int update_interval;
+	struct tmp464_channel channel[MAX_CHANNELS];
+};
+
+static int temp_from_reg(s16 reg)
+{
+	return DIV_ROUND_CLOSEST((reg >> 3) * 625, 10);
+}
+
+static s16 temp_to_limit_reg(long temp)
+{
+	return DIV_ROUND_CLOSEST(temp, 500) << 6;
+}
+
+static s16 temp_to_offset_reg(long temp)
+{
+	return DIV_ROUND_CLOSEST(temp * 10, 625) << 3;
+}
+
+static int tmp464_enable_channels(struct tmp464_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	u16 enable = 0;
+	int i;
+
+	for (i = 0; i < data->channels; i++)
+		if (data->channel[i].enabled)
+			enable |= TMP464_CONFIG_REG_REN(i);
+
+	return regmap_update_bits(regmap, TMP464_CONFIG_REG, TMP464_CONFIG_REG_REN_MASK, enable);
+}
+
+static int tmp464_chip_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct tmp464_data *data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		*val = data->update_interval;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct tmp464_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval, regval2;
+	int err = 0;
+
+	mutex_lock(&data->update_lock);
+
+	switch (attr) {
+	case hwmon_temp_max_alarm:
+		err = regmap_read(regmap, TMP464_THERM_STATUS_REG, &regval);
+		if (err < 0)
+			break;
+		*val = !!(regval & BIT(channel + 7));
+		break;
+	case hwmon_temp_crit_alarm:
+		err = regmap_read(regmap, TMP464_THERM2_STATUS_REG, &regval);
+		if (err < 0)
+			break;
+		*val = !!(regval & BIT(channel + 7));
+		break;
+	case hwmon_temp_fault:
+		/*
+		 * The chip clears TMP464_REMOTE_OPEN_REG after it is read
+		 * and only updates it after the next measurement cycle is
+		 * complete. That means we have to cache the value internally
+		 * for one measurement cycle and report the cached value.
+		 */
+		if (!data->valid || time_after(jiffies, data->last_updated +
+					       msecs_to_jiffies(data->update_interval))) {
+			err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, &regval);
+			if (err < 0)
+				break;
+			data->open_reg = regval;
+			data->last_updated = jiffies;
+			data->valid = true;
+		}
+		*val = !!(data->open_reg & BIT(channel + 7));
+		break;
+	case hwmon_temp_max_hyst:
+		err = regmap_read(regmap, TMP464_THERM_LIMIT[channel], &regval);
+		if (err < 0)
+			break;
+		err = regmap_read(regmap, TMP464_TEMP_HYST_REG, &regval2);
+		if (err < 0)
+			break;
+		regval -= regval2;
+		*val = temp_from_reg(regval);
+		break;
+	case hwmon_temp_max:
+		err = regmap_read(regmap, TMP464_THERM_LIMIT[channel], &regval);
+		if (err < 0)
+			break;
+		*val = temp_from_reg(regval);
+		break;
+	case hwmon_temp_crit_hyst:
+		err = regmap_read(regmap, TMP464_THERM2_LIMIT[channel], &regval);
+		if (err < 0)
+			break;
+		err = regmap_read(regmap, TMP464_TEMP_HYST_REG, &regval2);
+		if (err < 0)
+			break;
+		regval -= regval2;
+		*val = temp_from_reg(regval);
+		break;
+	case hwmon_temp_crit:
+		err = regmap_read(regmap, TMP464_THERM2_LIMIT[channel], &regval);
+		if (err < 0)
+			break;
+		*val = temp_from_reg(regval);
+		break;
+	case hwmon_temp_offset:
+		err = regmap_read(regmap, TMP464_TEMP_OFFSET_REG(channel), &regval);
+		if (err < 0)
+			break;
+		*val = temp_from_reg(regval);
+		break;
+	case hwmon_temp_input:
+		if (!data->channel[channel].enabled) {
+			err = -ENODATA;
+			break;
+		}
+		err = regmap_read(regmap, TMP464_TEMP_REG(channel), &regval);
+		if (err < 0)
+			break;
+		*val = temp_from_reg(regval);
+		break;
+	case hwmon_temp_enable:
+		*val = data->channel[channel].enabled;
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return err;
+}
+
+static int tmp464_read(struct device *dev, enum hwmon_sensor_types type,
+		       u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_chip:
+		return tmp464_chip_read(dev, attr, channel, val);
+	case hwmon_temp:
+		return tmp464_temp_read(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tmp464_read_string(struct device *dev, enum hwmon_sensor_types type,
+			      u32 attr, int channel, const char **str)
+{
+	struct tmp464_data *data = dev_get_drvdata(dev);
+
+	*str = data->channel[channel].label;
+
+	return 0;
+}
+
+static int tmp464_set_convrate(struct tmp464_data *data, long interval)
+{
+	int rate;
+
+	/*
+	 * For valid rates, interval in milli-seconds can be calculated as
+	 *      interval = 125 << (7 - rate);
+	 * or
+	 *      interval = (1 << (7 - rate)) * 125;
+	 * The rate is therefore
+	 *      rate = 7 - __fls(interval / 125);
+	 * and the rounded rate is
+	 *      rate = 7 - __fls(interval * 4 / (125 * 3));
+	 * Use clamp_val() to avoid overflows, and to ensure valid input
+	 * for __fls.
+	 */
+	interval = clamp_val(interval, 125, 16000);
+	rate = 7 - __fls(interval * 4 / (125 * 3));
+	data->update_interval = 125 << (7 - rate);
+
+	return regmap_update_bits(data->regmap, TMP464_CONFIG_REG,
+				  TMP464_CONFIG_CONVERSION_RATE_MASK,
+				  rate << TMP464_CONFIG_CONVERSION_RATE_B0);
+}
+
+static int tmp464_chip_write(struct tmp464_data *data, u32 attr, int channel, long val)
+{
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		return tmp464_set_convrate(data, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tmp464_temp_write(struct tmp464_data *data, u32 attr, int channel, long val)
+{
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int err = 0;
+
+	switch (attr) {
+	case hwmon_temp_max_hyst:
+		err = regmap_read(regmap, TMP464_THERM_LIMIT[0], &regval);
+		if (err < 0)
+			break;
+		val = clamp_val(val, -256000, 256000);	/* prevent overflow/underflow */
+		val = clamp_val(temp_from_reg(regval) - val, 0, 255000);
+		err = regmap_write(regmap, TMP464_TEMP_HYST_REG,
+				   DIV_ROUND_CLOSEST(val, 1000) << 7);
+		break;
+	case hwmon_temp_max:
+		val = temp_to_limit_reg(clamp_val(val, -255000, 255500));
+		err = regmap_write(regmap, TMP464_THERM_LIMIT[channel], val);
+		break;
+	case hwmon_temp_crit:
+		val = temp_to_limit_reg(clamp_val(val, -255000, 255500));
+		err = regmap_write(regmap, TMP464_THERM2_LIMIT[channel], val);
+		break;
+	case hwmon_temp_offset:
+		val = temp_to_offset_reg(clamp_val(val, -128000, 127937));
+		err = regmap_write(regmap, TMP464_TEMP_OFFSET_REG(channel), val);
+		break;
+	case hwmon_temp_enable:
+		data->channel[channel].enabled = !!val;
+		err = tmp464_enable_channels(data);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
+static int tmp464_write(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long val)
+{
+	struct tmp464_data *data = dev_get_drvdata(dev);
+	int err;
+
+	mutex_lock(&data->update_lock);
+
+	switch (type) {
+	case hwmon_chip:
+		err = tmp464_chip_write(data, attr, channel, val);
+		break;
+	case hwmon_temp:
+		err = tmp464_temp_write(data, attr, channel, val);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return err;
+}
+
+static umode_t tmp464_is_visible(const void *_data, enum hwmon_sensor_types type,
+				 u32 attr, int channel)
+{
+	const struct tmp464_data *data = _data;
+
+	if (channel >= data->channels)
+		return 0;
+
+	if (type == hwmon_chip) {
+		if (attr == hwmon_chip_update_interval)
+			return 0644;
+		return 0;
+	}
+
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_crit_alarm:
+	case hwmon_temp_crit_hyst:
+		return 0444;
+	case hwmon_temp_enable:
+	case hwmon_temp_max:
+	case hwmon_temp_crit:
+		return 0644;
+	case hwmon_temp_max_hyst:
+		if (!channel)
+			return 0644;
+		return 0444;
+	case hwmon_temp_label:
+		if (data->channel[channel].label)
+			return 0444;
+		return 0;
+	case hwmon_temp_fault:
+		if (channel)
+			return 0444;
+		return 0;
+	case hwmon_temp_offset:
+		if (channel)
+			return 0644;
+		return 0;
+	default:
+		return 0;
+	}
+}
+
+static void tmp464_restore_lock(void *regmap)
+{
+	regmap_write(regmap, TMP464_LOCK_REG, TMP464_LOCK_VAL);
+}
+
+static void tmp464_restore_config(void *_data)
+{
+	struct tmp464_data *data = _data;
+
+	regmap_write(data->regmap, TMP464_CONFIG_REG, data->config_orig);
+}
+
+static int tmp464_init_client(struct device *dev, struct tmp464_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int err;
+
+	err = regmap_read(regmap, TMP464_LOCK_REG, &regval);
+	if (err)
+		return err;
+	if (regval == TMP464_LOCKED) {
+		/* Explicitly unlock chip if it is locked */
+		err = regmap_write(regmap, TMP464_LOCK_REG, TMP464_UNLOCK_VAL);
+		if (err)
+			return err;
+		/* and lock it again when unloading the driver */
+		err = devm_add_action_or_reset(dev, tmp464_restore_lock, regmap);
+		if (err)
+			return err;
+	}
+
+	err = regmap_read(regmap, TMP464_CONFIG_REG, &regval);
+	if (err)
+		return err;
+	data->config_orig = regval;
+	err = devm_add_action_or_reset(dev, tmp464_restore_config, data);
+	if (err)
+		return err;
+
+	/* Default to 500 ms update interval */
+	err = regmap_update_bits(regmap, TMP464_CONFIG_REG,
+				 TMP464_CONFIG_CONVERSION_RATE_MASK | TMP464_CONFIG_SHUTDOWN,
+				 BIT(TMP464_CONFIG_CONVERSION_RATE_B0) |
+				 BIT(TMP464_CONFIG_CONVERSION_RATE_B2));
+	if (err)
+		return err;
+
+	data->update_interval = 500;
+
+	return tmp464_enable_channels(data);
+}
+
+static int tmp464_detect(struct i2c_client *client,
+			 struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	char *name, *chip;
+	int reg;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	reg = i2c_smbus_read_word_swapped(client, TMP464_MANUFACTURER_ID_REG);
+	if (reg < 0)
+		return reg;
+	if (reg != TMP464_MANUFACTURER_ID)
+		return -ENODEV;
+
+	/* Check for "always return zero" bits */
+	reg = i2c_smbus_read_word_swapped(client, TMP464_THERM_STATUS_REG);
+	if (reg < 0)
+		return reg;
+	if (reg & 0x1f)
+		return -ENODEV;
+	reg = i2c_smbus_read_word_swapped(client, TMP464_THERM2_STATUS_REG);
+	if (reg < 0)
+		return reg;
+	if (reg & 0x1f)
+		return -ENODEV;
+
+	reg = i2c_smbus_read_word_swapped(client, TMP464_DEVICE_ID_REG);
+	if (reg < 0)
+		return reg;
+	switch (reg) {
+	case TMP464_DEVICE_ID:
+		name = "tmp464";
+		chip = "TMP464";
+		break;
+	case TMP468_DEVICE_ID:
+		name = "tmp468";
+		chip = "TMP468";
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	strscpy(info->type, name, I2C_NAME_SIZE);
+	dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n", chip, client->addr);
+
+	return 0;
+}
+
+static int tmp464_probe_child_from_dt(struct device *dev,
+				      struct device_node *child,
+				      struct tmp464_data *data)
+
+{
+	struct regmap *regmap = data->regmap;
+	u32 channel;
+	s32 nfactor;
+	int err;
+
+	err = of_property_read_u32(child, "reg", &channel);
+	if (err) {
+		dev_err(dev, "missing reg property of %pOFn\n", child);
+		return err;
+	}
+
+	if (channel >= data->channels) {
+		dev_err(dev, "invalid reg %d of %pOFn\n", channel, child);
+		return -EINVAL;
+	}
+
+	of_property_read_string(child, "label", &data->channel[channel].label);
+
+	data->channel[channel].enabled = of_device_is_available(child);
+
+	err = of_property_read_s32(child, "ti,n-factor", &nfactor);
+	if (err && err != -EINVAL)
+		return err;
+	if (!err) {
+		if (channel == 0) {
+			dev_err(dev, "n-factor can't be set for internal channel\n");
+			return -EINVAL;
+		}
+		if (nfactor > 127 || nfactor < -128) {
+			dev_err(dev, "n-factor for channel %d invalid (%d)\n",
+				channel, nfactor);
+			return -EINVAL;
+		}
+		err = regmap_write(regmap, TMP464_N_FACTOR_REG(channel),
+				   (nfactor << 8) & 0xff00);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tmp464_probe_from_dt(struct device *dev, struct tmp464_data *data)
+{
+	const struct device_node *np = dev->of_node;
+	struct device_node *child;
+	int err;
+
+	for_each_child_of_node(np, child) {
+		if (strcmp(child->name, "channel"))
+			continue;
+
+		err = tmp464_probe_child_from_dt(dev, child, data);
+		if (err) {
+			of_node_put(child);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static const struct hwmon_ops tmp464_ops = {
+	.is_visible = tmp464_is_visible,
+	.read = tmp464_read,
+	.read_string = tmp464_read_string,
+	.write = tmp464_write,
+};
+
+static const struct hwmon_channel_info *tmp464_info[] = {
+	HWMON_CHANNEL_INFO(chip,
+			   HWMON_C_UPDATE_INTERVAL),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST | HWMON_T_CRIT |
+			   HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_LABEL | HWMON_T_ENABLE,
+			   HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL | HWMON_T_ENABLE,
+			   HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL | HWMON_T_ENABLE,
+			   HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL | HWMON_T_ENABLE,
+			   HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL | HWMON_T_ENABLE,
+			   HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL | HWMON_T_ENABLE,
+			   HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL | HWMON_T_ENABLE,
+			   HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL | HWMON_T_ENABLE,
+			   HWMON_T_INPUT | HWMON_T_OFFSET | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_MAX_ALARM |
+			   HWMON_T_CRIT_ALARM | HWMON_T_FAULT | HWMON_T_LABEL | HWMON_T_ENABLE),
+	NULL
+};
+
+static const struct hwmon_chip_info tmp464_chip_info = {
+	.ops = &tmp464_ops,
+	.info = tmp464_info,
+};
+
+/* regmap */
+
+static bool tmp464_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return (reg < TMP464_TEMP_REG(TMP468_NUM_CHANNELS) ||
+		reg == TMP464_THERM_STATUS_REG ||
+		reg == TMP464_THERM2_STATUS_REG ||
+		reg == TMP464_REMOTE_OPEN_REG);
+}
+
+static const struct regmap_config tmp464_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = TMP464_DEVICE_ID_REG,
+	.volatile_reg = tmp464_is_volatile_reg,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.cache_type = REGCACHE_RBTREE,
+	.use_single_read = true,
+	.use_single_write = true,
+};
+
+static int tmp464_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct device *hwmon_dev;
+	struct tmp464_data *data;
+	int i, err;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
+		dev_err(&client->dev, "i2c functionality check failed\n");
+		return -ENODEV;
+	}
+	data = devm_kzalloc(dev, sizeof(struct tmp464_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mutex_init(&data->update_lock);
+
+	if (dev->of_node)
+		data->channels = (int)(unsigned long)of_device_get_match_data(&client->dev);
+	else
+		data->channels = i2c_match_id(tmp464_id, client)->driver_data;
+
+	data->regmap = devm_regmap_init_i2c(client, &tmp464_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	for (i = 0; i < data->channels; i++)
+		data->channel[i].enabled = true;
+
+	err = tmp464_init_client(dev, data);
+	if (err)
+		return err;
+
+	if (dev->of_node) {
+		err = tmp464_probe_from_dt(dev, data);
+		if (err)
+			return err;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 data, &tmp464_chip_info, NULL);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct i2c_driver tmp464_driver = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "tmp464",
+		.of_match_table = of_match_ptr(tmp464_of_match),
+	},
+	.probe_new = tmp464_probe,
+	.id_table = tmp464_id,
+	.detect = tmp464_detect,
+	.address_list = normal_i2c,
+};
+
+module_i2c_driver(tmp464_driver);
+
+MODULE_AUTHOR("Agathe Porte <agathe.porte@nokia.com>");
+MODULE_DESCRIPTION("Texas Instruments TMP464 temperature sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1

Re: [PATCH v7 1/2] dt-bindings: hwmon: add tmp464.yaml

[Rob Herring]

On Tue, 22 Feb 2022 14:36:09 -0800, Guenter Roeck wrote:
> From: Agathe Porte <agathe.porte@nokia.com>
> 
> Add basic description of the tmp464 driver DT bindings.
> 
> Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
> Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v7:
> - No change
> 
> v6:
> - Model ti,n-factor as int32 instead of int8.
> 
> v5:
> - Dropped ti,n-factor from channel@0 example. Added additional
>   channel to examples to show positive ti,n-factor property.
> 
> v4:
> - No changes
> 
> v3:
> - Addedd support for TMP468.
> - Changed number of channels from 0..3 (which was wrong anyway) to 0..8.
> - Changed value range for ti,n-factor to int8, with an example for
>   a negative value.
> - Added myself as driver maintainer.
> 
>  .../devicetree/bindings/hwmon/ti,tmp464.yaml  | 114 ++++++++++++++++++
>  MAINTAINERS                                   |   7 ++
>  2 files changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
> 

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

Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

[Guenter Roeck]

On Tue, Feb 22, 2022 at 02:36:10PM -0800, Guenter Roeck wrote:
> Add support for Texas Instruments TMP464 and TMP468 temperature sensor
> ICs.
> 
> TI's TMP464 is an I2C temperature sensor chip. This chip is similar
> to TI's TMP421 chip, but with 16bit-wide registers (instead of
> 8bit-wide registers). The chip has one local sensor and four remote
> sensors. TMP468 is similar to TMP464 but has one local and eight
> remote sensors.
> 
> Originally-from: Agathe Porte <agathe.porte@nokia.com>
> Cc: Agathe Porte <agathe.porte@nokia.com>
> Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Any review / test feedback on this patch ? I would like to apply it
before the commit window opens, but the time is getting short.

Thanks,
Guenter

Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

[Agathe Porte]

Hi Guenter,

Le 02/03/2022 à 18:59, Guenter Roeck a écrit :
> Any review / test feedback on this patch ? I would like to apply it
> before the commit window opens, but the time is getting short.

I thought that you did receive the TMP464 samples and had the opportunity to test on it. I will test v7 on our hardware equipped with TMP464, verify that DT support works fine, and will reply to this email with my findings.

Bests.

Agathe.

Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

[Guenter Roeck]

On 3/3/22 00:57, Agathe Porte wrote:
> Hi Guenter,
> 
> Le 02/03/2022 à 18:59, Guenter Roeck a écrit :
>> Any review / test feedback on this patch ? I would like to apply it
>> before the commit window opens, but the time is getting short.
> 
> I thought that you did receive the TMP464 samples and had the opportunity to test on it. I will test v7 on our hardware equipped with TMP464, verify that DT support works fine, and will reply to this email with my findings.
> 

Yes, I did, and thanks a lot for it! I even wrote a qemu emulation
for the chip to be able to test the devicetree code.

Still, I need to have someone else confirm that I didn't mess up.

Thanks,
Guenter

Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

[Agathe Porte]

Le 3/3/2022 à 4:00 PM, Guenter Roeck a écrit :
> On 3/3/22 00:57, Agathe Porte wrote:
>> Hi Guenter,
>>
>> Le 02/03/2022 à 18:59, Guenter Roeck a écrit :
>>> Any review / test feedback on this patch ? I would like to apply it
>>> before the commit window opens, but the time is getting short.
>>
>> I thought that you did receive the TMP464 samples and had the 
>> opportunity to test on it. I will test v7 on our hardware equipped 
>> with TMP464, verify that DT support works fine, and will reply to 
>> this email with my findings.
>>
>
> Yes, I did, and thanks a lot for it! I even wrote a qemu emulation
> for the chip to be able to test the devicetree code.

Great!

> Still, I need to have someone else confirm that I didn't mess up.

I tested v7 on our hardware and the behavior seems to be the same as our 
previous, in-house driver, if that gives you a point of comparison. We 
only use temp*_input sysfs though.

No compilation warnings.

I can disable and enable sensors fine at runtime:

> # cat temp*_input
> 43063
> 35813
> 34938
> 39313
> 29125
> # echo 0 | tee temp*_enable
> 0
> # cat temp*_input
> cat: temp1_input: No data available
> cat: temp2_input: No data available
> cat: temp3_input: No data available
> cat: temp4_input: No data available
> cat: temp5_input: No data available
> # echo 1 | tee temp*_enable
> 1
> # cat temp*_input
> 43063
> 35750
> 34875
> 39313
> 29188

For what it's worth:

Tested-by: Agathe Porte <agathe.porte@nokia.com>

Bests.

Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

[Guenter Roeck]

On Thu, Mar 03, 2022 at 04:31:39PM +0100, Agathe Porte wrote:
> Le 3/3/2022 à 4:00 PM, Guenter Roeck a écrit :
> > On 3/3/22 00:57, Agathe Porte wrote:
> > > Hi Guenter,
> > > 
> > > Le 02/03/2022 à 18:59, Guenter Roeck a écrit :
> > > > Any review / test feedback on this patch ? I would like to apply it
> > > > before the commit window opens, but the time is getting short.
> > > 
> > > I thought that you did receive the TMP464 samples and had the
> > > opportunity to test on it. I will test v7 on our hardware equipped
> > > with TMP464, verify that DT support works fine, and will reply to
> > > this email with my findings.
> > > 
> > 
> > Yes, I did, and thanks a lot for it! I even wrote a qemu emulation
> > for the chip to be able to test the devicetree code.
> 
> Great!
> 
> > Still, I need to have someone else confirm that I didn't mess up.
> 
> I tested v7 on our hardware and the behavior seems to be the same as our
> previous, in-house driver, if that gives you a point of comparison. We only
> use temp*_input sysfs though.
> 
> No compilation warnings.
> 
> I can disable and enable sensors fine at runtime:
> 
> > # cat temp*_input
> > 43063
> > 35813
> > 34938
> > 39313
> > 29125
> > # echo 0 | tee temp*_enable
> > 0
> > # cat temp*_input
> > cat: temp1_input: No data available
> > cat: temp2_input: No data available
> > cat: temp3_input: No data available
> > cat: temp4_input: No data available
> > cat: temp5_input: No data available
> > # echo 1 | tee temp*_enable
> > 1
> > # cat temp*_input
> > 43063
> > 35750
> > 34875
> > 39313
> > 29188
> 
> For what it's worth:
> 
> Tested-by: Agathe Porte <agathe.porte@nokia.com>
> 

Thanks a lot! 

I applied the series to hwmon-next.

Guenter

Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

[Guenter Roeck]

Hi Agathe,

On 3/14/22 08:46, Agathe Porte wrote:
> Hi,
> 
> Le 2/22/2022 à 11:36 PM, Guenter Roeck a écrit :
>>   of_property_read_string(child,"label", &data->channel[channel].label);
> 
> Upon trying to merge v7 in our codebase, our static analyzer tool detected that the return code of this function was not checked.
> 
> As I guess putting a label is optional, maybe we should add a `(void)` on the same line just before the function call to clearly indicate that not checking the return value is intentional and that it is not a coding mistake?
> 
> EDIT: As I was reading the implementation of of_property_read_string [1], it will not touch the destination string in case of error. Which means that labels may sit uninitialized and contain garbage data?
> 

Thanks for the feedback.

If of_property_read_string() returns an error, it will not set the pointer
to &data->channel[channel].label, which by default is NULL because the
data structure was allocated with devm_kzalloc(). That means tmp464_is_visible()
will disable the label attribute. I don't see a problem with the current
code.

There are lots of examples in the kernel where the return value from
of_property_read_string() is silently ignored. Not a single one of
those uses a (void) typecast. I don't really want to start making
such changes just to make static analyzers happy.

Thanks,
Guenter

Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

[Agathe Porte]

Hi Guenter,

Le 3/15/2022 à 2:22 AM, Guenter Roeck a écrit :
> If of_property_read_string() returns an error, it will not set the 
> pointer
> to &data->channel[channel].label, which by default is NULL because the
> data structure was allocated with devm_kzalloc(). That means 
> tmp464_is_visible()
> will disable the label attribute. I don't see a problem with the current
> code.

Thanks for the explanation. I agree that there is no problem on this point.

> There are lots of examples in the kernel where the return value from
> of_property_read_string() is silently ignored. Not a single one of
> those uses a (void) typecast. I don't really want to start making
> such changes just to make static analyzers happy.

I have to disagree here. Because something has always (not) be done in 
the past should not be a reason to (not) do it in the future out of pure 
habit. I did not suggest to add the (void) casts in existing code: I 
agree it would be a burden with no real added value.

But making static analyzers happy seems justified *for new code*. It 
also makes *other developers* more confident, because with the cast we 
are sure that not checking the return value is very intentional.

Please enlighten me if there are any downsides that I did not think of 
and that would block this one-line change.

Best regards,

Agathe.

Re: [PATCH v7 2/2] hwmon: Add driver for Texas Instruments TMP464 and TMP468

[Guenter Roeck]

Hi Agathe,

On 3/15/22 06:03, Agathe Porte wrote:
> Hi Guenter,
> 
> Le 3/15/2022 à 2:22 AM, Guenter Roeck a écrit :
>> If of_property_read_string() returns an error, it will not set the pointer
>> to &data->channel[channel].label, which by default is NULL because the
>> data structure was allocated with devm_kzalloc(). That means tmp464_is_visible()
>> will disable the label attribute. I don't see a problem with the current
>> code.
> 
> Thanks for the explanation. I agree that there is no problem on this point.
> 
>> There are lots of examples in the kernel where the return value from
>> of_property_read_string() is silently ignored. Not a single one of
>> those uses a (void) typecast. I don't really want to start making
>> such changes just to make static analyzers happy.
> 
> I have to disagree here. Because something has always (not) be done in the past should not be a reason to (not) do it in the future out of pure habit. I did not suggest to add the (void) casts in existing code: I agree it would be a burden with no real added value.
> 
> But making static analyzers happy seems justified *for new code*. It also makes *other developers* more confident, because with the cast we are sure that not checking the return value is very intentional.
> 
> Please enlighten me if there are any downsides that I did not think of and that would block this one-line change.
> 

Changing the code now would require either a separate patch or
a rebase of the hwmon-next tree. Rebasing the hwmon-next tree
at this point of the release cycle (a few days before the commit
window opens) is something I really don't want to do, leaving the
option to add a separate patch for the change. That makes it
identical to changing existing code to add the (void).

In addition to that, I do not agree that adding (void) really
adds value here; it just says "this is done on purpose" because
the static analyzer doesn't know better. 0-day stopped reporting
this kind of perceived problem, presumably for good reason.
Since the result of the function call is implied in setting or not
setting the passed pointer, a return value check or adding (void)
is not warranted. This would be different if the property was mandatory,
but that is not the case here.

There are lots of other functions in the kernel where return values
are not checked, for a variety of reasons. Functions where checking
the return value is necessary/mandatory are tagged with __must_check.
For others it is left to the caller to decide if a return value
should be checked, and if it makes sense / adds value to add (void).

I'll give you another example: cancel_work_sync() and related functions.
I am sure your static analyzer will complain about the failure to check
its return value in almost all cases. A counter-example is, say,
platform_driver_register(), where the return value should really be
checked and a (void) typecast should be used if it is not checked on
purpose. The problem is that static analyzers can not determine if
the return value check is necessary, and should either leave it alone
or make reports conditional on some command line option.

Overall we'll have to agree to disagree.

Thanks,
Guenter