[PATCH v2 0/2] add support for MCP998X

[PATCH v2 0/2] add support for MCP998X

[victor.duicu]

From: Victor Duicu <victor.duicu@microchip.com>

Add support for Microchip MCP998X/33 and MCP998XD/33D
Multichannel Automotive Temperature Monitor Family.

The chips in the family have different numbers of external
channels, ranging from 1 (MCP9982) to 4 channels (MCP9985).
Reading diodes in anti-parallel connection is supported
by MCP9984/85/33 and MCP9984D/85D/33D.
Dedicated hardware shutdown circuitry is present only
in MCP998XD and MCP9933D.

Current version of driver does not support interrupts, events and data
buffering.

Differences related to previous patch:
v2:
- move hysteresis, extended temperature range and beta parameters
  from devicetree into user space.
- edit comments in yaml and driver.
- remove "|" in descpriptions, remove "+" from PatternProperties in yaml.
- add default to microchip,ideality-factor, delete blank lines and wrap to
  80 chars in yaml.
- remove variables with upper case.
- add check for microchip,apdd-state and microchip,recd34 in yaml.
- improve coding style in driver code.
- add includes for all functions used.
- rename MCP9982_INT_HIGH_BYTE_ADDR to MCP9982_INT_VALUE_ADDR and
  MCP9982_INT_LOW_BYTE_ADDR to MCP9982_FRAC_VALUE_ADDR.
- remove custom attribute running_average_window and
  running_average_window_available and map them to a low pass filter.
- update sysfs-bus-iio-temperature-mcp9982 to reflect current
  driver attributes and point to next kernel version (6.17).
- use compound literal to define driver channels.
- replace device_property_read_string() with i2c_get_match_data() to read
  chip name from devicetree.
- remove MCP9982_DEV_ATTR and mcp9982_prep_custom_attributes().
- remove client, chip_name, iio_info from mcp9982_priv.
- replace sprintf() with sysfs_emit().
- remove error messages which are triggered by keyboard input.
- replace devm_kzalloc() with devm_kcalloc(), array mcp9982_chip_config[] with
  individual structures, device_property_present() with device_property_read_bool().
- reordered parameters in mcp9982_features and mcp9982_priv to optimize memory
  allocation.
- remove .endianness from channel properties.
- change name of some parameters in mcp9982_priv.
- add check for reg value 0 from devicetree (channel 0 is for internal temperature
  and can't be disabled).

v1:
- inital version.

Victor Duicu (2):
  dt-bindings: iio: temperature: add support for MCP998X
  iio: temperature: add support for MCP998X

 .../testing/sysfs-bus-iio-temperature-mcp9982 |  27 +
 .../iio/temperature/microchip,mcp9982.yaml    | 174 ++++
 MAINTAINERS                                   |   7 +
 drivers/iio/temperature/Kconfig               |  10 +
 drivers/iio/temperature/Makefile              |   1 +
 drivers/iio/temperature/mcp9982.c             | 866 ++++++++++++++++++
 6 files changed, 1085 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
 create mode 100644 drivers/iio/temperature/mcp9982.c


base-commit: 0c86e33819785fe50616b6ee3fb35c1e4be406d5
-- 
2.48.1

[PATCH v2 1/2] dt-bindings: iio: temperature: add support for MCP998X

[victor.duicu]

From: Victor Duicu <victor.duicu@microchip.com>

This is the devicetree schema for Microchip MCP998X/33 and
MCP998XD/33D Multichannel Automotive Temperature Monitor Family.

Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
---
 .../iio/temperature/microchip,mcp9982.yaml    | 174 ++++++++++++++++++
 1 file changed, 174 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml

diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
new file mode 100644
index 000000000000..249470c8953b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
@@ -0,0 +1,174 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive
+       Temperature Monitor Family
+
+maintainers:
+  - Victor Duicu <victor.duicu@microchip.com>
+
+description: |
+  The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire multichannel
+  automotive temperature monitor.
+  The datasheet can be found here:
+    https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
+
+properties:
+  compatible:
+    enum:
+      - microchip,mcp9933
+      - microchip,mcp9933d
+      - microchip,mcp9982
+      - microchip,mcp9982d
+      - microchip,mcp9983
+      - microchip,mcp9983d
+      - microchip,mcp9984
+      - microchip,mcp9984d
+      - microchip,mcp9985
+      - microchip,mcp9985d
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 2
+
+  interrupt-names:
+    description:
+      alert1 indicates a HIGH or LOW limit was exceeded.
+      alert2 indicates a THERM limit was exceeded.
+    items:
+      - const: alert1
+      - const: alert2
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  microchip,apdd-state:
+    description:
+      Enable anti-parallel diode mode operation.
+      MCP9984/84D/85/85D and MCP9933/33D support reading two external diodes
+      in anti-parallel connection on the same set of pins.
+      Omit this tag to disable anti-parallel diode mode.
+    type: boolean
+
+  microchip,recd12:
+    description:
+      Enable resistance error correction for external channels 1 and 2.
+      Omit this tag to disable REC for channels 1 and 2.
+    type: boolean
+
+  microchip,recd34:
+    description:
+      Enable resistance error correction for external channels 3 and 4.
+      Omit this tag to disable REC for channels 3 and 4.
+    type: boolean
+
+  label:
+    description: Unique name to identify which device this is.
+
+  vdd-supply: true
+
+patternProperties:
+  "^channel@[1-4]$":
+    description:
+      Represents the external temperature channels to which
+      a remote diode is connected.
+    type: object
+
+    properties:
+      reg:
+        items:
+          minimum: 1
+          maximum: 4
+
+      microchip,ideality-factor:
+        description:
+          Each channel has an ideality factor.
+          Beta compensation and resistance error correction automatically
+          correct for most ideality errors. So ideality factor does not need
+          to be adjusted in general.
+          Omit this tag in order to set the default value.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        default: 18
+
+      label:
+        description: Unique name to identify which channel this is.
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - microchip,mcp9982
+              - microchip,mcp9982d
+              - microchip,mcp9983
+              - microchip,mcp9983d
+    then:
+      properties:
+        microchip,apdd-state: false
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - microchip,mcp9982
+              - microchip,mcp9982d
+              - microchip,mcp9933
+              - microchip,mcp9933d
+    then:
+      properties:
+        microchip,recd34: false
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        temperature-sensor@4c {
+            compatible = "microchip,mcp9985";
+            reg = <0x4c>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            label = "temperature-sensor";
+
+            microchip,apdd-state;
+            microchip,recd12;
+            microchip,recd34;
+            vdd-supply = <&vdd>;
+
+            channel@1 {
+                reg = <0x1>;
+                label = "CPU Temperature";
+            };
+
+            channel@2 {
+                reg = <0x2>;
+                label = "GPU Temperature";
+            };
+        };
+    };
+
+...
-- 
2.48.1

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: add support for MCP998X

[David Lechner]

On 5/29/25 4:36 AM, victor.duicu@microchip.com wrote:
> From: Victor Duicu <victor.duicu@microchip.com>
> 
> This is the devicetree schema for Microchip MCP998X/33 and
> MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
> 
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> ---
>  .../iio/temperature/microchip,mcp9982.yaml    | 174 ++++++++++++++++++
>  1 file changed, 174 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> new file mode 100644
> index 000000000000..249470c8953b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> @@ -0,0 +1,174 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive
> +       Temperature Monitor Family
> +
> +maintainers:
> +  - Victor Duicu <victor.duicu@microchip.com>
> +
> +description: |
> +  The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire multichannel
> +  automotive temperature monitor.
> +  The datasheet can be found here:
> +    https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,mcp9933
> +      - microchip,mcp9933d
> +      - microchip,mcp9982
> +      - microchip,mcp9982d
> +      - microchip,mcp9983
> +      - microchip,mcp9983d
> +      - microchip,mcp9984
> +      - microchip,mcp9984d
> +      - microchip,mcp9985
> +      - microchip,mcp9985d
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 2
> +
> +  interrupt-names:
> +    description:
> +      alert1 indicates a HIGH or LOW limit was exceeded.
> +      alert2 indicates a THERM limit was exceeded.

I think we need minItems: 1 here.

> +    items:
> +      - const: alert1
> +      - const: alert2

Typically, interrupts are named after the pin they are wired to, not
the signal. This is especially true when a single pin can be configured
for different signals as is the case here.

There is a /ALERT//THERM pin on all chips and a /THERM//ADDR pin on some
chips.

So I would expect the names to match that:

    items:
      - const: alert-therm
      - const: therm-addr

And then extra descriptions probably aren't needed.

If we want to be extra careful, we could also add an -if: below to set
maxItems: 1 for interrupts and interrupt-names on chips that only have
the one pin.

And I assume that the /SYS_SHDN pin would never be wired up as an interrupt?

> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  microchip,apdd-state:
> +    description:
> +      Enable anti-parallel diode mode operation.
> +      MCP9984/84D/85/85D and MCP9933/33D support reading two external diodes
> +      in anti-parallel connection on the same set of pins.
> +      Omit this tag to disable anti-parallel diode mode.
> +    type: boolean
> +
> +  microchip,recd12:
> +    description:
> +      Enable resistance error correction for external channels 1 and 2.
> +      Omit this tag to disable REC for channels 1 and 2.
> +    type: boolean
> +
> +  microchip,recd34:
> +    description:
> +      Enable resistance error correction for external channels 3 and 4.
> +      Omit this tag to disable REC for channels 3 and 4.
> +    type: boolean
> +
> +  label:
> +    description: Unique name to identify which device this is.
> +
> +  vdd-supply: true
> +
> +patternProperties:
> +  "^channel@[1-4]$":
> +    description:
> +      Represents the external temperature channels to which
> +      a remote diode is connected.
> +    type: object
> +
> +    properties:
> +      reg:
> +        items:
> +          minimum: 1
> +          maximum: 4
> +
> +      microchip,ideality-factor:
> +        description:
> +          Each channel has an ideality factor.
> +          Beta compensation and resistance error correction automatically
> +          correct for most ideality errors. So ideality factor does not need
> +          to be adjusted in general.
> +          Omit this tag in order to set the default value.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        default: 18
> +
> +      label:
> +        description: Unique name to identify which channel this is.
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - microchip,mcp9982
> +              - microchip,mcp9982d
> +              - microchip,mcp9983
> +              - microchip,mcp9983d
> +    then:
> +      properties:
> +        microchip,apdd-state: false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - microchip,mcp9982
> +              - microchip,mcp9982d
> +              - microchip,mcp9933
> +              - microchip,mcp9933d
> +    then:
> +      properties:
> +        microchip,recd34: false
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        temperature-sensor@4c {
> +            compatible = "microchip,mcp9985";
> +            reg = <0x4c>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            label = "temperature-sensor";

This is the same as the node name, so probably not the best
example of a label.

> +
> +            microchip,apdd-state;
> +            microchip,recd12;
> +            microchip,recd34;
> +            vdd-supply = <&vdd>;
> +
> +            channel@1 {
> +                reg = <0x1>;

Why 0x here?

> +                label = "CPU Temperature";
> +            };
> +
> +            channel@2 {
> +                reg = <0x2>;
> +                label = "GPU Temperature";
> +            };
> +        };
> +    };
> +
> +...

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: add support for MCP998X

