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@xxxxxxxxxxxxxx> 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. > >> > >> 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@xxxxxxxxxxxxxx/ > >> > >> 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
