[10/12] iio: adc: ad9467: convert to backend framework
Commit Message
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).
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.
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(-)
Comments
Hi Nuno,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.7-rc2 next-20231121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nuno-Sa-via-B4-Relay/driver-core-allow-modifying-device_links-flags/20231121-182010
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20231121-dev-iio-backend-v1-10-6a3d542eba35%40analog.com
patch subject: [PATCH 10/12] iio: adc: ad9467: convert to backend framework
config: powerpc-randconfig-r071-20231122 (https://download.01.org/0day-ci/archive/20231122/202311220807.NUS4r7ML-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231122/202311220807.NUS4r7ML-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311220807.NUS4r7ML-lkp@intel.com/
smatch warnings:
drivers/iio/adc/ad9467.c:276 ad9467_get_scale() warn: unsigned 'vref_val' is never less than zero.
vim +/vref_val +276 drivers/iio/adc/ad9467.c
ad67971202381c Michael Hennerich 2020-03-24 270
a78b758afbce92 Nuno Sa 2023-11-21 271 static int ad9467_get_scale(struct ad9467_state *st, int *val, int *val2)
ad67971202381c Michael Hennerich 2020-03-24 272 {
337dbb6ec1acc2 Alexandru Ardelean 2020-09-24 273 unsigned int i, vref_val;
ad67971202381c Michael Hennerich 2020-03-24 274
ad67971202381c Michael Hennerich 2020-03-24 275 vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
d1e957a3e7676f Nuno Sa 2023-11-21 @276 if (vref_val < 0)
d1e957a3e7676f Nuno Sa 2023-11-21 277 return vref_val;
ad67971202381c Michael Hennerich 2020-03-24 278
a78b758afbce92 Nuno Sa 2023-11-21 279 vref_val &= st->info->vref_mask;
ad67971202381c Michael Hennerich 2020-03-24 280
a78b758afbce92 Nuno Sa 2023-11-21 281 for (i = 0; i < st->info->num_scales; i++) {
a78b758afbce92 Nuno Sa 2023-11-21 282 if (vref_val == st->info->scale_table[i][1])
ad67971202381c Michael Hennerich 2020-03-24 283 break;
ad67971202381c Michael Hennerich 2020-03-24 284 }
ad67971202381c Michael Hennerich 2020-03-24 285
a78b758afbce92 Nuno Sa 2023-11-21 286 if (i == st->info->num_scales)
ad67971202381c Michael Hennerich 2020-03-24 287 return -ERANGE;
ad67971202381c Michael Hennerich 2020-03-24 288
a78b758afbce92 Nuno Sa 2023-11-21 289 __ad9467_get_scale(st, i, val, val2);
ad67971202381c Michael Hennerich 2020-03-24 290
ad67971202381c Michael Hennerich 2020-03-24 291 return IIO_VAL_INT_PLUS_MICRO;
ad67971202381c Michael Hennerich 2020-03-24 292 }
ad67971202381c Michael Hennerich 2020-03-24 293
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
>
>
On Thu, Nov 30, 2023 at 5:30 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<snip>
> > + 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?
>
Afterthought:
Put another way, it seems like it would be much easier to say "I, the
arbitrary frontend that actually handles the data from the LVDS
outputs, need a backend that provides a SPI connection to an AD9467
chip and takes care of turning on power supplies" than it is to say
"I, the AD9467 chip frontend, need an arbitrary backend that handles
reading data from the LVDS outputs in a very specific manner that is
determined by the driver, not the hardware".
On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> 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?
>
Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
wonder if it actually makes sense for a device like with no buffering support?!
> > +{
> > + 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?
>
Oh yes, I need to update the bindings. In the link I sent you we can see my thoughts
on this. In theory, hardwarewise, it would actually make sense for the DMA to be on
the backend device because that's where the connection is in HW. However, since we
want to have the IIO interface in the frontend, it would be hard to do that without
hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far from
wise to have the DMA buffer associated to a completely different device than the IIO
parent device. I mean, one way could just be export iio_dmaengine_buffer_free() and
iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the buffer
from the frontend device. If Jonathan is fine with this, I'm on board for it....
- Nuno Sá
>
On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> 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?
Device name for example changed. And it might be some other subtle breakage but that
was kind of agreed that would an ADI problem. I'm also fairly confident no one is
actually using the upstream version of the driver because it lacks some devices and
important features (like interface tuning).
- Nuno Sá
On Fri, Dec 1, 2023 at 3:08 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > 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?
> >
>
> Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> wonder if it actually makes sense for a device like with no buffering support?!
>
> > > +{
> > > + 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?
> >
>
> Oh yes, I need to update the bindings. In the link I sent you we can see my thoughts
> on this. In theory, hardwarewise, it would actually make sense for the DMA to be on
> the backend device because that's where the connection is in HW. However, since we
> want to have the IIO interface in the frontend, it would be hard to do that without
> hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far from
> wise to have the DMA buffer associated to a completely different device than the IIO
> parent device. I mean, one way could just be export iio_dmaengine_buffer_free() and
> iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the buffer
> from the frontend device. If Jonathan is fine with this, I'm on board for it....
>
> - Nuno Sá
> >
>
I was planning on exporting iio_dmaengine_buffer_alloc() [1] for SPI
Engine offload support, so I hope that is the right way to go. ;-)
[1]: https://github.com/analogdevicesinc/linux/pull/2341/commits/71048ff83a63e9d0a5ddb9ffa331871edd6bd2a5
On Fri, 2023-12-01 at 11:44 -0600, David Lechner wrote:
> On Fri, Dec 1, 2023 at 3:08 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > > 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?
> > >
> >
> > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> > thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> > wonder if it actually makes sense for a device like with no buffering support?!
> >
> > > > +{
> > > > + 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?
> > >
> >
> > Oh yes, I need to update the bindings. In the link I sent you we can see my
> > thoughts
> > on this. In theory, hardwarewise, it would actually make sense for the DMA to be
> > on
> > the backend device because that's where the connection is in HW. However, since
> > we
> > want to have the IIO interface in the frontend, it would be hard to do that
> > without
> > hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far
> > from
> > wise to have the DMA buffer associated to a completely different device than the
> > IIO
> > parent device. I mean, one way could just be export iio_dmaengine_buffer_free()
> > and
> > iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the
> > buffer
> > from the frontend device. If Jonathan is fine with this, I'm on board for it....
> >
> > - Nuno Sá
> > >
> >
>
> I was planning on exporting iio_dmaengine_buffer_alloc() [1] for SPI
> Engine offload support, so I hope that is the right way to go. ;-)
>
> [1]:
> https://github.com/analogdevicesinc/linux/pull/2341/commits/71048ff83a63e9d0a5ddb9ffa331871edd6bd2a5
I don't really want to extend much on this since this is still out of tree code so
I'm not sure we should be discussing it much in here. But there a couple of concerns
already I'm seeing:
* AFAIU, you export the function so you can use it in your pwm trigger. And you don't
want to attach the buffer to a device. That looks very questionable. If you don't
attach to a device, how do you have the userspace interface working on that buffer?
How can you fetch samples from it? Also hiding the buffer allocation in pure trigger
device is at the very least questionable. But the point is, in the end of the day,
the buffer should belong to a device.
* Your PWM trigger seems to be highly focused on the spi_engine offload feature. You
should go for a generic pwm trigger which is something that was already discussed on
the list and AFAIR, completely acceptable. That said, not so sure how much will we
win (in terms of code simplification) by having devices under the spi_engine
controller using a pwm trigger. But conceptually it is correct and we do have a mode
for hardware triggered buffers.
- Nuno Sá
On Sat, 2023-12-02 at 09:46 +0100, Nuno Sá wrote:
> On Fri, 2023-12-01 at 11:44 -0600, David Lechner wrote:
> > On Fri, Dec 1, 2023 at 3:08 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > >
> > > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > > > 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?
> > > >
> > >
> > > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that
> > > I'm
> > > thinking, I'm not so sure if this is just some legacy thing we had in ADI tree.
> > > I
> > > wonder if it actually makes sense for a device like with no buffering support?!
> > >
> > > > > +{
> > > > > + 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?
> > > >
> > >
> > > Oh yes, I need to update the bindings. In the link I sent you we can see my
> > > thoughts
> > > on this. In theory, hardwarewise, it would actually make sense for the DMA to
> > > be
> > > on
> > > the backend device because that's where the connection is in HW. However, since
> > > we
> > > want to have the IIO interface in the frontend, it would be hard to do that
> > > without
> > > hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be
> > > far
> > > from
> > > wise to have the DMA buffer associated to a completely different device than
> > > the
> > > IIO
> > > parent device. I mean, one way could just be export iio_dmaengine_buffer_free()
> > > and
> > > iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the
> > > buffer
> > > from the frontend device. If Jonathan is fine with this, I'm on board for
> > > it....
> > >
> > > - Nuno Sá
> > > >
> > >
> >
> > I was planning on exporting iio_dmaengine_buffer_alloc() [1] for SPI
> > Engine offload support, so I hope that is the right way to go. ;-)
> >
> > [1]:
> > https://github.com/analogdevicesinc/linux/pull/2341/commits/71048ff83a63e9d0a5ddb9ffa331871edd6bd2a5
>
> I don't really want to extend much on this since this is still out of tree code so
> I'm not sure we should be discussing it much in here. But there a couple of
> concerns
> already I'm seeing:
>
> * AFAIU, you export the function so you can use it in your pwm trigger. And you
> don't
> want to attach the buffer to a device. That looks very questionable. If you don't
> attach to a device, how do you have the userspace interface working on that buffer?
> How can you fetch samples from it? Also hiding the buffer allocation in pure
> trigger
> device is at the very least questionable. But the point is, in the end of the day,
> the buffer should belong to a device.
>
> * Your PWM trigger seems to be highly focused on the spi_engine offload feature.
> You
OTOH, it also seems there are some stm focused triggers. So maybe we can also have
something more oriented to spi_engine...
- Nuno Sá
On Fri, 01 Dec 2023 10:08:27 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > 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?
> >
>
> Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> wonder if it actually makes sense for a device like with no buffering support?!
>
> > > +{
> > > + 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?
> >
>
> Oh yes, I need to update the bindings. In the link I sent you we can see my thoughts
> on this. In theory, hardwarewise, it would actually make sense for the DMA to be on
> the backend device because that's where the connection is in HW. However, since we
> want to have the IIO interface in the frontend, it would be hard to do that without
> hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far from
> wise to have the DMA buffer associated to a completely different device than the IIO
> parent device. I mean, one way could just be export iio_dmaengine_buffer_free() and
> iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the buffer
> from the frontend device. If Jonathan is fine with this, I'm on board for it....
It is going to be fiddly but I'd kind of expect the front end to be using a more
abstracted interface to tell the backend to start grabbing data. Maybe that ends
up being so slim it's just these interfaces and it's not sensible to wrap it though.
The argument for the IIO device (you control) being associated with the analog/digital front end is that
is what happens with a consumer interface today. We think of the analog components (consuming device)
using the sevice of the ADC it is consuming to measure their state.
You can flip the whole thing and make the data grabbing engine the IIO device but
I guess that's just not how we did it and I'm sure I had many good reasons at the time
(one of them being that it needed to work for other cases such as a touchscreen driver
consuming the IIO device) I that case it's the touchscreen driver that is responsible
for data scaling etc not the ADC.
In my head, data acquisition is a service - what is interesting is what is being measured
and so I put the IIO device as near that as possible.
Jonathan
On Mon, 2023-12-04 at 15:48 +0000, Jonathan Cameron wrote:
> On Fri, 01 Dec 2023 10:08:27 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > > 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?
> > >
> >
> > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> > thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> > wonder if it actually makes sense for a device like with no buffering support?!
> >
> > > > +{
> > > > + 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?
> > >
> >
> > Oh yes, I need to update the bindings. In the link I sent you we can see my
> > thoughts
> > on this. In theory, hardwarewise, it would actually make sense for the DMA to be
> > on
> > the backend device because that's where the connection is in HW. However, since
> > we
> > want to have the IIO interface in the frontend, it would be hard to do that
> > without
> > hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far
> > from
> > wise to have the DMA buffer associated to a completely different device than the
> > IIO
> > parent device. I mean, one way could just be export iio_dmaengine_buffer_free()
> > and
> > iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the
> > buffer
> > from the frontend device. If Jonathan is fine with this, I'm on board for it....
>
> It is going to be fiddly but I'd kind of expect the front end to be using a more
> abstracted interface to tell the backend to start grabbing data. Maybe that ends
> up being so slim it's just these interfaces and it's not sensible to wrap it
> though.
>
Likely I'm missing your point but the discussion here is where the DMA buffer should
be allocated. In theory, in the backend (at least on ADI usecases - it's the proper
representation of the HW) but as we have the iio device in the frontend, it's more
appropriate to have the buffer there. Or at least to have a way to control the buffer
lifetime from there...
On the our usecases, it's not like we tell the backend to grab data, we just use the
normal .update_scan_mode() to enable/disable the channels in the backend so when we
enable the buffer (and the frontend starts receiving and sending data via the serial
interface) the data paths are enabaled/disabled accordingly. Bah, yeah, in a way is
another wording for "grab" or "grab not" :)
- Nuno Sá
On Mon, 04 Dec 2023 17:23:12 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Mon, 2023-12-04 at 15:48 +0000, Jonathan Cameron wrote:
> > On Fri, 01 Dec 2023 10:08:27 +0100
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > > > 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?
> > > >
> > >
> > > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> > > thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> > > wonder if it actually makes sense for a device like with no buffering support?!
> > >
> > > > > +{
> > > > > + 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?
> > > >
> > >
> > > Oh yes, I need to update the bindings. In the link I sent you we can see my
> > > thoughts
> > > on this. In theory, hardwarewise, it would actually make sense for the DMA to be
> > > on
> > > the backend device because that's where the connection is in HW. However, since
> > > we
> > > want to have the IIO interface in the frontend, it would be hard to do that
> > > without
> > > hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far
> > > from
> > > wise to have the DMA buffer associated to a completely different device than the
> > > IIO
> > > parent device. I mean, one way could just be export iio_dmaengine_buffer_free()
> > > and
> > > iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the
> > > buffer
> > > from the frontend device. If Jonathan is fine with this, I'm on board for it....
> >
> > It is going to be fiddly but I'd kind of expect the front end to be using a more
> > abstracted interface to tell the backend to start grabbing data. Maybe that ends
> > up being so slim it's just these interfaces and it's not sensible to wrap it
> > though.
> >
>
> Likely I'm missing your point but the discussion here is where the DMA buffer should
> be allocated. In theory, in the backend (at least on ADI usecases - it's the proper
> representation of the HW) but as we have the iio device in the frontend, it's more
> appropriate to have the buffer there. Or at least to have a way to control the buffer
> lifetime from there...
My instinct was put it in the backened and proxy / interfaces as necessary but (based
on my vague recollection of how this works) at times these DMA buffers are handed off
to userspace which is a front end problem rather than the hardware. So I guess it's
a question of who logically creates them? Then as you say provide the controls for
the other part to mess with their lifetimes or at least ensure the stick around whilst
it expects them to.
>
> On the our usecases, it's not like we tell the backend to grab data, we just use the
> normal .update_scan_mode() to enable/disable the channels in the backend so when we
> enable the buffer (and the frontend starts receiving and sending data via the serial
> interface) the data paths are enabaled/disabled accordingly. Bah, yeah, in a way is
> another wording for "grab" or "grab not" :)
Yup. It's not as easily separated as simple always on analog only front ends. Someone
drives the clock ultimately and that could be either end in theory at least.
What fun.
J
>
> - Nuno Sá
@@ -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
@@ -16,13 +16,13 @@
#include <linux/gpio/consumer.h>
#include <linux/of.h>
-
+#include <linux/iio/buffer-dmaengine.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
#include <linux/clk.h>
-#include <linux/iio/adc/adi-axi-adc.h>
+#include <linux/iio/backend.h>
/*
* ADI High-Speed ADC common spi interface registers
@@ -102,15 +102,20 @@
#define AD9467_REG_VREF_MASK 0x0F
struct ad9467_chip_info {
- struct adi_axi_adc_chip_info axi_adc_info;
- unsigned int default_output_mode;
- unsigned int vref_mask;
+ const char *name;
+ unsigned int id;
+ const struct iio_chan_spec *channels;
+ unsigned int num_channels;
+ const unsigned int (*scale_table)[2];
+ int num_scales;
+ unsigned long max_rate;
+ unsigned int default_output_mode;
+ unsigned int vref_mask;
};
-#define to_ad9467_chip_info(_info) \
- container_of(_info, struct ad9467_chip_info, axi_adc_info)
-
struct ad9467_state {
+ const struct ad9467_chip_info *info;
+ struct iio_backend *back;
struct spi_device *spi;
struct clk *clk;
unsigned int output_mode;
@@ -151,10 +156,10 @@ static int ad9467_spi_write(struct spi_device *spi, unsigned int reg,
return spi_write(spi, buf, ARRAY_SIZE(buf));
}
-static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
+static int ad9467_reg_access(struct iio_dev *indio_dev, unsigned int reg,
unsigned int writeval, unsigned int *readval)
{
- struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+ struct ad9467_state *st = iio_priv(indio_dev);
struct spi_device *spi = st->spi;
int ret;
@@ -191,14 +196,13 @@ static const unsigned int ad9467_scale_table[][2] = {
{2300, 8}, {2400, 9}, {2500, 10},
};
-static void __ad9467_get_scale(struct adi_axi_adc_conv *conv, int index,
+static void __ad9467_get_scale(struct ad9467_state *st, int index,
unsigned int *val, unsigned int *val2)
{
- const struct adi_axi_adc_chip_info *info = conv->chip_info;
- const struct iio_chan_spec *chan = &info->channels[0];
+ const struct iio_chan_spec *chan = &st->info->channels[0];
unsigned int tmp;
- tmp = (info->scale_table[index][0] * 1000000ULL) >>
+ tmp = (st->info->scale_table[index][0] * 1000000ULL) >>
chan->scan_type.realbits;
*val = tmp / 1000000;
*val2 = tmp % 1000000;
@@ -229,77 +233,66 @@ static const struct iio_chan_spec ad9467_channels[] = {
};
static const struct ad9467_chip_info ad9467_chip_tbl = {
- .axi_adc_info = {
- .name = "ad9467",
- .id = CHIPID_AD9467,
- .max_rate = 250000000UL,
- .scale_table = ad9467_scale_table,
- .num_scales = ARRAY_SIZE(ad9467_scale_table),
- .channels = ad9467_channels,
- .num_channels = ARRAY_SIZE(ad9467_channels),
- },
+ .name = "ad9467",
+ .id = CHIPID_AD9467,
+ .max_rate = 250000000UL,
+ .scale_table = ad9467_scale_table,
+ .num_scales = ARRAY_SIZE(ad9467_scale_table),
+ .channels = ad9467_channels,
+ .num_channels = ARRAY_SIZE(ad9467_channels),
.default_output_mode = AD9467_DEF_OUTPUT_MODE,
.vref_mask = AD9467_REG_VREF_MASK,
};
static const struct ad9467_chip_info ad9434_chip_tbl = {
- .axi_adc_info = {
- .name = "ad9434",
- .id = CHIPID_AD9434,
- .max_rate = 500000000UL,
- .scale_table = ad9434_scale_table,
- .num_scales = ARRAY_SIZE(ad9434_scale_table),
- .channels = ad9434_channels,
- .num_channels = ARRAY_SIZE(ad9434_channels),
- },
+ .name = "ad9434",
+ .id = CHIPID_AD9434,
+ .max_rate = 500000000UL,
+ .scale_table = ad9434_scale_table,
+ .num_scales = ARRAY_SIZE(ad9434_scale_table),
+ .channels = ad9434_channels,
+ .num_channels = ARRAY_SIZE(ad9434_channels),
.default_output_mode = AD9434_DEF_OUTPUT_MODE,
.vref_mask = AD9434_REG_VREF_MASK,
};
static const struct ad9467_chip_info ad9265_chip_tbl = {
- .axi_adc_info = {
- .name = "ad9265",
- .id = CHIPID_AD9265,
- .max_rate = 125000000UL,
- .scale_table = ad9265_scale_table,
- .num_scales = ARRAY_SIZE(ad9265_scale_table),
- .channels = ad9467_channels,
- .num_channels = ARRAY_SIZE(ad9467_channels),
- },
+ .name = "ad9265",
+ .id = CHIPID_AD9265,
+ .max_rate = 125000000UL,
+ .scale_table = ad9265_scale_table,
+ .num_scales = ARRAY_SIZE(ad9265_scale_table),
+ .channels = ad9467_channels,
+ .num_channels = ARRAY_SIZE(ad9467_channels),
.default_output_mode = AD9265_DEF_OUTPUT_MODE,
.vref_mask = AD9265_REG_VREF_MASK,
};
-static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
+static int ad9467_get_scale(struct ad9467_state *st, int *val, int *val2)
{
- const struct adi_axi_adc_chip_info *info = conv->chip_info;
- const struct ad9467_chip_info *info1 = to_ad9467_chip_info(info);
- struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
unsigned int i, vref_val;
vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
if (vref_val < 0)
return vref_val;
- vref_val &= info1->vref_mask;
+ vref_val &= st->info->vref_mask;
- for (i = 0; i < info->num_scales; i++) {
- if (vref_val == info->scale_table[i][1])
+ for (i = 0; i < st->info->num_scales; i++) {
+ if (vref_val == st->info->scale_table[i][1])
break;
}
- if (i == info->num_scales)
+ if (i == st->info->num_scales)
return -ERANGE;
- __ad9467_get_scale(conv, i, val, val2);
+ __ad9467_get_scale(st, i, val, val2);
return IIO_VAL_INT_PLUS_MICRO;
}
-static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
+static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
{
- const struct adi_axi_adc_chip_info *info = conv->chip_info;
- struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
unsigned int scale_val[2];
unsigned int i;
int ret;
@@ -307,14 +300,14 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
if (val != 0)
return -EINVAL;
- for (i = 0; i < info->num_scales; i++) {
- __ad9467_get_scale(conv, i, &scale_val[0], &scale_val[1]);
+ for (i = 0; i < st->info->num_scales; i++) {
+ __ad9467_get_scale(st, i, &scale_val[0], &scale_val[1]);
if (scale_val[0] != val || scale_val[1] != val2)
continue;
guard(mutex)(&st->lock);
ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
- info->scale_table[i][1]);
+ st->info->scale_table[i][1]);
if (ret < 0)
return ret;
@@ -325,15 +318,15 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
return -EINVAL;
}
-static int ad9467_read_raw(struct adi_axi_adc_conv *conv,
+static int ad9467_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long m)
{
- struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+ struct ad9467_state *st = iio_priv(indio_dev);
switch (m) {
case IIO_CHAN_INFO_SCALE:
- return ad9467_get_scale(conv, val, val2);
+ return ad9467_get_scale(st, val, val2);
case IIO_CHAN_INFO_SAMP_FREQ:
*val = clk_get_rate(st->clk);
@@ -343,20 +336,19 @@ static int ad9467_read_raw(struct adi_axi_adc_conv *conv,
}
}
-static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
+static int ad9467_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int val, int val2, long mask)
{
- const struct adi_axi_adc_chip_info *info = conv->chip_info;
- struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+ struct ad9467_state *st = iio_priv(indio_dev);
long r_clk;
switch (mask) {
case IIO_CHAN_INFO_SCALE:
- return ad9467_set_scale(conv, val, val2);
+ return ad9467_set_scale(st, val, val2);
case IIO_CHAN_INFO_SAMP_FREQ:
r_clk = clk_round_rate(st->clk, val);
- if (r_clk < 0 || r_clk > info->max_rate) {
+ if (r_clk < 0 || r_clk > st->info->max_rate) {
dev_warn(&st->spi->dev,
"Error setting ADC sample rate %ld", r_clk);
return -EINVAL;
@@ -368,26 +360,53 @@ static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
}
}
-static int ad9467_read_avail(struct adi_axi_adc_conv *conv,
+static int ad9467_read_avail(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
const int **vals, int *type, int *length,
long mask)
{
- const struct adi_axi_adc_chip_info *info = conv->chip_info;
- struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+ struct ad9467_state *st = iio_priv(indio_dev);
switch (mask) {
case IIO_CHAN_INFO_SCALE:
*vals = (const int *)st->scales;
*type = IIO_VAL_INT_PLUS_MICRO;
/* Values are stored in a 2D matrix */
- *length = info->num_scales * 2;
+ *length = st->info->num_scales * 2;
return IIO_AVAIL_LIST;
default:
return -EINVAL;
}
}
+static int ad9467_update_scan_mode(struct iio_dev *indio_dev,
+ const unsigned long *scan_mask)
+{
+ struct ad9467_state *st = iio_priv(indio_dev);
+ unsigned int c;
+ int ret;
+
+ for (c = 0; c < st->info->num_channels; c++) {
+ if (test_bit(c, scan_mask))
+ ret = iio_backend_chan_enable(st->back, c);
+ else
+ ret = iio_backend_chan_disable(st->back, c);
+
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct iio_info ad9467_info = {
+ .read_raw = ad9467_read_raw,
+ .write_raw = ad9467_write_raw,
+ .update_scan_mode = ad9467_update_scan_mode,
+ .debugfs_reg_access = ad9467_reg_access,
+ .read_avail = ad9467_read_avail,
+};
+
static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
{
int ret;
@@ -400,19 +419,17 @@ static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
AN877_ADC_TRANSFER_SYNC);
}
-static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
+static int ad9467_scale_fill(struct ad9467_state *st)
{
- const struct adi_axi_adc_chip_info *info = conv->chip_info;
- struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
unsigned int i, val1, val2;
st->scales = devm_kcalloc(&st->spi->dev, ARRAY_SIZE(*st->scales),
- info->num_scales, GFP_KERNEL);
+ st->info->num_scales, GFP_KERNEL);
if (!st->scales)
return -ENOMEM;
- for (i = 0; i < info->num_scales * 2; i++) {
- __ad9467_get_scale(conv, i, &val1, &val2);
+ for (i = 0; i < st->info->num_scales * 2; i++) {
+ __ad9467_get_scale(st, i, &val1, &val2);
st->scales[i][0] = val1;
st->scales[i][1] = val2;
}
@@ -420,11 +437,27 @@ static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
return 0;
}
-static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
+static int ad9467_setup(struct ad9467_state *st)
{
- struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+ struct iio_backend_data_fmt data = {
+ .sign_extend = true,
+ .enable = true,
+ };
+ unsigned int c, mode;
+ int ret;
- return ad9467_outputmode_set(st->spi, st->output_mode);
+ mode = st->info->default_output_mode | AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
+ ret = ad9467_outputmode_set(st->spi, mode);
+ if (ret)
+ return ret;
+
+ for (c = 0; c < st->info->num_channels; c++) {
+ ret = iio_backend_data_format_set(st->back, c, &data);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
}
static int ad9467_reset(struct device *dev)
@@ -444,25 +477,38 @@ static int ad9467_reset(struct device *dev)
return 0;
}
+static int ad9467_buffer_get(struct iio_dev *indio_dev)
+{
+ 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);
+}
+
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);
+ 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[] = {