| 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: <20250315190238.65e9a59d@jic23-huawei>
Date: Sat, 15 Mar 2025 19:02:38 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: linux-iio@...r.kernel.org, Michael Hennerich
<Michael.Hennerich@...log.com>, Angelo Dureghello
<adureghello@...libre.com>, Alexandru Ardelean <aardelean@...libre.com>,
Beniamin Bia <beniamin.bia@...log.com>, Stefan Popa
<stefan.popa@...log.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/11] iio: adc: ad7606: dynamically allocate channel
info
On Wed, 12 Mar 2025 20:15:48 -0500
David Lechner <dlechner@...libre.com> wrote:
> Refactor the ad7606 drivers to dynamically allocate the channel info.
>
> The channel info was getting a bit unwieldy. In some cases, the
> indio_dev->channels field was getting assigned up to 3 different times,
> each in a different function, making it difficult to see where the info
> was coming from. This problem stemps from the number of permutations of
> the channel array needed to support various modes of operation and data
> buses. We already have 4 per chip (hardware mode, software mode, AXI ADC
> backend and AXI ADC backend with software mode) and we intend to add two
> more per chip when adding SPI offload support.
>
> To make it easier to read and maintain, move all of the channel setup
> to a single function that dynamically allocates and fills in the channel
> info.
>
> Additionally, this lets us remove some hacks where we had to compute an
> offset due to the fact that sometimes there was a soft timestamp channel
> at the start of the array. Now the timestamp channel is always at the
> end of the array as is typical in other drivers.
>
> Signed-off-by: David Lechner <dlechner@...libre.com>
whilst I like static const channel arrays it is hard to argue with the logic
that it got too complex here so fair enough.
A few trivial things inline.
Jonathan
> @@ -1289,29 +1193,84 @@ static int ad7606b_sw_mode_setup(struct iio_dev *indio_dev)
> return st->bops->sw_mode_config(indio_dev);
> }
>
> -static int ad7606_chan_scales_setup(struct iio_dev *indio_dev)
> +static int ad7606_probe_channels(struct iio_dev *indio_dev)
> {
> struct ad7606_state *st = iio_priv(indio_dev);
> - unsigned int offset = indio_dev->num_channels - st->chip_info->num_adc_channels;
> - struct iio_chan_spec *chans;
> - size_t size;
> - int ch, ret;
> -
> - /* Clone IIO channels, since some may be differential */
> - size = indio_dev->num_channels * sizeof(*indio_dev->channels);
> - chans = devm_kzalloc(st->dev, size, GFP_KERNEL);
> - if (!chans)
> + struct device *dev = indio_dev->dev.parent;
> + struct iio_chan_spec *channels;
> + bool slow_bus;
> + int ret, i;
> +
> + slow_bus = !st->bops->iio_backend_config;
Maybe inline with declaration?
bool slow_bus = !st->...
> + indio_dev->num_channels = st->chip_info->num_adc_channels;
> +
> + /* Slow buses also get 1 more channel for soft timestamp */
> + if (slow_bus)
> + indio_dev->num_channels++;
> +
> + channels = devm_kcalloc(dev, indio_dev->num_channels, sizeof(*channels),
> + GFP_KERNEL);
> + if (!channels)
> return -ENOMEM;
>
> - memcpy(chans, indio_dev->channels, size);
> - indio_dev->channels = chans;
> + for (i = 0; i < indio_dev->num_channels; i++) {
> + struct iio_chan_spec *chan = &channels[i];
>
> - for (ch = 0; ch < st->chip_info->num_adc_channels; ch++) {
> - ret = st->chip_info->scale_setup_cb(indio_dev, &chans[ch + offset]);
> + chan->type = IIO_VOLTAGE;
> + chan->indexed = 1;
> + chan->channel = i;
> + chan->scan_index = i;
> + chan->scan_type.sign = 's';
> + chan->scan_type.realbits = st->chip_info->bits;
> + chan->scan_type.storagebits = st->chip_info->bits > 16 ? 32 : 16;
> + chan->scan_type.endianness = IIO_CPU;
> +
> + if (indio_dev->modes & INDIO_DIRECT_MODE)
> + chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> +
> + if (st->sw_mode_en) {
> + chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> + chan->info_mask_separate_available |=
> + BIT(IIO_CHAN_INFO_SCALE);
> +
> + /**
/* probably more appropriate here.
> + * All chips with software mode support oversampling,
> + * so we skip the oversampling_available check. And the
> + * shared_by_type instead of shared_by_all on slow
> + * buses is for backward compatibility.
> + */
> + if (slow_bus)
> + chan->info_mask_shared_by_type |=
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
> + else
> + chan->info_mask_shared_by_all |=
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
> +
> + chan->info_mask_shared_by_all_available |=
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
> + } else {
> + chan->info_mask_shared_by_type |=
> + BIT(IIO_CHAN_INFO_SCALE);
> +
> + if (st->chip_info->oversampling_avail)
> + chan->info_mask_shared_by_all |=
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
> + }
> +
> + if (!slow_bus)
> + chan->info_mask_shared_by_all |=
> + BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +
> + ret = st->chip_info->scale_setup_cb(indio_dev, chan);
> if (ret)
> return ret;
> }
>
> + if (slow_bus)
> + channels[i] = (struct iio_chan_spec)IIO_CHAN_SOFT_TIMESTAMP(i);
> +
> + indio_dev->channels = channels;
> +
> return 0;
> }
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index fd4c0d615a880ac6fdcaad213d4843329c3bd7fe..b67058cd021a3d00ff0f461766d51e46d7998f32 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> typedef int (*ad7606_scale_setup_cb_t)(struct iio_dev *indio_dev,
> @@ -118,10 +48,9 @@ typedef int (*ad7606_sw_setup_cb_t)(struct iio_dev *indio_dev);
>
> /**
> * struct ad7606_chip_info - chip specific information
> - * @channels: channel specification
> * @max_samplerate: maximum supported sample rate
> * @name: device name
> - * @num_channels: number of channels
> + * @bits: data width in bits
Hmm. Was previously in the wrong order. Maybe fix that in your
earlier cleanup then this will look more natural.
> * @num_adc_channels: the number of channels the ADC actually inputs.
> * @scale_setup_cb: callback to setup the scales for each channel
> * @sw_setup_cb: callback to setup the software mode if available.
> @@ -133,11 +62,10 @@ typedef int (*ad7606_sw_setup_cb_t)(struct iio_dev *indio_dev);
> * after a restart
> */
> struct ad7606_chip_info {
> - const struct iio_chan_spec *channels;
> unsigned int max_samplerate;
> const char *name;
> + unsigned int bits;
> unsigned int num_adc_channels;
> - unsigned int num_channels;
> ad7606_scale_setup_cb_t scale_setup_cb;
> ad7606_sw_setup_cb_t sw_setup_cb;
> const unsigned int *oversampling_avail;
Powered by blists - more mailing lists