[Conor Dooley]

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

On Thu, May 29, 2025 at 01:13:38PM -0500, David Lechner wrote:
> On 5/29/25 4:36 AM, victor.duicu@microchip.com wrote:
> > From: Victor Duicu <victor.duicu@microchip.com>
> > 
> > This is the devicetree schema for Microchip MCP998X/33 and
> > MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
> > 
> > Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> > ---
> >  .../iio/temperature/microchip,mcp9982.yaml    | 174 ++++++++++++++++++
> >  1 file changed, 174 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> > new file mode 100644
> > index 000000000000..249470c8953b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> > @@ -0,0 +1,174 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9982.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip MCP998X/33 and MCP998XD/33D Multichannel Automotive
> > +       Temperature Monitor Family
> > +
> > +maintainers:
> > +  - Victor Duicu <victor.duicu@microchip.com>
> > +
> > +description: |
> > +  The MCP998X/33 and MCP998XD/33D family is a high-accuracy 2-wire multichannel
> > +  automotive temperature monitor.
> > +  The datasheet can be found here:
> > +    https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - microchip,mcp9933
> > +      - microchip,mcp9933d
> > +      - microchip,mcp9982
> > +      - microchip,mcp9982d
> > +      - microchip,mcp9983
> > +      - microchip,mcp9983d
> > +      - microchip,mcp9984
> > +      - microchip,mcp9984d
> > +      - microchip,mcp9985
> > +      - microchip,mcp9985d
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 2
> > +
> > +  interrupt-names:
> > +    description:
> > +      alert1 indicates a HIGH or LOW limit was exceeded.
> > +      alert2 indicates a THERM limit was exceeded.
> 
> I think we need minItems: 1 here.
> 
> > +    items:
> > +      - const: alert1
> > +      - const: alert2
> 
> Typically, interrupts are named after the pin they are wired to, not
> the signal. This is especially true when a single pin can be configured
> for different signals as is the case here.
> 
> There is a /ALERT//THERM pin on all chips and a /THERM//ADDR pin on some
> chips.
> 
> So I would expect the names to match that:
> 
>     items:
>       - const: alert-therm
>       - const: therm-addr
> 
> And then extra descriptions probably aren't needed.
> 
> If we want to be extra careful, we could also add an -if: below to set
> maxItems: 1 for interrupts and interrupt-names on chips that only have
> the one pin.
> 
> And I assume that the /SYS_SHDN pin would never be wired up as an interrupt?
> 
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  microchip,apdd-state:
> > +    description:
> > +      Enable anti-parallel diode mode operation.
> > +      MCP9984/84D/85/85D and MCP9933/33D support reading two external diodes
> > +      in anti-parallel connection on the same set of pins.
> > +      Omit this tag to disable anti-parallel diode mode.
> > +    type: boolean
> > +
> > +  microchip,recd12:
> > +    description:
> > +      Enable resistance error correction for external channels 1 and 2.
> > +      Omit this tag to disable REC for channels 1 and 2.
> > +    type: boolean
> > +
> > +  microchip,recd34:
> > +    description:
> > +      Enable resistance error correction for external channels 3 and 4.
> > +      Omit this tag to disable REC for channels 3 and 4.

Why are these two devicetree properties, rather than runtime controls?

> > +    type: boolean
> > +
> > +  label:
> > +    description: Unique name to identify which device this is.
> > +
> > +  vdd-supply: true
> > +
> > +patternProperties:
> > +  "^channel@[1-4]$":
> > +    description:
> > +      Represents the external temperature channels to which
> > +      a remote diode is connected.
> > +    type: object
> > +
> > +    properties:
> > +      reg:
> > +        items:
> > +          minimum: 1
> > +          maximum: 4
> > +
> > +      microchip,ideality-factor:
> > +        description:
> > +          Each channel has an ideality factor.
> > +          Beta compensation and resistance error correction automatically
> > +          correct for most ideality errors. So ideality factor does not need
> > +          to be adjusted in general.
> > +          Omit this tag in order to set the default value.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        default: 18
> > +
> > +      label:
> > +        description: Unique name to identify which channel this is.
> > +
> > +    required:
> > +      - reg
> > +
> > +    additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vdd-supply
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - microchip,mcp9982
> > +              - microchip,mcp9982d
> > +              - microchip,mcp9983
> > +              - microchip,mcp9983d
> > +    then:
> > +      properties:
> > +        microchip,apdd-state: false
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - microchip,mcp9982
> > +              - microchip,mcp9982d
> > +              - microchip,mcp9933
> > +              - microchip,mcp9933d
> > +    then:
> > +      properties:
> > +        microchip,recd34: false
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        temperature-sensor@4c {
> > +            compatible = "microchip,mcp9985";
> > +            reg = <0x4c>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            label = "temperature-sensor";
> 
> This is the same as the node name, so probably not the best
> example of a label.

Ye, I'm not convinced this property has any value, when the channels
themselves can have labels.

> 
> > +
> > +            microchip,apdd-state;
> > +            microchip,recd12;
> > +            microchip,recd34;
> > +            vdd-supply = <&vdd>;
> > +
> > +            channel@1 {
> > +                reg = <0x1>;
> 
> Why 0x here?
> 
> > +                label = "CPU Temperature";
> > +            };
> > +
> > +            channel@2 {
> > +                reg = <0x2>;
> > +                label = "GPU Temperature";
> > +            };
> > +        };
> > +    };
> > +
> > +...
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: add support for MCP998X

[Victor.Duicu]

On Fri, 2025-05-30 at 16:55 +0100, Conor Dooley wrote:
> On Thu, May 29, 2025 at 01:13:38PM -0500, David Lechner wrote:
> > On 5/29/25 4:36 AM, victor.duicu@microchip.com wrote:
> > > From: Victor Duicu <victor.duicu@microchip.com>
> > > 
> > > 
Hi Conor,

...

> > > +  microchip,recd12:
> > > +    description:
> > > +      Enable resistance error correction for external channels 1
> > > and 2.
> > > +      Omit this tag to disable REC for channels 1 and 2.
> > > +    type: boolean
> > > +
> > > +  microchip,recd34:
> > > +    description:
> > > +      Enable resistance error correction for external channels 3
> > > and 4.
> > > +      Omit this tag to disable REC for channels 3 and 4.
> 
> Why are these two devicetree properties, rather than runtime
> controls?

The parasitic resistance added to the series resistance is dependent
only on the circuit. 
It is possible for the chip and the transistor to be at some distance
from each other. The manufacturer can approximate the error added and
decide if resistance error correction should be applied.
The user cannot influence the parasitic resistance nor calculate it.

With Kind Regards,
Victor Duicu

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: add support for MCP998X

[Conor Dooley]

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

Jonathan,

On Mon, Jun 02, 2025 at 02:48:52PM +0000, Victor.Duicu@microchip.com wrote:
> On Fri, 2025-05-30 at 16:55 +0100, Conor Dooley wrote:
> > On Thu, May 29, 2025 at 01:13:38PM -0500, David Lechner wrote:
> > > On 5/29/25 4:36 AM, victor.duicu@microchip.com wrote:
> > > > From: Victor Duicu <victor.duicu@microchip.com>
> > > > +  microchip,recd12:
> > > > +    description:
> > > > +      Enable resistance error correction for external channels 1
> > > > and 2.
> > > > +      Omit this tag to disable REC for channels 1 and 2.
> > > > +    type: boolean
> > > > +
> > > > +  microchip,recd34:
> > > > +    description:
> > > > +      Enable resistance error correction for external channels 3
> > > > and 4.
> > > > +      Omit this tag to disable REC for channels 3 and 4.
> > 
> > Why are these two devicetree properties, rather than runtime
> > controls?
> 
> The parasitic resistance added to the series resistance is dependent
> only on the circuit. 
> It is possible for the chip and the transistor to be at some distance
> from each other. The manufacturer can approximate the error added and
> decide if resistance error correction should be applied.

I don't think I buy this line of argument. The property is not
describing the hardware, it's literally being used as a toggle for some
software feature. It'd be more acceptable if it indicated that the chip
and transistor were distant, leaving software to make a decision on what
that meant. One user (say bsd) might want decide that the driver should
always enable it, but another (say linux) might expose it as a control
to userspace defaulting based the dt property.
Additionally, the name of the property is pretty awful, and does not
even hint at what it's doing - and there's no mention of why channel 1/2
and 3/4 are bound together.

> The user cannot influence the parasitic resistance nor calculate it.

I don't think that's super relevant here, since the property has nothing
to do with influencing or calculating the value. I meant deciding whether
or not the correction is applied, just as the dt property you propose
does now.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: add support for MCP998X

[Victor.Duicu]

On Fri, 2025-06-06 at 16:15 +0100, Conor Dooley wrote:
> Jonathan,
> 
> On Mon, Jun 02, 2025 at 02:48:52PM +0000,
> Victor.Duicu@microchip.com wrote:
> > On Fri, 2025-05-30 at 16:55 +0100, Conor Dooley wrote:
> > > On Thu, May 29, 2025 at 01:13:38PM -0500, David Lechner wrote:
> > > > On 5/29/25 4:36 AM, victor.duicu@microchip.com wrote:
> > > > > From: Victor Duicu <victor.duicu@microchip.com>
> > > > > +  microchip,recd12:
> > > > > +    description:
> > > > > +      Enable resistance error correction for external
> > > > > channels 1
> > > > > and 2.
> > > > > +      Omit this tag to disable REC for channels 1 and 2.
> > > > > +    type: boolean
> > > > > +
> > > > > +  microchip,recd34:
> > > > > +    description:
> > > > > +      Enable resistance error correction for external
> > > > > channels 3
> > > > > and 4.
> > > > > +      Omit this tag to disable REC for channels 3 and 4.
> > > 
> > > Why are these two devicetree properties, rather than runtime
> > > controls?
> > 
> > The parasitic resistance added to the series resistance is
> > dependent
> > only on the circuit. 
> > It is possible for the chip and the transistor to be at some
> > distance
> > from each other. The manufacturer can approximate the error added
> > and
> > decide if resistance error correction should be applied.
> 
> I don't think I buy this line of argument. The property is not
> describing the hardware, it's literally being used as a toggle for
> some
> software feature. It'd be more acceptable if it indicated that the
> chip
> and transistor were distant, leaving software to make a decision on
> what
> that meant. One user (say bsd) might want decide that the driver
> should
> always enable it, but another (say linux) might expose it as a
> control
> to userspace defaulting based the dt property.
> Additionally, the name of the property is pretty awful, and does not
> even hint at what it's doing - and there's no mention of why channel
> 1/2
> and 3/4 are bound together.
> 
You are correct that the parameters recd12 and recd34 do not directly
describe the hardware, but they control a software feature of the chip
itself. Resistance error correction is capable of counterbalancing
the parasitic resistance added to the external diodes, which can be
significant.

