On Mon, 2022-10-17 at 19:32 -0400, Krzysztof Kozlowski wrote: > On 17/10/2022 05:38, Nuno Sá wrote: > > Hi Krzysztof, > > > > (...) > > > > > @@ -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). > > Sure. > > > > > > > + 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... > > That's not the best approach for two reasons: > 1. The unit used by hardware matters less here, because bindings are > used to write DTS. In many, many other cases there will be some > translation (just take random voltage regulator bindings). > > 2. What the driver is doing matters even less. > > So just describe here what is expected in DTS. > Alright, I see. So we just refer to nv-uK as that is what I wanted for dts to expect (reason being to have more resolution). > > > > > > > > > + 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). > > Yep, but there are now multiple datasheets, aren't there? > Hmm yeah that's true. By the time I wrote this binding I was not even thinking on the possibility of new parts being added to it... I guess the lesson in here is to avoid this kind os specific descriptions. > > > > > > > > > + 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? > > Of course. It would be great if the file was improved before or after > this one. > Ok, IMO it would make sense to have it in this series but if Cosmin does not feel like fixing my mess :), I'll send a separate patch with your inputs... - Nuno Sá
