[PATCH 0/2] iio: adc: ad7124: Implement input validation

[PATCH 0/2] iio: adc: ad7124: Implement input validation

[Uwe Kleine-König]

Hello,

here come two commits that add some input validation of the number of
channels and the diff-inputs property values. The first limits the
number of iio channels to 16 which is the number of channel registers
for both hardware variants and so is the maximal number of (logical)
channels. The second commit refuses invalid channel numbers, that is
8-15 for ad7124-4 and values bigger than 31. The initial driver refused
all input specifiers > 15, but this limitation was lifted by commit
f1794fd7bdf7 ("iio: adc: ad7124: Remove input number limitation") with
the intention to allow values 16-31 but dropped all checks.

Note that at least some of the input specifiers in the 16-31 range are
different and using these yields bogus properties. E.g. 16 and 17 are
for temperature measurement, while using these in the device tree
results in a voltage property and the respective scale and offset
properties are bogus. Still these were explicitly allowed in
f1794fd7bdf7, so I didn't refuse these.

These patches don't conflict with the other ad7124 patches I sent
before, at least git can apply the series in both possible orders.

Best regards
Uwe

Uwe Kleine-König (2):
  iio: adc: ad7124: Don't create more channels than the hardware is
    capable of
  iio: adc: ad7124: Refuse invalid input specifiers

 drivers/iio/adc/ad7124.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
-- 
2.45.2

[PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

[Uwe Kleine-König]

The ad7124-4 and ad7124-8 both support 16 channel registers. Don't
accept more (logical) channels from dt than that.

Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index a5d91933f505..7473bcef7bc6 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -18,6 +18,7 @@
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/stringify.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/adc/ad_sigma_delta.h>
@@ -821,6 +822,11 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	if (!st->num_channels)
 		return dev_err_probe(dev, -ENODEV, "no channel children\n");
 
+	if (st->num_channels > AD7124_MAX_CHANNELS) {
+		dev_warn(dev, "Limit number of channels to " __stringify(AD7124_MAX_CHANNELS) "\n");
+		st->num_channels = AD7124_MAX_CHANNELS;
+	}
+
 	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
 			    sizeof(*chan), GFP_KERNEL);
 	if (!chan)
-- 
2.45.2

[PATCH 2/2] iio: adc: ad7124: Refuse invalid input specifiers

[Uwe Kleine-König]

The ad7124-4 has 8 analog inputs; the input select values 8 to 15 are
reserved and not to be used. These are fine for ad7124-8. For both
ad7124-4 and ad7124-8 values bigger than 15 are internal channels that
might appear as inputs in the channels specified in the device
description according to the description of commit f1794fd7bdf7 ("iio:
adc: ad7124: Remove input number limitation"), values bigger than 31
don't fit into the respective register bit field and the driver masked
them to smaller values.

Check for these invalid input specifiers and fail to probe if one is
found.

Fixes: f1794fd7bdf7 ("iio: adc: ad7124: Remove input number limitation")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibe.com>
---
 drivers/iio/adc/ad7124.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 7473bcef7bc6..bd5c0dbc0239 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -808,6 +808,19 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
 	return 0;
 }
 