The manufacturer knows where the chip and diode are and can decide if
correction is necessary. The user does not have that insight.

I can change the name of the parameter to something like
resistance_err_correction and mention in the description which channels
are affected.


> > The user cannot influence the parasitic resistance nor calculate
> > it.
> 
> I don't think that's super relevant here, since the property has
> nothing
> to do with influencing or calculating the value. I meant deciding
> whether
> or not the correction is applied, just as the dt property you propose
> does now.
> 
> Cheers,
> Conor.

Kind Regards,
Victor Duicu

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: add support for MCP998X

[Conor Dooley]

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

On Tue, Jun 10, 2025 at 01:29:01PM +0000, Victor.Duicu@microchip.com wrote:
> On Fri, 2025-06-06 at 16:15 +0100, Conor Dooley wrote:
> > Jonathan,
> > 
> > On Mon, Jun 02, 2025 at 02:48:52PM +0000,
> > Victor.Duicu@microchip.com wrote:
> > > On Fri, 2025-05-30 at 16:55 +0100, Conor Dooley wrote:
> > > > On Thu, May 29, 2025 at 01:13:38PM -0500, David Lechner wrote:
> > > > > On 5/29/25 4:36 AM, victor.duicu@microchip.com wrote:
> > > > > > From: Victor Duicu <victor.duicu@microchip.com>
> > > > > > +  microchip,recd12:
> > > > > > +    description:
> > > > > > +      Enable resistance error correction for external
> > > > > > channels 1
> > > > > > and 2.
> > > > > > +      Omit this tag to disable REC for channels 1 and 2.
> > > > > > +    type: boolean
> > > > > > +
> > > > > > +  microchip,recd34:
> > > > > > +    description:
> > > > > > +      Enable resistance error correction for external
> > > > > > channels 3
> > > > > > and 4.
> > > > > > +      Omit this tag to disable REC for channels 3 and 4.
> > > > 
> > > > Why are these two devicetree properties, rather than runtime
> > > > controls?
> > > 
> > > The parasitic resistance added to the series resistance is
> > > dependent
> > > only on the circuit. 
> > > It is possible for the chip and the transistor to be at some
> > > distance
> > > from each other. The manufacturer can approximate the error added
> > > and
> > > decide if resistance error correction should be applied.
> > 
> > I don't think I buy this line of argument. The property is not
> > describing the hardware, it's literally being used as a toggle for
> > some
> > software feature. It'd be more acceptable if it indicated that the
> > chip
> > and transistor were distant, leaving software to make a decision on
> > what
> > that meant. One user (say bsd) might want decide that the driver
> > should
> > always enable it, but another (say linux) might expose it as a
> > control
> > to userspace defaulting based the dt property.
> > Additionally, the name of the property is pretty awful, and does not
> > even hint at what it's doing - and there's no mention of why channel
> > 1/2
> > and 3/4 are bound together.
> > 
> You are correct that the parameters recd12 and recd34 do not directly
> describe the hardware, but they control a software feature of the chip
> itself. Resistance error correction is capable of counterbalancing
> the parasitic resistance added to the external diodes, which can be
> significant.

> The manufacturer knows where the chip and diode are and can decide if
> correction is necessary. The user does not have that insight.

The user _may_ not have it. The properties should not be written such
that they exclude the control of these things from userspace, and just
indicate that the hardware configuration has the chip and diode
sufficiently far apart that the feature can help.


> I can change the name of the parameter to something like
> resistance_err_correction and mention in the description which channels
> are affected.

No _s are allowed in properties, so bear that in mind. Again, I don't
think the property should be responsible for turning it on, just
indicate that there is a parasitic resistance, so and the name should
really be something that indicates a parasitic resistance. E.g.
microchip,parasitic-res-on-channel1. Software (be it userspace or
driver) can then made a decision about turning on the error correction
with that information.

> > > The user cannot influence the parasitic resistance nor calculate
> > > it.
> > 
> > I don't think that's super relevant here, since the property has
> > nothing
> > to do with influencing or calculating the value. I meant deciding
> > whether
> > or not the correction is applied, just as the dt property you propose
> > does now.
> > 
> > Cheers,
> > Conor.
> 
> Kind Regards,
> Victor Duicu

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 2/2] iio: temperature: add support for MCP998X

[victor.duicu]

From: Victor Duicu <victor.duicu@microchip.com>

This is the driver for Microchip MCP998X/33 and MCP998XD/33D
Multichannel Automotive Temperature Monitor Family.

Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
---
 .../testing/sysfs-bus-iio-temperature-mcp9982 |  27 +
 MAINTAINERS                                   |   7 +
 drivers/iio/temperature/Kconfig               |  10 +
 drivers/iio/temperature/Makefile              |   1 +
 drivers/iio/temperature/mcp9982.c             | 866 ++++++++++++++++++
 5 files changed, 911 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
 create mode 100644 drivers/iio/temperature/mcp9982.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
new file mode 100644
index 000000000000..c55e106e5863
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
@@ -0,0 +1,27 @@
+What:		/sys/bus/iio/devices/iio:deviceX/enable_extended_temp_range
+KernelVersion:	6.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute enables extended range of temperatures.
+		Value 1 uses the extended range, value 0 uses the default range.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_beta1
+KernelVersion:	6.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute controls the value of beta correction
+		for channel 1.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_beta2
+KernelVersion:	6.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute controls the value of beta correction
+		for channel 2.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_beta_available
+KernelVersion:	6.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute displays the values that can be
+		set for beta correction.
diff --git a/MAINTAINERS b/MAINTAINERS
index 86a2045ba62e..63573c517603 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15887,6 +15887,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
 F:	drivers/iio/adc/mcp3911.c
 
+MICROCHIP MCP9982 TEMPERATURE DRIVER
+M:	Victor Duicu <victor.duicu@microchip.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
+F:	drivers/iio/temperature/mcp9982.c
+
 MICROCHIP MMC/SD/SDIO MCI DRIVER
 M:	Aubin Constans <aubin.constans@microchip.com>
 S:	Maintained
diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index 1244d8e17d50..e72b49f95f86 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -182,4 +182,14 @@ config MCP9600
 	  This driver can also be built as a module. If so, the module
 	  will be called mcp9600.
 
+config MCP9982
+       tristate "Microchip Technology MCP9982 driver"
+       depends on I2C
+       help
+         Say yes here to build support for Microchip Technology's MCP998X/33
+         and MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
+
+         This driver can also be built as a module. If so, the module
+         will be called mcp9982.
+
 endmenu
diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
index 07d6e65709f7..83f5f4bb4ff3 100644
--- a/drivers/iio/temperature/Makefile
+++ b/drivers/iio/temperature/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_MAX30208) += max30208.o
 obj-$(CONFIG_MAX31856) += max31856.o
 obj-$(CONFIG_MAX31865) += max31865.o
 obj-$(CONFIG_MCP9600) += mcp9600.o
+obj-$(CONFIG_MCP9982) += mcp9982.o
 obj-$(CONFIG_MLX90614) += mlx90614.o
 obj-$(CONFIG_MLX90632) += mlx90632.o
 obj-$(CONFIG_MLX90632) += mlx90635.o
