On Tue, 30 Apr 2024 19:29:46 +0300
Alisa-Dariana Roman <alisadariana@xxxxxxxxx> wrote:
> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
>
> Also modify config AD7192 description for better scaling.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@xxxxxxxxxx>
Hi Alisa-Dariana,
I only had one minor thing to add to the other reviews,
Thanks,
Jonathan
> To compile this driver as a module, choose M here: the
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 3e797ff48086..0f6ecf953559 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
...
> +static int ad7194_parse_channels(struct iio_dev *indio_dev)
> +{
> + struct device *dev = indio_dev->dev.parent;
> + struct iio_chan_spec *ad7194_channels;
> + struct fwnode_handle *child;
> + struct iio_chan_spec ad7194_chan = AD7193_CHANNEL(0, 0, 0);
I think you are only using these as templates that get copied?
If so declare them const.
> + struct iio_chan_spec ad7194_chan_diff = AD7193_DIFF_CHANNEL(0, 0, 0, 0);
> + struct iio_chan_spec ad7194_chan_temp = AD719x_TEMP_CHANNEL(0, 0);
> + struct iio_chan_spec ad7194_chan_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
> + unsigned int num_channels, index = 0;
> + u32 ain[2];
> + int ret;
> +
> + num_channels = device_get_child_node_count(dev);
> + if (num_channels > AD7194_CH_MAX_NR)
> + return dev_err_probe(dev, -EINVAL,
> + "Too many channels: %u\n", num_channels);
> +
> + num_channels += AD7194_CH_BASE_NR;
> +
> + ad7194_channels = devm_kcalloc(dev, num_channels,
> + sizeof(*ad7194_channels), GFP_KERNEL);
> + if (!ad7194_channels)
> + return -ENOMEM;
> +
> + indio_dev->channels = ad7194_channels;
> + indio_dev->num_channels = num_channels;
> +
> + device_for_each_child_node(dev, child) {
Andy pointed out the need for scoped here to avoid leaking the fwnode in error paths.
> + ret = fwnode_property_read_u32_array(child, "diff-channels",
> + ain, ARRAY_SIZE(ain));
> + if (ret == 0) {
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)
> + return ret;
> +
> + ret = ad7194_validate_ain_channel(dev, ain[1]);
> + if (ret)
> + return ret;
> +
> + *ad7194_channels = ad7194_chan_diff;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->channel2 = ain[1];
> + ad7194_channels->address = AD7194_CH(ain[0], ain[1]);
> + } else {
> + ret = fwnode_property_read_u32(child, "single-channel",
> + &ain[0]);
> + if (ret) {
> + fwnode_handle_put(child);
With scoped variant above you can drop this one case of (currently correct)
release of the fwnode.
> + return ret;
> + }
> +
> + ret = ad7194_validate_ain_channel(dev, ain[0]);
> + if (ret)
> + return ret;
> +
> + *ad7194_channels = ad7194_chan;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->channel = ain[0];
> + ad7194_channels->address = AD7194_CH(ain[0], 0);
> + }
> + ad7194_channels++;
> + }
> +
> + *ad7194_channels = ad7194_chan_temp;
> + ad7194_channels->scan_index = index++;
> + ad7194_channels->address = AD7194_CH_TEMP;
> + ad7194_channels++;
> +
> + *ad7194_channels = ad7194_chan_timestamp;
> + ad7194_channels->scan_index = index;
> +
> + return 0;
> +}
