Date:   Mon, 16 Jan 2023 22:18:46 +0100
From:   Marijn Suijten <marijn.suijten@...ainline.org>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     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>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Nuno Sá <nuno.sa@...log.com>,
        Luca Weiss <luca@...tu.xyz>, linux-arm-msm@...r.kernel.org,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject: Re: [RFC PATCH] iio: adc: qcom-spmi-vadc: Propagate fw node
 name/label to extend_name

On 2022-12-23 17:44:45, Jonathan Cameron wrote:

<snip>

> > As in, as long as we don't touch extend_name which would affect sysfs
> > names, changing the label returned by read_label is fine? 
> 
> Sticky corner, but I think we should be fine doing so on basis of changing
> ABI we don't think anyone will notice. In lots of cases the label is effected
> by DTS files that may change under the kernel anyway so hopefully no one is
> relying on the value too much *crossed fingers*

Not sure I'd go as far as implementing read_label in adc5 (just queued
that up locally for vadc though and it's simple) only to get around
extend_name, but we could do so.  That still leaves @xx in the filename
and makes for general inconsistency when read_label (returned label name
from sysfs) and extend_name (filename in sysfs) use different codepaths
to determine their value.

> > And changing
> > datasheet_name to only ever use the datasheet_name provided by the
> > driver and never the name provided in DTS is also okay?
> 
> datasheet_name is internal to kernel only so can definitely change that
> as long as we don't have any upstream users (I'm fairly sure there aren't any)

Ack, queued up for RFC v2.

> > I am unfortunately completely unfamiliar with iio_map, and hope it
> > doesn't distract too much from trying to add label files to QCom's SPMI
> > VADC driver :)
> 
> Just think of it as the board file way of doing equivalent of what we have
> to set up IIO consumers in DT. It's also used in driver that hard code
> relationships with their consumers - not including this one so we should
> be fine.

Guess QCOM is all DT, so this shouldn't matter.

<snip>

> > > Ok. May make sense to use the datasheet name if noting better provided
> > > for the label.  Assuming the datasheet names are them selves somewhat
> > > useful information for a user.  
> > 
> > They're generated from the macro (hence capitalized) in VADC, manually
> > written in ADC5.  Would it make sense to add handwritten string
> > literals for this?
> 
> Not sure. I've rather lost track of where we are on this.

I'll send a v2 RFC shortly with what we've accumulated thus far, and
will make sure to mention this.  In short, in qcom-spmi-vadc.c there is:

    .datasheet_name = __stringify(_dname),

Which gives us a full-caps DIE_TEMP, for example, instead of a more
friendly "die_temp" string literal in qcom-spmi-adc5.c.  Not a
requirement for my patches, this should all go in separate bits.

<snip>

> > Feed into read_label when no label was otherwise provided in DTS, but
> > always feed into iio_chan_spec::datasheet_name since we discussed that
> > this should represent the name of the part (e.g. PMIC), not the board
> > and way in which it consumes the channel.
> 
> Should be the name of the pin on the part, but otherwise agreed.

Ack, I think that's what we have in the qcom-spmi-vadc/adc5 drivers
currently.

<snip>

> I hate it when we break ABI and don't notice, precisely because it then
> becomes guessing game on which one people might be relying on.
> 
> Let's take the view it is the older one without the @xx? 
> So strip that off as a fix that we backport.

I was intending to send a patch that falls back to
adc5_channels::datasheet_name when no label is supplied (and ignoring
fwnode_name for use in extend_name altogether) but that's breaking ABI
in a slightly different way... and depends on my "arm64: dts: qcom: Use
labels with generic node names for ADC channels" [1] RFC being resent
and actually landing.

[1]: https://lore.kernel.org/linux-arm-msm/20221209215308.1781047-1-marijn.suijten@somainline.org/

Will probably be shot down but I'll give it a try anyway.

<snip>

> I agree that a cross reference for that 'other' use of extend_name
> would make sense and that it can be overridden.  Though the override is
> kind of the common case, if you are looking at extend_name docs you
> are presumably in the case where such docs would help.
> Patches for docs welcome :) 

I'll see what I can add :)

> > Regardless of that VADC/ADC5 do some _really confusing_ things, passing
> > strings around in various weird ways (or not), and it took some time to
> > keep the various similar structs apart :)
> 
> I'm feeling a bit guilty I never noticed this insanity at the time.
> I blame my younger self.

Don't worry, we live and learn; just ABI (for something I doubt anyone
uses / cares about...) biting us every once in a while.

> Have a good break if you are having one.

Hope you had a good one; mine was filled with hardly any free time for
hobbies like the mainline kernel :)

- Marijn

Powered by blists - more mailing lists