Message-ID: <20250317104537.3a8819e5@jic23-huawei>
Date: Mon, 17 Mar 2025 10:45:37 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Matti Vaittinen <mazziesaccount@...il.com>, Matti Vaittinen
 <matti.vaittinen@...rohmeurope.com>, Lars-Peter Clausen <lars@...afoo.de>,
 Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>, Nuno Sa <nuno.sa@...log.com>, David
 Lechner <dlechner@...libre.com>, Javier Carrasco
 <javier.carrasco.cruz@...il.com>, Olivier Moysan
 <olivier.moysan@...s.st.com>, Guillaume Stols <gstols@...libre.com>,
 Dumitru Ceclan <mitrutzceclan@...il.com>, Trevor Gamblin
 <tgamblin@...libre.com>, Matteo Martelli <matteomartelli3@...il.com>,
 Alisa-Dariana Roman <alisadariana@...il.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

On Mon, 17 Mar 2025 11:27:37 +0200
Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:

> On Mon, Mar 17, 2025 at 10:42:07AM +0200, Matti Vaittinen wrote:
> > On 17/03/2025 09:51, Andy Shevchenko wrote:  
> > > On Mon, Mar 17, 2025 at 09:11:08AM +0200, Matti Vaittinen wrote:  
> > > > On 16/03/2025 11:41, Jonathan Cameron wrote:  
> > > > > On Thu, 13 Mar 2025 14:34:24 +0200
> > > > > Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:  
> > > > > > On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:  
> 
> ...
> 
> > > > > > > +	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.  
> > > 
> > > ENODEV can be easily clashed with other irrelevant cases,  
> > 
> > Can you please explain what cases?  
> 
> When it's a code path that returns the same error code for something different.
> 
> > > ENOENT is the correct
> > > error code.  
> > 
> > I kind of agree if we look this from the fwnode perspective. But, when we
> > look this from the intended user's perspective, I can very well understand
> > the -ENODEV. Returning -ENODEV from ADC driver's probe which can't find any
> > of the channels feels correct to me.  
> 
> Okay, it seems we have got yet another disagreement as I think this has to
> be ENOENT. Because this is related to the firmware description and not real
> hardware discovery path. If it is the latter, I may fully agree on ENODEV
> choice. But AFAICS it's not the case here.

I'd not rule so strongly but -ENOENT is fine and there should be no fall out from
standardizing around that.

> 
> > > If drivers return this instead of another error code, nothing bad
> > > happen, it's not an ABI path, correct?  
> > 
> > I don't know if failure returned from a probe is an ABI. I still feel
> > -ENODEV is correct value,  
> 
> And I have the opposite opinion. I think ENODEV is _incorrect_ choice
> in this case.
> 
> > and I don't want to change it for existing users -
> > and I think also new ADC drivers should use -ENODEV if they find no channels
> > at all.  
> 
> Again, the problem is that you are trying to apply the error code for HW to the
> information that comes from FW.
> 
> > Besides that I think -ENODEV to be right, changing it to -ENOENT for
> > existing callers requires a buy-in from Jonathan (and/or) the driver
> > maintainers.  
> 
> Yeah, will wait for Jonathan to judge, but I think you can find rationale above.

The distinction between hardware not there (-ENODEV) and missing stuff in 
FW (-ENOENT) seems reasonable to make. If anyone is relying on capturing
a specific error code and doing anything much with it beyond logging
for debug then they are very optimistic as this stuff is often
not particularly stable over kernel versions. That sort of error code specific
handling only makes sense very locally, not in probe() return values.

Hence my preference here is switch to -ENOENT and we'll go with that for
new code in general that hits this sort of problem.   I'd not consider
older code returning -ENODEV buggy as such, just slightly less than
ideal. So any changes for now probably belong in series touching the code
for other reasons rather than a mass cleanup.

Jonathan

Powered by blists - more mailing lists