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