[PATCH 0/8] Add support for ADT7481 in lm90

[PATCH 0/8] Add support for ADT7481 in lm90

[Slawomir Stepien]

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

This patch series adds support for ADT7481 in lm90.c driver and extends the
device-tree options for it.

The ADT7481 is quite similar to MAX6696 (already supported in lm90) so a lot of
code is reused.

One major problem in fitting the ADT7481 in lm90.c is the chip detection.
However it seems that the chip has been designed and produced with correct
values at locations: 0xfe (manufactured id) and 0xff (chip id), but this is not
documented in the datasheet.

$ i2cdump -y -f -r 254-255 1 0x4c
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
f0:                                           41 62                  Ab

The device-tree changes allow to: set the extended temperature range mode and
set the label and offset for specific channel.

Note: previous "attempts" for adding ADT7481 in lm90 where here: [1][2].

[1] https://www.spinics.net/lists/lm-sensors/msg25066.html
[2] https://marc.info/?l=lm-sensors&m=137786448326215&w=2

Slawomir Stepien (8):
      dt-bindings: hwmon: Add support for ADT7481 in lm90
      dt-bindings: hwmon: Add 'extended-range-enable' property support in lm90
      dt-bindings: hwmon: Allow specifying channels for lm90
      hwmon: (lm90) add support for ADT7481
      hwmon: (lm90) define maximum number of channels that are supported
      hwmon: (lm90) enable the extended temperature range
      hwmon: (lm90) read the channel's label from device-tree
      hwmon: (lm90) read the channel's offset from device-tree

 .../devicetree/bindings/hwmon/national,lm90.yaml          |  42 ++++
 Documentation/hwmon/lm90.rst                              |  12 +-
 drivers/hwmon/Kconfig                                     |  15 +-
 drivers/hwmon/lm90.c                                      | 251 ++++++++++++++++++++----
 4 files changed, 271 insertions(+), 49 deletions(-)

[PATCH 1/8] dt-bindings: hwmon: Add support for ADT7481 in lm90

[Slawomir Stepien]

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

The ADT7481 sensor is quite similar to MAX6696 so we can reuse a lot of
code from lm90.c driver.

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

diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
index 30db92977937..92d97ebefaae 100644
--- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
+++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
@@ -16,6 +16,7 @@ properties:
       - adi,adm1032
       - adi,adt7461
       - adi,adt7461a
+      - adi,adt7481
       - dallas,max6646
       - dallas,max6647
       - dallas,max6649
-- 
2.36.1

Re: [PATCH 1/8] dt-bindings: hwmon: Add support for ADT7481 in lm90

[Krzysztof Kozlowski]

On 20/05/2022 11:32, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> The ADT7481 sensor is quite similar to MAX6696 so we can reuse a lot of
> code from lm90.c driver.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH 2/8] dt-bindings: hwmon: Add 'extended-range-enable' property support in lm90

[Slawomir Stepien]

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

Using the 'extended-range-enable' prop will enable the extended
measurement range for device (if supported by the device).

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

diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
index 92d97ebefaae..066c02541fcf 100644
--- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
+++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
@@ -53,6 +53,9 @@ properties:
   vcc-supply:
     description: phandle to the regulator that provides the +VCC supply
 
+  extended-range-enable:
+    description: enables the extended temperature range measurement (if supported by device).
+
 required:
   - compatible
   - reg
-- 
2.36.1

Re: [PATCH 2/8] dt-bindings: hwmon: Add 'extended-range-enable' property support in lm90

[Krzysztof Kozlowski]

On 20/05/2022 11:32, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Using the 'extended-range-enable' prop will enable the extended
> measurement range for device (if supported by the device).

No. Please see other patchset:
https://lore.kernel.org/all/20220517135614.8185-1-holger.brunck@hitachienergy.com/


Best regards,
Krzysztof

Re: [PATCH 2/8] dt-bindings: hwmon: Add 'extended-range-enable' property support in lm90

[Slawomir Stepien]

On maj 20, 2022 12:09, Krzysztof Kozlowski wrote:
> On 20/05/2022 11:32, Slawomir Stepien wrote:
> > From: Slawomir Stepien <slawomir.stepien@nokia.com>
> > 
> > Using the 'extended-range-enable' prop will enable the extended
> > measurement range for device (if supported by the device).
> 
> No. Please see other patchset:
> https://lore.kernel.org/all/20220517135614.8185-1-holger.brunck@hitachienergy.com/

Oh that's nice! I will drop this patch then.

-- 
Slawomir Stepien

[PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Slawomir Stepien]

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

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

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

diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
index 066c02541fcf..9a5aa78d4db1 100644
--- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
+++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
@@ -62,6 +62,37 @@ required:
 
 additionalProperties: false
 
+patternProperties:
+  "^channel@([0-2])$":
+    type: object
+    description: |
+      Represents channels of the device and their specific configuration.
+
+    properties:
+      reg:
+        description: |
+          The channel number. 0 is local channel, 1-2 are remote channels.
+        items:
+          minimum: 0
+          maximum: 2
+
+      label:
+        description: |
+          A descriptive name for this channel, like "ambient" or "psu".
+
+      offset:
+        description: |
+          The value (millidegree Celsius) to be programmed in the channel specific offset register
+          (if supported by device).
+          For remote channels only.
+        $ref: /schemas/types.yaml#/definitions/int32
+        default: 0
+
+    required:
+      - reg
+
+    additionalProperties: false
+
 examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
@@ -76,5 +107,13 @@ examples:
             vcc-supply = <&palmas_ldo6_reg>;
             interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
             #thermal-sensor-cells = <1>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0x0>;
+                label = "internal";
+                offset = <1000>;
+            };
         };
     };
-- 
2.36.1

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Krzysztof Kozlowski]

On 20/05/2022 11:32, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Add binding description for temperature channels. Currently, support for
> label and offset is implemented.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> ---
>  .../bindings/hwmon/national,lm90.yaml         | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> index 066c02541fcf..9a5aa78d4db1 100644
> --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> @@ -62,6 +62,37 @@ required:
>  
>  additionalProperties: false
>  
> +patternProperties:

Which models use this?

> +  "^channel@([0-2])$":
> +    type: object
> +    description: |

No need for |

> +      Represents channels of the device and their specific configuration.
> +
> +    properties:
> +      reg:
> +        description: |

The same.

> +          The channel number. 0 is local channel, 1-2 are remote channels.
> +        items:
> +          minimum: 0
> +          maximum: 2
> +
> +      label:
> +        description: |

The same.

> +          A descriptive name for this channel, like "ambient" or "psu".
> +
> +      offset:
> +        description: |

This does not look like standard property, so you need vendor and unit
suffix.

> +          The value (millidegree Celsius) to be programmed in the channel specific offset register
> +          (if supported by device).

You described programming model which should not be put in the bindings.
Please describe the hardware.

> +          For remote channels only.
> +        $ref: /schemas/types.yaml#/definitions/int32
> +        default: 0
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
>  examples:
>    - |
>      #include <dt-bindings/interrupt-controller/irq.h>
> @@ -76,5 +107,13 @@ examples:
>              vcc-supply = <&palmas_ldo6_reg>;
>              interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
>              #thermal-sensor-cells = <1>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
I assume you tested the bindings with dt_bindings_check?

I have some doubts, as this should fail.

> +
> +            channel@0 {
> +                reg = <0x0>;
> +                label = "internal";
> +                offset = <1000>;
> +            };
>          };
>      };


Best regards,
Krzysztof

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Slawomir Stepien]

On maj 20, 2022 12:13, Krzysztof Kozlowski wrote:
> On 20/05/2022 11:32, Slawomir Stepien wrote:
> > From: Slawomir Stepien <slawomir.stepien@nokia.com>
> > 
> > Add binding description for temperature channels. Currently, support for
> > label and offset is implemented.
> > 
> > Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> > ---
> >  .../bindings/hwmon/national,lm90.yaml         | 39 +++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> > index 066c02541fcf..9a5aa78d4db1 100644
> > --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> > +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> > @@ -62,6 +62,37 @@ required:
> >  
> >  additionalProperties: false
> >  
> > +patternProperties:
> 
> Which models use this?

This is used in tmp421 model.

> > +  "^channel@([0-2])$":
> > +    type: object
> > +    description: |
> 
> No need for |

Will fix in v2.

> > +      Represents channels of the device and their specific configuration.
> > +
> > +    properties:
> > +      reg:
> > +        description: |
> 
> The same.

Will fix in v2.

> > +          The channel number. 0 is local channel, 1-2 are remote channels.
> > +        items:
> > +          minimum: 0
> > +          maximum: 2
> > +
> > +      label:
> > +        description: |
> 
> The same.

Will fix in v2.

> > +          A descriptive name for this channel, like "ambient" or "psu".
> > +
> > +      offset:
> > +        description: |
> 
> This does not look like standard property, so you need vendor and unit
> suffix.

Currently in lm90 we have support for devices that have different width (including sign) for offset
register. We have 10 bits, 11 bits and 12 bits. Do I understand correctly that I can use the same
vendor prefix if the width is the same? Just like "ti" was used for adi and ti in
"ti,extended-range-enable"?

For example:

adi,10-bit-offset-millicelsius # (only for adt7481)
adi,11-bit-offset-millicelsius # (for adt7461 but also for lm90 and others)
ti,12-bit-offset-millicelsius  # (ti - since only tmp451 and tmp461 supports 12 bit)

> > +          The value (millidegree Celsius) to be programmed in the channel specific offset register
> > +          (if supported by device).
> 
> You described programming model which should not be put in the bindings.
> Please describe the hardware.

I am also not sure about the "-millicelsius" in example above. From device point-of-view, this
offset is just two's complement, so is it more desirable to have the values here as just bytes
rather than millicelsius?

> > +          For remote channels only.
> > +        $ref: /schemas/types.yaml#/definitions/int32
> > +        default: 0
> > +
> > +    required:
> > +      - reg
> > +
> > +    additionalProperties: false
> > +
> >  examples:
> >    - |
> >      #include <dt-bindings/interrupt-controller/irq.h>
> > @@ -76,5 +107,13 @@ examples:
> >              vcc-supply = <&palmas_ldo6_reg>;
> >              interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
> >              #thermal-sensor-cells = <1>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> I assume you tested the bindings with dt_bindings_check?
> 
> I have some doubts, as this should fail.

I did. All was fine. What should fail here?

> > +
> > +            channel@0 {
> > +                reg = <0x0>;
> > +                label = "internal";
> > +                offset = <1000>;
> > +            };
> >          };
> >      };

-- 
Slawomir Stepien

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Krzysztof Kozlowski]

On 20/05/2022 14:38, Slawomir Stepien wrote:
> On maj 20, 2022 12:13, Krzysztof Kozlowski wrote:
>> On 20/05/2022 11:32, Slawomir Stepien wrote:
>>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>>
>>> Add binding description for temperature channels. Currently, support for
>>> label and offset is implemented.
>>>
>>> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
>>> ---
>>>  .../bindings/hwmon/national,lm90.yaml         | 39 +++++++++++++++++++
>>>  1 file changed, 39 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>>> index 066c02541fcf..9a5aa78d4db1 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>>> @@ -62,6 +62,37 @@ required:
>>>  
>>>  additionalProperties: false
>>>  
>>> +patternProperties:
>>
>> Which models use this?
> 
> This is used in tmp421 model.

