[PATCH v2] dt-bindings:iio:adc:st,spear600-adc: txt to yaml format conversion.

[PATCH v2] dt-bindings:iio:adc:st,spear600-adc: txt to yaml format conversion.

[Rodrigo Gobbi]

Straight forward conversion from spear-adc.txt into yaml format.

Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
---
Some constraints were extracted from the driver (spear_adc.c) and the public datasheet
referenced at the yaml.

Changelog:
v2: add constraints over properties and remove a ref at MAINTAINERS file.
v1: https://lore.kernel.org/linux-devicetree/20250423022956.31218-1-rodrigo.gobbi.7@gmail.com/
---
 .../bindings/iio/adc/st,spear600-adc.yaml     | 69 +++++++++++++++++++
 .../bindings/staging/iio/adc/spear-adc.txt    | 24 -------
 MAINTAINERS                                   |  2 +-
 3 files changed, 70 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml
 delete mode 100644 Documentation/devicetree/bindings/staging/iio/adc/spear-adc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml
new file mode 100644
index 000000000000..afce10eab1c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/st,spear600-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ST SPEAr ADC device driver
+
+maintainers:
+  - Jonathan Cameron <jic23@kernel.org>
+
+description: |
+  Integrated ADC inside the ST SPEAr SoC, SPEAr600, supporting
+  10-bit resolution. Datasheet can be found here:
+  https://www.st.com/resource/en/datasheet/spear600.pdf
+
+properties:
+  compatible:
+    enum:
+      - st,spear600-adc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  sampling-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 2500000
+    maximum: 20000000
+    description:
+      Default sampling frequency of the ADC
+
+  vref-external:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1000
+    maximum: 2800
+    description:
+      External voltage reference in milli-volts. If omitted
+      the internal voltage reference will be used.
+
+  average-samples:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 128
+    default: 0
+    description:
+      Number of samples to generate an average value. If
+      omitted, single data conversion will be used.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - sampling-frequency
+
+additionalProperties: false
+
+examples:
+  - |
+    adc: adc@d8200000 {
+        compatible = "st,spear600-adc";
+        reg = <0xd8200000 0x1000>;
+        interrupt-parent = <&vic1>;
+        interrupts = <6>;
+        sampling-frequency = <5000000>;
+        vref-external = <2500>;	/* 2.5V VRef */
+    };
diff --git a/Documentation/devicetree/bindings/staging/iio/adc/spear-adc.txt b/Documentation/devicetree/bindings/staging/iio/adc/spear-adc.txt
deleted file mode 100644
index 88bc94fe1f6d..000000000000
--- a/Documentation/devicetree/bindings/staging/iio/adc/spear-adc.txt
+++ /dev/null
@@ -1,24 +0,0 @@
-* ST SPEAr ADC device driver
-
-Required properties:
-- compatible: Should be "st,spear600-adc"
-- reg: Address and length of the register set for the device
-- interrupts: Should contain the ADC interrupt
-- sampling-frequency: Default sampling frequency
-
-Optional properties:
-- vref-external: External voltage reference in milli-volts. If omitted
-  the internal voltage reference will be used.
-- average-samples: Number of samples to generate an average value. If
-  omitted, single data conversion will be used.
-
-Examples:
-
-	adc: adc@d8200000 {
-		compatible = "st,spear600-adc";
-		reg = <0xd8200000 0x1000>;
-		interrupt-parent = <&vic1>;
-		interrupts = <6>;
-		sampling-frequency = <5000000>;
-		vref-external = <2500>;	/* 2.5V VRef */
-	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 906881b6c5cb..e923becb0633 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23213,7 +23213,7 @@ STAGING - INDUSTRIAL IO
 M:	Jonathan Cameron <jic23@kernel.org>
 L:	linux-iio@vger.kernel.org
 S:	Odd Fixes
-F:	Documentation/devicetree/bindings/staging/iio/
+F:	Documentation/devicetree/bindings/iio/
 F:	drivers/staging/iio/
 
 STAGING - NVIDIA COMPLIANT EMBEDDED CONTROLLER INTERFACE (nvec)
-- 
2.47.0

Re: [PATCH v2] dt-bindings:iio:adc:st,spear600-adc: txt to yaml format conversion.

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5262 bytes --]

Hey,