diff --git a/drivers/iio/temperature/mcp9982.c b/drivers/iio/temperature/mcp9982.c
new file mode 100644
index 000000000000..89e966beffe6
--- /dev/null
+++ b/drivers/iio/temperature/mcp9982.c
@@ -0,0 +1,866 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IIO driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family
+ *
+ * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Victor Duicu <victor.duicu@microchip.com>
+ *
+ * Datasheet can be found here:
+ * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/device/devres.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/irq.h>
+#include <linux/limits.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <linux/unaligned.h>
+#include <linux/units.h>
+
+/* MCP9982 Registers */
+#define MCP9982_INT_VALUE_ADDR(index)		(2 * (index))
+#define MCP9982_FRAC_VALUE_ADDR(index)		(2 * (index) + 1)
+#define MCP9982_ONE_SHOT_ADDR			0x0A
+#define MCP9982_INTERNAL_HIGH_LIMIT_ADDR	0x0B
+#define MCP9982_INTERNAL_LOW_LIMIT_ADDR		0x0C
+#define MCP9982_EXT1_HIGH_LIMIT_INT_VALUE_ADDR	0x0D
+#define MCP9982_EXT1_HIGH_LIMIT_FRAC_VALUE_ADDR	0x0E
+#define MCP9982_EXT1_LOW_LIMIT_INT_VALUE_ADDR	0x0F
+#define MCP9982_EXT1_LOW_LIMIT_FRAC_VALUE_ADDR	0x10
+#define MCP9982_INTERNAL_THERM_LIMIT_ADDR	0x1D
+#define MCP9982_EXT1_THERM_LIMIT_ADDR		0x1E
+#define MCP9982_CFG_ADDR			0x22
+#define MCP9982_CONV_ADDR			0x24
+#define MCP9982_HYS_ADDR			0x25
+#define MCP9982_CONSEC_ALRT_ADDR		0x26
+#define MCP9982_ALRT_CFG_ADDR			0x27
+#define MCP9982_RUNNING_AVG_ADDR		0x28
+#define MCP9982_HOTTEST_CFG_ADDR		0x29
+#define MCP9982_STATUS_ADDR			0x2A
+#define MCP9982_EXT_FAULT_STATUS_ADDR		0x2B
+#define MCP9982_HIGH_LIMIT_STATUS_ADDR		0x2C
+#define MCP9982_LOW_LIMIT_STATUS_ADDR		0x2D
+#define MCP9982_THERM_LIMIT_STATUS_ADDR		0x2E
+#define MCP9982_HOTTEST_INT_VALUE_ADDR		0x2F
+#define MCP9982_HOTTEST_FRAC_VALUE_ADDR		0x30
+#define MCP9982_HOTTEST_STATUS_ADDR		0x31
+#define MCP9982_THERM_SHTDWN_CFG_ADDR		0x32
+#define MCP9982_HRDW_THERM_SHTDWN_LIMIT_ADDR	0x33
+#define MCP9982_EXT_BETA_CFG_ADDR(index)	((index) + 52)
+#define MCP9982_EXT_IDEAL_ADDR(index)		((index) + 54)
+
+/* MCP9982 Bits */
+#define MCP9982_CFG_MSKAL			BIT(7)
+#define MCP9982_CFG_RS				BIT(6)
+#define MCP9982_CFG_ATTHM			BIT(5)
+#define MCP9982_CFG_RECD12			BIT(4)
+#define MCP9982_CFG_RECD34			BIT(3)
+#define MCP9982_CFG_RANGE			BIT(2)
+#define MCP9982_CFG_DA_ENA			BIT(1)
+#define MCP9982_CFG_APDD			BIT(0)
+#define MCP9982_EXT_BETA_ENBL			BIT(4)
+
+/* The maximum number of channels a member of the family can have */
+#define MCP9982_MAX_NUM_CHANNELS		5
+
+#define MCP9982_CHAN(index, si, __address)					\
+{										\
+	.type = IIO_TEMP,							\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),				\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
+	BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),			\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
+	BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |			\
+	BIT(IIO_CHAN_INFO_HYSTERESIS),						\
+	.channel = index,							\
+	.address = __address,							\
+	.scan_index = si,							\
+	.scan_type = {								\
+		.sign = 'u',							\
+		.realbits = 8,							\
+		.storagebits = 8,						\
+	},									\
+	.indexed = 1,								\
+}
+
+/**
+ * struct mcp9982_features - features of a mcp9982 instance
+ * @name:		chip's name
+ * @phys_channels:	number of physical channels supported by the chip
+ */
+struct mcp9982_features {
+	const char	*name;
+	u8		phys_channels;
+};
+
+static const struct mcp9982_features mcp9933_chip_config = {
+	.name = "mcp9933",
+	.phys_channels = 3,
+};
+
+static const struct mcp9982_features mcp9933d_chip_config = {
+	.name = "mcp9933d",
+	.phys_channels = 3,
+};
+
+static const struct mcp9982_features mcp9982_chip_config = {
+	.name = "mcp9982",
+	.phys_channels = 2,
+};
+
+static const struct mcp9982_features mcp9982d_chip_config = {
+	.name = "mcp9982d",
+	.phys_channels = 2,
+};
+
+static const struct mcp9982_features mcp9983_chip_config = {
+	.name = "mcp9983",
+	.phys_channels = 3,
+};
+
+static const struct mcp9982_features mcp9983d_chip_config = {
+	.name = "mcp9983d",
+	.phys_channels = 3,
+};
+
+static const struct mcp9982_features mcp9984_chip_config = {
+	.name = "mcp9984",
+	.phys_channels = 4,
+};
+
+static const struct mcp9982_features mcp9984d_chip_config = {
+	.name = "mcp9984d",
+	.phys_channels = 4,
+};
+
+static const struct mcp9982_features mcp9985_chip_config = {
+	.name = "mcp9985",
+	.phys_channels = 5,
+};
+
+static const struct mcp9982_features mcp9985d_chip_config = {
+	.name = "mcp9985d",
+	.phys_channels = 5,
+};
+
+static const int mcp9982_fractional_values[] = {
+	0,
+	125000,
+	250000,
+	375000,
+	500000,
+	625000,
+	750000,
+	875000,
+};
+
+static const int mcp9982_conv_rate[][2] = {
+	{ 0, 62500 },
+	{ 0, 125000 },
+	{ 0, 250000 },
+	{ 0, 500000 },
+	{ 1, 0 },
+	{ 2, 0 },
+	{ 4, 0 },
+	{ 8, 0 },
+	{ 16, 0 },
+	{ 32, 0 },
+	{ 64, 0 },
+};
+
+static const char mcp9982_beta_values[][6] = {
+	"0.050",
+	"0.066",
+	"0.087",
+	"0.114",
+	"0.150",
+	"0.197",
+	"0.260",
+	"0.342",
+	"0.449",
+	"0.591",
+	"0.778",
+	"1.024",
+	"1.348",
+	"1.773",
+	"2.333",
+};
+
+unsigned int mcp9982_3db_values_map_tbl[11][3][2];
+static const int mcp9982_sampl_fr[][2] = {
+	{ 1, 16 },
+	{ 1, 8 },
+	{ 1, 4 },
+	{ 1, 2 },
+	{ 1, 1 },
+	{ 2, 1 },
+	{ 4, 1 },
+	{ 8, 1 },
+	{ 16, 1 },
+	{ 32, 1 },
+	{ 64, 1 },
+};
+
+static const int mcp9982_window_size[3] = {1, 4, 8};
+
+/*
+ * (Sampling_Frequency * 1000000) / (Window_Size * 2)
+ */
+static int mcp9982_calc_all_3db_values(void)
+{
+	int i, j;
+	u64 numerator;
+	u32 denominator, remainder;
+
+	for (i = 0; i < ARRAY_SIZE(mcp9982_window_size); i++)
+		for (j = 0; j <  ARRAY_SIZE(mcp9982_sampl_fr); j++) {
+			numerator = 1000000 * mcp9982_sampl_fr[j][0];
+			denominator = 2 * mcp9982_window_size[i] * mcp9982_sampl_fr[j][1];
+			numerator = div_u64_rem(numerator, denominator, &remainder);
+			mcp9982_3db_values_map_tbl[j][i][0] = div_u64_rem(numerator, 1000000,
+									  &remainder);
+			mcp9982_3db_values_map_tbl[j][i][1] = remainder;
+		}
+	return 0;
+}
+
+/* mcp9982 regmap configuration */
+static const struct regmap_range mcp9982_regmap_wr_ranges[] = {
+	regmap_reg_range(MCP9982_ONE_SHOT_ADDR, MCP9982_EXT1_LOW_LIMIT_FRAC_VALUE_ADDR),
+	regmap_reg_range(MCP9982_INTERNAL_THERM_LIMIT_ADDR, MCP9982_EXT1_THERM_LIMIT_ADDR),
+	regmap_reg_range(MCP9982_CFG_ADDR, MCP9982_CFG_ADDR),
+	regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_HOTTEST_CFG_ADDR),
+	regmap_reg_range(MCP9982_THERM_SHTDWN_CFG_ADDR, MCP9982_THERM_SHTDWN_CFG_ADDR),
+	regmap_reg_range(MCP9982_EXT_BETA_CFG_ADDR(0), MCP9982_EXT_IDEAL_ADDR(3)),
+};
+
+static const struct regmap_access_table mcp9982_regmap_wr_table = {
+	.yes_ranges = mcp9982_regmap_wr_ranges,
+	.n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_wr_ranges),
+};
+
+static const struct regmap_range mcp9982_regmap_rd_ranges[] = {
+	regmap_reg_range(MCP9982_INT_VALUE_ADDR(0),
+			 MCP9982_EXT1_LOW_LIMIT_FRAC_VALUE_ADDR),
+	regmap_reg_range(MCP9982_INTERNAL_THERM_LIMIT_ADDR, MCP9982_EXT1_THERM_LIMIT_ADDR),
+	regmap_reg_range(MCP9982_CFG_ADDR, MCP9982_CFG_ADDR),
+	regmap_reg_range(MCP9982_CONV_ADDR, MCP9982_EXT_IDEAL_ADDR(3)),
+};
+
+static const struct regmap_access_table mcp9982_regmap_rd_table = {
+	.yes_ranges = mcp9982_regmap_rd_ranges,
+	.n_yes_ranges = ARRAY_SIZE(mcp9982_regmap_rd_ranges),
+};
+
+static bool mcp9982_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MCP9982_ONE_SHOT_ADDR:
+	case MCP9982_INTERNAL_HIGH_LIMIT_ADDR:
+	case MCP9982_INTERNAL_LOW_LIMIT_ADDR:
+	case MCP9982_EXT1_HIGH_LIMIT_INT_VALUE_ADDR:
+	case MCP9982_EXT1_HIGH_LIMIT_FRAC_VALUE_ADDR:
+	case MCP9982_EXT1_LOW_LIMIT_INT_VALUE_ADDR:
+	case MCP9982_EXT1_LOW_LIMIT_FRAC_VALUE_ADDR:
+	case MCP9982_INTERNAL_THERM_LIMIT_ADDR:
+	case MCP9982_EXT1_THERM_LIMIT_ADDR:
+	case MCP9982_CFG_ADDR:
+	case MCP9982_CONV_ADDR:
+	case MCP9982_HYS_ADDR:
+	case MCP9982_CONSEC_ALRT_ADDR:
+	case MCP9982_ALRT_CFG_ADDR:
+	case MCP9982_RUNNING_AVG_ADDR:
+	case MCP9982_HOTTEST_CFG_ADDR:
+	case MCP9982_THERM_SHTDWN_CFG_ADDR:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static const struct regmap_config mcp9982_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.rd_table = &mcp9982_regmap_rd_table,
+	.wr_table = &mcp9982_regmap_wr_table,
+	.volatile_reg	= mcp9982_is_volatile_reg,
+};
+
+/**
+ * struct mcp9992_priv - information about chip parameters
+ * @num_channels		number of physical channels
+ * @extended_temp_range		use extended temperature range or not
+ * @beta_autodetect		state of beta autodetection
+ * @lock			synchronize access to driver's state members
+ * @regmap:			device register map
+ * @iio_chan			specifications of channels
+ * @labels			labels of the channels
+ * @ideality_value		values given by user to ideality factor for all channels
+ * @recd34_enable		state of REC on channels 3 and 4
+ * @recd12_enable		state of REC on channels 1 and 2
+ * @apdd_enable			state of anti-parallel diode mode
+ * @sampl_idx			index representing the current sampling frequency
+ */
+struct mcp9982_priv {
+	u8 num_channels;
+	bool extended_temp_range;
+	bool beta_autodetect[2];
+	/*
+	 * Synchronize access to private members, and ensure
+	 * atomicity of consecutive regmap operations.
+	 */
+	struct mutex lock;
+	struct regmap *regmap;
+	struct iio_chan_spec *iio_chan;
+	char *labels[MCP9982_MAX_NUM_CHANNELS];
+	int ideality_value[4];
+	int recd34_enable;
+	int recd12_enable;
+	int apdd_enable;
+	int sampl_idx;
+};
+
+static int mcp9982_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length, long mask)
+{
+	struct mcp9982_priv *priv = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*vals = mcp9982_conv_rate[0];
+		*length = ARRAY_SIZE(mcp9982_conv_rate) * 2;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*vals = mcp9982_3db_values_map_tbl[priv->sampl_idx][0];
+		*length = ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]) * 2;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct mcp9982_priv *priv = iio_priv(indio_dev);
+	int ret, index, index2;
+
+	ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&priv->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_read(priv->regmap, MCP9982_INT_VALUE_ADDR(chan->channel), val);
+		if (ret)
+			return ret;
+
+		/* The extended temperature range is offset by 64 degrees C */
+		if (priv->extended_temp_range)
+			*val -= 64;
+
+		ret = regmap_read(priv->regmap, MCP9982_FRAC_VALUE_ADDR(chan->channel), val2);
+		if (ret)
+			return ret;
+
+		/* Only the 3 MSB in low byte registers are used */
+		*val2 = mcp9982_fractional_values[*val2 >> 5];
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = mcp9982_conv_rate[priv->sampl_idx][0];
+		*val2 = mcp9982_conv_rate[priv->sampl_idx][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+
+		ret = regmap_read(priv->regmap, MCP9982_RUNNING_AVG_ADDR, &index2);
+		if (ret)
+			return ret;
+
+		if (index2 >= 2)
+			index2 -= 1;
+		*val = mcp9982_3db_values_map_tbl[priv->sampl_idx][index2][0];
+		*val2 = mcp9982_3db_values_map_tbl[priv->sampl_idx][index2][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_HYSTERESIS:
+		ret = regmap_read(priv->regmap, MCP9982_HYS_ADDR, &index);
+		if (ret)
+			return ret;
+
+		*val = index;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp9982_read_label(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			      char *label)
+{
+	struct mcp9982_priv *priv = iio_priv(indio_dev);
+
+	if (chan->channel < 0 || chan->channel > 4)
+		return -EINVAL;
+
+	return sysfs_emit(label, "%s\n", priv->labels[chan->channel]);
+}
+
+static int mcp9982_write_raw_get_fmt(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+				     long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_HYSTERESIS:
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct mcp9982_priv *priv = iio_priv(indio_dev);
+	int i, ret;
+
+	guard(mutex)(&priv->lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		for (i = 0; i < ARRAY_SIZE(mcp9982_conv_rate); i++)
+			if (val == mcp9982_conv_rate[i][0] && val2 == mcp9982_conv_rate[i][1])
+				break;
+
+		if (i >= ARRAY_SIZE(mcp9982_conv_rate))
+			return -EINVAL;
+
+		ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, i);
+		if (ret)
+			return ret;
+
+		priv->sampl_idx = i;
+		return 0;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		for (i = 0; i < ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]); i++)
+			if (val == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][0] &&
+			    val2 == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][1])
+				break;
+
+		if (i >= ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]))
+			return -EINVAL;
+
+		/*
+		 * Filter register is coded with values:
+		 *-0 for OFF
+		 *-1 or 2 for level 1
+		 *-3 for level 2
+		 */
+		if (i == 2)
+			i = 3;
+		ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, i);
+
+		return ret;
+	case IIO_CHAN_INFO_HYSTERESIS:
+		if (val < 0 || val > 255)
+			return -EINVAL;
+
+		ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, val);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t mcp9982_extended_temp_range_show(struct device *dev,
+						struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct mcp9982_priv *priv = iio_priv(indio_dev);
+
+	return sysfs_emit(buf, "%d\n", priv->extended_temp_range);
+}
+
+static ssize_t mcp9982_extended_temp_range_store(struct device *dev,
+						 struct device_attribute *attr,
+						 const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct mcp9982_priv *priv = iio_priv(indio_dev);
+	int ret, val;
+
+	ret = kstrtouint(buf, 10, &val);
+	if (ret)
+		return -EINVAL;
+
+	switch (val) {
+	case 0:
+		priv->extended_temp_range = false;
+		break;
+	case 1:
+		priv->extended_temp_range = true;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	guard(mutex)(&priv->lock);
+	ret = regmap_assign_bits(priv->regmap, MCP9982_CFG_ADDR, MCP9982_CFG_RANGE,
+				 priv->extended_temp_range);
+
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t mcp9982_show_beta(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct mcp9982_priv *priv = iio_priv(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	int val, ret;
+
+	/* When APDD is enabled, betas are locked to autodetection */
+	if (priv->apdd_enable)
+		return sysfs_emit(buf, "Auto\n");
+
+	ret = regmap_read(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), &val);
+	if (ret)
+		return ret;
+
+	if (val < 15)
+		return sysfs_emit(buf, "%s\n", mcp9982_beta_values[val]);
+
+	if (val == 15)
+		return sysfs_emit(buf, "Diode_Mode\n");
+	else
+		return sysfs_emit(buf, "Auto\n");
+}
+
+static ssize_t mcp9982_store_beta(struct device *dev, struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct mcp9982_priv *priv = iio_priv(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	int i;
+
+	/* When APDD is enabled, betas are locked to autodetection */
+	if (priv->apdd_enable)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(mcp9982_beta_values); i++)
+		if (strncmp(buf, mcp9982_beta_values[i], 5) == 0)
+			break;
+
+	if (i < ARRAY_SIZE(mcp9982_beta_values)) {
+		regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), i);
+		return count;
+	}
+
+	if (strncmp(buf, "Diode_Mode", 10) == 0) {
+		regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), 15);
+		return count;
+	}
+
+	if (strncmp(buf, "Auto", 4) == 0) {
+		regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), BIT(4));
+		return count;
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t mcp9982_beta_available_show(struct device *dev,
+					   struct device_attribute *attr, char *buf)
+{
+	int i;
+
+	for (i = 0; i < 15; i++) {
+		strcat(buf, mcp9982_beta_values[i]);
+		strcat(buf, " ");
+	}
+	strcat(buf, "Diode_Mode Auto\n");
+	return sysfs_emit(buf, buf);
+}
+
+static IIO_DEVICE_ATTR(enable_extended_temp_range, 0644, mcp9982_extended_temp_range_show,
+		       mcp9982_extended_temp_range_store, 0);
+static IIO_DEVICE_ATTR(in_beta1, 0644, mcp9982_show_beta, mcp9982_store_beta, 0);
+static IIO_DEVICE_ATTR(in_beta2, 0644, mcp9982_show_beta, mcp9982_store_beta, 1);
+static IIO_DEVICE_ATTR(in_beta_available, 0444, mcp9982_beta_available_show, NULL, 0);
+
+static struct attribute *mcp9982_attributes[] = {
+	&iio_dev_attr_enable_extended_temp_range.dev_attr.attr,
+	&iio_dev_attr_in_beta1.dev_attr.attr,
+	&iio_dev_attr_in_beta2.dev_attr.attr,
+	&iio_dev_attr_in_beta_available.dev_attr.attr,
+	NULL,
+};
+
+static struct attribute_group mcp9982_attribute_group = {
+	.attrs = mcp9982_attributes,
+};
+
+static const struct iio_info mcp9982_info = {
+	.read_raw = mcp9982_read_raw,
+	.read_label = mcp9982_read_label,
+	.read_avail = mcp9982_read_avail,
+	.write_raw_get_fmt = mcp9982_write_raw_get_fmt,
+	.write_raw = mcp9982_write_raw,
+	.attrs = &mcp9982_attribute_group,
+};
+
+static int mcp9982_init(struct mcp9982_priv *priv)
+{
+	int ret, i;
+	u8 val;
+
+	/*
+	 * Set default values in registers.
+	 * APDD, RECD12 and RECD34 are active on 0.
+	 */
+	val = FIELD_PREP(MCP9982_CFG_MSKAL, 1) | FIELD_PREP(MCP9982_CFG_RS, 1) |
+	      FIELD_PREP(MCP9982_CFG_ATTHM, 1) |
+	      FIELD_PREP(MCP9982_CFG_RECD12, !priv->recd12_enable) |
+	      FIELD_PREP(MCP9982_CFG_RECD34, !priv->recd34_enable) |
+	      FIELD_PREP(MCP9982_CFG_RANGE, 0) | FIELD_PREP(MCP9982_CFG_DA_ENA, 0) |
+	      FIELD_PREP(MCP9982_CFG_APDD, !priv->apdd_enable);
+
+	ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val);
+	if (ret)
+		return ret;
+	priv->extended_temp_range = false;
+
+	ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, 6);
+	if (ret)
+		return ret;
+	priv->sampl_idx = 6;
+
+	ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, 10);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, 112);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0);
+	if (ret)
+		return ret;
+
+	/* Set beta compensation for channels 1 and 2 */
+	for (i = 0; i < 2; i++) {
+		priv->beta_autodetect[i] = true;
+		ret = regmap_assign_bits(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
+					 MCP9982_EXT_BETA_ENBL, 1);
+		if (ret)
+			return ret;
+	}
+	/* Set ideality factor for all external channels */
+	for (i = 0; i < 4; i++) {
+		ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i),
+				   priv->ideality_value[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int mcp9982_parse_of_config(struct iio_dev *indio_dev, struct device *dev,
+				   int device_nr_channels)
+{
+	struct mcp9982_priv *priv = iio_priv(indio_dev);
+	int reg_nr, iio_idx;
+
+	/* APDD, RECD12 and RECD34 are active on 0 */
+	if (device_property_read_bool(dev, "microchip,apdd-state"))
+		priv->apdd_enable = true;
+	else
+		priv->apdd_enable = false;
+
+	if (device_property_read_bool(dev, "microchip,recd12"))
+		priv->recd12_enable = true;
+	else
+		priv->recd12_enable = false;
+
+	if (device_property_read_bool(dev, "microchip,recd34"))
+		priv->recd34_enable = true;
+	else
+		priv->recd34_enable = false;
+
+	priv->num_channels = device_get_child_node_count(dev) + 1;
+
+	if (priv->num_channels > device_nr_channels)
+		return dev_err_probe(dev, -EINVAL, "More channels than the chip supports\n");
+
+	priv->iio_chan = devm_kcalloc(dev, priv->num_channels, sizeof(*priv->iio_chan), GFP_KERNEL);
+	if (!priv->iio_chan)
+		return -ENOMEM;
+
+	priv->iio_chan[0] = (struct iio_chan_spec)MCP9982_CHAN(0, 0, MCP9982_INT_VALUE_ADDR(0));
+
+	priv->labels[0] = "internal diode";
+	iio_idx++;
+	device_for_each_child_node_scoped(dev, child) {
+		fwnode_property_read_u32(child, "reg", &reg_nr);
+		if (!reg_nr || reg_nr >= device_nr_channels)
+			return dev_err_probe(dev, -EINVAL,
+				     "The index of the channels does not match the chip\n");
+
+		if (fwnode_property_present(child, "microchip,ideality-factor")) {
+			fwnode_property_read_u32(child, "microchip,ideality-factor",
+						 &priv->ideality_value[reg_nr - 1]);
+			if (priv->ideality_value[reg_nr - 1] > 63)
+				return dev_err_probe(dev, -EINVAL,
+				     "The ideality value is higher than maximum\n");
+		} else {
+			/* Set default value */
+			priv->ideality_value[reg_nr - 1] = 18;
+		}
+
+		fwnode_property_read_string(child, "label",
+					    (const char **)&priv->labels[reg_nr]);
+
+		priv->iio_chan[iio_idx++] = (struct iio_chan_spec)MCP9982_CHAN(reg_nr, reg_nr,
+							 MCP9982_INT_VALUE_ADDR(reg_nr));
+	}
+
+	return 0;
+}
+
+static int mcp9982_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct mcp9982_priv *priv;
+	struct iio_dev *indio_dev;
+	const struct mcp9982_features *chip;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	priv = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+
+	priv->regmap = devm_regmap_init_i2c(client, &mcp9982_regmap_config);
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(dev, PTR_ERR(priv->regmap),
+				     "Cannot initialize register map\n");
+
+	ret = devm_mutex_init(dev, &priv->lock);
+	if (ret)
+		return ret;
+
+	chip = i2c_get_match_data(client);
+	if (!chip)
+		return -EINVAL;
+
+	ret = mcp9982_parse_of_config(indio_dev, &client->dev, chip->phys_channels);
+	if (ret)
+		return dev_err_probe(dev, ret, "Parameter parsing error\n");
+
+	ret = mcp9982_init(priv);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot initialize device\n");
+
+	indio_dev->name = chip->name;
+	indio_dev->info = &mcp9982_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = priv->iio_chan;
+	indio_dev->num_channels = priv->num_channels;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot register IIO device\n");
+
+	mcp9982_calc_all_3db_values();
+	return 0;
+}
+
+static const struct i2c_device_id mcp9982_id[] = {
+	{ .name = "mcp9933", .driver_data = (kernel_ulong_t)&mcp9933_chip_config },
+	{ .name = "mcp9933d", .driver_data = (kernel_ulong_t)&mcp9933d_chip_config },
+	{ .name = "mcp9982", .driver_data = (kernel_ulong_t)&mcp9982_chip_config },
+	{ .name = "mcp9982d", .driver_data = (kernel_ulong_t)&mcp9982d_chip_config },
+	{ .name = "mcp9983", .driver_data = (kernel_ulong_t)&mcp9983_chip_config },
+	{ .name = "mcp9983d", .driver_data = (kernel_ulong_t)&mcp9983d_chip_config },
+	{ .name = "mcp9984", .driver_data = (kernel_ulong_t)&mcp9984_chip_config },
+	{ .name = "mcp9984d", .driver_data = (kernel_ulong_t)&mcp9984d_chip_config },
+	{ .name = "mcp9985", .driver_data = (kernel_ulong_t)&mcp9985_chip_config },
+	{ .name = "mcp9985d", .driver_data = (kernel_ulong_t)&mcp9985d_chip_config },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mcp9982_id);
+
+static const struct of_device_id mcp9982_of_match[] = {
+	{
+		.compatible = "microchip,mcp9933",
+		.data = &mcp9933_chip_config
+	}, {
+		.compatible = "microchip,mcp9933d",
+		.data = &mcp9933d_chip_config
+	}, {
+		.compatible = "microchip,mcp9982",
+		.data = &mcp9982_chip_config
+	}, {
+		.compatible = "microchip,mcp9982d",
+		.data = &mcp9982d_chip_config
+	}, {
+		.compatible = "microchip,mcp9983",
+		.data = &mcp9983_chip_config
+	}, {
+		.compatible = "microchip,mcp9983d",
+		.data = &mcp9983d_chip_config
+	}, {
+		.compatible = "microchip,mcp9984",
+		.data = &mcp9984_chip_config
+	}, {
+		.compatible = "microchip,mcp9984d",
+		.data = &mcp9984d_chip_config
+	}, {
+		.compatible = "microchip,mcp9985",
+		.data = &mcp9985_chip_config
+	}, {
+		.compatible = "microchip,mcp9985d",
+		.data = &mcp9985d_chip_config
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mcp9982_of_match);
+
+static struct i2c_driver mcp9982_driver = {
+	.driver	 = {
+		.name = "mcp9982",
+		.of_match_table = mcp9982_of_match,
+	},
+	.probe = mcp9982_probe,
+	.id_table = mcp9982_id,
+};
+module_i2c_driver(mcp9982_driver);
+
+MODULE_AUTHOR("Victor Duicu <victor.duicu@microchip.com>");
+MODULE_DESCRIPTION("MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Driver");
+MODULE_LICENSE("GPL");
-- 
2.48.1

Re: [PATCH v2 2/2] iio: temperature: add support for MCP998X

[Andy Shevchenko]

On Thu, May 29, 2025 at 12:37 PM <victor.duicu@microchip.com> wrote:
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.

...

> +/*
> + * (Sampling_Frequency * 1000000) / (Window_Size * 2)
> + */
> +static int mcp9982_calc_all_3db_values(void)
> +{

> +       int i, j;

Why signed?

> +       u64 numerator;
> +       u32 denominator, remainder;
> +
> +       for (i = 0; i < ARRAY_SIZE(mcp9982_window_size); i++)
> +               for (j = 0; j <  ARRAY_SIZE(mcp9982_sampl_fr); j++) {

> +                       numerator = 1000000 * mcp9982_sampl_fr[j][0];

MICRO ? Ditto for the below case.

> +                       denominator = 2 * mcp9982_window_size[i] * mcp9982_sampl_fr[j][1];
> +                       numerator = div_u64_rem(numerator, denominator, &remainder);

The remainder seems unused here. So, why do you use the div_u64_rem()
and not simply do_div()?

> +                       mcp9982_3db_values_map_tbl[j][i][0] = div_u64_rem(numerator, 1000000,
> +                                                                         &remainder);
> +                       mcp9982_3db_values_map_tbl[j][i][1] = remainder;
> +               }
> +       return 0;
> +}

...

> +struct mcp9982_priv {
> +       u8 num_channels;
> +       bool extended_temp_range;
> +       bool beta_autodetect[2];
> +       /*
> +        * Synchronize access to private members, and ensure
> +        * atomicity of consecutive regmap operations.
> +        */
> +       struct mutex lock;

> +       struct regmap *regmap;

Wouldn't moving this to be the first member help with the code
generation (size)?

> +       struct iio_chan_spec *iio_chan;
> +       char *labels[MCP9982_MAX_NUM_CHANNELS];
> +       int ideality_value[4];
> +       int recd34_enable;
> +       int recd12_enable;
> +       int apdd_enable;
> +       int sampl_idx;
> +};

...

> +static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +                           int *val, int *val2, long mask)
> +{
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       int ret, index, index2;

regmap API takes unsigned int, why are these signed? And in general
it's better not to mix the returning variable with something
semantically different.

> +       ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
> +       if (ret)
> +               return ret;
> +
> +       guard(mutex)(&priv->lock);
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               ret = regmap_read(priv->regmap, MCP9982_INT_VALUE_ADDR(chan->channel), val);
> +               if (ret)
> +                       return ret;
> +
> +               /* The extended temperature range is offset by 64 degrees C */
> +               if (priv->extended_temp_range)
> +                       *val -= 64;
> +
> +               ret = regmap_read(priv->regmap, MCP9982_FRAC_VALUE_ADDR(chan->channel), val2);
> +               if (ret)
> +                       return ret;
> +
> +               /* Only the 3 MSB in low byte registers are used */
> +               *val2 = mcp9982_fractional_values[*val2 >> 5];
> +               return IIO_VAL_INT_PLUS_MICRO;
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               *val = mcp9982_conv_rate[priv->sampl_idx][0];
> +               *val2 = mcp9982_conv_rate[priv->sampl_idx][1];
> +               return IIO_VAL_INT_PLUS_MICRO;
> +       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +
> +               ret = regmap_read(priv->regmap, MCP9982_RUNNING_AVG_ADDR, &index2);
> +               if (ret)
> +                       return ret;
> +
> +               if (index2 >= 2)
> +                       index2 -= 1;

Sounds like a clamp(). (Note, what if for whatever reason you will get
index2 bigger than array size?

> +               *val = mcp9982_3db_values_map_tbl[priv->sampl_idx][index2][0];
> +               *val2 = mcp9982_3db_values_map_tbl[priv->sampl_idx][index2][1];
> +               return IIO_VAL_INT_PLUS_MICRO;
> +       case IIO_CHAN_INFO_HYSTERESIS:
> +               ret = regmap_read(priv->regmap, MCP9982_HYS_ADDR, &index);
> +               if (ret)
> +                       return ret;
> +
> +               *val = index;
> +               return IIO_VAL_INT;
> +       default:
> +               return -EINVAL;
> +       }
> +}

