On 14/12/2022 21:55, Marijn Suijten wrote: > On 2022-12-12 09:48:26, Krzysztof Kozlowski wrote: >> On 10/12/2022 17:54, Marijn Suijten wrote: >>> On 2022-12-10 12:02:03, Krzysztof Kozlowski wrote: >>>> On 09/12/2022 22:53, Marijn Suijten wrote: >>>>> As discussed in [1] the DT should use labels to describe ADC >>>>> channels, with generic node names, since the IIO drivers now >>>>> moved to the fwnode API where node names include the `@xx` >>>>> address suffix. >>>>> >>>>> Especially for the ADC5 driver that uses extend_name - which >>>>> cannot be removed for compatibility reasons - this results in >>>>> sysfs files with the @xx name that wasn't previously present, and >>>>> leads to an unpleasant file-browsing experience. >>>>> >>>>> Also remove all the unused channel labels in pm660.dtsi. >>>>> >>>>> [1]: >>>>> https://lore.kernel.org/linux-arm-msm/20221106193018.270106-1-marijn.suijten@xxxxxxxxxxxxxx/T/#u >>>>> >>>>> >>>>> >> Signed-off-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx> >>>> >>>> The talk was in context of bindings, not about changing all >>>> existing users thus affecting DTS. >>> >>> And as a consequence, DTS. The already-merged transition from OF to >>> fwnode resulted in `@xx` to be included in the ADC channel name - and >>> in the case of ADC5 even in sysfs filenames - so this seems like a >>> necessary change to make. >>> >>> At the very least I would have changed the bindings submitted or >>> co-authored /by myself/ since I initially decided to rely on this >>> (now obviously) wrong behaviour, and should have used labels from the >>> get go. >>> >>>> What's more, to me "skin-temp-thermistor" is quite generic name, >>>> maybe "thermistor" would be more and reflects the purpose of the >>>> node, so it was more or less fine. >>> >>> Are you suggesting to not use "adc-chan", but "thermistor" as node >>> name (and still use skin_temp as label)? >> >> No, I am just saying that some of the names were correct, so the >> reasoning in commit msg is not entirely accurate. > > The commit msg doesn't state that any of these names was wrong, rather > that the label property should be used to convey a name to the driver. It stated: "with generic node names" and I said that some of the names are generic. Anyway, I am fine with changing the node names if this is coded in the bindings. > > If you think multiple /node names/ are acceptable, please discuss in > that direction. Do we use a predetermined set of names (adc-chan@xx, > thermistor@xx, ...) or do you have another suggestion? > >>> Or to keep the fully-written-out "thermistor" word in the label? >> >> No, I don't refer to labels. Labels don't matter, they are being removed >> entirely during DTS build. > > Ugh, I knew this was becoming a problem, even in an earlier IIO > discussions with Jonathan it was sometimes hard to keep apart: > > I am /not/ referring to DTS labels, but to the `label =` node > attribute/property, which is read by the driver (which isn't removed, > how else would the driver fish this value out at runtime...). > > After all none of this patch has any DTS labels, in fact a bunch of > unused ones in pm660 have been removed... but this patch /does add/ the > `label =` property to many nodes. > >>>> Anyway I am against such changes without expressing it in the >>>> bindings. >>> >>> As expressed in [1] I suggested and am all for locking this change >>> in via bindings, and you are right to expect that to have gone paired >>> with this patch. >> >> Yes, I expect such changes to have both binding and DTS change together. >> >>> >>> I'll submit that as the leading patch to this in v2, with the >>> wildcard pattern changed to adc-chan (or something else pending the >>> discussion above), and should I then also require the label property >>> via `label: true`? >> >> I don't think label is required. > > My commit message explains why the ADC5/VADC driver should really > receive a label property instead of using the node name. The reason "unpleasant file-browsing experience" is usually OS specific, so it does not justify requiring a label property. Label is just a helper for the users. Best regards, Krzysztof
