Message-ID: <5fd17b66eab1989b9cfb874445c18480a2282809.camel@gmail.com>
Date: Wed, 07 Feb 2024 11:10:44 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, linux-iio@...r.kernel.org
Cc: Michael Hennerich <Michael.Hennerich@...log.com>, Jonathan Cameron
 <jic23@...nel.org>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
 Nuno Sá <nuno.sa@...log.com>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: ad7944: add driver for
 AD7944/AD7985/AD7986

Hi David,

The driver It's in pretty good shape... Just some comments from me
 
On Tue, 2024-02-06 at 11:26 -0600, David Lechner wrote:
> This adds a driver for the Analog Devices Inc. AD7944, AD7985, and
> AD7986 ADCs. These are a family of pin-compatible ADCs that can sample
> at rates up to 2.5 MSPS.
> 
> The initial driver adds support for sampling at lower rates using the
> usual IIO triggered buffer and can handle all 3 possible reference
> voltage configurations.
> 
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  10 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad7944.c | 397
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 409 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4f1e658e1e0d..83d8367595f1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -458,6 +458,7 @@ R:	David Lechner <dlechner@...libre.com>
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> +F:	drivers/iio/adc/ad7944.c
>  
>  ADAFRUIT MINI I2C GAMEPAD
>  M:	Anshul Dalal <anshulusr@...il.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 59ae1d17b50d..93fbe6f8e306 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -280,6 +280,16 @@ config AD7923
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7923.
>  
> +config AD7944
> +	tristate "Analog Devices AD7944 and similar ADCs driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Analog Devices
> +	  AD7944, AD7985, AD7986 ADCs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad7944
> +
>  config AD7949
>  	tristate "Analog Devices AD7949 and similar ADCs driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 5a26ab6f1109..52d803b92cd7 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_AD7780) += ad7780.o
>  obj-$(CONFIG_AD7791) += ad7791.o
>  obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
> +obj-$(CONFIG_AD7944) += ad7944.o
>  obj-$(CONFIG_AD7949) += ad7949.o
>  obj-$(CONFIG_AD799X) += ad799x.o
>  obj-$(CONFIG_AD9467) += ad9467.o
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> new file mode 100644
> index 000000000000..67b525fb8e59
> --- /dev/null
> +++ b/drivers/iio/adc/ad7944.c
> @@ -0,0 +1,397 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD7944/85/86 PulSAR ADC family driver.
> + *
> + * Copyright 2024 Analog Devices, Inc.
> + * Copyright 2024 Baylibre, SAS
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define AD7944_INTERNAL_REF_MV		4096
> +
> +struct ad7944_timing_spec {
> +	/* Normal mode minimum CNV pulse width in nanoseconds. */
> +	unsigned int cnv_ns;
> +	/* TURBO mode minimum CNV pulse width in nanoseconds. */
> +	unsigned int turbo_cnv_ns;
> +};
> +
> 

..

> +}
> +
> +static int ad7944_single_conversion(struct ad7944_adc *adc,
> +				    const struct iio_chan_spec *chan,
> +				    int *val)
> +{
> +	int ret;
> +
> +	ret = ad7944_4_wire_mode_conversion(adc, chan);
> +	if (ret)
> +		return ret;
> +
> +	if (chan->scan_type.storagebits > 16)
> +		*val = adc->sample.raw.u32;
> +	else
> +		*val = adc->sample.raw.u16;
> +

Will this work both in big vs little endian archs? I don't think so but maybe
I'm missing something. At a first glance, it seems we get big endian from spi so
shouldn't we have __be16 and __be32?
 
> +	if (chan->scan_type.sign == 's')
> +		*val = sign_extend32(*val, chan->scan_type.realbits - 1);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad7944_read_raw(struct iio_dev *indio_dev,
> +			   const struct iio_chan_spec *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad7944_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +

I'm not totally sure but I think Jonathan already merged his series for the
cleanup stuff for the claim direct mode. Maybe take a look and use it? Not a big
win in here but I guess we could still reduce some LOC.

> +		ret = ad7944_single_conversion(adc, chan, val);
> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = adc->ref_mv;
> +			*val2 = chan->scan_type.realbits;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ad7944_iio_info = {
> +	.read_raw = &ad7944_read_raw,
> +};
> +
> +static irqreturn_t ad7944_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad7944_adc *adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad7944_4_wire_mode_conversion(adc, &indio_dev->channels[0]);
> +	if (ret)
> +		goto out;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
> +					   indio_dev->scan_timestamp);
> +
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const char * const ad7944_power_supplies[] = {
> +	"avdd",	"dvdd",	"bvdd", "vio"
> +};
> +
> +static void ad7944_ref_disable(void *ref)
> +{
> +	regulator_disable(ref);
> +}
> +
> +static int ad7944_probe(struct spi_device *spi)
> +{
> +	const struct ad7944_chip_info *chip_info;
> +	struct iio_dev *indio_dev;
> +	struct ad7944_adc *adc;
> +	struct regulator *ref;
> +	const char *str_val;
> +	int ret;
> +
> +	/* adi,spi-mode property defaults to "4-wire" if not present */
> +	if (device_property_read_string(&spi->dev, "adi,spi-mode", &str_val)
> < 0)
> +		str_val = "4-wire";
> +
> +	if (strcmp(str_val, "4-wire"))
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				     "only \"4-wire\" mode is currently
> supported\n");

Did you looked at spi core? I guess the chain mode is not available but IIRC spi
already has spi-3wire. So maybe you could just have a boolean property for the
chain mode and check both that and 3wire?

> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc = iio_priv(indio_dev);
> +	adc->spi = spi;
> +
> +	chip_info = spi_get_device_match_data(spi);
> +	if (!chip_info)
> +		return dev_err_probe(&spi->dev, -EINVAL, "no chip info\n");
> +
> +	adc->t = chip_info->t;
> +
> +	/*
> +	 * Some chips use unusual word sizes, so check now instead of waiting
> +	 * for the first xfer.
> +	 */
> +	if (!spi_is_bpw_supported(spi, chip_info-
> >channels[0].scan_type.realbits))
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				"SPI host does not support %d bits per
> word\n",
> +				chip_info->channels[0].scan_type.realbits);
> +
> +	ret = devm_regulator_bulk_get_enable(&spi->dev,
> +					    
> ARRAY_SIZE(ad7944_power_supplies),
> +					     ad7944_power_supplies);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "failed to get and enable supplies\n");
> +
> +	/* adi,reference property defaults to "internal" if not present */
> +	if (device_property_read_string(&spi->dev, "adi,reference", &str_val)
> < 0)
> +		str_val = "internal";
> +
> +	/* sort out what is being used for the reference voltage */
> +	if (strcmp(str_val, "internal") == 0) {

Maybe you can make the code neater with match_string() and some enum...

- Nuno Sá

Powered by blists - more mailing lists