...

> +static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +                            int val, int val2, long mask)
> +{
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       int i, ret;

Why is 'i' signed?

> +       guard(mutex)(&priv->lock);
> +       switch (mask) {
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               for (i = 0; i < ARRAY_SIZE(mcp9982_conv_rate); i++)
> +                       if (val == mcp9982_conv_rate[i][0] && val2 == mcp9982_conv_rate[i][1])
> +                               break;

> +               if (i >= ARRAY_SIZE(mcp9982_conv_rate))

What is the meaning of '>' here?

> +                       return -EINVAL;

> +               ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, i);
> +               if (ret)
> +                       return ret;
> +
> +               priv->sampl_idx = i;
> +               return 0;
> +       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +               for (i = 0; i < ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]); i++)
> +                       if (val == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][0] &&
> +                           val2 == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][1])
> +                               break;

> +               if (i >= ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]))
> +                       return -EINVAL;

Ditto.

> +               /*
> +                * Filter register is coded with values:
> +                *-0 for OFF
> +                *-1 or 2 for level 1
> +                *-3 for level 2

This lists the negative values.... Are you sure the comment is aligned
with what the code is really doing?

> +                */
> +               if (i == 2)
> +                       i = 3;
> +               ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, i);
> +
> +               return ret;
> +       case IIO_CHAN_INFO_HYSTERESIS:
> +               if (val < 0 || val > 255)
> +                       return -EINVAL;
> +
> +               ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, val);
> +               return ret;
> +       default:
> +               return -EINVAL;
> +       }
> +}