Then please add allOf:if:then disallowing the property for other models.

> 
>>> +  "^channel@([0-2])$":
>>> +    type: object
>>> +    description: |
>>
>> No need for |
> 
> Will fix in v2.
> 
>>> +      Represents channels of the device and their specific configuration.
>>> +
>>> +    properties:
>>> +      reg:
>>> +        description: |
>>
>> The same.
> 
> Will fix in v2.
> 
>>> +          The channel number. 0 is local channel, 1-2 are remote channels.
>>> +        items:
>>> +          minimum: 0
>>> +          maximum: 2
>>> +
>>> +      label:
>>> +        description: |
>>
>> The same.
> 
> Will fix in v2.
> 
>>> +          A descriptive name for this channel, like "ambient" or "psu".
>>> +
>>> +      offset:
>>> +        description: |
>>
>> This does not look like standard property, so you need vendor and unit
>> suffix.
> 
> Currently in lm90 we have support for devices that have different width (including sign) for offset
> register. We have 10 bits, 11 bits and 12 bits. Do I understand correctly that I can use the same
> vendor prefix if the width is the same? Just like "ti" was used for adi and ti in
> "ti,extended-range-enable"?
> 
> For example:
> 
> adi,10-bit-offset-millicelsius # (only for adt7481)
> adi,11-bit-offset-millicelsius # (for adt7461 but also for lm90 and others)
> ti,12-bit-offset-millicelsius  # (ti - since only tmp451 and tmp461 supports 12 bit)

Wait, these are then strictly per-compatible, so there is no sense in DT
property at all.

> 
>>> +          The value (millidegree Celsius) to be programmed in the channel specific offset register
>>> +          (if supported by device).
>>
>> You described programming model which should not be put in the bindings.
>> Please describe the hardware.
> 
> I am also not sure about the "-millicelsius" in example above. From device point-of-view, this
> offset is just two's complement, so is it more desirable to have the values here as just bytes
> rather than millicelsius?

No, the programming model of a device can change and should be
abstracted to hardware property.

> 
>>> +          For remote channels only.
>>> +        $ref: /schemas/types.yaml#/definitions/int32
>>> +        default: 0
>>> +
>>> +    required:
>>> +      - reg
>>> +
>>> +    additionalProperties: false
>>> +
>>>  examples:
>>>    - |
>>>      #include <dt-bindings/interrupt-controller/irq.h>
>>> @@ -76,5 +107,13 @@ examples:
>>>              vcc-supply = <&palmas_ldo6_reg>;
>>>              interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
>>>              #thermal-sensor-cells = <1>;
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>> I assume you tested the bindings with dt_bindings_check?
>>
>> I have some doubts, as this should fail.
> 
> I did. All was fine. What should fail here?

This:

linux/out/Documentation/devicetree/bindings/hwmon/national,lm90.example.dtb:
sensor@4c: '#address-cells', '#size-cells' do not match any of the
regexes: '^channel@([0-2])$', 'pinctrl-[0-9]+'

	From schema:
linux/linux/Documentation/devicetree/bindings/hwmon/national,lm90.yaml


So no, you did not test it. Asking reviewer to perform a test which you
can do by yourself is a huge waste of reviewers time.

Best regards,
Krzysztof

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Slawomir Stepien]

On maj 20, 2022 14:47, Krzysztof Kozlowski wrote:
> On 20/05/2022 14:38, Slawomir Stepien wrote:
> > On maj 20, 2022 12:13, Krzysztof Kozlowski wrote:
> >> On 20/05/2022 11:32, Slawomir Stepien wrote:
> >>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> >>>
> >>> Add binding description for temperature channels. Currently, support for
> >>> label and offset is implemented.
> >>>
> >>> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> >>> ---
> >>>  .../bindings/hwmon/national,lm90.yaml         | 39 +++++++++++++++++++
> >>>  1 file changed, 39 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> >>> index 066c02541fcf..9a5aa78d4db1 100644
> >>> --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> >>> +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> >>> @@ -62,6 +62,37 @@ required:
> >>>  
> >>>  additionalProperties: false
> >>>  
> >>> +patternProperties:
> >>
> >> Which models use this?
> > 
> > This is used in tmp421 model.
> 
> Then please add allOf:if:then disallowing the property for other models.

A misunderstanding. The channel node can be used by every device supported by lm90. At least each
channel of each device can have label.

> >>> +  "^channel@([0-2])$":
> >>> +    type: object
> >>> +    description: |
> >>
> >> No need for |
> > 
> > Will fix in v2.
> > 
> >>> +      Represents channels of the device and their specific configuration.
> >>> +
> >>> +    properties:
> >>> +      reg:
> >>> +        description: |
> >>
> >> The same.
> > 
> > Will fix in v2.
> > 
> >>> +          The channel number. 0 is local channel, 1-2 are remote channels.
> >>> +        items:
> >>> +          minimum: 0
> >>> +          maximum: 2
> >>> +
> >>> +      label:
> >>> +        description: |
> >>
> >> The same.
> > 
> > Will fix in v2.
> > 
> >>> +          A descriptive name for this channel, like "ambient" or "psu".
> >>> +
> >>> +      offset:
> >>> +        description: |
> >>
> >> This does not look like standard property, so you need vendor and unit
> >> suffix.
> > 
> > Currently in lm90 we have support for devices that have different width (including sign) for offset
> > register. We have 10 bits, 11 bits and 12 bits. Do I understand correctly that I can use the same
> > vendor prefix if the width is the same? Just like "ti" was used for adi and ti in
> > "ti,extended-range-enable"?
> > 
> > For example:
> > 
> > adi,10-bit-offset-millicelsius # (only for adt7481)
> > adi,11-bit-offset-millicelsius # (for adt7461 but also for lm90 and others)
> > ti,12-bit-offset-millicelsius  # (ti - since only tmp451 and tmp461 supports 12 bit)
> 
> Wait, these are then strictly per-compatible, so there is no sense in DT
> property at all.

But isn't the value of offset a hardware-design-time calculation? So if I have a piece of
hardware that describes itself using device-tree, then offset information should be stored on the
device-tree rather then be "calculated" by the software running on that piece of hardware?

And what if such piece of hardware has been changed (e.g. new PCB version) and now the offset are
different? Then if device-tree is on hardware (e.g. on EEPROM) with new offsets, then software would
not require a change to support this new hardware version.

> >>> +          The value (millidegree Celsius) to be programmed in the channel specific offset register
> >>> +          (if supported by device).
> >>
> >> You described programming model which should not be put in the bindings.
> >> Please describe the hardware.
> > 
> > I am also not sure about the "-millicelsius" in example above. From device point-of-view, this
> > offset is just two's complement, so is it more desirable to have the values here as just bytes
> > rather than millicelsius?
> 
> No, the programming model of a device can change and should be
> abstracted to hardware property.

OK, so I will leave millicelsius as the unit.

> >>> +          For remote channels only.
> >>> +        $ref: /schemas/types.yaml#/definitions/int32
> >>> +        default: 0
> >>> +
> >>> +    required:
> >>> +      - reg
> >>> +
> >>> +    additionalProperties: false
> >>> +
> >>>  examples:
> >>>    - |
> >>>      #include <dt-bindings/interrupt-controller/irq.h>
> >>> @@ -76,5 +107,13 @@ examples:
> >>>              vcc-supply = <&palmas_ldo6_reg>;
> >>>              interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
> >>>              #thermal-sensor-cells = <1>;
> >>> +            #address-cells = <1>;
> >>> +            #size-cells = <0>;
> >> I assume you tested the bindings with dt_bindings_check?
> >>
> >> I have some doubts, as this should fail.
> > 
> > I did. All was fine. What should fail here?
> 
> This:
> 
> linux/out/Documentation/devicetree/bindings/hwmon/national,lm90.example.dtb:
> sensor@4c: '#address-cells', '#size-cells' do not match any of the
> regexes: '^channel@([0-2])$', 'pinctrl-[0-9]+'
> 
> 	From schema:
> linux/linux/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
> 
> 
> So no, you did not test it. Asking reviewer to perform a test which you
> can do by yourself is a huge waste of reviewers time.

Ah I see it too. This is not stopping the make dt_bindings_check and that is why I have missed it.
My apologies! I will be more careful next time!

-- 
Slawomir Stepien

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Krzysztof Kozlowski]

On 20/05/2022 15:23, Slawomir Stepien wrote:
>>>>>  
>>>>> +patternProperties:
>>>>
>>>> Which models use this?
>>>
>>> This is used in tmp421 model.
>>
>> Then please add allOf:if:then disallowing the property for other models.
> 
> A misunderstanding. The channel node can be used by every device supported by lm90. At least each
> channel of each device can have label.

OK

> 
>>>>> +  "^channel@([0-2])$":
>>>>> +    type: object
>>>>> +    description: |
>>>>
>>>> No need for |
>>>
>>> Will fix in v2.
>>>
>>>>> +      Represents channels of the device and their specific configuration.
>>>>> +
>>>>> +    properties:
>>>>> +      reg:
>>>>> +        description: |
>>>>
>>>> The same.
>>>
>>> Will fix in v2.
>>>
>>>>> +          The channel number. 0 is local channel, 1-2 are remote channels.
>>>>> +        items:
>>>>> +          minimum: 0
>>>>> +          maximum: 2
>>>>> +
>>>>> +      label:
>>>>> +        description: |
>>>>
>>>> The same.
>>>
>>> Will fix in v2.
>>>
>>>>> +          A descriptive name for this channel, like "ambient" or "psu".
>>>>> +
>>>>> +      offset:
>>>>> +        description: |
>>>>
>>>> This does not look like standard property, so you need vendor and unit
>>>> suffix.
>>>
>>> Currently in lm90 we have support for devices that have different width (including sign) for offset
>>> register. We have 10 bits, 11 bits and 12 bits. Do I understand correctly that I can use the same
>>> vendor prefix if the width is the same? Just like "ti" was used for adi and ti in
>>> "ti,extended-range-enable"?
>>>
>>> For example:
>>>
>>> adi,10-bit-offset-millicelsius # (only for adt7481)
>>> adi,11-bit-offset-millicelsius # (for adt7461 but also for lm90 and others)
>>> ti,12-bit-offset-millicelsius  # (ti - since only tmp451 and tmp461 supports 12 bit)
>>
>> Wait, these are then strictly per-compatible, so there is no sense in DT
>> property at all.
> 
> But isn't the value of offset a hardware-design-time calculation? So if I have a piece of
> hardware that describes itself using device-tree, then offset information should be stored on the
> device-tree rather then be "calculated" by the software running on that piece of hardware?
> 
> And what if such piece of hardware has been changed (e.g. new PCB version) and now the offset are
> different? Then if device-tree is on hardware (e.g. on EEPROM) with new offsets, then software would
> not require a change to support this new hardware version.

OK, I misunderstood.

This should be only one property then, choose some reasonable vendor,
and in allOf:if:then you can put constraints about minimum/maximum
values per model.

Best regards,
Krzysztof

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Guenter Roeck]

On 5/20/22 05:38, Slawomir Stepien wrote:
> On maj 20, 2022 12:13, Krzysztof Kozlowski wrote:
>> On 20/05/2022 11:32, Slawomir Stepien wrote:
>>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>>
>>> Add binding description for temperature channels. Currently, support for
>>> label and offset is implemented.
>>>
>>> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
>>> ---
>>>   .../bindings/hwmon/national,lm90.yaml         | 39 +++++++++++++++++++
>>>   1 file changed, 39 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>>> index 066c02541fcf..9a5aa78d4db1 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>>> @@ -62,6 +62,37 @@ required:
>>>   
>>>   additionalProperties: false
>>>   
>>> +patternProperties:
>>
>> Which models use this?
> 
> This is used in tmp421 model.
> 

The driver does not support tmp421; I assume you mean tmp481.

If we are talking about the temperature offset register, and use my patch
series as base, it would be:

ADM1023, ADM1032, ADT7461, ADT7461A, ADT7481, ADT7482, ADT7483, G781, LM90,
LM99, MAX6680, MAX6681, NCT72, NCT218, NCT214, NCT1008, W83L771, SA56004,
TMP451, TMP461.

I may have missed some.

Guenter

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Guenter Roeck]

On 5/20/22 03:13, Krzysztof Kozlowski wrote:
> On 20/05/2022 11:32, Slawomir Stepien wrote:
>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>
>> Add binding description for temperature channels. Currently, support for
>> label and offset is implemented.
>>
>> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
>> ---
>>   .../bindings/hwmon/national,lm90.yaml         | 39 +++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>> index 066c02541fcf..9a5aa78d4db1 100644
>> --- a/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/national,lm90.yaml
>> @@ -62,6 +62,37 @@ required:
>>   
>>   additionalProperties: false
>>   
>> +patternProperties:
> 
> Which models use this?
> 
>> +  "^channel@([0-2])$":
>> +    type: object
>> +    description: |
> 
> No need for |
> 
>> +      Represents channels of the device and their specific configuration.
>> +
>> +    properties:
>> +      reg:
>> +        description: |
> 
> The same.
> 
>> +          The channel number. 0 is local channel, 1-2 are remote channels.
>> +        items:
>> +          minimum: 0
>> +          maximum: 2
>> +
>> +      label:
>> +        description: |
> 
> The same.
> 
>> +          A descriptive name for this channel, like "ambient" or "psu".
>> +
>> +      offset:
>> +        description: |
> 
> This does not look like standard property, so you need vendor and unit
> suffix.
> 

Temperature offset is a standard property for temperature sensor
chips with external channels, implemented by a diode or transistor.
Making it non-standard will mean that we'll have lots of
"vendor,offset" properties, one each for each vendor selling
temperature sensor chips with external channels. This gets
more complicated here because the lm90 driver does support chips
from several different vendors. Almost all of them support
this functionality. Which vendor do you select in this case ?

I would suggest to use temperature-offset-milliseconds, though.

>> +          The value (millidegree Celsius) to be programmed in the channel specific offset register
>> +          (if supported by device).
> 
> You described programming model which should not be put in the bindings.
> Please describe the hardware.
> 

It is a configuration value, which is hardware dependent because
it depends on the temperature diode or transistor connected to the chip.

Guenter

>> +          For remote channels only.
>> +        $ref: /schemas/types.yaml#/definitions/int32
>> +        default: 0
>> +
>> +    required:
>> +      - reg
>> +
>> +    additionalProperties: false
>> +
>>   examples:
>>     - |
>>       #include <dt-bindings/interrupt-controller/irq.h>
>> @@ -76,5 +107,13 @@ examples:
>>               vcc-supply = <&palmas_ldo6_reg>;
>>               interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
>>               #thermal-sensor-cells = <1>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
> I assume you tested the bindings with dt_bindings_check?
> 
> I have some doubts, as this should fail.
> 
>> +
>> +            channel@0 {
>> +                reg = <0x0>;
>> +                label = "internal";
>> +                offset = <1000>;
>> +            };
>>           };
>>       };
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Krzysztof Kozlowski]

On 20/05/2022 15:42, Guenter Roeck wrote:
>>
>>> +          A descriptive name for this channel, like "ambient" or "psu".
>>> +
>>> +      offset:
>>> +        description: |
>>
>> This does not look like standard property, so you need vendor and unit
>> suffix.
>>
> 
> Temperature offset is a standard property for temperature sensor

The original description was strictly connected to registers, so that
one as not a standard. It seems it was just a wording...

> chips with external channels, implemented by a diode or transistor.
> Making it non-standard will mean that we'll have lots of
> "vendor,offset" properties, one each for each vendor selling
> temperature sensor chips with external channels. This gets
> more complicated here because the lm90 driver does support chips
> from several different vendors. Almost all of them support
> this functionality. Which vendor do you select in this case ?
> 
> I would suggest to use temperature-offset-milliseconds, though.

Yes, this sounds good. Just not seconds but millicelsius, I guess?

> 
>>> +          The value (millidegree Celsius) to be programmed in the channel specific offset register
>>> +          (if supported by device).
>>
>> You described programming model which should not be put in the bindings.
>> Please describe the hardware.
>>
> 
> It is a configuration value, which is hardware dependent because
> it depends on the temperature diode or transistor connected to the chip.

Sure, so this could be reworded "Offset against some base value for each
channel temperature", or something similar (you know better than me).
Referring to registers and where exactly this should be programmed in
the device is related to device programming model, not to bindings.

Best regards,
Krzysztof

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Guenter Roeck]

On 5/20/22 07:09, Krzysztof Kozlowski wrote:
> On 20/05/2022 15:42, Guenter Roeck wrote:
>>>
>>>> +          A descriptive name for this channel, like "ambient" or "psu".
>>>> +
>>>> +      offset:
>>>> +        description: |
>>>
>>> This does not look like standard property, so you need vendor and unit
>>> suffix.
>>>
>>
>> Temperature offset is a standard property for temperature sensor
> 
> The original description was strictly connected to registers, so that
> one as not a standard. It seems it was just a wording...
> 
>> chips with external channels, implemented by a diode or transistor.
>> Making it non-standard will mean that we'll have lots of
>> "vendor,offset" properties, one each for each vendor selling
>> temperature sensor chips with external channels. This gets
>> more complicated here because the lm90 driver does support chips
>> from several different vendors. Almost all of them support
>> this functionality. Which vendor do you select in this case ?
>>
>> I would suggest to use temperature-offset-milliseconds, though.
> 
> Yes, this sounds good. Just not seconds but millicelsius, I guess?
> 

Uuh, yes. Sorry, must be too early in the morning here.

>>
>>>> +          The value (millidegree Celsius) to be programmed in the channel specific offset register
>>>> +          (if supported by device).
>>>
>>> You described programming model which should not be put in the bindings.
>>> Please describe the hardware.
>>>
>>
>> It is a configuration value, which is hardware dependent because
>> it depends on the temperature diode or transistor connected to the chip.
> 
> Sure, so this could be reworded "Offset against some base value for each
> channel temperature", or something similar (you know better than me).
> Referring to registers and where exactly this should be programmed in
> the device is related to device programming model, not to bindings.
> 

Maybe something like "Temperature offset to be added to or
subtracted from remote temperature measurements".

Thanks,
Guenter

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Slawomir Stepien]

On maj 20, 2022 07:22, Guenter Roeck wrote:
> On 5/20/22 07:09, Krzysztof Kozlowski wrote:
> > On 20/05/2022 15:42, Guenter Roeck wrote:
> > > > 
> > > > > +          A descriptive name for this channel, like "ambient" or "psu".
> > > > > +
> > > > > +      offset:
> > > > > +        description: |
> > > > 
> > > > This does not look like standard property, so you need vendor and unit
> > > > suffix.
> > > > 
> > > 
> > > Temperature offset is a standard property for temperature sensor
> > 
> > The original description was strictly connected to registers, so that
> > one as not a standard. It seems it was just a wording...
> > 
> > > chips with external channels, implemented by a diode or transistor.
> > > Making it non-standard will mean that we'll have lots of
> > > "vendor,offset" properties, one each for each vendor selling
> > > temperature sensor chips with external channels. This gets
> > > more complicated here because the lm90 driver does support chips
> > > from several different vendors. Almost all of them support
> > > this functionality. Which vendor do you select in this case ?
> > > 
> > > I would suggest to use temperature-offset-milliseconds, though.
> > 
> > Yes, this sounds good. Just not seconds but millicelsius, I guess?
> > 
> 
> Uuh, yes. Sorry, must be too early in the morning here.

Hello

I see that: *-millicelsius is defined as uint32-array:
  "-millicelsius$":
    $ref: "types.yaml#/definitions/uint32-array"
    description: Degreee milli-Celsius

But it would be nice to have negative values as the prop value, for example <(-1000)>.

How should I approach that? Is change to this definition possible? If yes, how should it be
conducted? On github or via device-tree mailing list?

Or maybe there is a way to overwrite this (using $defs?) for this particular binding? I haven't
found any solution that will pass dt_binding_check.

> > > > > +          The value (millidegree Celsius) to be programmed in the channel specific offset register
> > > > > +          (if supported by device).
> > > > 
> > > > You described programming model which should not be put in the bindings.
> > > > Please describe the hardware.
> > > > 
> > > 
> > > It is a configuration value, which is hardware dependent because
> > > it depends on the temperature diode or transistor connected to the chip.
> > 
> > Sure, so this could be reworded "Offset against some base value for each
> > channel temperature", or something similar (you know better than me).
> > Referring to registers and where exactly this should be programmed in
> > the device is related to device programming model, not to bindings.
> > 
> 
> Maybe something like "Temperature offset to be added to or
> subtracted from remote temperature measurements".

-- 
Slawomir Stepien

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Slawomir Stepien]

On maj 24, 2022 13:53, Slawomir Stepien wrote:
> On maj 20, 2022 07:22, Guenter Roeck wrote:
> > On 5/20/22 07:09, Krzysztof Kozlowski wrote:
> > > On 20/05/2022 15:42, Guenter Roeck wrote:
> > > > > 
> > > > > > +          A descriptive name for this channel, like "ambient" or "psu".
> > > > > > +
> > > > > > +      offset:
> > > > > > +        description: |
> > > > > 
> > > > > This does not look like standard property, so you need vendor and unit
> > > > > suffix.
> > > > > 
> > > > 
> > > > Temperature offset is a standard property for temperature sensor
> > > 
> > > The original description was strictly connected to registers, so that
> > > one as not a standard. It seems it was just a wording...
> > > 
> > > > chips with external channels, implemented by a diode or transistor.
> > > > Making it non-standard will mean that we'll have lots of
> > > > "vendor,offset" properties, one each for each vendor selling
> > > > temperature sensor chips with external channels. This gets
> > > > more complicated here because the lm90 driver does support chips
> > > > from several different vendors. Almost all of them support
> > > > this functionality. Which vendor do you select in this case ?
> > > > 
> > > > I would suggest to use temperature-offset-milliseconds, though.
> > > 
> > > Yes, this sounds good. Just not seconds but millicelsius, I guess?
> > > 
> > 
> > Uuh, yes. Sorry, must be too early in the morning here.
> 
> Hello
> 
> I see that: *-millicelsius is defined as uint32-array:
>   "-millicelsius$":
>     $ref: "types.yaml#/definitions/uint32-array"
>     description: Degreee milli-Celsius
> 
> But it would be nice to have negative values as the prop value, for example <(-1000)>.
> 
> How should I approach that? Is change to this definition possible? If yes, how should it be
> conducted? On github or via device-tree mailing list?
> 
> Or maybe there is a way to overwrite this (using $defs?) for this particular binding? I haven't
> found any solution that will pass dt_binding_check.