+/*
+ * Input specifiers 8 - 15 are explicitly reserved for ad7124-4
+ * while they are fine for ad7124-8. Values above 31 don't fit
+ * into the register field and so are invalid for sure.
+ */
+static bool ad7124_valid_input_select(unsigned int ain, const struct ad7124_chip_info *info)
+{
+	if (ain >= info->num_inputs && ain < 16)
+		return false;
+
+	return ain <= FIELD_MAX(AD7124_CHANNEL_AINM_MSK);
+}
+
 static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 				       struct device *dev)
 {
@@ -855,6 +868,11 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
+		if (!ad7124_valid_input_select(ain[0], st->chip_info) ||
+		    !ad7124_valid_input_select(ain[1], st->chip_info))
+			return dev_err_probe(dev, ret,
+					     "diff-channels property of %pfwP contains invalid data\n", child);
+
 		st->channels[channel].nr = channel;
 		st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
 						  AD7124_CHANNEL_AINM(ain[1]);
-- 
2.45.2

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

[David Lechner]

On 11/8/24 12:18 PM, Uwe Kleine-König wrote:
> The ad7124-4 and ad7124-8 both support 16 channel registers. Don't
> accept more (logical) channels from dt than that.

Why should the devicetree be limited by the number of channel
registers? Channel registers are a resource than can be
dynamically assigned, so it doesn't seem like the devicetree
should be specifying that assignment.

It's true we can't do a buffered read of more than 8 or 16
channels at the same time because it is limited by the number
of channel registers available on the chip.

But it seems reasonable that if there are more logical channels
than that, we could read 8 logical channels, then disable those
and enable a different 8 logical channels and read those by
reconfiguring the channel registers.

Re: [PATCH 2/2] iio: adc: ad7124: Refuse invalid input specifiers

[Dan Carpenter]

Hi Uwe,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Uwe-Kleine-K-nig/iio-adc-ad7124-Don-t-create-more-channels-than-the-hardware-is-capable-of/20241109-022036
base:   9852d85ec9d492ebef56dc5f229416c925758edc
patch link:    https://lore.kernel.org/r/20241108181813.272593-6-u.kleine-koenig%40baylibre.com
patch subject: [PATCH 2/2] iio: adc: ad7124: Refuse invalid input specifiers
config: i386-randconfig-141-20241109 (https://download.01.org/0day-ci/archive/20241109/202411090908.Ynrg4eS0-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202411090908.Ynrg4eS0-lkp@intel.com/

smatch warnings:
drivers/iio/adc/ad7124.c:874 ad7124_parse_channel_config() warn: passing zero to 'dev_err_probe'

vim +/dev_err_probe +874 drivers/iio/adc/ad7124.c

a6eaf02b82744b Jonathan Cameron  2024-02-18  824  static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
a6eaf02b82744b Jonathan Cameron  2024-02-18  825  				       struct device *dev)
b3af341bbd9662 Stefan Popa       2018-11-13  826  {
b3af341bbd9662 Stefan Popa       2018-11-13  827  	struct ad7124_state *st = iio_priv(indio_dev);
7b8d045e497a04 Alexandru Tachici 2021-03-11  828  	struct ad7124_channel_config *cfg;
7b8d045e497a04 Alexandru Tachici 2021-03-11  829  	struct ad7124_channel *channels;
b3af341bbd9662 Stefan Popa       2018-11-13  830  	struct iio_chan_spec *chan;
b3af341bbd9662 Stefan Popa       2018-11-13  831  	unsigned int ain[2], channel = 0, tmp;
b3af341bbd9662 Stefan Popa       2018-11-13  832  	int ret;
b3af341bbd9662 Stefan Popa       2018-11-13  833  
a6eaf02b82744b Jonathan Cameron  2024-02-18  834  	st->num_channels = device_get_child_node_count(dev);
a6eaf02b82744b Jonathan Cameron  2024-02-18  835  	if (!st->num_channels)
a6eaf02b82744b Jonathan Cameron  2024-02-18  836  		return dev_err_probe(dev, -ENODEV, "no channel children\n");
b3af341bbd9662 Stefan Popa       2018-11-13  837  
b478bd5e22404e Uwe Kleine-König  2024-11-08  838  	if (st->num_channels > AD7124_MAX_CHANNELS) {
b478bd5e22404e Uwe Kleine-König  2024-11-08  839  		dev_warn(dev, "Limit number of channels to " __stringify(AD7124_MAX_CHANNELS) "\n");
b478bd5e22404e Uwe Kleine-König  2024-11-08  840  		st->num_channels = AD7124_MAX_CHANNELS;
b478bd5e22404e Uwe Kleine-König  2024-11-08  841  	}
b478bd5e22404e Uwe Kleine-König  2024-11-08  842  
b3af341bbd9662 Stefan Popa       2018-11-13  843  	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
b3af341bbd9662 Stefan Popa       2018-11-13  844  			    sizeof(*chan), GFP_KERNEL);
b3af341bbd9662 Stefan Popa       2018-11-13  845  	if (!chan)
b3af341bbd9662 Stefan Popa       2018-11-13  846  		return -ENOMEM;
b3af341bbd9662 Stefan Popa       2018-11-13  847  
7b8d045e497a04 Alexandru Tachici 2021-03-11  848  	channels = devm_kcalloc(indio_dev->dev.parent, st->num_channels, sizeof(*channels),
7b8d045e497a04 Alexandru Tachici 2021-03-11  849  				GFP_KERNEL);
7b8d045e497a04 Alexandru Tachici 2021-03-11  850  	if (!channels)
1478a388f4baaa Mircea Caprioru   2019-06-25  851  		return -ENOMEM;
1478a388f4baaa Mircea Caprioru   2019-06-25  852  
b3af341bbd9662 Stefan Popa       2018-11-13  853  	indio_dev->channels = chan;
b3af341bbd9662 Stefan Popa       2018-11-13  854  	indio_dev->num_channels = st->num_channels;
7b8d045e497a04 Alexandru Tachici 2021-03-11  855  	st->channels = channels;
b3af341bbd9662 Stefan Popa       2018-11-13  856  
a6eaf02b82744b Jonathan Cameron  2024-02-18  857  	device_for_each_child_node_scoped(dev, child) {
a6eaf02b82744b Jonathan Cameron  2024-02-18  858  		ret = fwnode_property_read_u32(child, "reg", &channel);
b3af341bbd9662 Stefan Popa       2018-11-13  859  		if (ret)
a6eaf02b82744b Jonathan Cameron  2024-02-18  860  			return ret;
b3af341bbd9662 Stefan Popa       2018-11-13  861  
a6eaf02b82744b Jonathan Cameron  2024-02-18  862  		if (channel >= indio_dev->num_channels)
a6eaf02b82744b Jonathan Cameron  2024-02-18  863  			return dev_err_probe(dev, -EINVAL,
f2a772c51206b0 Jonathan Cameron  2021-05-13  864  				"Channel index >= number of channels\n");
f2a772c51206b0 Jonathan Cameron  2021-05-13  865  
a6eaf02b82744b Jonathan Cameron  2024-02-18  866  		ret = fwnode_property_read_u32_array(child, "diff-channels",
b3af341bbd9662 Stefan Popa       2018-11-13  867  						     ain, 2);
b3af341bbd9662 Stefan Popa       2018-11-13  868  		if (ret)
a6eaf02b82744b Jonathan Cameron  2024-02-18  869  			return ret;
b3af341bbd9662 Stefan Popa       2018-11-13  870  
4112b30ba58b5c Uwe Kleine-König  2024-11-08  871  		if (!ad7124_valid_input_select(ain[0], st->chip_info) ||
4112b30ba58b5c Uwe Kleine-König  2024-11-08  872  		    !ad7124_valid_input_select(ain[1], st->chip_info))
4112b30ba58b5c Uwe Kleine-König  2024-11-08  873  			return dev_err_probe(dev, ret,

s/ret/-EINVAL/?

4112b30ba58b5c Uwe Kleine-König  2024-11-08 @874  					     "diff-channels property of %pfwP contains invalid data\n", child);
4112b30ba58b5c Uwe Kleine-König  2024-11-08  875  
7b8d045e497a04 Alexandru Tachici 2021-03-11  876  		st->channels[channel].nr = channel;
7b8d045e497a04 Alexandru Tachici 2021-03-11  877  		st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
b3af341bbd9662 Stefan Popa       2018-11-13  878  						  AD7124_CHANNEL_AINM(ain[1]);
7b8d045e497a04 Alexandru Tachici 2021-03-11  879  
61cbfb5368dd50 Dumitru Ceclan    2024-08-06  880  		cfg = &st->channels[channel].cfg;
a6eaf02b82744b Jonathan Cameron  2024-02-18  881  		cfg->bipolar = fwnode_property_read_bool(child, "bipolar");
b3af341bbd9662 Stefan Popa       2018-11-13  882  
a6eaf02b82744b Jonathan Cameron  2024-02-18  883  		ret = fwnode_property_read_u32(child, "adi,reference-select", &tmp);
b3af341bbd9662 Stefan Popa       2018-11-13  884  		if (ret)
7b8d045e497a04 Alexandru Tachici 2021-03-11  885  			cfg->refsel = AD7124_INT_REF;
b3af341bbd9662 Stefan Popa       2018-11-13  886  		else
7b8d045e497a04 Alexandru Tachici 2021-03-11  887  			cfg->refsel = tmp;
b3af341bbd9662 Stefan Popa       2018-11-13  888  
a6eaf02b82744b Jonathan Cameron  2024-02-18  889  		cfg->buf_positive =
a6eaf02b82744b Jonathan Cameron  2024-02-18  890  			fwnode_property_read_bool(child, "adi,buffered-positive");
a6eaf02b82744b Jonathan Cameron  2024-02-18  891  		cfg->buf_negative =
a6eaf02b82744b Jonathan Cameron  2024-02-18  892  			fwnode_property_read_bool(child, "adi,buffered-negative");
0eaecea6e4878a Mircea Caprioru   2019-06-25  893  
d7857e4ee1ba69 Alexandru Tachici 2019-12-20  894  		chan[channel] = ad7124_channel_template;
d7857e4ee1ba69 Alexandru Tachici 2019-12-20  895  		chan[channel].address = channel;
d7857e4ee1ba69 Alexandru Tachici 2019-12-20  896  		chan[channel].scan_index = channel;
d7857e4ee1ba69 Alexandru Tachici 2019-12-20  897  		chan[channel].channel = ain[0];
d7857e4ee1ba69 Alexandru Tachici 2019-12-20  898  		chan[channel].channel2 = ain[1];
b3af341bbd9662 Stefan Popa       2018-11-13  899  	}
b3af341bbd9662 Stefan Popa       2018-11-13  900  
b3af341bbd9662 Stefan Popa       2018-11-13  901  	return 0;
b3af341bbd9662 Stefan Popa       2018-11-13  902  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

[Nuno Sá]

On Fri, 2024-11-08 at 19:18 +0100, Uwe Kleine-König wrote:
> The ad7124-4 and ad7124-8 both support 16 channel registers. Don't
> accept more (logical) channels from dt than that.
> 
> Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/iio/adc/ad7124.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index a5d91933f505..7473bcef7bc6 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -18,6 +18,7 @@
>  #include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> +#include <linux/stringify.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/adc/ad_sigma_delta.h>
> @@ -821,6 +822,11 @@ static int ad7124_parse_channel_config(struct iio_dev
> *indio_dev,
>  	if (!st->num_channels)
>  		return dev_err_probe(dev, -ENODEV, "no channel children\n");
>  
> +	if (st->num_channels > AD7124_MAX_CHANNELS) {
> +		dev_warn(dev, "Limit number of channels to "
> __stringify(AD7124_MAX_CHANNELS) "\n");
> +		st->num_channels = AD7124_MAX_CHANNELS;
> +	}

Hmmm, I would treat it as an error...

- Nuno Sá

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

Hello Nuno,

On Mon, Nov 11, 2024 at 11:37:46AM +0100, Nuno Sá wrote:
> On Fri, 2024-11-08 at 19:18 +0100, Uwe Kleine-König wrote:
> > @@ -821,6 +822,11 @@ static int ad7124_parse_channel_config(struct iio_dev
> > *indio_dev,
> >  	if (!st->num_channels)
> >  		return dev_err_probe(dev, -ENODEV, "no channel children\n");
> >  
> > +	if (st->num_channels > AD7124_MAX_CHANNELS) {
> > +		dev_warn(dev, "Limit number of channels to "
> > __stringify(AD7124_MAX_CHANNELS) "\n");
> > +		st->num_channels = AD7124_MAX_CHANNELS;
> > +	}
> 
> Hmmm, I would treat it as an error...

Well, it probably results in an error further below when the first child
is hit that uses a too high reg property. I considered erroring out
here, but thought this might not be justified if some children are not
logical channels. I'm not sure either way, but can rework accordingly if
all other concerns are resolved.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]

[dropped Mircea Caprioru from Cc: as their address bounces.]

Hello David,

On Fri, Nov 08, 2024 at 12:52:35PM -0600, David Lechner wrote:
> On 11/8/24 12:18 PM, Uwe Kleine-König wrote:
> > The ad7124-4 and ad7124-8 both support 16 channel registers. Don't
> > accept more (logical) channels from dt than that.
> 
> Why should the devicetree be limited by the number of channel
> registers? Channel registers are a resource than can be
> dynamically assigned, so it doesn't seem like the devicetree
> should be specifying that assignment.

Note the device tree isn't limited as I didn't adapt the binding. It's
just that the driver doesn't bind if too many channels are specified.
And while your statement about the channels being a dynamic resource is
right, currently the driver doesn't cope and allocates resources
statically, and happily assumes there is a CHANNEL_16 register if the
device tree specifies 17 (or more) logical channels and writes to
CONFIG_0 then which very likely results in strange effects.

So as long as the driver doesn't implement this (possible) dynamic
mapping to the CHANNEL registers, it's IMHO right to refuse to bind (or
alternatively only use the 16 first logical channels).

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] iio: adc: ad7124: Refuse invalid input specifiers

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

