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

[Jonathan Cameron]

On Wed, 31 Jul 2024 15:57:27 +0200
Krzysztof Kozlowski <krzk at 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 at 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 at 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 at 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
>