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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240803113959.12aa0c4a@jic23-huawei>
Date: Sat, 3 Aug 2024 11:39:59 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Thomas Bonnefille <thomas.bonnefille@...tlin.com>, Lars-Peter Clausen
 <lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Chen Wang
 <unicorn_wang@...look.com>, Inochi Amaoto <inochiama@...look.com>, Paul
 Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>,
 Albert Ou <aou@...s.berkeley.edu>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, Miquèl Raynal
 <miquel.raynal@...tlin.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v3 1/3] dt-bindings: iio: adc:
 sophgo,cv18xx-saradc.yaml: Add Sophgo SARADC binding documentation

On Wed, 31 Jul 2024 15:57:27 +0200
Krzysztof Kozlowski <krzk@...nel.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@...tlin.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@...tlin.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@...0000 {
> > +        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
>