...

> +static ssize_t mcp9982_extended_temp_range_store(struct device *dev,
> +                                                struct device_attribute *attr,
> +                                                const char *buf, size_t count)
> +{
> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       int ret, val;
> +
> +       ret = kstrtouint(buf, 10, &val);
> +       if (ret)
> +               return -EINVAL;

Why is the error code shadowed?

> +       switch (val) {
> +       case 0:
> +               priv->extended_temp_range = false;
> +               break;
> +       case 1:
> +               priv->extended_temp_range = true;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }

Useless, use kstrtobool() instead.

> +       guard(mutex)(&priv->lock);
> +       ret = regmap_assign_bits(priv->regmap, MCP9982_CFG_ADDR, MCP9982_CFG_RANGE,
> +                                priv->extended_temp_range);
> +
> +       if (ret)
> +               return ret;
> +
> +       return count;
> +}

...

> +static ssize_t mcp9982_show_beta(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +       int val, ret;

Why is val signed? Please, check your code and fix types all over the place.

> +       /* When APDD is enabled, betas are locked to autodetection */
> +       if (priv->apdd_enable)
> +               return sysfs_emit(buf, "Auto\n");
> +
> +       ret = regmap_read(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), &val);
> +       if (ret)
> +               return ret;
> +
> +       if (val < 15)
> +               return sysfs_emit(buf, "%s\n", mcp9982_beta_values[val]);

> +       if (val == 15)
> +               return sysfs_emit(buf, "Diode_Mode\n");

> +       else

Redundant 'else'.

> +               return sysfs_emit(buf, "Auto\n");
> +}
> +
> +static ssize_t mcp9982_store_beta(struct device *dev, struct device_attribute *attr,
> +                                 const char *buf, size_t count)
> +{
> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);

> +       int i;

Signed? Why?

