| Message ID | bde43579b41199f0c17f07dfacefcb137028e66e.1736201898.git.Jonathan.Santos@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7768-1: Add features, improvements, and fixes | expand |
On 1/7/25 9:24 AM, Jonathan Santos wrote: > Add adi,sync-in-spi property to enable synchronization over SPI. > This should be used in the case when the GPIO cannot provide a > pulse synchronous with the base MCLK signal. > > User can choose between SPI, GPIO synchronization or neither of them, > but only if a external pulse can be provided, for example, by another > device in a multidevice setup. > While we are fixing up these bindings, we could add some more trivial things, like power supplies. Also, the interrupt property could use a description since the chip has multiple output pins. I assume it means the /DRDY pin? > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > --- > .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > index 3ce59d4d065f..55cec27bfe60 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > @@ -47,6 +47,15 @@ properties: > in any way, for example if the filter decimation rate changes. > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > + adi,sync-in-spi: If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name should be something like adi,sync-in-sync-out. SPI seems irrelevant here since we should just be describing how things are wired up, not how it is being used. But if we also need to consider the case where SYNC_OUT of one chip is connected to SYNC_IN of another chip, we might want to consider using trigger-source bindings instead (recently standardized in dtschema). > + description: > + Enables synchronization of multiple devices over SPI. This property is > + used when a signal synchronous to the base MCLK signal cannot be provided > + via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin > + on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device > + should be routed to the SYNC_IN pins of the other devices. > + type: boolean > + > reset-gpios: > maxItems: 1 > > @@ -65,7 +74,6 @@ required: > - vref-supply > - spi-cpol > - spi-cpha > - - adi,sync-in-gpios > > patternProperties: > "^channel@([0-9]|1[0-5])$": > @@ -89,6 +97,20 @@ patternProperties: > allOf: > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > + # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid) > + - if: > + required: > + - adi,sync-in-gpios > + then: > + properties: > + adi,sync-in-spi: false > + - if: > + required: > + - adi,sync-in-spi > + then: > + properties: > + adi,sync-in-gpios: false I think this can be simplified to using oneOf: to provide XOR validation > + > unevaluatedProperties: false > > examples:
On 01/07, Jonathan Santos wrote: > Add adi,sync-in-spi property to enable synchronization over SPI. > This should be used in the case when the GPIO cannot provide a > pulse synchronous with the base MCLK signal. > > User can choose between SPI, GPIO synchronization or neither of them, > but only if a external pulse can be provided, for example, by another > device in a multidevice setup. > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > --- > .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > index 3ce59d4d065f..55cec27bfe60 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > @@ -47,6 +47,15 @@ properties: > in any way, for example if the filter decimation rate changes. > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > + adi,sync-in-spi: > + description: > + Enables synchronization of multiple devices over SPI. This property is > + used when a signal synchronous to the base MCLK signal cannot be provided > + via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin > + on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device > + should be routed to the SYNC_IN pins of the other devices. So, if I'm getting it right, /SYNC_IN may be driven by a GPIO (ADAQ7768-1 datasheet Figure 131), /SYNC_IN may be driven by own device /SYNC_OUT (ADAQ7768-1 datasheet Figure 133), or /SYNC_IN may be driven by other AD7768-1 /SYNC_OUT pin (also Figure 133). That is too much to describe with a boolean. If David's suggestion of using a trigger-source doesn't fit, this property should at least become an enum or string. > + type: boolean > + > reset-gpios: > maxItems: 1 > > @@ -65,7 +74,6 @@ required: > - vref-supply > - spi-cpol > - spi-cpha > - - adi,sync-in-gpios > > patternProperties: > "^channel@([0-9]|1[0-5])$": > @@ -89,6 +97,20 @@ patternProperties: > allOf: > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > + # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid) > + - if: > + required: > + - adi,sync-in-gpios > + then: > + properties: > + adi,sync-in-spi: false > + - if: > + required: > + - adi,sync-in-spi > + then: > + properties: > + adi,sync-in-gpios: false > + > unevaluatedProperties: false > > examples: > -- > 2.34.1 >
On 01/07, David Lechner wrote: > On 1/7/25 9:24 AM, Jonathan Santos wrote: > > Add adi,sync-in-spi property to enable synchronization over SPI. > > This should be used in the case when the GPIO cannot provide a > > pulse synchronous with the base MCLK signal. > > > > User can choose between SPI, GPIO synchronization or neither of them, > > but only if a external pulse can be provided, for example, by another > > device in a multidevice setup. > > > > While we are fixing up these bindings, we could add some more trivial things, > like power supplies. > > Also, the interrupt property could use a description since the chip has multiple > output pins. I assume it means the /DRDY pin? > Right! Yes, the interrupt pin refers to the /DRDY. > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > --- > > .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > index 3ce59d4d065f..55cec27bfe60 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > @@ -47,6 +47,15 @@ properties: > > in any way, for example if the filter decimation rate changes. > > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > > > + adi,sync-in-spi: > > If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name > should be something like adi,sync-in-sync-out. SPI seems irrelevant here since > we should just be describing how things are wired up, not how it is being used. > > But if we also need to consider the case where SYNC_OUT of one chip is connected > to SYNC_IN of another chip, we might want to consider using trigger-source > bindings instead (recently standardized in dtschema). > Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it handle the non-GPIO case? While testing a multidevice setup, I found it simpler to have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT without referencing another device and makes simultaneous buffered reads easier. Maybe we could stick to synchronization within the chip for now. > > + description: > > + Enables synchronization of multiple devices over SPI. This property is > > + used when a signal synchronous to the base MCLK signal cannot be provided > > + via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin > > + on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device > > + should be routed to the SYNC_IN pins of the other devices. > > + type: boolean > > + > > reset-gpios: > > maxItems: 1 > > > > @@ -65,7 +74,6 @@ required: > > - vref-supply > > - spi-cpol > > - spi-cpha > > - - adi,sync-in-gpios > > > > patternProperties: > > "^channel@([0-9]|1[0-5])$": > > @@ -89,6 +97,20 @@ patternProperties: > > allOf: > > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > > > + # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid) > > + - if: > > + required: > > + - adi,sync-in-gpios > > + then: > > + properties: > > + adi,sync-in-spi: false > > + - if: > > + required: > > + - adi,sync-in-spi > > + then: > > + properties: > > + adi,sync-in-gpios: false > > I think this can be simplified to using oneOf: to provide XOR validation > Right! > > + > > unevaluatedProperties: false > > > > examples: >
On Fri, 10 Jan 2025 18:51:06 -0300 Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > On 01/07, Jonathan Santos wrote: > > Add adi,sync-in-spi property to enable synchronization over SPI. > > This should be used in the case when the GPIO cannot provide a > > pulse synchronous with the base MCLK signal. > > > > User can choose between SPI, GPIO synchronization or neither of them, > > but only if a external pulse can be provided, for example, by another > > device in a multidevice setup. > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > --- > > .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > index 3ce59d4d065f..55cec27bfe60 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > @@ -47,6 +47,15 @@ properties: > > in any way, for example if the filter decimation rate changes. > > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > > > + adi,sync-in-spi: > > + description: > > + Enables synchronization of multiple devices over SPI. This property is > > + used when a signal synchronous to the base MCLK signal cannot be provided > > + via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin > > + on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device > > + should be routed to the SYNC_IN pins of the other devices. > So, if I'm getting it right, Datasheet on this is indeed complex! >/SYNC_IN may be driven by a GPIO (ADAQ7768-1 datasheet Figure 131), For that we expose a gpio binding already. If that's present we know what is going on. >/SYNC_IN may be driven by own device /SYNC_OUT (ADAQ7768-1 datasheet Figure 133), This is the default - no information provided so it isn't wired externally. We don't normally bother to describe required chip to chip connections. I couldn't entirely figure out if this is 'required' if we aren't driving explicitly from GPIO or another chip but i think it is(?). >/SYNC_IN may be driven by other AD7768-1 > /SYNC_OUT pin (also Figure 133). This is only case we are about for sync in I think. As long as there isn't a valid 'not connected' It think we are fine with a boolean. > That is too much to describe with a boolean. > > If David's suggestion of using a trigger-source doesn't fit, this property > should at least become an enum or string. > > > + type: boolean > > + > > reset-gpios: > > maxItems: 1 > > > > @@ -65,7 +74,6 @@ required: > > - vref-supply > > - spi-cpol > > - spi-cpha > > - - adi,sync-in-gpios > > > > patternProperties: > > "^channel@([0-9]|1[0-5])$": > > @@ -89,6 +97,20 @@ patternProperties: > > allOf: > > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > > > + # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid) > > + - if: > > + required: > > + - adi,sync-in-gpios > > + then: > > + properties: > > + adi,sync-in-spi: false > > + - if: > > + required: > > + - adi,sync-in-spi > > + then: > > + properties: > > + adi,sync-in-gpios: false > > + > > unevaluatedProperties: false > > > > examples: > > -- > > 2.34.1 > >
On Sat, 11 Jan 2025 19:34:14 -0300 Jonathan Santos <jonath4nns@gmail.com> wrote: > On 01/07, David Lechner wrote: > > On 1/7/25 9:24 AM, Jonathan Santos wrote: > > > Add adi,sync-in-spi property to enable synchronization over SPI. > > > This should be used in the case when the GPIO cannot provide a > > > pulse synchronous with the base MCLK signal. > > > > > > User can choose between SPI, GPIO synchronization or neither of them, > > > but only if a external pulse can be provided, for example, by another > > > device in a multidevice setup. > > > > > > > While we are fixing up these bindings, we could add some more trivial things, > > like power supplies. > > > > Also, the interrupt property could use a description since the chip has multiple > > output pins. I assume it means the /DRDY pin? > > > > Right! Yes, the interrupt pin refers to the /DRDY. > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > > --- > > > .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > index 3ce59d4d065f..55cec27bfe60 100644 > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > @@ -47,6 +47,15 @@ properties: > > > in any way, for example if the filter decimation rate changes. > > > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > > > > > + adi,sync-in-spi: > > > > If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name > > should be something like adi,sync-in-sync-out. SPI seems irrelevant here since > > we should just be describing how things are wired up, not how it is being used. > > > > But if we also need to consider the case where SYNC_OUT of one chip is connected > > to SYNC_IN of another chip, we might want to consider using trigger-source > > bindings instead (recently standardized in dtschema). > > > > Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it > handle the non-GPIO case? While testing a multidevice setup, I found it simpler to > have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT > without referencing another device and makes simultaneous buffered reads easier. Daisy-chain mode (figure 131)? In that case we normally end up with a single presented device with a 'lot' of channels. (See the electric car style battery charging chips, those can be chained in very large numbers!) Probably similar for figure 133 (which is a dual SPI setup) as the SPI clock must be shared so we still see it over a single interface. If those are the only two options then keeping this within the driver is fine. For daisy chain there are examples in tree and it normally means we have to have a DT parameter that says how long the chain is, though we maybe can do that with per channel nodes as well if those make sense here. Jonathan > > Maybe we could stick to synchronization within the chip for now. > > > > + description: > > > + Enables synchronization of multiple devices over SPI. This property is > > > + used when a signal synchronous to the base MCLK signal cannot be provided > > > + via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin > > > + on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device > > > + should be routed to the SYNC_IN pins of the other devices. > > > + type: boolean > > > + > > > reset-gpios: > > > maxItems: 1 > > > > > > @@ -65,7 +74,6 @@ required: > > > - vref-supply > > > - spi-cpol > > > - spi-cpha > > > - - adi,sync-in-gpios > > > > > > patternProperties: > > > "^channel@([0-9]|1[0-5])$": > > > @@ -89,6 +97,20 @@ patternProperties: > > > allOf: > > > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > > > > > + # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid) > > > + - if: > > > + required: > > > + - adi,sync-in-gpios > > > + then: > > > + properties: > > > + adi,sync-in-spi: false > > > + - if: > > > + required: > > > + - adi,sync-in-spi > > > + then: > > > + properties: > > > + adi,sync-in-gpios: false > > > > I think this can be simplified to using oneOf: to provide XOR validation > > > > Right! > > > > + > > > unevaluatedProperties: false > > > > > > examples: > >
On 1/11/25 4:34 PM, Jonathan Santos wrote: > On 01/07, David Lechner wrote: >> On 1/7/25 9:24 AM, Jonathan Santos wrote: >>> Add adi,sync-in-spi property to enable synchronization over SPI. >>> This should be used in the case when the GPIO cannot provide a >>> pulse synchronous with the base MCLK signal. >>> >>> User can choose between SPI, GPIO synchronization or neither of them, >>> but only if a external pulse can be provided, for example, by another >>> device in a multidevice setup. >>> >> >> While we are fixing up these bindings, we could add some more trivial things, >> like power supplies. >> >> Also, the interrupt property could use a description since the chip has multiple >> output pins. I assume it means the /DRDY pin? >> > > Right! Yes, the interrupt pin refers to the /DRDY. > >>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> >>> --- >>> .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- >>> 1 file changed, 23 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml >>> index 3ce59d4d065f..55cec27bfe60 100644 >>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml >>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml >>> @@ -47,6 +47,15 @@ properties: >>> in any way, for example if the filter decimation rate changes. >>> As the line is active low, it should be marked GPIO_ACTIVE_LOW. >>> >>> + adi,sync-in-spi: >> >> If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name >> should be something like adi,sync-in-sync-out. SPI seems irrelevant here since >> we should just be describing how things are wired up, not how it is being used. >> >> But if we also need to consider the case where SYNC_OUT of one chip is connected >> to SYNC_IN of another chip, we might want to consider using trigger-source >> bindings instead (recently standardized in dtschema). >> > > Do you mean the trigger-sources used for LEDs? Sort of. There is a more general version of this binding in dt-schema now [1]. But it is essentially the same binding. [1]: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/trigger/trigger-source.yaml > I can try to see if it works, but would it handle the non-GPIO case? Since the ADC chip is both a trigger provider and a trigger consumer, it would actually have both properties. A chip that has SYNC_OUT wired to SYNC_IN would look something like this: adc1: adc@0 { ... properties: #trigger-source-cells = <0>; trigger-sources = <&adc1> } > While testing a multidevice setup, I found it simpler to > have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT > without referencing another device and makes simultaneous buffered reads easier. > > Maybe we could stick to synchronization within the chip for now. In the driver, sure. But for the DT bindings, we want to make sure it makes sense for future use cases as well since it can't be changed later.
On 01/12, Jonathan Cameron wrote: > On Sat, 11 Jan 2025 19:34:14 -0300 > Jonathan Santos <jonath4nns@gmail.com> wrote: > > > On 01/07, David Lechner wrote: > > > On 1/7/25 9:24 AM, Jonathan Santos wrote: > > > > Add adi,sync-in-spi property to enable synchronization over SPI. > > > > This should be used in the case when the GPIO cannot provide a > > > > pulse synchronous with the base MCLK signal. > > > > > > > > User can choose between SPI, GPIO synchronization or neither of them, > > > > but only if a external pulse can be provided, for example, by another > > > > device in a multidevice setup. > > > > > > > > > > While we are fixing up these bindings, we could add some more trivial things, > > > like power supplies. > > > > > > Also, the interrupt property could use a description since the chip has multiple > > > output pins. I assume it means the /DRDY pin? > > > > > > > Right! Yes, the interrupt pin refers to the /DRDY. > > > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > > > --- > > > > .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- > > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > > index 3ce59d4d065f..55cec27bfe60 100644 > > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > > @@ -47,6 +47,15 @@ properties: > > > > in any way, for example if the filter decimation rate changes. > > > > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > > > > > > > + adi,sync-in-spi: > > > > > > If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name > > > should be something like adi,sync-in-sync-out. SPI seems irrelevant here since > > > we should just be describing how things are wired up, not how it is being used. > > > > > > But if we also need to consider the case where SYNC_OUT of one chip is connected > > > to SYNC_IN of another chip, we might want to consider using trigger-source > > > bindings instead (recently standardized in dtschema). > > > > > > > Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it > > handle the non-GPIO case? While testing a multidevice setup, I found it simpler to > > have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT > > without referencing another device and makes simultaneous buffered reads easier. > > Daisy-chain mode (figure 131)? In that case we normally end up with a single presented device > with a 'lot' of channels. (See the electric car style battery charging chips, those can > be chained in very large numbers!) > Actually, it is more like Figure 133 , but the premise is similar. We have here a Quad setup. > Probably similar for figure 133 (which is a dual SPI setup) as the SPI clock must > be shared so we still see it over a single interface. > We could view them as a single device with multiple channels, and since the goal is to read them simultaneously with buffered reads, some parameters such as sampling frequency should be equal to all devices. However, there are some implications: If we do the above, we have limitations in the customization of the "channels", they would have the same filter, frequency modulator and scale (we plan to add support for ADAQ776x-1 series, which include PGA and AAF gain). To customize them separetely, we would need to assert only the corresponding chip select, which is only possible with different instances, as far as I know. > If those are the only two options then keeping this within the driver is fine. > For daisy chain there are examples in tree and it normally means we have to > have a DT parameter that says how long the chain is, though we maybe can > do that with per channel nodes as well if those make sense here. > > Jonathan > Those are the options in the datasheet and in hardware so far. I was considering other scenarios in case the user combine them differently. I believe keping within the driver covers the main cases. > > > > > Maybe we could stick to synchronization within the chip for now. > > > > > > + description: > > > > + Enables synchronization of multiple devices over SPI. This property is > > > > + used when a signal synchronous to the base MCLK signal cannot be provided > > > > + via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin > > > > + on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device > > > > + should be routed to the SYNC_IN pins of the other devices. > > > > + type: boolean > > > > + > > > > reset-gpios: > > > > maxItems: 1 > > > > > > > > @@ -65,7 +74,6 @@ required: > > > > - vref-supply > > > > - spi-cpol > > > > - spi-cpha > > > > - - adi,sync-in-gpios > > > > > > > > patternProperties: > > > > "^channel@([0-9]|1[0-5])$": > > > > @@ -89,6 +97,20 @@ patternProperties: > > > > allOf: > > > > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > > > > > > > + # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid) > > > > + - if: > > > > + required: > > > > + - adi,sync-in-gpios > > > > + then: > > > > + properties: > > > > + adi,sync-in-spi: false > > > > + - if: > > > > + required: > > > > + - adi,sync-in-spi > > > > + then: > > > > + properties: > > > > + adi,sync-in-gpios: false > > > > > > I think this can be simplified to using oneOf: to provide XOR validation > > > > > > > Right! > > > > > > + > > > > unevaluatedProperties: false > > > > > > > > examples: > > > >
On 01/12, Jonathan Cameron wrote: > On Fri, 10 Jan 2025 18:51:06 -0300 > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > > On 01/07, Jonathan Santos wrote: > > > Add adi,sync-in-spi property to enable synchronization over SPI. > > > This should be used in the case when the GPIO cannot provide a > > > pulse synchronous with the base MCLK signal. > > > > > > User can choose between SPI, GPIO synchronization or neither of them, > > > but only if a external pulse can be provided, for example, by another > > > device in a multidevice setup. > > > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > > --- > > > .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > index 3ce59d4d065f..55cec27bfe60 100644 > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > @@ -47,6 +47,15 @@ properties: > > > in any way, for example if the filter decimation rate changes. > > > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > > > > > + adi,sync-in-spi: > > > + description: > > > + Enables synchronization of multiple devices over SPI. This property is > > > + used when a signal synchronous to the base MCLK signal cannot be provided > > > + via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin > > > + on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device > > > + should be routed to the SYNC_IN pins of the other devices. > > So, if I'm getting it right, > > Datasheet on this is indeed complex! > > >/SYNC_IN may be driven by a GPIO (ADAQ7768-1 datasheet Figure 131), > > For that we expose a gpio binding already. If that's present we know what is going on. > > >/SYNC_IN may be driven by own device /SYNC_OUT (ADAQ7768-1 datasheet Figure 133), > > This is the default - no information provided so it isn't wired externally. > We don't normally bother to describe required chip to chip connections. > I couldn't entirely figure out if this is 'required' if we aren't driving explicitly > from GPIO or another chip but i think it is(?). > It is. If the device is not being driven by a GPIO or an external device, the user should connect the /SYNC_OUT to /SYNC_IN. We could assume that this is the case if the GPIO is not present in the devicetree, maybe put that into the description. The sync-out-sync-in property as a boolean can be quite redundant, since we also need to remove the GPIO. > >/SYNC_IN may be driven by other AD7768-1 > /SYNC_OUT pin (also Figure 133). > This is only case we are about for sync in I think. > > As long as there isn't a valid 'not connected' It think we are fine with a boolean. > If opt for a single node instace, thats ok; otherwise, David's trigger-sources suggestion seems to be the best approach. > > That is too much to describe with a boolean. > > > > If David's suggestion of using a trigger-source doesn't fit, this property > > should at least become an enum or string. > > > > > + type: boolean > > > + > > > reset-gpios: > > > maxItems: 1 > > > > > > @@ -65,7 +74,6 @@ required: > > > - vref-supply > > > - spi-cpol > > > - spi-cpha > > > - - adi,sync-in-gpios > > > > > > patternProperties: > > > "^channel@([0-9]|1[0-5])$": > > > @@ -89,6 +97,20 @@ patternProperties: > > > allOf: > > > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > > > > > + # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid) > > > + - if: > > > + required: > > > + - adi,sync-in-gpios > > > + then: > > > + properties: > > > + adi,sync-in-spi: false > > > + - if: > > > + required: > > > + - adi,sync-in-spi > > > + then: > > > + properties: > > > + adi,sync-in-gpios: false > > > + > > > unevaluatedProperties: false > > > > > > examples: > > > -- > > > 2.34.1 > > > >
On 1/13/25 6:18 PM, Jonathan Santos wrote: > On 01/12, Jonathan Cameron wrote: >> On Sat, 11 Jan 2025 19:34:14 -0300 >> Jonathan Santos <jonath4nns@gmail.com> wrote: >> >>> On 01/07, David Lechner wrote: >>>> On 1/7/25 9:24 AM, Jonathan Santos wrote: >>>>> Add adi,sync-in-spi property to enable synchronization over SPI. >>>>> This should be used in the case when the GPIO cannot provide a >>>>> pulse synchronous with the base MCLK signal. >>>>> >>>>> User can choose between SPI, GPIO synchronization or neither of them, >>>>> but only if a external pulse can be provided, for example, by another >>>>> device in a multidevice setup. >>>>> >>>> >>>> While we are fixing up these bindings, we could add some more trivial things, >>>> like power supplies. >>>> >>>> Also, the interrupt property could use a description since the chip has multiple >>>> output pins. I assume it means the /DRDY pin? >>>> >>> >>> Right! Yes, the interrupt pin refers to the /DRDY. >>> >>>>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> >>>>> --- >>>>> .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- >>>>> 1 file changed, 23 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml >>>>> index 3ce59d4d065f..55cec27bfe60 100644 >>>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml >>>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml >>>>> @@ -47,6 +47,15 @@ properties: >>>>> in any way, for example if the filter decimation rate changes. >>>>> As the line is active low, it should be marked GPIO_ACTIVE_LOW. >>>>> >>>>> + adi,sync-in-spi: >>>> >>>> If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name >>>> should be something like adi,sync-in-sync-out. SPI seems irrelevant here since >>>> we should just be describing how things are wired up, not how it is being used. >>>> >>>> But if we also need to consider the case where SYNC_OUT of one chip is connected >>>> to SYNC_IN of another chip, we might want to consider using trigger-source >>>> bindings instead (recently standardized in dtschema). >>>> >>> >>> Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it >>> handle the non-GPIO case? While testing a multidevice setup, I found it simpler to >>> have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT >>> without referencing another device and makes simultaneous buffered reads easier. >> >> Daisy-chain mode (figure 131)? In that case we normally end up with a single presented device >> with a 'lot' of channels. (See the electric car style battery charging chips, those can >> be chained in very large numbers!) >> > > Actually, it is more like Figure 133 , but the premise is similar. We > have here a Quad setup. > >> Probably similar for figure 133 (which is a dual SPI setup) as the SPI clock must >> be shared so we still see it over a single interface. >> > > We could view them as a single device with multiple channels, and since > the goal is to read them simultaneously with buffered reads, some parameters > such as sampling frequency should be equal to all devices. > > However, there are some implications: If we do the above, we have > limitations in the customization of the "channels", they would have > the same filter, frequency modulator and scale (we plan to add support > for ADAQ776x-1 series, which include PGA and AAF gain). > > To customize them separetely, we would need to assert only the > corresponding chip select, which is only possible with different > instances, as far as I know. FYI, I've been discussing with the HDL folks at ADI about how we could make a multi-bus SPI controller, similar to controllers used for parallel SPI flash memories that are used as a single logical device. So that is the solution I am hoping for here. It would would allow a single IIO device instance for multiple chips. But the SPI controller would allow addressing individual chips for configuration and addressing all chips at the same time for reading sample data. > >> If those are the only two options then keeping this within the driver is fine. >> For daisy chain there are examples in tree and it normally means we have to >> have a DT parameter that says how long the chain is, though we maybe can >> do that with per channel nodes as well if those make sense here. >> >> Jonathan >> > > Those are the options in the datasheet and in hardware so far. I was > considering other scenarios in case the user combine them differently. > I believe keping within the driver covers the main cases. >
On Tue, 14 Jan 2025 10:05:02 -0600 David Lechner <dlechner@baylibre.com> wrote: > On 1/13/25 6:18 PM, Jonathan Santos wrote: > > On 01/12, Jonathan Cameron wrote: > >> On Sat, 11 Jan 2025 19:34:14 -0300 > >> Jonathan Santos <jonath4nns@gmail.com> wrote: > >> > >>> On 01/07, David Lechner wrote: > >>>> On 1/7/25 9:24 AM, Jonathan Santos wrote: > >>>>> Add adi,sync-in-spi property to enable synchronization over SPI. > >>>>> This should be used in the case when the GPIO cannot provide a > >>>>> pulse synchronous with the base MCLK signal. > >>>>> > >>>>> User can choose between SPI, GPIO synchronization or neither of them, > >>>>> but only if a external pulse can be provided, for example, by another > >>>>> device in a multidevice setup. > >>>>> > >>>> > >>>> While we are fixing up these bindings, we could add some more trivial things, > >>>> like power supplies. > >>>> > >>>> Also, the interrupt property could use a description since the chip has multiple > >>>> output pins. I assume it means the /DRDY pin? > >>>> > >>> > >>> Right! Yes, the interrupt pin refers to the /DRDY. > >>> > >>>>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > >>>>> --- > >>>>> .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- > >>>>> 1 file changed, 23 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > >>>>> index 3ce59d4d065f..55cec27bfe60 100644 > >>>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > >>>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > >>>>> @@ -47,6 +47,15 @@ properties: > >>>>> in any way, for example if the filter decimation rate changes. > >>>>> As the line is active low, it should be marked GPIO_ACTIVE_LOW. > >>>>> > >>>>> + adi,sync-in-spi: > >>>> > >>>> If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name > >>>> should be something like adi,sync-in-sync-out. SPI seems irrelevant here since > >>>> we should just be describing how things are wired up, not how it is being used. > >>>> > >>>> But if we also need to consider the case where SYNC_OUT of one chip is connected > >>>> to SYNC_IN of another chip, we might want to consider using trigger-source > >>>> bindings instead (recently standardized in dtschema). > >>>> > >>> > >>> Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it > >>> handle the non-GPIO case? While testing a multidevice setup, I found it simpler to > >>> have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT > >>> without referencing another device and makes simultaneous buffered reads easier. > >> > >> Daisy-chain mode (figure 131)? In that case we normally end up with a single presented device > >> with a 'lot' of channels. (See the electric car style battery charging chips, those can > >> be chained in very large numbers!) > >> > > > > Actually, it is more like Figure 133 , but the premise is similar. We > > have here a Quad setup. > > > >> Probably similar for figure 133 (which is a dual SPI setup) as the SPI clock must > >> be shared so we still see it over a single interface. > >> > > > > We could view them as a single device with multiple channels, and since > > the goal is to read them simultaneously with buffered reads, some parameters > > such as sampling frequency should be equal to all devices. > > > > However, there are some implications: If we do the above, we have > > limitations in the customization of the "channels", they would have > > the same filter, frequency modulator and scale (we plan to add support > > for ADAQ776x-1 series, which include PGA and AAF gain). > > > > To customize them separetely, we would need to assert only the > > corresponding chip select, which is only possible with different > > instances, as far as I know. Ah. This is different from the daisy chain cases where even this is done by writing through the single interface (usually you have to update same register on all devices). To support this they probably have to remain separate devices because of how the SPI subystem will present them. It's not impossible to have multiple spi parents feed into a single child device but it is complex. Or I guess we end up with something magic via a backend like David describes below. > > FYI, I've been discussing with the HDL folks at ADI about how we could make a > multi-bus SPI controller, similar to controllers used for parallel SPI flash > memories that are used as a single logical device. So that is the solution I > am hoping for here. It would would allow a single IIO device instance for > multiple chips. But the SPI controller would allow addressing individual chips > for configuration and addressing all chips at the same time for reading sample > data. Maybe this could be presented as if it were a typical daisy chain. So a long write sends the correct writes to each chip. However, even if we have HDL like this it isn't very general if someone wires this up to a different HDL it will look quite different :( I guess we don't have to keep to conventions of this looking like an SPI a device given there is going to be a backend involved. Nice to keep the bindings in line with conventional SPI though. Jonathan > > > > >> If those are the only two options then keeping this within the driver is fine. > >> For daisy chain there are examples in tree and it normally means we have to > >> have a DT parameter that says how long the chain is, though we maybe can > >> do that with per channel nodes as well if those make sense here. > >> > >> Jonathan > >> > > > > Those are the options in the datasheet and in hardware so far. I was > > considering other scenarios in case the user combine them differently. > > I believe keping within the driver covers the main cases. > >
On Mon, 13 Jan 2025 21:41:04 -0300 Jonathan Santos <jonath4nns@gmail.com> wrote: > On 01/12, Jonathan Cameron wrote: > > On Fri, 10 Jan 2025 18:51:06 -0300 > > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > > > > On 01/07, Jonathan Santos wrote: > > > > Add adi,sync-in-spi property to enable synchronization over SPI. > > > > This should be used in the case when the GPIO cannot provide a > > > > pulse synchronous with the base MCLK signal. > > > > > > > > User can choose between SPI, GPIO synchronization or neither of them, > > > > but only if a external pulse can be provided, for example, by another > > > > device in a multidevice setup. > > > > > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > > > --- > > > > .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- > > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > > index 3ce59d4d065f..55cec27bfe60 100644 > > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > > > > @@ -47,6 +47,15 @@ properties: > > > > in any way, for example if the filter decimation rate changes. > > > > As the line is active low, it should be marked GPIO_ACTIVE_LOW. > > > > > > > > + adi,sync-in-spi: > > > > + description: > > > > + Enables synchronization of multiple devices over SPI. This property is > > > > + used when a signal synchronous to the base MCLK signal cannot be provided > > > > + via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin > > > > + on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device > > > > + should be routed to the SYNC_IN pins of the other devices. > > > So, if I'm getting it right, > > > > Datasheet on this is indeed complex! > > > > >/SYNC_IN may be driven by a GPIO (ADAQ7768-1 datasheet Figure 131), > > > > For that we expose a gpio binding already. If that's present we know what is going on. > > > > >/SYNC_IN may be driven by own device /SYNC_OUT (ADAQ7768-1 datasheet Figure 133), > > > > This is the default - no information provided so it isn't wired externally. > > We don't normally bother to describe required chip to chip connections. > > I couldn't entirely figure out if this is 'required' if we aren't driving explicitly > > from GPIO or another chip but i think it is(?). > > > > It is. If the device is not being driven by a GPIO or an external > device, the user should connect the /SYNC_OUT to /SYNC_IN. We could > assume that this is the case if the GPIO is not present in the > devicetree, maybe put that into the description. The sync-out-sync-in > property as a boolean can be quite redundant, since we also need to > remove the GPIO. > > > >/SYNC_IN may be driven by other AD7768-1 > /SYNC_OUT pin (also Figure 133). > > This is only case we are about for sync in I think. > > > > As long as there isn't a valid 'not connected' It think we are fine with a boolean. > > > > If opt for a single node instace, thats ok; otherwise, David's > trigger-sources suggestion seems to be the best approach. That probably works on assumption that if no gpio, or trigger source etc bindings are present then we assume the pins are wired together and it's simple SPI triggering (so what we'd have if none of this complexity exists!) Jonathan > > > > That is too much to describe with a boolean. > > > > > > If David's suggestion of using a trigger-source doesn't fit, this property > > > should at least become an enum or string. > > > > > > > + type: boolean > > > > + > > > > reset-gpios: > > > > maxItems: 1 > > > > > > > > @@ -65,7 +74,6 @@ required: > > > > - vref-supply > > > > - spi-cpol > > > > - spi-cpha > > > > - - adi,sync-in-gpios > > > > > > > > patternProperties: > > > > "^channel@([0-9]|1[0-5])$": > > > > @@ -89,6 +97,20 @@ patternProperties: > > > > allOf: > > > > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > > > > > > > + # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid) > > > > + - if: > > > > + required: > > > > + - adi,sync-in-gpios > > > > + then: > > > > + properties: > > > > + adi,sync-in-spi: false > > > > + - if: > > > > + required: > > > > + - adi,sync-in-spi > > > > + then: > > > > + properties: > > > > + adi,sync-in-gpios: false > > > > + > > > > unevaluatedProperties: false > > > > > > > > examples: > > > > -- > > > > 2.34.1 > > > > > >
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml index 3ce59d4d065f..55cec27bfe60 100644 --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml @@ -47,6 +47,15 @@ properties: in any way, for example if the filter decimation rate changes. As the line is active low, it should be marked GPIO_ACTIVE_LOW. + adi,sync-in-spi: + description: + Enables synchronization of multiple devices over SPI. This property is + used when a signal synchronous to the base MCLK signal cannot be provided + via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin + on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device + should be routed to the SYNC_IN pins of the other devices. + type: boolean + reset-gpios: maxItems: 1 @@ -65,7 +74,6 @@ required: - vref-supply - spi-cpol - spi-cpha - - adi,sync-in-gpios patternProperties: "^channel@([0-9]|1[0-5])$": @@ -89,6 +97,20 @@ patternProperties: allOf: - $ref: /schemas/spi/spi-peripheral-props.yaml# + # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid) + - if: + required: + - adi,sync-in-gpios + then: + properties: + adi,sync-in-spi: false + - if: + required: + - adi,sync-in-spi + then: + properties: + adi,sync-in-gpios: false + unevaluatedProperties: false examples:
Add adi,sync-in-spi property to enable synchronization over SPI. This should be used in the case when the GPIO cannot provide a pulse synchronous with the base MCLK signal. User can choose between SPI, GPIO synchronization or neither of them, but only if a external pulse can be provided, for example, by another device in a multidevice setup. Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> --- .../bindings/iio/adc/adi,ad7768-1.yaml | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)