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