> +       /* When APDD is enabled, betas are locked to autodetection */
> +       if (priv->apdd_enable)
> +               return -EINVAL;

The below looks like an attempt to reimplement sysfs_match_string().

> +       for (i = 0; i < ARRAY_SIZE(mcp9982_beta_values); i++)
> +               if (strncmp(buf, mcp9982_beta_values[i], 5) == 0)
> +                       break;
> +
> +       if (i < ARRAY_SIZE(mcp9982_beta_values)) {
> +               regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), i);
> +               return count;
> +       }
> +
> +       if (strncmp(buf, "Diode_Mode", 10) == 0) {
> +               regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), 15);
> +               return count;
> +       }
> +
> +       if (strncmp(buf, "Auto", 4) == 0) {
> +               regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), BIT(4));
> +               return count;
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static ssize_t mcp9982_beta_available_show(struct device *dev,
> +                                          struct device_attribute *attr, char *buf)
> +{
> +       int i;
> +
> +       for (i = 0; i < 15; i++) {

> +               strcat(buf, mcp9982_beta_values[i]);
> +               strcat(buf, " ");

Huh?!

> +       }
> +       strcat(buf, "Diode_Mode Auto\n");
> +       return sysfs_emit(buf, buf);

What does this mean, please?

> +}
> +
> +static IIO_DEVICE_ATTR(enable_extended_temp_range, 0644, mcp9982_extended_temp_range_show,
> +                      mcp9982_extended_temp_range_store, 0);
> +static IIO_DEVICE_ATTR(in_beta1, 0644, mcp9982_show_beta, mcp9982_store_beta, 0);
> +static IIO_DEVICE_ATTR(in_beta2, 0644, mcp9982_show_beta, mcp9982_store_beta, 1);
> +static IIO_DEVICE_ATTR(in_beta_available, 0444, mcp9982_beta_available_show, NULL, 0);

First of all, we have IIO_DEVICE_ATTR_RO/RW, second, move each of them
to be closer to the related callback(s).

...

> +static struct attribute *mcp9982_attributes[] = {
> +       &iio_dev_attr_enable_extended_temp_range.dev_attr.attr,
> +       &iio_dev_attr_in_beta1.dev_attr.attr,
> +       &iio_dev_attr_in_beta2.dev_attr.attr,
> +       &iio_dev_attr_in_beta_available.dev_attr.attr,
> +       NULL,

No comma for the terminator.

> +};

I stop there as it reminds me that I had commented on something
similar in the past and nothing here was following those comments. Am
I correct?

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: temperature: add support for MCP998X

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 0c86e33819785fe50616b6ee3fb35c1e4be406d5]

url:    https://github.com/intel-lab-lkp/linux/commits/victor-duicu-microchip-com/dt-bindings-iio-temperature-add-support-for-MCP998X/20250529-173844
base:   0c86e33819785fe50616b6ee3fb35c1e4be406d5
patch link:    https://lore.kernel.org/r/20250529093628.15042-3-victor.duicu%40microchip.com
patch subject: [PATCH v2 2/2] iio: temperature: add support for MCP998X
config: m68k-randconfig-r111-20250530 (https://download.01.org/0day-ci/archive/20250530/202505301210.l4ZriiEX-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.4.0
reproduce: (https://download.01.org/0day-ci/archive/20250530/202505301210.l4ZriiEX-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505301210.l4ZriiEX-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/temperature/mcp9982.c:200:14: sparse: sparse: symbol 'mcp9982_3db_values_map_tbl' was not declared. Should it be static?

vim +/mcp9982_3db_values_map_tbl +200 drivers/iio/temperature/mcp9982.c

   199	
 > 200	unsigned int mcp9982_3db_values_map_tbl[11][3][2];
   201	static const int mcp9982_sampl_fr[][2] = {
   202		{ 1, 16 },
   203		{ 1, 8 },
   204		{ 1, 4 },
   205		{ 1, 2 },
   206		{ 1, 1 },
   207		{ 2, 1 },
   208		{ 4, 1 },
   209		{ 8, 1 },
   210		{ 16, 1 },
   211		{ 32, 1 },
   212		{ 64, 1 },
   213	};
   214	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/2] iio: temperature: add support for MCP998X

[Jonathan Cameron]

On Thu, 29 May 2025 12:36:28 +0300
victor.duicu@microchip.com wrote:

> From: Victor Duicu <victor.duicu@microchip.com>
> 
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.
> 
> Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
Hi Victor,

As often the case, the questions are mostly about the custom ABI.

Jonathan

> ---
>  .../testing/sysfs-bus-iio-temperature-mcp9982 |  27 +
>  MAINTAINERS                                   |   7 +
>  drivers/iio/temperature/Kconfig               |  10 +
>  drivers/iio/temperature/Makefile              |   1 +
>  drivers/iio/temperature/mcp9982.c             | 866 ++++++++++++++++++
>  5 files changed, 911 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
>  create mode 100644 drivers/iio/temperature/mcp9982.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
> new file mode 100644
> index 000000000000..c55e106e5863
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
> @@ -0,0 +1,27 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/enable_extended_temp_range
> +KernelVersion:	6.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute enables extended range of temperatures.
> +		Value 1 uses the extended range, value 0 uses the default range.

As mentioned below, if this corresponds to -64 as the offset, can we expose
it as in_voltage_offset or similar?

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_beta1
> +KernelVersion:	6.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute controls the value of beta correction
> +		for channel 1.

Is this something that we'd normally expect to manually update? What is
it a characteristic of?  If it is expected to the be related to the
diodes attached, that's a problem for firmware/dt, not sysfs interfaces.


> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_beta2
> +KernelVersion:	6.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute controls the value of beta correction
> +		for channel 2.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_beta_available
> +KernelVersion:	6.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute displays the values that can be
> +		set for beta correction.

Definitely mention the automode in these docs.




> diff --git a/drivers/iio/temperature/mcp9982.c b/drivers/iio/temperature/mcp9982.c
> new file mode 100644
> index 000000000000..89e966beffe6
> --- /dev/null
> +++ b/drivers/iio/temperature/mcp9982.c
> @@ -0,0 +1,866 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IIO driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family
> + *
> + * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Victor Duicu <victor.duicu@microchip.com>
> + *
> + * Datasheet can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/device/devres.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/irq.h>

Check if these are all needed.  This one doesn't seem to be...

> +#include <linux/limits.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>

> +
> +static const struct regmap_config mcp9982_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.rd_table = &mcp9982_regmap_rd_table,
> +	.wr_table = &mcp9982_regmap_wr_table,
> +	.volatile_reg	= mcp9982_is_volatile_reg,
I'd use single space before =
Forcing alignment goes wrong far too often and doesn't really help with readability.

> +};

> +static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct mcp9982_priv *priv = iio_priv(indio_dev);
> +	int ret, index, index2;
> +
> +	ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&priv->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(priv->regmap, MCP9982_INT_VALUE_ADDR(chan->channel), val);
> +		if (ret)
> +			return ret;
> +
> +		/* The extended temperature range is offset by 64 degrees C */
> +		if (priv->extended_temp_range)
> +			*val -= 64;

As that's the case, can we control this simply via standard ABI of IIO_CHAN_INFO_OFFSET and
provide the available as well.  It is a little non obvious, but has the advantage of providing
the control you want with out the need for custom ABI.


> +
> +		ret = regmap_read(priv->regmap, MCP9982_FRAC_VALUE_ADDR(chan->channel), val2);
> +		if (ret)
> +			return ret;
> +
> +		/* Only the 3 MSB in low byte registers are used */
> +		*val2 = mcp9982_fractional_values[*val2 >> 5];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = mcp9982_conv_rate[priv->sampl_idx][0];
> +		*val2 = mcp9982_conv_rate[priv->sampl_idx][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +
> +		ret = regmap_read(priv->regmap, MCP9982_RUNNING_AVG_ADDR, &index2);
> +		if (ret)
> +			return ret;
> +
> +		if (index2 >= 2)
> +			index2 -= 1;
> +		*val = mcp9982_3db_values_map_tbl[priv->sampl_idx][index2][0];
> +		*val2 = mcp9982_3db_values_map_tbl[priv->sampl_idx][index2][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_HYSTERESIS:
> +		ret = regmap_read(priv->regmap, MCP9982_HYS_ADDR, &index);
> +		if (ret)
> +			return ret;
> +
> +		*val = index;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct mcp9982_priv *priv = iio_priv(indio_dev);
> +	int i, ret;
> +
> +	guard(mutex)(&priv->lock);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		for (i = 0; i < ARRAY_SIZE(mcp9982_conv_rate); i++)
> +			if (val == mcp9982_conv_rate[i][0] && val2 == mcp9982_conv_rate[i][1])
> +				break;
> +
> +		if (i >= ARRAY_SIZE(mcp9982_conv_rate))

It won't be greater than so == is appropriate.

> +			return -EINVAL;
> +
> +		ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, i);
> +		if (ret)
> +			return ret;
> +
> +		priv->sampl_idx = i;
> +		return 0;
> +	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +		for (i = 0; i < ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]); i++)
> +			if (val == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][0] &&
> +			    val2 == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][1])
> +				break;
> +
> +		if (i >= ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]))
As above.

> +			return -EINVAL;
> +
> +		/*
> +		 * Filter register is coded with values:
> +		 *-0 for OFF
> +		 *-1 or 2 for level 1
> +		 *-3 for level 2
> +		 */
> +		if (i == 2)
> +			i = 3;
> +		ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, i);
> +
> +		return ret;
> +	case IIO_CHAN_INFO_HYSTERESIS:
> +		if (val < 0 || val > 255)
> +			return -EINVAL;
> +
> +		ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, val);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static ssize_t mcp9982_extended_temp_range_store(struct device *dev,
> +						 struct device_attribute *attr,
> +						 const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct mcp9982_priv *priv = iio_priv(indio_dev);
> +	int ret, val;
> +
> +	ret = kstrtouint(buf, 10, &val);

If you end up keeping this we have kstrtobool which tends to be more flexible
for an on/off parameter.

> +	if (ret)
> +		return -EINVAL;
> +
> +	switch (val) {
> +	case 0:
> +		priv->extended_temp_range = false;
> +		break;
> +	case 1:
> +		priv->extended_temp_range = true;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	guard(mutex)(&priv->lock);
> +	ret = regmap_assign_bits(priv->regmap, MCP9982_CFG_ADDR, MCP9982_CFG_RANGE,
> +				 priv->extended_temp_range);
> +
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t mcp9982_show_beta(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct mcp9982_priv *priv = iio_priv(indio_dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	int val, ret;
> +
> +	/* When APDD is enabled, betas are locked to autodetection */
> +	if (priv->apdd_enable)
> +		return sysfs_emit(buf, "Auto\n");
> +
> +	ret = regmap_read(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val < 15)
> +		return sysfs_emit(buf, "%s\n", mcp9982_beta_values[val]);
> +
> +	if (val == 15)
> +		return sysfs_emit(buf, "Diode_Mode\n");
> +	else
> +		return sysfs_emit(buf, "Auto\n");
> +}
> +
> +static ssize_t mcp9982_store_beta(struct device *dev, struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct mcp9982_priv *priv = iio_priv(indio_dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	int i;
> +
> +	/* When APDD is enabled, betas are locked to autodetection */
> +	if (priv->apdd_enable)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(mcp9982_beta_values); i++)
> +		if (strncmp(buf, mcp9982_beta_values[i], 5) == 0)
> +			break;
> +
> +	if (i < ARRAY_SIZE(mcp9982_beta_values)) {
> +		regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), i);
> +		return count;
> +	}
> +
> +	if (strncmp(buf, "Diode_Mode", 10) == 0) {
> +		regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), 15);
> +		return count;
> +	}

