| Message ID | f3972e6aa4ff3869ded1f0dbeb58c43b824b3932.1737985435.git.Jonathan.Santos@analog.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add features, improvements, and fixes | expand |
On Mon, 27 Jan 2025 12:11:30 -0300, Jonathan Santos wrote: > Add a new trigger-sources property to enable synchronization across > multiple devices. This property references the main device (or > trigger provider) responsible for generating the pulse to drive the > SYNC_IN of all devices in the setup. > > In addition to GPIO synchronization, The AD7768-1 also supports > synchronization over SPI, which use is recommended when the GPIO > cannot provide a pulse synchronous with the base MCLK signal. It > consists of looping back the SYNC_OUT to the SYNC_IN pin and send > a command via SPI to trigger the synchronization. > > SPI-based synchronization is enabled in the absence of adi,sync-in-gpios > property. Since adi,sync-in-gpios is not long the only method, remove it > from required properties. > > While at it, add description to the interrupt property. > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > --- > v2 Changes: > * Patch added as replacement for adi,sync-in-spi patch. > * addressed the request for a description to interrupts property. > --- > .../bindings/iio/adc/adi,ad7768-1.yaml | 22 +++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml:84:6: [error] missing starting space in comment (comments) dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml: ignoring, error in schema: required: 8 /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml: required:8: None is not of type 'string' from schema $id: http://json-schema.org/draft-07/schema# Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.example.dtb: /example-0/spi/adc@0: failed to match any schema with compatible: ['adi,ad7768-1'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/f3972e6aa4ff3869ded1f0dbeb58c43b824b3932.1737985435.git.Jonathan.Santos@analog.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Mon, Jan 27, 2025 at 12:11:30PM -0300, Jonathan Santos wrote: > Add a new trigger-sources property to enable synchronization across > multiple devices. This property references the main device (or > trigger provider) responsible for generating the pulse to drive the > SYNC_IN of all devices in the setup. > > In addition to GPIO synchronization, The AD7768-1 also supports > synchronization over SPI, which use is recommended when the GPIO > cannot provide a pulse synchronous with the base MCLK signal. It > consists of looping back the SYNC_OUT to the SYNC_IN pin and send > a command via SPI to trigger the synchronization. > > SPI-based synchronization is enabled in the absence of adi,sync-in-gpios > property. Since adi,sync-in-gpios is not long the only method, remove it > from required properties. > > While at it, add description to the interrupt property. interrupts > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > --- > v2 Changes: > * Patch added as replacement for adi,sync-in-spi patch. > * addressed the request for a description to interrupts property. > --- > .../bindings/iio/adc/adi,ad7768-1.yaml | 22 +++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > index 3ce59d4d065f..3e119cf1754b 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > @@ -26,7 +26,17 @@ properties: > clock-names: > const: mclk > > + trigger-sources: > + description: > + References the main device responsible for synchronization. In a single > + device setup, reference the own node. > + maxItems: 1 > + > interrupts: > + description: > + Specifies the interrupt line associated with the ADC. This refers > + to the DRDY (Data Ready) pin, which signals when conversion results are > + available. > maxItems: 1 > > '#address-cells': > @@ -46,6 +56,8 @@ properties: > sampling. A pulse is always required if the configuration is changed > in any way, for example if the filter decimation rate changes. > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > + In the absence of this property, Synchronization over SPI will be > + enabled. > > reset-gpios: > maxItems: 1 > @@ -57,6 +69,9 @@ properties: > "#io-channel-cells": > const: 1 > > + "#trigger-source-cells": > + const: 0 > + > required: > - compatible > - reg > @@ -65,7 +80,8 @@ required: > - vref-supply > - spi-cpol > - spi-cpha > - - adi,sync-in-gpios > + - trigger-sources > + - #trigger-source-cells You need quotes here. This device worked before without these properties, so why are they required? That's an ABI change. > > patternProperties: > "^channel@([0-9]|1[0-5])$": > @@ -99,7 +115,7 @@ examples: > #address-cells = <1>; > #size-cells = <0>; > > - adc@0 { > + adc0: adc@0 { > compatible = "adi,ad7768-1"; > reg = <0>; > spi-max-frequency = <2000000>; > @@ -109,6 +125,8 @@ examples: > interrupts = <25 IRQ_TYPE_EDGE_RISING>; > interrupt-parent = <&gpio>; > adi,sync-in-gpios = <&gpio 22 GPIO_ACTIVE_LOW>; > + trigger-sources = <&adc0>; > + #trigger-source-cells = <0>; > reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; > clocks = <&ad7768_mclk>; > clock-names = "mclk"; > -- > 2.34.1 >
On 1/27/25 9:11 AM, Jonathan Santos wrote: > Add a new trigger-sources property to enable synchronization across > multiple devices. This property references the main device (or > trigger provider) responsible for generating the pulse to drive the > SYNC_IN of all devices in the setup. > > In addition to GPIO synchronization, The AD7768-1 also supports > synchronization over SPI, which use is recommended when the GPIO > cannot provide a pulse synchronous with the base MCLK signal. It > consists of looping back the SYNC_OUT to the SYNC_IN pin and send > a command via SPI to trigger the synchronization. > > SPI-based synchronization is enabled in the absence of adi,sync-in-gpios > property. Since adi,sync-in-gpios is not long the only method, remove it > from required properties. > > While at it, add description to the interrupt property. > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > --- > v2 Changes: > * Patch added as replacement for adi,sync-in-spi patch. > * addressed the request for a description to interrupts property. > --- > .../bindings/iio/adc/adi,ad7768-1.yaml | 22 +++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > index 3ce59d4d065f..3e119cf1754b 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > @@ -26,7 +26,17 @@ properties: > clock-names: > const: mclk > > + trigger-sources: > + description: > + References the main device responsible for synchronization. In a single > + device setup, reference the own node. > + maxItems: 1 We probably actually need 2 here. One for /SYNC_IN and one for a GPIO3 pin acting as the /START signal. > + > interrupts: > + description: > + Specifies the interrupt line associated with the ADC. This refers > + to the DRDY (Data Ready) pin, which signals when conversion results are > + available. > maxItems: 1 > > '#address-cells': > @@ -46,6 +56,8 @@ properties: > sampling. A pulse is always required if the configuration is changed > in any way, for example if the filter decimation rate changes. > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > + In the absence of this property, Synchronization over SPI will be > + enabled. Isn't /SYNC_OUT connected to /SYNC_IN required for synchronization over SPI? If yes, instead of adding this text, I would make the binding have: oneOf: - required: - trigger-sources - required: - adi,sync-in-gpios > > reset-gpios: > maxItems: 1 > @@ -57,6 +69,9 @@ properties: > "#io-channel-cells": > const: 1 > > + "#trigger-source-cells": > + const: 0 > + > required: > - compatible > - reg > @@ -65,7 +80,8 @@ required: > - vref-supply > - spi-cpol > - spi-cpha > - - adi,sync-in-gpios > + - trigger-sources > + - #trigger-source-cells > > patternProperties: > "^channel@([0-9]|1[0-5])$": > @@ -99,7 +115,7 @@ examples: > #address-cells = <1>; > #size-cells = <0>; > > - adc@0 { > + adc0: adc@0 { > compatible = "adi,ad7768-1"; > reg = <0>; > spi-max-frequency = <2000000>; > @@ -109,6 +125,8 @@ examples: > interrupts = <25 IRQ_TYPE_EDGE_RISING>; > interrupt-parent = <&gpio>; > adi,sync-in-gpios = <&gpio 22 GPIO_ACTIVE_LOW>; Don't we need to drop adi,sync-in-gpios here? I don't think we would have two things connected to /SYNC_IN at the same time. > + trigger-sources = <&adc0>; > + #trigger-source-cells = <0>; > reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; > clocks = <&ad7768_mclk>; > clock-names = "mclk";
On 01/27, David Lechner wrote: > On 1/27/25 9:11 AM, Jonathan Santos wrote: > > Add a new trigger-sources property to enable synchronization across > > multiple devices. This property references the main device (or > > trigger provider) responsible for generating the pulse to drive the > > SYNC_IN of all devices in the setup. > > > > In addition to GPIO synchronization, The AD7768-1 also supports > > synchronization over SPI, which use is recommended when the GPIO > > cannot provide a pulse synchronous with the base MCLK signal. It > > consists of looping back the SYNC_OUT to the SYNC_IN pin and send > > a command via SPI to trigger the synchronization. > > > > SPI-based synchronization is enabled in the absence of adi,sync-in-gpios > > property. Since adi,sync-in-gpios is not long the only method, remove it > > from required properties. > > > > While at it, add description to the interrupt property. > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > --- > > v2 Changes: > > * Patch added as replacement for adi,sync-in-spi patch. > > * addressed the request for a description to interrupts property. > > --- > > .../bindings/iio/adc/adi,ad7768-1.yaml | 22 +++++++++++++++++-- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > index 3ce59d4d065f..3e119cf1754b 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > @@ -26,7 +26,17 @@ properties: > > clock-names: > > const: mclk > > > > + trigger-sources: > > + description: > > + References the main device responsible for synchronization. In a single > > + device setup, reference the own node. > > + maxItems: 1 > > We probably actually need 2 here. One for /SYNC_IN and one for a GPIO3 pin > acting as the /START signal. > > > + > > interrupts: > > + description: > > + Specifies the interrupt line associated with the ADC. This refers > > + to the DRDY (Data Ready) pin, which signals when conversion results are > > + available. > > maxItems: 1 > > > > '#address-cells': > > @@ -46,6 +56,8 @@ properties: > > sampling. A pulse is always required if the configuration is changed > > in any way, for example if the filter decimation rate changes. > > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > + In the absence of this property, Synchronization over SPI will be > > + enabled. > > Isn't /SYNC_OUT connected to /SYNC_IN required for synchronization over SPI? > > If yes, instead of adding this text, I would make the binding have: > Yes, but the synchronization over SPI is enabled in the absence of the GPIO. The trigger-sources property would indicate if the sync provider is the own device or not. As i said below, maybe i misunderstood. > oneOf: > - required: > - trigger-sources > - required: > - adi,sync-in-gpios > Wouldn't be simpler to consider the absence of sync-in-gpio? this way we have less changes in the ABI. > > > > reset-gpios: > > maxItems: 1 > > @@ -57,6 +69,9 @@ properties: > > "#io-channel-cells": > > const: 1 > > > > + "#trigger-source-cells": > > + const: 0 > > + > > required: > > - compatible > > - reg > > @@ -65,7 +80,8 @@ required: > > - vref-supply > > - spi-cpol > > - spi-cpha > > - - adi,sync-in-gpios > > + - trigger-sources > > + - #trigger-source-cells > > > > patternProperties: > > "^channel@([0-9]|1[0-5])$": > > @@ -99,7 +115,7 @@ examples: > > #address-cells = <1>; > > #size-cells = <0>; > > > > - adc@0 { > > + adc0: adc@0 { > > compatible = "adi,ad7768-1"; > > reg = <0>; > > spi-max-frequency = <2000000>; > > @@ -109,6 +125,8 @@ examples: > > interrupts = <25 IRQ_TYPE_EDGE_RISING>; > > interrupt-parent = <&gpio>; > > adi,sync-in-gpios = <&gpio 22 GPIO_ACTIVE_LOW>; > > Don't we need to drop adi,sync-in-gpios here? I don't think we would have two > things connected to /SYNC_IN at the same time. > I guess i misunderstood the use of trigger-sources. I thought it would indicate the trigger provider or main device. Like if it points to other device we should use it to drive the SYNC_IN of all devices. Then what happens if the trigger-sources points to other node? we would't be able to driver the SYNC_IN in case of any configuration change? > > + trigger-sources = <&adc0>; > > + #trigger-source-cells = <0>; > > reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; > > clocks = <&ad7768_mclk>; > > clock-names = "mclk"; >
On 1/28/25 9:04 AM, Jonathan Santos wrote: > On 01/27, David Lechner wrote: >> On 1/27/25 9:11 AM, Jonathan Santos wrote: >>> Add a new trigger-sources property to enable synchronization across >>> multiple devices. This property references the main device (or >>> trigger provider) responsible for generating the pulse to drive the >>> SYNC_IN of all devices in the setup. >>> >>> In addition to GPIO synchronization, The AD7768-1 also supports >>> synchronization over SPI, which use is recommended when the GPIO >>> cannot provide a pulse synchronous with the base MCLK signal. It >>> consists of looping back the SYNC_OUT to the SYNC_IN pin and send >>> a command via SPI to trigger the synchronization. >>> >>> SPI-based synchronization is enabled in the absence of adi,sync-in-gpios >>> property. Since adi,sync-in-gpios is not long the only method, remove it >>> from required properties. >>> >>> While at it, add description to the interrupt property. >>> >>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> >>> --- >>> v2 Changes: >>> * Patch added as replacement for adi,sync-in-spi patch. >>> * addressed the request for a description to interrupts property. >>> --- >>> .../bindings/iio/adc/adi,ad7768-1.yaml | 22 +++++++++++++++++-- >>> 1 file changed, 20 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml >>> index 3ce59d4d065f..3e119cf1754b 100644 >>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml >>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml >>> @@ -26,7 +26,17 @@ properties: >>> clock-names: >>> const: mclk >>> >>> + trigger-sources: >>> + description: >>> + References the main device responsible for synchronization. In a single >>> + device setup, reference the own node. >>> + maxItems: 1 >> >> We probably actually need 2 here. One for /SYNC_IN and one for a GPIO3 pin >> acting as the /START signal. >> >>> + >>> interrupts: >>> + description: >>> + Specifies the interrupt line associated with the ADC. This refers >>> + to the DRDY (Data Ready) pin, which signals when conversion results are >>> + available. >>> maxItems: 1 >>> >>> '#address-cells': >>> @@ -46,6 +56,8 @@ properties: >>> sampling. A pulse is always required if the configuration is changed >>> in any way, for example if the filter decimation rate changes. >>> As the line is active low, it should be marked GPIO_ACTIVE_LOW. >>> + In the absence of this property, Synchronization over SPI will be >>> + enabled. >> >> Isn't /SYNC_OUT connected to /SYNC_IN required for synchronization over SPI? >> >> If yes, instead of adding this text, I would make the binding have: >> > > Yes, but the synchronization over SPI is enabled in the absence of the GPIO. > The trigger-sources property would indicate if the sync provider is the > own device or not. As i said below, maybe i misunderstood. > >> oneOf: >> - required: >> - trigger-sources >> - required: >> - adi,sync-in-gpios >> > > Wouldn't be simpler to consider the absence of sync-in-gpio? this way we > have less changes in the ABI. Maybe it is me that missed something, but if I'm reading the datasheet correctly, then sync over SPI only works if /SYNC_IN is wired to /SYNC_OUT. And the chip isn't going to work correctly without some sort of sync. So we need something wired to /SYNC_IN no matter what. In any case, the DT bindings should just say how the chip is wired up and not dictate how the driver should behave. So what I was going for with this is to have the bindings say that something has to be wired to /SYNC_IN and we can leave it up to the driver to decide what to do with this information. > >>> >>> reset-gpios: >>> maxItems: 1 >>> @@ -57,6 +69,9 @@ properties: >>> "#io-channel-cells": >>> const: 1 >>> >>> + "#trigger-source-cells": >>> + const: 0 >>> + >>> required: >>> - compatible >>> - reg >>> @@ -65,7 +80,8 @@ required: >>> - vref-supply >>> - spi-cpol >>> - spi-cpha >>> - - adi,sync-in-gpios >>> + - trigger-sources >>> + - #trigger-source-cells >>> >>> patternProperties: >>> "^channel@([0-9]|1[0-5])$": >>> @@ -99,7 +115,7 @@ examples: >>> #address-cells = <1>; >>> #size-cells = <0>; >>> >>> - adc@0 { >>> + adc0: adc@0 { >>> compatible = "adi,ad7768-1"; >>> reg = <0>; >>> spi-max-frequency = <2000000>; >>> @@ -109,6 +125,8 @@ examples: >>> interrupts = <25 IRQ_TYPE_EDGE_RISING>; >>> interrupt-parent = <&gpio>; >>> adi,sync-in-gpios = <&gpio 22 GPIO_ACTIVE_LOW>; >> >> Don't we need to drop adi,sync-in-gpios here? I don't think we would have two >> things connected to /SYNC_IN at the same time. >> > > I guess i misunderstood the use of trigger-sources. I thought it would > indicate the trigger provider or main device. Like if it points to other > device we should use it to drive the SYNC_IN of all devices. > > Then what happens if the trigger-sources points to other node? we would't be > able to driver the SYNC_IN in case of any configuration change? I think you understand the trigger-source bindings correctly. The driver doesn't have to support everything that the DT bindings allow. This series is big enough already, so we can defer figuring out how to implement triggers other than the loopback case later. :-) We just want to make the DT bindings as complete as we can now. > >>> + trigger-sources = <&adc0>; >>> + #trigger-source-cells = <0>; >>> reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; >>> clocks = <&ad7768_mclk>; >>> clock-names = "mclk"; >>
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml index 3ce59d4d065f..3e119cf1754b 100644 --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml @@ -26,7 +26,17 @@ properties: clock-names: const: mclk + trigger-sources: + description: + References the main device responsible for synchronization. In a single + device setup, reference the own node. + maxItems: 1 + interrupts: + description: + Specifies the interrupt line associated with the ADC. This refers + to the DRDY (Data Ready) pin, which signals when conversion results are + available. maxItems: 1 '#address-cells': @@ -46,6 +56,8 @@ properties: sampling. A pulse is always required if the configuration is changed in any way, for example if the filter decimation rate changes. As the line is active low, it should be marked GPIO_ACTIVE_LOW. + In the absence of this property, Synchronization over SPI will be + enabled. reset-gpios: maxItems: 1 @@ -57,6 +69,9 @@ properties: "#io-channel-cells": const: 1 + "#trigger-source-cells": + const: 0 + required: - compatible - reg @@ -65,7 +80,8 @@ required: - vref-supply - spi-cpol - spi-cpha - - adi,sync-in-gpios + - trigger-sources + - #trigger-source-cells patternProperties: "^channel@([0-9]|1[0-5])$": @@ -99,7 +115,7 @@ examples: #address-cells = <1>; #size-cells = <0>; - adc@0 { + adc0: adc@0 { compatible = "adi,ad7768-1"; reg = <0>; spi-max-frequency = <2000000>; @@ -109,6 +125,8 @@ examples: interrupts = <25 IRQ_TYPE_EDGE_RISING>; interrupt-parent = <&gpio>; adi,sync-in-gpios = <&gpio 22 GPIO_ACTIVE_LOW>; + trigger-sources = <&adc0>; + #trigger-source-cells = <0>; reset-gpios = <&gpio 27 GPIO_ACTIVE_LOW>; clocks = <&ad7768_mclk>; clock-names = "mclk";
Add a new trigger-sources property to enable synchronization across multiple devices. This property references the main device (or trigger provider) responsible for generating the pulse to drive the SYNC_IN of all devices in the setup. In addition to GPIO synchronization, The AD7768-1 also supports synchronization over SPI, which use is recommended when the GPIO cannot provide a pulse synchronous with the base MCLK signal. It consists of looping back the SYNC_OUT to the SYNC_IN pin and send a command via SPI to trigger the synchronization. SPI-based synchronization is enabled in the absence of adi,sync-in-gpios property. Since adi,sync-in-gpios is not long the only method, remove it from required properties. While at it, add description to the interrupt property. Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> --- v2 Changes: * Patch added as replacement for adi,sync-in-spi patch. * addressed the request for a description to interrupts property. --- .../bindings/iio/adc/adi,ad7768-1.yaml | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)