Re: [PATCH v5 03/10] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 08/03/2025 18:29, Jonathan Cameron wrote:
> On Wed, 5 Mar 2025 12:54:33 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Thanks for the review David.
>>
>> On 04/03/2025 11:25, David Lechner wrote:
>>> On Mon, Mar 3, 2025 at 12:32 PM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
>>>>
>>>> There are ADC ICs which may have some of the AIN pins usable for other
>>>> functions. These ICs may have some of the AIN pins wired so that they
>>>> should not be used for ADC.
>>>>
>>>> (Preferred?) way for marking pins which can be used as ADC inputs is to
>>>> add corresponding channels@N nodes in the device tree as described in
>>>> the ADC binding yaml.
>>>>
>>>> Add couple of helper functions which can be used to retrieve the channel
>>>> information from the device node.
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>>
>>>> ---
>>
>>>> + *
>>>> + * Return:     Number of found channels on succes. Negative value to indicate
>>>
>>> s/succes/success/
>>
>> Thanks!
>>
>>>> +int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
>>>> +                                         const struct iio_chan_spec *template,
>>>> +                                         int max_chan_id,
>>>> +                                         struct iio_chan_spec **cs)
>>>> +{
>>>> +       struct iio_chan_spec *chan_array, *chan;
>>>> +       int num_chan = 0, ret;
>>>> +
>>>> +       num_chan = iio_adc_device_num_channels(dev);
>>>> +       if (num_chan < 1)
>>>> +               return num_chan;
>>>> +
>>>> +       chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array),
>>>> +                                 GFP_KERNEL);
>>>> +       if (!chan_array)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       chan = &chan_array[0];
>>>> +
>>>> +       device_for_each_child_node_scoped(dev, child) {
>>>> +               u32 ch;
>>>> +
>>>> +               if (!fwnode_name_eq(child, "channel"))
>>>> +                       continue;
>>>> +
>>>> +               ret = fwnode_property_read_u32(child, "reg", &ch);
>>>> +               if (ret)
>>>> +                       return ret;
>>>> +
>>>> +               if (max_chan_id != -1 && ch > max_chan_id)
>>>> +                       return -ERANGE;
>>>> +
>>>
>>> Should we use return dev_err_probe() on these to help with debugging a bad dtb?
>>>    
>>
>> I am not fan of using dev_err_probe() in a 'library code'. This is
>> because we never know if there'll be some odd use-case where this is not
>> called from the probe.
>>
>> All in all, I'd leave adding most of the debugs to the callers -
>> especially because we do not expect to have bad device-trees after the
>> initial 'development stage' of a board. The board 'development stage'
>> should really reveal bugs which prevent the channels from being
>> registered - and after the DT is correct, these debug prints become
>> unnecessary (albeit minor) binary bloat.
>>
>>>> +               *chan = *template;
>>>> +               chan->channel = ch;
>>>> +               chan++;
>>>> +       }
>>>> +
>>>> +       *cs = chan_array;
>>>> +
>>>> +       return num_chan;
>>>> +}
>>>> +EXPORT_SYMBOL_NS_GPL(devm_iio_adc_device_alloc_chaninfo_se, "IIO_DRIVER");
>>>
>>> We can make this less verbose by setting #define
>>> DEFAULT_SYMBOL_NAMESPACE at the start of the file. Then we can just do
>>> EXPORT_SYMBOL_GPL() throughout the rest of the file.
>>
>> I am not sure what to think of this. I use the good old 'ctrl + ]' in my
>> editor when I need to check how a function was supposed to be used. That
>> jumps to the spot of code where the function is. I'd like to see the
>> namespace mentioned there in order to not accidentally miss the fact the
>> function belongs to one.
>>
>> OTOH, I do like simplifications. Yet, the added simplification might not
>> warrant the namespace not being visible in the function definition.
>>
>>> Also, I would prefer if the namespace matched config name (IIO_ADC_HELPER).
>>
>> I had some lengthy discussion about this with Andy and Jonathan during
>> earlier review versions. In short, I don't like the idea of very
>> fragmented namespaces in IIO, which will just complicate the drivers
>> without providing any obvious benefit.
>>
>> https://lore.kernel.org/all/20250222174842.57c091c5@jic23-huawei/
>>
>>>> +
>>>> +int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
>>>> +                                         const struct iio_chan_spec *template,
>>>> +                                         int max_chan_id,
>>>> +                                         struct iio_chan_spec **cs);
>>>> +
>>>
>>> There are some different opinions on this, but on the last patch I did
>>> introducing a new namespace, the consensus seems to be that putting
>>> the MODULE_IMPORT_NS() in the header file was convenient so that users
>>> of the API don't have to remember to both include the header and add
>>> the import macro.
>>>    
>>
>> I do like this suggestion, and I believe this would be the balance
>> between getting the benefit of hiding part of the symbols - while not
>> unnecessarily complicating the callers. I know some people are opposing
>> it though. My personal opinion is that having the MODULE_IMPORT_NS() in
>> a header would be neatly simplifying the calling code with very little
>> harm, especially here where including the header hardly has use-cases
>> outside the IIO ADC.
>>
>> Unfortunately, the "safety" seems to often be a synonym for just "making
>> it intentionally hard". As Finnish people say: "Kärsi, kärsi,
>> kirkkaamman kruunun saat". :)
>> (Roughly translated as "Suffer, suffer, you will get a brighter crown").
>>
>> Let's hear what Jonathan thinks of your suggestion.
> 
> For this particular case my intent was that all the IIO exports that
> are suitable for use in simple IIO drives will be in this namespace,
> we just haven't started that conversion yet.
> 
> As such, having it defined from a header for this helper isn't a good
> thing to do.

Hmm. I agree.

>  Generally I prefer to see in driver code what namespaces
> are involved but do understand the other viewpoint. In this case I
> definitely don't think it is appropriate unless we go for a specific namespace
> for just this helper.

I suppose having the MODULE_IMPORT_NS() in the header would actually 
make the more fine-grained namespaces (like IIO_ADC_HELPERS, IIO_GTS, 
...) much more usable. That'd relieved the drivers from explicitly 
listing multiple namespaces while nicely limiting the visibility.

If IIO was my territory, I might want to ask people to go with that 
approach - but I am quite happy being a freeloade.. errm, I mean, 
bystander ;)

Thanks!

Yours,
	-- Matti