On Tue, Apr 29, 2025 at 10:50:01PM -0300, Rodrigo Gobbi wrote:
> Straight forward conversion from spear-adc.txt into yaml format.
> 
> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
> ---
> Some constraints were extracted from the driver (spear_adc.c) and the public datasheet
> referenced at the yaml.
> 
> Changelog:
> v2: add constraints over properties and remove a ref at MAINTAINERS file.
> v1: https://lore.kernel.org/linux-devicetree/20250423022956.31218-1-rodrigo.gobbi.7@gmail.com/
> ---
>  .../bindings/iio/adc/st,spear600-adc.yaml     | 69 +++++++++++++++++++
>  .../bindings/staging/iio/adc/spear-adc.txt    | 24 -------
>  MAINTAINERS                                   |  2 +-
>  3 files changed, 70 insertions(+), 25 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml
>  delete mode 100644 Documentation/devicetree/bindings/staging/iio/adc/spear-adc.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml
> new file mode 100644
> index 000000000000..afce10eab1c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/st,spear600-adc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ST SPEAr ADC device driver
> +
> +maintainers:
> +  - Jonathan Cameron <jic23@kernel.org>
> +
> +description: |
> +  Integrated ADC inside the ST SPEAr SoC, SPEAr600, supporting
> +  10-bit resolution. Datasheet can be found here:
> +  https://www.st.com/resource/en/datasheet/spear600.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - st,spear600-adc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  sampling-frequency:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 2500000
> +    maximum: 20000000
> +    description:
> +      Default sampling frequency of the ADC

I think you should note that this is in Hz, while you're at it.

> +  vref-external:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1000
> +    maximum: 2800
> +    description:
> +      External voltage reference in milli-volts. If omitted
> +      the internal voltage reference will be used.
> +
> +  average-samples:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 128
> +    default: 0

Is 0 the default here or 1? "Single data conversion" sounds more like 1
sample than 0, and the default of 0 is below the minimum of 1. What's
going on there?


> +    description:
> +      Number of samples to generate an average value. If
> +      omitted, single data conversion will be used.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - sampling-frequency
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    adc: adc@d8200000 {

The "adc:" label here can be removed, it's not doing anything!

> +        compatible = "st,spear600-adc";
> +        reg = <0xd8200000 0x1000>;
> +        interrupt-parent = <&vic1>;
> +        interrupts = <6>;
> +        sampling-frequency = <5000000>;
> +        vref-external = <2500>;	/* 2.5V VRef */
> +    };
> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/spear-adc.txt b/Documentation/devicetree/bindings/staging/iio/adc/spear-adc.txt
> deleted file mode 100644
> index 88bc94fe1f6d..000000000000
> --- a/Documentation/devicetree/bindings/staging/iio/adc/spear-adc.txt
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -* ST SPEAr ADC device driver
> -
> -Required properties:
> -- compatible: Should be "st,spear600-adc"
> -- reg: Address and length of the register set for the device
> -- interrupts: Should contain the ADC interrupt
> -- sampling-frequency: Default sampling frequency
> -
> -Optional properties:
> -- vref-external: External voltage reference in milli-volts. If omitted
> -  the internal voltage reference will be used.
> -- average-samples: Number of samples to generate an average value. If
> -  omitted, single data conversion will be used.
> -
> -Examples:
> -
> -	adc: adc@d8200000 {
> -		compatible = "st,spear600-adc";
> -		reg = <0xd8200000 0x1000>;
> -		interrupt-parent = <&vic1>;
> -		interrupts = <6>;
> -		sampling-frequency = <5000000>;
> -		vref-external = <2500>;	/* 2.5V VRef */
> -	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 906881b6c5cb..e923becb0633 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23213,7 +23213,7 @@ STAGING - INDUSTRIAL IO
>  M:	Jonathan Cameron <jic23@kernel.org>
>  L:	linux-iio@vger.kernel.org
>  S:	Odd Fixes
> -F:	Documentation/devicetree/bindings/staging/iio/
> +F:	Documentation/devicetree/bindings/iio/

This change seems unneeded? The main iio entry already covers this
directory. I think you can probably just drop the line from the staging
entry?

Cheers,
Conor.

>  F:	drivers/staging/iio/
>  
>  STAGING - NVIDIA COMPLIANT EMBEDDED CONTROLLER INTERFACE (nvec)
> -- 
> 2.47.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] dt-bindings:iio:adc:st,spear600-adc: txt to yaml format conversion.

[Rodrigo Gobbi]

