iio: adc: add Nuvoton NCT720x ADC driver
Subsystems (2)
cover iio: adc: add Nuvoton NCT720x ADC driver
Changes since version 1:
- Add new property in iio:adc binding document.
- Add new driver for Nuvoton NCT720x driver.
Eason Yang (2):
dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
iio: adc: add Nuvoton NCT720x ADC driver
.../bindings/iio/adc/nuvoton,nct720x.yaml | 47 ++ MAINTAINERS | 2 + drivers/iio/adc/Kconfig | 9 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/nct720x.c | 617 ++++++++++++++++++ 5 files changed, 676 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml create mode 100644 drivers/iio/adc/nct720x.c
Signature · 1 line
2.34.1
PATCH 1/2 dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
--- .../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
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +47 -0
@@ -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 <[email protected]> + +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>; + }; + };
MAINTAINERS +1 -0
@@ -2746,6 +2746,7 @@ L: [email protected] (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/
Signature · 1 line
2.34.1
Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
Quoted 30 lines · Eason Yang
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +14
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. +
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+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
Quoted 8 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +8
-0+ description: number of data bits per read vin + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [8, 16] + +required: + - compatible + - reg + - read-vin-data-size
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 42
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+ +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
Quoted 17 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +5
-0+ compatible = "nuvoton,nct7202"; + reg = <0x1d>; + read-vin-data-size = <8>; + }; + };MAINTAINERS +1
-0@@ -2746,6 +2746,7 @@ L: [email protected] (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/
Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
Dear Chanh Nguyen,
Thank you for your response.
Chanh Nguyen <[email protected]> 於 2024年11月6日 週三 下午12:58寫道:
Quoted 49 lines · Eason Yang
Quoted 30 lines
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +14
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. +Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+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.
Quoted 8 lines
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.
Quoted 23 lines
Quoted 8 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +8
-0+ description: number of data bits per read vin + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [8, 16] + +required: + - compatible + - reg + - read-vin-data-sizeDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 42
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+ +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
Quoted 6 lines
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.
Quoted 18 lines
Quoted 17 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +5
-0+ compatible = "nuvoton,nct7202"; + reg = <0x1d>; + read-vin-data-size = <8>; + }; + };MAINTAINERS +1
-0@@ -2746,6 +2746,7 @@ L: [email protected] (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
maintainerRe: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
Quoted 57 lines · On Wed, 6 Nov 2024 17:22:35 +0800 Yu-Hsian Yang <[email protected]> wrote:
Dear Chanh Nguyen,
Thank you for your response.
Chanh Nguyen <[email protected]> 於 2024年11月6日 週三 下午12:58寫道:
Quoted 52 lines · Eason Yang
Quoted 30 lines
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +14
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. +Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+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:
Quoted 7 lines
Quoted 5 lines
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#L22I 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.
Quoted 51 lines
Quoted 24 lines
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +8
-0+ description: number of data bits per read vin + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [8, 16] + +required: + - compatible + - reg + - read-vin-data-sizeDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 42
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+ +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-recommendationQuoted 6 lines
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#L49I would change it for the node naming.
Quoted 18 lines
Quoted 17 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +5
-0+ compatible = "nuvoton,nct7202"; + reg = <0x1d>; + read-vin-data-size = <8>; + }; + };MAINTAINERS +1
-0@@ -2746,6 +2746,7 @@ L: [email protected] (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
maintainerRe: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
Quoted 68 lines · On Sat, 9 Nov 2024 13:42:28 +0000 Jonathan Cameron <[email protected]> wrote:
Quoted 57 lines · On Wed, 6 Nov 2024 17:22:35 +0800 Yu-Hsian Yang <[email protected]> wrote:
Dear Chanh Nguyen,
Thank you for your response.
Chanh Nguyen <[email protected]> 於 2024年11月6日 週三 下午12:58寫道:
Quoted 52 lines · Eason Yang
Quoted 30 lines
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +14
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. +Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+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:
Quoted 7 lines
Quoted 5 lines
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#L22I would add a vendor prefix for it.
Quoted 8 lines
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.
Quoted 55 lines
Quoted 53 lines
Quoted 26 lines
Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +8
-0+ description: number of data bits per read vin + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [8, 16] + +required: + - compatible + - reg + - read-vin-data-sizeDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 42
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+ +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-recommendationQuoted 6 lines
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#L49I would change it for the node naming.
Quoted 18 lines
Quoted 17 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +5
-0+ compatible = "nuvoton,nct7202"; + reg = <0x1d>; + read-vin-data-size = <8>; + }; + };MAINTAINERS +1
-0@@ -2746,6 +2746,7 @@ L: [email protected] (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/
Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
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 <[email protected]> 於 2024年11月9日 週六 下午10:29寫道:
Quoted 142 lines
Quoted 68 lines · On Sat, 9 Nov 2024 13:42:28 +0000 Jonathan Cameron <[email protected]> wrote:
Quoted 57 lines · On Wed, 6 Nov 2024 17:22:35 +0800 Yu-Hsian Yang <[email protected]> wrote:
Dear Chanh Nguyen,
Thank you for your response.
Chanh Nguyen <[email protected]> 於 2024年11月6日 週三 下午12:58寫道:
Quoted 52 lines · Eason Yang
Quoted 30 lines
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +14
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. +Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+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:
Quoted 7 lines
Quoted 5 lines
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#L22I would add a vendor prefix for it.
Quoted 8 lines
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.
Quoted 55 lines
Quoted 53 lines
Quoted 26 lines
Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +8
-0+ description: number of data bits per read vin + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [8, 16] + +required: + - compatible + - reg + - read-vin-data-sizeDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 42
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+ +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-recommendationQuoted 6 lines
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#L49I would change it for the node naming.
Quoted 18 lines
Quoted 17 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +5
-0+ compatible = "nuvoton,nct7202"; + reg = <0x1d>; + read-vin-data-size = <8>; + }; + };MAINTAINERS +1
-0@@ -2746,6 +2746,7 @@ L: [email protected] (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
maintainerRe: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
Quoted 13 lines · On Mon, 11 Nov 2024 15:45:03 +0800 Yu-Hsian Yang <[email protected]> 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.
Quoted 6 lines
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 withi2c_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
Quoted 144 lines
Jonathan Cameron <[email protected]> 於 2024年11月9日 週六 下午10:29寫道:
Quoted 142 lines
Quoted 68 lines · On Sat, 9 Nov 2024 13:42:28 +0000 Jonathan Cameron <[email protected]> wrote:
Quoted 57 lines · On Wed, 6 Nov 2024 17:22:35 +0800 Yu-Hsian Yang <[email protected]> wrote:
Dear Chanh Nguyen,
Thank you for your response.
Chanh Nguyen <[email protected]> 於 2024年11月6日 週三 下午12:58寫道:
Quoted 52 lines · Eason Yang
Quoted 30 lines
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +14
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. +Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+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:
Quoted 7 lines
Quoted 5 lines
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#L22I would add a vendor prefix for it.
Quoted 8 lines
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.
Quoted 55 lines
Quoted 53 lines
Quoted 26 lines
Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +8
-0+ description: number of data bits per read vin + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [8, 16] + +required: + - compatible + - reg + - read-vin-data-sizeDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 42
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+ +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-recommendationQuoted 6 lines
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#L49I would change it for the node naming.
Quoted 18 lines
Quoted 17 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +5
-0+ compatible = "nuvoton,nct7202"; + reg = <0x1d>; + read-vin-data-size = <8>; + }; + };MAINTAINERS +1
-0@@ -2746,6 +2746,7 @@ L: [email protected] (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/
Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
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 <[email protected]> 於 2024年11月23日 週六 下午10:48寫道:
Quoted 186 lines
Quoted 13 lines · On Mon, 11 Nov 2024 15:45:03 +0800 Yu-Hsian Yang <[email protected]> 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.Quoted 6 lines
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_DATAwithi2c_check_functionality()If it's supported use
i2c_smbus_read_word_swapped()if not, do thei2c_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
Quoted 144 lines
Jonathan Cameron <[email protected]> 於 2024年11月9日 週六 下午10:29寫道:
Quoted 142 lines
Quoted 68 lines · On Sat, 9 Nov 2024 13:42:28 +0000 Jonathan Cameron <[email protected]> wrote:
Quoted 57 lines · On Wed, 6 Nov 2024 17:22:35 +0800 Yu-Hsian Yang <[email protected]> wrote:
Dear Chanh Nguyen,
Thank you for your response.
Chanh Nguyen <[email protected]> 於 2024年11月6日 週三 下午12:58寫道:
Quoted 52 lines · Eason Yang
Quoted 30 lines
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +14
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. +Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+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:
Quoted 7 lines
Quoted 5 lines
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#L22I would add a vendor prefix for it.
Quoted 8 lines
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.
Quoted 55 lines
Quoted 53 lines
Quoted 26 lines
Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +8
-0+ description: number of data bits per read vin + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [8, 16] + +required: + - compatible + - reg + - read-vin-data-sizeDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 42
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+ +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-recommendationQuoted 6 lines
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#L49I would change it for the node naming.
Quoted 18 lines
Quoted 17 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +5
-0+ compatible = "nuvoton,nct7202"; + reg = <0x1d>; + read-vin-data-size = <8>; + }; + };MAINTAINERS +1
-0@@ -2746,6 +2746,7 @@ L: [email protected] (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
reviewerRe: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
Quoted 11 lines · Yu-Hsian Yang
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.
Quoted 7 lines
I need two regmap configs with
val_bits8/16.
After I calldevm_regmap_init_i2cthese 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".
Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
Dear David Lechner,
Thank you your comment.
David Lechner <[email protected]> 於 2024年11月29日 週五 下午10:50寫道:
Quoted 13 lines · Yu-Hsian Yang
Quoted 11 lines
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.
Quoted 8 lines
Quoted 7 lines
I need two regmap configs with
val_bits8/16.
After I calldevm_regmap_init_i2cthese 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.
Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
In reply to Chanh NguyenQuoted 48 lines · Chanh Nguyen
Quoted 30 lines · Eason Yang
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +14
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. +Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+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.
Quoted 6 lines
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!
Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
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 <[email protected]> 於 2024年11月7日 週四 上午12:13寫道:
Quoted 60 lines · Chanh Nguyen
Quoted 48 lines · Eason Yang
Quoted 30 lines
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+)create mode 100644
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +14
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. +Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+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.Quoted 6 lines
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#L22An explanation of why it cannot be determined from the compatible would also be good. Two compatibles and two values makes me a little suspicious!
Re: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
In reply to Chanh NguyenDear Chanh Nguyen,
Thank you for your response.
Chanh Nguyen <[email protected]> 於 2024年11月6日 週三 下午12:58寫道:
Quoted 13 lines · Eason Yang
Quoted 9 lines
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>--- .../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
Quoted 2 lines
Quoted 2 lines
diff --git
a/Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml b/Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml
Quoted 23 lines
Quoted 23 lines
new file mode 100644
index 000000000000..3052039af10eDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +18
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. + +properties: + compatible: + enum: + - nuvoton,nct7201
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 24
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +6
-0+ - 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
Quoted 6 lines
, 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:
Quoted 1 line
Quoted 1 line
I would add a vendor prefix for it.
Quoted 14 lines
Quoted 14 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +12
-0+ 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:
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 42
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +6
-0+ - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + nct7202@1d {I think the Node name should follow
Quoted 5 lines
For some examples that were merged before
Quoted 1 line
Quoted 1 line
Quoted 1 line
I would change it for the node naming.
Quoted 10 lines
MAINTAINERS L: [email protected] (moderated for
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +5
-0+ compatible = "nuvoton,nct7202"; + reg = <0x1d>; + read-vin-data-size = <8>; + }; + };MAINTAINERS +0
-0@@ -2746,6 +2746,7 @@ L: [email protected] (moderated for
non-subscribers)
Quoted 8 lines
Quoted 7 lines
MAINTAINERS +1
-0S: 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
maintainerRe: [PATCH v1 1/2] dt-bindings: iio: adc: Add binding for Nuvoton NCT720x ADCs
In reply to Eason YangQuoted 4 lines · On Wed, 6 Nov 2024 10:39:15 +0800 Eason Yang <[email protected]> wrote:
This adds a binding specification for the Nuvoton NCT7201/NCT7202 family of ADCs.
Signed-off-by Eason Yang <[email protected]>
Hi Eason,
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml
Quoted 11 lines
--- .../bindings/iio/adc/nuvoton,nct720x.yaml | 47 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yamlDocumentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +0
-0
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.
Quoted 23 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +22
-0@@ -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 <[email protected]> + +description: | + Family of ADCs with i2c interface. + +properties: + compatible: + enum: + - nuvoton,nct7201 + - nuvoton,nct7202 + + reg: + maxItems: 1
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml line 32
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +10
-0+ + 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.
Quoted 27 lines
Documentation/devicetree/bindings/iio/adc/nuvoton,nct720x.yaml new file +15
-0+ +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + nct7202@1d { + compatible = "nuvoton,nct7202"; + reg = <0x1d>; + read-vin-data-size = <8>; + }; + };MAINTAINERS +1
-0@@ -2746,6 +2746,7 @@ L: [email protected] (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/
PATCH 2/2 iio: adc: add Nuvoton NCT720x ADC driver
In reply to Eason YangAdd Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
--- MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 9 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/nct720x.c | 617 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 628 insertions(+) create mode 100644 drivers/iio/adc/nct720x.c
MAINTAINERS +1 -0
@@ -2753,6 +2753,7 @@ F: arch/arm/mach-npcm/ F: arch/arm64/boot/dts/nuvoton/ F: drivers/*/*/*npcm* F: drivers/*/*npcm* +F: drivers/iio/adc/nct720x.c F: drivers/rtc/rtc-nct3018y.c F: include/dt-bindings/clock/nuvoton,npcm7xx-clock.h F: include/dt-bindings/clock/nuvoton,npcm845-clk.h
drivers/iio/adc/Kconfig +9 -0
@@ -1008,6 +1008,15 @@ config NAU7802 To compile this driver as a module, choose M here: the module will be called nau7802. +config NCT720X + tristate "Nuvoton Instruments NCT7201 and NCT7202 Power Monitor" + depends on I2C + help + If you say yes here you get support for the Nuvoton NCT7201 and + NCT7202 Voltage Monitor. + This driver can also be built as a module. If so, the module + will be called nct720x. + config NPCM_ADC tristate "Nuvoton NPCM ADC driver" depends on ARCH_NPCM || COMPILE_TEST
drivers/iio/adc/Makefile +1 -0
@@ -91,6 +91,7 @@ obj-$(CONFIG_MESON_SARADC) += meson_saradc.o obj-$(CONFIG_MP2629_ADC) += mp2629_adc.o obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o obj-$(CONFIG_NAU7802) += nau7802.o +obj-$(CONFIG_NCT720X) += nct720x.o obj-$(CONFIG_NPCM_ADC) += npcm_adc.o obj-$(CONFIG_PAC1921) += pac1921.o obj-$(CONFIG_PAC1934) += pac1934.o
drivers/iio/adc/nct720x.c new file +617 -0
617 lines not on this page · raw message
Signature · 1 line
2.34.1
kernel test robot
build botkernel test robot noticed the following build warnings:
Hi Eason,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.12-rc6 next-20241106] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Eason-Yang/dt-bindings-iio-adc-Add-binding-for-Nuvoton-NCT720x-ADCs/20241106-104046 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/20241106023916.440767-3-j2anfernee%40gmail.com patch subject: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241106/[email protected]/config) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241106/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from drivers/iio/adc/nct720x.c:9:
In file included from include/linux/device.h:32:
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:181:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/adc/nct720x.c:526:16: warning: cast to smaller integer type 'enum nct720x_chips' from 'const void *' [-Wvoid-pointer-to-enum-cast]
526 | chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~5 warnings generated.
vim +526 drivers/iio/adc/nct720x.c
511
512 static int nct720x_probe(struct i2c_client *client)
513 {
514 const struct i2c_device_id *id = i2c_client_get_device_id(client);
515 struct nct720x_chip_info *chip;
516 struct iio_dev *indio_dev;
517 int ret;
518 u32 tmp;
519
520 indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
521 if (!indio_dev)
522 return -ENOMEM;
523 chip = iio_priv(indio_dev);
524
525 if (client->dev.of_node)
> 526 chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
527 else
528 chip->type = i2c_match_id(nct720x_id, client)->driver_data;
529
530 chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
531
532 ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
533 if (ret < 0) {
534 pr_err("read-vin-data-size property not found\n");
535 return ret;
536 }
537
538 if (tmp == 8) {
539 chip->use_read_byte_vin = true;
540 } else if (tmp == 16) {
541 chip->use_read_byte_vin = false;
542 } else {
543 pr_err("invalid read-vin-data-size (%d)\n", tmp);
544 return -EINVAL;
545 }
546
547 mutex_init(&chip->access_lock);
548
549 /* this is only used for device removal purposes */
550 i2c_set_clientdata(client, indio_dev);
551
552 chip->client = client;
553
554 ret = nct720x_init_chip(chip);
555 if (ret < 0)
556 return ret;
557
558 indio_dev->name = id->name;
559 indio_dev->channels = nct720x_channels;
560 indio_dev->num_channels = ARRAY_SIZE(nct720x_channels);
561 indio_dev->info = &nct720x_info;
562 indio_dev->modes = INDIO_DIRECT_MODE;
563
564 iio_device_register(indio_dev);
565
566 return 0;
567 }
568
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Dear kernel test robot,
Thank you for your response.
I would check these warnings.
kernel test robot <[email protected]> 於 2024年11月6日 週三 下午8:32寫道:
Quoted 123 lines
Hi Eason,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.12-rc6 next-20241106] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]url: https://github.com/intel-lab-lkp/linux/commits/Eason-Yang/dt-bindings-iio-adc-Add-binding-for-Nuvoton-NCT720x-ADCs/20241106-104046 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/20241106023916.440767-3-j2anfernee%40gmail.com patch subject: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241106/[email protected]/config) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241106/[email protected]/reproduce)If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/All warnings (new ones prefixed by >>):
In file included from
drivers/iio/adc/nct720x.c:9:
In file included frominclude/linux/device.h:32:
In file included frominclude/linux/device/driver.h:21:
In file included frominclude/linux/module.h:19:
In file included frominclude/linux/elf.h:6:
In file included fromarch/s390/include/asm/elf.h:181:
In file included fromarch/s390/include/asm/mmu_context.h:11:
In file included fromarch/s390/include/asm/pgalloc.h:18:
In file included frominclude/linux/mm.h:2213:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enumzone_stat_item' and 'enumnuma_stat_item') [-Wenum-enum-conversion]504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS + | ~~~~~~~~~~~~~~~~~~~~~ ^ 505 | item]; | ~~~~ include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion] 511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS + | ~~~~~~~~~~~~~~~~~~~~~ ^ 512 | NR_VM_NUMA_EVENT_ITEMS + | ~~~~~~~~~~~~~~~~~~~~~~ include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion] 524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS + | ~~~~~~~~~~~~~~~~~~~~~ ^ 525 | NR_VM_NUMA_EVENT_ITEMS + | ~~~~~~~~~~~~~~~~~~~~~~Quoted 1 line
Quoted 1 line
drivers/iio/adc/nct720x.c:526:16: warning: cast to smaller integer type 'enumnct720x_chips' from 'const void *' [-Wvoid-pointer-to-enum-cast]526 | chip->type = (enum nct720x_chips)device_get_match_data(&client->dev); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~5 warnings generated.
vim +526
drivers/iio/adc/nct720x.c511 512 static int nct720x_probe(struct i2c_client *client) 513 { 514 const struct i2c_device_id *id = i2c_client_get_device_id(client); 515 struct nct720x_chip_info *chip; 516 struct iio_dev *indio_dev; 517 int ret; 518 u32 tmp; 519 520 indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); 521 if (!indio_dev) 522 return -ENOMEM; 523 chip = iio_priv(indio_dev); 524 525 if (client->dev.of_node)Quoted 1 line
526
chip->type= (enum nct720x_chips)device_get_match_data(&client->dev);527 else
528 chip->type = i2c_match_id(nct720x_id, client)->driver_data; 529 530 chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX; 531 532 ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp); 533 if (ret < 0) { 534 pr_err("read-vin-data-size property not found\n"); 535 return ret; 536 } 537 538 if (tmp == 8) { 539 chip->use_read_byte_vin = true; 540 } else if (tmp == 16) { 541 chip->use_read_byte_vin = false; 542 } else { 543 pr_err("invalid read-vin-data-size (%d)\n", tmp); 544 return -EINVAL; 545 } 546 547 mutex_init(&chip->access_lock); 548 549 /* this is only used for device removal purposes */ 550 i2c_set_clientdata(client, indio_dev); 551 552 chip->client = client; 553 554 ret = nct720x_init_chip(chip); 555 if (ret < 0) 556 return ret; 557 558 indio_dev->name = id->name; 559 indio_dev->channels = nct720x_channels; 560 indio_dev->num_channels = ARRAY_SIZE(nct720x_channels); 561 indio_dev->info = &nct720x_info; 562 indio_dev->modes = INDIO_DIRECT_MODE; 563 564 iio_device_register(indio_dev); 565 566 return 0; 567 } 568--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Quoted 11 lines · Eason Yang
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
Signed-off-by Eason Yang <[email protected]>
...
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+ +static int nct720x_probe (struct i2c_client *client) +{ +const struct i2c_device_id *id =i2c_client_get_device_id (client); +struct nct720x_chip_info *chip; +struct iio_dev *indio_dev; +int ret; +u32 tmp;
drivers/iio/adc/nct720x.c line 528
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*chip)); +if (!indio_dev) +return -ENOMEM ; + chip =iio_priv (indio_dev); + +if (client->dev.of_node) + chip->type = (enum nct720x_chips )device_get_match_data (&client->dev); +else + chip->type =i2c_match_id (nct720x_id, client)->driver_data;
I believe there is a I2C wrapper for above.
drivers/iio/adc/nct720x.c line 534
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ + chip->vin_max = (chip->type == nct7201) ?NCT7201_VIN_MAX :NCT7202_VIN_MAX ; + + ret =of_property_read_u32 (client->dev.of_node,"read-vin-data-size" , &tmp); +if (ret <0 ) { +pr_err ("read-vin-data-size property not found\n" );
Please use dev_xxx in your driver code.
Best regards,
Krzysztof
Dear Krzysztof Kozlowski,
Thank you for your response.
Krzysztof Kozlowski <[email protected]> 於 2024年11月6日 週三 下午9:41寫道:
Quoted 28 lines · Eason Yang
Quoted 11 lines
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
Signed-off-by Eason Yang <[email protected]>...
Quoted 12 lines
drivers/iio/adc/nct720x.c new file +12
-0+ +static int nct720x_probe (struct i2c_client *client) +{ +const struct i2c_device_id *id =i2c_client_get_device_id (client); +struct nct720x_chip_info *chip; +struct iio_dev *indio_dev; +int ret; +u32 tmp; + + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*chip)); +if (!indio_dev) +return -ENOMEM ;
Quoted 9 lines
drivers/iio/adc/nct720x.c line 528
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ chip =iio_priv (indio_dev); + +if (client->dev.of_node) + chip->type = (enum nct720x_chips )device_get_match_data (&client->dev); +else + chip->type =i2c_match_id (nct720x_id, client)->driver_data;I believe there is a I2C wrapper for above.
Got it.
Quoted 8 lines
drivers/iio/adc/nct720x.c line 534
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ + chip->vin_max = (chip->type == nct7201) ?NCT7201_VIN_MAX :NCT7202_VIN_MAX ; + + ret =of_property_read_u32 (client->dev.of_node,"read-vin-data-size" , &tmp); +if (ret <0 ) { +pr_err ("read-vin-data-size property not found\n" );Please use
dev_xxxin your driver code.
Got it.
Quoted 5 lines
Best regards,
Krzysztof
Jonathan Cameron
maintainerQuoted 43 lines · On Thu, 7 Nov 2024 08:41:21 +0800 Yu-Hsian Yang <[email protected]> wrote:
Dear Krzysztof Kozlowski,
Thank you for your response.
Krzysztof Kozlowski <[email protected]> 於 2024年11月6日 週三 下午9:41寫道:
Quoted 28 lines · Eason Yang
Quoted 11 lines
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
Signed-off-by Eason Yang <[email protected]>...
Quoted 12 lines
drivers/iio/adc/nct720x.c new file +12
-0+ +static int nct720x_probe (struct i2c_client *client) +{ +const struct i2c_device_id *id =i2c_client_get_device_id (client); +struct nct720x_chip_info *chip; +struct iio_dev *indio_dev; +int ret; +u32 tmp; + + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*chip)); +if (!indio_dev) +return -ENOMEM ;Quoted 9 lines
drivers/iio/adc/nct720x.c line 528
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ chip =iio_priv (indio_dev); + +if (client->dev.of_node) + chip->type = (enum nct720x_chips )device_get_match_data (&client->dev); +else + chip->type =i2c_match_id (nct720x_id, client)->driver_data;I believe there is a I2C wrapper for above.
Quoted 1 line
Got it.
Don't pass an enum value as data. Pass a pointer to a structure that describes the particular variant. The 0 value which tends to end up in enums is an error for device_get_match_data.
Quoted 17 lines
Quoted 9 lines
drivers/iio/adc/nct720x.c line 534
Quoted 7 lines
drivers/iio/adc/nct720x.c new file +6
-0+ + chip->vin_max = (chip->type == nct7201) ?NCT7201_VIN_MAX :NCT7202_VIN_MAX ; + + ret =of_property_read_u32 (client->dev.of_node,"read-vin-data-size" , &tmp); +if (ret <0 ) { +pr_err ("read-vin-data-size property not found\n" );Please use
dev_xxxin your driver code.Got it.
Quoted 5 lines
Best regards,
Krzysztof
Thank you for your comment.
Jonathan Cameron <[email protected]> 於 2024年11月9日 週六 下午9:45寫道:
Quoted 47 lines
Quoted 43 lines · On Thu, 7 Nov 2024 08:41:21 +0800 Yu-Hsian Yang <[email protected]> wrote:
Dear Krzysztof Kozlowski,
Thank you for your response.
Krzysztof Kozlowski <[email protected]> 於 2024年11月6日 週三 下午9:41寫道:
Quoted 37 lines · Eason Yang
Quoted 11 lines
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
Signed-off-by Eason Yang <[email protected]>...
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+ +static int nct720x_probe (struct i2c_client *client) +{ +const struct i2c_device_id *id =i2c_client_get_device_id (client); +struct nct720x_chip_info *chip; +struct iio_dev *indio_dev; +int ret; +u32 tmp;drivers/iio/adc/nct720x.c line 528
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*chip)); +if (!indio_dev) +return -ENOMEM ; + chip =iio_priv (indio_dev); + +if (client->dev.of_node) + chip->type = (enum nct720x_chips )device_get_match_data (&client->dev); +else + chip->type =i2c_match_id (nct720x_id, client)->driver_data;I believe there is a I2C wrapper for above.
Quoted 5 lines
Quoted 1 line
Got it.
Don't pass an enum value as data. Pass a pointer to a structure that describes the particular variant. The 0 value which tends to end up in enums is an error for
device_get_match_data.
I would pass a pointer to the data structure not id to describe the particular variant.
I would rewrite the code as below,
static const struct nct720x_adc_model_data nct7201_model_data = { .model_name = "nct7201",
.channels = nct7201_channels,
.num_channels = ARRAY_SIZE(nct7201_channels),
.vin_max = 8,
};
static const struct nct720x_adc_model_data nct7202_model_data = { .model_name = "nct7202",
.channels = nct7202_channels,
.num_channels = ARRAY_SIZE(nct7202_channels),
.vin_max = 12,
};
static const struct i2c_device_id nct720x_id[] = { { "nct7201", (kernel_ulong_t)&nct7201_model_data }, { "nct7202", (kernel_ulong_t)&nct7202_model_data }, { }
};MODULE_DEVICE_TABLE(i2c, nct720x_id);
static const struct of_device_id nct720x_of_match[] = { {
.compatible = "nuvoton,nct7201",
.data = &nct7201_model_data,
},
{
.compatible = "nuvoton,nct7202",
.data = &nct7202_model_data,
},
{ }
};MODULE_DEVICE_TABLE(of, nct720x_of_match);
Quoted 18 lines
Quoted 17 lines
Quoted 9 lines
drivers/iio/adc/nct720x.c line 534
Quoted 7 lines
drivers/iio/adc/nct720x.c new file +6
-0+ + chip->vin_max = (chip->type == nct7201) ?NCT7201_VIN_MAX :NCT7202_VIN_MAX ; + + ret =of_property_read_u32 (client->dev.of_node,"read-vin-data-size" , &tmp); +if (ret <0 ) { +pr_err ("read-vin-data-size property not found\n" );Please use
dev_xxxin your driver code.Got it.
Quoted 5 lines
Best regards,
Krzysztof
Quoted 463 lines · Eason Yang
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
Signed-off-by Eason Yang <[email protected]>--- MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 9 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/nct720x.c | 617 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 628 insertions(+) create mode 100644 drivers/iio/adc/nct720x.cMAINTAINERS +1
-0@@ -2753,6 +2753,7 @@ F: arch/arm/mach-npcm/ F: arch/arm64/boot/dts/nuvoton/ F: drivers/*/*/*npcm* F: drivers/*/*npcm* +F: drivers/iio/adc/nct720x.c F: drivers/rtc/rtc-nct3018y.c F: include/dt-bindings/clock/nuvoton,npcm7xx-clock.h F: include/dt-bindings/clock/nuvoton,npcm845-clk.hdrivers/iio/adc/Kconfig +9
-0@@ -1008,6 +1008,15 @@ config NAU7802 To compile this driver as a module, choose M here: the module will be called nau7802. +config NCT720X + tristate "Nuvoton Instruments NCT7201 and NCT7202 Power Monitor" + depends on I2C + help + If you say yes here you get support for the Nuvoton NCT7201 and + NCT7202 Voltage Monitor. + This driver can also be built as a module. If so, the module + will be called nct720x. + config NPCM_ADC tristate "Nuvoton NPCM ADC driver" depends on ARCH_NPCM || COMPILE_TESTdrivers/iio/adc/Makefile +1
-0@@ -91,6 +91,7 @@ obj-$(CONFIG_MESON_SARADC) += meson_saradc.o obj-$(CONFIG_MP2629_ADC) += mp2629_adc.o obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o obj-$(CONFIG_NAU7802) += nau7802.o +obj-$(CONFIG_NCT720X) += nct720x.o obj-$(CONFIG_NPCM_ADC) += npcm_adc.o obj-$(CONFIG_PAC1921) += pac1921.o obj-$(CONFIG_PAC1934) += pac1934.odrivers/iio/adc/nct720x.c new file +393
-0@@ -0,0 +1,617 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* Driver for Nuvoton nct7201 and nct7202 power monitor chips. +* +* Copyright (c) 2022 Nuvoton Inc. +*/ + +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/iio/events.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/mutex.h> + +#define REG_CHIP_ID 0xFD +#define NCT720X_ID 0xD7 +#define REG_VENDOR_ID 0xFE +#define NUVOTON_ID 0x50 +#define REG_DEVICE_ID 0xFF +#define NCT720X_DEVICE_ID 0x12 +#define VIN_MAX 12/* Counted from 1 */ +#define NCT7201_VIN_MAX 8 +#define NCT7202_VIN_MAX 12 +#define NCT720X_IN_SCALING 4995 + +#define REG_INTERRUPT_STATUS_1 0x0C +#define REG_INTERRUPT_STATUS_2 0x0D +#define REG_VOLT_LOW_BYTE 0x0F +#define REG_CONFIGURATION 0x10 +#define CONFIGURATION_START BIT(0) +#define CONFIGURATION_ALERT_MSK BIT(1) +#define CONFIGURATION_CONV_RATE BIT(2) +#define CONFIGURATION_INIT BIT(7) + +#define REG_ADVANCED_CONFIGURATION 0x11 +#define ADVANCED_CONF_MOD_ALERT BIT(0) +#define ADVANCED_CONF_MOD_STS BIT(1) +#define ADVANCED_CONF_FAULT_QUEUE BIT(2) +#define ADVANCED_CONF_EN_DEEP_SHUTDOWN BIT(4) +#define ADVANCED_CONF_EN_SMB_TIMEOUT BIT(5) +#define ADVANCED_CONF_MOD_RSTIN BIT(7) + +#define REG_CHANNEL_INPUT_MODE 0x12 +#define REG_CHANNEL_ENABLE_1 0x13 +#define REG_CHANNEL_ENABLE_2 0x14 +#define REG_INTERRUPT_MASK_1 0x15 +#define REG_INTERRUPT_MASK_2 0x16 +#define REG_BUSY_STATUS 0x1E +#define REG_ONE_SHOT 0x1F +#define REG_SMUS_ADDRESS 0xFC + +static const u8 REG_VIN [VIN_MAX ] = {0x00 ,0x01 ,0x02 ,0x03 ,0x04 ,0x05 , +0x06 ,0x07 ,0x08 ,0x09 ,0x0A ,0x0B }; +static const u8 REG_VIN_HIGH_LIMIT [VIN_MAX ] = {0x20 ,0x22 ,0x24 ,0x26 ,0x28 ,0x2A , +0x2C ,0x2E ,0x30 ,0x32 ,0x34 ,0x36 }; +static const u8 REG_VIN_LOW_LIMIT [VIN_MAX ] = {0x21 ,0x23 ,0x25 ,0x27 ,0x29 ,0x2B , +0x2D ,0x2F ,0x31 ,0x33 ,0x35 ,0x37 }; +static const u8 REG_VIN_HIGH_LIMIT_LSB [VIN_MAX ] = {0x40 ,0x42 ,0x44 ,0x46 ,0x48 ,0x4A , +0x4C ,0x4E ,0x50 ,0x52 ,0x54 ,0x56 }; +static const u8 REG_VIN_LOW_LIMIT_LSB [VIN_MAX ] = {0x41 ,0x43 ,0x45 ,0x47 ,0x49 ,0x4B , +0x4D ,0x4F ,0x51 ,0x53 ,0x55 ,0x57 }; +static u8 nct720x_chan_to_index[] = { +0 ,/* Not used */ +0 ,1 ,2 ,3 ,4 ,5 ,6 ,7 ,8 ,9 ,10 ,11 +}; + + +/* List of supported devices */ +enum nct720x_chips { + nct7201, nct7202 +}; + +struct nct720x_chip_info { +struct i2c_client *client; +enum nct720x_chips type; +struct mutex access_lock;/* for multi-byte read and write operations */ +int vin_max;/* number of VIN channels */ +u32 vin_mask; +bool use_read_byte_vin; +}; + +static const struct iio_event_spec nct720x_events[] = { + { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_RISING , + .mask_separate =BIT (IIO_EV_INFO_VALUE ) | +BIT (IIO_EV_INFO_ENABLE ), + }, { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_FALLING , + .mask_separate =BIT (IIO_EV_INFO_VALUE ) | +BIT (IIO_EV_INFO_ENABLE ), + }, +}; + +#define NCT720X_VOLTAGE_CHANNEL (chan, addr) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \ + .address = addr, \ + .event_spec = nct720x_events, \ + .num_event_specs = ARRAY_SIZE(nct720x_events), \ + } + +#define NCT720X_VOLTAGE_CHANNEL_DIFF (chan1, chan2, addr) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (chan1), \ + .channel2 = (chan2), \ + .differential = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \ + .address = addr, \ + .event_spec = nct720x_events, \ + .num_event_specs = ARRAY_SIZE(nct720x_events), \ + } + +static const struct iio_chan_spec nct720x_channels[] = { +NCT720X_VOLTAGE_CHANNEL (1 ,1 ), +NCT720X_VOLTAGE_CHANNEL (2 ,2 ), +NCT720X_VOLTAGE_CHANNEL (3 ,3 ), +NCT720X_VOLTAGE_CHANNEL (4 ,4 ), +NCT720X_VOLTAGE_CHANNEL (5 ,5 ), +NCT720X_VOLTAGE_CHANNEL (6 ,6 ), +NCT720X_VOLTAGE_CHANNEL (7 ,7 ), +NCT720X_VOLTAGE_CHANNEL (8 ,8 ), +NCT720X_VOLTAGE_CHANNEL (9 ,9 ), +NCT720X_VOLTAGE_CHANNEL (10 ,10 ), +NCT720X_VOLTAGE_CHANNEL (11 ,11 ), +NCT720X_VOLTAGE_CHANNEL (12 ,12 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (1 ,2 ,1 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (3 ,4 ,3 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (5 ,6 ,5 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (7 ,8 ,7 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (9 ,10 ,9 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (11 ,12 ,11 ), +}; + +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */ +static int nct720x_read_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; + +return i2c_smbus_read_byte_data (client, reg); +} + +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */ +static int nct720x_read_word_swapped_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; + +return i2c_smbus_read_word_swapped (client, reg); +} + +/* +* Read 2-byte register. Returns register in big-endian format or +* -ERRNO on error. +*/ +static int nct720x_read_reg16 (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; +int ret, low; + +mutex_lock (&chip->access_lock); + ret =i2c_smbus_read_byte_data (client, reg); +if (ret >=0 ) { + low = ret; + ret =i2c_smbus_read_byte_data (client, reg +1 ); +if (ret >=0 ) + ret = low | (ret <<8 ); + } + +mutex_unlock (&chip->access_lock); +return ret; +} + +/* Write 1-byte register. Returns 0 or -ERRNO on error. */ +static int nct720x_write_reg (struct nct720x_chip_info *chip,u8 reg,u8 val) +{ +struct i2c_client *client = chip->client; +int err; + + err =i2c_smbus_write_byte_data (client, reg, val); +/* wait for write command to be finished */ +mdelay (10 ); + +return err; +} + +static int nct720x_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask) +{ +int index = nct720x_chan_to_index[chan->address]; +int v1, v2, volt, err; +struct nct720x_chip_info *chip =iio_priv (indio_dev); + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +switch (mask) { +case IIO_CHAN_INFO_PROCESSED : +mutex_lock (&chip->access_lock); +if (chip->use_read_byte_vin) { +/* +* MNTVIN Low Byte together with MNTVIN High Byte +* forms the 13-bit count value. If MNTVIN High +* Byte readout is read successively, the +* NCT7201/NCT7202 will latch the MNTVIN Low Byte +* for next read. +*/ + v1 =nct720x_read_reg (chip,REG_VIN [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VOLT_LOW_BYTE ); +if (v2 <0 ) { + err = v2; + goto abort; + } + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip,REG_VIN [index]); + } + +/* Voltage(V) = 13bitCountValue * 0.0004995 */ + volt = (volt >>3 ) *NCT720X_IN_SCALING ; + *val = volt /10000 ; +mutex_unlock (&chip->access_lock); +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_read_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info, +int *val,int *val2) +{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int v1, v2, err; +int volt =0 ; +int index = nct720x_chan_to_index[chan->address]; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (info ==IIO_EV_INFO_VALUE ) { +if (dir ==IIO_EV_DIR_FALLING ) { +if (chip->use_read_byte_vin) { +/* +* Low limit VIN Low Byte together with Low limit VIN High Byte +forms the 13-bit count value +*/ +mutex_lock (&chip->access_lock); + v1 =nct720x_read_reg (chip,REG_VIN_LOW_LIMIT [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VIN_LOW_LIMIT_LSB [index]); +if (v2 <0 ) { + err = v2; + goto abort; + } +mutex_unlock (&chip->access_lock); + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip, +REG_VIN_LOW_LIMIT [index]); + } + }else { +if (chip->use_read_byte_vin) { +/* +* High limit VIN Low Byte together with high limit VIN High Byte +* forms the 13-bit count value +*/ +mutex_lock (&chip->access_lock); + v1 =nct720x_read_reg (chip,REG_VIN_HIGH_LIMIT [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VIN_HIGH_LIMIT_LSB [index]); +if (v2 <0 ) { + err = v2; + goto abort; + } +mutex_unlock (&chip->access_lock); + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip, +REG_VIN_HIGH_LIMIT [index]); + } + } + } +/* Voltage(V) = 13bitCountValue * 0.0004995 */ + volt = (volt >>3 ) *NCT720X_IN_SCALING ; + *val = volt /10000 ; + +return IIO_VAL_INT ; +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_write_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info, +int val,int val2) +{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index, err =0 ; +long v1, v2, volt; + + index = nct720x_chan_to_index[chan->address]; + volt = (val *10000 ) /NCT720X_IN_SCALING ; + v1 = volt >>5 ; + v2 = (volt &0x1f ) <<3 ; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (info ==IIO_EV_INFO_VALUE ) { +if (dir ==IIO_EV_DIR_FALLING ) { +mutex_lock (&chip->access_lock); + err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n" , index +1 ); + goto abort; + } + }else { +mutex_lock (&chip->access_lock); + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n" , index +1 ); + goto abort; + } + } + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_read_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir) +{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index = nct720x_chan_to_index[chan->address]; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +return !!(chip->vin_mask &BIT (index)); +} + +static int nct720x_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan,
drivers/iio/adc/nct720x.c line 403
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+enum iio_event_type type, + enum iio_event_direction dir, + int state) +{ + int err = 0; +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index = nct720x_chan_to_index[chan->address]; +unsigned int mask; + + mask =BIT (index);
nitpick:
You can set the mask near to the place where it is used i.e. just after below if statement and one added advantage will be in case below "return -EOPNOTSUPP" gets executed, you don't even have to set mask.
Quoted 53 lines
drivers/iio/adc/nct720x.c new file +53
-0+ +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (!state && (chip->vin_mask & mask)) + chip->vin_mask &= ~mask; +else if (state && !(chip->vin_mask & mask)) + chip->vin_mask |= mask; + +mutex_lock (&chip->access_lock); + + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_1 , chip->vin_mask &0xff ); +if (err <0 ) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_1\n" ); + goto abort; + } + +if (chip->type == nct7202) { + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_2 , chip->vin_mask >>8 ); +if (err <0 ) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_2\n" ); + goto abort; + } + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_detect (struct i2c_client *client, +struct i2c_board_info *info) +{ +struct i2c_adapter *adapter = client->adapter; + +if (!i2c_check_functionality (adapter,I2C_FUNC_SMBUS_BYTE_DATA | +I2C_FUNC_SMBUS_WORD_DATA )) +return -ENODEV ; + +/* Determine the chip type. */ +if (i2c_smbus_read_byte_data (client,REG_VENDOR_ID ) !=NUVOTON_ID || +i2c_smbus_read_byte_data (client,REG_CHIP_ID ) !=NCT720X_ID || +i2c_smbus_read_byte_data (client,REG_DEVICE_ID ) !=NCT720X_DEVICE_ID ) +return -ENODEV ; + +strscpy (info->type,"nct720x" ,I2C_NAME_SIZE ); + +return 0 ; +} + +static const struct iio_info nct720x_info = { + .read_raw = &nct720x_read_raw, + .read_event_config = &nct720x_read_event_config, + .write_event_config = &nct720x_write_event_config,
drivers/iio/adc/nct720x.c line 466
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ .read_event_value = &nct720x_read_event_value, + .write_event_value = &nct720x_write_event_value, +}; + +static const struct i2c_device_id nct720x_id[]; + +static int nct720x_init_chip (struct nct720x_chip_info *chip) +{ +int value =0 ; +int err =0 ;
Nit pick:
You don't need to initialize err and value.
Quoted 59 lines
drivers/iio/adc/nct720x.c new file +59
-0+ +/* Initial reset */ + err =nct720x_write_reg (chip,REG_CONFIGURATION ,CONFIGURATION_INIT ); +if (err) { +pr_err ("Failed to write REG_CONFIGURATION\n" ); +return err; + } + +/* Enable Channel */ + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_1 ,0xff ); +if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_1\n" ); +return err; + } + +if (chip->type == nct7202) { + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_2 ,0xf ); +if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_2\n" ); +return err; + } + } + + value =nct720x_read_reg16 (chip,REG_CHANNEL_ENABLE_1 ); +if (value <0 ) +return value; + chip->vin_mask = value; + +/* Start monitoring if needed */ + value =nct720x_read_reg (chip,REG_CONFIGURATION ); +if (value <0 ) { +pr_err ("Failed to read REG_CONFIGURATION\n" ); +return value; + } + + value |=CONFIGURATION_START ; + err =nct720x_write_reg (chip,REG_CONFIGURATION , value); +if (err <0 ) { +pr_err ("Failed to write REG_CONFIGURATION\n" ); +return err; + } + +return 0 ; +} + +static int nct720x_probe (struct i2c_client *client) +{ +const struct i2c_device_id *id =i2c_client_get_device_id (client); +struct nct720x_chip_info *chip; +struct iio_dev *indio_dev; +int ret; +u32 tmp; + + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*chip)); +if (!indio_dev) +return -ENOMEM ; + chip =iio_priv (indio_dev); + +if (client->dev.of_node)
drivers/iio/adc/nct720x.c line 535
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ chip->type = (enum nct720x_chips )device_get_match_data (&client->dev); + else + chip->type = i2c_match_id(nct720x_id, client)->driver_data; + + chip->vin_max = (chip->type == nct7201) ?NCT7201_VIN_MAX :NCT7202_VIN_MAX ; + + ret =of_property_read_u32 (client->dev.of_node,"read-vin-data-size" , &tmp); +if (ret <0 ) { +pr_err ("read-vin-data-size property not found\n" ); +return ret;
nit: dev_err_probe
drivers/iio/adc/nct720x.c line 544
Quoted 9 lines
drivers/iio/adc/nct720x.c new file +9
-0+ } + +if (tmp ==8 ) { + chip->use_read_byte_vin = true; + }else if (tmp ==16 ) { + chip->use_read_byte_vin = false; + }else { +pr_err ("invalid read-vin-data-size (%d)\n" , tmp); +return -EINVAL ;
nit: dev_err_probe
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ } + +mutex_init (&chip->access_lock); + +/* this is only used for device removal purposes */ +i2c_set_clientdata (client, indio_dev); + + chip->client = client; + + ret =nct720x_init_chip (chip);
drivers/iio/adc/nct720x.c line 564
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+if (ret <0 ) +return ret; + + indio_dev->name = id->name; + indio_dev->channels = nct720x_channels; + indio_dev->num_channels =ARRAY_SIZE (nct720x_channels); + indio_dev->info = &nct720x_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + +iio_device_register (indio_dev);
devm_iio_device_register?
drivers/iio/adc/nct720x.c line 573
Quoted 9 lines
drivers/iio/adc/nct720x.c new file +9
-0+ +return 0 ; +} + +static void nct720x_remove (struct i2c_client *client) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (client); + +iio_device_unregister (indio_dev);
wouldn't need if used devm_ version
Quoted 48 lines
drivers/iio/adc/nct720x.c new file +44
-1+} + +static const unsigned short nct720x_address_list[] = { +0x1d ,0x1e ,0x35 ,0x36 ,I2C_CLIENT_END +}; + +static const struct i2c_device_id nct720x_id[] = { + {"nct7201" , nct7201 }, + {"nct7202" , nct7202 }, + {} +}; +MODULE_DEVICE_TABLE (i2c, nct720x_id); + +static const struct of_device_id nct720x_of_match[] = { + { + .compatible ="nuvoton,nct7201" , + .data = (void *)nct7201 + }, + { + .compatible ="nuvoton,nct7202" , + .data = (void *)nct7202 + }, + { }, +}; + +MODULE_DEVICE_TABLE (of, nct720x_of_match); + +static struct i2c_driver nct720x_driver = { + .driver = { + .name ="nct720x" , + .of_match_table = nct720x_of_match, + }, + .probe = nct720x_probe, + .remove = nct720x_remove, + .id_table = nct720x_id, + .detect = nct720x_detect, + .address_list = nct720x_address_list, +}; + +module_i2c_driver (nct720x_driver); + +MODULE_AUTHOR ("Eason Yang <[email protected]>" ); +MODULE_DESCRIPTION ("Nuvoton NCT720x voltage monitor driver" ); +MODULE_LICENSE ("GPL" ); --2.34.1
Dear anish kumar,
Thank you for your response.
anish kumar <[email protected]> 於 2024年11月7日 週四 下午2:14寫道:
Quoted 473 lines · Eason Yang
Quoted 471 lines
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
Signed-off-by Eason Yang <[email protected]>--- MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 9 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/nct720x.c | 617 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 628 insertions(+) create mode 100644 drivers/iio/adc/nct720x.cMAINTAINERS +1
-0@@ -2753,6 +2753,7 @@ F: arch/arm/mach-npcm/ F: arch/arm64/boot/dts/nuvoton/ F: drivers/*/*/*npcm* F: drivers/*/*npcm* +F: drivers/iio/adc/nct720x.c F: drivers/rtc/rtc-nct3018y.c F: include/dt-bindings/clock/nuvoton,npcm7xx-clock.h F: include/dt-bindings/clock/nuvoton,npcm845-clk.hdrivers/iio/adc/Kconfig +9
-0@@ -1008,6 +1008,15 @@ config NAU7802 To compile this driver as a module, choose M here: the module will be called nau7802. +config NCT720X + tristate "Nuvoton Instruments NCT7201 and NCT7202 Power Monitor" + depends on I2C + help + If you say yes here you get support for the Nuvoton NCT7201 and + NCT7202 Voltage Monitor. + This driver can also be built as a module. If so, the module + will be called nct720x. + config NPCM_ADC tristate "Nuvoton NPCM ADC driver" depends on ARCH_NPCM || COMPILE_TESTdrivers/iio/adc/Makefile +1
-0@@ -91,6 +91,7 @@ obj-$(CONFIG_MESON_SARADC) += meson_saradc.o obj-$(CONFIG_MP2629_ADC) += mp2629_adc.o obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o obj-$(CONFIG_NAU7802) += nau7802.o +obj-$(CONFIG_NCT720X) += nct720x.o obj-$(CONFIG_NPCM_ADC) += npcm_adc.o obj-$(CONFIG_PAC1921) += pac1921.o obj-$(CONFIG_PAC1934) += pac1934.odrivers/iio/adc/nct720x.c new file +401
-0@@ -0,0 +1,617 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* Driver for Nuvoton nct7201 and nct7202 power monitor chips. +* +* Copyright (c) 2022 Nuvoton Inc. +*/ + +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/iio/events.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/mutex.h> + +#define REG_CHIP_ID 0xFD +#define NCT720X_ID 0xD7 +#define REG_VENDOR_ID 0xFE +#define NUVOTON_ID 0x50 +#define REG_DEVICE_ID 0xFF +#define NCT720X_DEVICE_ID 0x12 +#define VIN_MAX 12/* Counted from 1 */ +#define NCT7201_VIN_MAX 8 +#define NCT7202_VIN_MAX 12 +#define NCT720X_IN_SCALING 4995 + +#define REG_INTERRUPT_STATUS_1 0x0C +#define REG_INTERRUPT_STATUS_2 0x0D +#define REG_VOLT_LOW_BYTE 0x0F +#define REG_CONFIGURATION 0x10 +#define CONFIGURATION_START BIT(0) +#define CONFIGURATION_ALERT_MSK BIT(1) +#define CONFIGURATION_CONV_RATE BIT(2) +#define CONFIGURATION_INIT BIT(7) + +#define REG_ADVANCED_CONFIGURATION 0x11 +#define ADVANCED_CONF_MOD_ALERT BIT(0) +#define ADVANCED_CONF_MOD_STS BIT(1) +#define ADVANCED_CONF_FAULT_QUEUE BIT(2) +#define ADVANCED_CONF_EN_DEEP_SHUTDOWN BIT(4) +#define ADVANCED_CONF_EN_SMB_TIMEOUT BIT(5) +#define ADVANCED_CONF_MOD_RSTIN BIT(7) + +#define REG_CHANNEL_INPUT_MODE 0x12 +#define REG_CHANNEL_ENABLE_1 0x13 +#define REG_CHANNEL_ENABLE_2 0x14 +#define REG_INTERRUPT_MASK_1 0x15 +#define REG_INTERRUPT_MASK_2 0x16 +#define REG_BUSY_STATUS 0x1E +#define REG_ONE_SHOT 0x1F +#define REG_SMUS_ADDRESS 0xFC + +static const u8 REG_VIN [VIN_MAX ] = {0x00 ,0x01 ,0x02 ,0x03 ,0x04 ,0x05 , +0x06 ,0x07 ,0x08 ,0x09 ,0x0A ,0x0B }; +static const u8 REG_VIN_HIGH_LIMIT [VIN_MAX ] = {0x20 ,0x22 ,0x24 ,0x26 ,0x28 ,0x2A , +0x2C ,0x2E ,0x30 ,0x32 ,0x34 ,0x36 }; +static const u8 REG_VIN_LOW_LIMIT [VIN_MAX ] = {0x21 ,0x23 ,0x25 ,0x27 ,0x29 ,0x2B , +0x2D ,0x2F ,0x31 ,0x33 ,0x35 ,0x37 }; +static const u8 REG_VIN_HIGH_LIMIT_LSB [VIN_MAX ] = {0x40 ,0x42 ,0x44 ,0x46 ,0x48 ,0x4A , +0x4C ,0x4E ,0x50 ,0x52 ,0x54 ,0x56 }; +static const u8 REG_VIN_LOW_LIMIT_LSB [VIN_MAX ] = {0x41 ,0x43 ,0x45 ,0x47 ,0x49 ,0x4B , +0x4D ,0x4F ,0x51 ,0x53 ,0x55 ,0x57 }; +static u8 nct720x_chan_to_index[] = { +0 ,/* Not used */ +0 ,1 ,2 ,3 ,4 ,5 ,6 ,7 ,8 ,9 ,10 ,11 +}; + + +/* List of supported devices */ +enum nct720x_chips { + nct7201, nct7202 +}; + +struct nct720x_chip_info { +struct i2c_client *client; +enum nct720x_chips type; +struct mutex access_lock;/* for multi-byte read and write operations */ +int vin_max;/* number of VIN channels */ +u32 vin_mask; +bool use_read_byte_vin; +}; + +static const struct iio_event_spec nct720x_events[] = { + { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_RISING , + .mask_separate =BIT (IIO_EV_INFO_VALUE ) | +BIT (IIO_EV_INFO_ENABLE ), + }, { + .type =IIO_EV_TYPE_THRESH , + .dir =IIO_EV_DIR_FALLING , + .mask_separate =BIT (IIO_EV_INFO_VALUE ) | +BIT (IIO_EV_INFO_ENABLE ), + }, +}; + +#define NCT720X_VOLTAGE_CHANNEL (chan, addr) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \ + .address = addr, \ + .event_spec = nct720x_events, \ + .num_event_specs = ARRAY_SIZE(nct720x_events), \ + } + +#define NCT720X_VOLTAGE_CHANNEL_DIFF (chan1, chan2, addr) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (chan1), \ + .channel2 = (chan2), \ + .differential = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \ + .address = addr, \ + .event_spec = nct720x_events, \ + .num_event_specs = ARRAY_SIZE(nct720x_events), \ + } + +static const struct iio_chan_spec nct720x_channels[] = { +NCT720X_VOLTAGE_CHANNEL (1 ,1 ), +NCT720X_VOLTAGE_CHANNEL (2 ,2 ), +NCT720X_VOLTAGE_CHANNEL (3 ,3 ), +NCT720X_VOLTAGE_CHANNEL (4 ,4 ), +NCT720X_VOLTAGE_CHANNEL (5 ,5 ), +NCT720X_VOLTAGE_CHANNEL (6 ,6 ), +NCT720X_VOLTAGE_CHANNEL (7 ,7 ), +NCT720X_VOLTAGE_CHANNEL (8 ,8 ), +NCT720X_VOLTAGE_CHANNEL (9 ,9 ), +NCT720X_VOLTAGE_CHANNEL (10 ,10 ), +NCT720X_VOLTAGE_CHANNEL (11 ,11 ), +NCT720X_VOLTAGE_CHANNEL (12 ,12 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (1 ,2 ,1 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (3 ,4 ,3 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (5 ,6 ,5 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (7 ,8 ,7 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (9 ,10 ,9 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (11 ,12 ,11 ), +}; + +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */ +static int nct720x_read_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; + +return i2c_smbus_read_byte_data (client, reg); +} + +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */ +static int nct720x_read_word_swapped_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; + +return i2c_smbus_read_word_swapped (client, reg); +} + +/* +* Read 2-byte register. Returns register in big-endian format or +* -ERRNO on error. +*/ +static int nct720x_read_reg16 (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; +int ret, low; + +mutex_lock (&chip->access_lock); + ret =i2c_smbus_read_byte_data (client, reg); +if (ret >=0 ) { + low = ret; + ret =i2c_smbus_read_byte_data (client, reg +1 ); +if (ret >=0 ) + ret = low | (ret <<8 ); + } + +mutex_unlock (&chip->access_lock); +return ret; +} + +/* Write 1-byte register. Returns 0 or -ERRNO on error. */ +static int nct720x_write_reg (struct nct720x_chip_info *chip,u8 reg,u8 val) +{ +struct i2c_client *client = chip->client; +int err; + + err =i2c_smbus_write_byte_data (client, reg, val); +/* wait for write command to be finished */ +mdelay (10 ); + +return err; +} + +static int nct720x_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask) +{ +int index = nct720x_chan_to_index[chan->address]; +int v1, v2, volt, err; +struct nct720x_chip_info *chip =iio_priv (indio_dev); + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +switch (mask) { +case IIO_CHAN_INFO_PROCESSED : +mutex_lock (&chip->access_lock); +if (chip->use_read_byte_vin) { +/* +* MNTVIN Low Byte together with MNTVIN High Byte +* forms the 13-bit count value. If MNTVIN High +* Byte readout is read successively, the +* NCT7201/NCT7202 will latch the MNTVIN Low Byte +* for next read. +*/ + v1 =nct720x_read_reg (chip,REG_VIN [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VOLT_LOW_BYTE ); +if (v2 <0 ) { + err = v2; + goto abort; + } + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip,REG_VIN [index]); + } + +/* Voltage(V) = 13bitCountValue * 0.0004995 */ + volt = (volt >>3 ) *NCT720X_IN_SCALING ; + *val = volt /10000 ; +mutex_unlock (&chip->access_lock); +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_read_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info, +int *val,int *val2) +{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int v1, v2, err; +int volt =0 ; +int index = nct720x_chan_to_index[chan->address]; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (info ==IIO_EV_INFO_VALUE ) { +if (dir ==IIO_EV_DIR_FALLING ) { +if (chip->use_read_byte_vin) { +/* +* Low limit VIN Low Byte together with Low limit VIN High Byte +forms the 13-bit count value +*/ +mutex_lock (&chip->access_lock); + v1 =nct720x_read_reg (chip,REG_VIN_LOW_LIMIT [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VIN_LOW_LIMIT_LSB [index]); +if (v2 <0 ) { + err = v2; + goto abort; + } +mutex_unlock (&chip->access_lock); + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip, +REG_VIN_LOW_LIMIT [index]); + } + }else { +if (chip->use_read_byte_vin) { +/* +* High limit VIN Low Byte together with high limit VIN High Byte +* forms the 13-bit count value +*/ +mutex_lock (&chip->access_lock); + v1 =nct720x_read_reg (chip,REG_VIN_HIGH_LIMIT [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VIN_HIGH_LIMIT_LSB [index]); +if (v2 <0 ) { + err = v2; + goto abort; + } +mutex_unlock (&chip->access_lock); + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip, +REG_VIN_HIGH_LIMIT [index]); + } + } + } +/* Voltage(V) = 13bitCountValue * 0.0004995 */ + volt = (volt >>3 ) *NCT720X_IN_SCALING ; + *val = volt /10000 ; + +return IIO_VAL_INT ; +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_write_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info, +int val,int val2) +{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index, err =0 ; +long v1, v2, volt; + + index = nct720x_chan_to_index[chan->address]; + volt = (val *10000 ) /NCT720X_IN_SCALING ; + v1 = volt >>5 ; + v2 = (volt &0x1f ) <<3 ; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (info ==IIO_EV_INFO_VALUE ) { +if (dir ==IIO_EV_DIR_FALLING ) { +mutex_lock (&chip->access_lock); + err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n" , index +1 ); + goto abort; + } + }else { +mutex_lock (&chip->access_lock); + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n" , index +1 ); + goto abort; + } + } + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_read_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir) +{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index = nct720x_chan_to_index[chan->address]; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +return !!(chip->vin_mask &BIT (index)); +} + +static int nct720x_write_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +int state) +{ +int err =0 ; +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index = nct720x_chan_to_index[chan->address]; +unsigned int mask;
Quoted 9 lines
drivers/iio/adc/nct720x.c line 403
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+ + mask =BIT (index);nitpick:
You can set the mask near to the place where it is used i.e. just after below if statement and one added advantage will be in case below "return-EOPNOTSUPP" gets executed, you don't even have to set mask.
Got it.
Quoted 58 lines
Quoted 58 lines
drivers/iio/adc/nct720x.c new file +58
-0+ +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (!state && (chip->vin_mask & mask)) + chip->vin_mask &= ~mask; +else if (state && !(chip->vin_mask & mask)) + chip->vin_mask |= mask; + +mutex_lock (&chip->access_lock); + + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_1 , chip->vin_mask &0xff ); +if (err <0 ) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_1\n" ); + goto abort; + } + +if (chip->type == nct7202) { + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_2 , chip->vin_mask >>8 ); +if (err <0 ) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_2\n" ); + goto abort; + } + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_detect (struct i2c_client *client, +struct i2c_board_info *info) +{ +struct i2c_adapter *adapter = client->adapter; + +if (!i2c_check_functionality (adapter,I2C_FUNC_SMBUS_BYTE_DATA | +I2C_FUNC_SMBUS_WORD_DATA )) +return -ENODEV ; + +/* Determine the chip type. */ +if (i2c_smbus_read_byte_data (client,REG_VENDOR_ID ) !=NUVOTON_ID || +i2c_smbus_read_byte_data (client,REG_CHIP_ID ) !=NCT720X_ID || +i2c_smbus_read_byte_data (client,REG_DEVICE_ID ) !=NCT720X_DEVICE_ID ) +return -ENODEV ; + +strscpy (info->type,"nct720x" ,I2C_NAME_SIZE ); + +return 0 ; +} + +static const struct iio_info nct720x_info = { + .read_raw = &nct720x_read_raw, + .read_event_config = &nct720x_read_event_config, + .write_event_config = &nct720x_write_event_config, + .read_event_value = &nct720x_read_event_value, + .write_event_value = &nct720x_write_event_value, +}; + +static const struct i2c_device_id nct720x_id[];
Quoted 9 lines
drivers/iio/adc/nct720x.c line 466
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ +static int nct720x_init_chip (struct nct720x_chip_info *chip) +{ +int value =0 ; +int err =0 ;Nit pick:
You don't need to initialize err and value.
Got it.
Quoted 63 lines
Quoted 63 lines
drivers/iio/adc/nct720x.c new file +63
-0+ +/* Initial reset */ + err =nct720x_write_reg (chip,REG_CONFIGURATION ,CONFIGURATION_INIT ); +if (err) { +pr_err ("Failed to write REG_CONFIGURATION\n" ); +return err; + } + +/* Enable Channel */ + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_1 ,0xff ); +if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_1\n" ); +return err; + } + +if (chip->type == nct7202) { + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_2 ,0xf ); +if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_2\n" ); +return err; + } + } + + value =nct720x_read_reg16 (chip,REG_CHANNEL_ENABLE_1 ); +if (value <0 ) +return value; + chip->vin_mask = value; + +/* Start monitoring if needed */ + value =nct720x_read_reg (chip,REG_CONFIGURATION ); +if (value <0 ) { +pr_err ("Failed to read REG_CONFIGURATION\n" ); +return value; + } + + value |=CONFIGURATION_START ; + err =nct720x_write_reg (chip,REG_CONFIGURATION , value); +if (err <0 ) { +pr_err ("Failed to write REG_CONFIGURATION\n" ); +return err; + } + +return 0 ; +} + +static int nct720x_probe (struct i2c_client *client) +{ +const struct i2c_device_id *id =i2c_client_get_device_id (client); +struct nct720x_chip_info *chip; +struct iio_dev *indio_dev; +int ret; +u32 tmp; + + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*chip)); +if (!indio_dev) +return -ENOMEM ; + chip =iio_priv (indio_dev); + +if (client->dev.of_node) + chip->type = (enum nct720x_chips )device_get_match_data (&client->dev); +else + chip->type =i2c_match_id (nct720x_id, client)->driver_data; +
Quoted 9 lines
drivers/iio/adc/nct720x.c line 535
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ chip->vin_max = (chip->type == nct7201) ?NCT7201_VIN_MAX :NCT7202_VIN_MAX ; + + ret =of_property_read_u32 (client->dev.of_node,"read-vin-data-size" , &tmp); +if (ret <0 ) { +pr_err ("read-vin-data-size property not found\n" ); +return ret;nit:
dev_err_probe
I would use dev_err_probe to replace pr_err in probe function
Quoted 10 lines
drivers/iio/adc/nct720x.c line 544
Quoted 9 lines
drivers/iio/adc/nct720x.c new file +9
-0+ } + +if (tmp ==8 ) { + chip->use_read_byte_vin = true; + }else if (tmp ==16 ) { + chip->use_read_byte_vin = false; + }else { +pr_err ("invalid read-vin-data-size (%d)\n" , tmp); +return -EINVAL ;
Quoted 2 lines
nit:
dev_err_probe
I would use dev_err_probe to replace pr_err in probe function.
Quoted 14 lines
Quoted 14 lines
drivers/iio/adc/nct720x.c new file +14
-0+ } + +mutex_init (&chip->access_lock); + +/* this is only used for device removal purposes */ +i2c_set_clientdata (client, indio_dev); + + chip->client = client; + + ret =nct720x_init_chip (chip); +if (ret <0 ) +return ret; + + indio_dev->name = id->name;
Quoted 9 lines
drivers/iio/adc/nct720x.c line 564
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ indio_dev->channels = nct720x_channels; + indio_dev->num_channels =ARRAY_SIZE (nct720x_channels); + indio_dev->info = &nct720x_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + +iio_device_register (indio_dev);
devm_iio_device_register?
Yes, I would use devm_iio_device_registe to replace iio_device_register and no need to use remove function.
Quoted 11 lines
drivers/iio/adc/nct720x.c line 573
Quoted 9 lines
drivers/iio/adc/nct720x.c new file +9
-0+ +return 0 ; +} + +static void nct720x_remove (struct i2c_client *client) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (client); + +iio_device_unregister (indio_dev);wouldn't need if used
devm_version
Got it.
Quoted 48 lines
Quoted 48 lines
drivers/iio/adc/nct720x.c new file +44
-1+} + +static const unsigned short nct720x_address_list[] = { +0x1d ,0x1e ,0x35 ,0x36 ,I2C_CLIENT_END +}; + +static const struct i2c_device_id nct720x_id[] = { + {"nct7201" , nct7201 }, + {"nct7202" , nct7202 }, + {} +}; +MODULE_DEVICE_TABLE (i2c, nct720x_id); + +static const struct of_device_id nct720x_of_match[] = { + { + .compatible ="nuvoton,nct7201" , + .data = (void *)nct7201 + }, + { + .compatible ="nuvoton,nct7202" , + .data = (void *)nct7202 + }, + { }, +}; + +MODULE_DEVICE_TABLE (of, nct720x_of_match); + +static struct i2c_driver nct720x_driver = { + .driver = { + .name ="nct720x" , + .of_match_table = nct720x_of_match, + }, + .probe = nct720x_probe, + .remove = nct720x_remove, + .id_table = nct720x_id, + .detect = nct720x_detect, + .address_list = nct720x_address_list, +}; + +module_i2c_driver (nct720x_driver); + +MODULE_AUTHOR ("Eason Yang <[email protected]>" ); +MODULE_DESCRIPTION ("Nuvoton NCT720x voltage monitor driver" ); +MODULE_LICENSE ("GPL" ); --2.34.1
Andy Shevchenko
reviewerQuoted 9 lines · Eason Yang
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
...
Quoted 1 line
+ * Copyright (c) 2022 Nuvoton Inc.
2024?
...
+ array_size.h
+ bits.h
drivers/iio/adc/nct720x.c line 9
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+#include <linux/delay.h> +#include <linux/device.h>
+ err.h
drivers/iio/adc/nct720x.c line 13
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +4
-0+#include <linux/i2c.h> +#include <linux/iio/events.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h>
Move these to be a separate group...
Quoted 1 line
+#include <
linux/init.h>
+ mod_devicetable.h
drivers/iio/adc/nct720x.c line 16
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+#include <linux/module.h> +#include <linux/mutex.h>
+ types.h
...somewhere here after a blank line.
...
drivers/iio/adc/nct720x.c line 56
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+static const u8 REG_VIN [VIN_MAX ] = {0x00 ,0x01 ,0x02 ,0x03 ,0x04 ,0x05 , +0x06 ,0x07 ,0x08 ,0x09 ,0x0A ,0x0B };
Better to have it done like this
static const u8 REG_VIN[VIN_MAX] = {
0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, /* 0 - 7 */
0x08, 0x09, 0x0A, 0x0B, /* 8 - 11 */
};Note the following: the values are placed on separate lines with trailing commas on each line and with grouping by powwer-of-2 per line.
drivers/iio/adc/nct720x.c line 68
Quoted 12 lines
drivers/iio/adc/nct720x.c new file +12
-0+static const u8 REG_VIN_HIGH_LIMIT [VIN_MAX ] = {0x20 ,0x22 ,0x24 ,0x26 ,0x28 ,0x2A , +0x2C ,0x2E ,0x30 ,0x32 ,0x34 ,0x36 }; +static const u8 REG_VIN_LOW_LIMIT [VIN_MAX ] = {0x21 ,0x23 ,0x25 ,0x27 ,0x29 ,0x2B , +0x2D ,0x2F ,0x31 ,0x33 ,0x35 ,0x37 }; +static const u8 REG_VIN_HIGH_LIMIT_LSB [VIN_MAX ] = {0x40 ,0x42 ,0x44 ,0x46 ,0x48 ,0x4A , +0x4C ,0x4E ,0x50 ,0x52 ,0x54 ,0x56 }; +static const u8 REG_VIN_LOW_LIMIT_LSB [VIN_MAX ] = {0x41 ,0x43 ,0x45 ,0x47 ,0x49 ,0x4B , +0x4D ,0x4F ,0x51 ,0x53 ,0x55 ,0x57 }; +static u8 nct720x_chan_to_index[] = { +0 ,/* Not used */ +0 ,1 ,2 ,3 ,4 ,5 ,6 ,7 ,8 ,9 ,10 ,11 +};
Ditto for the above initialisers.
Review
Quoted 2 lines
(quoted diff) +2
-0+ +
One blank line is enough.
drivers/iio/adc/nct720x.c line 73
Quoted 3 lines
drivers/iio/adc/nct720x.c new file +3
-0+/* List of supported devices */ +enum nct720x_chips { + nct7201, nct7202
Leave trailing comma.
Quoted 1 line
+};
...
Quoted 16 lines
drivers/iio/adc/nct720x.c new file +16
-0+/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */ +static int nct720x_read_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; + +return i2c_smbus_read_byte_data (client, reg); +} + +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */ +static int nct720x_read_word_swapped_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; + +return i2c_smbus_read_word_swapped (client, reg); +} +
drivers/iio/adc/nct720x.c line 169
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +9
-0+/* +* Read 2-byte register. Returns register in big-endian format or +* -ERRNO on error. +*/ +static int nct720x_read_reg16 (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; +int ret, low; +mutex_lock (&chip->access_lock);
If you are going with this, use cleanup.h.
drivers/iio/adc/nct720x.c line 175
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ ret =i2c_smbus_read_byte_data (client, reg); +if (ret >=0 ) { + low = ret; + ret =i2c_smbus_read_byte_data (client, reg +1 ); +if (ret >=0 ) + ret = low | (ret <<8 );
Hmm... Does it require to have two separate transactions? In case of regmap, can't bulk transfer be performed for that?
drivers/iio/adc/nct720x.c line 180
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ } + +mutex_unlock (&chip->access_lock); +return ret; +}
I have no explanations why regmap I2C can't be used.
...
drivers/iio/adc/nct720x.c line 189
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+/* Write 1-byte register. Returns 0 or -ERRNO on error. */ +static int nct720x_write_reg (struct nct720x_chip_info *chip,u8 reg,u8 val) +{ +struct i2c_client *client = chip->client; +int err; + + err =i2c_smbus_write_byte_data (client, reg, val); +/* wait for write command to be finished */
Is there any justification for the chosen value (I mean datasheet recommendations or so)?
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+mdelay (10 ); + +return err; +}
...
Quoted 24 lines
drivers/iio/adc/nct720x.c new file +24
-0+static int nct720x_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask) +{ +int index = nct720x_chan_to_index[chan->address]; +int v1, v2, volt, err; +struct nct720x_chip_info *chip =iio_priv (indio_dev); + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +switch (mask) { +case IIO_CHAN_INFO_PROCESSED : +mutex_lock (&chip->access_lock); +if (chip->use_read_byte_vin) { +/* +* MNTVIN Low Byte together with MNTVIN High Byte +* forms the 13-bit count value. If MNTVIN High +* Byte readout is read successively, the +* NCT7201/NCT7202 will latch the MNTVIN Low Byte +* for next read. +*/ + v1 =nct720x_read_reg (chip,REG_VIN [index]); +if (v1 <0 ) {
drivers/iio/adc/nct720x.c line 228
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VOLT_LOW_BYTE ); +if (v2 <0 ) { + err = v2; + goto abort; + } + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */
Hmm... I would expect volt to be __be16, but since you have two separate reads it's probably fine.
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip,REG_VIN [index]); + } + +/* Voltage(V) = 13bitCountValue * 0.0004995 */
drivers/iio/adc/nct720x.c line 244
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ volt = (volt >>3 ) *NCT720X_IN_SCALING ; + *val = volt /10000 ; +mutex_unlock (&chip->access_lock); +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +abort: +mutex_unlock (&chip->access_lock); +return err;
Yeah, this will gone with cleanup.h in use.
Ditto for all other places like this.
Quoted 1 line
+}
...
drivers/iio/adc/nct720x.c line 334
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index, err =0 ; +long v1, v2, volt;
Why long? Wouldn't int / unsigned int suffice for v1 / v2?
drivers/iio/adc/nct720x.c line 337
Quoted 3 lines
drivers/iio/adc/nct720x.c new file +3
-0+ + index = nct720x_chan_to_index[chan->address]; + volt = (val *10000 ) /NCT720X_IN_SCALING ;
Please, use named constant for 10000 as you used it at least twice in the code.
drivers/iio/adc/nct720x.c line 339
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+ v1 = volt >>5 ; + v2 = (volt &0x1f ) <<3 ;
In the similar way it seems like some endianess and bit width manipulations.
Can v be an array of u8:s which you just fill with put_unaligned_be16() or so?
Quoted 36 lines
drivers/iio/adc/nct720x.c new file +36
-0+if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (info ==IIO_EV_INFO_VALUE ) { +if (dir ==IIO_EV_DIR_FALLING ) { +mutex_lock (&chip->access_lock); + err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n" , index +1 ); + goto abort; + } + }else { +mutex_lock (&chip->access_lock); + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n" , index +1 ); + goto abort; + } + } + } +abort: +mutex_unlock (&chip->access_lock); +return err; +}
...
drivers/iio/adc/nct720x.c line 448
Quoted 1 line
drivers/iio/adc/nct720x.c new file +1
-0+strscpy (info->type,"nct720x" ,I2C_NAME_SIZE );
Use 2-argument strscpy().
...
Quoted 1 line
+static const struct
i2c_device_idnct720x_id[];
Remove this. It's not needed.
...
drivers/iio/adc/nct720x.c line 465
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +3
-0+static int nct720x_init_chip (struct nct720x_chip_info *chip) +{ +int value =0 ;
Why signed?
Review
Quoted 1 line
(quoted diff) +1
-0+ int err = 0;
Redundant assignments.
drivers/iio/adc/nct720x.c line 476
Quoted 9 lines
drivers/iio/adc/nct720x.c new file +9
-0+/* Initial reset */ + err =nct720x_write_reg (chip,REG_CONFIGURATION ,CONFIGURATION_INIT ); +if (err) { +pr_err ("Failed to write REG_CONFIGURATION\n" ); +return err; + } + +/* Enable Channel */ + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_1 ,0xff );
Magic 0xff.
Should be GENMASK() ? Anything else?
drivers/iio/adc/nct720x.c line 483
Quoted 7 lines
drivers/iio/adc/nct720x.c new file +7
-0+if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_1\n" ); +return err; + } + +if (chip->type == nct7202) { + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_2 ,0xf );
Ditto.
Quoted 27 lines
drivers/iio/adc/nct720x.c new file +27
-0+if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_2\n" ); +return err; + } + } + + value =nct720x_read_reg16 (chip,REG_CHANNEL_ENABLE_1 ); +if (value <0 ) +return value; + chip->vin_mask = value; + +/* Start monitoring if needed */ + value =nct720x_read_reg (chip,REG_CONFIGURATION ); +if (value <0 ) { +pr_err ("Failed to read REG_CONFIGURATION\n" ); +return value; + } + + value |=CONFIGURATION_START ; + err =nct720x_write_reg (chip,REG_CONFIGURATION , value); +if (err <0 ) { +pr_err ("Failed to write REG_CONFIGURATION\n" ); +return err; + } + +return 0 ; +}
...
drivers/iio/adc/nct720x.c line 532
Quoted 1 line
drivers/iio/adc/nct720x.c new file +1
-0+ ret =of_property_read_u32 (client->dev.of_node,"read-vin-data-size" , &tmp);
No OF-centric APIs in a new code, please.
Use device_property_read_u32() and include property.h for that.
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+if (ret <0 ) { +pr_err ("read-vin-data-size property not found\n" ); +return ret; + }
...
drivers/iio/adc/nct720x.c line 547
Quoted 1 line
drivers/iio/adc/nct720x.c new file +1
-0+mutex_init (&chip->access_lock);
ret = devm_mutex_init(...);
...
drivers/iio/adc/nct720x.c line 550
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+/* this is only used for device removal purposes */ +i2c_set_clientdata (client, indio_dev);
Will be gone after removing ->remove().
...
drivers/iio/adc/nct720x.c line 564
Quoted 1 line
drivers/iio/adc/nct720x.c new file +1
-0+iio_device_register (indio_dev);
No error checks? In any case devm_ variant especially needs this.
Review
Quoted 1 line
(quoted diff) +1
-0+ return 0;
return devm_iio_device_register(...);
...
drivers/iio/adc/nct720x.c line 582
Quoted 3 lines
drivers/iio/adc/nct720x.c new file +3
-0+static const struct i2c_device_id nct720x_id[] = { + {"nct7201" , nct7201 }, + {"nct7202" , nct7202 },
Please, use chip_info custom data structure instead of enum values.
drivers/iio/adc/nct720x.c line 594
Quoted 12 lines
drivers/iio/adc/nct720x.c new file +12
-0+ {} +}; +MODULE_DEVICE_TABLE (i2c, nct720x_id); + +static const struct of_device_id nct720x_of_match[] = { + { + .compatible ="nuvoton,nct7201" , + .data = (void *)nct7201 + }, + { + .compatible ="nuvoton,nct7202" , + .data = (void *)nct7202
As per above and leave trailing commas.
Review
Quoted 2 lines
(quoted diff) +2
-0+ }, + { },
Drop trailing comma in the terminator entries.
drivers/iio/adc/nct720x.c line 69
Quoted 3 lines
drivers/iio/adc/nct720x.c new file +2
-0+}; +
Redundant blank line.
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+MODULE_DEVICE_TABLE (of, nct720x_of_match); + +static struct i2c_driver nct720x_driver = { + .driver = { + .name ="nct720x" ,
drivers/iio/adc/nct720x.c line 612
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +9
-0+ .of_match_table = nct720x_of_match, + }, + .probe = nct720x_probe, + .remove = nct720x_remove, + .id_table = nct720x_id, + .detect = nct720x_detect, + .address_list = nct720x_address_list, +}; +
Redundant blank line.
Quoted 1 line
+module_i2c_driver(nct720x_driver);
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerQuoted 11 lines · On Wed, 6 Nov 2024 10:39:16 +0800 Eason Yang <[email protected]> wrote:
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
Signed-off-by Eason Yang <[email protected]>
You've gotten some good review already so I may well repeat stuff plus will only take a fairly superficial look at this version.
drivers/iio/adc/nct720x.c line 73
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +4
-0+ +/* List of supported devices */ +enum nct720x_chips { + nct7201, nct7202
This should be replaced by chip specific information structures. That ends up a lot more extensible than using an enum and fixing up what happens in all code paths on a per enum value basis.
Quoted 20 lines
drivers/iio/adc/nct720x.c new file +19
-0+}; + +struct nct720x_chip_info { +struct i2c_client *client; +enum nct720x_chips type; +struct mutex access_lock;/* for multi-byte read and write operations */ +int vin_max;/* number of VIN channels */ +u32 vin_mask; +bool use_read_byte_vin; +}; +#define NCT720X_VOLTAGE_CHANNEL (chan, addr) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \ + .address = addr, \ + .event_spec = nct720x_events, \ + .num_event_specs = ARRAY_SIZE(nct720x_events), \
drivers/iio/adc/nct720x.c line 117
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ } + +#define NCT720X_VOLTAGE_CHANNEL_DIFF (chan1, chan2, addr) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (chan1), \ + .channel2 = (chan2), \ + .differential = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
See below - should be _RAW and _SCALE not _PROCESSED.
Quoted 30 lines
drivers/iio/adc/nct720x.c new file +30
-0+ .address = addr, \ + .event_spec = nct720x_events, \ + .num_event_specs = ARRAY_SIZE(nct720x_events), \ + } + +static const struct iio_chan_spec nct720x_channels[] = { +NCT720X_VOLTAGE_CHANNEL (1 ,1 ), +NCT720X_VOLTAGE_CHANNEL (2 ,2 ), +NCT720X_VOLTAGE_CHANNEL (3 ,3 ), +NCT720X_VOLTAGE_CHANNEL (4 ,4 ), +NCT720X_VOLTAGE_CHANNEL (5 ,5 ), +NCT720X_VOLTAGE_CHANNEL (6 ,6 ), +NCT720X_VOLTAGE_CHANNEL (7 ,7 ), +NCT720X_VOLTAGE_CHANNEL (8 ,8 ), +NCT720X_VOLTAGE_CHANNEL (9 ,9 ), +NCT720X_VOLTAGE_CHANNEL (10 ,10 ), +NCT720X_VOLTAGE_CHANNEL (11 ,11 ), +NCT720X_VOLTAGE_CHANNEL (12 ,12 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (1 ,2 ,1 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (3 ,4 ,3 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (5 ,6 ,5 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (7 ,8 ,7 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (9 ,10 ,9 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (11 ,12 ,11 ), +}; + +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */ +static int nct720x_read_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client;
drivers/iio/adc/nct720x.c line 157
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ +return i2c_smbus_read_byte_data (client, reg); +} + +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */ +static int nct720x_read_word_swapped_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; + +return i2c_smbus_read_word_swapped (client, reg);
Don't provide these wrappers as they don't add anything useful. Make the calls directly inline.
drivers/iio/adc/nct720x.c line 169
Quoted 12 lines
drivers/iio/adc/nct720x.c new file +12
-0+} + +/* +* Read 2-byte register. Returns register in big-endian format or +* -ERRNO on error. +*/ +static int nct720x_read_reg16 (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; +int ret, low; + +mutex_lock (&chip->access_lock);
guard()
drivers/iio/adc/nct720x.c line 175
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ ret =i2c_smbus_read_byte_data (client, reg); +if (ret >=0 ) { + low = ret; + ret =i2c_smbus_read_byte_data (client, reg +1 ); +if (ret >=0 ) + ret = low | (ret <<8 );
if (ret < 0) return ret; reg = get_unaligned_le16()
on an appropriate u8 data[2]; ideally filled by a bulk regmap read.
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+ } + +mutex_unlock (&chip->access_lock); +return ret;
drivers/iio/adc/nct720x.c line 189
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+} + +/* Write 1-byte register. Returns 0 or -ERRNO on error. */ +static int nct720x_write_reg (struct nct720x_chip_info *chip,u8 reg,u8 val) +{ +struct i2c_client *client = chip->client; +int err; + + err =i2c_smbus_write_byte_data (client, reg, val); +/* wait for write command to be finished */
If this is needed, provide a datasheet reference. It is very ususual to see a significant delay needed.
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+mdelay (10 ); + +return err; +} + +static int nct720x_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask) +{ +int index = nct720x_chan_to_index[chan->address];
drivers/iio/adc/nct720x.c line 209
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+int v1, v2, volt, err; +struct nct720x_chip_info *chip =iio_priv (indio_dev); + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +switch (mask) { +case IIO_CHAN_INFO_PROCESSED : +mutex_lock (&chip->access_lock); +if (chip->use_read_byte_vin) {
Ah. So resolution doesn't change, you are controlling the i2c acceses?
That should come from the i2c controller capabilities, not DT for this device.
Quoted 15 lines
drivers/iio/adc/nct720x.c new file +15
-0+/* +* MNTVIN Low Byte together with MNTVIN High Byte +* forms the 13-bit count value. If MNTVIN High +* Byte readout is read successively, the +* NCT7201/NCT7202 will latch the MNTVIN Low Byte +* for next read. +*/ + v1 =nct720x_read_reg (chip,REG_VIN [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VOLT_LOW_BYTE ); +if (v2 <0 ) {
drivers/iio/adc/nct720x.c line 234
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ err = v2; + goto abort; + } + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip,REG_VIN [index]); + } + +/* Voltage(V) = 13bitCountValue * 0.0004995 */
Present this as _RAW and provide _SCALE to userspace to be able to do this maths. Very unusual for an ADC driver to provided processed channels. Normally only occurs it there is something non linear going on.
Quoted 18 lines
drivers/iio/adc/nct720x.c new file +18
-0+ volt = (volt >>3 ) *NCT720X_IN_SCALING ; + *val = volt /10000 ; +mutex_unlock (&chip->access_lock); +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_read_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info, +int *val,int *val2)
drivers/iio/adc/nct720x.c line 262
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int v1, v2, err; +int volt =0 ; +int index = nct720x_chan_to_index[chan->address]; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (info ==IIO_EV_INFO_VALUE ) {
As below - flip logic.
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+if (dir ==IIO_EV_DIR_FALLING ) { +if (chip->use_read_byte_vin) { +/* +* Low limit VIN Low Byte together with Low limit VIN High Byte +forms the 13-bit count value +*/ +mutex_lock (&chip->access_lock); + v1 =nct720x_read_reg (chip,REG_VIN_LOW_LIMIT [index]); +if (v1 <0 ) { + err = v1;
drivers/iio/adc/nct720x.c line 282
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VIN_LOW_LIMIT_LSB [index]); +if (v2 <0 ) { + err = v2; + goto abort; + } +mutex_unlock (&chip->access_lock); + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */
rather see this as a get_unaligned_le16 on an array of u8.
In some cases that ends up quite a bit cheaper and it also documents what is going on.
Quoted 29 lines
drivers/iio/adc/nct720x.c new file +29
-0+ }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip, +REG_VIN_LOW_LIMIT [index]); + } + }else { +if (chip->use_read_byte_vin) { +/* +* High limit VIN Low Byte together with high limit VIN High Byte +* forms the 13-bit count value +*/ +mutex_lock (&chip->access_lock); + v1 =nct720x_read_reg (chip,REG_VIN_HIGH_LIMIT [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VIN_HIGH_LIMIT_LSB [index]); +if (v2 <0 ) { + err = v2; + goto abort; + } +mutex_unlock (&chip->access_lock); + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip, +REG_VIN_HIGH_LIMIT [index]);
drivers/iio/adc/nct720x.c line 321
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ } + } + } +/* Voltage(V) = 13bitCountValue * 0.0004995 */ + volt = (volt >>3 ) *NCT720X_IN_SCALING ; + *val = volt /10000 ; + +return IIO_VAL_INT ; +abort: +mutex_unlock (&chip->access_lock);
guard() in appropriate places.
Again, the lock and unlock should ideally be in same scope.
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+return err; +} + +static int nct720x_write_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info,
drivers/iio/adc/nct720x.c line 339
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ int val, int val2) +{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index, err =0 ; +long v1, v2, volt; + + index = nct720x_chan_to_index[chan->address]; + volt = (val *10000 ) /NCT720X_IN_SCALING ; + v1 = volt >>5 ; + v2 = (volt &0x1f ) <<3 ;
Some explanatory comments for this would be good.
drivers/iio/adc/nct720x.c line 262
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (info ==IIO_EV_INFO_VALUE ) {
Flip logic.
if (info != IIO_EV_INFO_VALUE) return -EINVAL; if (dir == IIO_EVE_DIR_FALLING) etc
drivers/iio/adc/nct720x.c
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+if (dir ==IIO_EV_DIR_FALLING ) { +mutex_lock (&chip->access_lock);
This is badly nested. Mutex lock and unlock should be in same scope. So I'd pull mutex_lock() out of this if stack and use guard(mutex) instead. That way you can just return on error.
Quoted 44 lines
drivers/iio/adc/nct720x.c new file +43
-0+ err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n" , index +1 ); + goto abort; + } + }else { +mutex_lock (&chip->access_lock); + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n" , index +1 ); + goto abort; + } + } + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_write_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +int state) +{ +int err =0 ; +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index = nct720x_chan_to_index[chan->address]; +unsigned int mask; + + mask =BIT (index);
drivers/iio/adc/nct720x.c line 413
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (!state && (chip->vin_mask & mask)) + chip->vin_mask &= ~mask; +else if (state && !(chip->vin_mask & mask)) + chip->vin_mask |= mask; + +mutex_lock (&chip->access_lock);
guard(mutex)(&chip->access_lock);
drivers/iio/adc/nct720x.c line 418
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_1 , chip->vin_mask &0xff ); +if (err <0 ) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_1\n" ); + goto abort;
dev_err() return err;
drivers/iio/adc/nct720x.c line 421
Quoted 3 lines
drivers/iio/adc/nct720x.c new file +3
-0+ } + +if (chip->type == nct7202) {
Gain, base this on a chip_info flag.
drivers/iio/adc/nct720x.c line 425
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+ err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_2 , chip->vin_mask >>8 ); +if (err <0 ) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_2\n" ); + goto abort;
Same as above.
Quoted 13 lines
drivers/iio/adc/nct720x.c new file +13
-0+ } + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_detect(struct i2c_client *client, + struct i2c_board_info *info) +{ + struct i2c_adapter *adapter = client->adapter; + + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
drivers/iio/adc/nct720x.c line 448
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ I2C_FUNC_SMBUS_WORD_DATA)) +return -ENODEV ; + +/* Determine the chip type. */ +if (i2c_smbus_read_byte_data (client,REG_VENDOR_ID ) !=NUVOTON_ID || +i2c_smbus_read_byte_data (client,REG_CHIP_ID ) !=NCT720X_ID || +i2c_smbus_read_byte_data (client,REG_DEVICE_ID ) !=NCT720X_DEVICE_ID ) +return -ENODEV ; + +strscpy (info->type,"nct720x" ,I2C_NAME_SIZE );
as below. It's unusual to find a detect in an IIO driver because the firmware normally tells us what is there.
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ +return 0 ; +} + +static const struct iio_info nct720x_info = { + .read_raw = &nct720x_read_raw, + .read_event_config = &nct720x_read_event_config, + .write_event_config = &nct720x_write_event_config, + .read_event_value = &nct720x_read_event_value, + .write_event_value = &nct720x_write_event_value,
drivers/iio/adc/nct720x.c line 468
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+}; + +static const struct i2c_device_id nct720x_id[]; + +static int nct720x_init_chip (struct nct720x_chip_info *chip) +{ +int value =0 ; +int err =0 ; + +/* Initial reset */
Maybe ignore datasheet naming and call that CONFIGURATION_INIT CONFIGURATION_RESET at which point he comment is unneeded.
drivers/iio/adc/nct720x.c line 476
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+ err =nct720x_write_reg (chip,REG_CONFIGURATION ,CONFIGURATION_INIT ); +if (err) { +pr_err ("Failed to write REG_CONFIGURATION\n" ); +return err; + } + +/* Enable Channel */ + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_1 ,0xff );
What does 0xFF represent? I guess all channels. If so maybe need to build with GENMASK so it is obvious this is enabling 8 channels.
drivers/iio/adc/nct720x.c line 482
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_1\n" ); +return err; + } + +if (chip->type == nct7202) {
Make this 'data' in the chip_info structure. Probably a flat to say there is a REG_CHANNEL_ENABLE_2. That chip->type wants to go away infavour of chip->chip_info.X flags.
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+ err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_2 ,0xf ); +if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_2\n" ); +return err;
drivers/iio/adc/nct720x.c line 496
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ } + } + + value =nct720x_read_reg16 (chip,REG_CHANNEL_ENABLE_1 ); +if (value <0 ) +return value; + chip->vin_mask = value; + +/* Start monitoring if needed */ + value =nct720x_read_reg (chip,REG_CONFIGURATION );
Using regmap would let you simple do a single bit write in one call Definitely look at whether there is anything stopping you using that helpful infrastructure.
drivers/iio/adc/nct720x.c line 505
Quoted 9 lines
drivers/iio/adc/nct720x.c new file +9
-0+if (value <0 ) { +pr_err ("Failed to read REG_CONFIGURATION\n" ); +return value; + } + + value |=CONFIGURATION_START ; + err =nct720x_write_reg (chip,REG_CONFIGURATION , value); +if (err <0 ) { +pr_err ("Failed to write REG_CONFIGURATION\n" );
dev_err() if not called only from probe, return dev_err_probe() if only called from probe()
Quoted 13 lines
drivers/iio/adc/nct720x.c new file +13
-0+return err; + } + +return 0 ; +} + +static int nct720x_probe (struct i2c_client *client) +{ +const struct i2c_device_id *id =i2c_client_get_device_id (client); +struct nct720x_chip_info *chip; +struct iio_dev *indio_dev; +int ret; +u32 tmp;
drivers/iio/adc/nct720x.c line 528
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*chip)); +if (!indio_dev) +return -ENOMEM ; + chip =iio_priv (indio_dev); + +if (client->dev.of_node) + chip->type = (enum nct720x_chips )device_get_match_data (&client->dev); +else + chip->type =i2c_match_id (nct720x_id, client)->driver_data;
i2c_get_match_data() but careful with zeros... It is much better to use that with an actual pointer.
drivers/iio/adc/nct720x.c line 532
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+ + chip->vin_max = (chip->type == nct7201) ?NCT7201_VIN_MAX :NCT7202_VIN_MAX ; + + ret =of_property_read_u32 (client->dev.of_node,"read-vin-data-size" , &tmp);
As in dt binding. If we keep this (I'm doubtful that it makes sense) define a default so that the property doesn't need to be provided. 16 would be the most obvious choice. Then just don't check the error value when reading the property. That is.
tmp = 16; of_property_read_u32();
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+if (ret <0 ) { +pr_err ("read-vin-data-size property not found\n" ); +return ret; + } +
drivers/iio/adc/nct720x.c line 547
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+if (tmp ==8 ) { + chip->use_read_byte_vin = true; + }else if (tmp ==16 ) { + chip->use_read_byte_vin = false; + }else { +pr_err ("invalid read-vin-data-size (%d)\n" , tmp); +return -EINVAL ; + } + +mutex_init (&chip->access_lock);
For new code prefer
ret = devm_mutex_init() if (ret) return ret;
It only helps in weird debug cases but also costs us very little to support those.
drivers/iio/adc/nct720x.c line 550
Quoted 3 lines
drivers/iio/adc/nct720x.c new file +3
-0+ +/* this is only used for device removal purposes */ +i2c_set_clientdata (client, indio_dev);
Won't be needed after the change below.
drivers/iio/adc/nct720x.c line 558
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+ + chip->client = client; + + ret =nct720x_init_chip (chip); +if (ret <0 ) +return ret; + + indio_dev->name = id->name;
Put the name in the chip_info structure. id->name is a complex thing when other firmwares come into play, so better to just hard code the strings somewhere so we always know what we are getting.
drivers/iio/adc/nct720x.c line 564
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ indio_dev->channels = nct720x_channels; + indio_dev->num_channels =ARRAY_SIZE (nct720x_channels); + indio_dev->info = &nct720x_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + +iio_device_register (indio_dev);
return devm_iio_device_register();
Then you can drop the remove callback.
drivers/iio/adc/nct720x.c line 569
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ +return 0 ; +} + +static void nct720x_remove (struct i2c_client *client)
Don't use wildcards even within the driver. Just name everything after one supported part.nct7202_remove() etc.
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+{ +struct iio_dev *indio_dev =i2c_get_clientdata (client); + +iio_device_unregister (indio_dev);
drivers/iio/adc/nct720x.c line 583
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+} + +static const unsigned short nct720x_address_list[] = { +0x1d ,0x1e ,0x35 ,0x36 ,I2C_CLIENT_END +}; + +static const struct i2c_device_id nct720x_id[] = { + {"nct7201" , nct7201 }, + {"nct7202" , nct7202 }, + {}
{ }I'm trying to slowly standardise on this formatting in IIO so keen not to introduce more cases. It's an arbitrary choice but I went with the space.
drivers/iio/adc/nct720x.c line 594
Quoted 11 lines
drivers/iio/adc/nct720x.c new file +11
-0+}; +MODULE_DEVICE_TABLE (i2c, nct720x_id); + +static const struct of_device_id nct720x_of_match[] = { + { + .compatible ="nuvoton,nct7201" , + .data = (void *)nct7201 + }, + { + .compatible ="nuvoton,nct7202" , + .data = (void *)nct7202
Use a pointer to a chip_info structure with all the chip specific stuff encoded as data. That is both more extensible and removes some ambiguities around whether zero is an error or not.
drivers/iio/adc/nct720x.c
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+ }, + { },
No trailing comma
drivers/iio/adc/nct720x.c line 609
Quoted 13 lines
drivers/iio/adc/nct720x.c new file +13
-0+}; + +MODULE_DEVICE_TABLE (of, nct720x_of_match); + +static struct i2c_driver nct720x_driver = { + .driver = { + .name ="nct720x" , + .of_match_table = nct720x_of_match, + }, + .probe = nct720x_probe, + .remove = nct720x_remove, + .id_table = nct720x_id, + .detect = nct720x_detect,
Do you need detect? That's kind of ancient infrastructure that I thought no one used any more (same for the address list).
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+ .address_list = nct720x_address_list, +}; + +module_i2c_driver (nct720x_driver); + +MODULE_AUTHOR ("Eason Yang <[email protected]>" ); +MODULE_DESCRIPTION ("Nuvoton NCT720x voltage monitor driver" ); +MODULE_LICENSE ("GPL" );
Dear Jonathan Cameron,
Thank you for your comment.
Jonathan Cameron <[email protected]> 於 2024年11月9日 週六 下午10:37寫道:
Quoted 25 lines
Quoted 11 lines · On Wed, 6 Nov 2024 10:39:16 +0800 Eason Yang <[email protected]> wrote:
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver
NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition.
Currently, only single-edge mode conversion and threshold events support.
Signed-off-by Eason Yang <[email protected]>You've gotten some good review already so I may well repeat stuff plus will only take a fairly superficial look at this version.
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +0
-0
Quoted 8 lines
drivers/iio/adc/nct720x.c line 73
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+ +/* List of supported devices */ +enum nct720x_chips { + nct7201, nct7202This should be replaced by chip specific information structures. That ends up a lot more extensible than using an enum and fixing up what happens in all code paths on a per enum value basis.
We would pass a pointer to the data structure not id to describe the particular variant.
Quoted 23 lines
Quoted 23 lines
drivers/iio/adc/nct720x.c new file +22
-0+}; + +struct nct720x_chip_info { +struct i2c_client *client; +enum nct720x_chips type; +struct mutex access_lock;/* for multi-byte read and write operations */ +int vin_max;/* number of VIN channels */ +u32 vin_mask; +bool use_read_byte_vin; +}; +#define NCT720X_VOLTAGE_CHANNEL (chan, addr) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \ + .address = addr, \ + .event_spec = nct720x_events, \ + .num_event_specs = ARRAY_SIZE(nct720x_events), \ + } + +#define NCT720X_VOLTAGE_CHANNEL_DIFF (chan1, chan2, addr) \
Quoted 8 lines
drivers/iio/adc/nct720x.c line 117
Quoted 7 lines
drivers/iio/adc/nct720x.c new file +7
-0+ { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = (chan1), \ + .channel2 = (chan2), \ + .differential = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \See below - should be
_RAWand_SCALEnot_PROCESSED.
We would use _RAW and _SCALE to instead of _PROCESSED.
Quoted 35 lines
Quoted 35 lines
drivers/iio/adc/nct720x.c new file +35
-0+ .address = addr, \ + .event_spec = nct720x_events, \ + .num_event_specs = ARRAY_SIZE(nct720x_events), \ + } + +static const struct iio_chan_spec nct720x_channels[] = { +NCT720X_VOLTAGE_CHANNEL (1 ,1 ), +NCT720X_VOLTAGE_CHANNEL (2 ,2 ), +NCT720X_VOLTAGE_CHANNEL (3 ,3 ), +NCT720X_VOLTAGE_CHANNEL (4 ,4 ), +NCT720X_VOLTAGE_CHANNEL (5 ,5 ), +NCT720X_VOLTAGE_CHANNEL (6 ,6 ), +NCT720X_VOLTAGE_CHANNEL (7 ,7 ), +NCT720X_VOLTAGE_CHANNEL (8 ,8 ), +NCT720X_VOLTAGE_CHANNEL (9 ,9 ), +NCT720X_VOLTAGE_CHANNEL (10 ,10 ), +NCT720X_VOLTAGE_CHANNEL (11 ,11 ), +NCT720X_VOLTAGE_CHANNEL (12 ,12 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (1 ,2 ,1 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (3 ,4 ,3 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (5 ,6 ,5 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (7 ,8 ,7 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (9 ,10 ,9 ), +NCT720X_VOLTAGE_CHANNEL_DIFF (11 ,12 ,11 ), +}; + +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */ +static int nct720x_read_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; + +return i2c_smbus_read_byte_data (client, reg); +} + +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */
Quoted 9 lines
drivers/iio/adc/nct720x.c line 157
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+static int nct720x_read_word_swapped_reg (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; + +return i2c_smbus_read_word_swapped (client, reg);Don't provide these wrappers as they don't add anything useful. Make the calls directly inline.
We would remove this function and use regmap to instead of i2c API.
Quoted 5 lines
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+} + +/* +* Read 2-byte register. Returns register in big-endian format or +* -ERRNO on error.
Quoted 9 lines
drivers/iio/adc/nct720x.c line 169
Quoted 7 lines
+ */
drivers/iio/adc/nct720x.c new file +6
-0+static int nct720x_read_reg16 (struct nct720x_chip_info *chip,u8 reg) +{ +struct i2c_client *client = chip->client; +int ret, low; + +mutex_lock (&chip->access_lock);
guard()
Got it.
Quoted 6 lines
drivers/iio/adc/nct720x.c line 175
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ ret =i2c_smbus_read_byte_data (client, reg); +if (ret >=0 ) { + low = ret; + ret =i2c_smbus_read_byte_data (client, reg +1 ); +if (ret >=0 ) + ret = low | (ret <<8 );
Quoted 6 lines
if (ret < 0) return ret; reg = get_unaligned_le16()on an appropriate u8 data[2]; ideally filled by a bulk regmap read.
We would use regmap read.
Quoted 17 lines
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +4
-0+ } + +mutex_unlock (&chip->access_lock); +return ret;drivers/iio/adc/nct720x.c line 189
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+} + +/* Write 1-byte register. Returns 0 or -ERRNO on error. */ +static int nct720x_write_reg (struct nct720x_chip_info *chip,u8 reg,u8 val) +{ +struct i2c_client *client = chip->client; +int err; + + err =i2c_smbus_write_byte_data (client, reg, val); +/* wait for write command to be finished */ If this is needed, provide a datasheet reference. It is very ususual to see a significant delay needed.
Chip RD do the simulation, we need delay 25 msec in reset register for registers ready to access.
And I2c write is not needed to delay.
These information would add into datasheet afterwards.
Quoted 23 lines
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+mdelay (10 ); + +return err; +} + +static int nct720x_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask) +{ +int index = nct720x_chan_to_index[chan->address];drivers/iio/adc/nct720x.c line 209
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+int v1, v2, volt, err; +struct nct720x_chip_info *chip =iio_priv (indio_dev); + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +switch (mask) { +case IIO_CHAN_INFO_PROCESSED : +mutex_lock (&chip->access_lock); +if (chip->use_read_byte_vin) {Ah. So resolution doesn't change, you are controlling the i2c acceses?
Quoted 5 lines
That should come from the i2c controller capabilities, not DT for this device.
The use_read_byte_vin was discussed in dt-binding commit, We would use read word first and remove read-vin-data-size property.
Quoted 20 lines
Quoted 20 lines
drivers/iio/adc/nct720x.c new file +20
-0+/* +* MNTVIN Low Byte together with MNTVIN High Byte +* forms the 13-bit count value. If MNTVIN High +* Byte readout is read successively, the +* NCT7201/NCT7202 will latch the MNTVIN Low Byte +* for next read. +*/ + v1 =nct720x_read_reg (chip,REG_VIN [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VOLT_LOW_BYTE ); +if (v2 <0 ) { + err = v2; + goto abort; + } + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else {
Quoted 8 lines
drivers/iio/adc/nct720x.c line 234
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip,REG_VIN [index]); + } + +/* Voltage(V) = 13bitCountValue * 0.0004995 */ Present this as
_RAWand provide_SCALEto userspace to be able to do this maths. Very unusual for an ADC driver to provided processed channels. Normally only occurs it there is something non linear going on.
Yes, we would use _RAW and _SCALE to instead of _PROCESSED.
Quoted 53 lines
Quoted 20 lines
drivers/iio/adc/nct720x.c new file +18
-0+ volt = (volt >>3 ) *NCT720X_IN_SCALING ; + *val = volt /10000 ; +mutex_unlock (&chip->access_lock); +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_read_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info, +int *val,int *val2)drivers/iio/adc/nct720x.c line 262
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+{ +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int v1, v2, err; +int volt =0 ; +int index = nct720x_chan_to_index[chan->address]; + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (info ==IIO_EV_INFO_VALUE ) {As below - flip logic.
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+if (dir ==IIO_EV_DIR_FALLING ) { +if (chip->use_read_byte_vin) { +/* +* Low limit VIN Low Byte together with Low limit VIN High Byte +forms the 13-bit count value +*/ +mutex_lock (&chip->access_lock); + v1 =nct720x_read_reg (chip,REG_VIN_LOW_LIMIT [index]); +if (v1 <0 ) { + err = v1;drivers/iio/adc/nct720x.c line 282
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VIN_LOW_LIMIT_LSB [index]); +if (v2 <0 ) { + err = v2; + goto abort; + } +mutex_unlock (&chip->access_lock); + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */
Quoted 3 lines
rather see this as a
get_unaligned_le16on an array of u8.
In some cases that ends up quite a bit cheaper and it also documents what is going on.
We would use get_unaligned_le16 to convert the array of u8.
Quoted 33 lines
Quoted 33 lines
drivers/iio/adc/nct720x.c new file +33
-0+ }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip, +REG_VIN_LOW_LIMIT [index]); + } + }else { +if (chip->use_read_byte_vin) { +/* +* High limit VIN Low Byte together with high limit VIN High Byte +* forms the 13-bit count value +*/ +mutex_lock (&chip->access_lock); + v1 =nct720x_read_reg (chip,REG_VIN_HIGH_LIMIT [index]); +if (v1 <0 ) { + err = v1; + goto abort; + } + + v2 =nct720x_read_reg (chip,REG_VIN_HIGH_LIMIT_LSB [index]); +if (v2 <0 ) { + err = v2; + goto abort; + } +mutex_unlock (&chip->access_lock); + volt = (v1 <<8 ) | v2;/* Convert back to 16-bit value */ + }else { +/* NCT7201/NCT7202 also supports read word-size data */ + volt =nct720x_read_word_swapped_reg (chip, +REG_VIN_HIGH_LIMIT [index]); + } + } + } +/* Voltage(V) = 13bitCountValue * 0.0004995 */
Quoted 9 lines
drivers/iio/adc/nct720x.c line 321
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ volt = (volt >>3 ) *NCT720X_IN_SCALING ; + *val = volt /10000 ; + +return IIO_VAL_INT ; +abort: +mutex_unlock (&chip->access_lock);
guard()in appropriate places.
Again, the lock and unlock should ideally be in same scope.
We would put guard(mutex) in appropriate places.
Quoted 11 lines
Quoted 11 lines
drivers/iio/adc/nct720x.c new file +11
-0+return err; +} + +static int nct720x_write_event_value (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +enum iio_event_info info, +int val,int val2) +{ +struct nct720x_chip_info *chip =iio_priv (indio_dev);
Quoted 9 lines
drivers/iio/adc/nct720x.c line 339
Quoted 7 lines
drivers/iio/adc/nct720x.c new file +7
-0+int index, err =0 ; +long v1, v2, volt; + + index = nct720x_chan_to_index[chan->address]; + volt = (val *10000 ) /NCT720X_IN_SCALING ; + v1 = volt >>5 ; + v2 = (volt &0x1f ) <<3 ;Some explanatory comments for this would be good.
/* Voltage(V) = 13bitCountValue * 0.0004995 */
volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING; v1 = volt >> 5;
v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
Quoted 11 lines
drivers/iio/adc/nct720x.c line 262
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ; + +if (info ==IIO_EV_INFO_VALUE ) {Flip logic.
if (info != IIO_EV_INFO_VALUE) return -EINVAL; if (dir == IIO_EVE_DIR_FALLING) etc
Quoted 6 lines
drivers/iio/adc/nct720x.c
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+if (dir ==IIO_EV_DIR_FALLING ) { +mutex_lock (&chip->access_lock);This is badly nested. Mutex lock and unlock should be in same scope. So I'd pull
mutex_lock()out of this if stack and use guard(mutex) instead. That way you can just return on error.
We would put guard(mutex) in appropriate places.
Quoted 47 lines
Quoted 47 lines
drivers/iio/adc/nct720x.c new file +46
-0+ err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_LOW_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n" , index +1 ); + goto abort; + } + }else { +mutex_lock (&chip->access_lock); + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT [index], v1); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT\n" , index +1 ); + goto abort; + } + + err =nct720x_write_reg (chip,REG_VIN_HIGH_LIMIT_LSB [index], v2); +if (err <0 ) { +pr_err ("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n" , index +1 ); + goto abort; + } + } + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_write_event_config (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +enum iio_event_type type, +enum iio_event_direction dir, +int state) +{ +int err =0 ; +struct nct720x_chip_info *chip =iio_priv (indio_dev); +int index = nct720x_chan_to_index[chan->address]; +unsigned int mask; + + mask =BIT (index); + +if (chan->type !=IIO_VOLTAGE ) +return -EOPNOTSUPP ;
Quoted 9 lines
drivers/iio/adc/nct720x.c line 413
Quoted 7 lines
drivers/iio/adc/nct720x.c new file +7
-0+ +if (!state && (chip->vin_mask & mask)) + chip->vin_mask &= ~mask; +else if (state && !(chip->vin_mask & mask)) + chip->vin_mask |= mask; + +mutex_lock (&chip->access_lock);guard(mutex)(&chip->access_lock);
We would put guard(mutex) in appropriate places.
Quoted 5 lines
drivers/iio/adc/nct720x.c line 418
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_1 , chip->vin_mask &0xff ); +if (err <0 ) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_1\n" ); + goto abort;
Quoted 9 lines
dev_err() return err;drivers/iio/adc/nct720x.c line 421
Quoted 3 lines
drivers/iio/adc/nct720x.c new file +3
-0+ } + +if (chip->type == nct7202) {Gain, base this on a
chip_infoflag.
We would pass a pointer to the data structure and not use chip->type anymore.
Quoted 5 lines
drivers/iio/adc/nct720x.c line 425
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+ err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_2 , chip->vin_mask >>8 ); +if (err <0 ) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_2\n" ); + goto abort;Same as above.
Use dev_err
Quoted 17 lines
Quoted 17 lines
drivers/iio/adc/nct720x.c new file +17
-0+ } + } +abort: +mutex_unlock (&chip->access_lock); +return err; +} + +static int nct720x_detect (struct i2c_client *client, +struct i2c_board_info *info) +{ +struct i2c_adapter *adapter = client->adapter; + +if (!i2c_check_functionality (adapter,I2C_FUNC_SMBUS_BYTE_DATA | +I2C_FUNC_SMBUS_WORD_DATA )) +return -ENODEV ; + +/* Determine the chip type. */
Quoted 8 lines
drivers/iio/adc/nct720x.c line 448
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+if (i2c_smbus_read_byte_data (client,REG_VENDOR_ID ) !=NUVOTON_ID || +i2c_smbus_read_byte_data (client,REG_CHIP_ID ) !=NCT720X_ID || +i2c_smbus_read_byte_data (client,REG_DEVICE_ID ) !=NCT720X_DEVICE_ID ) +return -ENODEV ; + +strscpy (info->type,"nct720x" ,I2C_NAME_SIZE );as below. It's unusual to find a detect in an IIO driver because the firmware normally tells us what is there.
We would pass a pointer to the data structure to obtain the name and not use chip->type anymore.
Quoted 14 lines
Quoted 14 lines
drivers/iio/adc/nct720x.c new file +14
-0+ +return 0 ; +} + +static const struct iio_info nct720x_info = { + .read_raw = &nct720x_read_raw, + .read_event_config = &nct720x_read_event_config, + .write_event_config = &nct720x_write_event_config, + .read_event_value = &nct720x_read_event_value, + .write_event_value = &nct720x_write_event_value, +}; + +static const struct i2c_device_id nct720x_id[]; +
Quoted 9 lines
drivers/iio/adc/nct720x.c line 468
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+static int nct720x_init_chip (struct nct720x_chip_info *chip) +{ +int value =0 ; +int err =0 ; + +/* Initial reset */ Maybe ignore datasheet naming and call that
CONFIGURATION_INITCONFIGURATION_RESETat which point he comment is unneeded.
Understand it.
Quoted 11 lines
drivers/iio/adc/nct720x.c line 476
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+ err =nct720x_write_reg (chip,REG_CONFIGURATION ,CONFIGURATION_INIT ); +if (err) { +pr_err ("Failed to write REG_CONFIGURATION\n" ); +return err; + } + +/* Enable Channel */ + err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_1 ,0xff );What does
0xFFrepresent? I guess all channels. If so maybe need to build with GENMASK so it is obvious this is enabling 8 channels.
Use GENMASK(7, 0) and declare it.
#define REG_CHANNEL_ENABLE_1_MASK GENMASK(7, 0)
Quoted 7 lines
drivers/iio/adc/nct720x.c line 482
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_1\n" ); +return err; + } + +if (chip->type == nct7202) {
Quoted 5 lines
Make this 'data' in the
chip_infostructure. Probably a flat to say there is aREG_CHANNEL_ENABLE_2. Thatchip->typewants to go away infavour ofchip->chip_info.Xflags.
We would pass a pointer to the data structure to describe the particular variant.
Quoted 10 lines
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+ err =nct720x_write_reg (chip,REG_CHANNEL_ENABLE_2 ,0xf ); +if (err) { +pr_err ("Failed to write REG_CHANNEL_ENABLE_2\n" ); +return err; + } + } + + value =nct720x_read_reg16 (chip,REG_CHANNEL_ENABLE_1 ); +if (value <0 ) +return value;
Quoted 9 lines
drivers/iio/adc/nct720x.c line 496
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+ chip->vin_mask = value; + +/* Start monitoring if needed */ + value =nct720x_read_reg (chip,REG_CONFIGURATION );Using regmap would let you simple do a single bit write in one call Definitely look at whether there is anything stopping you using that helpful infrastructure.
We take some effort to convert regmap API to instead of I2C API
Quoted 10 lines
drivers/iio/adc/nct720x.c line 505
Quoted 9 lines
drivers/iio/adc/nct720x.c new file +9
-0+if (value <0 ) { +pr_err ("Failed to read REG_CONFIGURATION\n" ); +return value; + } + + value |=CONFIGURATION_START ; + err =nct720x_write_reg (chip,REG_CONFIGURATION , value); +if (err <0 ) { +pr_err ("Failed to write REG_CONFIGURATION\n" );
Quoted 4 lines
dev_err()if not called only from probe, returndev_err_probe()if only called fromprobe()
Yes, use dev_err_probe() in probe function.
Quoted 18 lines
Quoted 18 lines
drivers/iio/adc/nct720x.c new file +18
-0+return err; + } + +return 0 ; +} + +static int nct720x_probe (struct i2c_client *client) +{ +const struct i2c_device_id *id =i2c_client_get_device_id (client); +struct nct720x_chip_info *chip; +struct iio_dev *indio_dev; +int ret; +u32 tmp; + + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*chip)); +if (!indio_dev) +return -ENOMEM ; + chip =iio_priv (indio_dev);
Quoted 10 lines
drivers/iio/adc/nct720x.c line 528
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ +if (client->dev.of_node) + chip->type = (enum nct720x_chips )device_get_match_data (&client->dev); +else + chip->type =i2c_match_id (nct720x_id, client)->driver_data;
i2c_get_match_data()but careful with zeros... It is much better to use that with an actual pointer.
We would pass a pointer to the data structure and i2c_get_match_data() to get info
Quoted 8 lines
drivers/iio/adc/nct720x.c line 532
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+ + chip->vin_max = (chip->type == nct7201) ?NCT7201_VIN_MAX :NCT7202_VIN_MAX ; + + ret =of_property_read_u32 (client->dev.of_node,"read-vin-data-size" , &tmp);As in dt binding. If we keep this (I'm doubtful that it makes sense) define a default so that the property doesn't need to be provided. 16 would be the most obvious choice. Then just don't check the error value when reading the property. That is.
Quoted 5 lines
tmp = 16;
of_property_read_u32();We already remove read-vin-data-size property and use word read for VIN data.
Quoted 11 lines
Quoted 11 lines
drivers/iio/adc/nct720x.c new file +11
-0+if (ret <0 ) { +pr_err ("read-vin-data-size property not found\n" ); +return ret; + } + +if (tmp ==8 ) { + chip->use_read_byte_vin = true; + }else if (tmp ==16 ) { + chip->use_read_byte_vin = false; + }else { +pr_err ("invalid read-vin-data-size (%d)\n" , tmp);
Quoted 9 lines
drivers/iio/adc/nct720x.c line 547
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+return -EINVAL ; + } + +mutex_init (&chip->access_lock);For new code prefer
ret =devm_mutex_init()
if (ret)
return ret;
Understand it.
Quoted 6 lines
It only helps in weird debug cases but also costs us very little to support those.
drivers/iio/adc/nct720x.c line 550
Quoted 3 lines
drivers/iio/adc/nct720x.c new file +3
-0+ +/* this is only used for device removal purposes */ +i2c_set_clientdata (client, indio_dev);Won't be needed after the change below.
Understand it.
Quoted 12 lines
drivers/iio/adc/nct720x.c line 558
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+ + chip->client = client; + + ret =nct720x_init_chip (chip); +if (ret <0 ) +return ret; + + indio_dev->name = id->name;Put the name in the
chip_infostructure.id->nameis a complex thing when other firmwares come into play, so better to just hard code the strings somewhere so we always know what we are getting.
We would pass a pointer to the data structure to obtain the name
Quoted 10 lines
drivers/iio/adc/nct720x.c line 564
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+ indio_dev->channels = nct720x_channels; + indio_dev->num_channels =ARRAY_SIZE (nct720x_channels); + indio_dev->info = &nct720x_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + +iio_device_register (indio_dev);return devm_iio_device_register();Then you can drop the remove callback.
Understand it.
Quoted 11 lines
drivers/iio/adc/nct720x.c line 569
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ +return 0 ; +} + +static void nct720x_remove (struct i2c_client *client)Don't use wildcards even within the driver. Just name everything after one supported part.
nct7202_remove()etc.
Remove the nct7202_remove()
Quoted 16 lines
Quoted 4 lines
drivers/iio/adc/nct720x.c new file +4
-0+{ +struct iio_dev *indio_dev =i2c_get_clientdata (client); + +iio_device_unregister (indio_dev);drivers/iio/adc/nct720x.c line 583
Quoted 10 lines
drivers/iio/adc/nct720x.c new file +10
-0+} + +static const unsigned short nct720x_address_list[] = { +0x1d ,0x1e ,0x35 ,0x36 ,I2C_CLIENT_END +}; + +static const struct i2c_device_id nct720x_id[] = { + {"nct7201" , nct7201 }, + {"nct7202" , nct7202 }, + {}{ }
Quoted 4 lines
I'm trying to slowly standardise on this formatting in IIO so keen not to introduce more cases. It's an arbitrary choice but I went with the space.
Rewite the data structure as below,
static const struct nct720x_adc_model_data nct7201_model_data = { .model_name = "nct7201",
.channels = nct7201_channels,
.num_channels = ARRAY_SIZE(nct7201_channels),
.vin_max = 8,
};
static const struct nct720x_adc_model_data nct7202_model_data = { .model_name = "nct7202",
.channels = nct7202_channels,
.num_channels = ARRAY_SIZE(nct7202_channels),
.vin_max = 12,
};
static const struct i2c_device_id nct720x_id[] = { { "nct7201", (kernel_ulong_t)&nct7201_model_data }, { "nct7202", (kernel_ulong_t)&nct7202_model_data }, { }
};
Quoted 6 lines
Quoted 6 lines
drivers/iio/adc/nct720x.c new file +6
-0+}; +MODULE_DEVICE_TABLE (i2c, nct720x_id); + +static const struct of_device_id nct720x_of_match[] = { + { + .compatible ="nuvoton,nct7201" ,
Quoted 9 lines
drivers/iio/adc/nct720x.c line 594
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ .data = (void *)nct7201 + }, + { + .compatible ="nuvoton,nct7202" , + .data = (void *)nct7202Use a pointer to a
chip_infostructure with all the chip specific stuff encoded as data. That is both more extensible and removes some ambiguities around whether zero is an error or not.
Rewrite as below,
static const struct of_device_id nct720x_of_match[] = { {
.compatible = "nuvoton,nct7201",
.data = &nct7201_model_data,
},
{
.compatible = "nuvoton,nct7202",
.data = &nct7202_model_data,
},
{ }
};
Quoted 3 lines
drivers/iio/adc/nct720x.c
Quoted 2 lines
drivers/iio/adc/nct720x.c new file +2
-0+ }, + { },No trailing comma
Understand it.
Quoted 8 lines
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+}; + +MODULE_DEVICE_TABLE (of, nct720x_of_match); + +static struct i2c_driver nct720x_driver = { + .driver = { + .name ="nct720x" , + .of_match_table = nct720x_of_match,
Quoted 9 lines
drivers/iio/adc/nct720x.c line 609
Quoted 5 lines
drivers/iio/adc/nct720x.c new file +5
-0+ }, + .probe = nct720x_probe, + .remove = nct720x_remove, + .id_table = nct720x_id, + .detect = nct720x_detect,Do you need detect? That's kind of ancient infrastructure that I thought no one used any more (same for the address list).
We would remove detect and address list.
Quoted 9 lines
Quoted 8 lines
drivers/iio/adc/nct720x.c new file +8
-0+ .address_list = nct720x_address_list, +}; + +module_i2c_driver (nct720x_driver); + +MODULE_AUTHOR ("Eason Yang <[email protected]>" ); +MODULE_DESCRIPTION ("Nuvoton NCT720x voltage monitor driver" ); +MODULE_LICENSE ("GPL" );