diff mbox series

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

Message ID 20240812-sg2002-adc-v4-1-599bdb67592f@bootlin.com (mailing list archive)
State Changes Requested
Headers show
Series Add SARADC support on Sophgo CV18XX series | expand

Commit Message

Thomas Bonnefille Aug. 12, 2024, 3 p.m. UTC
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     | 85 ++++++++++++++++++++++
 1 file changed, 85 insertions(+)

Comments

Conor Dooley Aug. 12, 2024, 3:53 p.m. UTC | #1
On Mon, Aug 12, 2024 at 05:00:55PM +0200, 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>
> ---
>  .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 85 ++++++++++++++++++++++
>  1 file changed, 85 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..846590808e5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> @@ -0,0 +1,85 @@
> +# 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#

Filename matching the compatible please.

> +$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:
> +    const: sophgo,cv1800b-saradc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  "^channel@[0-3]+$":
> +    $ref: adc.yaml
> +
> +    description: |

This | is not required.

> +      Represents the channels of the ADC.
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> +        items:
> +          - minimum: 0
> +            maximum: 2

Is this sufficient to limit the number of channels to 3? Aren't you relying
on the unique unit addresses warning in dtc to limit it, rather than
actually limiting with min/maxItems?

Otherwise, looks fine to me.

Cheers,
Conor.

> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/sophgo,cv1800.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    adc@30f0000 {
> +        compatible = "sophgo,cv1800b-saradc";
> +        reg = <0x030f0000 0x1000>;
> +        clocks = <&clk CLK_SARADC>;
> +        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        channel@0 {
> +            reg = <0>;
> +        };
> +        channel@1 {
> +            reg = <1>;
> +        };
> +        channel@2 {
> +            reg = <2>;
> +        };
> +    };
> 
> -- 
> 2.46.0
>
Krzysztof Kozlowski Aug. 13, 2024, 9:50 a.m. UTC | #2
On 12/08/2024 17:00, 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>


<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

.. and more ignored comments further.

> ---
>  .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 85 ++++++++++++++++++++++
>  1 file changed, 85 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..846590808e5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> @@ -0,0 +1,85 @@
> +# 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:
> +    const: sophgo,cv1800b-saradc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  "^channel@[0-3]+$":
> +    $ref: adc.yaml
> +
> +    description: |
> +      Represents the channels of the ADC.
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> +        items:
> +          - minimum: 0
> +            maximum: 2
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
> +

Just one blank line.

> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +unevaluatedProperties: false

No, how did this appear here? This must be additionalProperties.

I already commented on this.

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

Best regards,
Krzysztof
Thomas Bonnefille Aug. 20, 2024, 4:21 p.m. UTC | #3
Hello Conor,

On 8/12/24 5:53 PM, Conor Dooley wrote:
> On Mon, Aug 12, 2024 at 05:00:55PM +0200, 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>
>> ---
>>   .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 85 ++++++++++++++++++++++
>>   1 file changed, 85 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..846590808e5f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
>> @@ -0,0 +1,85 @@
>> +# 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#
> 
> Filename matching the compatible please.
> 
>> +$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:
>> +    const: sophgo,cv1800b-saradc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +patternProperties:
>> +  "^channel@[0-3]+$":
>> +    $ref: adc.yaml
>> +
>> +    description: |
> 
> This | is not required.
> 
>> +      Represents the channels of the ADC.
>> +
>> +    properties:
>> +      reg:
>> +        description: |
>> +          The channel number. It can have up to 3 channels numbered from 0 to 2.
>> +        items:
>> +          - minimum: 0
>> +            maximum: 2
> 
> Is this sufficient to limit the number of channels to 3? Aren't you relying
> on the unique unit addresses warning in dtc to limit it, rather than
> actually limiting with min/maxItems?
> 
It seems like I can't use min/maxItems on this property. I think that it 
is using size-cells + address-cells to deduce that the number of items 
should be equal to 1.
Looking at the dtschema repository it seems to be the case in reg.yaml 
with address-cells/size-cells = 2/2, 1/1 and 2/1.
If I try to use maxItems here :

     properties:
       reg:
         maxItems: 1
         items:
           - minimum: 0
             maximum: 2