> Is 0 the default here or 1? "Single data conversion" sounds more like 1
> sample than 0, and the default of 0 is below the minimum of 1. What's
> going on there?

Good point, after I`ve submitted the patch I was double checking it and noticed 
that too. It`s stange because the public datasheet mentions "Programmable averaging of results 
from 1 (No averaging) up to 128". Meanwhile, the spear_adc.c driver at probe
stated the following:

	/*
	 * Optional avg_samples defaults to 0, resulting in single data
	 * conversion
	 */
	device_property_read_u32(dev, "average-samples", &st->avg_samples);

Since avg_samples is inside 

	struct spear_adc_state *st;

which is allocated with devm_iio_device_alloc() (which uses the kzalloc/zero filling the priv data):

	indio_dev = devm_iio_device_alloc(dev, sizeof(struct spear_adc_state));
	if (!indio_dev)
		return dev_err_probe(dev, -ENOMEM,
				     "failed allocating iio device\n");

	st = iio_priv(indio_dev);

...matches the driver comment meaning the default is actually "0", single data, but it does
not match the public datasheet in my understanding. Since I don`t have access to a more
detailed datasheet, I chose to describe "1" as a minimum value, but I agree it is weird.
Maybe we could drop the minimum constraint in this case (go with default and max)?
Tks and regards.

Re: [PATCH v2] dt-bindings:iio:adc:st,spear600-adc: txt to yaml format conversion.

[Jonathan Cameron]

On Tue, 29 Apr 2025 22:50:01 -0300
Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com> wrote:

> Straight forward conversion from spear-adc.txt into yaml format.
> 
> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
> ---
> Some constraints were extracted from the driver (spear_adc.c) and the public datasheet
> referenced at the yaml.
> 
> Changelog:
> v2: add constraints over properties and remove a ref at MAINTAINERS file.
> v1: https://lore.kernel.org/linux-devicetree/20250423022956.31218-1-rodrigo.gobbi.7@gmail.com/
> ---
>  .../bindings/iio/adc/st,spear600-adc.yaml     | 69 +++++++++++++++++++
>  .../bindings/staging/iio/adc/spear-adc.txt    | 24 -------
>  MAINTAINERS                                   |  2 +-
>  3 files changed, 70 insertions(+), 25 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml
>  delete mode 100644 Documentation/devicetree/bindings/staging/iio/adc/spear-adc.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml
> new file mode 100644
> index 000000000000..afce10eab1c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/st,spear600-adc.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/st,spear600-adc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ST SPEAr ADC device driver
> +
> +maintainers:
> +  - Jonathan Cameron <jic23@kernel.org>
> +
> +description: |
> +  Integrated ADC inside the ST SPEAr SoC, SPEAr600, supporting
> +  10-bit resolution. Datasheet can be found here:
> +  https://www.st.com/resource/en/datasheet/spear600.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - st,spear600-adc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  sampling-frequency:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 2500000
> +    maximum: 20000000
> +    description:
> +      Default sampling frequency of the ADC
For consistency probably wants a full stop.  (as well as units)

> +
> +  vref-external:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1000
> +    maximum: 2800
> +    description:
> +      External voltage reference in milli-volts. If omitted

Very short wrap. Go up to 80 chars.

> +      the internal voltage reference will be used.
> +
> +  average-samples:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 128
> +    default: 0
> +    description:
> +      Number of samples to generate an average value. If
> +      omitted, single data conversion will be used.
It is odd to have a default outside of range.  I'll wait
to see what Conor suggests for that. I'm fine with anything
that doesn't trip up the dt parsing!

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - sampling-frequency
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    adc: adc@d8200000 {
> +        compatible = "st,spear600-adc";
> +        reg = <0xd8200000 0x1000>;
> +        interrupt-parent = <&vic1>;
> +        interrupts = <6>;
> +        sampling-frequency = <5000000>;
> +        vref-external = <2500>;	/* 2.5V VRef */
> +    };

Re: [PATCH v2] dt-bindings:iio:adc:st,spear600-adc: txt to yaml format conversion.

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

On Sat, May 03, 2025 at 03:44:12PM -0300, Rodrigo Gobbi wrote:
> > Is 0 the default here or 1? "Single data conversion" sounds more like 1
> > sample than 0, and the default of 0 is below the minimum of 1. What's
> > going on there?
> 
> Good point, after I`ve submitted the patch I was double checking it and noticed 
> that too. It`s stange because the public datasheet mentions "Programmable averaging of results 
> from 1 (No averaging) up to 128". Meanwhile, the spear_adc.c driver at probe
> stated the following:
> 
> 	/*
> 	 * Optional avg_samples defaults to 0, resulting in single data
> 	 * conversion
> 	 */
> 	device_property_read_u32(dev, "average-samples", &st->avg_samples);
> 
> Since avg_samples is inside 
> 
> 	struct spear_adc_state *st;
> 
> which is allocated with devm_iio_device_alloc() (which uses the kzalloc/zero filling the priv data):
> 
> 	indio_dev = devm_iio_device_alloc(dev, sizeof(struct spear_adc_state));
> 	if (!indio_dev)
> 		return dev_err_probe(dev, -ENOMEM,
> 				     "failed allocating iio device\n");
> 
> 	st = iio_priv(indio_dev);
> 
> ...matches the driver comment meaning the default is actually "0", single data, but it does
> not match the public datasheet in my understanding. Since I don`t have access to a more
> detailed datasheet, I chose to describe "1" as a minimum value, but I agree it is weird.
> Maybe we could drop the minimum constraint in this case (go with default and max)?
> Tks and regards.

