* [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
2024-11-19 12:53 [PATCH v3 0/4] Timestamp and PulSAR support for ad4000 Marcelo Schmitt
@ 2024-11-19 12:53 ` Marcelo Schmitt
2024-11-20 8:14 ` Krzysztof Kozlowski
2024-11-19 12:53 ` [PATCH v3 2/4] iio: adc: ad4000: Add timestamp channel Marcelo Schmitt
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Marcelo Schmitt @ 2024-11-19 12:53 UTC (permalink / raw)
To: lars, Michael.Hennerich, marcelo.schmitt, jic23, robh, krzk+dt,
conor+dt, marcelo.schmitt1
Cc: linux-iio, devicetree, linux-kernel
Extend the AD4000 series device tree documentation to also describe
PulSAR devices.
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
No changes from v2 -> v3.
.../bindings/iio/adc/adi,ad4000.yaml | 71 +++++++++++++++++++
1 file changed, 71 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
index e413a9d8d2a2..4dbb3d2876f9 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
@@ -19,6 +19,20 @@ description: |
https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7685.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7686.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7687.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7688.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7690.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7693.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7942.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7946.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7980.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7982.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7983.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7984.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad7988-1_7988-5.pdf
$ref: /schemas/spi/spi-peripheral-props.yaml#
@@ -63,6 +77,37 @@ properties:
- const: adi,adaq4003
+ - const: adi,ad7946
+ - items:
+ - enum:
+ - adi,ad7942
+ - const: adi,ad7946
+
+ - const: adi,ad7983
+ - items:
+ - enum:
+ - adi,ad7980
+ - adi,ad7988-5
+ - adi,ad7686
+ - adi,ad7685
+ - adi,ad7988-1
+ - const: adi,ad7983
+
+ - const: adi,ad7688
+ - items:
+ - enum:
+ - adi,ad7693
+ - adi,ad7687
+ - const: adi,ad7688
+
+ - const: adi,ad7984
+ - items:
+ - enum:
+ - adi,ad7982
+ - adi,ad7690
+ - adi,ad7691
+ - const: adi,ad7984
+
reg:
maxItems: 1
@@ -133,6 +178,32 @@ required:
- ref-supply
allOf:
+ # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - adi,ad7685
+ - adi,ad7686
+ - adi,ad7687
+ - adi,ad7688
+ - adi,ad7690
+ - adi,ad7691
+ - adi,ad7693
+ - adi,ad7942
+ - adi,ad7946
+ - adi,ad7980
+ - adi,ad7982
+ - adi,ad7983
+ - adi,ad7984
+ - adi,ad7988-1
+ - adi,ad7988-5
+ then:
+ properties:
+ adi,sdi-pin:
+ enum: [ high, low, cs ]
+ default: cs
# The configuration register can only be accessed if SDI is connected to MOSI
- if:
required:
--
2.45.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
2024-11-19 12:53 ` [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR Marcelo Schmitt
@ 2024-11-20 8:14 ` Krzysztof Kozlowski
2024-11-22 15:33 ` Marcelo Schmitt
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2024-11-20 8:14 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: lars, Michael.Hennerich, jic23, robh, krzk+dt, conor+dt,
marcelo.schmitt1, linux-iio, devicetree, linux-kernel
On Tue, Nov 19, 2024 at 09:53:40AM -0300, Marcelo Schmitt wrote:
> Extend the AD4000 series device tree documentation to also describe
> PulSAR devices.
>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> No changes from v2 -> v3.
>
> .../bindings/iio/adc/adi,ad4000.yaml | 71 +++++++++++++++++++
> 1 file changed, 71 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> index e413a9d8d2a2..4dbb3d2876f9 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> @@ -19,6 +19,20 @@ description: |
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7685.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7686.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7687.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7688.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7690.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7693.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7942.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7946.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7980.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7982.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7983.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7984.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7988-1_7988-5.pdf
>
> $ref: /schemas/spi/spi-peripheral-props.yaml#
>
> @@ -63,6 +77,37 @@ properties:
>
> - const: adi,adaq4003
>
> + - const: adi,ad7946
All such cases are just one enum. That's the preferred syntax.
> + - items:
> + - enum:
> + - adi,ad7942
> + - const: adi,ad7946
> +
> + - const: adi,ad7983
> + - items:
> + - enum:
> + - adi,ad7980
> + - adi,ad7988-5
> + - adi,ad7686
> + - adi,ad7685
Keep alphabetical order.
> + - adi,ad7988-1
> + - const: adi,ad7983
> +
> + - const: adi,ad7688
> + - items:
> + - enum:
> + - adi,ad7693
> + - adi,ad7687
> + - const: adi,ad7688
> +
> + - const: adi,ad7984
> + - items:
> + - enum:
> + - adi,ad7982
> + - adi,ad7690
> + - adi,ad7691
> + - const: adi,ad7984
> +
> reg:
> maxItems: 1
>
> @@ -133,6 +178,32 @@ required:
> - ref-supply
>
> allOf:
> + # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - adi,ad7685
Why do you need this? It's fallback is already here.
> + - adi,ad7686
> + - adi,ad7687
> + - adi,ad7688
> + - adi,ad7690
> + - adi,ad7691
> + - adi,ad7693
> + - adi,ad7942
> + - adi,ad7946
> + - adi,ad7980
> + - adi,ad7982
> + - adi,ad7983
> + - adi,ad7984
> + - adi,ad7988-1
> + - adi,ad7988-5
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
2024-11-20 8:14 ` Krzysztof Kozlowski
@ 2024-11-22 15:33 ` Marcelo Schmitt
2024-11-22 18:29 ` Krzysztof Kozlowski
0 siblings, 1 reply; 11+ messages in thread
From: Marcelo Schmitt @ 2024-11-22 15:33 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Marcelo Schmitt, lars, Michael.Hennerich, jic23, robh, krzk+dt,
conor+dt, linux-iio, devicetree, linux-kernel
On 11/20, Krzysztof Kozlowski wrote:
> On Tue, Nov 19, 2024 at 09:53:40AM -0300, Marcelo Schmitt wrote:
> > Extend the AD4000 series device tree documentation to also describe
> > PulSAR devices.
> >
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > ---
> > No changes from v2 -> v3.
> >
> > .../bindings/iio/adc/adi,ad4000.yaml | 71 +++++++++++++++++++
> > 1 file changed, 71 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > index e413a9d8d2a2..4dbb3d2876f9 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > @@ -19,6 +19,20 @@ description: |
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7685.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7686.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7687.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7688.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7690.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7693.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7942.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7946.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7980.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7982.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7983.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7984.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7988-1_7988-5.pdf
> >
> > $ref: /schemas/spi/spi-peripheral-props.yaml#
> >
> > @@ -63,6 +77,37 @@ properties:
> >
> > - const: adi,adaq4003
> >
> > + - const: adi,ad7946
>
> All such cases are just one enum. That's the preferred syntax.
>
Ack
>
> > + - items:
> > + - enum:
> > + - adi,ad7942
> > + - const: adi,ad7946
> > +
> > + - const: adi,ad7983
> > + - items:
> > + - enum:
> > + - adi,ad7980
> > + - adi,ad7988-5
> > + - adi,ad7686
> > + - adi,ad7685
>
> Keep alphabetical order.
Do the fallbacks declared here have any impact on the match try order or on how
the compatible list should be ordered?
The only significant difference between each group of devices is the sample rate.
A faster device can read at slower sample rates so if somebody knows to have
a 16-bit pseudo-differential PulSAR but doesn't know about the exact model they
could have a compatible like
compatible = "adi,ad7980", "adi,ad7988-5", "adi,ad7686", "adi,ad7685",
"adi,ad7988-1", "adi,ad7983";
to try from fastest to slowest device.
The dt doc would indicate that order in the fallback list?
- items:
- enum:
- adi,ad7980 # Fastest 16-bit pseudo-differential ADC
- adi,ad7988-5 # 2nd fastest 16-bit pseudo-differential ADC
- adi,ad7686 # 3rd fastest 16-bit pseudo-differential ADC
- adi,ad7685 # 4th fastest 16-bit pseudo-differential ADC
- adi,ad7988-1 # 5th fastest 16-bit pseudo-differential ADC
- const: adi,ad7983 # Slowest 16-bit pseudo-differential ADC
https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
has a nice table with the different devices and sample rates.
writing-bindings.rst says "DO use fallback compatibles when devices are the same
as or a subset of prior implementations."
But, how can we use fallbacks properly?
From Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml I'm
inferring only one fallback should be provided per group of devices.
>
> > + - adi,ad7988-1
> > + - const: adi,ad7983
> > +
> > + - const: adi,ad7688
> > + - items:
> > + - enum:
> > + - adi,ad7693
> > + - adi,ad7687
> > + - const: adi,ad7688
> > +
> > + - const: adi,ad7984
> > + - items:
> > + - enum:
> > + - adi,ad7982
> > + - adi,ad7690
> > + - adi,ad7691
> > + - const: adi,ad7984
> > +
> > reg:
> > maxItems: 1
> >
> > @@ -133,6 +178,32 @@ required:
> > - ref-supply
> >
> > allOf:
> > + # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - adi,ad7685
>
> Why do you need this? It's fallback is already here.
So dtbs_check can provide an error message if for example compatible = "adi,ad7687";
and adi,sdi-pin = "sdi";
zynq-coraz7s-ad7687.dtb: adc@0: adi,sdi-pin:0: 'sdi' is not one of ['high', 'low', 'cs']
>
> > + - adi,ad7686
> > + - adi,ad7687
> > + - adi,ad7688
> > + - adi,ad7690
> > + - adi,ad7691
> > + - adi,ad7693
> > + - adi,ad7942
> > + - adi,ad7946
> > + - adi,ad7980
> > + - adi,ad7982
> > + - adi,ad7983
> > + - adi,ad7984
> > + - adi,ad7988-1
> > + - adi,ad7988-5
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
2024-11-22 15:33 ` Marcelo Schmitt
@ 2024-11-22 18:29 ` Krzysztof Kozlowski
2024-11-22 21:15 ` Marcelo Schmitt
0 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2024-11-22 18:29 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: Marcelo Schmitt, lars, Michael.Hennerich, jic23, robh, krzk+dt,
conor+dt, linux-iio, devicetree, linux-kernel
On 22/11/2024 16:33, Marcelo Schmitt wrote:
>>
>>> + - items:
>>> + - enum:
>>> + - adi,ad7942
>>> + - const: adi,ad7946
>>> +
>>> + - const: adi,ad7983
>>> + - items:
>>> + - enum:
>>> + - adi,ad7980
>>> + - adi,ad7988-5
>>> + - adi,ad7686
>>> + - adi,ad7685
>>
>> Keep alphabetical order.
>
> Do the fallbacks declared here have any impact on the match try order or on how
> the compatible list should be ordered?
I don't understand, we do not talk about fallbacks. I also do not
understand at all how this relates to my comment.
> The only significant difference between each group of devices is the sample rate.
> A faster device can read at slower sample rates so if somebody knows to have
> a 16-bit pseudo-differential PulSAR but doesn't know about the exact model they
> could have a compatible like
> compatible = "adi,ad7980", "adi,ad7988-5", "adi,ad7686", "adi,ad7685",
> "adi,ad7988-1", "adi,ad7983";
Can't you autodetect this?
>
> to try from fastest to slowest device.
> The dt doc would indicate that order in the fallback list?
> - items:
> - enum:
> - adi,ad7980 # Fastest 16-bit pseudo-differential ADC
> - adi,ad7988-5 # 2nd fastest 16-bit pseudo-differential ADC
> - adi,ad7686 # 3rd fastest 16-bit pseudo-differential ADC
> - adi,ad7685 # 4th fastest 16-bit pseudo-differential ADC
> - adi,ad7988-1 # 5th fastest 16-bit pseudo-differential ADC
> - const: adi,ad7983 # Slowest 16-bit pseudo-differential ADC
Again, only one fallback here, not sure what are you asking about. BTW,
DT spec explains compatibles...
>
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
> has a nice table with the different devices and sample rates.
>
> writing-bindings.rst says "DO use fallback compatibles when devices are the same
> as or a subset of prior implementations."
> But, how can we use fallbacks properly?
How DT spec and tutorials like elinux ask... What is exactly the problem
or question?
> From Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml I'm
How LVDS bridge is related to this one here?
> inferring only one fallback should be provided per group of devices.
>
>>
>>> + - adi,ad7988-1
>>> + - const: adi,ad7983
>>> +
>>> + - const: adi,ad7688
>>> + - items:
>>> + - enum:
>>> + - adi,ad7693
>>> + - adi,ad7687
>>> + - const: adi,ad7688
>>> +
>>> + - const: adi,ad7984
>>> + - items:
>>> + - enum:
>>> + - adi,ad7982
>>> + - adi,ad7690
>>> + - adi,ad7691
>>> + - const: adi,ad7984
>>> +
>>> reg:
>>> maxItems: 1
>>>
>>> @@ -133,6 +178,32 @@ required:
>>> - ref-supply
>>>
>>> allOf:
>>> + # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
>>> + - if:
>>> + properties:
>>> + compatible:
>>> + contains:
>>> + enum:
>>> + - adi,ad7685
>>
>> Why do you need this? It's fallback is already here.
>
> So dtbs_check can provide an error message if for example compatible = "adi,ad7687";
> and adi,sdi-pin = "sdi";
I mean this compatible, not if clause.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
2024-11-22 18:29 ` Krzysztof Kozlowski
@ 2024-11-22 21:15 ` Marcelo Schmitt
2024-11-23 16:11 ` Krzysztof Kozlowski
0 siblings, 1 reply; 11+ messages in thread
From: Marcelo Schmitt @ 2024-11-22 21:15 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Marcelo Schmitt, lars, Michael.Hennerich, jic23, robh, krzk+dt,
conor+dt, linux-iio, devicetree, linux-kernel
On 11/22, Krzysztof Kozlowski wrote:
> On 22/11/2024 16:33, Marcelo Schmitt wrote:
> >>
> >>> + - items:
> >>> + - enum:
> >>> + - adi,ad7942
> >>> + - const: adi,ad7946
> >>> +
> >>> + - const: adi,ad7983
> >>> + - items:
> >>> + - enum:
> >>> + - adi,ad7980
> >>> + - adi,ad7988-5
> >>> + - adi,ad7686
> >>> + - adi,ad7685
> >>
> >> Keep alphabetical order.
> >
> > Do the fallbacks declared here have any impact on the match try order or on how
> > the compatible list should be ordered?
>
> I don't understand, we do not talk about fallbacks. I also do not
> understand at all how this relates to my comment.
I was wondering if the arrangement in which compatible strings appear in dt doc
could be used to suggest the sequence to add them to the compatible property of a
device node in a dts. Apparently, the arrangement of compatible strings in dt doc
has nothing to do with how they can appear in a dts file. Will sort them in
alphabetical order.
>
> > The only significant difference between each group of devices is the sample rate.
> > A faster device can read at slower sample rates so if somebody knows to have
> > a 16-bit pseudo-differential PulSAR but doesn't know about the exact model they
> > could have a compatible like
> > compatible = "adi,ad7980", "adi,ad7988-5", "adi,ad7686", "adi,ad7685",
> > "adi,ad7988-1", "adi,ad7983";
>
> Can't you autodetect this?
There is no way of detecting the maximum sample rate other than the compatible
string or, maybe, running a data capture.
>
> >
> > to try from fastest to slowest device.
> > The dt doc would indicate that order in the fallback list?
> > - items:
> > - enum:
> > - adi,ad7980 # Fastest 16-bit pseudo-differential ADC
> > - adi,ad7988-5 # 2nd fastest 16-bit pseudo-differential ADC
> > - adi,ad7686 # 3rd fastest 16-bit pseudo-differential ADC
> > - adi,ad7685 # 4th fastest 16-bit pseudo-differential ADC
> > - adi,ad7988-1 # 5th fastest 16-bit pseudo-differential ADC
> > - const: adi,ad7983 # Slowest 16-bit pseudo-differential ADC
>
[...]
> >
> > writing-bindings.rst says "DO use fallback compatibles when devices are the same
> > as or a subset of prior implementations."
> > But, how can we use fallbacks properly?
>
> How DT spec and tutorials like elinux ask... What is exactly the problem
> or question?
Never mind. Do the bellow follow the preferred syntax?
- items:
- enum:
- adi,ad7980
- adi,ad7685
- adi,ad7686
- adi,ad7988-1
- adi,ad7988-5
- const: adi,ad7983
- items:
- enum:
- adi,ad7688
- adi,ad7693
- const: adi,ad7687
- items:
- enum:
- adi,ad7982
- adi,ad7984
- adi,ad7690
- const: adi,ad7691
- enum:
- adi,ad7942
- adi,ad7946
- adi,ad7984
>
> > From Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml I'm
>
> How LVDS bridge is related to this one here?
Aside from having compatible fallbacks, not related.
>
> > inferring only one fallback should be provided per group of devices.
> >
> >>
> >>> + - adi,ad7988-1
> >>> + - const: adi,ad7983
> >>> +
> >>> + - const: adi,ad7688
> >>> + - items:
> >>> + - enum:
> >>> + - adi,ad7693
> >>> + - adi,ad7687
> >>> + - const: adi,ad7688
> >>> +
> >>> + - const: adi,ad7984
> >>> + - items:
> >>> + - enum:
> >>> + - adi,ad7982
> >>> + - adi,ad7690
> >>> + - adi,ad7691
> >>> + - const: adi,ad7984
> >>> +
> >>> reg:
> >>> maxItems: 1
> >>>
> >>> @@ -133,6 +178,32 @@ required:
> >>> - ref-supply
> >>>
> >>> allOf:
> >>> + # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
> >>> + - if:
> >>> + properties:
> >>> + compatible:
> >>> + contains:
> >>> + enum:
> >>> + - adi,ad7685
> >>
> >> Why do you need this? It's fallback is already here.
> >
> > So dtbs_check can provide an error message if for example compatible = "adi,ad7687";
> > and adi,sdi-pin = "sdi";
>
>
> I mean this compatible, not if clause.
dtbs_check don't show an error message if the allOf list only has the fallback
compatible for adi,ad7685 and a device node has both
compatible = "adi,ad7685" and adi,sdi-pin = "sdi".
The new set of devices that will be supported by this binding don't have a
configuration register like the previous ones did. Because the PulSAR devices
don't have a config reg, they don't support all features of AD4000-like devices
and thus fewer IIO ABI interfaces are provided to user space. Though, AD4000
devices also can be wired in a way that no reg access is possible, in which
case they provide the same IIO interfaces that PulSAR devices do. The difference
is on what is connected to the peripheral SDI pin. When AD4000 SDI is connected
to SPI controller MOSI line, more interfaces are provided because the config
reg can be accessed to set additional features. But that is not an option for
PulSAR devices. Even if controller MOSI is connected to a PulSAR device, we
cannot provide the additional interfaces because every attempt to use them would
fail (the device has no register to configure). No datasheets mentions
connecting a PulSAR device SDI pin to a SPI MOSI line. All datasheets show
PulSAR SDI pin connected either to VIO (high), GND (low), or controller CS.
IMHO, it would be nice to have dtbs_check warn about invalid SDI pin
configuration otherwise it may only be noticed on driver probe.
Anyway, I'm also fine keeping only the fallback compatibles in the allOf list
if that makes dt maintainers happy.
>
>
>
>
> Best regards,
> Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR
2024-11-22 21:15 ` Marcelo Schmitt
@ 2024-11-23 16:11 ` Krzysztof Kozlowski
0 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2024-11-23 16:11 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: Marcelo Schmitt, lars, Michael.Hennerich, jic23, robh, krzk+dt,
conor+dt, linux-iio, devicetree, linux-kernel
On 22/11/2024 22:15, Marcelo Schmitt wrote:
> On 11/22, Krzysztof Kozlowski wrote:
>> On 22/11/2024 16:33, Marcelo Schmitt wrote:
>>>>
>>>>> + - items:
>>>>> + - enum:
>>>>> + - adi,ad7942
>>>>> + - const: adi,ad7946
>>>>> +
>>>>> + - const: adi,ad7983
>>>>> + - items:
>>>>> + - enum:
>>>>> + - adi,ad7980
>>>>> + - adi,ad7988-5
>>>>> + - adi,ad7686
>>>>> + - adi,ad7685
>>>>
>>>> Keep alphabetical order.
>>>
>>> Do the fallbacks declared here have any impact on the match try order or on how
>>> the compatible list should be ordered?
>>
>> I don't understand, we do not talk about fallbacks. I also do not
>> understand at all how this relates to my comment.
>
> I was wondering if the arrangement in which compatible strings appear in dt doc
> could be used to suggest the sequence to add them to the compatible property of a
> device node in a dts. Apparently, the arrangement of compatible strings in dt doc
> has nothing to do with how they can appear in a dts file. Will sort them in
> alphabetical order.
We talk here about enum. Enum enumerates, so obviously they cannot
appear one after another.
>
>>
>>> The only significant difference between each group of devices is the sample rate.
>>> A faster device can read at slower sample rates so if somebody knows to have
>>> a 16-bit pseudo-differential PulSAR but doesn't know about the exact model they
>>> could have a compatible like
>>> compatible = "adi,ad7980", "adi,ad7988-5", "adi,ad7686", "adi,ad7685",
>>> "adi,ad7988-1", "adi,ad7983";
>>
>> Can't you autodetect this?
>
> There is no way of detecting the maximum sample rate other than the compatible
> string or, maybe, running a data capture.
Devices do not have version/revision/model register?
>
>>
>>>
>>> to try from fastest to slowest device.
>>> The dt doc would indicate that order in the fallback list?
>>> - items:
>>> - enum:
>>> - adi,ad7980 # Fastest 16-bit pseudo-differential ADC
>>> - adi,ad7988-5 # 2nd fastest 16-bit pseudo-differential ADC
>>> - adi,ad7686 # 3rd fastest 16-bit pseudo-differential ADC
>>> - adi,ad7685 # 4th fastest 16-bit pseudo-differential ADC
>>> - adi,ad7988-1 # 5th fastest 16-bit pseudo-differential ADC
>>> - const: adi,ad7983 # Slowest 16-bit pseudo-differential ADC
>>
> [...]
>>>
>>> writing-bindings.rst says "DO use fallback compatibles when devices are the same
>>> as or a subset of prior implementations."
>>> But, how can we use fallbacks properly?
>>
>> How DT spec and tutorials like elinux ask... What is exactly the problem
>> or question?
>
> Never mind. Do the bellow follow the preferred syntax?
>
> - items:
> - enum:
> - adi,ad7980
> - adi,ad7685
> - adi,ad7686
> - adi,ad7988-1
> - adi,ad7988-5
> - const: adi,ad7983
>
> - items:
> - enum:
> - adi,ad7688
> - adi,ad7693
> - const: adi,ad7687
>
> - items:
> - enum:
> - adi,ad7982
> - adi,ad7984
> - adi,ad7690
> - const: adi,ad7691
>
> - enum:
> - adi,ad7942
> - adi,ad7946
> - adi,ad7984
Yes
>
>>
>>> From Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml I'm
>>
>> How LVDS bridge is related to this one here?
>
> Aside from having compatible fallbacks, not related.
>
>>
>>> inferring only one fallback should be provided per group of devices.
>>>
>>>>
>>>>> + - adi,ad7988-1
>>>>> + - const: adi,ad7983
>>>>> +
>>>>> + - const: adi,ad7688
>>>>> + - items:
>>>>> + - enum:
>>>>> + - adi,ad7693
>>>>> + - adi,ad7687
>>>>> + - const: adi,ad7688
>>>>> +
>>>>> + - const: adi,ad7984
>>>>> + - items:
>>>>> + - enum:
>>>>> + - adi,ad7982
>>>>> + - adi,ad7690
>>>>> + - adi,ad7691
>>>>> + - const: adi,ad7984
>>>>> +
>>>>> reg:
>>>>> maxItems: 1
>>>>>
>>>>> @@ -133,6 +178,32 @@ required:
>>>>> - ref-supply
>>>>>
>>>>> allOf:
>>>>> + # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.
>>>>> + - if:
>>>>> + properties:
>>>>> + compatible:
>>>>> + contains:
>>>>> + enum:
>>>>> + - adi,ad7685
>>>>
>>>> Why do you need this? It's fallback is already here.
>>>
>>> So dtbs_check can provide an error message if for example compatible = "adi,ad7687";
>>> and adi,sdi-pin = "sdi";
>>
>>
>> I mean this compatible, not if clause.
>
> dtbs_check don't show an error message if the allOf list only has the fallback
> compatible for adi,ad7685 and a device node has both
> compatible = "adi,ad7685" and adi,sdi-pin = "sdi".
It must and your compatibles should not affect it. I don't know which
code you are testing, but I even tested the correct approach and it
correctly shows error.
>
> The new set of devices that will be supported by this binding don't have a
> configuration register like the previous ones did. Because the PulSAR devices
> don't have a config reg, they don't support all features of AD4000-like devices
> and thus fewer IIO ABI interfaces are provided to user space. Though, AD4000
> devices also can be wired in a way that no reg access is possible, in which
> case they provide the same IIO interfaces that PulSAR devices do. The difference
> is on what is connected to the peripheral SDI pin. When AD4000 SDI is connected
> to SPI controller MOSI line, more interfaces are provided because the config
> reg can be accessed to set additional features. But that is not an option for
> PulSAR devices. Even if controller MOSI is connected to a PulSAR device, we
> cannot provide the additional interfaces because every attempt to use them would
> fail (the device has no register to configure). No datasheets mentions
> connecting a PulSAR device SDI pin to a SPI MOSI line. All datasheets show
> PulSAR SDI pin connected either to VIO (high), GND (low), or controller CS.
>
> IMHO, it would be nice to have dtbs_check warn about invalid SDI pin
> configuration otherwise it may only be noticed on driver probe.
> Anyway, I'm also fine keeping only the fallback compatibles in the allOf list
> if that makes dt maintainers happy.
Only fallbacks go there,
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/4] iio: adc: ad4000: Add timestamp channel
2024-11-19 12:53 [PATCH v3 0/4] Timestamp and PulSAR support for ad4000 Marcelo Schmitt
2024-11-19 12:53 ` [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR Marcelo Schmitt
@ 2024-11-19 12:53 ` Marcelo Schmitt
2024-11-19 12:54 ` [PATCH v3 3/4] iio: adc: ad4000: Use device specific timing for SPI transfers Marcelo Schmitt
2024-11-19 12:54 ` [PATCH v3 4/4] iio: adc: ad4000: Add support for PulSAR devices Marcelo Schmitt
3 siblings, 0 replies; 11+ messages in thread
From: Marcelo Schmitt @ 2024-11-19 12:53 UTC (permalink / raw)
To: lars, Michael.Hennerich, marcelo.schmitt, jic23, robh, krzk+dt,
conor+dt, marcelo.schmitt1
Cc: linux-iio, devicetree, linux-kernel, David Lechner
The ADC data is pushed to the IIO buffer along with timestamp but no
timestamp channel was provided to retried the time data.
Add a timestamp channel to provide sample capture time.
Suggested-by: David Lechner <dlechner@baylibre.com>
Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
drivers/iio/adc/ad4000.c | 98 +++++++++++++++++++++++-----------------
1 file changed, 56 insertions(+), 42 deletions(-)
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
index b3b82535f5c1..21731c4d31ee 100644
--- a/drivers/iio/adc/ad4000.c
+++ b/drivers/iio/adc/ad4000.c
@@ -49,6 +49,7 @@
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
BIT(IIO_CHAN_INFO_SCALE), \
.info_mask_separate_available = _reg_access ? BIT(IIO_CHAN_INFO_SCALE) : 0,\
+ .scan_index = 0, \
.scan_type = { \
.sign = _sign, \
.realbits = _real_bits, \
@@ -62,6 +63,12 @@
__AD4000_DIFF_CHANNEL((_sign), (_real_bits), \
((_real_bits) > 16 ? 32 : 16), (_reg_access))
+#define AD4000_DIFF_CHANNELS(_sign, _real_bits, _reg_access) \
+{ \
+ AD4000_DIFF_CHANNEL(_sign, _real_bits, _reg_access), \
+ IIO_CHAN_SOFT_TIMESTAMP(1) \
+}
+
#define __AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _reg_access)\
{ \
.type = IIO_VOLTAGE, \
@@ -71,6 +78,7 @@
BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_OFFSET), \
.info_mask_separate_available = _reg_access ? BIT(IIO_CHAN_INFO_SCALE) : 0,\
+ .scan_index = 0, \
.scan_type = { \
.sign = _sign, \
.realbits = _real_bits, \
@@ -84,6 +92,12 @@
__AD4000_PSEUDO_DIFF_CHANNEL((_sign), (_real_bits), \
((_real_bits) > 16 ? 32 : 16), (_reg_access))
+#define AD4000_PSEUDO_DIFF_CHANNELS(_sign, _real_bits, _reg_access) \
+{ \
+ AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits, _reg_access), \
+ IIO_CHAN_SOFT_TIMESTAMP(1) \
+}
+
static const char * const ad4000_power_supplies[] = {
"vdd", "vio"
};
@@ -110,106 +124,106 @@ static const int ad4000_gains[] = {
struct ad4000_chip_info {
const char *dev_name;
- struct iio_chan_spec chan_spec;
- struct iio_chan_spec reg_access_chan_spec;
+ struct iio_chan_spec chan_spec[2];
+ struct iio_chan_spec reg_access_chan_spec[2];
bool has_hardware_gain;
};
static const struct ad4000_chip_info ad4000_chip_info = {
.dev_name = "ad4000",
- .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
- .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
+ .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 1),
};
static const struct ad4000_chip_info ad4001_chip_info = {
.dev_name = "ad4001",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 16, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 16, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 16, 1),
};
static const struct ad4000_chip_info ad4002_chip_info = {
.dev_name = "ad4002",
- .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 0),
- .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 1),
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 0),
+ .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 1),
};
static const struct ad4000_chip_info ad4003_chip_info = {
.dev_name = "ad4003",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 18, 1),
};
static const struct ad4000_chip_info ad4004_chip_info = {
.dev_name = "ad4004",
- .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
- .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
+ .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 1),
};
static const struct ad4000_chip_info ad4005_chip_info = {
.dev_name = "ad4005",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 16, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 16, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 16, 1),
};
static const struct ad4000_chip_info ad4006_chip_info = {
.dev_name = "ad4006",
- .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 0),
- .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 1),
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 0),
+ .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 1),
};
static const struct ad4000_chip_info ad4007_chip_info = {
.dev_name = "ad4007",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 18, 1),
};
static const struct ad4000_chip_info ad4008_chip_info = {
.dev_name = "ad4008",
- .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 0),
- .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16, 1),
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
+ .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 1),
};
static const struct ad4000_chip_info ad4010_chip_info = {
.dev_name = "ad4010",
- .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 0),
- .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18, 1),
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 0),
+ .reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 1),
};
static const struct ad4000_chip_info ad4011_chip_info = {
.dev_name = "ad4011",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 18, 1),
};
static const struct ad4000_chip_info ad4020_chip_info = {
.dev_name = "ad4020",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 20, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 20, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 20, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 20, 1),
};
static const struct ad4000_chip_info ad4021_chip_info = {
.dev_name = "ad4021",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 20, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 20, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 20, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 20, 1),
};
static const struct ad4000_chip_info ad4022_chip_info = {
.dev_name = "ad4022",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 20, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 20, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 20, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 20, 1),
};
static const struct ad4000_chip_info adaq4001_chip_info = {
.dev_name = "adaq4001",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 16, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 16, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 16, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 16, 1),
.has_hardware_gain = true,
};
static const struct ad4000_chip_info adaq4003_chip_info = {
.dev_name = "adaq4003",
- .chan_spec = AD4000_DIFF_CHANNEL('s', 18, 0),
- .reg_access_chan_spec = AD4000_DIFF_CHANNEL('s', 18, 1),
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
+ .reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 18, 1),
.has_hardware_gain = true,
};
@@ -591,7 +605,7 @@ static int ad4000_probe(struct spi_device *spi)
switch (st->sdi_pin) {
case AD4000_SDI_MOSI:
indio_dev->info = &ad4000_reg_access_info;
- indio_dev->channels = &chip->reg_access_chan_spec;
+ indio_dev->channels = chip->reg_access_chan_spec;
/*
* In "3-wire mode", the ADC SDI line must be kept high when
@@ -603,7 +617,7 @@ static int ad4000_probe(struct spi_device *spi)
if (ret < 0)
return ret;
- ret = ad4000_prepare_3wire_mode_message(st, indio_dev->channels);
+ ret = ad4000_prepare_3wire_mode_message(st, &indio_dev->channels[0]);
if (ret)
return ret;
@@ -614,16 +628,16 @@ static int ad4000_probe(struct spi_device *spi)
break;
case AD4000_SDI_VIO:
indio_dev->info = &ad4000_info;
- indio_dev->channels = &chip->chan_spec;
- ret = ad4000_prepare_3wire_mode_message(st, indio_dev->channels);
+ indio_dev->channels = chip->chan_spec;
+ ret = ad4000_prepare_3wire_mode_message(st, &indio_dev->channels[0]);
if (ret)
return ret;
break;
case AD4000_SDI_CS:
indio_dev->info = &ad4000_info;
- indio_dev->channels = &chip->chan_spec;
- ret = ad4000_prepare_4wire_mode_message(st, indio_dev->channels);
+ indio_dev->channels = chip->chan_spec;
+ ret = ad4000_prepare_4wire_mode_message(st, &indio_dev->channels[0]);
if (ret)
return ret;
@@ -637,7 +651,7 @@ static int ad4000_probe(struct spi_device *spi)
}
indio_dev->name = chip->dev_name;
- indio_dev->num_channels = 1;
+ indio_dev->num_channels = 2;
ret = devm_mutex_init(dev, &st->lock);
if (ret)
@@ -658,7 +672,7 @@ static int ad4000_probe(struct spi_device *spi)
}
}
- ad4000_fill_scale_tbl(st, indio_dev->channels);
+ ad4000_fill_scale_tbl(st, &indio_dev->channels[0]);
ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
&iio_pollfunc_store_time,
--
2.45.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 3/4] iio: adc: ad4000: Use device specific timing for SPI transfers
2024-11-19 12:53 [PATCH v3 0/4] Timestamp and PulSAR support for ad4000 Marcelo Schmitt
2024-11-19 12:53 ` [PATCH v3 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR Marcelo Schmitt
2024-11-19 12:53 ` [PATCH v3 2/4] iio: adc: ad4000: Add timestamp channel Marcelo Schmitt
@ 2024-11-19 12:54 ` Marcelo Schmitt
2024-11-19 12:54 ` [PATCH v3 4/4] iio: adc: ad4000: Add support for PulSAR devices Marcelo Schmitt
3 siblings, 0 replies; 11+ messages in thread
From: Marcelo Schmitt @ 2024-11-19 12:54 UTC (permalink / raw)
To: lars, Michael.Hennerich, marcelo.schmitt, jic23, robh, krzk+dt,
conor+dt, marcelo.schmitt1
Cc: linux-iio, devicetree, linux-kernel
The SPI transfers for AD4020, AD4021, and AD4022 have slightly different
timing specifications. Use device specific timing constraints to set SPI
transfer parameters. While tweaking time constraints, remove time related
defines including unused AD4000_TQUIET1_NS.
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
drivers/iio/adc/ad4000.c | 51 +++++++++++++++++++++++++++++++++-------
1 file changed, 42 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
index 21731c4d31ee..c700d51b5637 100644
--- a/drivers/iio/adc/ad4000.c
+++ b/drivers/iio/adc/ad4000.c
@@ -35,10 +35,6 @@
#define AD4000_SCALE_OPTIONS 2
-#define AD4000_TQUIET1_NS 190
-#define AD4000_TQUIET2_NS 60
-#define AD4000_TCONV_NS 320
-
#define __AD4000_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _reg_access) \
{ \
.type = IIO_VOLTAGE, \
@@ -122,10 +118,31 @@ static const int ad4000_gains[] = {
454, 909, 1000, 1900,
};
+struct ad4000_time_spec {
+ int t_conv_ns;
+ int t_quiet2_ns;
+};
+
+/*
+ * Same timing specifications for all of AD4000, AD4001, ..., AD4008, AD4010,
+ * ADAQ4001, and ADAQ4003.
+ */
+static const struct ad4000_time_spec ad4000_t_spec = {
+ .t_conv_ns = 320,
+ .t_quiet2_ns = 60,
+};
+
+/* AD4020, AD4021, AD4022 */
+static const struct ad4000_time_spec ad4020_t_spec = {
+ .t_conv_ns = 350,
+ .t_quiet2_ns = 60,
+};
+
struct ad4000_chip_info {
const char *dev_name;
struct iio_chan_spec chan_spec[2];
struct iio_chan_spec reg_access_chan_spec[2];
+ const struct ad4000_time_spec *time_spec;
bool has_hardware_gain;
};
@@ -133,90 +150,105 @@ static const struct ad4000_chip_info ad4000_chip_info = {
.dev_name = "ad4000",
.chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
.reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4001_chip_info = {
.dev_name = "ad4001",
.chan_spec = AD4000_DIFF_CHANNELS('s', 16, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 16, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4002_chip_info = {
.dev_name = "ad4002",
.chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 0),
.reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4003_chip_info = {
.dev_name = "ad4003",
.chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 18, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4004_chip_info = {
.dev_name = "ad4004",
.chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
.reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4005_chip_info = {
.dev_name = "ad4005",
.chan_spec = AD4000_DIFF_CHANNELS('s', 16, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 16, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4006_chip_info = {
.dev_name = "ad4006",
.chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 0),
.reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4007_chip_info = {
.dev_name = "ad4007",
.chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 18, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4008_chip_info = {
.dev_name = "ad4008",
.chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
.reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4010_chip_info = {
.dev_name = "ad4010",
.chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 0),
.reg_access_chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 18, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4011_chip_info = {
.dev_name = "ad4011",
.chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 18, 1),
+ .time_spec = &ad4000_t_spec,
};
static const struct ad4000_chip_info ad4020_chip_info = {
.dev_name = "ad4020",
.chan_spec = AD4000_DIFF_CHANNELS('s', 20, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 20, 1),
+ .time_spec = &ad4020_t_spec,
};
static const struct ad4000_chip_info ad4021_chip_info = {
.dev_name = "ad4021",
.chan_spec = AD4000_DIFF_CHANNELS('s', 20, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 20, 1),
+ .time_spec = &ad4020_t_spec,
};
static const struct ad4000_chip_info ad4022_chip_info = {
.dev_name = "ad4022",
.chan_spec = AD4000_DIFF_CHANNELS('s', 20, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 20, 1),
+ .time_spec = &ad4020_t_spec,
};
static const struct ad4000_chip_info adaq4001_chip_info = {
.dev_name = "adaq4001",
.chan_spec = AD4000_DIFF_CHANNELS('s', 16, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 16, 1),
+ .time_spec = &ad4000_t_spec,
.has_hardware_gain = true,
};
@@ -224,6 +256,7 @@ static const struct ad4000_chip_info adaq4003_chip_info = {
.dev_name = "adaq4003",
.chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
.reg_access_chan_spec = AD4000_DIFF_CHANNELS('s', 18, 1),
+ .time_spec = &ad4000_t_spec,
.has_hardware_gain = true,
};
@@ -238,6 +271,7 @@ struct ad4000_state {
bool span_comp;
u16 gain_milli;
int scale_tbl[AD4000_SCALE_OPTIONS][2];
+ const struct ad4000_time_spec *time_spec;
/*
* DMA (thus cache coherency maintenance) requires the transfer buffers
@@ -502,16 +536,15 @@ static const struct iio_info ad4000_info = {
static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
const struct iio_chan_spec *chan)
{
- unsigned int cnv_pulse_time = AD4000_TCONV_NS;
struct spi_transfer *xfers = st->xfers;
xfers[0].cs_change = 1;
- xfers[0].cs_change_delay.value = cnv_pulse_time;
+ xfers[0].cs_change_delay.value = st->time_spec->t_conv_ns;
xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
xfers[1].rx_buf = &st->scan.data;
xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
- xfers[1].delay.value = AD4000_TQUIET2_NS;
+ xfers[1].delay.value = st->time_spec->t_quiet2_ns;
xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
spi_message_init_with_transfers(&st->msg, st->xfers, 2);
@@ -529,7 +562,6 @@ static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
const struct iio_chan_spec *chan)
{
- unsigned int cnv_to_sdi_time = AD4000_TCONV_NS;
struct spi_transfer *xfers = st->xfers;
/*
@@ -537,7 +569,7 @@ static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
* going low.
*/
xfers[0].cs_off = 1;
- xfers[0].delay.value = cnv_to_sdi_time;
+ xfers[0].delay.value = st->time_spec->t_conv_ns;
xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
xfers[1].rx_buf = &st->scan.data;
@@ -576,6 +608,7 @@ static int ad4000_probe(struct spi_device *spi)
st = iio_priv(indio_dev);
st->spi = spi;
+ st->time_spec = chip->time_spec;
ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad4000_power_supplies),
ad4000_power_supplies);
--
2.45.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 4/4] iio: adc: ad4000: Add support for PulSAR devices
2024-11-19 12:53 [PATCH v3 0/4] Timestamp and PulSAR support for ad4000 Marcelo Schmitt
` (2 preceding siblings ...)
2024-11-19 12:54 ` [PATCH v3 3/4] iio: adc: ad4000: Use device specific timing for SPI transfers Marcelo Schmitt
@ 2024-11-19 12:54 ` Marcelo Schmitt
2024-11-24 13:19 ` Jonathan Cameron
3 siblings, 1 reply; 11+ messages in thread
From: Marcelo Schmitt @ 2024-11-19 12:54 UTC (permalink / raw)
To: lars, Michael.Hennerich, marcelo.schmitt, jic23, robh, krzk+dt,
conor+dt, marcelo.schmitt1
Cc: linux-iio, devicetree, linux-kernel, David Lechner
The ADI PulSAR series of single-channel devices comprises differential and
pseudo-differential ADCs that don't require any input data from the host
controller. By not requiring a data input line, PulSAR devices can operate
with a 3-wire only data bus in some setups.
The AD4000 series and the single-channel PulSAR series of devices have
similar SPI transfer specifications and wiring configurations.
Single-channel PulSAR devices are slower than AD4000 and don't have a
configuration register. That taken into account, single-channel PulSARs can
be supported by the ad4000 driver without any increase in code complexity.
Extend the AD4000 driver to also support single-channel PulSAR devices.
Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
drivers/iio/adc/ad4000.c | 162 +++++++++++++++++++++++++++++++++++++++
1 file changed, 162 insertions(+)
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
index c700d51b5637..74b8894d1a2a 100644
--- a/drivers/iio/adc/ad4000.c
+++ b/drivers/iio/adc/ad4000.c
@@ -138,6 +138,48 @@ static const struct ad4000_time_spec ad4020_t_spec = {
.t_quiet2_ns = 60,
};
+/* AD7983, AD7984 */
+static const struct ad4000_time_spec ad7983_t_spec = {
+ .t_conv_ns = 500,
+ .t_quiet2_ns = 0,
+};
+
+/* AD7980, AD7982 */
+static const struct ad4000_time_spec ad7980_t_spec = {
+ .t_conv_ns = 800,
+ .t_quiet2_ns = 0,
+};
+
+/* AD7946, AD7686, AD7688, AD7988-5, AD7693 */
+static const struct ad4000_time_spec ad7686_t_spec = {
+ .t_conv_ns = 1600,
+ .t_quiet2_ns = 0,
+};
+
+/* AD7690 */
+static const struct ad4000_time_spec ad7690_t_spec = {
+ .t_conv_ns = 2100,
+ .t_quiet2_ns = 0,
+};
+
+/* AD7942, AD7685, AD7687 */
+static const struct ad4000_time_spec ad7687_t_spec = {
+ .t_conv_ns = 3200,
+ .t_quiet2_ns = 0,
+};
+
+/* AD7691 */
+static const struct ad4000_time_spec ad7691_t_spec = {
+ .t_conv_ns = 3700,
+ .t_quiet2_ns = 0,
+};
+
+/* AD7988-1 */
+static const struct ad4000_time_spec ad7988_1_t_spec = {
+ .t_conv_ns = 9500,
+ .t_quiet2_ns = 0,
+};
+
struct ad4000_chip_info {
const char *dev_name;
struct iio_chan_spec chan_spec[2];
@@ -260,6 +302,96 @@ static const struct ad4000_chip_info adaq4003_chip_info = {
.has_hardware_gain = true,
};
+static const struct ad4000_chip_info ad7685_chip_info = {
+ .dev_name = "ad7685",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
+ .time_spec = &ad7687_t_spec,
+};
+
+static const struct ad4000_chip_info ad7686_chip_info = {
+ .dev_name = "ad7686",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
+ .time_spec = &ad7686_t_spec,
+};
+
+static const struct ad4000_chip_info ad7687_chip_info = {
+ .dev_name = "ad7687",
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 16, 0),
+ .time_spec = &ad7687_t_spec,
+};
+
+static const struct ad4000_chip_info ad7688_chip_info = {
+ .dev_name = "ad7688",
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 16, 0),
+ .time_spec = &ad7686_t_spec,
+};
+
+static const struct ad4000_chip_info ad7690_chip_info = {
+ .dev_name = "ad7690",
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
+ .time_spec = &ad7690_t_spec,
+};
+
+static const struct ad4000_chip_info ad7691_chip_info = {
+ .dev_name = "ad7691",
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
+ .time_spec = &ad7691_t_spec,
+};
+
+static const struct ad4000_chip_info ad7693_chip_info = {
+ .dev_name = "ad7693",
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 16, 0),
+ .time_spec = &ad7686_t_spec,
+};
+
+static const struct ad4000_chip_info ad7942_chip_info = {
+ .dev_name = "ad7942",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 14, 0),
+ .time_spec = &ad7687_t_spec,
+};
+
+static const struct ad4000_chip_info ad7946_chip_info = {
+ .dev_name = "ad7946",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 14, 0),
+ .time_spec = &ad7686_t_spec,
+};
+
+static const struct ad4000_chip_info ad7980_chip_info = {
+ .dev_name = "ad7980",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
+ .time_spec = &ad7980_t_spec,
+};
+
+static const struct ad4000_chip_info ad7982_chip_info = {
+ .dev_name = "ad7982",
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
+ .time_spec = &ad7980_t_spec,
+};
+
+static const struct ad4000_chip_info ad7983_chip_info = {
+ .dev_name = "ad7983",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
+ .time_spec = &ad7983_t_spec,
+};
+
+static const struct ad4000_chip_info ad7984_chip_info = {
+ .dev_name = "ad7984",
+ .chan_spec = AD4000_DIFF_CHANNELS('s', 18, 0),
+ .time_spec = &ad7983_t_spec,
+};
+
+static const struct ad4000_chip_info ad7988_1_chip_info = {
+ .dev_name = "ad7988-1",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
+ .time_spec = &ad7988_1_t_spec,
+};
+
+static const struct ad4000_chip_info ad7988_5_chip_info = {
+ .dev_name = "ad7988-5",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNELS('u', 16, 0),
+ .time_spec = &ad7686_t_spec,
+};
+
struct ad4000_state {
struct spi_device *spi;
struct gpio_desc *cnv_gpio;
@@ -733,6 +865,21 @@ static const struct spi_device_id ad4000_id[] = {
{ "ad4022", (kernel_ulong_t)&ad4022_chip_info },
{ "adaq4001", (kernel_ulong_t)&adaq4001_chip_info },
{ "adaq4003", (kernel_ulong_t)&adaq4003_chip_info },
+ { "ad7685", (kernel_ulong_t)&ad7685_chip_info },
+ { "ad7686", (kernel_ulong_t)&ad7686_chip_info },
+ { "ad7687", (kernel_ulong_t)&ad7687_chip_info },
+ { "ad7688", (kernel_ulong_t)&ad7688_chip_info },
+ { "ad7690", (kernel_ulong_t)&ad7690_chip_info },
+ { "ad7691", (kernel_ulong_t)&ad7691_chip_info },
+ { "ad7693", (kernel_ulong_t)&ad7693_chip_info },
+ { "ad7942", (kernel_ulong_t)&ad7942_chip_info },
+ { "ad7946", (kernel_ulong_t)&ad7946_chip_info },
+ { "ad7980", (kernel_ulong_t)&ad7980_chip_info },
+ { "ad7982", (kernel_ulong_t)&ad7982_chip_info },
+ { "ad7983", (kernel_ulong_t)&ad7983_chip_info },
+ { "ad7984", (kernel_ulong_t)&ad7984_chip_info },
+ { "ad7988-1", (kernel_ulong_t)&ad7988_1_chip_info },
+ { "ad7988-5", (kernel_ulong_t)&ad7988_5_chip_info },
{ }
};
MODULE_DEVICE_TABLE(spi, ad4000_id);
@@ -754,6 +901,21 @@ static const struct of_device_id ad4000_of_match[] = {
{ .compatible = "adi,ad4022", .data = &ad4022_chip_info },
{ .compatible = "adi,adaq4001", .data = &adaq4001_chip_info },
{ .compatible = "adi,adaq4003", .data = &adaq4003_chip_info },
+ { .compatible = "adi,ad7685", .data = &ad7685_chip_info },
+ { .compatible = "adi,ad7686", .data = &ad7686_chip_info },
+ { .compatible = "adi,ad7687", .data = &ad7687_chip_info },
+ { .compatible = "adi,ad7688", .data = &ad7688_chip_info },
+ { .compatible = "adi,ad7690", .data = &ad7690_chip_info },
+ { .compatible = "adi,ad7691", .data = &ad7691_chip_info },
+ { .compatible = "adi,ad7693", .data = &ad7693_chip_info },
+ { .compatible = "adi,ad7942", .data = &ad7942_chip_info },
+ { .compatible = "adi,ad7946", .data = &ad7946_chip_info },
+ { .compatible = "adi,ad7980", .data = &ad7980_chip_info },
+ { .compatible = "adi,ad7982", .data = &ad7982_chip_info },
+ { .compatible = "adi,ad7983", .data = &ad7983_chip_info },
+ { .compatible = "adi,ad7984", .data = &ad7984_chip_info },
+ { .compatible = "adi,ad7988-1", .data = &ad7988_1_chip_info },
+ { .compatible = "adi,ad7988-5", .data = &ad7988_5_chip_info },
{ }
};
MODULE_DEVICE_TABLE(of, ad4000_of_match);
--
2.45.2
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 4/4] iio: adc: ad4000: Add support for PulSAR devices
2024-11-19 12:54 ` [PATCH v3 4/4] iio: adc: ad4000: Add support for PulSAR devices Marcelo Schmitt
@ 2024-11-24 13:19 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2024-11-24 13:19 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: lars, Michael.Hennerich, robh, krzk+dt, conor+dt,
marcelo.schmitt1, linux-iio, devicetree, linux-kernel,
David Lechner
On Tue, 19 Nov 2024 09:54:39 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:
> The ADI PulSAR series of single-channel devices comprises differential and
> pseudo-differential ADCs that don't require any input data from the host
> controller. By not requiring a data input line, PulSAR devices can operate
> with a 3-wire only data bus in some setups.
>
> The AD4000 series and the single-channel PulSAR series of devices have
> similar SPI transfer specifications and wiring configurations.
> Single-channel PulSAR devices are slower than AD4000 and don't have a
> configuration register. That taken into account, single-channel PulSARs can
> be supported by the ad4000 driver without any increase in code complexity.
>
> Extend the AD4000 driver to also support single-channel PulSAR devices.
>
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
The series looks fine to be other than the remaining DT discussion.
Fingers crossed for v4 :)
Jonathan
^ permalink raw reply [flat|nested] 11+ messages in thread