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;
+}
