diff mbox series

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

Message ID 20241106023916.440767-2-j2anfernee@gmail.com
State New
Headers show
Series iio: adc: add Nuvoton NCT720x ADC driver | expand

Commit Message

Yu-Hsian Yang Nov. 6, 2024, 2:39 a.m. UTC
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

Comments

Chanh Nguyen Nov. 6, 2024, 4:58 a.m. UTC | #1
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


> +    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

> +            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/
Yu-Hsian Yang Nov. 6, 2024, 8:48 a.m. UTC | #2
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.

>
>
> > +    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/
>
Yu-Hsian Yang Nov. 6, 2024, 9:22 a.m. UTC | #3
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.

> > +    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/
>
Conor Dooley Nov. 6, 2024, 4:13 p.m. UTC | #4
On Wed, Nov 06, 2024 at 11:58:06AM +0700, Chanh Nguyen wrote:
> 
> 
> 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

An explanation of why it cannot be determined from the compatible would
also be good. Two compatibles and two values makes me a little
suspicious!
Yu-Hsian Yang Nov. 7, 2024, 12:59 a.m. UTC | #5
Dear Conor Dooley,

Thank you for your response.

NCT7201 supports 8 voltage monitor inputs while NCT7202 supports 12
voltage monitor inputs.
NCT7201 and NCT7202 provide SMBus to access the internal register, and
support SMBus byte write and byte/word read protocols.

Conor Dooley <conor@kernel.org> 於 2024年11月7日 週四 上午12:13寫道:
>
> On Wed, Nov 06, 2024 at 11:58:06AM +0700, Chanh Nguyen wrote:
> >
> >
> > 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
>
> An explanation of why it cannot be determined from the compatible would
> also be good. Two compatibles and two values makes me a little
> suspicious!
Jonathan Cameron Nov. 9, 2024, 1:42 p.m. UTC | #6
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/  
> >
Jonathan Cameron Nov. 9, 2024, 1:44 p.m. UTC | #7
On Wed,  6 Nov 2024 10:39:15 +0800
Eason Yang <j2anfernee@gmail.com> wrote:

> This adds a binding specification for the Nuvoton NCT7201/NCT7202
> family of ADCs.
> 
> Signed-off-by: Eason Yang <j2anfernee@gmail.com>
Hi Eason,

> ---
>  .../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

Already raised in another review I think, but pick a part number and name after that.  Wild
card get broken far too often for them to be usable like this.


> @@ -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:
> +    description: number of data bits per read vin
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [8, 16]
> +
> +required:
> +  - compatible
> +  - reg
> +  - read-vin-data-size

