dt-bindings: hwmon: add tmp464.yaml

From: Agathe Porte <[email protected]>

Add basic description of the tmp464 driver DT bindings.

---
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

new file mode 100644
index 000000000000..801ca9ba7d34
@@ -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 <[email protected]>
+
+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";
+        };
+      };
+    };

index fca970a46e77..f51bc7c7e439 100644
@@ -19489,6 +19489,13 @@ S:	Maintained
 F:	Documentation/hwmon/tmp401.rst
 F:	drivers/hwmon/tmp401.c
 
+TMP464 HARDWARE MONITOR DRIVER
+M:	Agathe Porte <[email protected]>
+M:	Guenter Roeck <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
+
 TMP513 HARDWARE MONITOR DRIVER
 M:	Eric Tremblay <[email protected]>
 L:	[email protected]

2.35.1

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 <[email protected]>

---
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

index df20022c741f..37590db85e65 100644
@@ -193,6 +193,7 @@ Hardware Monitoring Kernel Drivers
    tmp108
    tmp401
    tmp421
+   tmp464
    tmp513
    tps23861
    tps40422

index f51bc7c7e439..9b51bf5ca3b8 100644
@@ -19495,6 +19495,8 @@ M:	Guenter Roeck <[email protected]>
 L:	[email protected]
 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 <[email protected]>

index 8df25f1079ba..b84932fd0c13 100644
@@ -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

index 185f946d698b..a1f2d6686227 100644
@@ -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

2.35.1

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

Hi Guenter,

Le 02/03/2022 à 18:59, Guenter Roeck a écrit :

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.

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

Le 3/3/2022 à 4:00 PM, Guenter Roeck a écrit :

Great!

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:

For what it's worth:

Bests.

Thanks a lot!

I applied the series to hwmon-next.

Guenter

Hi Agathe,

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

Hi Guenter,

Le 3/15/2022 à 2:22 AM, Guenter Roeck a écrit :

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

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.

Hi Agathe,

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