Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

From: David Lechner <dlechner@baylibre.com>
To: nuno.sa@analog.com
Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-iio@vger.kernel.org,
	Olivier MOYSAN <olivier.moysan@foss.st.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Michael Hennerich <Michael.Hennerich@analog.com>
Subject: Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework
Date: Thu, 30 Nov 2023 17:30:40 -0600	[thread overview]
Message-ID: <CAMknhBFbLju8UQJ7Uz85kHKrbK4mzt=wTRdnp40+PwWCJa5dsA@mail.gmail.com> (raw)
In-Reply-To: <20231121-dev-iio-backend-v1-10-6a3d542eba35@analog.com>

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> Convert the driver to use the new IIO backend framework. The device
> functionality is expected to be the same (meaning no added or removed
> features).

Missing a devicetree bindings patch before this one?

>
> Also note this patch effectively breaks ABI and that's needed so we can
> properly support this device and add needed features making use of the
> new IIO framework.

Can you be more specific about what is actually breaking?

>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/Kconfig  |   2 +-
>  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++------------------
>  2 files changed, 157 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 1e2b7a2c67c6..af56df63beff 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -275,7 +275,7 @@ config AD799X
>  config AD9467
>         tristate "Analog Devices AD9467 High Speed ADC driver"
>         depends on SPI
> -       depends on ADI_AXI_ADC
> +       select IIO_BACKEND
>         help
>           Say yes here to build support for Analog Devices:
>           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 5db5690ccee8..8b0402e73ace 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c

<snip>

> +static int ad9467_buffer_get(struct iio_dev *indio_dev)

perhaps a more descriptive name: ad9467_buffer_setup_optional?

> +{
> +       struct device *dev = indio_dev->dev.parent;
> +       const char *dma_name;
> +
> +       if (!device_property_present(dev, "dmas"))
> +               return 0;
> +
> +       if (device_property_read_string(dev, "dma-names", &dma_name))
> +               dma_name = "rx";
> +
> +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);

The device tree bindings for "adi,ad9467" don't include dma properties
(nor should they). Perhaps the DMA lookup should be a callback to the
backend? Or something similar to the SPI Engine offload that we are
working on?

> +}
> +
>  static int ad9467_probe(struct spi_device *spi)
>  {
> -       const struct ad9467_chip_info *info;
> -       struct adi_axi_adc_conv *conv;
> +       struct iio_dev *indio_dev;
>         struct ad9467_state *st;
>         unsigned int id;
>         int ret;
>
> -       info = spi_get_device_match_data(spi);
> -       if (!info)
> -               return -ENODEV;
> -
> -       conv = devm_adi_axi_adc_conv_register(&spi->dev, sizeof(*st));
> -       if (IS_ERR(conv))
> -               return PTR_ERR(conv);
> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +       if (!indio_dev)
> +               return -ENOMEM;
>
> -       st = adi_axi_adc_conv_priv(conv);
> +       st = iio_priv(indio_dev);
>         st->spi = spi;
>
> +       st->info = spi_get_device_match_data(spi);
> +       if (!st->info)
> +               return -ENODEV;
> +
>         st->clk = devm_clk_get_enabled(&spi->dev, "adc-clk");
>         if (IS_ERR(st->clk))
>                 return PTR_ERR(st->clk);
> @@ -476,29 +522,39 @@ static int ad9467_probe(struct spi_device *spi)
>         if (ret)
>                 return ret;
>
> -       conv->chip_info = &info->axi_adc_info;
> -
> -       ret = ad9467_scale_fill(conv);
> +       ret = ad9467_scale_fill(st);
>         if (ret)
>                 return ret;
>
>         id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
> -       if (id != conv->chip_info->id) {
> +       if (id != st->info->id) {
>                 dev_err(&spi->dev, "Mismatch CHIP_ID, got 0x%X, expected 0x%X\n",
> -                       id, conv->chip_info->id);
> +                       id, st->info->id);
>                 return -ENODEV;
>         }
>
> -       conv->reg_access = ad9467_reg_access;
> -       conv->write_raw = ad9467_write_raw;
> -       conv->read_raw = ad9467_read_raw;
> -       conv->read_avail = ad9467_read_avail;
> -       conv->preenable_setup = ad9467_preenable_setup;
> +       indio_dev->name = st->info->name;
> +       indio_dev->channels = st->info->channels;
> +       indio_dev->num_channels = st->info->num_channels;
> +       indio_dev->info = &ad9467_info;
> +
> +       ret = ad9467_buffer_get(indio_dev);
> +       if (ret)
> +               return ret;
>
> -       st->output_mode = info->default_output_mode |
> -                         AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
> +       st->back = devm_iio_backend_get(&spi->dev, NULL);

Based on the descriptions given of IIO frontend and backend, I was
expecting this driver to be the backend since SPI is only used to
configure the chip while the adi-axi-adc driver is the one determining
the scan data format, providing the DMA (INDIO_BUFFER_HARDWARE), etc.

Also, from a devicetree "describe the hardware" mindset, it doesn't
seem like this chip (AD9467) should dictate a specific backend. I know
it doesn't make sense practlically for this chip to not use DMA given
the high sample rate, but why should the devicetree for this chip
require it when there is nothing intrensic about this chip itself
related to DMA?

> +       if (IS_ERR(st->back))
> +               return PTR_ERR(st->back);
>
> -       return 0;
> +       ret = iio_backend_enable(st->back);
> +       if (ret)
> +               return ret;
> +
> +       ret = ad9467_setup(st);
> +       if (ret)
> +               return ret;
> +
> +       return devm_iio_device_register(&spi->dev, indio_dev);
>  }
>
>  static const struct of_device_id ad9467_of_match[] = {
>
> --
> 2.42.1
>
>