Date:   Sat, 21 Mar 2020 10:48:29 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
Cc:     "ardeleanalex@...il.com" <ardeleanalex@...il.com>,
        "Nagy, Laszlo" <Laszlo.Nagy@...log.com>,
        "Csomortani, Istvan" <Istvan.Csomortani@...log.com>,
        "Grozav, Andrei" <Andrei.Grozav@...log.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "Bogdan, Dragos" <Dragos.Bogdan@...log.com>,
        "Costina, Adrian" <Adrian.Costina@...log.com>
Subject: Re: [PATCH v9 0/8] iio: adi-axi-adc,ad9647: Add support for AD9467
 ADC

On Mon, 16 Mar 2020 08:47:27 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@...log.com> wrote:

> On Sun, 2020-03-15 at 13:06 +0000, Jonathan Cameron wrote:
> > On Thu, 12 Mar 2020 10:35:03 +0200
> > Alexandru Ardelean <ardeleanalex@...il.com> wrote:
> >   
> > > This changeset adds support for the AD9467 LVDS High-Speed ADC.
> > > In order to support it, support for an FPGA ADI AXI ADC is added in this
> > > set.
> > > This uses the current support for IIO buffer DMAEngine.  
> > 
> > I took a look through this series and I'm think I'm now happy with it.
> > However, I'd like to leave a bit of time for Rob, the DT binding
> > Maintainer to take a look at the bindings.
> > 
> > Long term I sort of wonder if we might get some alternative for the
> > ADI-AXI interface, either because Analog decide to rewrite it, or
> > because someone else puts together similar IP. At that point we may
> > want a more sophisticated registration framework etc.  I guess we
> > can wait and see if anyone ever does that though.  
> 
> This registration mechanism is a bit of a rewrite of an older mechanism that's
> been in-use for 7+ years.
> https://github.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/cf_axi_adc_core.c#L832
> Essentially, the old one searches on the SPI bus for a device, while this one
> makes the coupling to the AXI-ADC driver a bit tighter, by requiring it to
> explicitly register with the AXI-ADC driver.
> No idea which is better; since both implementations require knowledge about the
> AXI-ADC driver.
> In the DT, the main difference is a rename of the property 'spibus-connected' to
> 'adi,adc-dev'.

The approach here seems reasonable to me.  We can be more flexible
if we ever need to.

> 
> I agree that a more sophisticated/generalized interface between the 2 would be a
> better idea, but [given how long this has been as-is], I don't feel it happening
> really soon.
> 
> I'm also preparing now an AXI-DAC driver, similar to this one.
> The idea would be to also prepare upstreaming the AD9361, which is an RF
> transceiver. I'm not sure whether that would classify as an MFD device [ADC &
> DAC], or whether as an IIO device, with a drivers/iio/transceiver sub-type.

We did have the slightly silly naming of addac in staging for a while.

Anyhow, I'd say a single driver (not mfd), in a new directory as appropriate.

> 
> Related, what would be interesting, is a generic multi-chip-sync mechanism.
> For many complicated devices [typically high-speed ADCs/DACs/transceivers], this
> seems to be a more common use-case.
> I've been wondering about a way to do it; if you have any input, or a subsystem
> that does that well, I'm open to inputs.
> MFD doesn't seem to be quite be that. Doing it with clocks has been tried and is
> not very easy.
> Another idea would be to try to implement some device-group mechanism. The idea
> would be to have state-transition tables, and each driver defines it's own
> callbacks for each state/state-transition. Whenever a device-group needs to jump
> a certain state, all callbacks are called to put each device in it's own sub-
> state.
> Still not sure how good this approach is, but it sounds the closest to something
> that would work.

I'm not sure I follow in enough detail what you mean to offer detailed opinions.
Are we talking pipeling type setups similar to the media framework does?

It would certainly be possible to have a notification system that multiple driver
can register to but I'm not sure I follow the usecase.  Perhaps some in depth
examples would help me understand?

Thanks,

Jonathan