Sounds like it's a 4-bit register where the samples is (1 + written value),
and the property is expected to be written directly to the register.
I'd then expect the property to be min 0, default 0, max 127. If you
write 128 to the register, you'll accidentally set the external vref
bit. I'd maybe go as far as &ing the value to make sure out of range
stuff is not permitted?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] dt-bindings:iio:adc:st,spear600-adc: txt to yaml format conversion.

[Rodrigo Gobbi]

> Sounds like it's a 4-bit register where the samples is (1 + written value),
> and the property is expected to be written directly to the register.
> I'd then expect the property to be min 0, default 0, max 127. If you
> write 128 to the register, you'll accidentally set the external vref
> bit. I'd maybe go as far as &ing the value to make sure out of range
> stuff is not permitted?

Well, it looks like 4bit as you said (bits 5,6,7 and 8) and 9th bit is vref.
But, in this case, it looks to me that we can only configure 0x15 as a max value,
(didn`t see that before) which doesn`t fit the datasheet sentence that I`ve mentioned before:

"Programmable averaging of results from 1 (No averaging) up to 128"

I mean, I`m not sure how many samples are configured when using
SPEAR_ADC_STATUS_AVG_SAMPLE(0xD) since we don`t have a register map describing
how it encodes that internally. Maybe we can change the requirements for this field to be
min 0, default 0, max 15?
And at the drive side, we could use &ing as you said with 0xf before shiffting?
If you have a different suggestion, I would appreciate it.

...
#define SPEAR_ADC_STATUS_AVG_SAMPLE(x)		((x) << 5)
#define SPEAR_ADC_STATUS_VREF_INTERNAL		BIT(9)

...
		status = SPEAR_ADC_STATUS_CHANNEL_NUM(chan->channel) |
			SPEAR_ADC_STATUS_AVG_SAMPLE(st->avg_samples) |
			SPEAR_ADC_STATUS_START_CONVERSION |
			SPEAR_ADC_STATUS_ADC_ENABLE;

Tks and regards.

Re: [PATCH v2] dt-bindings:iio:adc:st,spear600-adc: txt to yaml format conversion.

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

On Tue, May 20, 2025 at 05:43:45PM -0300, Rodrigo Gobbi wrote:
> > Sounds like it's a 4-bit register where the samples is (1 + written value),
> > and the property is expected to be written directly to the register.
> > I'd then expect the property to be min 0, default 0, max 127. If you
> > write 128 to the register, you'll accidentally set the external vref
> > bit. I'd maybe go as far as &ing the value to make sure out of range
> > stuff is not permitted?
> 
> Well, it looks like 4bit as you said (bits 5,6,7 and 8) and 9th bit is vref.
> But, in this case, it looks to me that we can only configure 0x15 as a max value,
> (didn`t see that before) which doesn`t fit the datasheet sentence that I`ve mentioned before:
> 
> "Programmable averaging of results from 1 (No averaging) up to 128"
> 
> I mean, I`m not sure how many samples are configured when using
> SPEAR_ADC_STATUS_AVG_SAMPLE(0xD) since we don`t have a register map describing
> how it encodes that internally. Maybe we can change the requirements for this field to be
> min 0, default 0, max 15?

I think so, since that matches the behaviour that's been there in the
driver since the property was introduced.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]