Well ok, looks like this:

      temperature-offset-millicelsius:
        description: Temperature offset to be added to or subtracted from remote temperature measurements.
        items:
          items:
            type: integer
            minimum: -128000
            maximum: 127000

Will overwrite the definition...most likely just minimum: -128000 in 2nd items will be enough.

> > > > > > +          The value (millidegree Celsius) to be programmed in the channel specific offset register
> > > > > > +          (if supported by device).
> > > > > 
> > > > > You described programming model which should not be put in the bindings.
> > > > > Please describe the hardware.
> > > > > 
> > > > 
> > > > It is a configuration value, which is hardware dependent because
> > > > it depends on the temperature diode or transistor connected to the chip.
> > > 
> > > Sure, so this could be reworded "Offset against some base value for each
> > > channel temperature", or something similar (you know better than me).
> > > Referring to registers and where exactly this should be programmed in
> > > the device is related to device programming model, not to bindings.
> > > 
> > 
> > Maybe something like "Temperature offset to be added to or
> > subtracted from remote temperature measurements".
> 

-- 
Slawomir Stepien

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Slawomir Stepien]

On maj 24, 2022 14:17, Slawomir Stepien wrote:
> On maj 24, 2022 13:53, Slawomir Stepien wrote:
> > On maj 20, 2022 07:22, Guenter Roeck wrote:
> > > On 5/20/22 07:09, Krzysztof Kozlowski wrote:
> > > > On 20/05/2022 15:42, Guenter Roeck wrote:
> > > > > > 
> > > > > > > +          A descriptive name for this channel, like "ambient" or "psu".
> > > > > > > +
> > > > > > > +      offset:
> > > > > > > +        description: |
> > > > > > 
> > > > > > This does not look like standard property, so you need vendor and unit
> > > > > > suffix.
> > > > > > 
> > > > > 
> > > > > Temperature offset is a standard property for temperature sensor
> > > > 
> > > > The original description was strictly connected to registers, so that
> > > > one as not a standard. It seems it was just a wording...
> > > > 
> > > > > chips with external channels, implemented by a diode or transistor.
> > > > > Making it non-standard will mean that we'll have lots of
> > > > > "vendor,offset" properties, one each for each vendor selling
> > > > > temperature sensor chips with external channels. This gets
> > > > > more complicated here because the lm90 driver does support chips
> > > > > from several different vendors. Almost all of them support
> > > > > this functionality. Which vendor do you select in this case ?
> > > > > 
> > > > > I would suggest to use temperature-offset-milliseconds, though.
> > > > 
> > > > Yes, this sounds good. Just not seconds but millicelsius, I guess?
> > > > 
> > > 
> > > Uuh, yes. Sorry, must be too early in the morning here.
> > 
> > Hello
> > 
> > I see that: *-millicelsius is defined as uint32-array:
> >   "-millicelsius$":
> >     $ref: "types.yaml#/definitions/uint32-array"
> >     description: Degreee milli-Celsius
> > 
> > But it would be nice to have negative values as the prop value, for example <(-1000)>.
> > 
> > How should I approach that? Is change to this definition possible? If yes, how should it be
> > conducted? On github or via device-tree mailing list?
> > 
> > Or maybe there is a way to overwrite this (using $defs?) for this particular binding? I haven't
> > found any solution that will pass dt_binding_check.
> 
> Well ok, looks like this:
> 
>       temperature-offset-millicelsius:
>         description: Temperature offset to be added to or subtracted from remote temperature measurements.
>         items:
>           items:
>             type: integer
>             minimum: -128000
>             maximum: 127000

This isn't working...from what I've read we cannot just simply overwrite existing schemas.

Krzysztof, Guenter what I should do? Is there a way to match with uint32-array schema and with
schema that allows items in array to be below 0 (seems impossible to me)? I've tried a lot of
combinations today without any luck. Any helpful tips? Thanks!

> Will overwrite the definition...most likely just minimum: -128000 in 2nd items will be enough.
> 
> > > > > > > +          The value (millidegree Celsius) to be programmed in the channel specific offset register
> > > > > > > +          (if supported by device).
> > > > > > 
> > > > > > You described programming model which should not be put in the bindings.
> > > > > > Please describe the hardware.
> > > > > > 
> > > > > 
> > > > > It is a configuration value, which is hardware dependent because
> > > > > it depends on the temperature diode or transistor connected to the chip.
> > > > 
> > > > Sure, so this could be reworded "Offset against some base value for each
> > > > channel temperature", or something similar (you know better than me).
> > > > Referring to registers and where exactly this should be programmed in
> > > > the device is related to device programming model, not to bindings.
> > > > 
> > > 
> > > Maybe something like "Temperature offset to be added to or
> > > subtracted from remote temperature measurements".

-- 
Slawomir Stepien

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Krzysztof Kozlowski]

On 24/05/2022 19:27, Slawomir Stepien wrote:
>> Well ok, looks like this:
>>
>>       temperature-offset-millicelsius:
>>         description: Temperature offset to be added to or subtracted from remote temperature measurements.
>>         items:
>>           items:

I think this is not an array, so items are not needed.

>>             type: integer

types are instead:
$ref: /schemas/types.yaml#/definitions/int32
but I think it still does not work.

>>             minimum: -128000
>>             maximum: 127000
> 
> This isn't working...from what I've read we cannot just simply overwrite existing schemas.
> 
> Krzysztof, Guenter what I should do? Is there a way to match with uint32-array schema and with
> schema that allows items in array to be below 0 (seems impossible to me)? I've tried a lot of
> combinations today without any luck. Any helpful tips? Thanks!

However this still does not work. I changed in schema:

   # Temperature

   "-celsius$":

-    $ref: "types.yaml#/definitions/uint32-array"

+    $ref: "types.yaml#/definitions/int32-array"


but that does not solve the problem that property is stored as uint32
and parsed like uint32:

    4294967291 is greater than the maximum of 40


Maybe Rob has some idea. Till then, you can skip minimum.

Best regards,
Krzysztof

Re: [PATCH 3/8] dt-bindings: hwmon: Allow specifying channels for lm90

[Slawomir Stepien]

On maj 24, 2022 19:59, Krzysztof Kozlowski wrote:
> On 24/05/2022 19:27, Slawomir Stepien wrote:
> >> Well ok, looks like this:
> >>
> >>       temperature-offset-millicelsius:
> >>         description: Temperature offset to be added to or subtracted from remote temperature measurements.
> >>         items:
> >>           items:
> 
> I think this is not an array, so items are not needed.
> 
> >>             type: integer
> 
> types are instead:
> $ref: /schemas/types.yaml#/definitions/int32
> but I think it still does not work.
> 
> >>             minimum: -128000
> >>             maximum: 127000
> > 
> > This isn't working...from what I've read we cannot just simply overwrite existing schemas.
> > 
> > Krzysztof, Guenter what I should do? Is there a way to match with uint32-array schema and with
> > schema that allows items in array to be below 0 (seems impossible to me)? I've tried a lot of
> > combinations today without any luck. Any helpful tips? Thanks!
> 
> However this still does not work. I changed in schema:
> 
>    # Temperature
> 
>    "-celsius$":

I'm using -millicelsius.

> -    $ref: "types.yaml#/definitions/uint32-array"
> 
> +    $ref: "types.yaml#/definitions/int32-array"

If I drop the "-millicelsius" and set the ref to types.yaml#/definitions/int32, all seems fine (the
sign is parsed correctly and the max/min are enforced correctly too).

> but that does not solve the problem that property is stored as uint32
> and parsed like uint32:
> 
>     4294967291 is greater than the maximum of 40
> 
> 
> Maybe Rob has some idea. Till then, you can skip minimum.

I will skip for now. I will send the new series and we can continue the discussion there. Thank you!

-- 
Slawomir Stepien

[PATCH 4/8] hwmon: (lm90) add support for ADT7481

[Slawomir Stepien]

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

The ADT7481 is a 3-channel digital thermometer and under/over
temperature alarm, intended for use in PCs and thermal management
systems.

From driver point-of-view, it is quite similar to MAX6696 so a lot of
code for MAX6696 is reused.

Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
---
 Documentation/hwmon/lm90.rst |  12 +++-
 drivers/hwmon/Kconfig        |  15 +++--
 drivers/hwmon/lm90.c         | 114 ++++++++++++++++++++++++++---------
 3 files changed, 105 insertions(+), 36 deletions(-)

diff --git a/Documentation/hwmon/lm90.rst b/Documentation/hwmon/lm90.rst
index 05391fb4042d..2e67d602e275 100644
--- a/Documentation/hwmon/lm90.rst
+++ b/Documentation/hwmon/lm90.rst
@@ -73,6 +73,16 @@ Supported chips:
 
 	       https://www.onsemi.com/PowerSolutions/product.do?id=ADT7461A
 
+  * Analog Devices ADT7481
+
+    Prefix: 'adt7481'
+
+    Addresses scanned: I2C 0x4c and 0x4b
+
+    Datasheet: Publicly available at the ON Semiconductor website
+
+	       http://www.onsemi.com/PowerSolutions/product.do?id=ADT7481
+
   * ON Semiconductor NCT1008
 
     Prefix: 'nct1008'
@@ -319,7 +329,7 @@ ADM1032:
   * ALERT is triggered by open remote sensor.
   * SMBus PEC support for Write Byte and Receive Byte transactions.
 
-ADT7461, ADT7461A, NCT1008:
+ADT7461, ADT7461A, ADT7481, NCT1008:
   * Extended temperature range (breaks compatibility)
   * Lower resolution for remote temperature
 
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index f2b038fa3b84..9586b610b5d8 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1339,13 +1339,16 @@ config SENSORS_LM90
 	tristate "National Semiconductor LM90 and compatibles"
 	depends on I2C
 	help
-	  If you say yes here you get support for National Semiconductor LM90,
-	  LM86, LM89 and LM99, Analog Devices ADM1032, ADT7461, and ADT7461A,
+	  If you say yes here you will get support for sensor chips:
+	  National Semiconductor LM90, LM86, LM89 and LM99,
+	  Analog Devices ADM1032, ADT7461, ADT7461A and ADT7481,
 	  Maxim MAX6646, MAX6647, MAX6648, MAX6649, MAX6654, MAX6657, MAX6658,
