| Message ID | 20241122113322.242875-13-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: adc: ad7124: Various fixes | expand |
On 11/22/24 5:33 AM, Uwe Kleine-König wrote: > The ad7124-4 and ad7124-8 both support 16 channel registers and assigns > each channel defined in dt statically such a register. While the driver > could be a bit more clever about this, it currently isn't and specifying > more than 16 channels yields broken behaviour. So just refuse to bind in > this situation. The ad7124-4 datasheet I am looking at says that it only has registers CONFIG_0 to CONFIG_7, so do we need to limit those chips to 8 channels? > > Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > --- > drivers/iio/adc/ad7124.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c > index 8d94bc2b1cac..5352b26bb391 100644 > --- a/drivers/iio/adc/ad7124.c > +++ b/drivers/iio/adc/ad7124.c > @@ -821,6 +821,16 @@ 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"); > > + /* > + * The driver assigns each logical channel defined in the device tree > + * statically one channel register. So only accept 16 such logical > + * channels to not treat CONFIG_0 (i.e. the register following > + * CHANNEL_15) as an additional channel register. The driver could be > + * improved to lift this limitation. > + */ > + if (st->num_channels > AD7124_MAX_CHANNELS) > + return dev_err_probe(dev, -EINVAL, "Too many channels defined\n"); > + > chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels, > sizeof(*chan), GFP_KERNEL); > if (!chan)
On Fri, Nov 22, 2024 at 09:14:18AM -0600, David Lechner wrote: > On 11/22/24 5:33 AM, Uwe Kleine-König wrote: > > The ad7124-4 and ad7124-8 both support 16 channel registers and assigns > > each channel defined in dt statically such a register. While the driver > > could be a bit more clever about this, it currently isn't and specifying > > more than 16 channels yields broken behaviour. So just refuse to bind in > > this situation. > > The ad7124-4 datasheet I am looking at says that it only has registers > CONFIG_0 to CONFIG_7, so do we need to limit those chips to 8 channels? These could be reused for different channels if the settings match. I'm unsure what happens if the 16 channels use more than 8 different configs and you want to bulk read them. Single channel use should work fine I think. If that is a problem I might have to extend this series of fixes, but this is something orthogonal to this patch I think. Best regards Uwe
On 11/22/24 9:54 AM, Uwe Kleine-König wrote: > On Fri, Nov 22, 2024 at 09:14:18AM -0600, David Lechner wrote: >> On 11/22/24 5:33 AM, Uwe Kleine-König wrote: >>> The ad7124-4 and ad7124-8 both support 16 channel registers and assigns >>> each channel defined in dt statically such a register. While the driver >>> could be a bit more clever about this, it currently isn't and specifying >>> more than 16 channels yields broken behaviour. So just refuse to bind in >>> this situation. >> >> The ad7124-4 datasheet I am looking at says that it only has registers >> CONFIG_0 to CONFIG_7, so do we need to limit those chips to 8 channels? > > These could be reused for different channels if the settings match. I'm > unsure what happens if the 16 channels use more than 8 different > configs and you want to bulk read them. Single channel use should work > fine I think. If that is a problem I might have to extend this series of > fixes, but this is something orthogonal to this patch I think. > > Best regards > Uwe Oh, oops, you are right. I was mixing up _channel_ registers and _config_ registers.
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c index 8d94bc2b1cac..5352b26bb391 100644 --- a/drivers/iio/adc/ad7124.c +++ b/drivers/iio/adc/ad7124.c @@ -821,6 +821,16 @@ 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"); + /* + * The driver assigns each logical channel defined in the device tree + * statically one channel register. So only accept 16 such logical + * channels to not treat CONFIG_0 (i.e. the register following + * CHANNEL_15) as an additional channel register. The driver could be + * improved to lift this limitation. + */ + if (st->num_channels > AD7124_MAX_CHANNELS) + return dev_err_probe(dev, -EINVAL, "Too many channels defined\n"); + chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels, sizeof(*chan), GFP_KERNEL); if (!chan)
The ad7124-4 and ad7124-8 both support 16 channel registers and assigns each channel defined in dt statically such a register. While the driver could be a bit more clever about this, it currently isn't and specifying more than 16 channels yields broken behaviour. So just refuse to bind in this situation. Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/iio/adc/ad7124.c | 10 ++++++++++ 1 file changed, 10 insertions(+)