[v3] Add SARADC support on Sophgo SoC

[PATCH v3 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo SARADC binding documentation

Posted by Thomas Bonnefille 1 year, 4 months ago

The Sophgo SARADC is a Successive Approximation ADC that can be found in
the Sophgo SoC.

Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>
---
 .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
new file mode 100644
index 000000000000..79d8cb52279f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-saradc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title:
+  Sophgo CV18XX SoC series 3 channels Successive Approximation Analog to
+  Digital Converters
+
+maintainers:
+  - Thomas Bonnefille <thomas.bonnefille@bootlin.com>
+
+description:
+  Datasheet at https://github.com/sophgo/sophgo-doc/releases
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - const: sophgo,cv1800b-saradc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - clocks
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sophgo,cv1800.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    adc@30f0000 {
+        compatible = "sophgo,cv1800b-saradc";
+        clocks = <&clk CLK_SARADC>;
+        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
+        reg = <0x030F0000 0x1000>;
+    };

-- 
2.45.2

Re: [PATCH v3 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo SARADC binding documentation

Posted by Krzysztof Kozlowski 1 year, 4 months ago

On 31/07/2024 14:24, Thomas Bonnefille wrote:
> The Sophgo SARADC is a Successive Approximation ADC that can be found in
> the Sophgo SoC.
> 
> Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>

A nit, subject: drop second/last, redundant "binding documentation". The
"dt-bindings" prefix is already stating that these are bindings and this
is documentation. Cannot be anything else.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


> ---
>  .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> new file mode 100644
> index 000000000000..79d8cb52279f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-saradc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title:
> +  Sophgo CV18XX SoC series 3 channels Successive Approximation Analog to
> +  Digital Converters
> +
> +maintainers:
> +  - Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> +
> +description:
> +  Datasheet at https://github.com/sophgo/sophgo-doc/releases
> +
> +properties:
> +  compatible:
> +    oneOf:

Drop

> +      - items:

Drop, use const directly.


> +          - const: sophgo,cv1800b-saradc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - clocks
> +  - compatible
> +  - reg

Odd order. compatible is always first. Anyway, list here has the same
order as "properties:".

> +
> +unevaluatedProperties: false

I don't see any other $ref. Don't you miss adc.yaml? Or channels? Or
some more properties? This looks incomplete for ADC.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/sophgo,cv1800.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    adc@30f0000 {
> +        compatible = "sophgo,cv1800b-saradc";
> +        clocks = <&clk CLK_SARADC>;
> +        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> +        reg = <0x030F0000 0x1000>;

Hex is *always* lower case, see DTS coding style.

Order the properties correctly, so again, please read DTS coding style.

> +    };
> 

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo SARADC binding documentation

Posted by Jonathan Cameron 1 year, 4 months ago

On Wed, 31 Jul 2024 15:57:27 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 31/07/2024 14:24, Thomas Bonnefille wrote:
> > The Sophgo SARADC is a Successive Approximation ADC that can be found in
> > the Sophgo SoC.
> > 
> > Signed-off-by: Thomas Bonnefille <thomas.bonnefille@bootlin.com>  
> 
> A nit, subject: drop second/last, redundant "binding documentation". The
> "dt-bindings" prefix is already stating that these are bindings and this
> is documentation. Cannot be anything else.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> 
> > ---
> >  .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 48 ++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> > new file mode 100644
> > index 000000000000..79d8cb52279f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> > @@ -0,0 +1,48 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/sophgo,cv18xx-saradc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title:
> > +  Sophgo CV18XX SoC series 3 channels Successive Approximation Analog to
> > +  Digital Converters
> > +
> > +maintainers:
> > +  - Thomas Bonnefille <thomas.bonnefille@bootlin.com>
> > +
> > +description:
> > +  Datasheet at https://github.com/sophgo/sophgo-doc/releases
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:  
> 
> Drop
> 
> > +      - items:  
> 
> Drop, use const directly.
> 
> 
> > +          - const: sophgo,cv1800b-saradc
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +required:
> > +  - clocks
> > +  - compatible
> > +  - reg  
> 
> Odd order. compatible is always first. Anyway, list here has the same
> order as "properties:".
> 
> > +
> > +unevaluatedProperties: false  
> 
> I don't see any other $ref. Don't you miss adc.yaml? Or channels? Or
> some more properties? This looks incomplete for ADC.

It's acceptable on ADCs in particular (but more generally)
to just assume all channels are exposed.  They may all be wired
to internal power lines anyway, in which case what is there is
a chip feature.  This only works if their isn't any channel specific
configuration, then not providing the channels child nodes is fine.

However, that requires us to be fairly sure there won't ever be
a per channel thing that needs configuring from DT.
That's generally only the case in simple devices.

So might be better to put the channels nodes in there and deal with
dynamic channel configuration (so don't present any without
a channel node) from the start as it's more future proof.


> 
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/sophgo,cv1800.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    adc@30f0000 {
> > +        compatible = "sophgo,cv1800b-saradc";
> > +        clocks = <&clk CLK_SARADC>;
> > +        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> > +        reg = <0x030F0000 0x1000>;  
> 
> Hex is *always* lower case, see DTS coding style.
> 
> Order the properties correctly, so again, please read DTS coding style.
> 
> > +    };
> >   
> 
> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/3] dt-bindings: iio: adc: sophgo,cv18xx-saradc.yaml: Add Sophgo SARADC binding documentation

Posted by Krzysztof Kozlowski 1 year, 4 months ago

On 03/08/2024 12:39, Jonathan Cameron wrote:
>>
>>> +
>>> +unevaluatedProperties: false  
>>
>> I don't see any other $ref. Don't you miss adc.yaml? Or channels? Or
>> some more properties? This looks incomplete for ADC.
> 
> It's acceptable on ADCs in particular (but more generally)
> to just assume all channels are exposed.  They may all be wired
> to internal power lines anyway, in which case what is there is
> a chip feature.  This only works if their isn't any channel specific
> configuration, then not providing the channels child nodes is fine.
> 
> However, that requires us to be fairly sure there won't ever be
> a per channel thing that needs configuring from DT.
> That's generally only the case in simple devices.
> 
> So might be better to put the channels nodes in there and deal with
> dynamic channel configuration (so don't present any without
> a channel node) from the start as it's more future proof.

OK. Then anyway this should be additionalProperties: false (unless I
missed somewhere $ref?).

Best regards,
Krzysztof