Hi Krzysztof, As I wrote the original bindings, let me reply to some of your points... On Sun, 2022-10-16 at 21:59 -0400, Krzysztof Kozlowski wrote: > On 14/10/2022 08:37, Cosmin Tanislav wrote: > > From: Cosmin Tanislav <cosmin.tanislav@xxxxxxxxxx> > > > > Add support for the following parts: > > * LTC2984 > > * LTC2986 > > * LTM2985 > > > > The LTC2984 is a variant of the LTC2983 with EEPROM. > > The LTC2986 is a variant of the LTC2983 with only 10 channels, > > EEPROM and support for active analog temperature sensors. > > The LTM2985 is software-compatible with the LTC2986. > > > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@xxxxxxxxxx> > > --- > > .../bindings/iio/temperature/adi,ltc2983.yaml | 63 > > +++++++++++++++++-- > > 1 file changed, 59 insertions(+), 4 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam > > l > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam > > l > > index 722781aa4697..c33ab524fb64 100644 > > --- > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam > > l > > +++ > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam > > l > > @@ -4,19 +4,27 @@ > > $id: > > http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml# > > $schema: http://devicetree.org/meta-schemas/core.yaml# > > > > -title: Analog Devices LTC2983 Multi-sensor Temperature system > > +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor > > Temperature system > > > > maintainers: > > - Nuno Sá <nuno.sa@xxxxxxxxxx> > > > > description: | > > - Analog Devices LTC2983 Multi-Sensor Digital Temperature > > Measurement System > > + Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor > > Digital > > + Temperature Measurement Systems > > + > > > > https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf > > + > > https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf > > + > > https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf > > + > > https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf > > > > properties: > > compatible: > > enum: > > - adi,ltc2983 > > + - adi,ltc2984 > > + - adi,ltc2986 > > + - adi,ltm2985 > > > > reg: > > maxItems: 1 > > @@ -26,7 +34,7 @@ properties: > > > > adi,mux-delay-config-us: > > description: > > - The LTC2983 performs 2 or 3 internal conversion cycles per > > temperature > > + The device performs 2 or 3 internal conversion cycles per > > temperature > > result. Each conversion cycle is performed with different > > excitation and > > input multiplexer configurations. Prior to each conversion, > > these > > excitation circuits and input switch configurations are > > changed and an > > @@ -145,7 +153,7 @@ patternProperties: > > adi,three-conversion-cycles: > > description: > > Boolean property which set's three conversion cycles > > removing > > - parasitic resistance effects between the LTC2983 and the > > diode. > > + parasitic resistance effects between the device and the > > diode. > > type: boolean > > > > adi,average-on: > > @@ -353,6 +361,41 @@ patternProperties: > > description: Boolean property which set's the adc as > > single-ended. > > type: boolean > > > > + "^temp@": > > There is already a property for thermocouple. Isn't a thermocouple a > temperature sensor? IOW, why new property is needed? > Well, most of the patternProps in this bindings are temperature sensors... It's just that the device(s) support different types of them. 'adi,sensor-type' is used to identify each sensor (as this translates in different configurations being written in the device channels). > > + type: object > > + description: > > + Represents a channel which is being used as an active analog > > temperature > > + sensor. > > + > > + properties: > > + adi,sensor-type: > > + description: > > + Identifies the sensor as an active analog temperature > > sensor. > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + const: 31 > > + > > + adi,single-ended: > > + description: Boolean property which sets the sensor as > > single-ended. > > Drop "Boolean property which sets" - it's obvious from the type. > > > > > + type: boolean > > + > > + adi,custom-temp: > > + description: > > + This is a table, where each entry should be a pair of > > "This is a table" - obvious from the type. > > > + voltage(mv)-temperature(K). The entries must be given in > > nv and uK > > mv-K or nv-uK? Confusing... Yeah, a bit. In Cosmin defense, I think he's just keeping the same "style" as the rest of the properties... > > > + so that, the original values must be multiplied by > > 1000000. For > > + more details look at table 71 and 72. > > There is no table 71 in the bindings... It seems you pasted it from > somewhere. I'm fairly sure this refers to the datasheet. I see now that this can be confusing (again this kind of references are being (ab)used in the rest of the file). > > > + Note should be signed, but dtc doesn't currently > > maintain the > > + sign. > > What do you mean? "Maintain" as allow or keep when building FDT? > What's > the problem of using negative numbers here and why it should be part > of > bindings? > > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > + minItems: 3 > > + maxItems: 64 > > + items: > > + minItems: 2 > > + maxItems: 2 > > Instead describe the items with "description" (and maybe constraints) > like here: > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278 > Neat... My only comment (which probably applies to my previous ones) is that the rest of the properties are already in this "style". So maybe, follow up patches with small clean-ups would be more appropriate? > - Nuno Sá
