| Message ID | 20220520093243.2523749-4-sst@poczta.fm (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for ADT7481 in lm90 | expand |
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
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>; > > + }; > > }; > > };
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
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!
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
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
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
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
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
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".
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". >
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".
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
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!
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>; + }; }; };