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

[Slawomir Stepien <sst@poczta.fm>]

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