diff mbox series

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

Message ID 20221209215308.1781047-1-marijn.suijten@somainline.org (mailing list archive)
State Superseded
Headers show
Series [RFC] arm64: dts: qcom: Use labels with generic node names for ADC channels | expand

Commit Message

Marijn Suijten Dec. 9, 2022, 9:53 p.m. UTC
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@somainline.org/T/#u

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 arch/arm64/boot/dts/qcom/pm6125.dtsi          | 18 ++++++---
 arch/arm64/boot/dts/qcom/pm660.dtsi           | 33 ++++++++++------
 arch/arm64/boot/dts/qcom/pm8950.dtsi          | 39 ++++++++++++-------
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts      | 15 ++++---
 .../dts/qcom/sc7180-trogdor-coachz-r1.dts     |  4 +-
 .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi  |  3 +-
 .../dts/qcom/sc7180-trogdor-homestar.dtsi     |  3 +-
 .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi   |  2 +-
 .../dts/qcom/sc7180-trogdor-pompom-r1.dts     |  2 +-
 .../dts/qcom/sc7180-trogdor-pompom-r2.dts     |  2 +-
 .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi  |  3 +-
 .../dts/qcom/sc7180-trogdor-wormdingler.dtsi  |  3 +-
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |  3 +-
 .../qcom/sm6125-sony-xperia-seine-pdx201.dts  | 15 ++++---
 arch/arm64/boot/dts/qcom/sm8250-mtp.dts       | 21 ++++++----
 15 files changed, 109 insertions(+), 57 deletions(-)

Comments

Krzysztof Kozlowski Dec. 10, 2022, 11:02 a.m. UTC | #1
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@somainline.org/T/#u
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

The talk was in context of bindings, not about changing all existing
users thus affecting DTS. 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.

Anyway I am against such changes without expressing it in the bindings.

Best regards,
Krzysztof
Marijn Suijten Dec. 10, 2022, 4:54 p.m. UTC | #2
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@somainline.org/T/#u
> > 
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> 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)?  Or to keep the fully-written-out
"thermistor" word in the label?

> 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.

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`?

[1]: https://lore.kernel.org/linux-arm-msm/20221208101232.536i3cmjf4uk2z52@SoMainline.org/

- Marijn
Jonathan Cameron Dec. 11, 2022, 2:15 p.m. UTC | #3
On Sat, 10 Dec 2022 17:54:34 +0100
Marijn Suijten <marijn.suijten@somainline.org> 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@somainline.org/T/#u
> > > 
> > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>  
> > 
> > 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.

Gah. We missed that at the time.  Arguably we should first fix that
particular issue as we will have lots of old DT out there.
(add a bit of code to strip the @xxx bit from that particular usecase).
It gets tricky because now we might have code relying on the new
broken behavior.

> 
> 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)?  Or to keep the fully-written-out
> "thermistor" word in the label?
> 
> > 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.
> 
> 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`?
> 
> [1]: https://lore.kernel.org/linux-arm-msm/20221208101232.536i3cmjf4uk2z52@SoMainline.org/

So the 'fun' here is what to do with old DTS as we need to support that
even if we update the binding docs and all in kernel users.

Probably right option in driver is:
a) Use label if present
b) Use node name if it's not adc-chan but strip the @xxx off it.
c) return an error.

p.s. Please add iio@vger.kernel.org to future versions of this. If nothing
else I tend to lose direct emails about IIO stuff as they aren't in the
relevant patchwork instance.

> 
> - Marijn
Krzysztof Kozlowski Dec. 12, 2022, 8:48 a.m. UTC | #4
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@somainline.org/T/#u
>>>
>>>
>>> 
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>> 
>> 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.

> 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.

> 
>> 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.

Best regards,
Krzysztof
Marijn Suijten Dec. 14, 2022, 8:49 p.m. UTC | #5
On 2022-12-11 14:15:26, Jonathan Cameron wrote:
> On Sat, 10 Dec 2022 17:54:34 +0100
> Marijn Suijten <marijn.suijten@somainline.org> 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@somainline.org/T/#u
> > > > 
> > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>  
> > > 
> > > 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.
> 
> Gah. We missed that at the time.  Arguably we should first fix that
> particular issue as we will have lots of old DT out there.
> (add a bit of code to strip the @xxx bit from that particular usecase).
> It gets tricky because now we might have code relying on the new
> broken behavior.

