Message-ID: <bca95d63-fb6e-4d6c-8ab6-df67f0e697e6@gmail.com>
Date: Thu, 13 Mar 2025 15:17:27 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
 Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
 Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>,
 Hugo Villeneuve <hvilleneuve@...onoff.com>, Nuno Sa <nuno.sa@...log.com>,
 David Lechner <dlechner@...libre.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.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>,
 Ramona Alexandra Nechita <ramona.nechita@...log.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
 linux-renesas-soc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v7 03/10] iio: adc: add helpers for parsing ADC nodes

On 13/03/2025 14:31, Andy Shevchenko wrote:
> On Thu, Mar 13, 2025 at 09:18:18AM +0200, Matti Vaittinen 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.
> 
> ...
> 
>> +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;
> 
> Unneeded assignment.

Hmm. I have a deja-vu. Thanks for the reminder.

> 
>> +	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.

> 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.

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).

> (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.

>> +	chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array),
>> +				  GFP_KERNEL);
>> +	if (!chan_array)
>> +		return -ENOMEM;
>> +
>> +	chan = &chan_array[0];
>> +
>> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
>> +		u32 ch;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &ch);
>> +		if (ret)
>> +			return ret;
> 
>> +		if (max_chan_id != -1 && ch > max_chan_id)
>> +			return -ERANGE;
> 
> Hmm... What if max_chan_id is equal to an error code?
> Or in other words, why -1 is special and not all negative numbers?

-1 was just picked to represent a 'don't care' value. Old habit. In the 
old days I handled lots of code where -1 was defined as 'invalid' for 
APIs with unsigned ints as well. It works nicely on systems where it 
turns out to be maximum positive value - leaving most of the number 
space for valid values.

I suppose saying any negative means "don't care" works well here. And, 
dropping all negatives here will also make the check below just work 
with unsigned comparison.

> Also note, you used unsigned type and compare it to int which,
> in case of being negative will give promotion.

Right. I didn't thin negative IDs would make sense and trusted users to 
pass only positive ones. Treating all negatives as "don't care" is 
indeed better than trusting this.

Thanks.

> The ch will not be
> big enough in most cases (unless it's great than (INT_MAX + 1).
> 
> TL;DR: you have a potential integer overflow here.
> 
>> +		*chan = *template;
>> +		chan->channel = ch;
>> +		chan++;
>> +	}
>> +
>> +	*cs = chan_array;
>> +
>> +	return num_chan;
>> +}
> 

Powered by blists - more mailing lists