Message-ID: <ce9c0b9d73435e923752c25ae6e66f6dbb562a14.camel@analog.com>
Date:   Mon, 30 Mar 2020 08:52:15 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "jic23@...nel.org" <jic23@...nel.org>
CC:     "Costina, Adrian" <Adrian.Costina@...log.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "Nagy, Laszlo" <Laszlo.Nagy@...log.com>,
        "Csomortani, Istvan" <Istvan.Csomortani@...log.com>,
        "ardeleanalex@...il.com" <ardeleanalex@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "Bogdan, Dragos" <Dragos.Bogdan@...log.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "Grozav, Andrei" <Andrei.Grozav@...log.com>
Subject: Re: [PATCH v9 0/8] iio: adi-axi-adc,ad9647: Add support for AD9467
 ADC

On Sat, 2020-03-21 at 10:48 +0000, Jonathan Cameron wrote:
> 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?

Seems I forgot about this email.
Sorry about that. If they go through the weekend, I might forget about some
emails.

So, I'm not super-familiar with meda. If you are referring to
of_graph/fwnode_graph stuff [for defining order of init], then yes, something
similar.
Though, I think I tried something with of_graph and it seemed to be a bit too
many DT entries.
I think the clock "links/connections" are a bit neater as they specify the
minimum required to know that 2 devices depend on one another in a certain
order.

> 
> 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?
> 

The use-case typically is when there are 2 x high-speed transceivers.
And this gets more complicated when doing JESD204.
Because you need certain clocks/PLLs and certain chips to initialize in a
certain order.

If you just want to do a single-static initialization during probe [for some
complex transceiver configuration], you can do it via probe and that is fine [it
works].

But if you want to re-configure the JESD204 link [at run-time], or change some
transceiver parameters, you would [sometimes] need to re-do a big part of the
configuration topology, same as during probe, including in the same order that
they were probed; admittedly, some things can be done via the clock framework,
but there are limitations.
A workaround [which we didn't try and likely won't] is to convert the drivers to
modules and move some things to userspace.  
In the meantime, one thing I'm trying is to decouple some init stuff away from
probe and move it to a generic state-machine that takes care of re-
initialization ordering.


> 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