Re: [PATCH RFC v6 02/10] iio: adc: ad7380: new driver for AD7380 ADCs

[Jonathan Cameron]

On Wed, 01 May 2024 16:55:35 +0200
Julien Stephan <jstephan@xxxxxxxxxxxx> wrote:

> From: David Lechner <dlechner@xxxxxxxxxxxx>
> 
> This adds a new driver for the AD7380 family ADCs.
> 
> The driver currently implements basic support for the AD7380, AD7381,
> 2-channel differential ADCs. Support for additional single-ended,
> pseudo-differential and 4-channel chips that use the same register map
> as well as additional features of the chip will be added in future patches.
> 
> Co-developed-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> [Julien Stephan: add datasheet links of supported parts]
> [Julien Stephan: fix rx/tx buffer for regmap access]
> [Julien Stephan: fix scale issue]
> [Julien Stephan: use the new iio_device_claim_direct_scoped
> instead of iio_device_claim_direct_mode]
> Signed-off-by: Julien Stephan <jstephan@xxxxxxxxxxxx>

I took another look, mostly so I could refresh my memory of the driver
before getting to the newer patches.

A few minor things inline, + I wondering if the structure for the
scan is what was meant about keeping timestamps in the same place.

> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> new file mode 100644
> index 000000000000..a218f59c276e
> --- /dev/null
> +++ b/drivers/iio/adc/ad7380.c
> @@ -0,0 +1,443 @@

> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

This and linux/sysfs.h are are normally only needed it you are providing
custom attrs.  I don't see those here so not sure those headers are used
directly by this driver.

> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +

> +struct ad7380_state {
> +	const struct ad7380_chip_info *chip_info;
> +	struct spi_device *spi;
> +	struct regmap *regmap;
> +	unsigned int vref_mv;
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 * Make the buffer large enough for 2 16-bit samples and one 64-bit
> +	 * aligned 64 bit timestamp.
> +	 */
> +	struct {
> +		u16 raw[2];
> +
> +		s64 ts __aligned(8);

Ah is the comment in patch 10 about consistent timestamps coming from this?
If so then don't worry about that - you just loose the pretty option of
using a structure to define the data layout.

	union {
		u16 raw[4 + 4] __aligned(8); /* include the ts*/
		u32 raw[4 + 2] __aligned(8);  /* include ts */
	} scan_data __aligned(IIO_DMA_MINALIGN);

There are lots of drivers that are highly flexible in how many
channels we have so they always have to take this more manual path
to data sizing.   Some do magic with ALIGN() so if you want you could
do something like that here. That magic ensures the right amount of
padding to have the naturally aligned 8 byte timestamp in an array of
smaller type.	

Note you can't solve this by putting the timestamp first because it
is always optional whether it is enabled (that bit is just dealt with
by the IIO core rather than a driver).

> +	} scan_data __aligned(IIO_DMA_MINALIGN);
> +	u16 tx;
> +	u16 rx;
> +};

> +static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> +				     u32 writeval, u32 *readval)
> +{
> +	iio_device_claim_direct_scoped(return  -EBUSY, indio_dev) {
> +		struct ad7380_state *st = iio_priv(indio_dev);
> +		int ret;
> +
> +		if (readval)
> +			ret = regmap_read(st->regmap, reg, readval);
Trivial, but 
			return regmap_read()
> +		else
> +			ret = regmap_write(st->regmap, reg, writeval);
			return regmap_write()

unless something gets added here later in the series.

> +
> +		return ret;
> +	}
> +	unreachable();
> +}