Re: [PATCH v7 3/3] drivers: iio: adc: add support for ad777x family

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 14 Oct 2024 17:32:00 +0300
Ramona Alexandra Nechita <ramona.nechita@analog.com> wrote:

> Add support for AD7770, AD7771, AD7779 ADCs. The device is capable of
> sending out data both on DOUT lines interface,as on the SDO line.
> The driver currently implements only the SDO data streaming mode. SPI
> communication is used alternatively for accessing registers and streaming
> data. Register accesses are protected by crc8.
> 
> Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com>

A few comments inline.  I also tweaked white space in a few places
whilst applying.  Target in IIO is still sub 80 chars whenever it
doesn't hurt readability.

Also, you had unusual formatting for some of the macros. Avoid mixing
tabs then spaces for indentation of the \ 

series applied to the togreg branch of iio.git and initially pushed
out as testing so 0-day can take a look.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ad7779.c b/drivers/iio/adc/ad7779.c
> new file mode 100644
> index 000000000000..5904cc669f3a
> --- /dev/null
> +++ b/drivers/iio/adc/ad7779.c
> @@ -0,0 +1,909 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD7770, AD7771, AD7779 ADC
> + *
> + * Copyright 2023-2024 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/clk.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <asm/unaligned.h>

This moved, I'll fix up.

> +	{								       \
> +		.type = IIO_VOLTAGE,					       \
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE)  |	       \
> +				      BIT(IIO_CHAN_INFO_CALIBBIAS),	       \
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),       \

It's not technically an ABI issue, but ususual to have an ADC driver that provides
no direct readings via sysfs and also provides no indication of channel scaling.

A quick glance at the datasheet suggests there is a PGA in the path, so perhaps
you plan to add scaling later.  Raw reads of single channels would I think
have to just select from the bulk channel read back so not that trivial to implement.
Maybe worth doing longer term though as that is a useful debug interface if nothing
else.

Anyhow, I'm fine with taking the driver with the current feature set
I was just a bit surprised at which features were implemented.

> +		.address = (index),					       \
> +		.indexed = 1,						       \
> +		.channel = (index),					       \
> +		.scan_index = (index),					       \
> +		.ext_info = (_ext_info),				       \
> +		.scan_type = {						       \
> +			.sign = 's',					       \
> +			.realbits = 24,					       \
> +			.storagebits = 32,				       \
> +			.endianness = IIO_BE,				   \
> +		},								\
> +	}
> +
> +#define AD777x_CHAN_NO_FILTER_S(index)					       \
> +	AD777x_CHAN_S(index, NULL)
> +
> +#define AD777x_CHAN_FILTER_S(index)					       \
> +	AD777x_CHAN_S(index, ad7779_ext_filter)
> +static const struct iio_chan_spec ad7779_channels[] = {
> +	AD777x_CHAN_NO_FILTER_S(0),
> +	AD777x_CHAN_NO_FILTER_S(1),
> +	AD777x_CHAN_NO_FILTER_S(2),
> +	AD777x_CHAN_NO_FILTER_S(3),
> +	AD777x_CHAN_NO_FILTER_S(4),
> +	AD777x_CHAN_NO_FILTER_S(5),
> +	AD777x_CHAN_NO_FILTER_S(6),
> +	AD777x_CHAN_NO_FILTER_S(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +};
> +
> +static const struct iio_chan_spec ad7779_channels_filter[] = {
> +	AD777x_CHAN_FILTER_S(0),
> +	AD777x_CHAN_FILTER_S(1),
> +	AD777x_CHAN_FILTER_S(2),
> +	AD777x_CHAN_FILTER_S(3),
> +	AD777x_CHAN_FILTER_S(4),
> +	AD777x_CHAN_FILTER_S(5),
> +	AD777x_CHAN_FILTER_S(6),
> +	AD777x_CHAN_FILTER_S(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +};