On maj 20, 2022 12:13, Krzysztof Kozlowski wrote:
> On 20/05/2022 11:32, Slawomir Stepien wrote:
> > From: Slawomir Stepien <slawomir.stepien@xxxxxxxxx>
> >
> > Add binding description for temperature channels. Currently, support for
> > label and offset is implemented.
> >
> > Signed-off-by: Slawomir Stepien <slawomir.stepien@xxxxxxxxx>
> > ---
> > .../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
