Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 6 Nov 2024 17:22:35 +0800
Yu-Hsian Yang <j2anfernee@gmail.com> wrote:

> Dear Chanh Nguyen,
> 
> Thank you for your response.
> 
> Chanh Nguyen <chanh@amperemail.onmicrosoft.com> 於 2024年11月6日 週三 下午12:58寫道：
> >
> >
> >
> > On 06/11/2024 09:39, Eason Yang wrote:  
> > > This adds a binding specification for the Nuvoton NCT7201/NCT7202
> > > family of ADCs.
> > >
> > > Signed-off-by: Eason Yang <j2anfernee@gmail.com>
> > > ---
> > >   .../bindings/iio/adc/nuvoton,nct720x.yaml     | 47 +++++++++++++++++++
> > >   MAINTAINERS                                   |  1 +
> > >   2 files changed, 48 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml
> > > new file mode 100644
> > > index 000000000000..3052039af10e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml
> > > @@ -0,0 +1,47 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/adc/nuvoton,nct720x.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Nuvoton nct7202 and similar ADCs
> > > +
> > > +maintainers:
> > > +  - Eason Yang <yhyang2@nuvoton.com>
> > > +
> > > +description: |
> > > +   Family of ADCs with i2c interface.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - nuvoton,nct7201
> > > +      - nuvoton,nct7202
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  read-vin-data-size:  
> >
> > Is it generic property or vendor property? I tried to find in the
> > https://github.com/torvalds/linux/tree/master/Documentation/devicetree/bindings
> > , but it seems this property hasn't been used on other devices.
> >
> > If it is vendor property, then I think it should include a vendor
> > prefix. For examples:
> >
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/adi%2Cad7780.yaml#L50
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/fsl%2Cvf610-adc.yaml#L42
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/st%2Cstmpe-adc.yaml#L22
> >
> >  
> 
> I would add a vendor prefix for it.

Why do we want this at all?  Is this device sufficiently high
performance that Linux will ever want to trade of resolution against
sampling speed?

If so that seems like a policy control that belongs in userspace. Note
that to support that in IIO I would want a strong justification for why we dno't
just set it to 16 always. We just go for maximum resolution in the vast majority
of drivers that support control of this.


> 
> > > +    description: number of data bits per read vin
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [8, 16]
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - read-vin-data-size
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        nct7202@1d {  
> >
> > I think the Node name should follow
> > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> >
> >
> > For some examples that were merged before
> >
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/adi%2Cad7091r5.yaml#L102
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/maxim%2Cmax1238.yaml#L73
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/iio/adc/ti%2Cadc081c.yaml#L49
> >  
> 
> I would change it for the node naming.
> 
> > > +            compatible = "nuvoton,nct7202";
> > > +            reg = <0x1d>;
> > > +            read-vin-data-size = <8>;
> > > +        };
> > > +    };
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 91d0609db61b..68570c58e7aa 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -2746,6 +2746,7 @@ L:      openbmc@lists.ozlabs.org (moderated for non-subscribers)
> > >   S:  Supported
> > >   F:  Documentation/devicetree/bindings/*/*/*npcm*
> > >   F:  Documentation/devicetree/bindings/*/*npcm*
> > > +F:   Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml
> > >   F:  Documentation/devicetree/bindings/rtc/nuvoton,nct3018y.yaml
> > >   F:  arch/arm/boot/dts/nuvoton/nuvoton-npcm*
> > >   F:  arch/arm/mach-npcm/  
> >