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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Mar 13, 2025 at 03:17:27PM +0200, Matti Vaittinen wrote:
> On 13/03/2025 14:31, Andy Shevchenko wrote:
> > On Thu, Mar 13, 2025 at 09:18:18AM +0200, Matti Vaittinen wrote:

...

> > > +	num_chan = iio_adc_device_num_channels(dev);
> > > +	if (num_chan < 1)
> > > +		return num_chan;
> > 
> > This is really interesting code. So, if the above returns negative error code,
> > we return it, if it returns 0, we return success (but 0 channels)?
> 
> Yes. I don't think it's that interesting though. Checking the devicetree
> succeeded while no channels were found. I think returning 0 is very much
> aligned with this.

Right, but as I suggested, let's follow already established APIs that return
-ENOENT and never 0 in similar cases.

> > Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success?
> 
> I suppose you're right.
> 
> But, as you pointed out in review of the 05/10:
> > 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.
> 
> I don't know whether to agree with you here. For majority of the ADC
> drivers, having no channels in devicetree is indeed just another error,
> which I think is not in any ways special.

So...? (I see below your answer :-)

> However, for 33,3333% of the users added in this patch, the "no channels
> found" is not really an error condition ;) The BD79124 could have all
> channels used for GPO - although this would probably be very very unusual.
> (Why buying an ADC chip if you need just a GPO?). Still, this wouldn't be an
> error. (And I need to handle this better in BD79124 probe - so thanks).

ENOENT check is again established for optional/not_found cases.

> > (Under success I assume that returned values are okay to go with, and cs in
> > your case will be left uninitialised or contain something we don't control.
> 
> I see your point although I wouldn't be concerned with cs not being NULL for
> as long as number of channels is zero.
> 
> Anyway, I think it makes sense to simplify ~67% of callers by returning
> -ENODEV if there is no channels. The remaining ~33% can then check for the
> -ENODEV and handle it separately from other returned errors. So, thanks.

Not at all!

-- 
With Best Regards,
Andy Shevchenko