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

On Sat, 2020-04-04 at 14:54 +0100, Jonathan Cameron wrote:
> On Mon, 30 Mar 2020 08:52:15 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@...log.com> wrote:
> 
> > 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://urldefense.com/v3/__https://github.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/cf_axi_adc_core.c*L832__;Iw!!A3Ni8CS0y2Y!rzKQ4ehAdI4N7JdZh6ujDXOLseAGVviKwPdgMWvE2SSyhVN5YVFLtS9dA5JUxVVeQDIr_A$ 
> > > > 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.
> I was more thinking about the media controller framework that lets you stitch
> together multiple hardware blocks at runtime.
> 
> https://urldefense.com/v3/__https://lwn.net/Articles/415714/__;!!A3Ni8CS0y2Y!rzKQ4ehAdI4N7JdZh6ujDXOLseAGVviKwPdgMWvE2SSyhVN5YVFLtS9dA5JUxVU8EV-xhA$ 
> 
> > > 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.
> 
> It does sound similar to the constraints seen in both media controller
> related stuff, and the for that matter some of the asoc bits where really
> quite
> complex reconfiguration can be needed.

Hmm, I'll take a look into media then a bit.

Thanks
Alex

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