Re: [RFC PATCH] arm64: dts: qcom: Use labels with generic node names for ADC channels

[Marijn Suijten]

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.

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.

- Marijn