Re: [PATCH v2 4/4] iio: adc: ad7405: add ad7405 driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 8 May 2025 15:30:57 +0300
Pop Ioan Daniel <pop.ioan-daniel@analog.com> wrote:

> Add support for the AD7405/ADUM770x, a high performance isolated ADC,
> 1-channel, 16-bit with a second-order Σ-Δ modulator that converts an
> analog input signal into a high speed, single-bit data stream.
> 
> Signed-off-by: Pop Ioan Daniel <pop.ioan-daniel@analog.com>
Hi,

Just a few comments inline.  In particular I'd avoid the gcc specific
behaviour unless we have it documented somewhere in the kernel that
static flexible array instantiation is allowed.

Doesn't make sense here anyway just make it 1 element.

Jonathan

> diff --git a/drivers/iio/adc/ad7405.c b/drivers/iio/adc/ad7405.c
> new file mode 100644
> index 000000000000..5fe36ce61819
> --- /dev/null
> +++ b/drivers/iio/adc/ad7405.c
> @@ -0,0 +1,264 @@

> +
> +struct ad7405_chip_info {
> +	const char *name;
> +	unsigned int max_rate;
> +	struct iio_chan_spec channel[];
See below.

> +};

> +static int ad7405_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long info)
> +{
> +	struct ad7405_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +			*vals = st->sample_frequency_tbl;
1 tab only.
> +			*length = ARRAY_SIZE(st->sample_frequency_tbl);
> +			*type = IIO_VAL_INT;
> +			return IIO_AVAIL_LIST;
> +	default:
> +			return -EINVAL;
> +	}
> +}

> +
> +static const struct ad7405_chip_info ad7405_chip_info = {
> +		.name = "AD7405",
> +		.channel = {
> +			AD7405_IIO_CHANNEL,
> +		},
> +};
> +
> +static const struct ad7405_chip_info adum7701_chip_info = {
> +		.name = "ADUM7701",

Convention here is 1 tab indent only.

> +		.channel = {
This will look nicer without the array of 1 thing going on.

Interesting corner of the c spec to use a flexible array member
for this. Definitely don't do that as I hate reading that spec to
check corners like this.  On this occasion I looked and could
not find an answer.  There is a gcc extension that makes this work
though.

> +			AD7405_IIO_CHANNEL,
> +		},
> +};