[Dropped Mircea Caprioru from Cc: as the analog.com MTA doesn't know
their address]

Hello Dan,

On Mon, Nov 11, 2024 at 12:15:45PM +0300, Dan Carpenter wrote:
> 4112b30ba58b5c Uwe Kleine-König  2024-11-08  871  		if (!ad7124_valid_input_select(ain[0], st->chip_info) ||
> 4112b30ba58b5c Uwe Kleine-König  2024-11-08  872  		    !ad7124_valid_input_select(ain[1], st->chip_info))
> 4112b30ba58b5c Uwe Kleine-König  2024-11-08  873  			return dev_err_probe(dev, ret,
> 
> s/ret/-EINVAL/?
> 
> 4112b30ba58b5c Uwe Kleine-König  2024-11-08 @874  					     "diff-channels property of %pfwP contains invalid data\n", child);

Indeed. I adapted that in my tree, so will be fixed in v2.

Thanks
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

[David Lechner]

On 11/11/24 6:08 AM, Uwe Kleine-König wrote:
> [dropped Mircea Caprioru from Cc: as their address bounces.]
> 
> Hello David,
> 
> On Fri, Nov 08, 2024 at 12:52:35PM -0600, David Lechner wrote:
>> On 11/8/24 12:18 PM, Uwe Kleine-König wrote:
>>> The ad7124-4 and ad7124-8 both support 16 channel registers. Don't
>>> accept more (logical) channels from dt than that.
>>
>> Why should the devicetree be limited by the number of channel
>> registers? Channel registers are a resource than can be
>> dynamically assigned, so it doesn't seem like the devicetree
>> should be specifying that assignment.
> 
> Note the device tree isn't limited as I didn't adapt the binding. It's
> just that the driver doesn't bind if too many channels are specified.
> And while your statement about the channels being a dynamic resource is
> right, currently the driver doesn't cope and allocates resources
> statically, and happily assumes there is a CHANNEL_16 register if the
> device tree specifies 17 (or more) logical channels and writes to
> CONFIG_0 then which very likely results in strange effects.
> 
> So as long as the driver doesn't implement this (possible) dynamic
> mapping to the CHANNEL registers, it's IMHO right to refuse to bind (or
> alternatively only use the 16 first logical channels).
> 
> Best regards
> Uwe