I get this strange error message from `make dt_binding_check`:

DTEX 
Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.example.dts
/home/thomas/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.yaml: 
patternProperties:^channel@[0-2]+$:properties:reg: {'maxItems': 1, 
'items': [{'minimum': 0, 'maximum': 2}]} should not be valid under 
{'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
/home/thomas/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.yaml: 
patternProperties:^channel@[0-2]+$:properties:reg: 'anyOf' conditional 
failed, one must be fixed:
	'items' is not one of ['maxItems', 'description', 'deprecated']
		hint: Only "maxItems" is required for a single entry if there are no 
constraints defined for the values.
	'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 
'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
	'items' is not one of ['description', 'deprecated', 'const', 'enum', 
'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
	1 is less than the minimum of 2
		hint: Arrays must be described with a combination of 
minItems/maxItems/items
	hint: cell array properties must define how many entries and what the 
entries are when there is more than one entry.
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

Isn't it okay to just use minimum and maximum and rely on 
address-cells/size-cells for the number of items allowed ?
Conor Dooley Aug. 20, 2024, 4:38 p.m. UTC | #4
On Tue, Aug 20, 2024 at 06:21:07PM +0200, Thomas Bonnefille wrote:
> Hello Conor,
> 
> On 8/12/24 5:53 PM, Conor Dooley wrote:
> > On Mon, Aug 12, 2024 at 05:00:55PM +0200, 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>
> > > ---
> > >   .../bindings/iio/adc/sophgo,cv18xx-saradc.yaml     | 85 ++++++++++++++++++++++
> > >   1 file changed, 85 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..846590808e5f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
> > > @@ -0,0 +1,85 @@
> > > +# 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#
> > 
> > Filename matching the compatible please.
> > 
> > > +$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:
> > > +    const: sophgo,cv1800b-saradc
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  '#address-cells':
> > > +    const: 1
> > > +
> > > +  '#size-cells':
> > > +    const: 0
> > > +
> > > +patternProperties:
> > > +  "^channel@[0-3]+$":
> > > +    $ref: adc.yaml
> > > +
> > > +    description: |
> > 
> > This | is not required.
> > 
> > > +      Represents the channels of the ADC.
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description: |
> > > +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> > > +        items:
> > > +          - minimum: 0
> > > +            maximum: 2
> > 
> > Is this sufficient to limit the number of channels to 3? Aren't you relying
> > on the unique unit addresses warning in dtc to limit it, rather than
> > actually limiting with min/maxItems?
> > 
> It seems like I can't use min/maxItems on this property. I think that it is
> using size-cells + address-cells to deduce that the number of items should
> be equal to 1.

I think I was mistaken in talking about mix/max items here. I had the
right idea, but mentioned an incorrect solution - sorry about that. I
wasn't talking about the number of elements in the reg property, what I
meant was limiting the number of channel nodes in the first place -
something which min/maxItems cannot do. As examples of the problem I was
thinking of, see the below two examples:

    adc@30f0000 {
        compatible = "sophgo,cv1800b-saradc";
        reg = <0x030f0000 0x1000>;
        clocks = <&clk CLK_SARADC>;
        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
        #address-cells = <1>;
        #size-cells = <0>;

        channel@0 {
            reg = <0>;
        };
        channel@2 {
            reg = <2>;
        };
        channel@22 {
            reg = <2>;
        };
    };

    adc@30f0000 {
        compatible = "sophgo,cv1800b-saradc";
        reg = <0x030f0000 0x1000>;
        clocks = <&clk CLK_SARADC>;
        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
        #address-cells = <1>;
        #size-cells = <0>;

        channel@0 {
            reg = <0>;
        };
        channel@2 {
            reg = <2>;
        };
        channel@22 {
            reg = <2>;
        };
    };

The solution is simple, remove the + from the regex. Sorry for sending
you on the wrong track Thomas.

Thanks,
Conor.

> Looking at the dtschema repository it seems to be the case in reg.yaml with
> address-cells/size-cells = 2/2, 1/1 and 2/1.
> If I try to use maxItems here :
> 
>     properties:
>       reg:
>         maxItems: 1
>         items:
>           - minimum: 0
>             maximum: 2
> 
> I get this strange error message from `make dt_binding_check`:
> 
> DTEX
> Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.example.dts
> /home/thomas/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.yaml:
> patternProperties:^channel@[0-2]+$:properties:reg: {'maxItems': 1, 'items':
> [{'minimum': 0, 'maximum': 2}]} should not be valid under {'required':
> ['maxItems']}
> 	hint: "maxItems" is not needed with an "items" list
> 	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
> /home/thomas/linux/Documentation/devicetree/bindings/iio/adc/sophgo,cv1800b-saradc.yaml:
> patternProperties:^channel@[0-2]+$:properties:reg: 'anyOf' conditional
> failed, one must be fixed:
> 	'items' is not one of ['maxItems', 'description', 'deprecated']
> 		hint: Only "maxItems" is required for a single entry if there are no
> constraints defined for the values.
> 	'maxItems' is not one of ['description', 'deprecated', 'const', 'enum',
> 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
> 	'items' is not one of ['description', 'deprecated', 'const', 'enum',
> 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
> 	1 is less than the minimum of 2
> 		hint: Arrays must be described with a combination of
> minItems/maxItems/items
> 	hint: cell array properties must define how many entries and what the
> entries are when there is more than one entry.
> 	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> 
> Isn't it okay to just use minimum and maximum and rely on
> address-cells/size-cells for the number of items allowed ?
>
Miquel Raynal Aug. 21, 2024, 7:41 a.m. UTC | #5
Hello,

> > > > +      Represents the channels of the ADC.
> > > > +
> > > > +    properties:
> > > > +      reg:
> > > > +        description: |
> > > > +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> > > > +        items:
> > > > +          - minimum: 0
> > > > +            maximum: 2  
> > > 
> > > Is this sufficient to limit the number of channels to 3? Aren't you relying
> > > on the unique unit addresses warning in dtc to limit it, rather than
> > > actually limiting with min/maxItems?
> > >   
> > It seems like I can't use min/maxItems on this property. I think that it is
> > using size-cells + address-cells to deduce that the number of items should
> > be equal to 1.  

Looking at dt-schema, I couldn't personally understand from where did
the error messages reported by Thomas came from. There are clear
constraints over minItems/maxItems regarding the use of {#address-cells,
#sizez-cells} being {1, 1}, {2, 2} and {2, 1} (in reg.yaml), but nothing
explicit regarding the other situations, namely {1, 0} in this case
which enforces maxItems to 1 is not clearly stated in any of the core
yaml files. Any idea where to look at? Although, I'm convinced there is
something defined because renaming the property from 'reg' to 'foo'
silences these warnings.

> I think I was mistaken in talking about mix/max items here. I had the
> right idea, but mentioned an incorrect solution - sorry about that. I
> wasn't talking about the number of elements in the reg property, what I
> meant was limiting the number of channel nodes in the first place -
> something which min/maxItems cannot do. As examples of the problem I was
> thinking of, see the below two examples:
> 
>     adc@30f0000 {
>         compatible = "sophgo,cv1800b-saradc";
>         reg = <0x030f0000 0x1000>;
>         clocks = <&clk CLK_SARADC>;
>         interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         channel@0 {
>             reg = <0>;
>         };
>         channel@2 {
>             reg = <2>;
>         };
>         channel@22 {
>             reg = <2>;
>         };
>     };
> 
>     adc@30f0000 {
>         compatible = "sophgo,cv1800b-saradc";
>         reg = <0x030f0000 0x1000>;
>         clocks = <&clk CLK_SARADC>;
>         interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         channel@0 {
>             reg = <0>;
>         };
>         channel@2 {
>             reg = <2>;
>         };
>         channel@22 {
>             reg = <2>;
>         };
>     };
> 
> The solution is simple, remove the + from the regex. Sorry for sending
> you on the wrong track Thomas.

Ah! Thanks Conor for the details, now it makes full sense :-) BTW Thomas
the regex is

	^channel@[0-3]+$

and I guess it should instead be

	^channel@[0-2]$
                    ^

in order to fully match the real indexing constraints you're enforcing
with minimum/maximum.

Thanks,
Miquèl
Conor Dooley Aug. 21, 2024, 3:29 p.m. UTC | #6
On Wed, Aug 21, 2024 at 09:41:50AM +0200, Miquel Raynal wrote:
> > > > > +      Represents the channels of the ADC.
> > > > > +
> > > > > +    properties:
> > > > > +      reg:
> > > > > +        description: |
> > > > > +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> > > > > +        items:
> > > > > +          - minimum: 0
> > > > > +            maximum: 2  
> > > > 
> > > > Is this sufficient to limit the number of channels to 3? Aren't you relying
> > > > on the unique unit addresses warning in dtc to limit it, rather than
> > > > actually limiting with min/maxItems?
> > > >   
> > > It seems like I can't use min/maxItems on this property. I think that it is
> > > using size-cells + address-cells to deduce that the number of items should
> > > be equal to 1.  
> 
> Looking at dt-schema, I couldn't personally understand from where did
> the error messages reported by Thomas came from. There are clear

I think the complaints are on a more meta level than that. He provided
an items list
     properties:
       reg:
         maxItems: 1
         items:
           - minimum: 0
             maximum: 2
but this list only has one entry as there's one -. The first complaint
from dt_binding_check is that having maxItems is not needed with an
items list, because the items list contains the maximum number of
elements.

The second one comes from cell.yaml:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/meta-schemas/cell.yaml

It either allows a single item, with maxItems: 1 or multiple items, in
which case maxitems must be greater than 1. That's where the "anyOf
conditonal failed" and "1 is less than the minimum of 2" stuff comes
from.

I hope that helps?

> constraints over minItems/maxItems regarding the use of {#address-cells,
> #sizez-cells} being {1, 1}, {2, 2} and {2, 1} (in reg.yaml), but nothing
> explicit regarding the other situations, namely {1, 0} in this case
> which enforces maxItems to 1 is not clearly stated in any of the core
> yaml files. Any idea where to look at? Although, I'm convinced there is
> something defined because renaming the property from 'reg' to 'foo'
> silences these warnings.
> 
> > I think I was mistaken in talking about mix/max items here. I had the
> > right idea, but mentioned an incorrect solution - sorry about that. I
> > wasn't talking about the number of elements in the reg property, what I
> > meant was limiting the number of channel nodes in the first place -
> > something which min/maxItems cannot do. As examples of the problem I was
> > thinking of, see the below two examples:
> > 
> >     adc@30f0000 {
> >         compatible = "sophgo,cv1800b-saradc";
> >         reg = <0x030f0000 0x1000>;
> >         clocks = <&clk CLK_SARADC>;
> >         interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> > 
> >         channel@0 {
> >             reg = <0>;
> >         };
> >         channel@2 {
> >             reg = <2>;
> >         };
> >         channel@22 {
> >             reg = <2>;
> >         };
> >     };
> > 
> >     adc@30f0000 {
> >         compatible = "sophgo,cv1800b-saradc";
> >         reg = <0x030f0000 0x1000>;
> >         clocks = <&clk CLK_SARADC>;
> >         interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> > 
> >         channel@0 {
> >             reg = <0>;
> >         };
> >         channel@2 {
> >             reg = <2>;
> >         };
> >         channel@22 {
> >             reg = <2>;
> >         };

I noticed that I pasted two of the same example. I must have just
yoinked the latter to a vim buffer rather than to my clipboard. At least
it didn't matter in the end.

Cheers,
Conor.

> >     };
> > 
> > The solution is simple, remove the + from the regex. Sorry for sending
> > you on the wrong track Thomas.
> 
> Ah! Thanks Conor for the details, now it makes full sense :-) BTW Thomas
> the regex is
> 
> 	^channel@[0-3]+$
> 
> and I guess it should instead be
> 
> 	^channel@[0-2]$
>                     ^
> 
> in order to fully match the real indexing constraints you're enforcing
> with minimum/maximum.
> 
> Thanks,
> Miquèl
Miquel Raynal Aug. 22, 2024, 8:52 a.m. UTC | #7
Hi Conor,

conor@kernel.org wrote on Wed, 21 Aug 2024 16:29:42 +0100:

> On Wed, Aug 21, 2024 at 09:41:50AM +0200, Miquel Raynal wrote:
> > > > > > +      Represents the channels of the ADC.
> > > > > > +
> > > > > > +    properties:
> > > > > > +      reg:
> > > > > > +        description: |
> > > > > > +          The channel number. It can have up to 3 channels numbered from 0 to 2.
> > > > > > +        items:
> > > > > > +          - minimum: 0
> > > > > > +            maximum: 2    
> > > > > 
> > > > > Is this sufficient to limit the number of channels to 3? Aren't you relying
> > > > > on the unique unit addresses warning in dtc to limit it, rather than
> > > > > actually limiting with min/maxItems?
> > > > >     
> > > > It seems like I can't use min/maxItems on this property. I think that it is
> > > > using size-cells + address-cells to deduce that the number of items should
> > > > be equal to 1.    
> > 
> > Looking at dt-schema, I couldn't personally understand from where did
> > the error messages reported by Thomas came from. There are clear  
> 
> I think the complaints are on a more meta level than that. He provided
> an items list
>      properties:
>        reg:
>          maxItems: 1
>          items:
>            - minimum: 0
>              maximum: 2
> but this list only has one entry as there's one -. The first complaint
> from dt_binding_check is that having maxItems is not needed with an
> items list, because the items list contains the maximum number of
> elements.
> 
> The second one comes from cell.yaml:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/meta-schemas/cell.yaml
> 
> It either allows a single item, with maxItems: 1 or multiple items, in
> which case maxitems must be greater than 1. That's where the "anyOf
> conditonal failed" and "1 is less than the minimum of 2" stuff comes
> from.
> 
> I hope that helps?

Ah yeah, makes sense. Thanks a lot for your feedback!

Miquèl
diff mbox series

Patch

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..846590808e5f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/sophgo,cv18xx-saradc.yaml
@@ -0,0 +1,85 @@ 
+# 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:
+    const: sophgo,cv1800b-saradc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^channel@[0-3]+$":
+    $ref: adc.yaml
+
+    description: |
+      Represents the channels of the ADC.
+
+    properties:
+      reg:
+        description: |
+          The channel number. It can have up to 3 channels numbered from 0 to 2.
+        items:
+          - minimum: 0
+            maximum: 2
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sophgo,cv1800.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    adc@30f0000 {
+        compatible = "sophgo,cv1800b-saradc";
+        reg = <0x030f0000 0x1000>;
+        clocks = <&clk CLK_SARADC>;
+        interrupts = <100 IRQ_TYPE_LEVEL_HIGH>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+            reg = <0>;
+        };
+        channel@1 {
+            reg = <1>;
+        };
+        channel@2 {
+            reg = <2>;
+        };
+    };