This interface is non obvious. Add a full description to the ABI docs.

> +
> +	if (strncmp(buf, "Auto", 4) == 0) {
> +		regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), BIT(4));
> +		return count;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t mcp9982_beta_available_show(struct device *dev,
> +					   struct device_attribute *attr, char *buf)
> +{
> +	int i;
> +
> +	for (i = 0; i < 15; i++) {
> +		strcat(buf, mcp9982_beta_values[i]);
> +		strcat(buf, " ");
> +	}
> +	strcat(buf, "Diode_Mode Auto\n");

As above. What is this?

> +	return sysfs_emit(buf, buf);
> +}

> +
> +static int mcp9982_parse_of_config(struct iio_dev *indio_dev, struct device *dev,
> +				   int device_nr_channels)
> +{
> +	struct mcp9982_priv *priv = iio_priv(indio_dev);
> +	int reg_nr, iio_idx;
> +
> +	/* APDD, RECD12 and RECD34 are active on 0 */
> +	if (device_property_read_bool(dev, "microchip,apdd-state"))
> +		priv->apdd_enable = true;
> +	else
> +		priv->apdd_enable = false;

	priv->appd_enable = device_property_read_bool();

> +
> +	if (device_property_read_bool(dev, "microchip,recd12"))
> +		priv->recd12_enable = true;
> +	else
> +		priv->recd12_enable = false;
> +
> +	if (device_property_read_bool(dev, "microchip,recd34"))
> +		priv->recd34_enable = true;
> +	else
> +		priv->recd34_enable = false;
> +
> +	priv->num_channels = device_get_child_node_count(dev) + 1;
> +
> +	if (priv->num_channels > device_nr_channels)
> +		return dev_err_probe(dev, -EINVAL, "More channels than the chip supports\n");
> +
> +	priv->iio_chan = devm_kcalloc(dev, priv->num_channels, sizeof(*priv->iio_chan), GFP_KERNEL);
> +	if (!priv->iio_chan)
> +		return -ENOMEM;
> +
> +	priv->iio_chan[0] = (struct iio_chan_spec)MCP9982_CHAN(0, 0, MCP9982_INT_VALUE_ADDR(0));

as below.

> +
> +	priv->labels[0] = "internal diode";
> +	iio_idx++;
> +	device_for_each_child_node_scoped(dev, child) {
> +		fwnode_property_read_u32(child, "reg", &reg_nr);
> +		if (!reg_nr || reg_nr >= device_nr_channels)
> +			return dev_err_probe(dev, -EINVAL,
> +				     "The index of the channels does not match the chip\n");
> +
> +		if (fwnode_property_present(child, "microchip,ideality-factor")) {
> +			fwnode_property_read_u32(child, "microchip,ideality-factor",
> +						 &priv->ideality_value[reg_nr - 1]);
> +			if (priv->ideality_value[reg_nr - 1] > 63)
> +				return dev_err_probe(dev, -EINVAL,
> +				     "The ideality value is higher than maximum\n");
> +		} else {
> +			/* Set default value */
> +			priv->ideality_value[reg_nr - 1] = 18;
> +		}

Set default and rely on no side effects if the read fails + that default must
be a valid choice.

		priv->ideality_value[reg_nr - 1] = 18;
		fwnode_property_read_u32(child, "microchip,ideality-factor",
					 &priv->ideality_value[reg_nr - 1]);
		if (priv->ideality_vavlue[reg_nr - 1] > 63)
			return dev_err_probe();

> +
> +		fwnode_property_read_string(child, "label",
> +					    (const char **)&priv->labels[reg_nr]);

I'm curious why that cast is needed. Should the string in the priv be const?
Do we perhaps have an issue with that elsewhere?

> +
> +		priv->iio_chan[iio_idx++] = (struct iio_chan_spec)MCP9982_CHAN(reg_nr, reg_nr,
> +							 MCP9982_INT_VALUE_ADDR(reg_nr));

Seems very likely that the (struct iio_chan_spec) should be in the macro definition.

> +	}
> +
> +	return 0;
> +}
> +
> +static int mcp9982_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct mcp9982_priv *priv;
> +	struct iio_dev *indio_dev;
> +	const struct mcp9982_features *chip;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	priv = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

Is this used? If not don't set it.

> +
> +	priv->regmap = devm_regmap_init_i2c(client, &mcp9982_regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
> +				     "Cannot initialize register map\n");
> +
> +	ret = devm_mutex_init(dev, &priv->lock);
> +	if (ret)
> +		return ret;
> +
> +	chip = i2c_get_match_data(client);
> +	if (!chip)
> +		return -EINVAL;
> +
> +	ret = mcp9982_parse_of_config(indio_dev, &client->dev, chip->phys_channels);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Parameter parsing error\n");
> +
> +	ret = mcp9982_init(priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot initialize device\n");
> +
> +	indio_dev->name = chip->name;
> +	indio_dev->info = &mcp9982_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = priv->iio_chan;
> +	indio_dev->num_channels = priv->num_channels;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot register IIO device\n");
> +
> +	mcp9982_calc_all_3db_values();

Seems likely you want to do this before exposing the sysfs interfaces?
If not blank line before this return.

> +	return 0;
> +}

Re: [PATCH v2 2/2] iio: temperature: add support for MCP998X

[Victor.Duicu]

On Fri, 2025-05-30 at 17:53 +0100, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Thu, 29 May 2025 12:36:28 +0300
> victor.duicu@microchip.com wrote:
> 
> > From: Victor Duicu <victor.duicu@microchip.com>
> > 
> > This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> > Multichannel Automotive Temperature Monitor Family.
> > 
> > Signed-off-by: Victor Duicu <victor.duicu@microchip.com>
> Hi Victor,
> 
Hi Jonathan,

...

> 
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_beta1
> > +KernelVersion:       6.17
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             This attribute controls the value of beta correction
> > +             for channel 1.
> 
> Is this something that we'd normally expect to manually update? What
> is
> it a characteristic of?  If it is expected to the be related to the
> diodes attached, that's a problem for firmware/dt, not sysfs
> interfaces.
> 

Beta is a characteristic of the diode/transistor placed on the setup.
Different diodes require different betas.
So yes, beta value should be in devicetree.

...
> > 
> > +
> > +             priv->iio_chan[iio_idx++] = (struct
> > iio_chan_spec)MCP9982_CHAN(reg_nr, reg_nr,
> > +                                                     
> > MCP9982_INT_VALUE_ADDR(reg_nr));
> 
> Seems very likely that the (struct iio_chan_spec) should be in the
> macro definition.
> 

In version 1 the macro used to define the channels was:

> 
#define MCP9982_CHAN(index, si, __address) ({ \
	struct iio_chan_spec __chan = { \
		.type = IIO_TEMP, \
		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
		.info_mask_shared_by_all_available =
BIT(IIO_CHAN_INFO_SAMP_FREQ), \
		.info_mask_shared_by_all =
BIT(IIO_CHAN_INFO_SAMP_FREQ), \
		.channel = index, \
		.address = __address, \
		.scan_index = si, \
		.scan_type = { \
			.sign = 'u', \
			.realbits = 8, \
			.storagebits = 8, \
			.endianness = IIO_CPU \
		}, \
		.indexed = 1, \
	}; \
	__chan; \
})

which contains struct iio_chan_spec inside.
Andy suggested to use compound literal, so I switched to version 2.
I still use a macro because most of the parameters are common among the
channels.

With Kind Regards,
Victor Duicu

Re: [PATCH v2 2/2] iio: temperature: add support for MCP998X

[Jonathan Cameron]

On Mon, 2 Jun 2025 14:49:29 +0000
<Victor.Duicu@microchip.com> wrote:

> On Fri, 2025-05-30 at 17:53 +0100, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Thu, 29 May 2025 12:36:28 +0300
> > victor.duicu@microchip.com wrote:
> >   
> > > From: Victor Duicu <victor.duicu@microchip.com>
> > > 
> > > This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> > > Multichannel Automotive Temperature Monitor Family.
> > > 
> > > Signed-off-by: Victor Duicu <victor.duicu@microchip.com>  
> > Hi Victor,
> >   
> Hi Jonathan,
> 
> ...
> 
> >   
> > > +
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_beta1
> > > +KernelVersion:       6.17
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             This attribute controls the value of beta correction
> > > +             for channel 1.  
> > 
> > Is this something that we'd normally expect to manually update? What
> > is
> > it a characteristic of?  If it is expected to the be related to the
> > diodes attached, that's a problem for firmware/dt, not sysfs
> > interfaces.
> >   
> 
> Beta is a characteristic of the diode/transistor placed on the setup.
> Different diodes require different betas.
> So yes, beta value should be in devicetree.
> 
> ...
> > > 
> > > +
> > > +             priv->iio_chan[iio_idx++] = (struct
> > > iio_chan_spec)MCP9982_CHAN(reg_nr, reg_nr,
> > > +                                                     
> > > MCP9982_INT_VALUE_ADDR(reg_nr));  
> > 
> > Seems very likely that the (struct iio_chan_spec) should be in the
> > macro definition.
> >   
> 
> In version 1 the macro used to define the channels was:
> 
> >   
> #define MCP9982_CHAN(index, si, __address) ({ \
> 	struct iio_chan_spec __chan = { \
> 		.type = IIO_TEMP, \
> 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> 		.info_mask_shared_by_all_available =
> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> 		.info_mask_shared_by_all =
> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> 		.channel = index, \
> 		.address = __address, \
> 		.scan_index = si, \
> 		.scan_type = { \
> 			.sign = 'u', \
> 			.realbits = 8, \
> 			.storagebits = 8, \
> 			.endianness = IIO_CPU \
> 		}, \
> 		.indexed = 1, \
> 	}; \
> 	__chan; \
> })
> 
> which contains struct iio_chan_spec inside.
> Andy suggested to use compound literal, so I switched to version 2.
> I still use a macro because most of the parameters are common among the
> channels.

I think we can add the (struct iio_chan_spec) to the macro definition. 

> 
> With Kind Regards,
> Victor Duicu