Re: [PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Matti Vaittinen]

On 16/03/2025 11:41, Jonathan Cameron wrote:
> On Thu, 13 Mar 2025 14:34:24 +0200
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> > On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:
> > > The new devm_iio_adc_device_alloc_chaninfo_se() -helper is intended to
> > > help drivers avoid open-coding the for_each_node -loop for getting the
> > > channel IDs. The helper provides standard way to detect the ADC channel
> > > nodes (by the node name), and a standard way to convert the "reg"
> > > -properties to channel identification numbers, used in the struct
> > > iio_chan_spec. Furthermore, the helper can optionally check the found
> > > channel IDs are smaller than given maximum. This is useful for callers
> > > which later use the IDs for example for indexing a channel data array.
> > >
> > > The original driver treated all found child nodes as channel nodes. The
> > > new helper requires channel nodes to be named channel[@N]. This should
> > > help avoid problems with devices which may contain also other but ADC
> > > child nodes. Quick grep from arch/* with the sun20i-gpadc's compatible
> > > string didn't reveal any in-tree .dts with channel nodes named
> > > otherwise. Also, same grep shows all the in-tree .dts seem to have
> > > channel IDs between 0..num of channels.
> > >
> > > Use the new helper.
> >
> > ...
> >
> > > +	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
> > > +				&sun20i_gpadc_chan_template, -1, &channels);
> > > +	if (num_channels < 0)
> > > +		return num_channels;
> > > +
> > >   	if (num_channels == 0)
> > >   		return dev_err_probe(dev, -ENODEV, "no channel children\n");
> >
> > Note, this what I would expected in your helper to see, i.e. separated cases
> > for < 0 (error code) and == 0, no channels.
> >
> > Also, are all users going to have this check? Usually in other similar APIs
> > we return -ENOENT. And user won't need to have an additional check in case of
> > 0 being considered as an error case too.
> In a few cases we'll need to do the dance the other way in the caller.
> So specifically check for -ENOENT and not treat it as an error.
>
> That stems from channel nodes being optionally added to drivers after
> they have been around a while (usually to add more specific configuration)
> and needing to maintain old behaviour of presenting all channels with default
> settings.
>
> I agree that returning -ENOENT is a reasonable way to handle this.

I agree - but I'm going to use -ENODEV instead of -ENOENT because that's 
what the current callers return if they find no channels. That way the 
drivers can return the value directly without converting -ENOENT to -ENODEV.

Yours,
	-- Matti