Message-ID: <28b9dda3-6fb6-49b5-90bd-716eb35a3104@gmail.com>
Date: Mon, 17 Feb 2025 09:08:51 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>,
 David Lechner <dlechner@...libre.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

On 11/02/2025 21:07, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:52:51 +0200
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> Hi Jonathan,
>>
>> Thanks for the review and all the comments!
>>
>> Just a note that I am currently spending some quality time with
>> rebuilding the floor of my house. Leaking floor drain can cause a bit of
>> a work... :rolleyes: So, my time with upstream work is a bit limited -
>> although writing an occasional bug or two can help one to keep the
>> balance ;)

The good thing when doing manual labour is that one can run thinking 
processes at the background - and has no time to reply instantly. XD So, 
I've been thinking about this quite a bit while installing new floor...

...

>>
>> Right, thanks.
>>
>>>> +			u32 ch;
>>>> +
>>>> +			ret = fwnode_property_read_u32(child, "reg", &ch);
>>>> +			if (ret)
>>>> +				return ret;
>>>
>>> In general the association between reg and channel is more complex.
>>> We need to deal with a reasonable subset of cases (single-channel, diff-channels
>>> etc) where it isn't the case that chan == chan->channel.
>>
>> I guess this is right. I, however, lacked of knowledge on how the
>> differential channels etc. are handled :) Hence I just implemented what
>> I believe is "the most common" approach, while leaving the rest to be
>> implemented by those who need it.
>>
>> Still, I agree that a generic looking API which silently produces bad
>> results for a few of the use-cases is not nice. By the very minimum we
>> should check if single-channel, diff-channels properties are present,
>> and then error out with a warning print. That should give a good hint
>> for those driver writers who are trying to use API for something
>> unsupported.
>>
>> Also, restrictions should be mentioned in the documentation.
>>
>> Do you think we should use some more specific function naming, like
>> "_simple" - suffix?
> 
> No to _simple. I think this needs to handle those cases before we
> take it at all  They should all have enough docs in adc.yaml to
> work out what happens.
> 

I am thinking it would be conceptually better to provide small helper 
(like the *_simpe() ) - which handled only specific properties. We can 
additionally provide '*_differential()' helper and potentially some 
'*_full()' helper which covers both cases.

Rationale is that:
  - writing _simple() helper is simpler. So is understanding it.
  - Devices may not support differential inputs. Drivers for those
   devices would want to call helper which does not accept the
   'differential' channel config. Parsing the differential channel
   information successfully and filling it in iio_chan_spec would be an
   error for those drivers.

> The most complex cases are all Analog Devices parts and those
> folk are very active on linux-iio so it should be easy to get
> them to review a series using it.
> 
> I don't think it is going to be particularly hard to generalise
> this.
> 
> We may end up with a variant that takes a 'per channel' callback
> to fill in more data + a private pointer of some type as often
> those are putting data in a parallel array of extra info about
> the channels.  Let's see what it looks like.

I was playing with this thought for a bit. I do love the regulator 
framework's '.of_parse_cp' which the driver can fill in the regulator 
desc. It's a pointer to function which is called by a regulator core 
after it locates the regulator node so the driver can handle regulator 
specific properties and do relevant configs in the registers.

I some sense the use case is similar - PMICs usually contain multiple 
regulators, each having own node. Being able to 'offload' locating the 
node to the regulator-core is often very handy for drivers.

I thought we could maybe add .fwnode_parse_chan_cp field in the iio_info 
structure. Idea being that it'd be a function which would be called for 
each channel - and it would probably be useful on other areas in IIO 
besides the ADCs.

Problem is that the number of channels may not be known prior parsing 
the device-tree. So, here is kind of a chicken-egg problem with 
populating the iio_chan_spec. Furthermore, locating the channel node in 
device-tree may not be 'standard enough' throughout all the different 
IIO areas.

What comes to a devicetree parsing helper which takes a pointer to a 
device specific configuration function as argument - my initial feeling 
is that it might get a bit overly complex to be user-friendly.

Hence, I'd keep this simple and provided small-but-usefull _simple() ;)

Well, if others aren't convinced by the usefulness of these helpers - 
then I'll probably just squash it into bd79124 driver and be done with 
this :) Later if I work for another ADC variant I might export it as 
'rohm_adc_helper' with an internal to IIO header :)

Thanks for the opinions / discussion!

Yours,
  -- Matti

Powered by blists - more mailing lists