Message-ID: <20221216105321.bxol64or5j4jeynh@SoMainline.org>
Date:   Fri, 16 Dec 2022 11:53:21 +0100
From:   Marijn Suijten <marijn.suijten@...ainline.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Jonathan Cameron <jic23@...nel.org>, phone-devel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Martin Botka <martin.botka@...ainline.org>,
        Jami Kettunen <jami.kettunen@...ainline.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] arm64: dts: qcom: Use labels with generic node names
 for ADC channels

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

Powered by blists - more mailing lists