On Thu, 25 Apr 2024 09:09:59 -0500
David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> This adds support for the chain mode of the AD7944 ADC. This mode allows
> multiple ADCs to be daisy-chained together. Data from all of the ADCs in
> is read by reading multiple words from the first ADC in the chain.
>
> Each chip in the chain adds an extra IIO input voltage channel to the
> IIO device.
>
> Only the wiring configuration where the SPI controller CS line is
> connected to the CNV pin of all of the ADCs in the chain is supported
> in this patch.
>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
Looks good except for one minor tweak needed to ensure the allocated buffer
is zeroed as we don't necessarily overwrite the the whole thing.
Given that's all I found, I've just switched that to devm_kzalloc and
applied the series.
Applied to the togreg branch of iio.git and pushed out initially as testing
for 0-day to look at it.
Thanks,
Jonathan
>
> +/**
> + * ad7944_chain_mode_alloc - allocate and initialize channel specs and buffers
> + * for daisy-chained devices
> + * @dev: The device for devm_ functions
> + * @chan_template: The channel template for the devices (array of 2 channels
> + * voltage and timestamp)
> + * @n_chain_dev: The number of devices in the chain
> + * @chain_chan: Pointer to receive the allocated channel specs
> + * @chain_mode_buf: Pointer to receive the allocated rx buffer
> + * @chain_scan_masks: Pointer to receive the allocated scan masks
> + * Return: 0 on success, a negative error code on failure
> + */
> +static int ad7944_chain_mode_alloc(struct device *dev,
> + const struct iio_chan_spec *chan_template,
> + u32 n_chain_dev,
> + struct iio_chan_spec **chain_chan,
> + void **chain_mode_buf,
> + unsigned long **chain_scan_masks)
> +{
> + struct iio_chan_spec *chan;
> + size_t chain_mode_buf_size;
> + unsigned long *scan_masks;
> + void *buf;
> + int i;
> +
> + /* 1 channel for each device in chain plus 1 for soft timestamp */
> +
> + chan = devm_kcalloc(dev, n_chain_dev + 1, sizeof(*chan), GFP_KERNEL);
> + if (!chan)
> + return -ENOMEM;
> +
> + for (i = 0; i < n_chain_dev; i++) {
> + chan[i] = chan_template[0];
> +
> + if (chan_template[0].differential) {
> + chan[i].channel = 2 * i;
> + chan[i].channel2 = 2 * i + 1;
> + } else {
> + chan[i].channel = i;
> + }
> +
> + chan[i].scan_index = i;
> + }
> +
> + /* soft timestamp */
> + chan[i] = chan_template[1];
> + chan[i].scan_index = i;
> +
> + *chain_chan = chan;
> +
> + /* 1 word for each voltage channel + aligned u64 for timestamp */
> +
> + chain_mode_buf_size = ALIGN(n_chain_dev *
> + BITS_TO_BYTES(chan[0].scan_type.storagebits), sizeof(u64))
> + + sizeof(u64);
> + buf = devm_kmalloc(dev, chain_mode_buf_size, GFP_KERNEL);
Zero it - It's not a problem to leak stale ADC data or similar
into the gap between the data and the timestamp, but it is a problem
if it's general kernel data potentially leaking.
So play it safe and devm_kzalloc()
> + if (!buf)
> + return -ENOMEM;
> +
> + *chain_mode_buf = buf;
> +
> + /*
> + * Have to limit n_chain_dev due to current implementation of
> + * available_scan_masks.
> + */
> + if (n_chain_dev > BITS_PER_LONG)
> + return dev_err_probe(dev, -EINVAL,
> + "chain is limited to 32 devices\n");
> +
> + scan_masks = devm_kcalloc(dev, 2, sizeof(*scan_masks), GFP_KERNEL);
> + if (!scan_masks)
> + return -ENOMEM;
> +
> + /*
> + * Scan mask is needed since we always have to read all devices in the
> + * chain in one SPI transfer.
> + */
> + scan_masks[0] = GENMASK(n_chain_dev - 1, 0);
> +
> + *chain_scan_masks = scan_masks;
> +
> + return 0;
> +}
