Received-SPF: pass (google.com: domain of linux-kernel+bounces-56282-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <5fd17b66eab1989b9cfb874445c18480a2282809.camel@gmail.com>
Subject: Re: [PATCH 2/2] iio: adc: ad7944: add driver for
 AD7944/AD7985/AD7986
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: David Lechner <dlechner@baylibre.com>, linux-iio@vger.kernel.org
Cc: Michael Hennerich <Michael.Hennerich@analog.com>, Jonathan Cameron
 <jic23@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
 Nuno =?ISO-8859-1?Q?S=E1?= <nuno.sa@analog.com>, Liam Girdwood
 <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Date: Wed, 07 Feb 2024 11:10:44 +0100
In-Reply-To: <20240206-ad7944-mainline-v1-2-bf115fa9474f@baylibre.com>
References: <20240206-ad7944-mainline-v1-0-bf115fa9474f@baylibre.com>
	 <20240206-ad7944-mainline-v1-2-bf115fa9474f@baylibre.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 
Precedence: bulk
MIME-Version: 1.0

Hi David,

The driver It's in pretty good shape... Just some comments from me
=20
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.
>=20
> 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.
>=20
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> =C2=A0MAINTAINERS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0 1 +
> =C2=A0drivers/iio/adc/Kconfig=C2=A0 |=C2=A0 10 ++
> =C2=A0drivers/iio/adc/Makefile |=C2=A0=C2=A0 1 +
> =C2=A0drivers/iio/adc/ad7944.c | 397
> +++++++++++++++++++++++++++++++++++++++++++++++
> =C2=A04 files changed, 409 insertions(+)
>=20
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4f1e658e1e0d..83d8367595f1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -458,6 +458,7 @@ R:	David Lechner <dlechner@baylibre.com>
> =C2=A0S:	Supported
> =C2=A0W:	https://ez.analog.com/linux-software-drivers
> =C2=A0F:	Documentation/devicetree/bindings/iio/adc/adi,ad7944.yaml
> +F:	drivers/iio/adc/ad7944.c
> =C2=A0
> =C2=A0ADAFRUIT MINI I2C GAMEPAD
> =C2=A0M:	Anshul Dalal <anshulusr@gmail.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
> =C2=A0	=C2=A0 To compile this driver as a module, choose M here: the
> =C2=A0	=C2=A0 module will be called ad7923.
> =C2=A0
> +config AD7944
> +	tristate "Analog Devices AD7944 and similar ADCs driver"
> +	depends on SPI
> +	help
> +	=C2=A0 Say yes here to build support for Analog Devices
> +	=C2=A0 AD7944, AD7985, AD7986 ADCs.
> +
> +	=C2=A0 To compile this driver as a module, choose M here: the
> +	=C2=A0 module will be called ad7944
> +
> =C2=A0config AD7949
> =C2=A0	tristate "Analog Devices AD7949 and similar ADCs driver"
> =C2=A0	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) +=3D ad7780.o
> =C2=A0obj-$(CONFIG_AD7791) +=3D ad7791.o
> =C2=A0obj-$(CONFIG_AD7793) +=3D ad7793.o
> =C2=A0obj-$(CONFIG_AD7887) +=3D ad7887.o
> +obj-$(CONFIG_AD7944) +=3D ad7944.o
> =C2=A0obj-$(CONFIG_AD7949) +=3D ad7949.o
> =C2=A0obj-$(CONFIG_AD799X) +=3D ad799x.o
> =C2=A0obj-$(CONFIG_AD9467) +=3D 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;
> +};
> +
>=20

..

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

Will this work both in big vs little endian archs? I don't think so but may=
be
I'm missing something. At a first glance, it seems we get big endian from s=
pi so
shouldn't we have __be16 and __be32?
=20
> +	if (chan->scan_type.sign =3D=3D 's')
> +		*val =3D sign_extend32(*val, chan->scan_type.realbits - 1);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad7944_read_raw(struct iio_dev *indio_dev,
> +			=C2=A0=C2=A0 const struct iio_chan_spec *chan,
> +			=C2=A0=C2=A0 int *val, int *val2, long info)
> +{
> +	struct ad7944_adc *adc =3D iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret =3D 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 =3D 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 =3D adc->ref_mv;
> +			*val2 =3D chan->scan_type.realbits;
> +
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ad7944_iio_info =3D {
> +	.read_raw =3D &ad7944_read_raw,
> +};
> +
> +static irqreturn_t ad7944_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf =3D p;
> +	struct iio_dev *indio_dev =3D pf->indio_dev;
> +	struct ad7944_adc *adc =3D iio_priv(indio_dev);
> +	int ret;
> +
> +	ret =3D 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,
> +					=C2=A0=C2=A0 indio_dev->scan_timestamp);
> +
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const char * const ad7944_power_supplies[] =3D {
> +	"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 =3D "4-wire";
> +
> +	if (strcmp(str_val, "4-wire"))
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				=C2=A0=C2=A0=C2=A0=C2=A0 "only \"4-wire\" mode is currently
> supported\n");

Did you looked at spi core? I guess the chain mode is not available but IIR=
C 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 =3D devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc =3D iio_priv(indio_dev);
> +	adc->spi =3D spi;
> +
> +	chip_info =3D spi_get_device_match_data(spi);
> +	if (!chip_info)
> +		return dev_err_probe(&spi->dev, -EINVAL, "no chip info\n");
> +
> +	adc->t =3D 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 =3D devm_regulator_bulk_get_enable(&spi->dev,
> +					=C2=A0=C2=A0=C2=A0=C2=A0
> ARRAY_SIZE(ad7944_power_supplies),
> +					=C2=A0=C2=A0=C2=A0=C2=A0 ad7944_power_supplies);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret,
> +				=C2=A0=C2=A0=C2=A0=C2=A0 "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 =3D "internal";
> +
> +	/* sort out what is being used for the reference voltage */
> +	if (strcmp(str_val, "internal") =3D=3D 0) {

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

- Nuno S=C3=A1