| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <tc4od3jtqnj743naxefx5lxkha46wohuuvw46mik6nullvsqbe@knj4t23eaodw>
Date: Thu, 7 Aug 2025 13:41:31 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec
On Thu, Aug 07, 2025 at 12:34:52PM +0300, Matti Vaittinen wrote:
> The ad7476 driver defines separate chan_spec structures for operation
> with and without convstart GPIO. At quick glance this may seem as if the
> driver did provide more than 1 data-channel to users - one for the
> regular data, other for the data obtained with the convstart GPIO.
>
> The only difference between the 'convstart' and 'non convstart'
> -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in
> channel's flags.
>
> We can drop the convstart channel spec, and related convstart macro, by
> allocating a mutable per driver instance channel spec an adding the flag
> in probe if needed. This will simplify the driver with the cost of added
> memory consumption.
>
> Assuming there aren't systems with very many ADCs and very few
> resources, this tradeoff seems worth making.
>
> Simplify the driver by dropping the 'convstart' channel spec and
> allocating the chan spec for each driver instance.
I do not agree with this one. Looking at the diff, code does not look
simpler to me...
- Nuno Sá
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>
> ---
> Revision history:
> v1 => v2:
> - New patch
>
> I considered squashing this change with the one limiting the chip_info
> scope. Having this as a separate change should help reverting if someone
> complains about the increased memory consumption though.
> ---
> drivers/iio/adc/ad7476.c | 31 ++++++++++++++++++-------------
> 1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> index e97742912b8e..a30eb016c11c 100644
> --- a/drivers/iio/adc/ad7476.c
> +++ b/drivers/iio/adc/ad7476.c
> @@ -29,8 +29,6 @@ struct ad7476_state;
> struct ad7476_chip_info {
> unsigned int int_vref_mv;
> struct iio_chan_spec channel[2];
> - /* channels used when convst gpio is defined */
> - struct iio_chan_spec convst_channel[2];
> void (*reset)(struct ad7476_state *);
> bool has_vref;
> bool has_vdrive;
> @@ -41,6 +39,7 @@ struct ad7476_state {
> struct gpio_desc *convst_gpio;
> struct spi_transfer xfer;
> struct spi_message msg;
> + struct iio_chan_spec channel[2];
> int scale_mv;
> /*
> * DMA (thus cache coherency maintenance) may require the
> @@ -153,24 +152,18 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
> #define AD7940_CHAN(bits) _AD7476_CHAN((bits), 15 - (bits), \
> BIT(IIO_CHAN_INFO_RAW))
> #define AD7091R_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), 0)
> -#define AD7091R_CONVST_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), \
> - BIT(IIO_CHAN_INFO_RAW))
> #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
> BIT(IIO_CHAN_INFO_RAW))
>
> static const struct ad7476_chip_info ad7091_chip_info = {
> .channel[0] = AD7091R_CHAN(12),
> .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> - .convst_channel[0] = AD7091R_CONVST_CHAN(12),
> - .convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> .reset = ad7091_reset,
> };
>
> static const struct ad7476_chip_info ad7091r_chip_info = {
> .channel[0] = AD7091R_CHAN(12),
> .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> - .convst_channel[0] = AD7091R_CONVST_CHAN(12),
> - .convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> .int_vref_mv = 2500,
> .has_vref = true,
> .reset = ad7091_reset,
> @@ -282,7 +275,7 @@ static int ad7476_probe(struct spi_device *spi)
> const struct ad7476_chip_info *chip_info;
> struct ad7476_state *st;
> struct iio_dev *indio_dev;
> - int ret;
> + int ret, i;
>
> indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> if (!indio_dev)
> @@ -332,16 +325,28 @@ static int ad7476_probe(struct spi_device *spi)
> if (IS_ERR(st->convst_gpio))
> return PTR_ERR(st->convst_gpio);
>
> + /*
> + * This will never realize. Unless someone changes the channel specs
> + * in this driver. And if someone does, without changing the loop
> + * below, then we'd better immediately produce a big fat error, before
> + * the change proceeds from that developer's table.
> + */
> + BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));
> + for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
> + st->channel[i] = chip_info->channel[i];
> + if (st->convst_gpio)
> + st->channel[i].info_mask_separate |=
> + BIT(IIO_CHAN_INFO_RAW);
> + }
> +
> st->spi = spi;
>
> indio_dev->name = spi_get_device_id(spi)->name;
> indio_dev->modes = INDIO_DIRECT_MODE;
> - indio_dev->channels = chip_info->channel;
> - indio_dev->num_channels = 2;
> + indio_dev->channels = st->channel;
> + indio_dev->num_channels = ARRAY_SIZE(st->channel);
> indio_dev->info = &ad7476_info;
>
> - if (st->convst_gpio)
> - indio_dev->channels = chip_info->convst_channel;
> /* Setup default message */
>
> st->xfer.rx_buf = &st->data;
> --
> 2.50.1
>
Powered by blists - more mailing lists