| Message ID | 20240920-ad7606_add_iio_backend_support-v2-2-0e78782ae7d0@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| 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 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).
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 + 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:
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. There is also a correction on the spi-cpha that is not required when SPI interface is selected, while spi-cpol is. Signed-off-by: Guillaume Stols <gstols@baylibre.com> --- .../devicetree/bindings/iio/adc/adi,ad7606.yaml | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)