Before rushing to fix that, my idea was to simply only read DT labels in
the driver, and otherwise fall back to the hardcoded names inside that
IIO driver (again: ADC5 defines friendly names in the driver, but
doesn't ever reference them besides a worthless non-NULL check).

> > 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)?  Or to keep the fully-written-out
> > "thermistor" word in the label?
> > 
> > > 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.
> > 
> > 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`?
> > 
> > [1]: https://lore.kernel.org/linux-arm-msm/20221208101232.536i3cmjf4uk2z52@SoMainline.org/
> 
> So the 'fun' here is what to do with old DTS as we need to support that
> even if we update the binding docs and all in kernel users.

Personally I have never cared about backwards compat as all my devices
rely on the DT and drivers to be brought up in tandem, but yes that is a
problem for others...

> Probably right option in driver is:
> a) Use label if present
> b) Use node name if it's not adc-chan but strip the @xxx off it.

Perhaps we can skip this entirely: as shown by this patch the use of
node names instead of labels is limited to "newer" devices and SoCs,
given their "active" development leads to the assumption they must also
flash their kernel and DTB updates in tandem?  Unless I missed a whole
bunch of nodes.

> c) return an error.

Again, we could also fall back to the names coded in the driver.  They
exist for ADC5, for VADC they're preprocessor-stringified definitions,
and should be made less shouty first IMO.

> p.s. Please add iio@vger.kernel.org to future versions of this. If nothing
> else I tend to lose direct emails about IIO stuff as they aren't in the
> relevant patchwork instance.

Ack, blindly used get_maintained without realizing this!

Apologies for not having replied to our initial discussion yet; it
spiraled out of control in terms of size.  I'll see if I can clarify
some of the points in hopes of bringing this forward too.

- Marijn
Marijn Suijten Dec. 14, 2022, 8:55 p.m. UTC | #6
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@somainline.org/T/#u
> >>>
> >>>
> >>> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> >> 
> >> 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
Krzysztof Kozlowski Dec. 16, 2022, 10:44 a.m. UTC | #7
On 14/12/2022 21:49, Marijn Suijten wrote:
> On 2022-12-11 14:15:26, Jonathan Cameron wrote:
>> On Sat, 10 Dec 2022 17:54:34 +0100
>> Marijn Suijten <marijn.suijten@somainline.org> 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@somainline.org/T/#u
>>>>>
>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>  
>>>>
>>>> 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.
>>
>> Gah. We missed that at the time.  Arguably we should first fix that
>> particular issue as we will have lots of old DT out there.
>> (add a bit of code to strip the @xxx bit from that particular usecase).
>> It gets tricky because now we might have code relying on the new
>> broken behavior.
> 
> Before rushing to fix that, my idea was to simply only read DT labels in
> the driver, and otherwise fall back to the hardcoded names inside that
> IIO driver (again: ADC5 defines friendly names in the driver, but
> doesn't ever reference them besides a worthless non-NULL check).
> 
>>> 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)?  Or to keep the fully-written-out
>>> "thermistor" word in the label?
>>>
>>>> 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.
>>>
>>> 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`?
>>>
>>> [1]: https://lore.kernel.org/linux-arm-msm/20221208101232.536i3cmjf4uk2z52@SoMainline.org/
>>
>> So the 'fun' here is what to do with old DTS as we need to support that
>> even if we update the binding docs and all in kernel users.
> 
> Personally I have never cared about backwards compat as all my devices
> rely on the DT and drivers to be brought up in tandem, but yes that is a
> problem for others...

Yeah, but we do not accept patches with each other person point of view
of ABI. ABI rules are generic, otherwise it would not be ABI, right?

> 
>> Probably right option in driver is:
>> a) Use label if present
>> b) Use node name if it's not adc-chan but strip the @xxx off it.
> 
> Perhaps we can skip this entirely: as shown by this patch the use of
> node names instead of labels is limited to "newer" devices and SoCs,
> given their "active" development leads to the assumption they must also

