| Message ID | 20240920-ad7606_add_iio_backend_support-v2-0-0e78782ae7d0@baylibre.com |
|---|---|
| Headers | show |
| Series | Add iio backend compatibility for ad7606 | expand |
On Fri, Sep 20, 2024 at 05:33:22PM +0000, Guillaume Stols wrote: > The SPI conditions are not always required, because there is also a > parallel interface. The way used to detect that the SPI interface is > used is to check if the reg value is between 0 and 256. And, yaknow, not that the bus you're on is a spi bus? I don't think this comment is relevant to the binding, especially given you have a property for it. > There is also a correction on the spi-cpha that is not required when SPI > interface is selected, while spi-cpol is. I don't see this change in your patch, there's no cpha in the before. > > Signed-off-by: Guillaume Stols <gstols@baylibre.com> > --- > .../devicetree/bindings/iio/adc/adi,ad7606.yaml | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > index 75334a033539..12995ebcddc2 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > @@ -112,18 +112,32 @@ properties: > assumed that the pins are hardwired to VDD. > type: boolean > > + parallel-interface: > + description: > + If the parallel interface is used, be it directly or through a backend, > + this property must be defined. > + type: boolean The type you would want here is actually "flag", but I'm not sure why a property is needed. If you're using the parallel interface, why would you still be on a spi bus? I think I'm a bit confused here as to how this interface is supposed to be used. Thanks, Conor. > + > required: > - compatible > - reg > - - spi-cpol > - avcc-supply > - vdrive-supply > - interrupts > - adi,conversion-start-gpios > > -allOf: > - - $ref: /schemas/spi/spi-peripheral-props.yaml# > +oneOf: > + - required: > + - parallel-interface > + - allOf: > + - properties: > + parallel-interface: false > + spi-cpol: true > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + - required: > + - spi-cpol > > +allOf: > - if: > properties: > compatible: > > -- > 2.34.1 >
On Fri, Sep 20, 2024 at 05:33:23PM +0000, Guillaume Stols wrote: > Add the required properties for iio-backend support, as well as an > example and the conditions to mutually exclude interruption and > conversion trigger with iio-backend. > The iio-backend's function is to controls the communication, and thus the > interruption pin won't be available anymore. > As a consequence, the conversion pin must be controlled externally since > we will miss information about when every single conversion cycle (i.e > conversion + data transfer) ends, hence a PWM is introduced to trigger > the conversions. > > Signed-off-by: Guillaume Stols <gstols@baylibre.com> > --- > .../devicetree/bindings/iio/adc/adi,ad7606.yaml | 76 +++++++++++++++++++++- > 1 file changed, 74 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > index 12995ebcddc2..74a8680904b1 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > @@ -118,13 +118,32 @@ properties: > this property must be defined. > type: boolean > > + pwms: > + description: > + In case the conversion is triggered by a PWM instead of a GPIO plugged to > + the CONVST pin, the PWM must be referenced. > + minItems: 1 > + maxItems: 2 Please use an items list to describe what each item is, rather than doing so in the pwm-names description below. > + > + pwm-names: > + description: > + The name of each PWM, the first is connected to CONVST, and the second is > + connected to CONVST2 if CONVST2 is available and not connected to CONVST1. > + minItems: 1 > + maxItems: 2 You need to define what the names actually are, otherwise you have no ABI. Cheers, Conor. > + > + io-backends: > + description: > + A reference to the iio-backend, which is responsible handling the BUSY > + pin's falling edge and communication. > + An example of backend can be found at > + http://analogdevicesinc.github.io/hdl/library/axi_ad7606x/index.html > + > required: > - compatible > - reg > - avcc-supply > - vdrive-supply > - - interrupts > - - adi,conversion-start-gpios > > oneOf: > - required: > @@ -138,6 +157,34 @@ oneOf: > - spi-cpol > > allOf: > + - if: > + properties: > + pwms: false > + then: > + required: > + - adi,conversion-start-gpios > + > + - if: > + properties: > + adi,conversion-start-gpios: false > + then: > + required: > + - pwms > + > + - if: > + properties: > + interrupts: false > + then: > + required: > + - io-backends > + > + - if: > + properties: > + io-backends: false > + then: > + required: > + - interrupts > + > - if: > properties: > compatible: > @@ -179,12 +226,37 @@ allOf: > adi,sw-mode: false > else: > properties: > + pwms: > + maxItems: 1 > + pwm-names: > + maxItems: 1 > adi,conversion-start-gpios: > maxItems: 1 > > unevaluatedProperties: false > > examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + / { > + adi_adc { > + compatible = "adi,ad7606b"; > + parallel-interface; > + pwms = <&axi_pwm_gen 0 0>; > + > + avcc-supply = <&adc_vref>; > + vdrive-supply = <&vdd_supply>; > + > + reset-gpios = <&gpio0 91 GPIO_ACTIVE_HIGH>; > + standby-gpios = <&gpio0 90 GPIO_ACTIVE_LOW>; > + adi,range-gpios = <&gpio0 89 GPIO_ACTIVE_HIGH>; > + adi,oversampling-ratio-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH > + &gpio0 87 GPIO_ACTIVE_HIGH > + &gpio0 86 GPIO_ACTIVE_HIGH>; > + io-backends = <&iio_backend>; > + }; > + }; > + > - | > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/interrupt-controller/irq.h> > > -- > 2.34.1 >
On 9/21/24 23:55, Conor Dooley wrote: > On Fri, Sep 20, 2024 at 05:33:22PM +0000, Guillaume Stols wrote: >> The SPI conditions are not always required, because there is also a >> parallel interface. The way used to detect that the SPI interface is >> used is to check if the reg value is between 0 and 256. > And, yaknow, not that the bus you're on is a spi bus? I don't think this > comment is relevant to the binding, especially given you have a property > for it. Apologies, I missed to change the commit message, it will be fixed in the next series. Since Jonathan did not like very much inferring the interface with the reg's value that I used i the previous verison, I introduced this flag. However this is only intended to be use in bindings, to determine whether or not spi properties should be added. In the driver side of things, the bus interface is inferred by the parent's node (SPI driver is an module_spi_driver while parallel driver is module_platform_driver). > >> There is also a correction on the spi-cpha that is not required when SPI >> interface is selected, while spi-cpol is. > I don't see this change in your patch, there's no cpha in the before. > Again a problem with the commit message, this belongs now to another commit. >> Signed-off-by: Guillaume Stols <gstols@baylibre.com> >> --- >> .../devicetree/bindings/iio/adc/adi,ad7606.yaml | 20 +++++++++++++++++--- >> 1 file changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml >> index 75334a033539..12995ebcddc2 100644 >> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml >> @@ -112,18 +112,32 @@ properties: >> assumed that the pins are hardwired to VDD. >> type: boolean >> >> + parallel-interface: >> + description: >> + If the parallel interface is used, be it directly or through a backend, >> + this property must be defined. >> + type: boolean > The type you would want here is actually "flag", but I'm not sure why a > property is needed. If you're using the parallel interface, why would > you still be on a spi bus? I think I'm a bit confused here as to how > this interface is supposed to be used. > > Thanks, > Conor. > >> + >> required: >> - compatible >> - reg >> - - spi-cpol >> - avcc-supply >> - vdrive-supply >> - interrupts >> - adi,conversion-start-gpios >> >> -allOf: >> - - $ref: /schemas/spi/spi-peripheral-props.yaml# >> +oneOf: >> + - required: >> + - parallel-interface >> + - allOf: >> + - properties: >> + parallel-interface: false >> + spi-cpol: true >> + - $ref: /schemas/spi/spi-peripheral-props.yaml# >> + - required: >> + - spi-cpol >> >> +allOf: >> - if: >> properties: >> compatible: >> >> -- >> 2.34.1 >>
On Tue, Sep 24, 2024 at 04:41:50PM +0200, Guillaume Stols wrote: > > On 9/21/24 23:55, Conor Dooley wrote: > > On Fri, Sep 20, 2024 at 05:33:22PM +0000, Guillaume Stols wrote: > > > The SPI conditions are not always required, because there is also a > > > parallel interface. The way used to detect that the SPI interface is > > > used is to check if the reg value is between 0 and 256. > > And, yaknow, not that the bus you're on is a spi bus? I don't think this > > comment is relevant to the binding, especially given you have a property > > for it. > > Apologies, I missed to change the commit message, it will be fixed in the > next series. > > Since Jonathan did not like very much inferring the interface with the reg's > value that I used i the previous verison, I introduced this flag. > > However this is only intended to be use in bindings, to determine whether or > not spi properties should be added. To be honest, if it is not needed by software to understand what bus the device is on, it shouldn't be in the bindings at all. What was Jonathan opposed to? Doing an if reg < 1000: do y, otherwise do x? I'd not bother with any of that, and just make cpha (or w/e it was) optional with a description explaining the circumstances in which is it needed. > In the driver side of things, the bus interface is inferred by the parent's > node (SPI driver is an module_spi_driver while parallel driver is > module_platform_driver).
On 9/24/24 16:59, Conor Dooley wrote: > On Tue, Sep 24, 2024 at 04:41:50PM +0200, Guillaume Stols wrote: >> On 9/21/24 23:55, Conor Dooley wrote: >>> On Fri, Sep 20, 2024 at 05:33:22PM +0000, Guillaume Stols wrote: >>>> The SPI conditions are not always required, because there is also a >>>> parallel interface. The way used to detect that the SPI interface is >>>> used is to check if the reg value is between 0 and 256. >>> And, yaknow, not that the bus you're on is a spi bus? I don't think this >>> comment is relevant to the binding, especially given you have a property >>> for it. >> Apologies, I missed to change the commit message, it will be fixed in the >> next series. >> >> Since Jonathan did not like very much inferring the interface with the reg's >> value that I used i the previous verison, I introduced this flag. >> >> However this is only intended to be use in bindings, to determine whether or >> not spi properties should be added. > To be honest, if it is not needed by software to understand what bus the > device is on, it shouldn't be in the bindings at all. What was Jonathan > opposed to? Doing an if reg < 1000: do y, otherwise do x? > I'd not bother with any of that, and just make cpha (or w/e it was) > optional with a description explaining the circumstances in which is it > needed. OK, it will be removed from the series and sent as a side patch because it anyways does not really belong to this series. >> In the driver side of things, the bus interface is inferred by the parent's >> node (SPI driver is an module_spi_driver while parallel driver is >> module_platform_driver).
On Wed, 25 Sep 2024 17:28:30 +0200 Guillaume Stols <gstols@baylibre.com> wrote: > On 9/24/24 16:59, Conor Dooley wrote: > > On Tue, Sep 24, 2024 at 04:41:50PM +0200, Guillaume Stols wrote: > >> On 9/21/24 23:55, Conor Dooley wrote: > >>> On Fri, Sep 20, 2024 at 05:33:22PM +0000, Guillaume Stols wrote: > >>>> The SPI conditions are not always required, because there is also a > >>>> parallel interface. The way used to detect that the SPI interface is > >>>> used is to check if the reg value is between 0 and 256. > >>> And, yaknow, not that the bus you're on is a spi bus? I don't think this > >>> comment is relevant to the binding, especially given you have a property > >>> for it. > >> Apologies, I missed to change the commit message, it will be fixed in the > >> next series. > >> > >> Since Jonathan did not like very much inferring the interface with the reg's > >> value that I used i the previous verison, I introduced this flag. > >> > >> However this is only intended to be use in bindings, to determine whether or > >> not spi properties should be added. > > To be honest, if it is not needed by software to understand what bus the > > device is on, it shouldn't be in the bindings at all. What was Jonathan > > opposed to? Doing an if reg < 1000: do y, otherwise do x? > > I'd not bother with any of that, and just make cpha (or w/e it was) > > optional with a description explaining the circumstances in which is it > > needed. > OK, it will be removed from the series and sent as a side patch because > it anyways does not really belong to this series. I can't remember the original thread (or immediately find it). So I may have this totally wrong. - I don't want checks on reg value to change how the binding works as that is a wieird corner and in theory this device could be at a small address anyway. - Fine to do as Conor suggests and just add a comment for this corner case rather than making it required. Jonathan > >> In the driver side of things, the bus interface is inferred by the parent's > >> node (SPI driver is an module_spi_driver while parallel driver is > >> module_platform_driver).
This series aims to add iio backend support for AD7606X ADCs. In a nutshell, iio backend is a paradigm to shift the logic establishing the connexion between iio buffers and backend buffers into the backend's driver. This provides a more stable programming interface to the driver developers, and give more flexibility in the way the hardware communicates. The support will be first added on AD7606B, and on next patches AD7606C16 and AD7606C18 will be added. The series have been tested on a Zedboard, using the latest HDL available, i.e https://github.com/analogdevicesinc/hdl/commit/7d0a4cee1b5fa403f175af513d7eb804c3bd75d0 and an AD7606B FMCZ EKV. This HDL handles both the conversion trigger (through a PWM), and the end of conversion interruption, and is compatible with axi-adc, which is "iio-backendable". More information about this HDL design can be found at: https://wiki.analog.com/resources/eval/user-guides/ad7606x-fmc/hdl The support is thus separated in two parts: - PWM support was first added. My first intention was to make it available for any version of the driver, but the time required to handle the interruption is not neglectable, and I saw drifts that would eventually cause an overlapping SPI read with a new conversion trigger, whith catastrphic consequences. To mitigate this, CRC check must be implemented, but indeed increasing the samplerate causes more sample to be lost. Therefore, I decided to only allow PWM for iio-backend powered device as a first intention, leaving open the possibility to add the general compatibility afterwards. - IIO backend support was added: Once the PWM support was ready, the driver can be extended to iio-backend. The iio-backend powered version of the driver is a platform driver, and an exemple devicetree node is available in the bindings. The following features will be added in subsequent patch series: - software mode for iio backend - 18 bits mode (AD7606C18) - single read (IIO_CHAN_READ_RAW) Signed-off-by: Guillaume Stols <gstols@baylibre.com> --- Changes in v2: - Logical change in dt-bindings, using a flag for the interface instead of infering it from the value of the "reg" property. - Removal of get_platform_match_data addition, instead the logic is directly used in the file. - Removal of use and export of pwm_get_state_hw, returning the configured frequency instead of the running one. - Correction on various typos, whitespaces, bad order of includes. - Separation of SPI conditions and PWM disabling for no backend in other commits. - Link to v1: https://lore.kernel.org/r/20240815-ad7606_add_iio_backend_support-v1-0-cea3e11b1aa4@baylibre.com --- Guillaume Stols (10): dt-bindings: iio: adc: ad7606: Set the correct polarity dt-bindings: iio: adc: ad7606: Make corrections on spi conditions dt-bindings: iio: adc: ad7606: Add iio backend bindings Documentation: iio: Document ad7606 driver iio: adc: ad7606: Sort includes in alphabetical order iio: adc: ad7606: Add PWM support for conversion trigger iio: adc: ad7606: Add compatibility to fw_nodes iio: adc: ad7606: Fix typo in the driver name iio: adc: ad7606: Add iio-backend support iio: adc: ad7606: Disable PWM usage for non backend version .../devicetree/bindings/iio/adc/adi,ad7606.yaml | 97 ++++- Documentation/iio/ad7606.rst | 143 +++++++ drivers/iio/adc/Kconfig | 4 +- drivers/iio/adc/ad7606.c | 474 +++++++++++++++------ drivers/iio/adc/ad7606.h | 51 ++- drivers/iio/adc/ad7606_par.c | 126 +++++- drivers/iio/adc/ad7606_spi.c | 33 +- 7 files changed, 749 insertions(+), 179 deletions(-) --- base-commit: 8bea3878a1511bceadc2fbf284b00bcc5a2ef28d change-id: 20240725-ad7606_add_iio_backend_support-c401305a6924 Best regards, -- Guillaume Stols <gstols@baylibre.com>