Received-SPF: pass (google.com: domain of linux-kernel+bounces-89760-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sun, 3 Mar 2024 13:43:59 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
Cc: linux-iio@vger.kernel.org, Michael Hennerich
 <Michael.Hennerich@analog.com>, Rob Herring <robh+dt@kernel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley
 <conor+dt@kernel.org>, Nuno =?UTF-8?B?U8Oh?= <nuno.sa@analog.com>, Liam
 Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] iio: adc: ad7944: add driver for
 AD7944/AD7985/AD7986
Message-ID: <20240303134359.16d5e5f1@jic23-huawei>
In-Reply-To: <20240229-ad7944-mainline-v4-2-f88b5ec4baed@baylibre.com>
References: <20240229-ad7944-mainline-v4-0-f88b5ec4baed@baylibre.com>
	<20240229-ad7944-mainline-v4-2-f88b5ec4baed@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Thu, 29 Feb 2024 10:25:51 -0600
David Lechner <dlechner@baylibre.com> 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@baylibre.com>
> 
Hi David,

Fresh read through showed up a few more things.  They are all trivial except
for what I think is an inverted error condition which would break
cases where ref was not supplied in DT.

Jonathan


> ---
> v4 changes: none
> 
> v3 changes:
> - Replaced _sign with _diff in chip info struct to properly handle
>   pseudo-differential vs. true differential chips. Pseudo-differential chips
>   now just have a voltage0 channel instead of voltage0-voltage1.
> - Fixed not resetting the CNV gpio on error return.
> - Simplified check of adi,spi-mode property now that "multi" is no longer a
>   valid option.

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

> +#define AD7944_INTERNAL_REF_MV		4096
> +
> +struct ad7944_timing_spec {
> +	/* Normal mode max conversion time (t_{CONV}) in nanoseconds. */
Trivial.
Name makes unit obvious (which is exactly how it should be!),
so can drop it from the comment.

> +	unsigned int conv_ns;
> +	/* TURBO mode max conversion time (t_{CONV}) in nanoseconds. */
> +	unsigned int turbo_conv_ns;
> +};
> +
> +struct ad7944_adc {
> +	struct spi_device *spi;
> +	/* Chip-specific timing specifications. */
> +	const struct ad7944_timing_spec *t;

As mentioned below, t is too succinct. Just pay the price in characters for
timing_spec or something along those lines.

> +	/* GPIO connected to CNV pin. */
> +	struct gpio_desc *cnv;
> +	/* Optional GPIO to enable turbo mode. */
> +	struct gpio_desc *turbo;
> +	/* Indicates TURBO is hard-wired to be always enabled. */
> +	bool always_turbo;
> +	/* Reference voltage (millivolts). */
> +	unsigned int ref_mv;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	struct {
> +		union {
> +			u16 u16;
> +			u32 u32;
> +		} raw;
> +		u64 timestamp __aligned(8);
> +	 } sample __aligned(IIO_DMA_MINALIGN);
> +};


> +
> +static int ad7944_probe(struct spi_device *spi)
> +{
> +	const struct ad7944_chip_info *chip_info;
> +	struct iio_dev *indio_dev;
> +	struct ad7944_adc *adc;
> +	bool have_refin = false;
> +	struct regulator *ref;
> +	int ret;
> +
> +	/*
> +	 * driver currently only supports the conventional "4-wire" mode and
> +	 * not other special wiring configurations.
> +	 */
> +	if (device_property_present(&spi->dev, "adi,spi-mode"))
Trivial, so ignore if you like..

There are a lot of spi->dev in here, maybe it's wroth
a local variable
struct device *dev = &spi->dev;

> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				     "adi,spi-mode is not currently supported\n");
> +
> +	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;

That might want a bit more informative a name. Even ts is better than t!

>
> +
> +	/*
> +	 * Sort out what is being used for the reference voltage. Options are:
> +	 * - internal reference: neither REF or REFIN is connected
> +	 * - internal reference with external buffer: REF not connected, REFIN
> +	 *   is connected
> +	 * - external reference: REF is connected, REFIN is not connected
> +	 */
> +
> +	ref = devm_regulator_get_optional(&spi->dev, "ref");
> +	if (IS_ERR(ref)) {
> +		if (PTR_ERR(ref) != -ENODEV)

Confused. Isn't this inverse of what we want?
return an error if we got anything other than -ENODEV.
		if (PTR_ERR(ref) |= -ENODEV)
			return dev_err_probe(&spi->dev, PTR_ERR(ref),
					     "failed to get REF supply\n");
		ref = NULL;
	}

> +			ref = NULL;
> +		else
> +			return dev_err_probe(&spi->dev, PTR_ERR(ref),
> +					     "failed to get REF supply\n");
> +	}
> +

> +
> +	adc->cnv = devm_gpiod_get(&spi->dev, "cnv", GPIOD_OUT_LOW);
> +	if (IS_ERR(adc->cnv))
> +		return dev_err_probe(&spi->dev, PTR_ERR(adc->cnv),
> +				     "failed to get CNV GPIO\n");

Is this optional?  If we don't yet support the case the dt binding talks
about worth a comment here to say we don't yet support XYZ so this
is not optional.  

> +
> +	adc->turbo = devm_gpiod_get_optional(&spi->dev, "turbo", GPIOD_OUT_LOW);
> +	if (IS_ERR(adc->turbo))
> +		return dev_err_probe(&spi->dev, PTR_ERR(adc->turbo),
> +				     "failed to get TURBO GPIO\n");
> +
> +	if (device_property_present(&spi->dev, "adi,always-turbo"))
> +		adc->always_turbo = true;
> +

Trivial, but maybe..
	adc->always_turbo = device_property_present(&spi->dev, "adi,always_turbo");

Jonathan