-	  MAX6659, MAX6680, MAX6681, MAX6692, MAX6695, MAX6696,
-	  ON Semiconductor NCT1008, Winbond/Nuvoton W83L771W/G/AWG/ASG,
-	  Philips SA56004, GMT G781, Texas Instruments TMP451 and TMP461
-	  sensor chips.
+	  MAX6659, MAX6680, MAX6681, MAX6692, MAX6695 and MAX6696,
+	  ON Semiconductor NCT1008,
+	  Winbond/Nuvoton W83L771W/G/AWG/ASG,
+	  Philips SA56004,
+	  GMT G781,
+	  Texas Instruments TMP451 and TMP461.
 
 	  This driver can also be built as a module. If so, the module
 	  will be called lm90.
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 1c9493c70813..00fd5734f217 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -59,10 +59,10 @@
  * chips, but support three temperature sensors instead of two. MAX6695
  * and MAX6696 only differ in the pinout so they can be treated identically.
  *
- * This driver also supports ADT7461 and ADT7461A from Analog Devices as well as
- * NCT1008 from ON Semiconductor. The chips are supported in both compatibility
- * and extended mode. They are mostly compatible with LM90 except for a data
- * format difference for the temperature value registers.
+ * This driver also supports ADT7461, ADT7461A and ADT7481 from Analog Devices
+ * as well as NCT1008 from ON Semiconductor. The chips are supported in both
+ * compatibility and extended mode. They are mostly compatible with LM90 except
+ * for a data format difference for the temperature value registers.
  *
  * This driver also supports the SA56004 from Philips. This device is
  * pin-compatible with the LM86, the ED/EDP parts are also address-compatible.
@@ -106,14 +106,16 @@
  * MAX6654, MAX6680, and MAX6681 can have address 0x18, 0x19, 0x1a, 0x29,
  * 0x2a, 0x2b, 0x4c, 0x4d or 0x4e.
  * SA56004 can have address 0x48 through 0x4F.
+ * ADT7481 can have address 0x4c or 0x4b (ADT7481-1).
  */
 
 static const unsigned short normal_i2c[] = {
 	0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x48, 0x49, 0x4a, 0x4b, 0x4c,
 	0x4d, 0x4e, 0x4f, I2C_CLIENT_END };
 
-enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
-	max6646, w83l771, max6696, sa56004, g781, tmp451, tmp461, max6654 };
+enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, adt7481,
+	max6680, max6646, w83l771, max6696, sa56004, g781, tmp451, tmp461,
+	max6654 };
 
 /*
  * The LM90 registers
@@ -141,6 +143,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
 #define LM90_REG_W_REMOTE_OFFSH		0x11
 #define LM90_REG_R_REMOTE_OFFSL		0x12
 #define LM90_REG_W_REMOTE_OFFSL		0x12
+#define LM90_REG_R_REMOTE2_OFFSH	0x34
+#define LM90_REG_W_REMOTE2_OFFSH	0x34
+#define LM90_REG_R_REMOTE2_OFFSL	0x35
+#define LM90_REG_W_REMOTE2_OFFSL	0x35
 #define LM90_REG_R_REMOTE_HIGHH		0x07
 #define LM90_REG_W_REMOTE_HIGHH		0x0D
 #define LM90_REG_R_REMOTE_HIGHL		0x13
@@ -154,6 +160,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
 #define LM90_REG_R_TCRIT_HYST		0x21
 #define LM90_REG_W_TCRIT_HYST		0x21
 
+/* ADT7481 registers */
+
+#define ADT7481_REG_R_STATUS2           0x23
+
 /* MAX6646/6647/6649/6654/6657/6658/6659/6695/6696 registers */
 
 #define MAX6657_REG_R_LOCAL_TEMPL	0x11
@@ -202,6 +212,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, max6659, adt7461, max6680,
 #define LM90_STATUS_LHIGH	(1 << 6) /* local high temp limit tripped */
 #define LM90_STATUS_BUSY	(1 << 7) /* conversion is ongoing */
 
+/* Note: bits 1, 2, 3 and 4 are used by ADT7481 too */
 #define MAX6696_STATUS2_R2THRM	(1 << 1) /* remote2 THERM limit tripped */
 #define MAX6696_STATUS2_R2OPEN	(1 << 2) /* remote2 is an open circuit */
 #define MAX6696_STATUS2_R2LOW	(1 << 3) /* remote2 low temp limit tripped */