Understood. It would be nice to implement such dynamic allocation
in the future but as a fix to backport to stable kernels, this makes
sense.

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

[Nuno Sá]

On Mon, 2024-11-11 at 12:53 +0100, Uwe Kleine-König wrote:
> Hello Nuno,
> 
> On Mon, Nov 11, 2024 at 11:37:46AM +0100, Nuno Sá wrote:
> > On Fri, 2024-11-08 at 19:18 +0100, Uwe Kleine-König wrote:
> > > @@ -821,6 +822,11 @@ static int ad7124_parse_channel_config(struct iio_dev
> > > *indio_dev,
> > >  	if (!st->num_channels)
> > >  		return dev_err_probe(dev, -ENODEV, "no channel
> > > children\n");
> > >  
> > > +	if (st->num_channels > AD7124_MAX_CHANNELS) {
> > > +		dev_warn(dev, "Limit number of channels to "
> > > __stringify(AD7124_MAX_CHANNELS) "\n");
> > > +		st->num_channels = AD7124_MAX_CHANNELS;
> > > +	}
> > 
> > Hmmm, I would treat it as an error...
> 
> Well, it probably results in an error further below when the first child
> is hit that uses a too high reg property. I considered erroring out

Assuming no one points two different nodes to the same reg :)

> here, but thought this might not be justified if some children are not
> logical channels. I'm not sure either way, but can rework accordingly if
> all other concerns are resolved.
> 

Anyways, I'm not totally sure about the logical channels you and David are
discussing (did not went through it), but FWIW, I agree that we should stop
probe if something is not supported rather than proceed just to get non obvious,
subtle bugs.

- Nuno Sá

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

[Jonathan Cameron]

On Mon, 11 Nov 2024 08:21:45 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 11/11/24 6:08 AM, Uwe Kleine-König wrote:
> > [dropped Mircea Caprioru from Cc: as their address bounces.]
> > 
> > Hello David,
> > 
> > On Fri, Nov 08, 2024 at 12:52:35PM -0600, David Lechner wrote:  
> >> On 11/8/24 12:18 PM, Uwe Kleine-König wrote:  
> >>> The ad7124-4 and ad7124-8 both support 16 channel registers. Don't
> >>> accept more (logical) channels from dt than that.  
> >>
> >> Why should the devicetree be limited by the number of channel
> >> registers? Channel registers are a resource than can be
> >> dynamically assigned, so it doesn't seem like the devicetree
> >> should be specifying that assignment.  
> > 
> > Note the device tree isn't limited as I didn't adapt the binding. It's
> > just that the driver doesn't bind if too many channels are specified.
> > And while your statement about the channels being a dynamic resource is
> > right, currently the driver doesn't cope and allocates resources
> > statically, and happily assumes there is a CHANNEL_16 register if the
> > device tree specifies 17 (or more) logical channels and writes to
> > CONFIG_0 then which very likely results in strange effects.
> > 
> > So as long as the driver doesn't implement this (possible) dynamic
> > mapping to the CHANNEL registers, it's IMHO right to refuse to bind (or
> > alternatively only use the 16 first logical channels).
> > 
> > Best regards
> > Uwe  
> 
> Understood. It would be nice to implement such dynamic allocation
> in the future but as a fix to backport to stable kernels, this makes
> sense.

Agreed.  We do have other drivers that have internal allocators
for constrained sequencer resources, but they are complex so not
fix material!

Jonathan