Only SM8550, SC8280xp and maybe SM8450 qualify to such "active"
development platforms.

> flash their kernel and DTB updates in tandem? 

No, DTS is used outside in other projects and out of tree kernels. Just
because you use them together does not change the requirements of DTS
and kernel of not breaking other users, without clear, justified reason.

Best regards,
Krzysztof
Krzysztof Kozlowski Dec. 16, 2022, 10:49 a.m. UTC | #8
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@somainline.org/T/#u
>>>>>
>>>>>
>>>>>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>
>>>> 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
Marijn Suijten Dec. 16, 2022, 10:53 a.m. UTC | #9
On 2022-12-16 11:44:34, Krzysztof Kozlowski wrote:
> On 14/12/2022 21:49, Marijn Suijten wrote:
> > On 2022-12-11 14:15:26, Jonathan Cameron wrote:
> >> On Sat, 10 Dec 2022 17:54:34 +0100
> >> Marijn Suijten <marijn.suijten@somainline.org> 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@somainline.org/T/#u
> >>>>>
> >>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>  
> >>>>
> >>>> 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.
> >>
> >> Gah. We missed that at the time.  Arguably we should first fix that
> >> particular issue as we will have lots of old DT out there.
> >> (add a bit of code to strip the @xxx bit from that particular usecase).
> >> It gets tricky because now we might have code relying on the new
> >> broken behavior.
> > 
> > Before rushing to fix that, my idea was to simply only read DT labels in
> > the driver, and otherwise fall back to the hardcoded names inside that
> > IIO driver (again: ADC5 defines friendly names in the driver, but
> > doesn't ever reference them besides a worthless non-NULL check).
> > 
> >>> 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)?  Or to keep the fully-written-out
> >>> "thermistor" word in the label?
> >>>
> >>>> 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.
> >>>
> >>> 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`?
> >>>
> >>> [1]: https://lore.kernel.org/linux-arm-msm/20221208101232.536i3cmjf4uk2z52@SoMainline.org/
> >>
> >> So the 'fun' here is what to do with old DTS as we need to support that
> >> even if we update the binding docs and all in kernel users.
> > 
> > Personally I have never cared about backwards compat as all my devices
> > rely on the DT and drivers to be brought up in tandem, but yes that is a
> > problem for others...
> 
> Yeah, but we do not accept patches with each other person point of view
> of ABI. ABI rules are generic, otherwise it would not be ABI, right?

Hence I am not outright rejecting the proposal to maintain ABI
compatibility.

However I won't be the one reverting the fwnode conversion or writing
the code to strip the `@xx` suffix.  We're already on a tangent of a
tangent, with my original goal to add the missing label names to the
VADC driver.

> > 
> >> Probably right option in driver is:
> >> a) Use label if present
> >> b) Use node name if it's not adc-chan but strip the @xxx off it.
> > 
> > Perhaps we can skip this entirely: as shown by this patch the use of
> > node names instead of labels is limited to "newer" devices and SoCs,
> > given their "active" development leads to the assumption they must also
> 
> Only SM8550, SC8280xp and maybe SM8450 qualify to such "active"
> development platforms.

I guess we'll have to agree to disagree here.  Many more SoCs are still
in swing or so incomplete that it hardly makes any sense to not update
/both/ regularly.

That's not a valid argument to break ABI though, so we'll leave it at
that.

> > flash their kernel and DTB updates in tandem? 
> 
> No, DTS is used outside in other projects and out of tree kernels. Just
> because you use them together does not change the requirements of DTS
> and kernel of not breaking other users, without clear, justified reason.

That's known, but inconvenient nevertheless given the rate at which both
"evolve".

 - Marijn
Marijn Suijten Dec. 16, 2022, 11:03 a.m. UTC | #10
On 2022-12-16 11:49:09, Krzysztof Kozlowski wrote:
> [..]
> > 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.

Ack.  Then it's up to the driver to figure out what's best here.  My
suggestion on top of the proposed handling by Jonathan:

a) Use label if present.
b) Use node name if it's not adc-chan but strip the @xxx off it.
c) Use (currently unused) hardcoded name in the driver.

Unfortunately we have two drivers (VADC and ADC5) to deal with, where the
VADC driver is not affected by any of these filename problems, nor won't
be.  It doesn't have a label in Linux at all yet so there's no userspace
ABI to (un)break, only DT ABI.

- Marijn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/pm6125.dtsi b/arch/arm64/boot/dts/qcom/pm6125.dtsi
index 59092a551a16..7cfd73f8707e 100644
--- a/arch/arm64/boot/dts/qcom/pm6125.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm6125.dtsi
@@ -85,36 +85,42 @@  pm6125_adc: adc@3100 {
 			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
 			#io-channel-cells = <1>;
 
-			ref-gnd@0 {
+			adc-chan@0 {
 				reg = <ADC5_REF_GND>;
 				qcom,pre-scaling = <1 1>;
+				label = "ref_gnd";
 			};
 
-			vref-1p25@1 {
+			adc-chan@1 {
 				reg = <ADC5_1P25VREF>;
 				qcom,pre-scaling = <1 1>;
+				label = "vref_1p25";
 			};
 
-			die-temp@6 {
+			adc-chan@6 {
 				reg = <ADC5_DIE_TEMP>;
 				qcom,pre-scaling = <1 1>;
+				label = "die_temp";
 			};
 
-			vph-pwr@83 {
+			adc-chan@83 {
 				reg = <ADC5_VPH_PWR>;
 				qcom,pre-scaling = <1 3>;
+				label = "vph_pwr";
 			};
 
-			vcoin@85 {
+			adc-chan@85 {
 				reg = <ADC5_VCOIN>;
 				qcom,pre-scaling = <1 3>;
+				label = "vcoin";
 			};
 
-			xo-therm@4c {
+			adc-chan@4c {
 				reg = <ADC5_XO_THERM_100K_PU>;
 				qcom,pre-scaling = <1 1>;
 				qcom,hw-settle-time = <200>;
 				qcom,ratiometric;
+				label = "xo_therm";
 			};
 		};
 
diff --git a/arch/arm64/boot/dts/qcom/pm660.dtsi b/arch/arm64/boot/dts/qcom/pm660.dtsi
index fc0eccaccdf6..d05bd8f2170b 100644
--- a/arch/arm64/boot/dts/qcom/pm660.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm660.dtsi
@@ -91,82 +91,93 @@  pm660_adc: adc@3100 {
 			#size-cells = <0>;
 			#io-channel-cells = <1>;
 
-			ref_gnd: ref_gnd@0 {
+			adc-chan@0 {
 				reg = <ADC5_REF_GND>;
 				qcom,decimation = <1024>;
 				qcom,pre-scaling = <1 1>;
+				label = "ref_gnd";
 			};
 
-			vref_1p25: vref_1p25@1 {
+			adc-chan@1 {
 				reg = <ADC5_1P25VREF>;
 				qcom,decimation = <1024>;
 				qcom,pre-scaling = <1 1>;
+				label = "vref_1p25";
 			};
 
-			die_temp: die_temp@6 {
+			adc-chan@6 {
 				reg = <ADC5_DIE_TEMP>;
 				qcom,decimation = <1024>;
 				qcom,pre-scaling = <1 1>;
+				label = "die_temp";
 			};
 
-			xo_therm: xo_therm@4c {
+			adc-chan@4c {
 				reg = <ADC5_XO_THERM_100K_PU>;
 				qcom,pre-scaling = <1 1>;
 				qcom,decimation = <1024>;
 				qcom,hw-settle-time = <200>;
 				qcom,ratiometric;
+				label = "xo_therm";
 			};
 
-			msm_therm: msm_therm@4d {
+			adc-chan@4d {
 				reg = <ADC5_AMUX_THM1_100K_PU>;
 				qcom,pre-scaling = <1 1>;
 				qcom,decimation = <1024>;
 				qcom,hw-settle-time = <200>;
 				qcom,ratiometric;
+				label = "msm_therm";
 			};
 
-			emmc_therm: emmc_therm@4e {
+			adc-chan@4e {
 				reg = <ADC5_AMUX_THM2_100K_PU>;
 				qcom,pre-scaling = <1 1>;
 				qcom,decimation = <1024>;
 				qcom,hw-settle-time = <200>;
 				qcom,ratiometric;
+				label = "emmc_therm";
 			};
 
-			pa_therm0: thermistor0@4f {
+			adc-chan@4f {
 				reg = <ADC5_AMUX_THM3_100K_PU>;
 				qcom,pre-scaling = <1 1>;
 				qcom,decimation = <1024>;
 				qcom,hw-settle-time = <200>;
 				qcom,ratiometric;
+				label = "pa_therm0";
 			};
 
-			pa_therm1: thermistor1@50 {
+			adc-chan@50 {
 				reg = <ADC5_AMUX_THM4_100K_PU>;
 				qcom,pre-scaling = <1 1>;
 				qcom,decimation = <1024>;
 				qcom,hw-settle-time = <200>;
 				qcom,ratiometric;
+				label = "pa_therm1";
 			};
 
-			quiet_therm: quiet_therm@51 {
+			adc-chan@51 {
 				reg = <ADC5_AMUX_THM5_100K_PU>;
 				qcom,pre-scaling = <1 1>;
 				qcom,decimation = <1024>;
 				qcom,hw-settle-time = <200>;
 				qcom,ratiometric;
+				label = "quiet_therm";
 			};
 
-			vadc_vph_pwr: vph_pwr@83 {
+			adc-chan@83 {
 				reg = <ADC5_VPH_PWR>;
 				qcom,decimation = <1024>;
 				qcom,pre-scaling = <1 3>;
+				label = "vph_pwr";
 			};
 
-			vcoin: vcoin@85 {
+			adc-chan@85 {
 				reg = <ADC5_VCOIN>;
 				qcom,decimation = <1024>;
 				qcom,pre-scaling = <1 3>;
+				label = "vcoin";
 			};
 		};
 
diff --git a/arch/arm64/boot/dts/qcom/pm8950.dtsi b/arch/arm64/boot/dts/qcom/pm8950.dtsi
index 631761f98999..c07572b732dc 100644
--- a/arch/arm64/boot/dts/qcom/pm8950.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8950.dtsi
@@ -50,77 +50,90 @@  pm8950_vadc: adc@3100 {
 			#size-cells = <0>;
 			#io-channel-cells = <1>;
 
-			vcoin@5 {
+			adc-chan@5 {
 				reg = <VADC_VCOIN>;
 				qcom,pre-scaling = <1 1>;
+				label = "vcoin";
 			};
 
-			vph-pwr@7 {
+			adc-chan@7 {
 				reg = <VADC_VSYS>;
 				qcom,pre-scaling = <1 1>;
+				label = "vph_pwr";
 			};
 
-			die-temp@8 {
+			adc-chan@8 {
 				reg = <VADC_DIE_TEMP>;
 				qcom,pre-scaling = <1 1>;
+				label = "die_temp";
 			};
 
-			ref-625mv@9 {
+			adc-chan@9 {
 				reg = <VADC_REF_625MV>;
 				qcom,pre-scaling = <1 1>;
+				label = "ref_625mv";
 			};
 
-			ref-1250mv@a {
+			adc-chan@a {
 				reg = <VADC_REF_1250MV>;
 				qcom,pre-scaling = <1 1>;
+				label = "ref_1250mv";
 			};
 
-			ref-buf-625mv@c {
+			adc-chan@c {
 				reg = <VADC_SPARE1>;
 				qcom,pre-scaling = <1 1>;
+				label = "ref_buf_625mv";
 			};
 
-			ref-gnd@e {
+			adc-chan@e {
 				reg = <VADC_GND_REF>;
+				label = "ref_gnd";
 			};
 
-			ref-vdd@f {
+			adc-chan@f {
 				reg = <VADC_VDD_VADC>;
+				label = "ref_vdd";
 			};
 
-			pa-therm1@11 {
+			adc-chan@11 {
 				reg = <VADC_P_MUX2_1_1>;
 				qcom,pre-scaling = <1 1>;
 				qcom,ratiometric;
 				qcom,hw-settle-time = <200>;
+				label = "pa_therm1";
 			};
 
-			case-therm@13 {
+			adc-chan@13 {
 				reg = <VADC_P_MUX4_1_1>;
 				qcom,pre-scaling = <1 1>;
 				qcom,ratiometric;
 				qcom,hw-settle-time = <200>;
+				label = "case_therm";
 			};
 
-			xo-therm@32 {
+			adc-chan@32 {
 				reg = <VADC_LR_MUX3_XO_THERM>;
 				qcom,pre-scaling = <1 1>;
 				qcom,ratiometric;
 				qcom,hw-settle-time = <200>;
+				label = "xo_therm";
 			};
 
-			pa-therm0@36 {
+			adc-chan@36 {
 				reg = <VADC_LR_MUX7_HW_ID>;
 				qcom,pre-scaling = <1 1>;
 				qcom,ratiometric;
 				qcom,hw-settle-time = <200>;
+				label = "pa_therm0";
 			};
 
-			xo-therm-buf@3c {
+			adc-chan@3c {
 				reg = <VADC_LR_MUX3_BUF_XO_THERM>;
 				qcom,pre-scaling = <1 1>;
 				qcom,ratiometric;
 				qcom,hw-settle-time = <200>;
+				label = "xo_therm_buf";
 			};
 		};
 
diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
index 8c64cb060e21..4ebf2e0fe838 100644
--- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
+++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
@@ -644,16 +644,18 @@  &mdss_mdp {
 };
 
 &pm8150_adc {
-	xo-therm@4c {
+	adc-chan@4c {
 		reg = <ADC5_XO_THERM_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "xo_therm";
 	};
 
-	wifi-therm@4e {
+	adc-chan@4e {
 		reg = <ADC5_AMUX_THM2_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "wifi_therm";
 	};
 };
 
@@ -721,10 +723,11 @@  &pm8150_gpios {
 };
 
 &pm8150b_adc {
-	conn-therm@4f {
+	adc-chan@4f {
 		reg = <ADC5_AMUX_THM3_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "conn_therm";
 	};
 };
 
@@ -756,16 +759,18 @@  &pm8150b_gpios {
 };
 
 &pm8150l_adc {
-	skin-msm-therm@4e {
+	adc-chan@4e {
 		reg = <ADC5_AMUX_THM2_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "skin_msm_therm";
 	};
 
-	pm8150l-therm@4f {
+	adc-chan@4f {
 		reg = <ADC5_AMUX_THM3_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "pm8150l_therm";
 	};
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts
index edfcd47e1a00..2db57ecd71c5 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts
@@ -24,8 +24,8 @@  &charger_thermal {
 };
 
 &pm6150_adc {
-	/delete-node/ skin-temp-thermistor@4e;
-	/delete-node/ charger-thermistor@4f;
+	/delete-node/ adc-chan@4e;
+	/delete-node/ adc-chan@4f;
 };
 
 &pm6150_adc_tm {
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
index 8b8ea8af165d..4dd51ba62f6f 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
@@ -119,10 +119,11 @@  &panel {
 };
 
 &pm6150_adc {
-	skin-temp-thermistor@4e {
+	adc-chan@4e {
 		reg = <ADC5_AMUX_THM2_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "skin_therm";
 	};
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
index d3cf64c16dcd..1511c48f5278 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
@@ -127,10 +127,11 @@  &panel {
 };
 
 &pm6150_adc {
-	skin-temp-thermistor@4d {
+	adc-chan@4d {
 		reg = <ADC5_AMUX_THM1_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "skin_therm";
 	};
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
index 269007d73162..508d7e4bdf62 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
@@ -55,7 +55,7 @@  &panel {
 };
 
 &pm6150_adc {
-	/delete-node/ charger-thermistor@4f;
+	/delete-node/ adc-chan@4f;
 };
 
 &pm6150_adc_tm {
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts
index 8467ff41e6d5..b2a0164529d4 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts
@@ -27,7 +27,7 @@  &charger_thermal {
 };
 
 &pm6150_adc {
-	/delete-node/ charger-thermistor@4f;
+	/delete-node/ adc-chan@4f;
 };
 
 &pm6150_adc_tm {
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts
index 88cf2246c18a..e620001240db 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts
@@ -24,7 +24,7 @@  &charger_thermal {
 };
 
 &pm6150_adc {
-	/delete-node/ charger-thermistor@4f;
+	/delete-node/ adc-chan@4f;
 };
 
 &pm6150_adc_tm {
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
index 6c5287bd27d6..701ec7892b42 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
@@ -148,10 +148,11 @@  &pen_insert {
 };
 
 &pm6150_adc {
-	5v-choke-thermistor@4e {
+	adc-chan@4e {
 		reg = <ADC5_AMUX_THM2_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "5v_choke_therm";
 	};
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
index 9832e752da35..5eccb7860711 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
@@ -168,10 +168,11 @@  ap_ts: touchscreen@1 {
 };
 
 &pm6150_adc {
-	skin-temp-thermistor@4d {
+	adc-chan@4d {
 		reg = <ADC5_AMUX_THM1_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "skin_therm";
 	};
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index b82956f8f1cf..3c3031aa0e55 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -820,10 +820,11 @@  &mdss_dp {
 };
 
 &pm6150_adc {
-	charger-thermistor@4f {
+	adc-chan@4f {
 		reg = <ADC5_AMUX_THM3_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "charger_therm";
 	};
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts b/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts
index 650819c028b6..2805a490a6df 100644
--- a/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts
+++ b/arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts
@@ -149,39 +149,44 @@  &pm6125_adc {
 	pinctrl-names = "default";
 	pinctrl-0 = <&camera_flash_therm &emmc_ufs_therm &rf_pa1_therm>;
 
-	rf-pa0-therm@4d {
+	adc-chan@4d {
 		reg = <ADC5_AMUX_THM1_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
 		qcom,pre-scaling = <1 1>;
+		label = "rf_pa0_therm";
 	};
 
-	quiet-therm@4e {
+	adc-chan@4e {
 		reg = <ADC5_AMUX_THM2_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
 		qcom,pre-scaling = <1 1>;
+		label = "quiet_therm";
 	};
 
-	camera-flash-therm@52 {
+	adc-chan@52 {
 		reg = <ADC5_GPIO1_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
 		qcom,pre-scaling = <1 1>;
+		label = "camera_flash_therm";
 	};
 
-	emmc-ufs-therm@54 {
+	adc-chan@54 {
 		reg = <ADC5_GPIO3_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
 		qcom,pre-scaling = <1 1>;
+		label = "emmc_ufs_therm";
 	};
 
-	rf-pa1-therm@55 {
+	adc-chan@55 {
 		reg = <ADC5_GPIO4_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
 		qcom,pre-scaling = <1 1>;
+		label = "rf_pa1_therm";
 	};
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sm8250-mtp.dts b/arch/arm64/boot/dts/qcom/sm8250-mtp.dts
index 3ed8c84e25b8..f4ac617dfab4 100644
--- a/arch/arm64/boot/dts/qcom/sm8250-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8250-mtp.dts
@@ -502,22 +502,25 @@  &i2c15 {
 };
 
 &pm8150_adc {
-	xo-therm@4c {
+	adc-chan@4c {
 		reg = <ADC5_XO_THERM_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "xo_therm";
 	};
 
-	skin-therm@4d {
+	adc-chan@4d {
 		reg = <ADC5_AMUX_THM1_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "skin_therm";
 	};
 
-	pa-therm1@4e {
+	adc-chan@4e {
 		reg = <ADC5_AMUX_THM2_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "pa_therm1";
 	};
 };
 
@@ -547,10 +550,11 @@  pa-therm1@2 {
 };
 
 &pm8150b_adc {
-	conn-therm@4f {
+	adc-chan@4f {
 		reg = <ADC5_AMUX_THM3_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "conn_therm";
 	};
 };
 
@@ -591,22 +595,25 @@  pa-therm2@2 {
 };
 
 &pm8150l_adc {
-	camera-flash-therm@4d {
+	adc-chan@4d {
 		reg = <ADC5_AMUX_THM1_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "camera_flash_therm";
 	};
 
-	skin-msm-therm@4e {
+	adc-chan@4e {
 		reg = <ADC5_AMUX_THM2_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "skin_msm_therm";
 	};
 
-	pa-therm2@4f {
+	adc-chan2@4f {
 		reg = <ADC5_AMUX_THM3_100K_PU>;
 		qcom,ratiometric;
 		qcom,hw-settle-time = <200>;
+		label = "pa_therm";
 	};
 };