@@ -218,6 +229,7 @@ static const struct i2c_device_id lm90_id[] = {
 	{ "adm1032", adm1032 },
 	{ "adt7461", adt7461 },
 	{ "adt7461a", adt7461 },
+	{ "adt7481", adt7481 },
 	{ "g781", g781 },
 	{ "lm90", lm90 },
 	{ "lm86", lm86 },
@@ -256,6 +268,10 @@ static const struct of_device_id __maybe_unused lm90_of_match[] = {
 		.compatible = "adi,adt7461a",
 		.data = (void *)adt7461
 	},
+	{
+		.compatible = "adi,adt7481",
+		.data = (void *)adt7481
+	},
 	{
 		.compatible = "gmt,g781",
 		.data = (void *)g781
@@ -353,6 +369,7 @@ struct lm90_params {
 				/* Upper 8 bits for max6695/96 */
 	u8 max_convrate;	/* Maximum conversion rate register value */
 	u8 reg_local_ext;	/* Extended local temp register (optional) */
+	u8 reg_status2;		/* Second status register (optional) */
 };
 
 static const struct lm90_params lm90_params[] = {
@@ -369,6 +386,14 @@ static const struct lm90_params lm90_params[] = {
 		.alert_alarms = 0x7c,
 		.max_convrate = 10,
 	},
+	[adt7481] = {
+		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
+		  | LM90_HAVE_BROKEN_ALERT | LM90_HAVE_EXTENDED_TEMP
+		  | LM90_HAVE_CRIT | LM90_HAVE_TEMP3,
+		.alert_alarms = 0x1c7c,
+		.max_convrate = 11,
+		.reg_status2 = ADT7481_REG_R_STATUS2,
+	},
 	[g781] = {
 		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT
 		  | LM90_HAVE_BROKEN_ALERT | LM90_HAVE_CRIT,
@@ -429,6 +454,7 @@ static const struct lm90_params lm90_params[] = {
 		.alert_alarms = 0x1c7c,
 		.max_convrate = 6,
 		.reg_local_ext = MAX6657_REG_R_LOCAL_TEMPL,
+		.reg_status2 = MAX6696_REG_R_STATUS2,
 	},
 	[w83l771] = {
 		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT | LM90_HAVE_CRIT,
@@ -480,10 +506,11 @@ enum lm90_temp11_reg_index {
 	REMOTE_LOW,
 	REMOTE_HIGH,
 	REMOTE_OFFSET,	/* except max6646, max6657/58/59, and max6695/96 */
+	REMOTE2_OFFSET, /* for dev that can set offset for the 2nd channel */
 	LOCAL_TEMP,
-	REMOTE2_TEMP,	/* max6695/96 only */
-	REMOTE2_LOW,	/* max6695/96 only */
-	REMOTE2_HIGH,	/* max6695/96 only */
+	REMOTE2_TEMP,	/* max6695/96 and adt7481 only */
+	REMOTE2_LOW,	/* max6695/96 and adt7481 only */
+	REMOTE2_HIGH,	/* max6695/96 and adt7481 only */
 	TEMP11_REG_NUM
 };
 
@@ -512,7 +539,8 @@ struct lm90_data {
 	u16 alert_alarms;	/* Which alarm bits trigger ALERT# */
 				/* Upper 8 bits for max6695/96 */
 	u8 max_convrate;	/* Maximum conversion rate */
-	u8 reg_local_ext;	/* local extension register offset */
+	u8 reg_local_ext;	/* Local extension register offset */
+	u8 reg_status2;		/* Second status register */
 
 	/* registers values */
 	s8 temp8[TEMP8_REG_NUM];
@@ -617,7 +645,7 @@ static int lm90_select_remote_channel(struct lm90_data *data, int channel)
 {
 	int err = 0;
 
-	if (data->kind == max6696) {
+	if (data->kind == max6696 || data->kind == adt7481) {
 		u8 config = data->config & ~0x08;
 
 		if (channel)
@@ -726,6 +754,14 @@ static int lm90_update_limits(struct device *dev)
 		if (val < 0)
 			return val;
 		data->temp11[REMOTE_OFFSET] = val;
+
+		if (data->flags & LM90_HAVE_TEMP3) {
+			val = lm90_read16(client, LM90_REG_R_REMOTE2_OFFSH,
+					  LM90_REG_R_REMOTE2_OFFSL);
+			if (val < 0)
+				return val;
+			data->temp11[REMOTE2_OFFSET] = val;
+		}
 	}
 
 	if (data->flags & LM90_HAVE_EMERGENCY) {
@@ -740,7 +776,7 @@ static int lm90_update_limits(struct device *dev)
 		data->temp8[REMOTE_EMERG] = val;
 	}
 
-	if (data->kind == max6696) {
+	if (data->kind == max6696 || data->kind == adt7481) {
 		val = lm90_select_remote_channel(data, 1);
 		if (val < 0)
 			return val;
@@ -750,10 +786,12 @@ static int lm90_update_limits(struct device *dev)
 			return val;
 		data->temp8[REMOTE2_CRIT] = val;
 
-		val = lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG);
-		if (val < 0)
-			return val;
-		data->temp8[REMOTE2_EMERG] = val;
+		if (data->kind == max6696) {
+			val = lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG);
+			if (val < 0)
+				return val;
+			data->temp8[REMOTE2_EMERG] = val;
+		}
 
 		val = lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH);
 		if (val < 0)
@@ -824,7 +862,7 @@ static int lm90_update_device(struct device *dev)
 			return val;
 		data->alarms = val & ~LM90_STATUS_BUSY;
 
-		if (data->kind == max6696) {
+		if (data->kind == max6696 || data->kind == adt7481) {
 			val = lm90_select_remote_channel(data, 1);
 			if (val < 0)
 				return val;
@@ -838,8 +876,10 @@ static int lm90_update_device(struct device *dev)
 			data->temp11[REMOTE2_TEMP] = val;
 
 			lm90_select_remote_channel(data, 0);
+		}
 
-			val = lm90_read_reg(client, MAX6696_REG_R_STATUS2);
+		if (data->reg_status2) {
+			val = lm90_read_reg(client, data->reg_status2);
 			if (val < 0)
 				return val;
 			data->alarms |= val << 8;
@@ -1051,6 +1091,7 @@ static int lm90_set_temp11(struct lm90_data *data, int index, long val)
 	[REMOTE_LOW] = { LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL },
 	[REMOTE_HIGH] = { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL },
 	[REMOTE_OFFSET] = { LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL },
+	[REMOTE2_OFFSET] = { LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL },
 	[REMOTE2_LOW] = { LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL },
 	[REMOTE2_HIGH] = { LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL }
 	};
@@ -1074,7 +1115,7 @@ static int lm90_set_temp11(struct lm90_data *data, int index, long val)
 	else
 		data->temp11[index] = temp_to_s8(val) << 8;
 
-	lm90_select_remote_channel(data, index >= 3);
+	lm90_select_remote_channel(data, index >= REMOTE2_OFFSET);
 	err = i2c_smbus_write_byte_data(client, regp->high,
 				  data->temp11[index] >> 8);
 	if (err < 0)
@@ -1271,7 +1312,10 @@ static int lm90_temp_read(struct device *dev, u32 attr, int channel, long *val)
 		*val = lm90_get_temphyst(data, lm90_temp_emerg_index[channel]);
 		break;
 	case hwmon_temp_offset:
-		*val = lm90_get_temp11(data, REMOTE_OFFSET);
+		if (channel == 1)
+			*val = lm90_get_temp11(data, REMOTE_OFFSET);
+		else
+			*val = lm90_get_temp11(data, REMOTE2_OFFSET);
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -1321,7 +1365,10 @@ static int lm90_temp_write(struct device *dev, u32 attr, int channel, long val)
 		err = lm90_set_temp8(data, lm90_temp_emerg_index[channel], val);
 		break;
 	case hwmon_temp_offset:
-		err = lm90_set_temp11(data, REMOTE_OFFSET, val);
+		if (channel == 1)
+			err = lm90_set_temp11(data, REMOTE_OFFSET, val);
+		else
+			err = lm90_set_temp11(data, REMOTE2_OFFSET, val);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -1513,7 +1560,7 @@ static int lm90_detect(struct i2c_client *client,
 			}
 		}
 	} else
-	if ((address == 0x4C || address == 0x4D)
+	if ((address == 0x4C || address == 0x4D || address == 0x4B)
 	 && man_id == 0x41) { /* Analog Devices */
 		if ((chip_id & 0xF0) == 0x40 /* ADM1032 */
 		 && (config1 & 0x3F) == 0x00
@@ -1536,6 +1583,11 @@ static int lm90_detect(struct i2c_client *client,
 		 && (config1 & 0x1B) == 0x00
 		 && convrate <= 0x0A) {
 			name = "adt7461a";
+		} else
+		if (chip_id == 0x62 /* ADT7481 (undocumented in datasheet) */
+		 && (config1 & 0x1B) == 0x00
+		 && convrate <= 0x0B) {
+			name = "adt7481";
 		}
 	} else
 	if (man_id == 0x4D) { /* Maxim */
@@ -1749,9 +1801,9 @@ static int lm90_init_client(struct i2c_client *client, struct lm90_data *data)
 		config |= 0x20;
 
 	/*
-	 * Select external channel 0 for max6695/96
+	 * Select external channel 0 for max6695/96 or adt7481
 	 */
-	if (data->kind == max6696)
+	if (data->kind == max6696 || data->kind == adt7481)
 		config &= ~0x08;
 
 	/*
@@ -1776,11 +1828,11 @@ static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
 	if (st < 0)
 		return false;
 
-	if (data->kind == max6696) {
-		st2 = lm90_read_reg(client, MAX6696_REG_R_STATUS2);
-		if (st2 < 0)
-			return false;
-	}
+	if (data->reg_status2)
+		st2 = lm90_read_reg(client, data->reg_status2);
+
+	if (st2 < 0)
+		return false;
 
 	*status = st | (st2 << 8);
 
@@ -1951,9 +2003,13 @@ static int lm90_probe(struct i2c_client *client)
 			HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM |
 			HWMON_T_CRIT_ALARM | HWMON_T_EMERGENCY_ALARM |
 			HWMON_T_FAULT;
+
+		if (data->flags & LM90_HAVE_OFFSET)
+			data->channel_config[2] |= HWMON_T_OFFSET;
 	}
 
 	data->reg_local_ext = lm90_params[data->kind].reg_local_ext;
+	data->reg_status2 = lm90_params[data->kind].reg_status2;
 
 	/* Set maximum conversion rate */
 	data->max_convrate = lm90_params[data->kind].max_convrate;
-- 
2.36.1

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

[Slawomir Stepien]

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

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

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

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 00fd5734f217..f642c6fd1641 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -93,6 +93,9 @@
 #include <linux/interrupt.h>
 #include <linux/regulator/consumer.h>
 
+/* The maximum number of channels currently supported */
+#define MAX_CHANNELS 3
+
 /*
  * Addresses to scan
  * Address is fully defined internally and cannot be changed except for
@@ -521,9 +524,9 @@ enum lm90_temp11_reg_index {
 struct lm90_data {
 	struct i2c_client *client;
 	struct device *hwmon_dev;
-	u32 channel_config[4];
+	u32 channel_config[MAX_CHANNELS + 1];
 	struct hwmon_channel_info temp_info;
-	const struct hwmon_channel_info *info[3];
+	const struct hwmon_channel_info *info[MAX_CHANNELS];
 	struct hwmon_chip_info chip;
 	struct mutex update_lock;
 	bool valid;		/* true if register values are valid */
@@ -1223,32 +1226,32 @@ static int lm90_set_temphyst(struct lm90_data *data, long val)
 	return err;
 }
 
-static const u8 lm90_temp_index[3] = {
+static const u8 lm90_temp_index[MAX_CHANNELS] = {
 	LOCAL_TEMP, REMOTE_TEMP, REMOTE2_TEMP
 };
 
-static const u8 lm90_temp_min_index[3] = {
+static const u8 lm90_temp_min_index[MAX_CHANNELS] = {
 	LOCAL_LOW, REMOTE_LOW, REMOTE2_LOW
 };
 
-static const u8 lm90_temp_max_index[3] = {
+static const u8 lm90_temp_max_index[MAX_CHANNELS] = {
 	LOCAL_HIGH, REMOTE_HIGH, REMOTE2_HIGH
 };
 
-static const u8 lm90_temp_crit_index[3] = {
+static const u8 lm90_temp_crit_index[MAX_CHANNELS] = {
 	LOCAL_CRIT, REMOTE_CRIT, REMOTE2_CRIT
 };
 
-static const u8 lm90_temp_emerg_index[3] = {
+static const u8 lm90_temp_emerg_index[MAX_CHANNELS] = {
 	LOCAL_EMERG, REMOTE_EMERG, REMOTE2_EMERG
 };
 
-static const u8 lm90_min_alarm_bits[3] = { 5, 3, 11 };
-static const u8 lm90_max_alarm_bits[3] = { 6, 4, 12 };
-static const u8 lm90_crit_alarm_bits[3] = { 0, 1, 9 };
-static const u8 lm90_crit_alarm_bits_swapped[3] = { 1, 0, 9 };
-static const u8 lm90_emergency_alarm_bits[3] = { 15, 13, 14 };
-static const u8 lm90_fault_bits[3] = { 0, 2, 10 };
+static const u8 lm90_min_alarm_bits[MAX_CHANNELS] = { 5, 3, 11 };
+static const u8 lm90_max_alarm_bits[MAX_CHANNELS] = { 6, 4, 12 };
+static const u8 lm90_crit_alarm_bits[MAX_CHANNELS] = { 0, 1, 9 };
+static const u8 lm90_crit_alarm_bits_swapped[MAX_CHANNELS] = { 1, 0, 9 };
+static const u8 lm90_emergency_alarm_bits[MAX_CHANNELS] = { 15, 13, 14 };
+static const u8 lm90_fault_bits[MAX_CHANNELS] = { 0, 2, 10 };
 
 static int lm90_temp_read(struct device *dev, u32 attr, int channel, long *val)
 {
-- 
2.36.1

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

[Guenter Roeck]

On Fri, May 20, 2022 at 11:32:41AM +0200, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Use this define in all the places where literal '3' was used in this
> context.

The literal '3' does not always reflect the number of channels.

> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> ---
>  drivers/hwmon/lm90.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 00fd5734f217..f642c6fd1641 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -93,6 +93,9 @@
>  #include <linux/interrupt.h>
>  #include <linux/regulator/consumer.h>
>  
> +/* The maximum number of channels currently supported */
> +#define MAX_CHANNELS 3

#define<space>NAME<tab>value

> +
>  /*
>   * Addresses to scan
>   * Address is fully defined internally and cannot be changed except for
> @@ -521,9 +524,9 @@ enum lm90_temp11_reg_index {
>  struct lm90_data {
>  	struct i2c_client *client;
>  	struct device *hwmon_dev;
> -	u32 channel_config[4];
> +	u32 channel_config[MAX_CHANNELS + 1];
>  	struct hwmon_channel_info temp_info;
> -	const struct hwmon_channel_info *info[3];
> +	const struct hwmon_channel_info *info[MAX_CHANNELS];

This is wrong.

Guenter

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

[Slawomir Stepien]

On maj 20, 2022 19:36, Guenter Roeck wrote:
> On Fri, May 20, 2022 at 11:32:41AM +0200, Slawomir Stepien wrote:
> > From: Slawomir Stepien <slawomir.stepien@nokia.com>
> > 
> > Use this define in all the places where literal '3' was used in this
> > context.
> 
> The literal '3' does not always reflect the number of channels.

You caught me.
But besides this problem with struct hwmon_chip_info the '3' was used in context of
max number of channels.

> > 
> > Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> > ---
> >  drivers/hwmon/lm90.c | 29 ++++++++++++++++-------------
> >  1 file changed, 16 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 00fd5734f217..f642c6fd1641 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -93,6 +93,9 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/regulator/consumer.h>
> >  
> > +/* The maximum number of channels currently supported */
> > +#define MAX_CHANNELS 3
> 
> #define<space>NAME<tab>value

OK

> > +
> >  /*
> >   * Addresses to scan
> >   * Address is fully defined internally and cannot be changed except for
> > @@ -521,9 +524,9 @@ enum lm90_temp11_reg_index {
> >  struct lm90_data {
> >  	struct i2c_client *client;
> >  	struct device *hwmon_dev;
> > -	u32 channel_config[4];
> > +	u32 channel_config[MAX_CHANNELS + 1];
> >  	struct hwmon_channel_info temp_info;
> > -	const struct hwmon_channel_info *info[3];
> > +	const struct hwmon_channel_info *info[MAX_CHANNELS];
> 
> This is wrong.

Yes, sorry for that. I have not checked struct hwmon_chip_info description.

-- 
Slawomir Stepien

[PATCH 6/8] hwmon: (lm90) enable the extended temperature range

[Slawomir Stepien]

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

Enable the extended temperature range if it was requested from
device-tree node.

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

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index f642c6fd1641..67d48707a8d6 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -1914,6 +1914,7 @@ static const struct hwmon_ops lm90_ops = {
 static int lm90_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
+	const struct device_node *np = dev->of_node;
 	struct i2c_adapter *adapter = client->adapter;
 	struct hwmon_channel_info *info;
 	struct regulator *regulator;
@@ -2017,6 +2018,17 @@ static int lm90_probe(struct i2c_client *client)
 	/* Set maximum conversion rate */
 	data->max_convrate = lm90_params[data->kind].max_convrate;
 
+	/* Parse device-tree information */
+	if (np) {
+		if (data->flags & LM90_HAVE_EXTENDED_TEMP) {
+			if (of_property_read_bool(np, "extended-range-enable")) {
+				err = lm90_update_confreg(data, data->config | 0x04);
+				if (err)
+					return err;
+			}
+		}
+	}
+
 	/* Initialize the LM90 chip */
 	err = lm90_init_client(client, data);
 	if (err < 0) {
-- 
2.36.1

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

[Slawomir Stepien]

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

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

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

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 67d48707a8d6..e81cc21e525f 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -525,6 +525,7 @@ struct lm90_data {
 	struct i2c_client *client;
 	struct device *hwmon_dev;
 	u32 channel_config[MAX_CHANNELS + 1];
+	const char *channel_label[MAX_CHANNELS];
 	struct hwmon_channel_info temp_info;
 	const struct hwmon_channel_info *info[MAX_CHANNELS];
 	struct hwmon_chip_info chip;
@@ -1393,6 +1394,7 @@ static umode_t lm90_temp_is_visible(const void *data, u32 attr, int channel)
 	case hwmon_temp_emergency_alarm:
 	case hwmon_temp_emergency_hyst:
 	case hwmon_temp_fault:
+	case hwmon_temp_label:
 		return 0444;
 	case hwmon_temp_min:
 	case hwmon_temp_max:
@@ -1486,6 +1488,16 @@ static int lm90_read(struct device *dev, enum hwmon_sensor_types type,
 	}
 }
 
+static int lm90_read_string(struct device *dev, enum hwmon_sensor_types type,
+			    u32 attr, int channel, const char **str)
+{
+	struct lm90_data *data = dev_get_drvdata(dev);
+
+	*str = data->channel_label[channel];
+
+	return 0;
+}
+
 static int lm90_write(struct device *dev, enum hwmon_sensor_types type,
 		      u32 attr, int channel, long val)
 {
@@ -1904,10 +1916,64 @@ static void lm90_regulator_disable(void *regulator)
 	regulator_disable(regulator);
 }
 
+static int lm90_probe_channel_from_dt(struct i2c_client *client,
+				      struct device_node *child,
+				      struct lm90_data *data)
+{
+	u32 id;
+	int err;
+	struct device *dev = &client->dev;
+
+	err = of_property_read_u32(child, "reg", &id);
+	if (err) {
+		dev_err(dev, "missing reg property of %pOFn\n", child);
+		return err;
+	}
+
+	if (id >= MAX_CHANNELS) {
+		dev_err(dev, "invalid reg %d in %pOFn\n", id, child);
+		return -EINVAL;
+	}
+
+	err = of_property_read_string(child, "label", &data->channel_label[id]);
+	if (err == -ENODATA || err == -EILSEQ) {
+		dev_err(dev, "invalid label in %pOFn\n", child);
+		return err;
+	}
+
+	if (data->channel_label[id])
+		data->channel_config[id] |= HWMON_T_LABEL;
+
+	return 0;
+}
+
+static int lm90_parse_dt_channel_info(struct i2c_client *client,
+				      struct lm90_data *data)
+{
+	int err;
+	struct device_node *child;
+	struct device *dev = &client->dev;
+	const struct device_node *np = dev->of_node;
+
+	for_each_child_of_node(np, child) {
+		if (strcmp(child->name, "channel"))
+			continue;
+
+		err = lm90_probe_channel_from_dt(client, child, data);
+		if (err) {
+			of_node_put(child);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 
 static const struct hwmon_ops lm90_ops = {
 	.is_visible = lm90_is_visible,
 	.read = lm90_read,
+	.read_string = lm90_read_string,
 	.write = lm90_write,
 };
 
@@ -2027,6 +2093,10 @@ static int lm90_probe(struct i2c_client *client)
 					return err;
 			}
 		}
+
+		err = lm90_parse_dt_channel_info(client, data);
+		if (err)
+			return err;
 	}
 
 	/* Initialize the LM90 chip */
-- 
2.36.1

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

[Krzysztof Kozlowski]

On 20/05/2022 11:32, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Try to read the channel's label from device-tree. Having label in
> device-tree node is not mandatory.
> 
> Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> ---
>  drivers/hwmon/lm90.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 67d48707a8d6..e81cc21e525f 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -525,6 +525,7 @@ struct lm90_data {
>  	struct i2c_client *client;
>  	struct device *hwmon_dev;
>  	u32 channel_config[MAX_CHANNELS + 1];
> +	const char *channel_label[MAX_CHANNELS];
>  	struct hwmon_channel_info temp_info;
>  	const struct hwmon_channel_info *info[MAX_CHANNELS];
>  	struct hwmon_chip_info chip;
> @@ -1393,6 +1394,7 @@ static umode_t lm90_temp_is_visible(const void *data, u32 attr, int channel)
>  	case hwmon_temp_emergency_alarm:
>  	case hwmon_temp_emergency_hyst:
>  	case hwmon_temp_fault:
> +	case hwmon_temp_label:
>  		return 0444;
>  	case hwmon_temp_min:
>  	case hwmon_temp_max:
> @@ -1486,6 +1488,16 @@ static int lm90_read(struct device *dev, enum hwmon_sensor_types type,
>  	}
>  }
>  
> +static int lm90_read_string(struct device *dev, enum hwmon_sensor_types type,
> +			    u32 attr, int channel, const char **str)
> +{
> +	struct lm90_data *data = dev_get_drvdata(dev);
> +
> +	*str = data->channel_label[channel];
> +
> +	return 0;
> +}
> +
>  static int lm90_write(struct device *dev, enum hwmon_sensor_types type,
>  		      u32 attr, int channel, long val)
>  {
> @@ -1904,10 +1916,64 @@ static void lm90_regulator_disable(void *regulator)
>  	regulator_disable(regulator);
>  }
>  
> +static int lm90_probe_channel_from_dt(struct i2c_client *client,
> +				      struct device_node *child,
> +				      struct lm90_data *data)
> +{
> +	u32 id;
> +	int err;
> +	struct device *dev = &client->dev;
> +
> +	err = of_property_read_u32(child, "reg", &id);
> +	if (err) {
> +		dev_err(dev, "missing reg property of %pOFn\n", child);
> +		return err;
> +	}
> +
> +	if (id >= MAX_CHANNELS) {
> +		dev_err(dev, "invalid reg %d in %pOFn\n", id, child);
> +		return -EINVAL;
> +	}
> +
> +	err = of_property_read_string(child, "label", &data->channel_label[id]);
> +	if (err == -ENODATA || err == -EILSEQ) {
> +		dev_err(dev, "invalid label in %pOFn\n", child);

You did not make it a required property, so why failing?

> +		return err;
> +	}
> +
> +	if (data->channel_label[id])
> +		data->channel_config[id] |= HWMON_T_LABEL;
> +
> +	return 0;
> +}
> +
> +static int lm90_parse_dt_channel_info(struct i2c_client *client,
> +				      struct lm90_data *data)
> +{
> +	int err;
> +	struct device_node *child;
> +	struct device *dev = &client->dev;
> +	const struct device_node *np = dev->of_node;
> +
> +	for_each_child_of_node(np, child) {
> +		if (strcmp(child->name, "channel"))
> +			continue;
> +
> +		err = lm90_probe_channel_from_dt(client, child, data);
> +		if (err) {
> +			of_node_put(child);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  

No need for double blank lines.

>  static const struct hwmon_ops lm90_ops = {
>  	.is_visible = lm90_is_visible,
>  	.read = lm90_read,
> +	.read_string = lm90_read_string,
>  	.write = lm90_write,
>  };
>  
> @@ -2027,6 +2093,10 @@ static int lm90_probe(struct i2c_client *client)
>  					return err;
>  			}
>  		}
> +
> +		err = lm90_parse_dt_channel_info(client, data);
> +		if (err)
> +			return err;
>  	}
>  
>  	/* Initialize the LM90 chip */


Best regards,
Krzysztof

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

[Slawomir Stepien]

On maj 20, 2022 12:15, Krzysztof Kozlowski wrote:
> On 20/05/2022 11:32, Slawomir Stepien wrote:
> > From: Slawomir Stepien <slawomir.stepien@nokia.com>
> > 
> > Try to read the channel's label from device-tree. Having label in
> > device-tree node is not mandatory.
> > 
> > Signed-off-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> > ---
> >  drivers/hwmon/lm90.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 67d48707a8d6..e81cc21e525f 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -525,6 +525,7 @@ struct lm90_data {
> >  	struct i2c_client *client;
> >  	struct device *hwmon_dev;
> >  	u32 channel_config[MAX_CHANNELS + 1];
> > +	const char *channel_label[MAX_CHANNELS];
> >  	struct hwmon_channel_info temp_info;
> >  	const struct hwmon_channel_info *info[MAX_CHANNELS];
> >  	struct hwmon_chip_info chip;
> > @@ -1393,6 +1394,7 @@ static umode_t lm90_temp_is_visible(const void *data, u32 attr, int channel)
> >  	case hwmon_temp_emergency_alarm:
> >  	case hwmon_temp_emergency_hyst:
> >  	case hwmon_temp_fault:
> > +	case hwmon_temp_label:
> >  		return 0444;
> >  	case hwmon_temp_min:
> >  	case hwmon_temp_max:
> > @@ -1486,6 +1488,16 @@ static int lm90_read(struct device *dev, enum hwmon_sensor_types type,
> >  	}
> >  }
> >  
> > +static int lm90_read_string(struct device *dev, enum hwmon_sensor_types type,
> > +			    u32 attr, int channel, const char **str)
> > +{
> > +	struct lm90_data *data = dev_get_drvdata(dev);
> > +
> > +	*str = data->channel_label[channel];
> > +
> > +	return 0;
> > +}
> > +
> >  static int lm90_write(struct device *dev, enum hwmon_sensor_types type,
> >  		      u32 attr, int channel, long val)
> >  {
> > @@ -1904,10 +1916,64 @@ static void lm90_regulator_disable(void *regulator)
> >  	regulator_disable(regulator);
> >  }
> >  
> > +static int lm90_probe_channel_from_dt(struct i2c_client *client,
> > +				      struct device_node *child,
> > +				      struct lm90_data *data)
> > +{
> > +	u32 id;
> > +	int err;
> > +	struct device *dev = &client->dev;
> > +
> > +	err = of_property_read_u32(child, "reg", &id);
> > +	if (err) {
> > +		dev_err(dev, "missing reg property of %pOFn\n", child);
> > +		return err;
> > +	}
> > +
> > +	if (id >= MAX_CHANNELS) {
> > +		dev_err(dev, "invalid reg %d in %pOFn\n", id, child);
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = of_property_read_string(child, "label", &data->channel_label[id]);
> > +	if (err == -ENODATA || err == -EILSEQ) {
> > +		dev_err(dev, "invalid label in %pOFn\n", child);
> 
> You did not make it a required property, so why failing?

But if you have it in device-tree, then at least inform the user that there is some problem with it.

> > +		return err;
> > +	}
> > +
> > +	if (data->channel_label[id])
> > +		data->channel_config[id] |= HWMON_T_LABEL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int lm90_parse_dt_channel_info(struct i2c_client *client,
> > +				      struct lm90_data *data)
> > +{
> > +	int err;
> > +	struct device_node *child;
> > +	struct device *dev = &client->dev;
> > +	const struct device_node *np = dev->of_node;
> > +
> > +	for_each_child_of_node(np, child) {
> > +		if (strcmp(child->name, "channel"))
> > +			continue;
> > +
> > +		err = lm90_probe_channel_from_dt(client, child, data);
> > +		if (err) {
> > +			of_node_put(child);
> > +			return err;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  
> 
> No need for double blank lines.

Will fix in v2.

> >  static const struct hwmon_ops lm90_ops = {
> >  	.is_visible = lm90_is_visible,
> >  	.read = lm90_read,
> > +	.read_string = lm90_read_string,
> >  	.write = lm90_write,
> >  };
> >  
> > @@ -2027,6 +2093,10 @@ static int lm90_probe(struct i2c_client *client)
> >  					return err;
> >  			}
> >  		}
> > +
> > +		err = lm90_parse_dt_channel_info(client, data);
> > +		if (err)
> > +			return err;
> >  	}
> >  
> >  	/* Initialize the LM90 chip */
> 
> 
> Best regards,
> Krzysztof

-- 
Slawomir Stepien

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

[Krzysztof Kozlowski]

On 20/05/2022 13:01, Slawomir Stepien wrote:

>>> +static int lm90_probe_channel_from_dt(struct i2c_client *client,
>>> +				      struct device_node *child,
>>> +				      struct lm90_data *data)
>>> +{
>>> +	u32 id;
>>> +	int err;
>>> +	struct device *dev = &client->dev;
>>> +
>>> +	err = of_property_read_u32(child, "reg", &id);
>>> +	if (err) {
>>> +		dev_err(dev, "missing reg property of %pOFn\n", child);
>>> +		return err;
>>> +	}
>>> +
>>> +	if (id >= MAX_CHANNELS) {
>>> +		dev_err(dev, "invalid reg %d in %pOFn\n", id, child);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	err = of_property_read_string(child, "label", &data->channel_label[id]);
>>> +	if (err == -ENODATA || err == -EILSEQ) {
>>> +		dev_err(dev, "invalid label in %pOFn\n", child);
>>
>> You did not make it a required property, so why failing?
> 
> But if you have it in device-tree, then at least inform the user that there is some problem with it.

Hm, true, the missing property would return EINVAL. Sounds good then.
x


Best regards,
Krzysztof

[PATCH 8/8] hwmon: (lm90) read the channel's offset from device-tree

[Slawomir Stepien]

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

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

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

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index e81cc21e525f..e4e7b4ddbfb6 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -1921,6 +1921,7 @@ static int lm90_probe_channel_from_dt(struct i2c_client *client,
 				      struct lm90_data *data)
 {
 	u32 id;
+	s32 val;
 	int err;
 	struct device *dev = &client->dev;
 
@@ -1944,6 +1945,25 @@ static int lm90_probe_channel_from_dt(struct i2c_client *client,
 	if (data->channel_label[id])
 		data->channel_config[id] |= HWMON_T_LABEL;
 
+	err = of_property_read_s32(child, "offset", &val);
+	if (!err) {
+		if (id == 0) {
+			dev_err(dev, "offset can't be set for internal channel\n");
+			return -EINVAL;
+		}
+
+		if (id == 1)
+			err = lm90_set_temp11(data, REMOTE_OFFSET, val);
+		else
+			err = lm90_set_temp11(data, REMOTE2_OFFSET, val);
+
+		if (err) {
+			dev_err(dev, "can't set offset %d for channel %d (%d)\n",
+				val, id, err);
+			return err;
+		}
+	}
+
 	return 0;
 }
 
@@ -2091,6 +2111,12 @@ static int lm90_probe(struct i2c_client *client)
 				err = lm90_update_confreg(data, data->config | 0x04);
 				if (err)
 					return err;
+
+				/*
+				 * In lm90_parse_dt_channel_info() we might set offset, so we need
+				 * to use correct format indication before that
+				 */
+				data->flags |= LM90_FLAG_ADT7461_EXT;
 			}
 		}
 
-- 
2.36.1

Re: [PATCH 0/8] Add support for ADT7481 in lm90

[Guenter Roeck]

On 5/20/22 02:32, Slawomir Stepien wrote:
> From: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> This patch series adds support for ADT7481 in lm90.c driver and extends the
> device-tree options for it.
> 
> The ADT7481 is quite similar to MAX6696 (already supported in lm90) so a lot of
> code is reused.
> 
> One major problem in fitting the ADT7481 in lm90.c is the chip detection.
> However it seems that the chip has been designed and produced with correct
> values at locations: 0xfe (manufactured id) and 0xff (chip id), but this is not
> documented in the datasheet.
> 

Before we go too far along this route, would you mind using my own patch series
as base to implement the devicetree changes ? I had prepared an extensive
patch series a while ago, adding not only support for ADT7481 but for
several other chips as well, I just never got around to sending it out.

Thanks,
Guenter

> $ i2cdump -y -f -r 254-255 1 0x4c
> No size specified (using byte-data access)
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> f0:                                           41 62                  Ab
> 
> The device-tree changes allow to: set the extended temperature range mode and
> set the label and offset for specific channel.
> 
> Note: previous "attempts" for adding ADT7481 in lm90 where here: [1][2].
> 
> [1] https://www.spinics.net/lists/lm-sensors/msg25066.html
> [2] https://marc.info/?l=lm-sensors&m=137786448326215&w=2
> 
> Slawomir Stepien (8):
>        dt-bindings: hwmon: Add support for ADT7481 in lm90
>        dt-bindings: hwmon: Add 'extended-range-enable' property support in lm90
>        dt-bindings: hwmon: Allow specifying channels for lm90
>        hwmon: (lm90) add support for ADT7481
>        hwmon: (lm90) define maximum number of channels that are supported
>        hwmon: (lm90) enable the extended temperature range
>        hwmon: (lm90) read the channel's label from device-tree
>        hwmon: (lm90) read the channel's offset from device-tree
> 
>   .../devicetree/bindings/hwmon/national,lm90.yaml          |  42 ++++
>   Documentation/hwmon/lm90.rst                              |  12 +-
>   drivers/hwmon/Kconfig                                     |  15 +-
>   drivers/hwmon/lm90.c                                      | 251 ++++++++++++++++++++----
>   4 files changed, 271 insertions(+), 49 deletions(-)
> 
> 

Re: [PATCH 0/8] Add support for ADT7481 in lm90

[Guenter Roeck]

On 5/20/22 06:45, Guenter Roeck wrote:
> On 5/20/22 02:32, Slawomir Stepien wrote:
>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>
>> This patch series adds support for ADT7481 in lm90.c driver and extends the
>> device-tree options for it.
>>
>> The ADT7481 is quite similar to MAX6696 (already supported in lm90) so a lot of
>> code is reused.
>>
>> One major problem in fitting the ADT7481 in lm90.c is the chip detection.
>> However it seems that the chip has been designed and produced with correct
>> values at locations: 0xfe (manufactured id) and 0xff (chip id), but this is not
>> documented in the datasheet.
>>
> 
> Before we go too far along this route, would you mind using my own patch series
> as base to implement the devicetree changes ? I had prepared an extensive
> patch series a while ago, adding not only support for ADT7481 but for
> several other chips as well, I just never got around to sending it out.
> 

I made my lm90 patch series available in branch hwmon-lm90 of
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git

The patches in this series were module tested and tested on real hardware
with the test scripts at git@github.com:groeck/module-tests.git;
look for scripts/lm90-real.sh and scripts/lm90.sh.

Guenter

Re: [PATCH 0/8] Add support for ADT7481 in lm90

[Slawomir Stepien]

On maj 20, 2022 09:01, Guenter Roeck wrote:
> On 5/20/22 06:45, Guenter Roeck wrote:
> > On 5/20/22 02:32, Slawomir Stepien wrote:
> > > From: Slawomir Stepien <slawomir.stepien@nokia.com>
> > > 
> > > This patch series adds support for ADT7481 in lm90.c driver and extends the
> > > device-tree options for it.
> > > 
> > > The ADT7481 is quite similar to MAX6696 (already supported in lm90) so a lot of
> > > code is reused.
> > > 
> > > One major problem in fitting the ADT7481 in lm90.c is the chip detection.
> > > However it seems that the chip has been designed and produced with correct
> > > values at locations: 0xfe (manufactured id) and 0xff (chip id), but this is not
> > > documented in the datasheet.
> > > 
> > 
> > Before we go too far along this route, would you mind using my own patch series
> > as base to implement the devicetree changes ? I had prepared an extensive
> > patch series a while ago, adding not only support for ADT7481 but for
> > several other chips as well, I just never got around to sending it out.
> > 
> 
> I made my lm90 patch series available in branch hwmon-lm90 of
> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
> 
> The patches in this series were module tested and tested on real hardware
> with the test scripts at git@github.com:groeck/module-tests.git;
> look for scripts/lm90-real.sh and scripts/lm90.sh.

I will test that out.

If I would be happy with that branch, should I rebase all my changes based on that branch and send
the patches to the lists?

-- 
Slawomir Stepien

Re: [PATCH 0/8] Add support for ADT7481 in lm90

[Guenter Roeck]

On 5/22/22 07:52, Slawomir Stepien wrote:
> On maj 20, 2022 09:01, Guenter Roeck wrote:
>> On 5/20/22 06:45, Guenter Roeck wrote:
>>> On 5/20/22 02:32, Slawomir Stepien wrote:
>>>> From: Slawomir Stepien <slawomir.stepien@nokia.com>
>>>>
>>>> This patch series adds support for ADT7481 in lm90.c driver and extends the
>>>> device-tree options for it.
>>>>
>>>> The ADT7481 is quite similar to MAX6696 (already supported in lm90) so a lot of
>>>> code is reused.
>>>>
>>>> One major problem in fitting the ADT7481 in lm90.c is the chip detection.
>>>> However it seems that the chip has been designed and produced with correct
>>>> values at locations: 0xfe (manufactured id) and 0xff (chip id), but this is not
>>>> documented in the datasheet.
>>>>
>>>
>>> Before we go too far along this route, would you mind using my own patch series
>>> as base to implement the devicetree changes ? I had prepared an extensive
>>> patch series a while ago, adding not only support for ADT7481 but for
>>> several other chips as well, I just never got around to sending it out.
>>>
>>
>> I made my lm90 patch series available in branch hwmon-lm90 of
>> git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
>>
>> The patches in this series were module tested and tested on real hardware
>> with the test scripts at git@github.com:groeck/module-tests.git;
>> look for scripts/lm90-real.sh and scripts/lm90.sh.
> 
> I will test that out.
> 
> If I would be happy with that branch, should I rebase all my changes based on that branch and send
> the patches to the lists?
> 

Yes, please, only I'll need to send my series first. With the commit
window coming up, I'll do that after I sent my pull request.

Thanks,
Guenter