> 
> Thanks
> Alex
> 
> > 
> > Jonathan
> >   
> > > Changelog v8 -> v9:
> > > * adding more Analog people to the list; predominantly HDL people; this
> > >   should help me sync people about the details of regs/reg-names
> > > * added 'Acked-by: Moritz Fischer <mdf@...nel.org>' tag to fpga patches
> > >   - we can always re-update these patches if something else is decided about
> > >     the location of the 'adi-axi-common.h' header; I'm not insisting about
> > >     where to put it; I'm open to other proposals
> > > * patch 'iio: adc: adi-axi-adc: add support for AXI ADC IP core'
> > >   - prefixed regs ADI_AXI_ ; I tried ADI_AXI_ADC_, but that seemed to make
> > >     them too long
> > >   - dropped unused regs; will add them as stuff gets added in the upstream
> > >     driver; in the meantime, reg-names can be reworked
> > >   - dropped generic LOWERXY_SET/GET macros
> > >   - update reg-names a bit; will update them in the docs and HDL
> > >   - order in adi_axi_adc_conv_unregister() should now be symmetrically
> > >     oppposite now to the register function
> > >   - implemented 'is_visible()' callback to adi_axi_adc_attributes[] so that
> > >     attrs can be made invisible to userspace if needed;
> > >   - 'indio_dev->name = "adi-axi-adc";'
> > >   - added kernel doc-string for @reg_access
> > > * patch 'iio: adc: ad9467: add support AD9467 ADC'
> > >   - ad9467_spi_read() split in 2 buffers; tbuf & rbuf
> > >   - removed 'if (chan->extend_name)' test ; left-over from initial driver
> > >   - removed 'if (!st->clk)' check; driver will fail probe without a clock
> > >   - removed 'if (!spi->dev.of_node)' in probe; shouldn't be needed
> > >   - using 'of_device_get_match_data()' in probe to get data; moved chip
> > >     info table entry as data on the of_device_id table
> > > 
> > > Changelog v7 -> v8:
> > > * in 'iio: adc: adi-axi-adc: add support for AXI ADC IP core'
> > >   - updated register definitions and bits to newer format/docs; the ref
> > > driver wasn't really up-to-date
> > >     -- prefixed bit names with reg-name to avoid bit definition colisions;
> > > that makes some macros longer, but at least the format is consistent
> > >   - using dev_name(&pdev->dev) for indio_dev->name
> > >   - moved reset to own axi_adc_reset() function; may be re-used later
> > >   - some re-formatting/alignment changes
> > >   - address ENOSYS checkpatch complaint; changed with EOPNOTSUPP
> > > 
> > > Changelog v6 -> v7:
> > > * Fixed dt-schema build for adi,axi-adc.yaml based on Rob's suggestion
> > >   - added '$ref: /schemas/types.yaml#/definitions/phandle' to 'adi,adc-dev'
> > >   - dropped 'maxItems' from 'adi,adc-dev'
> > > 
> > > Changelog v5 -> v6
> > > * fix URLs; got changed during rename
> > >    https://wiki.analog.com/resources/fpga/docs/adi_axi_adc_ip ->
> > >    https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> > >   - noticed while working on the AXI DAC driver
> > > 
> > > Changelog v4 -> v5:
> > > * update drivers/iio/adc/Kconfig note about module name; omitted during
> > > first rename
> > >    - 'module will be called axi-adc.' -> 'module will be called adi-axi-
> > > adc.'
> > > 
> > > Changelog v3 -> v4:
> > > * addressed Rob's dt-remarks
> > >    - change 'adi-axi-adc-client' prop to 'adi,adc-dev'
> > > 
> > > Changelog v2 -> v3:
> > > * addressed compiler warning
> > > 
> > > Changelog v1 -> v2:
> > > * first series was added a bit hastily
> > > * addressed  'make dt_binding_check' complaints; seems I missed a few when
> > > running the check; 
> > > * added missing patches to include/linux/fpga/adi-axi-common.h
> > >    - 'include: fpga: adi-axi-common.h: fixup whitespace tab -> space'
> > >    - 'include: fpga: adi-axi-common.h: add version helper macros'
> > > * patch 'iio: buffer-dmaengine: add dev-managed calls for buffer alloc/free'
> > >    - remove copy+pasted comment for 'devm_iio_dmaengine_buffer_alloc()'
> > >    - removed devm_iio_dmaengine_buffer_free() ; hopefully it might never be
> > > needed
> > >    - fix-up alignment for devm_iio_dmaengine_buffer_alloc() in header
> > > * patch 'iio: adc: adi-axi-adc: add support for AXI ADC IP core'
> > >    - renamed axi-adc.c -> adi-axi-adc.c & Kconfig symbol
> > >    - prefix all axi_adc -> adi_axi_adc
> > >    - removed switch statement in axi_adc_read_raw() & axi_adc_write_raw()
> > >    - remove axi_adc_chan_spec ; replaced with iio_chan_spec directly ; will
> > > think of a simpler solution for extra chan params
> > >    - removed left-over 'struct axi_adc_cleanup_data'
> > >    - moved 'devm_add_action_or_reset()' call right after
> > > 'adi_axi_adc_attach_client()'
> > >    - switched to using 'devm_platform_ioremap_resource()'
> > > * patch 'iio: adc: ad9467: add support AD9467 ADC'
> > >   - renamed ADI_ADC reg prefixes to AN877_ADC
> > >   - dropped 'info_mask_separate' field in AD9467_CHAN - will be re-added
> > > later when driver gets more features; was left-over from the initial ref
> > > driver
> > >   - remove .shift = 0,  in AD9467_CHAN
> > >   - renamed 'sample-clock' -> 'adc-clock'
> > >   - direct returns in ad9467_read_raw() & ad9467_write_raw() &
> > > ad9467_setup() switch statements
> > >   - removed blank line after devm_axi_adc_conv_register()
> > >   - removed ad9467_id & reworked to use ad9467_of_match
> > > 
> > > Alexandru Ardelean (6):
> > >   include: fpga: adi-axi-common.h: fixup whitespace tab -> space
> > >   include: fpga: adi-axi-common.h: add version helper macros
> > >   iio: buffer-dmaengine: use %zu specifier for sprintf(align)
> > >   iio: buffer-dmaengine: add dev-managed calls for buffer alloc
> > >   dt-bindings: iio: adc: add bindings doc for AXI ADC driver
> > >   dt-bindings: iio: adc: add bindings doc for AD9467 ADC
> > > 
> > > Michael Hennerich (2):
> > >   iio: adc: adi-axi-adc: add support for AXI ADC IP core
> > >   iio: adc: ad9467: add support AD9467 ADC
> > > 
> > >  .../bindings/iio/adc/adi,ad9467.yaml          |  65 +++
> > >  .../bindings/iio/adc/adi,axi-adc.yaml         |  63 +++
> > >  drivers/iio/adc/Kconfig                       |  35 ++
> > >  drivers/iio/adc/Makefile                      |   2 +
> > >  drivers/iio/adc/ad9467.c                      | 420 ++++++++++++++
> > >  drivers/iio/adc/adi-axi-adc.c                 | 518 ++++++++++++++++++
> > >  .../buffer/industrialio-buffer-dmaengine.c    |  41 +-
> > >  include/linux/fpga/adi-axi-common.h           |   6 +-
> > >  include/linux/iio/adc/adi-axi-adc.h           |  64 +++
> > >  include/linux/iio/buffer-dmaengine.h          |   3 +
> > >  10 files changed, 1215 insertions(+), 2 deletions(-)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
> > >  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,axi-
> > > adc.yaml
> > >  create mode 100644 drivers/iio/adc/ad9467.c
> > >  create mode 100644 drivers/iio/adc/adi-axi-adc.c
> > >  create mode 100644 include/linux/iio/adc/adi-axi-adc.h
> > >   

Powered by blists - more mailing lists