If you do keep this, then pick a sensible default (16)
and drop the required.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        nct7202@1d {
> +            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/
Jonathan Cameron Nov. 9, 2024, 2:29 p.m. UTC | #8
On Sat, 9 Nov 2024 13:42:28 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> 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.
I'd misunderstood what this is. It's a control no what the i2c word size is.
Do we actually care about supporting rubbish i2c controllers?  How many
can't do a word access?

If you do it should be detected from the controller rather than in DT.

> 
> 
> >   
> > > > +    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/    
> > >    
> 
>
Yu-Hsian Yang Nov. 11, 2024, 7:45 a.m. UTC | #9
Dear Jonathan Cameron,

For property read-vin-data-size, we have a internal discussion.

For Nuvoton NCT7201/NCT7202 chip,
Take an example as to Vin1:
The VIN reading supports Byte read (One Byte) and Word read (Two Byte)

For Byte read:
First read Index 00h to get VIN1 MSB, then read Index 0Fh Bit 3~7 to
get VIN1 LSB.
Index 0Fh is a shared LSB for all VINs.

For Word read:
Read Index 00h and get 2 Byte (VIN1 MSB and VIN1 LSB).

We would refer your suggestion,
we  declare a property named "nvuoton,read-vin-data-size" with default value 16
for user to use.

Jonathan Cameron <jic23@kernel.org> 於 2024年11月9日 週六 下午10:29寫道:
>
> On Sat, 9 Nov 2024 13:42:28 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > 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.
> I'd misunderstood what this is. It's a control no what the i2c word size is.
> Do we actually care about supporting rubbish i2c controllers?  How many
> can't do a word access?
>
> If you do it should be detected from the controller rather than in DT.
>
> >
> >
> > >
> > > > > +    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/
> > > >
> >
> >
>
Jonathan Cameron Nov. 23, 2024, 2:47 p.m. UTC | #10
On Mon, 11 Nov 2024 15:45:03 +0800
Yu-Hsian Yang <j2anfernee@gmail.com> wrote:

> Dear Jonathan Cameron,
> 
> For property read-vin-data-size, we have a internal discussion.
> 
> For Nuvoton NCT7201/NCT7202 chip,
> Take an example as to Vin1:
> The VIN reading supports Byte read (One Byte) and Word read (Two Byte)
> 
> For Byte read:
> First read Index 00h to get VIN1 MSB, then read Index 0Fh Bit 3~7 to
> get VIN1 LSB.
> Index 0Fh is a shared LSB for all VINs.
> 
> For Word read:
> Read Index 00h and get 2 Byte (VIN1 MSB and VIN1 LSB).
> 
> We would refer your suggestion,
> we  declare a property named "nvuoton,read-vin-data-size" with default value 16
> for user to use.

Thanks for the info.  If the i2c controller allows word read
then the right thing is to always use it.

Just check for I2C_FUNC_SMBUS_READ_WORD_DATA with
i2c_check_functionality()

If it's supported use i2c_smbus_read_word_swapped()
if not, do the i2c_smbus_read_byte() approach.

We don't need to want this in DT as it is a property of the smbus
controller, not this device.

Jonathan




> 
> Jonathan Cameron <jic23@kernel.org> 於 2024年11月9日 週六 下午10:29寫道:
> >
> > On Sat, 9 Nov 2024 13:42:28 +0000
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> > > 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.  
> > I'd misunderstood what this is. It's a control no what the i2c word size is.
> > Do we actually care about supporting rubbish i2c controllers?  How many
> > can't do a word access?
> >
> > If you do it should be detected from the controller rather than in DT.
> >  
> > >
> > >  
> > > >  
> > > > > > +    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/  
> > > > >  
> > >
> > >  
> >
Yu-Hsian Yang Nov. 28, 2024, 2:14 a.m. UTC | #11
Dear Jonathan Cameron,

Thank you for your advice.

I would remove the "nvuoton,read-vin-data-size" property.

Read VIN info can use word read or byte read, and other registers
should use byte read.
If I use word read for VIN info and byte read for other registers,
I encounter an issue when I use regmap instead of i2c smbus API.

I need two regmap configs with val_bits 8/16.
After I call devm_regmap_init_i2c these two configs,
the error message:
"debugfs: Directory '5-001d' with parent 'regmap' already present!"

Do you have any suggestions?

Jonathan Cameron <jic23@kernel.org> 於 2024年11月23日 週六 下午10:48寫道:
>
> On Mon, 11 Nov 2024 15:45:03 +0800
> Yu-Hsian Yang <j2anfernee@gmail.com> wrote:
>
> > Dear Jonathan Cameron,
> >
> > For property read-vin-data-size, we have a internal discussion.
> >
> > For Nuvoton NCT7201/NCT7202 chip,
> > Take an example as to Vin1:
> > The VIN reading supports Byte read (One Byte) and Word read (Two Byte)
> >
> > For Byte read:
> > First read Index 00h to get VIN1 MSB, then read Index 0Fh Bit 3~7 to
> > get VIN1 LSB.
> > Index 0Fh is a shared LSB for all VINs.
> >
> > For Word read:
> > Read Index 00h and get 2 Byte (VIN1 MSB and VIN1 LSB).
> >
> > We would refer your suggestion,
> > we  declare a property named "nvuoton,read-vin-data-size" with default value 16
> > for user to use.
>
> Thanks for the info.  If the i2c controller allows word read
> then the right thing is to always use it.
>
> Just check for I2C_FUNC_SMBUS_READ_WORD_DATA with
> i2c_check_functionality()
>
> If it's supported use i2c_smbus_read_word_swapped()
> if not, do the i2c_smbus_read_byte() approach.
>
> We don't need to want this in DT as it is a property of the smbus
> controller, not this device.
>
> Jonathan
>
>
>
>
> >
> > Jonathan Cameron <jic23@kernel.org> 於 2024年11月9日 週六 下午10:29寫道:
> > >
> > > On Sat, 9 Nov 2024 13:42:28 +0000
> > > Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > > 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.
> > > I'd misunderstood what this is. It's a control no what the i2c word size is.
> > > Do we actually care about supporting rubbish i2c controllers?  How many
> > > can't do a word access?
> > >
> > > If you do it should be detected from the controller rather than in DT.
> > >
> > > >
> > > >
> > > > >
> > > > > > > +    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/
> > > > > >
> > > >
> > > >
> > >
>
David Lechner Nov. 29, 2024, 2:50 p.m. UTC | #12
On 11/27/24 8:14 PM, Yu-Hsian Yang wrote:
> Dear Jonathan Cameron,
> 
> Thank you for your advice.
> 
> I would remove the "nvuoton,read-vin-data-size" property.
> 
> Read VIN info can use word read or byte read, and other registers
> should use byte read.
> If I use word read for VIN info and byte read for other registers,
> I encounter an issue when I use regmap instead of i2c smbus API.
> 
> I need two regmap configs with val_bits 8/16.
> After I call devm_regmap_init_i2c these two configs,
> the error message:
> "debugfs: Directory '5-001d' with parent 'regmap' already present!"
> 
> Do you have any suggestions?
> 
Give each regmap a unique name, like "5-001d-8bit" and "5-001d-16bit".
Yu-Hsian Yang Dec. 2, 2024, 3:57 a.m. UTC | #13
Dear David Lechner,

Thank you your comment.

David Lechner <dlechner@baylibre.com> 於 2024年11月29日 週五 下午10:50寫道:
>
> On 11/27/24 8:14 PM, Yu-Hsian Yang wrote:
> > Dear Jonathan Cameron,
> >
> > Thank you for your advice.
> >
> > I would remove the "nvuoton,read-vin-data-size" property.
> >
> > Read VIN info can use word read or byte read, and other registers
> > should use byte read.
> > If I use word read for VIN info and byte read for other registers,
> > I encounter an issue when I use regmap instead of i2c smbus API.
> >
> > I need two regmap configs with val_bits 8/16.
> > After I call devm_regmap_init_i2c these two configs,
> > the error message:
> > "debugfs: Directory '5-001d' with parent 'regmap' already present!"
> >
> > Do you have any suggestions?
> >
> Give each regmap a unique name, like "5-001d-8bit" and "5-001d-16bit".

It worked fine to add each regmap a unique name.
diff mbox series

Patch

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:
+    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 {
+            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/