[PATCH v4 0/3] hwmon: Driver for Nuvoton NCT7363Y

[PATCH v4 0/3] hwmon: Driver for Nuvoton NCT7363Y

[baneric926]

From: Ban Feng <kcfeng0@nuvoton.com>

NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.

Changes since version 3:
- Cherry-pick the fan-common.yaml in [1]
- Fix "checkpatch --strict" report
- Replace BIT_CHECK() with BIT()
- Fix CamelCase defines or variables
- Drop enum chips
- Drop all local caching and just read values through regmap
- Drop chip auto-detection since it increases boot time

[1]: https://patchwork.kernel.org/project/linux-hwmon/patch/
     20240221104025.1306227-2-billy_tsai@aspeedtech.com/

Changes since version 2:
- Cherry-pick the fan-common.yaml in [1]
- Fix nct736x typo and add unevaluatedProperties

[1]: https://patchwork.kernel.org/project/linux-hwmon/patch/
     20231107105025.1480561-2-billy_tsai@aspeedtech.com/

Changes since version 1:
- Modify NCT736X(nct736x) to NCT7363Y(nct7363)
- Convert to devm_hwmon_device_register_with_info API
- All ID tables are next to each other and should be consistent
  between i2c_device_id and of_device_id
- Ref. fan-common.yaml and modify properties (nuvoton,pwm-mask/
  nuvoton,fanin-mask) to (pwms/tach-ch)
- Convert to devm_regmap_init_i2c API
- Remove unused function (watchdog timer)
- Fix uninitialized symbol which is reported by kernel test robot

Ban Feng (2):
  dt-bindings: hwmon: Add NCT7363Y documentation
  hwmon: Driver for Nuvoton NCT7363Y

Naresh Solanki (1):
  dt-bindings: hwmon: fan: Add fan binding to schema

 .../devicetree/bindings/hwmon/fan-common.yaml |  78 ++++
 .../bindings/hwmon/nuvoton,nct7363.yaml       |  63 +++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/nct7363.rst               |  33 ++
 MAINTAINERS                                   |   8 +
 drivers/hwmon/Kconfig                         |  11 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/nct7363.c                       | 412 ++++++++++++++++++
 8 files changed, 607 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml
 create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
 create mode 100644 Documentation/hwmon/nct7363.rst
 create mode 100644 drivers/hwmon/nct7363.c

-- 
2.34.1

[PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[baneric926]

From: Naresh Solanki <naresh.solanki@9elements.com>

Add common fan properties bindings to a schema.

Bindings for fan controllers can reference the common schema for the
fan

child nodes:

  patternProperties:
    "^fan@[0-2]":
      type: object
      $ref: fan-common.yaml#
      unevaluatedProperties: false

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
---
 .../devicetree/bindings/hwmon/fan-common.yaml | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/fan-common.yaml b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
new file mode 100644
index 000000000000..15c591c74545
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/fan-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common Fan Properties
+
+maintainers:
+  - Naresh Solanki <naresh.solanki@9elements.com>
+  - Billy Tsai <billy_tsai@aspeedtech.com>
+
+properties:
+  max-rpm:
+    description:
+      Max RPM supported by fan.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maximum: 100000
+
+  min-rpm:
+    description:
+      Min RPM supported by fan.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maximum: 1000
+
+  pulses-per-revolution:
+    description:
+      The number of pulse from fan sensor per revolution.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    maximum: 4
+
+  tach-div:
+    description:
+      Divisor for the tach sampling clock, which determines the sensitivity of the tach pin.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  target-rpm:
+    description:
+      The default desired fan speed in RPM.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  fan-driving-mode:
+    description:
+      Select the driving mode of the fan.(DC, PWM and so on)
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ dc, pwm ]
+
+  pwms:
+    description:
+      PWM provider.
+    maxItems: 1
+
+  "#cooling-cells":
+    const: 2
+
+  cooling-levels:
+    description:
+      The control value which correspond to thermal cooling states.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+
+  tach-ch:
+    description:
+      The tach channel used for the fan.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+
+  label:
+    description:
+      Optional fan label
+
+  fan-supply:
+    description:
+      Power supply for fan.
+
+  reg:
+    maxItems: 1
+
+additionalProperties: true
+
-- 
2.34.1

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Zev Weiss]

On Mon, Feb 26, 2024 at 04:56:04PM PST, baneric926@gmail.com wrote:
>From: Naresh Solanki <naresh.solanki@9elements.com>
>
>Add common fan properties bindings to a schema.
>
>Bindings for fan controllers can reference the common schema for the
>fan
>
>child nodes:
>
>  patternProperties:
>    "^fan@[0-2]":
>      type: object
>      $ref: fan-common.yaml#
>      unevaluatedProperties: false
>
>Reviewed-by: Rob Herring <robh@kernel.org>
>Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
>Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
>---
> .../devicetree/bindings/hwmon/fan-common.yaml | 78 +++++++++++++++++++
> 1 file changed, 78 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml
>
>diff --git a/Documentation/devicetree/bindings/hwmon/fan-common.yaml b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
>new file mode 100644
>index 000000000000..15c591c74545
>--- /dev/null
>+++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
>@@ -0,0 +1,78 @@
>+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>+%YAML 1.2
>+---
>+$id: http://devicetree.org/schemas/hwmon/fan-common.yaml#
>+$schema: http://devicetree.org/meta-schemas/core.yaml#
>+
>+title: Common Fan Properties
>+
>+maintainers:
>+  - Naresh Solanki <naresh.solanki@9elements.com>
>+  - Billy Tsai <billy_tsai@aspeedtech.com>
>+
>+properties:
>+  max-rpm:
>+    description:
>+      Max RPM supported by fan.
>+    $ref: /schemas/types.yaml#/definitions/uint32
>+    maximum: 100000
>+
>+  min-rpm:
>+    description:
>+      Min RPM supported by fan.
>+    $ref: /schemas/types.yaml#/definitions/uint32
>+    maximum: 1000

I can't say with certainty that it's not, but are we sure 1000 is low 
enough?  Looking at just what I've got on hand, an 80mm fan I have will 
run steadily at about 1500RPM, and I'd assume larger ones (e.g. 120mm) 
could potentially go significantly lower...

>+
>+  pulses-per-revolution:
>+    description:
>+      The number of pulse from fan sensor per revolution.
>+    $ref: /schemas/types.yaml#/definitions/uint32
>+    maximum: 4

Might we want 'default: 2' here?

>+
>+  tach-div:
>+    description:
>+      Divisor for the tach sampling clock, which determines the sensitivity of the tach pin.
>+    $ref: /schemas/types.yaml#/definitions/uint32
>+
>+  target-rpm:
>+    description:
>+      The default desired fan speed in RPM.
>+    $ref: /schemas/types.yaml#/definitions/uint32
>+
>+  fan-driving-mode:
>+    description:
>+      Select the driving mode of the fan.(DC, PWM and so on)

Nit: could use a space before the parenthetical.

>+    $ref: /schemas/types.yaml#/definitions/string
>+    enum: [ dc, pwm ]
>+
>+  pwms:
>+    description:
>+      PWM provider.
>+    maxItems: 1
>+
>+  "#cooling-cells":
>+    const: 2
>+
>+  cooling-levels:
>+    description:
>+      The control value which correspond to thermal cooling states.
>+    $ref: /schemas/types.yaml#/definitions/uint32-array
>+
>+  tach-ch:
>+    description:
>+      The tach channel used for the fan.
>+    $ref: /schemas/types.yaml#/definitions/uint8-array

Nit: s/channel/channels/ given that it's an array?

>+
>+  label:
>+    description:
>+      Optional fan label
>+
>+  fan-supply:
>+    description:
>+      Power supply for fan.
>+
>+  reg:
>+    maxItems: 1
>+
>+additionalProperties: true
>+
>-- 
>2.34.1
>
>

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Guenter Roeck]

On 3/4/24 16:22, Zev Weiss wrote:
> On Mon, Feb 26, 2024 at 04:56:04PM PST, baneric926@gmail.com wrote:
>> From: Naresh Solanki <naresh.solanki@9elements.com>
>>
>> Add common fan properties bindings to a schema.
>>
>> Bindings for fan controllers can reference the common schema for the
>> fan
>>
>> child nodes:
>>
>>  patternProperties:
>>    "^fan@[0-2]":
>>      type: object
>>      $ref: fan-common.yaml#
>>      unevaluatedProperties: false
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
>> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
>> ---
>> .../devicetree/bindings/hwmon/fan-common.yaml | 78 +++++++++++++++++++
>> 1 file changed, 78 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/fan-common.yaml b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
>> new file mode 100644
>> index 000000000000..15c591c74545
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
>> @@ -0,0 +1,78 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/fan-common.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Common Fan Properties
>> +
>> +maintainers:
>> +  - Naresh Solanki <naresh.solanki@9elements.com>
>> +  - Billy Tsai <billy_tsai@aspeedtech.com>
>> +
>> +properties:
>> +  max-rpm:
>> +    description:
>> +      Max RPM supported by fan.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    maximum: 100000
>> +
>> +  min-rpm:
>> +    description:
>> +      Min RPM supported by fan.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    maximum: 1000
> 
> I can't say with certainty that it's not, but are we sure 1000 is low enough?  Looking at just what I've got on hand, an 80mm fan I have will run steadily at about 1500RPM, and I'd assume larger ones (e.g. 120mm) could potentially go significantly lower...
> 

I have seen fans which run stable at < 400rpm.
One of my systems right now has:

fan1:              732 RPM  (min =    0 RPM)
fan2:                0 RPM  (min =    0 RPM)
fan3:              586 RPM  (min =    0 RPM)
fan4:              472 RPM  (min =    0 RPM)
fan5:              480 RPM  (min =    0 RPM)

Those are 80mm fans. A quick check shows that various Noctua fans have a
minimum speed of 300 rpm. So 1000 is indeed a bit high for the minimum speed.

Guenter

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Guenter Roeck]

On 3/4/24 16:41, Guenter Roeck wrote:

>>> +
>>> +  min-rpm:
>>> +    description:
>>> +      Min RPM supported by fan.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    maximum: 1000
>>
>> I can't say with certainty that it's not, but are we sure 1000 is low enough?  Looking at just what I've got on hand, an 80mm fan I have will run steadily at about 1500RPM, and I'd assume larger ones (e.g. 120mm) could potentially go significantly lower...
>>
> 
> I have seen fans which run stable at < 400rpm.
> One of my systems right now has:
> 
> fan1:              732 RPM  (min =    0 RPM)
> fan2:                0 RPM  (min =    0 RPM)
> fan3:              586 RPM  (min =    0 RPM)
> fan4:              472 RPM  (min =    0 RPM)
> fan5:              480 RPM  (min =    0 RPM)
> 
> Those are 80mm fans. A quick check shows that various Noctua fans have a
> minimum speed of 300 rpm. So 1000 is indeed a bit high for the minimum speed.
> 

No, wait, that is the _maximum_ minimum speed. Got me there.

So, there is Noctua's NF-A4x20 PWM with a minimum rotational speed of 1,200 RPM.

If I interpret
https://www.mouser.com/datasheet/2/471/SanyoDenki_San_Ace_40LG28_E-3198440.pdf
correctly, it lists some fans with a minimum speed of 7,500 RPM.

Guenter

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Ban Feng]

Hi Zev,

This patch was suggested by reviewer and I cherry-pick from below link:
https://patchwork.kernel.org/project/linux-hwmon/patch/20240221104025.1306227-2-billy_tsai@aspeedtech.com/

Because I don't know the rule about the patch with cherry-pick, maybe
we should discuss it there?

Thanks,
Ban

On Tue, Mar 5, 2024 at 8:22 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Mon, Feb 26, 2024 at 04:56:04PM PST, baneric926@gmail.com wrote:
> >From: Naresh Solanki <naresh.solanki@9elements.com>
> >
> >Add common fan properties bindings to a schema.
> >
> >Bindings for fan controllers can reference the common schema for the
> >fan
> >
> >child nodes:
> >
> >  patternProperties:
> >    "^fan@[0-2]":
> >      type: object
> >      $ref: fan-common.yaml#
> >      unevaluatedProperties: false
> >
> >Reviewed-by: Rob Herring <robh@kernel.org>
> >Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> >Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> >Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> >---
> > .../devicetree/bindings/hwmon/fan-common.yaml | 78 +++++++++++++++++++
> > 1 file changed, 78 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml
> >
> >diff --git a/Documentation/devicetree/bindings/hwmon/fan-common.yaml b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> >new file mode 100644
> >index 000000000000..15c591c74545
> >--- /dev/null
> >+++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> >@@ -0,0 +1,78 @@
> >+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >+%YAML 1.2
> >+---
> >+$id: http://devicetree.org/schemas/hwmon/fan-common.yaml#
> >+$schema: http://devicetree.org/meta-schemas/core.yaml#
> >+
> >+title: Common Fan Properties
> >+
> >+maintainers:
> >+  - Naresh Solanki <naresh.solanki@9elements.com>
> >+  - Billy Tsai <billy_tsai@aspeedtech.com>
> >+
> >+properties:
> >+  max-rpm:
> >+    description:
> >+      Max RPM supported by fan.
> >+    $ref: /schemas/types.yaml#/definitions/uint32
> >+    maximum: 100000
> >+
> >+  min-rpm:
> >+    description:
> >+      Min RPM supported by fan.
> >+    $ref: /schemas/types.yaml#/definitions/uint32
> >+    maximum: 1000
>
> I can't say with certainty that it's not, but are we sure 1000 is low
> enough?  Looking at just what I've got on hand, an 80mm fan I have will
> run steadily at about 1500RPM, and I'd assume larger ones (e.g. 120mm)
> could potentially go significantly lower...
>
> >+
> >+  pulses-per-revolution:
> >+    description:
> >+      The number of pulse from fan sensor per revolution.
> >+    $ref: /schemas/types.yaml#/definitions/uint32
> >+    maximum: 4
>
> Might we want 'default: 2' here?
>
> >+
> >+  tach-div:
> >+    description:
> >+      Divisor for the tach sampling clock, which determines the sensitivity of the tach pin.
> >+    $ref: /schemas/types.yaml#/definitions/uint32
> >+
> >+  target-rpm:
> >+    description:
> >+      The default desired fan speed in RPM.
> >+    $ref: /schemas/types.yaml#/definitions/uint32
> >+
> >+  fan-driving-mode:
> >+    description:
> >+      Select the driving mode of the fan.(DC, PWM and so on)
>
> Nit: could use a space before the parenthetical.
>
> >+    $ref: /schemas/types.yaml#/definitions/string
> >+    enum: [ dc, pwm ]
> >+
> >+  pwms:
> >+    description:
> >+      PWM provider.
> >+    maxItems: 1
> >+
> >+  "#cooling-cells":
> >+    const: 2
> >+
> >+  cooling-levels:
> >+    description:
> >+      The control value which correspond to thermal cooling states.
> >+    $ref: /schemas/types.yaml#/definitions/uint32-array
> >+
> >+  tach-ch:
> >+    description:
> >+      The tach channel used for the fan.
> >+    $ref: /schemas/types.yaml#/definitions/uint8-array
>
> Nit: s/channel/channels/ given that it's an array?
>
> >+
> >+  label:
> >+    description:
> >+      Optional fan label
> >+
> >+  fan-supply:
> >+    description:
> >+      Power supply for fan.
> >+
> >+  reg:
> >+    maxItems: 1
> >+
> >+additionalProperties: true
> >+
> >--
> >2.34.1
> >
> >

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Guenter Roeck]

On 2/26/24 16:56, baneric926@gmail.com wrote:
> From: Naresh Solanki <naresh.solanki@9elements.com>
> 
> Add common fan properties bindings to a schema.
> 
> Bindings for fan controllers can reference the common schema for the
> fan
> 
> child nodes:
> 
>    patternProperties:
>      "^fan@[0-2]":
>        type: object
>        $ref: fan-common.yaml#
>        unevaluatedProperties: false
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>

This patch (through its submission with the aspeed-g6 fan driver) is now in hwmon-next.

Please do not resend. Any updates should be submitted as follow-up patches.

Guenter

Re: [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Ban Feng]

Hi Guenter,

ok.

Thanks,
Ban

On Fri, Mar 8, 2024 at 2:53 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 2/26/24 16:56, baneric926@gmail.com wrote:
> > From: Naresh Solanki <naresh.solanki@9elements.com>
> >
> > Add common fan properties bindings to a schema.
> >
> > Bindings for fan controllers can reference the common schema for the
> > fan
> >
> > child nodes:
> >
> >    patternProperties:
> >      "^fan@[0-2]":
> >        type: object
> >        $ref: fan-common.yaml#
> >        unevaluatedProperties: false
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com>
> > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> > Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
>
> This patch (through its submission with the aspeed-g6 fan driver) is now in hwmon-next.
>
> Please do not resend. Any updates should be submitted as follow-up patches.
>
> Guenter
>

[PATCH v4 2/3] dt-bindings: hwmon: Add NCT7363Y documentation

[baneric926]

From: Ban Feng <kcfeng0@nuvoton.com>

Adding bindings for the Nuvoton NCT7363Y Fan Controller

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
---
 .../bindings/hwmon/nuvoton,nct7363.yaml       | 63 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
new file mode 100644
index 000000000000..1a9d9a5d614e
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/nuvoton,nct7363.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NCT7363Y Hardware Monitoring IC
+
+maintainers:
+  - Ban Feng <kcfeng0@nuvoton.com>
+
+description: |
+  The NCT7363Y is a Fan controller which provides up to 16 independent
+  FAN input monitors, and up to 16 independent PWM output with SMBus interface.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,nct7363
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+patternProperties:
+  "^fan-[0-9]+$":
+    $ref: fan-common.yaml#
+    unevaluatedProperties: false
+    required:
+      - pwms
+      - tach-ch
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        hwmon: hwmon@22 {
+            compatible = "nuvoton,nct7363";
+            reg = <0x22>;
+            #pwm-cells = <2>;
+
+            fan-0 {
+                pwms = <&hwmon 0 50000>;
+                tach-ch = /bits/ 8 <0x00>;
+            };
+            fan-1 {
+                pwms = <&hwmon 1 50000>;
+                tach-ch = /bits/ 8 <0x01>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 2ecaaec6a6bf..7b1efefed7c4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15084,6 +15084,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct6775.yaml
 F:	drivers/hwmon/nct6775-i2c.c
 
+NCT7363 HARDWARE MONITOR DRIVER
+M:	Ban Feng <kcfeng0@nuvoton.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
+
 NETDEVSIM
 M:	Jakub Kicinski <kuba@kernel.org>
 S:	Maintained
-- 
2.34.1

Re: [PATCH v4 2/3] dt-bindings: hwmon: Add NCT7363Y documentation

[Paul Menzel]

Dear Ban,


Thank you for your patch.


Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
> From: Ban Feng <kcfeng0@nuvoton.com>
> 
> Adding bindings for the Nuvoton NCT7363Y Fan Controller

s/Adding/Add/ or even Document bindings …

Do you have an URL to the datasheet?

> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> ---
>   .../bindings/hwmon/nuvoton,nct7363.yaml       | 63 +++++++++++++++++++
>   MAINTAINERS                                   |  6 ++
>   2 files changed, 69 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> new file mode 100644
> index 000000000000..1a9d9a5d614e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct7363.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton NCT7363Y Hardware Monitoring IC
> +
> +maintainers:
> +  - Ban Feng <kcfeng0@nuvoton.com>
> +
> +description: |
> +  The NCT7363Y is a Fan controller which provides up to 16 independent

lowecase: fan controller?

> +  FAN input monitors, and up to 16 independent PWM output with SMBus interface.

output*s*?

> +
> +properties:
> +  compatible:
> +    enum:
> +      - nuvoton,nct7363
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 2
> +
> +patternProperties:
> +  "^fan-[0-9]+$":
> +    $ref: fan-common.yaml#
> +    unevaluatedProperties: false
> +    required:
> +      - pwms
> +      - tach-ch
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#pwm-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        hwmon: hwmon@22 {
> +            compatible = "nuvoton,nct7363";
> +            reg = <0x22>;
> +            #pwm-cells = <2>;
> +
> +            fan-0 {
> +                pwms = <&hwmon 0 50000>;
> +                tach-ch = /bits/ 8 <0x00>;
> +            };
> +            fan-1 {
> +                pwms = <&hwmon 1 50000>;
> +                tach-ch = /bits/ 8 <0x01>;
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2ecaaec6a6bf..7b1efefed7c4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15084,6 +15084,12 @@ S:	Maintained
>   F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct6775.yaml
>   F:	drivers/hwmon/nct6775-i2c.c
>   
> +NCT7363 HARDWARE MONITOR DRIVER
> +M:	Ban Feng <kcfeng0@nuvoton.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> +
>   NETDEVSIM
>   M:	Jakub Kicinski <kuba@kernel.org>
>   S:	Maintained

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [PATCH v4 2/3] dt-bindings: hwmon: Add NCT7363Y documentation

[Ban Feng]

Hi Paul,

On Wed, Feb 28, 2024 at 3:30 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Ban,
>
>
> Thank you for your patch.
>
>
> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
> > From: Ban Feng <kcfeng0@nuvoton.com>
> >
> > Adding bindings for the Nuvoton NCT7363Y Fan Controller
>
> s/Adding/Add/ or even Document bindings …

ok, fix in v5

>
> Do you have an URL to the datasheet?

I'll add "Datasheet: Available from Nuvoton upon request" per Guenter
suggested in v5.

>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> > ---
> >   .../bindings/hwmon/nuvoton,nct7363.yaml       | 63 +++++++++++++++++++
> >   MAINTAINERS                                   |  6 ++
> >   2 files changed, 69 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> > new file mode 100644
> > index 000000000000..1a9d9a5d614e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> > @@ -0,0 +1,63 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> > +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct7363.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Nuvoton NCT7363Y Hardware Monitoring IC
> > +
> > +maintainers:
> > +  - Ban Feng <kcfeng0@nuvoton.com>
> > +
> > +description: |
> > +  The NCT7363Y is a Fan controller which provides up to 16 independent
>
> lowecase: fan controller?

ok, fix in v5

>
> > +  FAN input monitors, and up to 16 independent PWM output with SMBus interface.
>
> output*s*?

ok, fix in v5

Thanks,
Ban

>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - nuvoton,nct7363
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#pwm-cells":
> > +    const: 2
> > +
> > +patternProperties:
> > +  "^fan-[0-9]+$":
> > +    $ref: fan-common.yaml#
> > +    unevaluatedProperties: false
> > +    required:
> > +      - pwms
> > +      - tach-ch
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#pwm-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        hwmon: hwmon@22 {
> > +            compatible = "nuvoton,nct7363";
> > +            reg = <0x22>;
> > +            #pwm-cells = <2>;
> > +
> > +            fan-0 {
> > +                pwms = <&hwmon 0 50000>;
> > +                tach-ch = /bits/ 8 <0x00>;
> > +            };
> > +            fan-1 {
> > +                pwms = <&hwmon 1 50000>;
> > +                tach-ch = /bits/ 8 <0x01>;
> > +            };
> > +        };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2ecaaec6a6bf..7b1efefed7c4 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15084,6 +15084,12 @@ S:   Maintained
> >   F:  Documentation/devicetree/bindings/hwmon/nuvoton,nct6775.yaml
> >   F:  drivers/hwmon/nct6775-i2c.c
> >
> > +NCT7363 HARDWARE MONITOR DRIVER
> > +M:   Ban Feng <kcfeng0@nuvoton.com>
> > +L:   linux-hwmon@vger.kernel.org
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> > +
> >   NETDEVSIM
> >   M:  Jakub Kicinski <kuba@kernel.org>
> >   S:  Maintained
>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
>
>
> Kind regards,
>
> Paul

[PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[baneric926]

From: Ban Feng <kcfeng0@nuvoton.com>

NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.

Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
---
 Documentation/hwmon/index.rst   |   1 +
 Documentation/hwmon/nct7363.rst |  33 +++
 MAINTAINERS                     |   2 +
 drivers/hwmon/Kconfig           |  11 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/nct7363.c         | 412 ++++++++++++++++++++++++++++++++
 6 files changed, 460 insertions(+)
 create mode 100644 Documentation/hwmon/nct7363.rst
 create mode 100644 drivers/hwmon/nct7363.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index c7ed1f73ac06..302f954b45be 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -165,6 +165,7 @@ Hardware Monitoring Kernel Drivers
    mp5990
    nct6683
    nct6775
+   nct7363
    nct7802
    nct7904
    npcm750-pwm-fan
diff --git a/Documentation/hwmon/nct7363.rst b/Documentation/hwmon/nct7363.rst
new file mode 100644
index 000000000000..89699c95aa4b
--- /dev/null
+++ b/Documentation/hwmon/nct7363.rst
@@ -0,0 +1,33 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver nct7363
+=====================
+
+Supported chip:
+
+  * Nuvoton NCT7363Y
+
+    Prefix: nct7363
+
+    Addresses: I2C 0x20, 0x21, 0x22, 0x23
+
+Author: Ban Feng <kcfeng0@nuvoton.com>
+
+
+Description
+-----------
+
+The NCT7363Y is a Fan controller which provides up to 16 independent
+FAN input monitors, and up to 16 independent PWM output with SMBus interface.
+
+
+Sysfs entries
+-------------
+
+Currently, the driver supports the following features:
+
+======================= =======================================================
+fanX_input		provide current fan rotation value in RPM
+
+pwmX			get or set PWM fan control value.
+======================= =======================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 7b1efefed7c4..7ca66b713e30 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15089,6 +15089,8 @@ M:	Ban Feng <kcfeng0@nuvoton.com>
 L:	linux-hwmon@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
+F:	Documentation/hwmon/nct7363.rst
+F:	drivers/hwmon/nct7363.c
 
 NETDEVSIM
 M:	Jakub Kicinski <kuba@kernel.org>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index a608264da87d..a297b5409b04 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1616,6 +1616,17 @@ config SENSORS_NCT6775_I2C
 	  This driver can also be built as a module. If so, the module
 	  will be called nct6775-i2c.
 
+config SENSORS_NCT7363
+	tristate "Nuvoton NCT7363Y"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for the Nuvoton NCT7363Y,
+	  hardware monitoring chip.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nct7363.
+
 config SENSORS_NCT7802
 	tristate "Nuvoton NCT7802Y"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 47be39af5c03..d5e7531204df 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o
 nct6775-objs			:= nct6775-platform.o
 obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
 obj-$(CONFIG_SENSORS_NCT6775_I2C) += nct6775-i2c.o
+obj-$(CONFIG_SENSORS_NCT7363)	+= nct7363.o
 obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
 obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
 obj-$(CONFIG_SENSORS_NPCM7XX)	+= npcm750-pwm-fan.o
diff --git a/drivers/hwmon/nct7363.c b/drivers/hwmon/nct7363.c
new file mode 100644
index 000000000000..c79d3ca4f111
--- /dev/null
+++ b/drivers/hwmon/nct7363.c
@@ -0,0 +1,412 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 Nuvoton Technology corporation.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define NCT7363_REG_GPIO_0_3		0x20
+#define NCT7363_REG_GPIO_4_7		0x21
+#define NCT7363_REG_GPIO_10_13		0x22
+#define NCT7363_REG_GPIO_14_17		0x23
+#define NCT7363_REG_PWMEN_0_7		0x38
+#define NCT7363_REG_PWMEN_8_15		0x39
+#define NCT7363_REG_FANINEN_0_7		0x41
+#define NCT7363_REG_FANINEN_8_15	0x42
+#define NCT7363_REG_FANINX_HVAL(x)	(0x48 + ((x) * 2))
+#define NCT7363_REG_FANINX_LVAL(x)	(0x49 + ((x) * 2))
+#define NCT7363_REG_FSCPXDUTY(x)	(0x90 + ((x) * 2))
+
+#define PWM_SEL(x)			(BIT(0) << (((x) % 4) * 2))
+#define FANIN_SEL(x)			(BIT(1) << (((x) % 4) * 2))
+
+#define NCT7363_FANINX_LVAL_MASK	GENMASK(4, 0)
+#define NCT7363_FANIN_MASK		GENMASK(12, 0)
+
+#define NCT7363_PWM_COUNT		16
+
+static inline unsigned long FAN_FROM_REG(u16 val)
+{
+	/* In case fan is stopped or divide by 0 */
+	if (val == NCT7363_FANIN_MASK || val == 0)
+		return	0;
+
+	return (1350000UL / val);
+}
+
+static const struct of_device_id nct7363_of_match[] = {
+	{ .compatible = "nuvoton,nct7363" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, nct7363_of_match);
+
+struct nct7363_data {
+	struct regmap		*regmap;
+	struct mutex		lock; /* protect register access */
+
+	u16			fanin_mask;
+	u16			pwm_mask;
+};
+
+static int nct7363_read_fan(struct device *dev, u32 attr, int channel,
+			    long *val)
+{
+	struct nct7363_data *data = dev_get_drvdata(dev);
+	unsigned int hi, lo;
+	u16 cnt, rpm;
+	int ret = 0;
+
+	switch (attr) {
+	case hwmon_fan_input:
+		/*
+		 * High-byte register should be read first to latch
+		 * synchronous low-byte value
+		 */
+		ret = regmap_read(data->regmap,
+				  NCT7363_REG_FANINX_HVAL(channel), &hi);
+		if (ret)
+			return ret;
+
+		ret = regmap_read(data->regmap,
+			NCT7363_REG_FANINX_LVAL(channel), &lo);
+		if (ret)
+			return ret;
+
+		cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
+		rpm = FAN_FROM_REG(cnt);
+		*val = (long)rpm;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t nct7363_fan_is_visible(const void *_data, u32 attr, int channel)
+{
+	const struct nct7363_data *data = _data;
+
+	switch (attr) {
+	case hwmon_fan_input:
+		if (data->fanin_mask & BIT(channel))
+			return 0444;
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int nct7363_read_pwm(struct device *dev, u32 attr, int channel,
+			    long *val)
+{
+	struct nct7363_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+	u16 ret;
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		ret = regmap_read(data->regmap,
+				  NCT7363_REG_FSCPXDUTY(channel), &regval);
+		if (ret)
+			return ret;
+
+		*val = (long)regval;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int nct7363_write_pwm(struct device *dev, u32 attr, int channel,
+			     long val)
+{
+	struct nct7363_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		if (val < 0 || val > 255)
+			return -EINVAL;
+
+		mutex_lock(&data->lock);
+		ret = regmap_write(data->regmap,
+				   NCT7363_REG_FSCPXDUTY(channel), val);
+		mutex_unlock(&data->lock);
+
+		return ret;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t nct7363_pwm_is_visible(const void *_data, u32 attr, int channel)
+{
+	const struct nct7363_data *data = _data;
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		if (data->pwm_mask & BIT(channel))
+			return 0644;
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int nct7363_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_fan:
+		return nct7363_read_fan(dev, attr, channel, val);
+	case hwmon_pwm:
+		return nct7363_read_pwm(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int nct7363_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	switch (type) {
+	case hwmon_pwm:
+		return nct7363_write_pwm(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t nct7363_is_visible(const void *data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_fan:
+		return nct7363_fan_is_visible(data, attr, channel);
+	case hwmon_pwm:
+		return nct7363_pwm_is_visible(data, attr, channel);
+	default:
+		return 0;
+	}
+}
+
+static const struct hwmon_channel_info *nct7363_info[] = {
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT),
+	HWMON_CHANNEL_INFO(pwm,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT),
+	NULL
+};
+
+static const struct hwmon_ops nct7363_hwmon_ops = {
+	.is_visible = nct7363_is_visible,
+	.read = nct7363_read,
+	.write = nct7363_write,
+};
+
+static const struct hwmon_chip_info nct7363_chip_info = {
+	.ops = &nct7363_hwmon_ops,
+	.info = nct7363_info,
+};
+
+static int nct7363_init_chip(struct nct7363_data *data)
+{
+	u8 i, gpio0_3 = 0, gpio4_7 = 0, gpio10_13 = 0, gpio14_17 = 0;
+	int ret;
+
+	for (i = 0; i < NCT7363_PWM_COUNT; i++) {
+		if (i < 4) {
+			if (data->pwm_mask & BIT(i))
+				gpio0_3 |= PWM_SEL(i);
+			if (data->fanin_mask & BIT(i))
+				gpio10_13 |= FANIN_SEL(i);
+		} else if (i < 8) {
+			if (data->pwm_mask & BIT(i))
+				gpio4_7 |= PWM_SEL(i);
+			if (data->fanin_mask & BIT(i))
+				gpio14_17 |= FANIN_SEL(i);
+		} else if (i < 12) {
+			if (data->pwm_mask & BIT(i))
+				gpio10_13 |= PWM_SEL(i);
+			if (data->fanin_mask & BIT(i))
+				gpio0_3 |= FANIN_SEL(i);
+		} else {
+			if (data->pwm_mask & BIT(i))
+				gpio14_17 |= PWM_SEL(i);
+			if (data->fanin_mask & BIT(i))
+				gpio4_7 |= FANIN_SEL(i);
+		}
+	}
+
+	/* Pin Function Configuration */
+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_0_3, gpio0_3);
+	if (ret < 0)
+		return ret;
+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_4_7, gpio4_7);
+	if (ret < 0)
+		return ret;
+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_10_13, gpio10_13);
+	if (ret < 0)
+		return ret;
+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_14_17, gpio14_17);
+	if (ret < 0)
+		return ret;
+
+	/* PWM and FANIN Monitoring Enable */
+	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_0_7,
+			   data->pwm_mask & 0xff);
+	if (ret < 0)
+		return ret;
+	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_8_15,
+			   (data->pwm_mask >> 8) & 0xff);
+	if (ret < 0)
+		return ret;
+	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_0_7,
+			   data->fanin_mask & 0xff);
+	if (ret < 0)
+		return ret;
+	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_8_15,
+			   (data->fanin_mask >> 8) & 0xff);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int nct7363_present_pwm_fanin(struct device *dev,
+				     struct device_node *child,
+				     struct nct7363_data *data)
+{
+	struct of_phandle_args args;
+	int ret, fanin_cnt;
+	u8 *fanin_ch;
+	u8 ch, index;
+
+	ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
+					 0, &args);
+	if (ret)
+		return ret;
+
+	data->pwm_mask |= BIT(args.args[0]);
+
+	fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
+	if (fanin_cnt < 1)
+		return -EINVAL;
+
+	fanin_ch = devm_kcalloc(dev, fanin_cnt, sizeof(*fanin_ch), GFP_KERNEL);
+	if (!fanin_ch)
+		return -ENOMEM;
+
+	ret = of_property_read_u8_array(child, "tach-ch", fanin_ch, fanin_cnt);
+	if (ret)
+		return ret;
+
+	for (ch = 0; ch < fanin_cnt; ch++) {
+		index = fanin_ch[ch];
+		data->fanin_mask |= BIT(index);
+	}
+
+	return 0;
+}
+
+static const struct regmap_config nct7363_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int nct7363_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct device_node *child;
+	struct nct7363_data *data;
+	struct device *hwmon_dev;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->regmap = devm_regmap_init_i2c(client, &nct7363_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	mutex_init(&data->lock);
+
+	for_each_child_of_node(dev->of_node, child) {
+		ret = nct7363_present_pwm_fanin(dev, child, data);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
+	}
+
+	/* Initialize the chip */
+	ret = nct7363_init_chip(data);
+	if (ret)
+		return ret;
+
+	hwmon_dev =
+		devm_hwmon_device_register_with_info(dev, client->name, data,
+						     &nct7363_chip_info, NULL);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct i2c_driver nct7363_driver = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name = "nct7363",
+		.of_match_table = nct7363_of_match,
+	},
+	.probe = nct7363_probe,
+};
+
+module_i2c_driver(nct7363_driver);
+
+MODULE_AUTHOR("CW Ho <cwho@nuvoton.com>");
+MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
+MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Paul Menzel]

Dear Ban,


Thank you for your patch. Some minor things from my side.


Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
> From: Ban Feng <kcfeng0@nuvoton.com>
> 
> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.

Please reference the datasheet.

Could you please give a high level description of the driver design?

I’d also add a verb (in imperative mood) to the commit message summary, 
so it’s a statement:

Add driver for I2C chip Nuvoton NCT7363Y

> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> ---
>   Documentation/hwmon/index.rst   |   1 +
>   Documentation/hwmon/nct7363.rst |  33 +++
>   MAINTAINERS                     |   2 +
>   drivers/hwmon/Kconfig           |  11 +
>   drivers/hwmon/Makefile          |   1 +
>   drivers/hwmon/nct7363.c         | 412 ++++++++++++++++++++++++++++++++
>   6 files changed, 460 insertions(+)
>   create mode 100644 Documentation/hwmon/nct7363.rst
>   create mode 100644 drivers/hwmon/nct7363.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index c7ed1f73ac06..302f954b45be 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -165,6 +165,7 @@ Hardware Monitoring Kernel Drivers
>      mp5990
>      nct6683
>      nct6775
> +   nct7363
>      nct7802
>      nct7904
>      npcm750-pwm-fan
> diff --git a/Documentation/hwmon/nct7363.rst b/Documentation/hwmon/nct7363.rst
> new file mode 100644
> index 000000000000..89699c95aa4b
> --- /dev/null
> +++ b/Documentation/hwmon/nct7363.rst
> @@ -0,0 +1,33 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver nct7363
> +=====================
> +
> +Supported chip:
> +
> +  * Nuvoton NCT7363Y
> +
> +    Prefix: nct7363
> +
> +    Addresses: I2C 0x20, 0x21, 0x22, 0x23
> +
> +Author: Ban Feng <kcfeng0@nuvoton.com>
> +
> +
> +Description
> +-----------
> +
> +The NCT7363Y is a Fan controller which provides up to 16 independent

fan controller

> +FAN input monitors, and up to 16 independent PWM output with SMBus interface.

output*s*

> +
> +
> +Sysfs entries
> +-------------
> +
> +Currently, the driver supports the following features:
> +
> +======================= =======================================================
> +fanX_input		provide current fan rotation value in RPM
> +
> +pwmX			get or set PWM fan control value.
> +======================= =======================================================

In the diff view this does not align with the “holes”.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7b1efefed7c4..7ca66b713e30 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15089,6 +15089,8 @@ M:	Ban Feng <kcfeng0@nuvoton.com>
>   L:	linux-hwmon@vger.kernel.org
>   S:	Maintained
>   F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> +F:	Documentation/hwmon/nct7363.rst
> +F:	drivers/hwmon/nct7363.c
>   
>   NETDEVSIM
>   M:	Jakub Kicinski <kuba@kernel.org>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index a608264da87d..a297b5409b04 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1616,6 +1616,17 @@ config SENSORS_NCT6775_I2C
>   	  This driver can also be built as a module. If so, the module
>   	  will be called nct6775-i2c.
>   
> +config SENSORS_NCT7363
> +	tristate "Nuvoton NCT7363Y"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for the Nuvoton NCT7363Y,

The comma is not needed.

> +	  hardware monitoring chip.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called nct7363.
> +
>   config SENSORS_NCT7802
>   	tristate "Nuvoton NCT7802Y"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 47be39af5c03..d5e7531204df 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o
>   nct6775-objs			:= nct6775-platform.o
>   obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
>   obj-$(CONFIG_SENSORS_NCT6775_I2C) += nct6775-i2c.o
> +obj-$(CONFIG_SENSORS_NCT7363)	+= nct7363.o
>   obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
>   obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
>   obj-$(CONFIG_SENSORS_NPCM7XX)	+= npcm750-pwm-fan.o
> diff --git a/drivers/hwmon/nct7363.c b/drivers/hwmon/nct7363.c
> new file mode 100644
> index 000000000000..c79d3ca4f111
> --- /dev/null
> +++ b/drivers/hwmon/nct7363.c
> @@ -0,0 +1,412 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Nuvoton Technology corporation.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define NCT7363_REG_GPIO_0_3		0x20
> +#define NCT7363_REG_GPIO_4_7		0x21
> +#define NCT7363_REG_GPIO_10_13		0x22
> +#define NCT7363_REG_GPIO_14_17		0x23
> +#define NCT7363_REG_PWMEN_0_7		0x38
> +#define NCT7363_REG_PWMEN_8_15		0x39
> +#define NCT7363_REG_FANINEN_0_7		0x41
> +#define NCT7363_REG_FANINEN_8_15	0x42
> +#define NCT7363_REG_FANINX_HVAL(x)	(0x48 + ((x) * 2))
> +#define NCT7363_REG_FANINX_LVAL(x)	(0x49 + ((x) * 2))
> +#define NCT7363_REG_FSCPXDUTY(x)	(0x90 + ((x) * 2))
> +
> +#define PWM_SEL(x)			(BIT(0) << (((x) % 4) * 2))
> +#define FANIN_SEL(x)			(BIT(1) << (((x) % 4) * 2))
> +
> +#define NCT7363_FANINX_LVAL_MASK	GENMASK(4, 0)
> +#define NCT7363_FANIN_MASK		GENMASK(12, 0)
> +
> +#define NCT7363_PWM_COUNT		16
> +
> +static inline unsigned long FAN_FROM_REG(u16 val)
> +{
> +	/* In case fan is stopped or divide by 0 */
> +	if (val == NCT7363_FANIN_MASK || val == 0)

The comment seems redundant. Maybe also apply the mask to `val` and 
check the result.

> +		return	0;
> +
> +	return (1350000UL / val);

Why does the mask not to be applied to `val`?

> +}
> +
> +static const struct of_device_id nct7363_of_match[] = {
> +	{ .compatible = "nuvoton,nct7363" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, nct7363_of_match);
> +
> +struct nct7363_data {
> +	struct regmap		*regmap;
> +	struct mutex		lock; /* protect register access */
> +
> +	u16			fanin_mask;
> +	u16			pwm_mask;
> +};
> +
> +static int nct7363_read_fan(struct device *dev, u32 attr, int channel,
> +			    long *val)
> +{
> +	struct nct7363_data *data = dev_get_drvdata(dev);
> +	unsigned int hi, lo;
> +	u16 cnt, rpm;
> +	int ret = 0;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		/*
> +		 * High-byte register should be read first to latch
> +		 * synchronous low-byte value
> +		 */
> +		ret = regmap_read(data->regmap,
> +				  NCT7363_REG_FANINX_HVAL(channel), &hi);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(data->regmap,
> +			NCT7363_REG_FANINX_LVAL(channel), &lo);
> +		if (ret)
> +			return ret;
> +
> +		cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
> +		rpm = FAN_FROM_REG(cnt);
> +		*val = (long)rpm;

Could `FAN_FROM_REG()` return long, so the cast here is not needed?

> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t nct7363_fan_is_visible(const void *_data, u32 attr, int channel)
> +{
> +	const struct nct7363_data *data = _data;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		if (data->fanin_mask & BIT(channel))
> +			return 0444;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int nct7363_read_pwm(struct device *dev, u32 attr, int channel,
> +			    long *val)
> +{
> +	struct nct7363_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	u16 ret;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		ret = regmap_read(data->regmap,
> +				  NCT7363_REG_FSCPXDUTY(channel), &regval);

     int regmap_read(struct regmap *map, unsigned int reg, unsigned int 
val);

So `ret` should be `int`?


Kind regards,

Paul


> +		if (ret)
> +			return ret;
> +
> +		*val = (long)regval;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int nct7363_write_pwm(struct device *dev, u32 attr, int channel,
> +			     long val)
> +{
> +	struct nct7363_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		if (val < 0 || val > 255)
> +			return -EINVAL;
> +
> +		mutex_lock(&data->lock);
> +		ret = regmap_write(data->regmap,
> +				   NCT7363_REG_FSCPXDUTY(channel), val);
> +		mutex_unlock(&data->lock);
> +
> +		return ret;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t nct7363_pwm_is_visible(const void *_data, u32 attr, int channel)
> +{
> +	const struct nct7363_data *data = _data;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		if (data->pwm_mask & BIT(channel))
> +			return 0644;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int nct7363_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		return nct7363_read_fan(dev, attr, channel, val);
> +	case hwmon_pwm:
> +		return nct7363_read_pwm(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int nct7363_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_pwm:
> +		return nct7363_write_pwm(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t nct7363_is_visible(const void *data,
> +				  enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		return nct7363_fan_is_visible(data, attr, channel);
> +	case hwmon_pwm:
> +		return nct7363_pwm_is_visible(data, attr, channel);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static const struct hwmon_channel_info *nct7363_info[] = {
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_ops nct7363_hwmon_ops = {
> +	.is_visible = nct7363_is_visible,
> +	.read = nct7363_read,
> +	.write = nct7363_write,
> +};
> +
> +static const struct hwmon_chip_info nct7363_chip_info = {
> +	.ops = &nct7363_hwmon_ops,
> +	.info = nct7363_info,
> +};
> +
> +static int nct7363_init_chip(struct nct7363_data *data)
> +{
> +	u8 i, gpio0_3 = 0, gpio4_7 = 0, gpio10_13 = 0, gpio14_17 = 0;
> +	int ret;
> +
> +	for (i = 0; i < NCT7363_PWM_COUNT; i++) {
> +		if (i < 4) {
> +			if (data->pwm_mask & BIT(i))
> +				gpio0_3 |= PWM_SEL(i);
> +			if (data->fanin_mask & BIT(i))
> +				gpio10_13 |= FANIN_SEL(i);
> +		} else if (i < 8) {
> +			if (data->pwm_mask & BIT(i))
> +				gpio4_7 |= PWM_SEL(i);
> +			if (data->fanin_mask & BIT(i))
> +				gpio14_17 |= FANIN_SEL(i);
> +		} else if (i < 12) {
> +			if (data->pwm_mask & BIT(i))
> +				gpio10_13 |= PWM_SEL(i);
> +			if (data->fanin_mask & BIT(i))
> +				gpio0_3 |= FANIN_SEL(i);
> +		} else {
> +			if (data->pwm_mask & BIT(i))
> +				gpio14_17 |= PWM_SEL(i);
> +			if (data->fanin_mask & BIT(i))
> +				gpio4_7 |= FANIN_SEL(i);
> +		}
> +	}
> +
> +	/* Pin Function Configuration */
> +	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_0_3, gpio0_3);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_4_7, gpio4_7);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_10_13, gpio10_13);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_14_17, gpio14_17);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* PWM and FANIN Monitoring Enable */
> +	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_0_7,
> +			   data->pwm_mask & 0xff);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_8_15,
> +			   (data->pwm_mask >> 8) & 0xff);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_0_7,
> +			   data->fanin_mask & 0xff);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_8_15,
> +			   (data->fanin_mask >> 8) & 0xff);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int nct7363_present_pwm_fanin(struct device *dev,
> +				     struct device_node *child,
> +				     struct nct7363_data *data)
> +{
> +	struct of_phandle_args args;
> +	int ret, fanin_cnt;
> +	u8 *fanin_ch;
> +	u8 ch, index;
> +
> +	ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
> +					 0, &args);
> +	if (ret)
> +		return ret;
> +
> +	data->pwm_mask |= BIT(args.args[0]);
> +
> +	fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
> +	if (fanin_cnt < 1)
> +		return -EINVAL;
> +
> +	fanin_ch = devm_kcalloc(dev, fanin_cnt, sizeof(*fanin_ch), GFP_KERNEL);
> +	if (!fanin_ch)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u8_array(child, "tach-ch", fanin_ch, fanin_cnt);
> +	if (ret)
> +		return ret;
> +
> +	for (ch = 0; ch < fanin_cnt; ch++) {
> +		index = fanin_ch[ch];
> +		data->fanin_mask |= BIT(index);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config nct7363_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int nct7363_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device_node *child;
> +	struct nct7363_data *data;
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &nct7363_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	mutex_init(&data->lock);
> +
> +	for_each_child_of_node(dev->of_node, child) {
> +		ret = nct7363_present_pwm_fanin(dev, child, data);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +	}
> +
> +	/* Initialize the chip */
> +	ret = nct7363_init_chip(data);
> +	if (ret)
> +		return ret;
> +
> +	hwmon_dev =
> +		devm_hwmon_device_register_with_info(dev, client->name, data,
> +						     &nct7363_chip_info, NULL);
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct i2c_driver nct7363_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name = "nct7363",
> +		.of_match_table = nct7363_of_match,
> +	},
> +	.probe = nct7363_probe,
> +};
> +
> +module_i2c_driver(nct7363_driver);
> +
> +MODULE_AUTHOR("CW Ho <cwho@nuvoton.com>");
> +MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
> +MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Guenter Roeck]

On 2/27/24 23:57, Paul Menzel wrote:
> Dear Ban,
> 
> 
> Thank you for your patch. Some minor things from my side.
> 
> 
> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
>> From: Ban Feng <kcfeng0@nuvoton.com>
>>
>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
> 
> Please reference the datasheet.
> 

Note that something like

Datasheet: Available from Nuvoton upon request

is quite common for hardware monitoring chips and acceptable.

> Could you please give a high level description of the driver design?
> 

Can you be more specific ? I didn't have time yet to look into details,
but at first glance this looks like a standard hardware monitoring driver.
One could argue that the high level design of such drivers is described
in Documentation/hwmon/hwmon-kernel-api.rst.
I don't usually ask for a additional design information for hwmon drivers
unless some chip interaction is unusual and needs to be explained,
and then I prefer to have it explained in the code. Given that, I am
quite curious and would like to understand what you are looking for.

Thanks,
Guenter

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Paul Menzel]

Dear Guenter,


Am 28.02.24 um 10:03 schrieb Guenter Roeck:
> On 2/27/24 23:57, Paul Menzel wrote:

>> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
>>> From: Ban Feng <kcfeng0@nuvoton.com>
>>>
>>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>>
>> Please reference the datasheet.
> 
> Note that something like
> 
> Datasheet: Available from Nuvoton upon request
> 
> is quite common for hardware monitoring chips and acceptable.

Yes, it would be nice to document it though. (And finally for vendors to 
just make them available for download.)

>> Could you please give a high level description of the driver design?
> 
> Can you be more specific ? I didn't have time yet to look into details,
> but at first glance this looks like a standard hardware monitoring driver.
> One could argue that the high level design of such drivers is described
> in Documentation/hwmon/hwmon-kernel-api.rst.
> 
> I don't usually ask for a additional design information for hwmon drivers
> unless some chip interaction is unusual and needs to be explained,
> and then I prefer to have it explained in the code. Given that, I am
> quite curious and would like to understand what you are looking for.
For a 10+ lines commit, in my opinion the commit message should say 
something about the implementation. Even it is just, as you wrote, a 
note, that it follows the standard design.


Kind regards,

Paul

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Guenter Roeck]

On 2/28/24 03:03, Paul Menzel wrote:
> Dear Guenter,
> 
> 
> Am 28.02.24 um 10:03 schrieb Guenter Roeck:
>> On 2/27/24 23:57, Paul Menzel wrote:
> 
>>> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
>>>> From: Ban Feng <kcfeng0@nuvoton.com>
>>>>
>>>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>>>
>>> Please reference the datasheet.
>>
>> Note that something like
>>
>> Datasheet: Available from Nuvoton upon request
>>
>> is quite common for hardware monitoring chips and acceptable.
> 
> Yes, it would be nice to document it though. (And finally for vendors to just make them available for download.)
> 

Nuvoton is nice enough and commonly makes datasheets available on request.
The only exception I have seen so far is where they were forced into an NDA
by a large chip and board vendor, which prevented them from publishing a
specific datasheet.

Others are much worse. Many PMIC vendors don't publish their datasheets at
all, and sometimes chips don't even officially exist (notorious for chips
intended for the automotive market). Just look at the whole discussion
around MAX31335.

Anyway, there are lots of examples in Documentation/hwmon/. I don't see
the need to add further documentation, and I specifically don't want to
make it official that "Datasheet not public" is acceptable as well.
We really don't have a choice unless we want to exclude a whole class
of chips from the kernel, but that doesn't make it better.

>>> Could you please give a high level description of the driver design?
>>
>> Can you be more specific ? I didn't have time yet to look into details,
>> but at first glance this looks like a standard hardware monitoring driver.
>> One could argue that the high level design of such drivers is described
>> in Documentation/hwmon/hwmon-kernel-api.rst.
>>
>> I don't usually ask for a additional design information for hwmon drivers
>> unless some chip interaction is unusual and needs to be explained,
>> and then I prefer to have it explained in the code. Given that, I am
>> quite curious and would like to understand what you are looking for.
> For a 10+ lines commit, in my opinion the commit message should say something about the implementation. Even it is just, as you wrote, a note, that it follows the standard design.
> 

Again, I have not looked into the submission, but usually we ask for that
to be documented in Documentation/hwmon/. I find that much better than
a soon-to-be-forgotten commit message. I don't mind something like
"The NCT7363Y is a fan controller with up to 16 independent fan input
  monitors and up to 16 independent PWM outputs. It also supports up
  to 16 GPIO pins"
or in other words a description of the chip, not the implementation.
That a driver hwmon driver uses the hardware monitoring API seems to be
obvious to me, so I don't see the value of adding it to the commit
description. I would not mind having something there, but I don't
see it as mandatory.

On the  other side, granted, that is just _my_ personal opinion.
Do we have a common guideline for what exactly should be in commit
descriptions for driver submissions ? I guess I should look that up.

Thanks,
Guenter

Commit messages (was: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y)

[Paul Menzel]

Dear Guenter,


Thank you for your reply.

Am 28.02.24 um 17:03 schrieb Guenter Roeck:
> On 2/28/24 03:03, Paul Menzel wrote:

>> Am 28.02.24 um 10:03 schrieb Guenter Roeck:
>>> On 2/27/24 23:57, Paul Menzel wrote:
>>
>>>> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
>>>>> From: Ban Feng <kcfeng0@nuvoton.com>
>>>>>
>>>>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>>>>
>>>> Please reference the datasheet.
>>>
>>> Note that something like
>>>
>>> Datasheet: Available from Nuvoton upon request
>>>
>>> is quite common for hardware monitoring chips and acceptable.
>>
>> Yes, it would be nice to document it though. (And finally for vendors 
>> to just make them available for download.)
> 
> Nuvoton is nice enough and commonly makes datasheets available on request.
> The only exception I have seen so far is where they were forced into an NDA
> by a large chip and board vendor, which prevented them from publishing a
> specific datasheet.

Nice, that they are better in this regard than others.

> Others are much worse. Many PMIC vendors don't publish their datasheets at
> all, and sometimes chips don't even officially exist (notorious for chips
> intended for the automotive market). Just look at the whole discussion
> around MAX31335.
> 
> Anyway, there are lots of examples in Documentation/hwmon/. I don't see
> the need to add further documentation, and I specifically don't want to
> make it official that "Datasheet not public" is acceptable as well.
> We really don't have a choice unless we want to exclude a whole class
> of chips from the kernel, but that doesn't make it better.

I know folks figure it out eventually, but I found it helpful to have 
the datesheet name in the commit message to know what to search for, ask 
for, or in case of difference between datasheet revision what to compare 
against.

>>>> Could you please give a high level description of the driver design?
>>>
>>> Can you be more specific ? I didn't have time yet to look into details,
>>> but at first glance this looks like a standard hardware monitoring 
>>> driver.
>>> One could argue that the high level design of such drivers is described
>>> in Documentation/hwmon/hwmon-kernel-api.rst.
>>>
>>> I don't usually ask for a additional design information for hwmon drivers
>>> unless some chip interaction is unusual and needs to be explained,
>>> and then I prefer to have it explained in the code. Given that, I am
>>> quite curious and would like to understand what you are looking for.
>> For a 10+ lines commit, in my opinion the commit message should say 
>> something about the implementation. Even it is just, as you wrote, a 
>> note, that it follows the standard design.
> 
> Again, I have not looked into the submission, but usually we ask for that
> to be documented in Documentation/hwmon/. I find that much better than
> a soon-to-be-forgotten commit message. I don't mind something like
> "The NCT7363Y is a fan controller with up to 16 independent fan input
>   monitors and up to 16 independent PWM outputs. It also supports up
>   to 16 GPIO pins"
> or in other words a description of the chip, not the implementation.
> That a driver hwmon driver uses the hardware monitoring API seems to be
> obvious to me, so I don't see the value of adding it to the commit
> description. I would not mind having something there, but I don't
> see it as mandatory.
> 
> On the  other side, granted, that is just _my_ personal opinion.
> Do we have a common guideline for what exactly should be in commit
> descriptions for driver submissions ? I guess I should look that up.

`Documentation/hwmon/submitting-patches.rst` refers to 
`Documentation/process/submitting-patches.rst`, and there *Describe your 
changes* seems to have been written for documenting bug fixes or 
enhancements and not new additions. It for example contains:

> Once the problem is established, describe what you are actually doing
> about it in technical detail.  It's important to describe the change
> in plain English for the reviewer to verify that the code is behaving
> as you intend it to.

I agree with your description, but I am also convinced if you write 500 
lines of code, that you can write ten lines of commit messages giving a 
broad overview. In this case, saying that it follows the standard driver 
model would be good enough for me.

Also, at least for me, often having to bisect stuff and using `git 
blame` to look at old commits, commit messages are very valuable to me, 
and not “forgotten”. ;-)


Kind regards,

Paul

Re: Commit messages (was: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y)

[Ban Feng]

Hi Paul and Guenter,

Appreciate your valuable comments, and I'll add some commit messages
to describe this chip for pwm and fan.

Thanks,
Ban

On Thu, Feb 29, 2024 at 12:25 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Guenter,
>
>
> Thank you for your reply.
>
> Am 28.02.24 um 17:03 schrieb Guenter Roeck:
> > On 2/28/24 03:03, Paul Menzel wrote:
>
> >> Am 28.02.24 um 10:03 schrieb Guenter Roeck:
> >>> On 2/27/24 23:57, Paul Menzel wrote:
> >>
> >>>> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
> >>>>> From: Ban Feng <kcfeng0@nuvoton.com>
> >>>>>
> >>>>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
> >>>>
> >>>> Please reference the datasheet.
> >>>
> >>> Note that something like
> >>>
> >>> Datasheet: Available from Nuvoton upon request
> >>>
> >>> is quite common for hardware monitoring chips and acceptable.
> >>
> >> Yes, it would be nice to document it though. (And finally for vendors
> >> to just make them available for download.)
> >
> > Nuvoton is nice enough and commonly makes datasheets available on request.
> > The only exception I have seen so far is where they were forced into an NDA
> > by a large chip and board vendor, which prevented them from publishing a
> > specific datasheet.
>
> Nice, that they are better in this regard than others.
>
> > Others are much worse. Many PMIC vendors don't publish their datasheets at
> > all, and sometimes chips don't even officially exist (notorious for chips
> > intended for the automotive market). Just look at the whole discussion
> > around MAX31335.
> >
> > Anyway, there are lots of examples in Documentation/hwmon/. I don't see
> > the need to add further documentation, and I specifically don't want to
> > make it official that "Datasheet not public" is acceptable as well.
> > We really don't have a choice unless we want to exclude a whole class
> > of chips from the kernel, but that doesn't make it better.
>
> I know folks figure it out eventually, but I found it helpful to have
> the datesheet name in the commit message to know what to search for, ask
> for, or in case of difference between datasheet revision what to compare
> against.
>
> >>>> Could you please give a high level description of the driver design?
> >>>
> >>> Can you be more specific ? I didn't have time yet to look into details,
> >>> but at first glance this looks like a standard hardware monitoring
> >>> driver.
> >>> One could argue that the high level design of such drivers is described
> >>> in Documentation/hwmon/hwmon-kernel-api.rst.
> >>>
> >>> I don't usually ask for a additional design information for hwmon drivers
> >>> unless some chip interaction is unusual and needs to be explained,
> >>> and then I prefer to have it explained in the code. Given that, I am
> >>> quite curious and would like to understand what you are looking for.
> >> For a 10+ lines commit, in my opinion the commit message should say
> >> something about the implementation. Even it is just, as you wrote, a
> >> note, that it follows the standard design.
> >
> > Again, I have not looked into the submission, but usually we ask for that
> > to be documented in Documentation/hwmon/. I find that much better than
> > a soon-to-be-forgotten commit message. I don't mind something like
> > "The NCT7363Y is a fan controller with up to 16 independent fan input
> >   monitors and up to 16 independent PWM outputs. It also supports up
> >   to 16 GPIO pins"
> > or in other words a description of the chip, not the implementation.
> > That a driver hwmon driver uses the hardware monitoring API seems to be
> > obvious to me, so I don't see the value of adding it to the commit
> > description. I would not mind having something there, but I don't
> > see it as mandatory.
> >
> > On the  other side, granted, that is just _my_ personal opinion.
> > Do we have a common guideline for what exactly should be in commit
> > descriptions for driver submissions ? I guess I should look that up.
>
> `Documentation/hwmon/submitting-patches.rst` refers to
> `Documentation/process/submitting-patches.rst`, and there *Describe your
> changes* seems to have been written for documenting bug fixes or
> enhancements and not new additions. It for example contains:
>
> > Once the problem is established, describe what you are actually doing
> > about it in technical detail.  It's important to describe the change
> > in plain English for the reviewer to verify that the code is behaving
> > as you intend it to.
>
> I agree with your description, but I am also convinced if you write 500
> lines of code, that you can write ten lines of commit messages giving a
> broad overview. In this case, saying that it follows the standard driver
> model would be good enough for me.
>
> Also, at least for me, often having to bisect stuff and using `git
> blame` to look at old commits, commit messages are very valuable to me,
> and not “forgotten”. ;-)
>
>
> Kind regards,
>
> Paul

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Ban Feng]

Hi Paul,

On Wed, Feb 28, 2024 at 3:57 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Ban,
>
>
> Thank you for your patch. Some minor things from my side.
>
>
> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
> > From: Ban Feng <kcfeng0@nuvoton.com>
> >
> > NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>
> Please reference the datasheet.
>
> Could you please give a high level description of the driver design?
>
> I’d also add a verb (in imperative mood) to the commit message summary,
> so it’s a statement:
>
> Add driver for I2C chip Nuvoton NCT7363Y

ok, I'll add some reviews to describe this chip for pwm and fan in v5.

>
> > Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> > ---
> >   Documentation/hwmon/index.rst   |   1 +
> >   Documentation/hwmon/nct7363.rst |  33 +++
> >   MAINTAINERS                     |   2 +
> >   drivers/hwmon/Kconfig           |  11 +
> >   drivers/hwmon/Makefile          |   1 +
> >   drivers/hwmon/nct7363.c         | 412 ++++++++++++++++++++++++++++++++
> >   6 files changed, 460 insertions(+)
> >   create mode 100644 Documentation/hwmon/nct7363.rst
> >   create mode 100644 drivers/hwmon/nct7363.c
> >
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index c7ed1f73ac06..302f954b45be 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -165,6 +165,7 @@ Hardware Monitoring Kernel Drivers
> >      mp5990
> >      nct6683
> >      nct6775
> > +   nct7363
> >      nct7802
> >      nct7904
> >      npcm750-pwm-fan
> > diff --git a/Documentation/hwmon/nct7363.rst b/Documentation/hwmon/nct7363.rst
> > new file mode 100644
> > index 000000000000..89699c95aa4b
> > --- /dev/null
> > +++ b/Documentation/hwmon/nct7363.rst
> > @@ -0,0 +1,33 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Kernel driver nct7363
> > +=====================
> > +
> > +Supported chip:
> > +
> > +  * Nuvoton NCT7363Y
> > +
> > +    Prefix: nct7363
> > +
> > +    Addresses: I2C 0x20, 0x21, 0x22, 0x23
> > +
> > +Author: Ban Feng <kcfeng0@nuvoton.com>
> > +
> > +
> > +Description
> > +-----------
> > +
> > +The NCT7363Y is a Fan controller which provides up to 16 independent
>
> fan controller

ok, fix in v5

>
> > +FAN input monitors, and up to 16 independent PWM output with SMBus interface.
>
> output*s*

ok, fix in v5

>
> > +
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +Currently, the driver supports the following features:
> > +
> > +======================= =======================================================
> > +fanX_input           provide current fan rotation value in RPM
> > +
> > +pwmX                 get or set PWM fan control value.
> > +======================= =======================================================
>
> In the diff view this does not align with the “holes”.

ok, fix in v5

>
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 7b1efefed7c4..7ca66b713e30 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15089,6 +15089,8 @@ M:    Ban Feng <kcfeng0@nuvoton.com>
> >   L:  linux-hwmon@vger.kernel.org
> >   S:  Maintained
> >   F:  Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> > +F:   Documentation/hwmon/nct7363.rst
> > +F:   drivers/hwmon/nct7363.c
> >
> >   NETDEVSIM
> >   M:  Jakub Kicinski <kuba@kernel.org>
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index a608264da87d..a297b5409b04 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1616,6 +1616,17 @@ config SENSORS_NCT6775_I2C
> >         This driver can also be built as a module. If so, the module
> >         will be called nct6775-i2c.
> >
> > +config SENSORS_NCT7363
> > +     tristate "Nuvoton NCT7363Y"
> > +     depends on I2C
> > +     select REGMAP_I2C
> > +     help
> > +       If you say yes here you get support for the Nuvoton NCT7363Y,
>
> The comma is not needed.

ok, fix in v5

>
> > +       hardware monitoring chip.
> > +
> > +       This driver can also be built as a module. If so, the module
> > +       will be called nct7363.
> > +
> >   config SENSORS_NCT7802
> >       tristate "Nuvoton NCT7802Y"
> >       depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 47be39af5c03..d5e7531204df 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o
> >   nct6775-objs                        := nct6775-platform.o
> >   obj-$(CONFIG_SENSORS_NCT6775)       += nct6775.o
> >   obj-$(CONFIG_SENSORS_NCT6775_I2C) += nct6775-i2c.o
> > +obj-$(CONFIG_SENSORS_NCT7363)        += nct7363.o
> >   obj-$(CONFIG_SENSORS_NCT7802)       += nct7802.o
> >   obj-$(CONFIG_SENSORS_NCT7904)       += nct7904.o
> >   obj-$(CONFIG_SENSORS_NPCM7XX)       += npcm750-pwm-fan.o
> > diff --git a/drivers/hwmon/nct7363.c b/drivers/hwmon/nct7363.c
> > new file mode 100644
> > index 000000000000..c79d3ca4f111
> > --- /dev/null
> > +++ b/drivers/hwmon/nct7363.c
> > @@ -0,0 +1,412 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2023 Nuvoton Technology corporation.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#define NCT7363_REG_GPIO_0_3         0x20
> > +#define NCT7363_REG_GPIO_4_7         0x21
> > +#define NCT7363_REG_GPIO_10_13               0x22
> > +#define NCT7363_REG_GPIO_14_17               0x23
> > +#define NCT7363_REG_PWMEN_0_7                0x38
> > +#define NCT7363_REG_PWMEN_8_15               0x39
> > +#define NCT7363_REG_FANINEN_0_7              0x41
> > +#define NCT7363_REG_FANINEN_8_15     0x42
> > +#define NCT7363_REG_FANINX_HVAL(x)   (0x48 + ((x) * 2))
> > +#define NCT7363_REG_FANINX_LVAL(x)   (0x49 + ((x) * 2))
> > +#define NCT7363_REG_FSCPXDUTY(x)     (0x90 + ((x) * 2))
> > +
> > +#define PWM_SEL(x)                   (BIT(0) << (((x) % 4) * 2))
> > +#define FANIN_SEL(x)                 (BIT(1) << (((x) % 4) * 2))
> > +
> > +#define NCT7363_FANINX_LVAL_MASK     GENMASK(4, 0)
> > +#define NCT7363_FANIN_MASK           GENMASK(12, 0)
> > +
> > +#define NCT7363_PWM_COUNT            16
> > +
> > +static inline unsigned long FAN_FROM_REG(u16 val)
> > +{
> > +     /* In case fan is stopped or divide by 0 */
> > +     if (val == NCT7363_FANIN_MASK || val == 0)
>
> The comment seems redundant. Maybe also apply the mask to `val` and
> check the result.
>
> > +             return  0;
> > +
> > +     return (1350000UL / val);
>
> Why does the mask not to be applied to `val`?
>

The combination of the fan count value is given by FANINx_HVAL 8-bit
register (fan count value[12:5]) and FANINx_LVAL 8-bit register (fan
count value[4:0], [7:5] is reserved and the default value is 0).
Therefore, I don't apply NCT7363_FANIN_MASK to val. And there are two
values we need to consider, 0x0000 and 0xff1f. 0x0000 means FANINx
monitoring is disabled, and 0xff1f means FANINx monitoring is enabled
but doesn't connect to the fan.

> > +}
> > +
> > +static const struct of_device_id nct7363_of_match[] = {
> > +     { .compatible = "nuvoton,nct7363" },
> > +     { },
> > +};
> > +MODULE_DEVICE_TABLE(of, nct7363_of_match);
> > +
> > +struct nct7363_data {
> > +     struct regmap           *regmap;
> > +     struct mutex            lock; /* protect register access */
> > +
> > +     u16                     fanin_mask;
> > +     u16                     pwm_mask;
> > +};
> > +
> > +static int nct7363_read_fan(struct device *dev, u32 attr, int channel,
> > +                         long *val)
> > +{
> > +     struct nct7363_data *data = dev_get_drvdata(dev);
> > +     unsigned int hi, lo;
> > +     u16 cnt, rpm;
> > +     int ret = 0;
> > +
> > +     switch (attr) {
> > +     case hwmon_fan_input:
> > +             /*
> > +              * High-byte register should be read first to latch
> > +              * synchronous low-byte value
> > +              */
> > +             ret = regmap_read(data->regmap,
> > +                               NCT7363_REG_FANINX_HVAL(channel), &hi);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regmap_read(data->regmap,
> > +                     NCT7363_REG_FANINX_LVAL(channel), &lo);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
> > +             rpm = FAN_FROM_REG(cnt);
> > +             *val = (long)rpm;
>
> Could `FAN_FROM_REG()` return long, so the cast here is not needed?

ok, fix in v5

>
> > +             return 0;
> > +     default:
> > +             return -EOPNOTSUPP;
> > +     }
> > +}
> > +
> > +static umode_t nct7363_fan_is_visible(const void *_data, u32 attr, int channel)
> > +{
> > +     const struct nct7363_data *data = _data;
> > +
> > +     switch (attr) {
> > +     case hwmon_fan_input:
> > +             if (data->fanin_mask & BIT(channel))
> > +                     return 0444;
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int nct7363_read_pwm(struct device *dev, u32 attr, int channel,
> > +                         long *val)
> > +{
> > +     struct nct7363_data *data = dev_get_drvdata(dev);
> > +     unsigned int regval;
> > +     u16 ret;
> > +
> > +     switch (attr) {
> > +     case hwmon_pwm_input:
> > +             ret = regmap_read(data->regmap,
> > +                               NCT7363_REG_FSCPXDUTY(channel), &regval);
>
>      int regmap_read(struct regmap *map, unsigned int reg, unsigned int
> val);
>
> So `ret` should be `int`?

ok, fix in v5

Thanks,
Ban

>
>
> Kind regards,
>
> Paul
>
>
> > +             if (ret)
> > +                     return ret;
> > +
> > +             *val = (long)regval;
> > +             return 0;
> > +     default:
> > +             return -EOPNOTSUPP;
> > +     }
> > +}
> > +
> > +static int nct7363_write_pwm(struct device *dev, u32 attr, int channel,
> > +                          long val)
> > +{
> > +     struct nct7363_data *data = dev_get_drvdata(dev);
> > +     int ret;
> > +
> > +     switch (attr) {
> > +     case hwmon_pwm_input:
> > +             if (val < 0 || val > 255)
> > +                     return -EINVAL;
> > +
> > +             mutex_lock(&data->lock);
> > +             ret = regmap_write(data->regmap,
> > +                                NCT7363_REG_FSCPXDUTY(channel), val);
> > +             mutex_unlock(&data->lock);
> > +
> > +             return ret;
> > +
> > +     default:
> > +             return -EOPNOTSUPP;
> > +     }
> > +}
> > +
> > +static umode_t nct7363_pwm_is_visible(const void *_data, u32 attr, int channel)
> > +{
> > +     const struct nct7363_data *data = _data;
> > +
> > +     switch (attr) {
> > +     case hwmon_pwm_input:
> > +             if (data->pwm_mask & BIT(channel))
> > +                     return 0644;
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int nct7363_read(struct device *dev, enum hwmon_sensor_types type,
> > +                     u32 attr, int channel, long *val)
> > +{
> > +     switch (type) {
> > +     case hwmon_fan:
> > +             return nct7363_read_fan(dev, attr, channel, val);
> > +     case hwmon_pwm:
> > +             return nct7363_read_pwm(dev, attr, channel, val);
> > +     default:
> > +             return -EOPNOTSUPP;
> > +     }
> > +}
> > +
> > +static int nct7363_write(struct device *dev, enum hwmon_sensor_types type,
> > +                      u32 attr, int channel, long val)
> > +{
> > +     switch (type) {
> > +     case hwmon_pwm:
> > +             return nct7363_write_pwm(dev, attr, channel, val);
> > +     default:
> > +             return -EOPNOTSUPP;
> > +     }
> > +}
> > +
> > +static umode_t nct7363_is_visible(const void *data,
> > +                               enum hwmon_sensor_types type,
> > +                               u32 attr, int channel)
> > +{
> > +     switch (type) {
> > +     case hwmon_fan:
> > +             return nct7363_fan_is_visible(data, attr, channel);
> > +     case hwmon_pwm:
> > +             return nct7363_pwm_is_visible(data, attr, channel);
> > +     default:
> > +             return 0;
> > +     }
> > +}
> > +
> > +static const struct hwmon_channel_info *nct7363_info[] = {
> > +     HWMON_CHANNEL_INFO(fan,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT,
> > +                        HWMON_F_INPUT),
> > +     HWMON_CHANNEL_INFO(pwm,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT,
> > +                        HWMON_PWM_INPUT),
> > +     NULL
> > +};
> > +
> > +static const struct hwmon_ops nct7363_hwmon_ops = {
> > +     .is_visible = nct7363_is_visible,
> > +     .read = nct7363_read,
> > +     .write = nct7363_write,
> > +};
> > +
> > +static const struct hwmon_chip_info nct7363_chip_info = {
> > +     .ops = &nct7363_hwmon_ops,
> > +     .info = nct7363_info,
> > +};
> > +
> > +static int nct7363_init_chip(struct nct7363_data *data)
> > +{
> > +     u8 i, gpio0_3 = 0, gpio4_7 = 0, gpio10_13 = 0, gpio14_17 = 0;
> > +     int ret;
> > +
> > +     for (i = 0; i < NCT7363_PWM_COUNT; i++) {
> > +             if (i < 4) {
> > +                     if (data->pwm_mask & BIT(i))
> > +                             gpio0_3 |= PWM_SEL(i);
> > +                     if (data->fanin_mask & BIT(i))
> > +                             gpio10_13 |= FANIN_SEL(i);
> > +             } else if (i < 8) {
> > +                     if (data->pwm_mask & BIT(i))
> > +                             gpio4_7 |= PWM_SEL(i);
> > +                     if (data->fanin_mask & BIT(i))
> > +                             gpio14_17 |= FANIN_SEL(i);
> > +             } else if (i < 12) {
> > +                     if (data->pwm_mask & BIT(i))
> > +                             gpio10_13 |= PWM_SEL(i);
> > +                     if (data->fanin_mask & BIT(i))
> > +                             gpio0_3 |= FANIN_SEL(i);
> > +             } else {
> > +                     if (data->pwm_mask & BIT(i))
> > +                             gpio14_17 |= PWM_SEL(i);
> > +                     if (data->fanin_mask & BIT(i))
> > +                             gpio4_7 |= FANIN_SEL(i);
> > +             }
> > +     }
> > +
> > +     /* Pin Function Configuration */
> > +     ret = regmap_write(data->regmap, NCT7363_REG_GPIO_0_3, gpio0_3);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = regmap_write(data->regmap, NCT7363_REG_GPIO_4_7, gpio4_7);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = regmap_write(data->regmap, NCT7363_REG_GPIO_10_13, gpio10_13);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = regmap_write(data->regmap, NCT7363_REG_GPIO_14_17, gpio14_17);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* PWM and FANIN Monitoring Enable */
> > +     ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_0_7,
> > +                        data->pwm_mask & 0xff);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_8_15,
> > +                        (data->pwm_mask >> 8) & 0xff);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_0_7,
> > +                        data->fanin_mask & 0xff);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_8_15,
> > +                        (data->fanin_mask >> 8) & 0xff);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static int nct7363_present_pwm_fanin(struct device *dev,
> > +                                  struct device_node *child,
> > +                                  struct nct7363_data *data)
> > +{
> > +     struct of_phandle_args args;
> > +     int ret, fanin_cnt;
> > +     u8 *fanin_ch;
> > +     u8 ch, index;
> > +
> > +     ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
> > +                                      0, &args);
> > +     if (ret)
> > +             return ret;
> > +
> > +     data->pwm_mask |= BIT(args.args[0]);
> > +
> > +     fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
> > +     if (fanin_cnt < 1)
> > +             return -EINVAL;
> > +
> > +     fanin_ch = devm_kcalloc(dev, fanin_cnt, sizeof(*fanin_ch), GFP_KERNEL);
> > +     if (!fanin_ch)
> > +             return -ENOMEM;
> > +
> > +     ret = of_property_read_u8_array(child, "tach-ch", fanin_ch, fanin_cnt);
> > +     if (ret)
> > +             return ret;
> > +
> > +     for (ch = 0; ch < fanin_cnt; ch++) {
> > +             index = fanin_ch[ch];
> > +             data->fanin_mask |= BIT(index);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct regmap_config nct7363_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +};
> > +
> > +static int nct7363_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct device_node *child;
> > +     struct nct7363_data *data;
> > +     struct device *hwmon_dev;
> > +     int ret;
> > +
> > +     data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     data->regmap = devm_regmap_init_i2c(client, &nct7363_regmap_config);
> > +     if (IS_ERR(data->regmap))
> > +             return PTR_ERR(data->regmap);
> > +
> > +     mutex_init(&data->lock);
> > +
> > +     for_each_child_of_node(dev->of_node, child) {
> > +             ret = nct7363_present_pwm_fanin(dev, child, data);
> > +             if (ret) {
> > +                     of_node_put(child);
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     /* Initialize the chip */
> > +     ret = nct7363_init_chip(data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     hwmon_dev =
> > +             devm_hwmon_device_register_with_info(dev, client->name, data,
> > +                                                  &nct7363_chip_info, NULL);
> > +     return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static struct i2c_driver nct7363_driver = {
> > +     .class = I2C_CLASS_HWMON,
> > +     .driver = {
> > +             .name = "nct7363",
> > +             .of_match_table = nct7363_of_match,
> > +     },
> > +     .probe = nct7363_probe,
> > +};
> > +
> > +module_i2c_driver(nct7363_driver);
> > +
> > +MODULE_AUTHOR("CW Ho <cwho@nuvoton.com>");
> > +MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
> > +MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
> > +MODULE_LICENSE("GPL");

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Zev Weiss]

On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
>From: Ban Feng <kcfeng0@nuvoton.com>
>
>NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>
>Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
>---
> Documentation/hwmon/index.rst   |   1 +
> Documentation/hwmon/nct7363.rst |  33 +++
> MAINTAINERS                     |   2 +
> drivers/hwmon/Kconfig           |  11 +
> drivers/hwmon/Makefile          |   1 +
> drivers/hwmon/nct7363.c         | 412 ++++++++++++++++++++++++++++++++
> 6 files changed, 460 insertions(+)
> create mode 100644 Documentation/hwmon/nct7363.rst
> create mode 100644 drivers/hwmon/nct7363.c
>
>diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>index c7ed1f73ac06..302f954b45be 100644
>--- a/Documentation/hwmon/index.rst
>+++ b/Documentation/hwmon/index.rst
>@@ -165,6 +165,7 @@ Hardware Monitoring Kernel Drivers
>    mp5990
>    nct6683
>    nct6775
>+   nct7363
>    nct7802
>    nct7904
>    npcm750-pwm-fan
>diff --git a/Documentation/hwmon/nct7363.rst b/Documentation/hwmon/nct7363.rst
>new file mode 100644
>index 000000000000..89699c95aa4b
>--- /dev/null
>+++ b/Documentation/hwmon/nct7363.rst
>@@ -0,0 +1,33 @@
>+.. SPDX-License-Identifier: GPL-2.0
>+
>+Kernel driver nct7363
>+=====================
>+
>+Supported chip:
>+
>+  * Nuvoton NCT7363Y
>+
>+    Prefix: nct7363
>+
>+    Addresses: I2C 0x20, 0x21, 0x22, 0x23
>+
>+Author: Ban Feng <kcfeng0@nuvoton.com>
>+
>+
>+Description
>+-----------
>+
>+The NCT7363Y is a Fan controller which provides up to 16 independent
>+FAN input monitors, and up to 16 independent PWM output with SMBus interface.
>+
>+
>+Sysfs entries
>+-------------
>+
>+Currently, the driver supports the following features:
>+
>+======================= =======================================================
>+fanX_input		provide current fan rotation value in RPM
>+
>+pwmX			get or set PWM fan control value.
>+======================= =======================================================
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 7b1efefed7c4..7ca66b713e30 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -15089,6 +15089,8 @@ M:	Ban Feng <kcfeng0@nuvoton.com>
> L:	linux-hwmon@vger.kernel.org
> S:	Maintained
> F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
>+F:	Documentation/hwmon/nct7363.rst
>+F:	drivers/hwmon/nct7363.c
>
> NETDEVSIM
> M:	Jakub Kicinski <kuba@kernel.org>
>diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>index a608264da87d..a297b5409b04 100644
>--- a/drivers/hwmon/Kconfig
>+++ b/drivers/hwmon/Kconfig
>@@ -1616,6 +1616,17 @@ config SENSORS_NCT6775_I2C
> 	  This driver can also be built as a module. If so, the module
> 	  will be called nct6775-i2c.
>
>+config SENSORS_NCT7363
>+	tristate "Nuvoton NCT7363Y"
>+	depends on I2C
>+	select REGMAP_I2C
>+	help
>+	  If you say yes here you get support for the Nuvoton NCT7363Y,
>+	  hardware monitoring chip.
>+
>+	  This driver can also be built as a module. If so, the module
>+	  will be called nct7363.
>+
> config SENSORS_NCT7802
> 	tristate "Nuvoton NCT7802Y"
> 	depends on I2C
>diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>index 47be39af5c03..d5e7531204df 100644
>--- a/drivers/hwmon/Makefile
>+++ b/drivers/hwmon/Makefile
>@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o
> nct6775-objs			:= nct6775-platform.o
> obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
> obj-$(CONFIG_SENSORS_NCT6775_I2C) += nct6775-i2c.o
>+obj-$(CONFIG_SENSORS_NCT7363)	+= nct7363.o
> obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
> obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
> obj-$(CONFIG_SENSORS_NPCM7XX)	+= npcm750-pwm-fan.o
>diff --git a/drivers/hwmon/nct7363.c b/drivers/hwmon/nct7363.c
>new file mode 100644
>index 000000000000..c79d3ca4f111
>--- /dev/null
>+++ b/drivers/hwmon/nct7363.c
>@@ -0,0 +1,412 @@
>+// SPDX-License-Identifier: GPL-2.0-or-later
>+/*
>+ * Copyright (c) 2023 Nuvoton Technology corporation.
>+ */
>+
>+#include <linux/bitfield.h>
>+#include <linux/bits.h>
>+#include <linux/err.h>
>+#include <linux/hwmon.h>
>+#include <linux/hwmon-sysfs.h>
>+#include <linux/i2c.h>
>+#include <linux/module.h>
>+#include <linux/mutex.h>
>+#include <linux/regmap.h>
>+#include <linux/slab.h>
>+
>+#define NCT7363_REG_GPIO_0_3		0x20
>+#define NCT7363_REG_GPIO_4_7		0x21
>+#define NCT7363_REG_GPIO_10_13		0x22
>+#define NCT7363_REG_GPIO_14_17		0x23
>+#define NCT7363_REG_PWMEN_0_7		0x38
>+#define NCT7363_REG_PWMEN_8_15		0x39
>+#define NCT7363_REG_FANINEN_0_7		0x41
>+#define NCT7363_REG_FANINEN_8_15	0x42
>+#define NCT7363_REG_FANINX_HVAL(x)	(0x48 + ((x) * 2))
>+#define NCT7363_REG_FANINX_LVAL(x)	(0x49 + ((x) * 2))
>+#define NCT7363_REG_FSCPXDUTY(x)	(0x90 + ((x) * 2))
>+
>+#define PWM_SEL(x)			(BIT(0) << (((x) % 4) * 2))
>+#define FANIN_SEL(x)			(BIT(1) << (((x) % 4) * 2))
>+
>+#define NCT7363_FANINX_LVAL_MASK	GENMASK(4, 0)
>+#define NCT7363_FANIN_MASK		GENMASK(12, 0)
>+
>+#define NCT7363_PWM_COUNT		16
>+
>+static inline unsigned long FAN_FROM_REG(u16 val)
>+{
>+	/* In case fan is stopped or divide by 0 */
>+	if (val == NCT7363_FANIN_MASK || val == 0)
>+		return	0;
>+
>+	return (1350000UL / val);
>+}
>+
>+static const struct of_device_id nct7363_of_match[] = {
>+	{ .compatible = "nuvoton,nct7363" },

As far as I can see from the code in this driver, it looks like this 
driver should also be compatible with the nct7362; shall we add the ID 
table entry for it now?  (Though I only have a datasheet for the 
nct7362, not the nct7363, so I don't know exactly how they differ.)

>+	{ },
>+};
>+MODULE_DEVICE_TABLE(of, nct7363_of_match);
>+
>+struct nct7363_data {
>+	struct regmap		*regmap;
>+	struct mutex		lock; /* protect register access */
>+
>+	u16			fanin_mask;
>+	u16			pwm_mask;
>+};
>+
>+static int nct7363_read_fan(struct device *dev, u32 attr, int channel,
>+			    long *val)
>+{
>+	struct nct7363_data *data = dev_get_drvdata(dev);
>+	unsigned int hi, lo;
>+	u16 cnt, rpm;
>+	int ret = 0;
>+
>+	switch (attr) {
>+	case hwmon_fan_input:
>+		/*
>+		 * High-byte register should be read first to latch
>+		 * synchronous low-byte value
>+		 */
>+		ret = regmap_read(data->regmap,
>+				  NCT7363_REG_FANINX_HVAL(channel), &hi);
>+		if (ret)
>+			return ret;
>+
>+		ret = regmap_read(data->regmap,
>+			NCT7363_REG_FANINX_LVAL(channel), &lo);
>+		if (ret)
>+			return ret;

I think this pair of reads should be done under data->lock to ensure 
that the LVAL read gets the right latched value in a scenario where 
multiple threads executing this function end up with their register 
reads interleaved.

>+
>+		cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
>+		rpm = FAN_FROM_REG(cnt);
>+		*val = (long)rpm;

Given that rpm is assigned from an unsigned long (FAN_FROM_REG()) and 
then to a long (*val), is there any reason we want it as a u16 in 
between the two?  Or for that matter, why not just:

   *val = FAN_FROM_REG(cnt);

?

>+		return 0;
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static umode_t nct7363_fan_is_visible(const void *_data, u32 attr, int channel)
>+{
>+	const struct nct7363_data *data = _data;
>+
>+	switch (attr) {
>+	case hwmon_fan_input:
>+		if (data->fanin_mask & BIT(channel))
>+			return 0444;
>+		break;
>+	default:
>+		break;
>+	}
>+
>+	return 0;
>+}
>+
>+static int nct7363_read_pwm(struct device *dev, u32 attr, int channel,
>+			    long *val)
>+{
>+	struct nct7363_data *data = dev_get_drvdata(dev);
>+	unsigned int regval;
>+	u16 ret;
>+
>+	switch (attr) {
>+	case hwmon_pwm_input:
>+		ret = regmap_read(data->regmap,
>+				  NCT7363_REG_FSCPXDUTY(channel), &regval);
>+		if (ret)
>+			return ret;
>+
>+		*val = (long)regval;
>+		return 0;
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static int nct7363_write_pwm(struct device *dev, u32 attr, int channel,
>+			     long val)
>+{
>+	struct nct7363_data *data = dev_get_drvdata(dev);
>+	int ret;
>+
>+	switch (attr) {
>+	case hwmon_pwm_input:
>+		if (val < 0 || val > 255)
>+			return -EINVAL;
>+
>+		mutex_lock(&data->lock);
>+		ret = regmap_write(data->regmap,
>+				   NCT7363_REG_FSCPXDUTY(channel), val);
>+		mutex_unlock(&data->lock);

...though here, I'm not sure the locking is needed -- is there something 
that necessitates it for a single register write?

>+
>+		return ret;
>+
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static umode_t nct7363_pwm_is_visible(const void *_data, u32 attr, int channel)
>+{
>+	const struct nct7363_data *data = _data;
>+
>+	switch (attr) {
>+	case hwmon_pwm_input:
>+		if (data->pwm_mask & BIT(channel))
>+			return 0644;
>+		break;
>+	default:
>+		break;
>+	}
>+
>+	return 0;
>+}
>+
>+static int nct7363_read(struct device *dev, enum hwmon_sensor_types type,
>+			u32 attr, int channel, long *val)
>+{
>+	switch (type) {
>+	case hwmon_fan:
>+		return nct7363_read_fan(dev, attr, channel, val);
>+	case hwmon_pwm:
>+		return nct7363_read_pwm(dev, attr, channel, val);
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static int nct7363_write(struct device *dev, enum hwmon_sensor_types type,
>+			 u32 attr, int channel, long val)
>+{
>+	switch (type) {
>+	case hwmon_pwm:
>+		return nct7363_write_pwm(dev, attr, channel, val);
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static umode_t nct7363_is_visible(const void *data,
>+				  enum hwmon_sensor_types type,
>+				  u32 attr, int channel)
>+{
>+	switch (type) {
>+	case hwmon_fan:
>+		return nct7363_fan_is_visible(data, attr, channel);
>+	case hwmon_pwm:
>+		return nct7363_pwm_is_visible(data, attr, channel);
>+	default:
>+		return 0;
>+	}
>+}
>+
>+static const struct hwmon_channel_info *nct7363_info[] = {
>+	HWMON_CHANNEL_INFO(fan,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT),
>+	HWMON_CHANNEL_INFO(pwm,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT),
>+	NULL
>+};
>+
>+static const struct hwmon_ops nct7363_hwmon_ops = {
>+	.is_visible = nct7363_is_visible,
>+	.read = nct7363_read,
>+	.write = nct7363_write,
>+};
>+
>+static const struct hwmon_chip_info nct7363_chip_info = {
>+	.ops = &nct7363_hwmon_ops,
>+	.info = nct7363_info,
>+};
>+
>+static int nct7363_init_chip(struct nct7363_data *data)
>+{
>+	u8 i, gpio0_3 = 0, gpio4_7 = 0, gpio10_13 = 0, gpio14_17 = 0;
>+	int ret;
>+
>+	for (i = 0; i < NCT7363_PWM_COUNT; i++) {
>+		if (i < 4) {
>+			if (data->pwm_mask & BIT(i))
>+				gpio0_3 |= PWM_SEL(i);
>+			if (data->fanin_mask & BIT(i))
>+				gpio10_13 |= FANIN_SEL(i);
>+		} else if (i < 8) {
>+			if (data->pwm_mask & BIT(i))
>+				gpio4_7 |= PWM_SEL(i);
>+			if (data->fanin_mask & BIT(i))
>+				gpio14_17 |= FANIN_SEL(i);
>+		} else if (i < 12) {
>+			if (data->pwm_mask & BIT(i))
>+				gpio10_13 |= PWM_SEL(i);
>+			if (data->fanin_mask & BIT(i))
>+				gpio0_3 |= FANIN_SEL(i);
>+		} else {
>+			if (data->pwm_mask & BIT(i))
>+				gpio14_17 |= PWM_SEL(i);
>+			if (data->fanin_mask & BIT(i))
>+				gpio4_7 |= FANIN_SEL(i);
>+		}
>+	}

With the caveat that I haven't actually sketched it out myself to be 
sure, might it be a bit cleaner to do this with a 4-element array 
indexed by 'i / 4' instead of a big if/else chain and a bunch of 
near-duplicated blocks?

>+
>+	/* Pin Function Configuration */
>+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_0_3, gpio0_3);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_4_7, gpio4_7);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_10_13, gpio10_13);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_14_17, gpio14_17);
>+	if (ret < 0)
>+		return ret;
>+
>+	/* PWM and FANIN Monitoring Enable */
>+	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_0_7,
>+			   data->pwm_mask & 0xff);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_8_15,
>+			   (data->pwm_mask >> 8) & 0xff);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_0_7,
>+			   data->fanin_mask & 0xff);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_8_15,
>+			   (data->fanin_mask >> 8) & 0xff);
>+	if (ret < 0)
>+		return ret;
>+
>+	return 0;
>+}
>+
>+static int nct7363_present_pwm_fanin(struct device *dev,
>+				     struct device_node *child,
>+				     struct nct7363_data *data)
>+{
>+	struct of_phandle_args args;
>+	int ret, fanin_cnt;
>+	u8 *fanin_ch;
>+	u8 ch, index;
>+
>+	ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
>+					 0, &args);
>+	if (ret)
>+		return ret;
>+
>+	data->pwm_mask |= BIT(args.args[0]);

Perhaps we should have

   if (args.args[0] >= NCT7363_PWM_COUNT)
     return -EINVAL;

here?

>+
>+	fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
>+	if (fanin_cnt < 1)

fanin_cnt < 1 || fanin_cnt >= NCT7363_PWM_COUNT

>+		return -EINVAL;
>+
>+	fanin_ch = devm_kcalloc(dev, fanin_cnt, sizeof(*fanin_ch), GFP_KERNEL);

Keeping this allocation around persistently for the whole lifetime of 
the device seems unnecessary -- it's not used beyond this function, 
right?  In fact, dynamically allocating it at all seems like overkill, 
considering that in addition to being temporary, it's also got a pretty 
small upper bound on its size.  I'd think a simple

   u8 fanin_ch[NCT7363_PWM_COUNT];

would suffice?  (Along with the check above to ensure fanin_cnt is 
within range of course.)

>+	if (!fanin_ch)
>+		return -ENOMEM;
>+
>+	ret = of_property_read_u8_array(child, "tach-ch", fanin_ch, fanin_cnt);
>+	if (ret)
>+		return ret;
>+
>+	for (ch = 0; ch < fanin_cnt; ch++) {
>+		index = fanin_ch[ch];

...and likewise a range check here too?

>+		data->fanin_mask |= BIT(index);
>+	}
>+

Should we also grab the pulses-per-revolution property here and factor 
that in in FAN_FROM_REG()?

>+	return 0;
>+}
>+
>+static const struct regmap_config nct7363_regmap_config = {
>+	.reg_bits = 8,
>+	.val_bits = 8,
>+};
>+
>+static int nct7363_probe(struct i2c_client *client)
>+{
>+	struct device *dev = &client->dev;
>+	struct device_node *child;
>+	struct nct7363_data *data;
>+	struct device *hwmon_dev;
>+	int ret;
>+
>+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>+	if (!data)
>+		return -ENOMEM;
>+
>+	data->regmap = devm_regmap_init_i2c(client, &nct7363_regmap_config);
>+	if (IS_ERR(data->regmap))
>+		return PTR_ERR(data->regmap);
>+
>+	mutex_init(&data->lock);
>+
>+	for_each_child_of_node(dev->of_node, child) {
>+		ret = nct7363_present_pwm_fanin(dev, child, data);
>+		if (ret) {
>+			of_node_put(child);
>+			return ret;
>+		}
>+	}
>+
>+	/* Initialize the chip */
>+	ret = nct7363_init_chip(data);
>+	if (ret)
>+		return ret;
>+
>+	hwmon_dev =
>+		devm_hwmon_device_register_with_info(dev, client->name, data,
>+						     &nct7363_chip_info, NULL);
>+	return PTR_ERR_OR_ZERO(hwmon_dev);
>+}
>+
>+static struct i2c_driver nct7363_driver = {
>+	.class = I2C_CLASS_HWMON,

Maybe add an i2c_device_id table to accompany the of_match table?

>+	.driver = {
>+		.name = "nct7363",
>+		.of_match_table = nct7363_of_match,
>+	},
>+	.probe = nct7363_probe,
>+};
>+
>+module_i2c_driver(nct7363_driver);
>+
>+MODULE_AUTHOR("CW Ho <cwho@nuvoton.com>");
>+MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
>+MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
>+MODULE_LICENSE("GPL");
>-- 
>2.34.1
>
>

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Zev Weiss]

On Sat, Mar 02, 2024 at 12:19:07AM PST, Zev Weiss wrote:
>On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:

<snip>

>
>>+
>>+	fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
>>+	if (fanin_cnt < 1)
>
>fanin_cnt < 1 || fanin_cnt >= NCT7363_PWM_COUNT
>

Er, off by one -- just '>' rather than '>=' there I realize now.


Zev

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Ban Feng]

Hi Zev,

On Tue, Mar 5, 2024 at 8:28 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Sat, Mar 02, 2024 at 12:19:07AM PST, Zev Weiss wrote:
> >On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
>
> <snip>
>
> >
> >>+
> >>+     fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
> >>+     if (fanin_cnt < 1)
> >
> >fanin_cnt < 1 || fanin_cnt >= NCT7363_PWM_COUNT
> >
>
> Er, off by one -- just '>' rather than '>=' there I realize now.

ok, fix in v5

Thanks,
Ban

>
>
> Zev
>

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Ban Feng]

Hi Zev,

On Sat, Mar 2, 2024 at 4:19 PM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
> >From: Ban Feng <kcfeng0@nuvoton.com>
> >
> >NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
> >
> >Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> >---
> > Documentation/hwmon/index.rst   |   1 +
> > Documentation/hwmon/nct7363.rst |  33 +++
> > MAINTAINERS                     |   2 +
> > drivers/hwmon/Kconfig           |  11 +
> > drivers/hwmon/Makefile          |   1 +
> > drivers/hwmon/nct7363.c         | 412 ++++++++++++++++++++++++++++++++
> > 6 files changed, 460 insertions(+)
> > create mode 100644 Documentation/hwmon/nct7363.rst
> > create mode 100644 drivers/hwmon/nct7363.c
> >
> >diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> >index c7ed1f73ac06..302f954b45be 100644
> >--- a/Documentation/hwmon/index.rst
> >+++ b/Documentation/hwmon/index.rst
> >@@ -165,6 +165,7 @@ Hardware Monitoring Kernel Drivers
> >    mp5990
> >    nct6683
> >    nct6775
> >+   nct7363
> >    nct7802
> >    nct7904
> >    npcm750-pwm-fan
> >diff --git a/Documentation/hwmon/nct7363.rst b/Documentation/hwmon/nct7363.rst
> >new file mode 100644
> >index 000000000000..89699c95aa4b
> >--- /dev/null
> >+++ b/Documentation/hwmon/nct7363.rst
> >@@ -0,0 +1,33 @@
> >+.. SPDX-License-Identifier: GPL-2.0
> >+
> >+Kernel driver nct7363
> >+=====================
> >+
> >+Supported chip:
> >+
> >+  * Nuvoton NCT7363Y
> >+
> >+    Prefix: nct7363
> >+
> >+    Addresses: I2C 0x20, 0x21, 0x22, 0x23
> >+
> >+Author: Ban Feng <kcfeng0@nuvoton.com>
> >+
> >+
> >+Description
> >+-----------
> >+
> >+The NCT7363Y is a Fan controller which provides up to 16 independent
> >+FAN input monitors, and up to 16 independent PWM output with SMBus interface.
> >+
> >+
> >+Sysfs entries
> >+-------------
> >+
> >+Currently, the driver supports the following features:
> >+
> >+======================= =======================================================
> >+fanX_input            provide current fan rotation value in RPM
> >+
> >+pwmX                  get or set PWM fan control value.
> >+======================= =======================================================
> >diff --git a/MAINTAINERS b/MAINTAINERS
> >index 7b1efefed7c4..7ca66b713e30 100644
> >--- a/MAINTAINERS
> >+++ b/MAINTAINERS
> >@@ -15089,6 +15089,8 @@ M:     Ban Feng <kcfeng0@nuvoton.com>
> > L:    linux-hwmon@vger.kernel.org
> > S:    Maintained
> > F:    Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> >+F:    Documentation/hwmon/nct7363.rst
> >+F:    drivers/hwmon/nct7363.c
> >
> > NETDEVSIM
> > M:    Jakub Kicinski <kuba@kernel.org>
> >diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >index a608264da87d..a297b5409b04 100644
> >--- a/drivers/hwmon/Kconfig
> >+++ b/drivers/hwmon/Kconfig
> >@@ -1616,6 +1616,17 @@ config SENSORS_NCT6775_I2C
> >         This driver can also be built as a module. If so, the module
> >         will be called nct6775-i2c.
> >
> >+config SENSORS_NCT7363
> >+      tristate "Nuvoton NCT7363Y"
> >+      depends on I2C
> >+      select REGMAP_I2C
> >+      help
> >+        If you say yes here you get support for the Nuvoton NCT7363Y,
> >+        hardware monitoring chip.
> >+
> >+        This driver can also be built as a module. If so, the module
> >+        will be called nct7363.
> >+
> > config SENSORS_NCT7802
> >       tristate "Nuvoton NCT7802Y"
> >       depends on I2C
> >diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> >index 47be39af5c03..d5e7531204df 100644
> >--- a/drivers/hwmon/Makefile
> >+++ b/drivers/hwmon/Makefile
> >@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o
> > nct6775-objs                  := nct6775-platform.o
> > obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o
> > obj-$(CONFIG_SENSORS_NCT6775_I2C) += nct6775-i2c.o
> >+obj-$(CONFIG_SENSORS_NCT7363) += nct7363.o
> > obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o
> > obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o
> > obj-$(CONFIG_SENSORS_NPCM7XX) += npcm750-pwm-fan.o
> >diff --git a/drivers/hwmon/nct7363.c b/drivers/hwmon/nct7363.c
> >new file mode 100644
> >index 000000000000..c79d3ca4f111
> >--- /dev/null
> >+++ b/drivers/hwmon/nct7363.c
> >@@ -0,0 +1,412 @@
> >+// SPDX-License-Identifier: GPL-2.0-or-later
> >+/*
> >+ * Copyright (c) 2023 Nuvoton Technology corporation.
> >+ */
> >+
> >+#include <linux/bitfield.h>
> >+#include <linux/bits.h>
> >+#include <linux/err.h>
> >+#include <linux/hwmon.h>
> >+#include <linux/hwmon-sysfs.h>
> >+#include <linux/i2c.h>
> >+#include <linux/module.h>
> >+#include <linux/mutex.h>
> >+#include <linux/regmap.h>
> >+#include <linux/slab.h>
> >+
> >+#define NCT7363_REG_GPIO_0_3          0x20
> >+#define NCT7363_REG_GPIO_4_7          0x21
> >+#define NCT7363_REG_GPIO_10_13                0x22
> >+#define NCT7363_REG_GPIO_14_17                0x23
> >+#define NCT7363_REG_PWMEN_0_7         0x38
> >+#define NCT7363_REG_PWMEN_8_15                0x39
> >+#define NCT7363_REG_FANINEN_0_7               0x41
> >+#define NCT7363_REG_FANINEN_8_15      0x42
> >+#define NCT7363_REG_FANINX_HVAL(x)    (0x48 + ((x) * 2))
> >+#define NCT7363_REG_FANINX_LVAL(x)    (0x49 + ((x) * 2))
> >+#define NCT7363_REG_FSCPXDUTY(x)      (0x90 + ((x) * 2))
> >+
> >+#define PWM_SEL(x)                    (BIT(0) << (((x) % 4) * 2))
> >+#define FANIN_SEL(x)                  (BIT(1) << (((x) % 4) * 2))
> >+
> >+#define NCT7363_FANINX_LVAL_MASK      GENMASK(4, 0)
> >+#define NCT7363_FANIN_MASK            GENMASK(12, 0)
> >+
> >+#define NCT7363_PWM_COUNT             16
> >+
> >+static inline unsigned long FAN_FROM_REG(u16 val)
> >+{
> >+      /* In case fan is stopped or divide by 0 */
> >+      if (val == NCT7363_FANIN_MASK || val == 0)
> >+              return  0;
> >+
> >+      return (1350000UL / val);
> >+}
> >+
> >+static const struct of_device_id nct7363_of_match[] = {
> >+      { .compatible = "nuvoton,nct7363" },
>
> As far as I can see from the code in this driver, it looks like this
> driver should also be compatible with the nct7362; shall we add the ID
> table entry for it now?  (Though I only have a datasheet for the
> nct7362, not the nct7363, so I don't know exactly how they differ.)

As far as I know, the difference between these two ICs is 0x2A~0x2C
Fading LED for 7362, and 0x2A Watchdog Timer for 7363.
In my v1 patch, I indeed add the nct7362 to the ID table, however,
this makes it more complicated and our datasheet isn't public yet.
I think maybe supporting more chips will be done in the future, but not now.

>
> >+      { },
> >+};
> >+MODULE_DEVICE_TABLE(of, nct7363_of_match);
> >+
> >+struct nct7363_data {
> >+      struct regmap           *regmap;
> >+      struct mutex            lock; /* protect register access */
> >+
> >+      u16                     fanin_mask;
> >+      u16                     pwm_mask;
> >+};
> >+
> >+static int nct7363_read_fan(struct device *dev, u32 attr, int channel,
> >+                          long *val)
> >+{
> >+      struct nct7363_data *data = dev_get_drvdata(dev);
> >+      unsigned int hi, lo;
> >+      u16 cnt, rpm;
> >+      int ret = 0;
> >+
> >+      switch (attr) {
> >+      case hwmon_fan_input:
> >+              /*
> >+               * High-byte register should be read first to latch
> >+               * synchronous low-byte value
> >+               */
> >+              ret = regmap_read(data->regmap,
> >+                                NCT7363_REG_FANINX_HVAL(channel), &hi);
> >+              if (ret)
> >+                      return ret;
> >+
> >+              ret = regmap_read(data->regmap,
> >+                      NCT7363_REG_FANINX_LVAL(channel), &lo);
> >+              if (ret)
> >+                      return ret;
>
> I think this pair of reads should be done under data->lock to ensure
> that the LVAL read gets the right latched value in a scenario where
> multiple threads executing this function end up with their register
> reads interleaved.

ok, fix in v5

>
> >+
> >+              cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
> >+              rpm = FAN_FROM_REG(cnt);
> >+              *val = (long)rpm;
>
> Given that rpm is assigned from an unsigned long (FAN_FROM_REG()) and
> then to a long (*val), is there any reason we want it as a u16 in
> between the two?  Or for that matter, why not just:
>
>    *val = FAN_FROM_REG(cnt);
>
> ?

I'll modify this to align with the style of the nct7363_read_pwm function in v5.

>
> >+              return 0;
> >+      default:
> >+              return -EOPNOTSUPP;
> >+      }
> >+}
> >+
> >+static umode_t nct7363_fan_is_visible(const void *_data, u32 attr, int channel)
> >+{
> >+      const struct nct7363_data *data = _data;
> >+
> >+      switch (attr) {
> >+      case hwmon_fan_input:
> >+              if (data->fanin_mask & BIT(channel))
> >+                      return 0444;
> >+              break;
> >+      default:
> >+              break;
> >+      }
> >+
> >+      return 0;
> >+}
> >+
> >+static int nct7363_read_pwm(struct device *dev, u32 attr, int channel,
> >+                          long *val)
> >+{
> >+      struct nct7363_data *data = dev_get_drvdata(dev);
> >+      unsigned int regval;
> >+      u16 ret;
> >+
> >+      switch (attr) {
> >+      case hwmon_pwm_input:
> >+              ret = regmap_read(data->regmap,
> >+                                NCT7363_REG_FSCPXDUTY(channel), &regval);
> >+              if (ret)
> >+                      return ret;
> >+
> >+              *val = (long)regval;
> >+              return 0;
> >+      default:
> >+              return -EOPNOTSUPP;
> >+      }
> >+}
> >+
> >+static int nct7363_write_pwm(struct device *dev, u32 attr, int channel,
> >+                           long val)
> >+{
> >+      struct nct7363_data *data = dev_get_drvdata(dev);
> >+      int ret;
> >+
> >+      switch (attr) {
> >+      case hwmon_pwm_input:
> >+              if (val < 0 || val > 255)
> >+                      return -EINVAL;
> >+
> >+              mutex_lock(&data->lock);
> >+              ret = regmap_write(data->regmap,
> >+                                 NCT7363_REG_FSCPXDUTY(channel), val);
> >+              mutex_unlock(&data->lock);
>
> ...though here, I'm not sure the locking is needed -- is there something
> that necessitates it for a single register write?

ok, fix in v5

>
> >+
> >+              return ret;
> >+
> >+      default:
> >+              return -EOPNOTSUPP;
> >+      }
> >+}
> >+
> >+static umode_t nct7363_pwm_is_visible(const void *_data, u32 attr, int channel)
> >+{
> >+      const struct nct7363_data *data = _data;
> >+
> >+      switch (attr) {
> >+      case hwmon_pwm_input:
> >+              if (data->pwm_mask & BIT(channel))
> >+                      return 0644;
> >+              break;
> >+      default:
> >+              break;
> >+      }
> >+
> >+      return 0;
> >+}
> >+
> >+static int nct7363_read(struct device *dev, enum hwmon_sensor_types type,
> >+                      u32 attr, int channel, long *val)
> >+{
> >+      switch (type) {
> >+      case hwmon_fan:
> >+              return nct7363_read_fan(dev, attr, channel, val);
> >+      case hwmon_pwm:
> >+              return nct7363_read_pwm(dev, attr, channel, val);
> >+      default:
> >+              return -EOPNOTSUPP;
> >+      }
> >+}
> >+
> >+static int nct7363_write(struct device *dev, enum hwmon_sensor_types type,
> >+                       u32 attr, int channel, long val)
> >+{
> >+      switch (type) {
> >+      case hwmon_pwm:
> >+              return nct7363_write_pwm(dev, attr, channel, val);
> >+      default:
> >+              return -EOPNOTSUPP;
> >+      }
> >+}
> >+
> >+static umode_t nct7363_is_visible(const void *data,
> >+                                enum hwmon_sensor_types type,
> >+                                u32 attr, int channel)
> >+{
> >+      switch (type) {
> >+      case hwmon_fan:
> >+              return nct7363_fan_is_visible(data, attr, channel);
> >+      case hwmon_pwm:
> >+              return nct7363_pwm_is_visible(data, attr, channel);
> >+      default:
> >+              return 0;
> >+      }
> >+}
> >+
> >+static const struct hwmon_channel_info *nct7363_info[] = {
> >+      HWMON_CHANNEL_INFO(fan,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT,
> >+                         HWMON_F_INPUT),
> >+      HWMON_CHANNEL_INFO(pwm,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT,
> >+                         HWMON_PWM_INPUT),
> >+      NULL
> >+};
> >+
> >+static const struct hwmon_ops nct7363_hwmon_ops = {
> >+      .is_visible = nct7363_is_visible,
> >+      .read = nct7363_read,
> >+      .write = nct7363_write,
> >+};
> >+
> >+static const struct hwmon_chip_info nct7363_chip_info = {
> >+      .ops = &nct7363_hwmon_ops,
> >+      .info = nct7363_info,
> >+};
> >+
> >+static int nct7363_init_chip(struct nct7363_data *data)
> >+{
> >+      u8 i, gpio0_3 = 0, gpio4_7 = 0, gpio10_13 = 0, gpio14_17 = 0;
> >+      int ret;
> >+
> >+      for (i = 0; i < NCT7363_PWM_COUNT; i++) {
> >+              if (i < 4) {
> >+                      if (data->pwm_mask & BIT(i))
> >+                              gpio0_3 |= PWM_SEL(i);
> >+                      if (data->fanin_mask & BIT(i))
> >+                              gpio10_13 |= FANIN_SEL(i);
> >+              } else if (i < 8) {
> >+                      if (data->pwm_mask & BIT(i))
> >+                              gpio4_7 |= PWM_SEL(i);
> >+                      if (data->fanin_mask & BIT(i))
> >+                              gpio14_17 |= FANIN_SEL(i);
> >+              } else if (i < 12) {
> >+                      if (data->pwm_mask & BIT(i))
> >+                              gpio10_13 |= PWM_SEL(i);
> >+                      if (data->fanin_mask & BIT(i))
> >+                              gpio0_3 |= FANIN_SEL(i);
> >+              } else {
> >+                      if (data->pwm_mask & BIT(i))
> >+                              gpio14_17 |= PWM_SEL(i);
> >+                      if (data->fanin_mask & BIT(i))
> >+                              gpio4_7 |= FANIN_SEL(i);
> >+              }
> >+      }
>
> With the caveat that I haven't actually sketched it out myself to be
> sure, might it be a bit cleaner to do this with a 4-element array
> indexed by 'i / 4' instead of a big if/else chain and a bunch of
> near-duplicated blocks?

ok, fix in v5

>
> >+
> >+      /* Pin Function Configuration */
> >+      ret = regmap_write(data->regmap, NCT7363_REG_GPIO_0_3, gpio0_3);
> >+      if (ret < 0)
> >+              return ret;
> >+      ret = regmap_write(data->regmap, NCT7363_REG_GPIO_4_7, gpio4_7);
> >+      if (ret < 0)
> >+              return ret;
> >+      ret = regmap_write(data->regmap, NCT7363_REG_GPIO_10_13, gpio10_13);
> >+      if (ret < 0)
> >+              return ret;
> >+      ret = regmap_write(data->regmap, NCT7363_REG_GPIO_14_17, gpio14_17);
> >+      if (ret < 0)
> >+              return ret;
> >+
> >+      /* PWM and FANIN Monitoring Enable */
> >+      ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_0_7,
> >+                         data->pwm_mask & 0xff);
> >+      if (ret < 0)
> >+              return ret;
> >+      ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_8_15,
> >+                         (data->pwm_mask >> 8) & 0xff);
> >+      if (ret < 0)
> >+              return ret;
> >+      ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_0_7,
> >+                         data->fanin_mask & 0xff);
> >+      if (ret < 0)
> >+              return ret;
> >+      ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_8_15,
> >+                         (data->fanin_mask >> 8) & 0xff);
> >+      if (ret < 0)
> >+              return ret;
> >+
> >+      return 0;
> >+}
> >+
> >+static int nct7363_present_pwm_fanin(struct device *dev,
> >+                                   struct device_node *child,
> >+                                   struct nct7363_data *data)
> >+{
> >+      struct of_phandle_args args;
> >+      int ret, fanin_cnt;
> >+      u8 *fanin_ch;
> >+      u8 ch, index;
> >+
> >+      ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
> >+                                       0, &args);
> >+      if (ret)
> >+              return ret;
> >+
> >+      data->pwm_mask |= BIT(args.args[0]);
>
> Perhaps we should have
>
>    if (args.args[0] >= NCT7363_PWM_COUNT)
>      return -EINVAL;
>
> here?

ok, fix in v5

>
> >+
> >+      fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
> >+      if (fanin_cnt < 1)
>
> fanin_cnt < 1 || fanin_cnt >= NCT7363_PWM_COUNT

ok, fix in v5

>
> >+              return -EINVAL;
> >+
> >+      fanin_ch = devm_kcalloc(dev, fanin_cnt, sizeof(*fanin_ch), GFP_KERNEL);
>
> Keeping this allocation around persistently for the whole lifetime of
> the device seems unnecessary -- it's not used beyond this function,
> right?  In fact, dynamically allocating it at all seems like overkill,
> considering that in addition to being temporary, it's also got a pretty
> small upper bound on its size.  I'd think a simple
>
>    u8 fanin_ch[NCT7363_PWM_COUNT];
>
> would suffice?  (Along with the check above to ensure fanin_cnt is
> within range of course.)

ok, fix in v5

>
> >+      if (!fanin_ch)
> >+              return -ENOMEM;
> >+
> >+      ret = of_property_read_u8_array(child, "tach-ch", fanin_ch, fanin_cnt);
> >+      if (ret)
> >+              return ret;
> >+
> >+      for (ch = 0; ch < fanin_cnt; ch++) {
> >+              index = fanin_ch[ch];
>
> ...and likewise a range check here too?

ok, fix in v5

>
> >+              data->fanin_mask |= BIT(index);
> >+      }
> >+
>
> Should we also grab the pulses-per-revolution property here and factor
> that in in FAN_FROM_REG()?

So far our FanPoles in tachometer count calculation formula is always 4.
Therefore, I don't add the pulses-per-revolution property into FAN_FROM_REG().

>
> >+      return 0;
> >+}
> >+
> >+static const struct regmap_config nct7363_regmap_config = {
> >+      .reg_bits = 8,
> >+      .val_bits = 8,
> >+};
> >+
> >+static int nct7363_probe(struct i2c_client *client)
> >+{
> >+      struct device *dev = &client->dev;
> >+      struct device_node *child;
> >+      struct nct7363_data *data;
> >+      struct device *hwmon_dev;
> >+      int ret;
> >+
> >+      data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >+      if (!data)
> >+              return -ENOMEM;
> >+
> >+      data->regmap = devm_regmap_init_i2c(client, &nct7363_regmap_config);
> >+      if (IS_ERR(data->regmap))
> >+              return PTR_ERR(data->regmap);
> >+
> >+      mutex_init(&data->lock);
> >+
> >+      for_each_child_of_node(dev->of_node, child) {
> >+              ret = nct7363_present_pwm_fanin(dev, child, data);
> >+              if (ret) {
> >+                      of_node_put(child);
> >+                      return ret;
> >+              }
> >+      }
> >+
> >+      /* Initialize the chip */
> >+      ret = nct7363_init_chip(data);
> >+      if (ret)
> >+              return ret;
> >+
> >+      hwmon_dev =
> >+              devm_hwmon_device_register_with_info(dev, client->name, data,
> >+                                                   &nct7363_chip_info, NULL);
> >+      return PTR_ERR_OR_ZERO(hwmon_dev);
> >+}
> >+
> >+static struct i2c_driver nct7363_driver = {
> >+      .class = I2C_CLASS_HWMON,
>
> Maybe add an i2c_device_id table to accompany the of_match table?

ok, fix in v5

Thanks,
Ban

>
> >+      .driver = {
> >+              .name = "nct7363",
> >+              .of_match_table = nct7363_of_match,
> >+      },
> >+      .probe = nct7363_probe,
> >+};
> >+
> >+module_i2c_driver(nct7363_driver);
> >+
> >+MODULE_AUTHOR("CW Ho <cwho@nuvoton.com>");
> >+MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
> >+MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
> >+MODULE_LICENSE("GPL");
> >--
> >2.34.1
> >
> >

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Zev Weiss]

On Wed, Mar 06, 2024 at 11:35:31PM PST, Ban Feng wrote:
>Hi Zev,
>
>On Sat, Mar 2, 2024 at 4:19 PM Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
>> >From: Ban Feng <kcfeng0@nuvoton.com>
>> >
>> >NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>> >
>> >Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
>> >---

<snip>

>> >+
>> >+static const struct of_device_id nct7363_of_match[] = {
>> >+      { .compatible = "nuvoton,nct7363" },
>>
>> As far as I can see from the code in this driver, it looks like this
>> driver should also be compatible with the nct7362; shall we add the ID
>> table entry for it now?  (Though I only have a datasheet for the
>> nct7362, not the nct7363, so I don't know exactly how they differ.)
>
>As far as I know, the difference between these two ICs is 0x2A~0x2C
>Fading LED for 7362, and 0x2A Watchdog Timer for 7363.
>In my v1 patch, I indeed add the nct7362 to the ID table, however,
>this makes it more complicated and our datasheet isn't public yet.
>I think maybe supporting more chips will be done in the future, but not now.
>

If the only differences are in features the driver doesn't utilize, I'm 
not clear on how it adds any complexity.  As far as I'm aware, neither 
datasheet is public (NCT7363 nor NCT7362), so if we're going to have a 
public driver for one, why not also do so for the other?  It's a single 
additional line -- and furthermore, having made that change and tested 
it out, I can report that the driver seems to work just fine on NCT7362 
hardware as well.


Zev

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Guenter Roeck]

On 3/12/24 16:18, Zev Weiss wrote:
> On Wed, Mar 06, 2024 at 11:35:31PM PST, Ban Feng wrote:
>> Hi Zev,
>>
>> On Sat, Mar 2, 2024 at 4:19 PM Zev Weiss <zev@bewilderbeest.net> wrote:
>>>
>>> On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
>>> >From: Ban Feng <kcfeng0@nuvoton.com>
>>> >
>>> >NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>>> >
>>> >Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
>>> >---
> 
> <snip>
> 
>>> >+
>>> >+static const struct of_device_id nct7363_of_match[] = {
>>> >+      { .compatible = "nuvoton,nct7363" },
>>>
>>> As far as I can see from the code in this driver, it looks like this
>>> driver should also be compatible with the nct7362; shall we add the ID
>>> table entry for it now?  (Though I only have a datasheet for the
>>> nct7362, not the nct7363, so I don't know exactly how they differ.)
>>
>> As far as I know, the difference between these two ICs is 0x2A~0x2C
>> Fading LED for 7362, and 0x2A Watchdog Timer for 7363.
>> In my v1 patch, I indeed add the nct7362 to the ID table, however,
>> this makes it more complicated and our datasheet isn't public yet.
>> I think maybe supporting more chips will be done in the future, but not now.
>>
> 
> If the only differences are in features the driver doesn't utilize, I'm not clear on how it adds any complexity.  As far as I'm aware, neither datasheet is public (NCT7363 nor NCT7362), so if we're going to have a public driver for one, why not also do so for the other?  It's a single additional line -- and furthermore, having made that change and tested it out, I can report that the driver seems to work just fine on NCT7362 hardware as well.
> 

"if we're going to have a public driver for one, why not also do so for the other"

If you are trying to say that there should be two separate drivers, sorry, that
would be absolutely unacceptable.

Guenter

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Zev Weiss]

On Tue, Mar 12, 2024 at 04:58:12PM PDT, Guenter Roeck wrote:
>On 3/12/24 16:18, Zev Weiss wrote:
>>On Wed, Mar 06, 2024 at 11:35:31PM PST, Ban Feng wrote:
>>>Hi Zev,
>>>
>>>On Sat, Mar 2, 2024 at 4:19 PM Zev Weiss <zev@bewilderbeest.net> wrote:
>>>>
>>>>On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
>>>>>From: Ban Feng <kcfeng0@nuvoton.com>
>>>>>
>>>>>NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>>>>>
>>>>>Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
>>>>>---
>>
>><snip>
>>
>>>>>+
>>>>>+static const struct of_device_id nct7363_of_match[] = {
>>>>>+      { .compatible = "nuvoton,nct7363" },
>>>>
>>>>As far as I can see from the code in this driver, it looks like this
>>>>driver should also be compatible with the nct7362; shall we add the ID
>>>>table entry for it now?  (Though I only have a datasheet for the
>>>>nct7362, not the nct7363, so I don't know exactly how they differ.)
>>>
>>>As far as I know, the difference between these two ICs is 0x2A~0x2C
>>>Fading LED for 7362, and 0x2A Watchdog Timer for 7363.
>>>In my v1 patch, I indeed add the nct7362 to the ID table, however,
>>>this makes it more complicated and our datasheet isn't public yet.
>>>I think maybe supporting more chips will be done in the future, but not now.
>>>
>>
>>If the only differences are in features the driver doesn't utilize, I'm not clear on how it adds any complexity.  As far as I'm aware, neither datasheet is public (NCT7363 nor NCT7362), so if we're going to have a public driver for one, why not also do so for the other?  It's a single additional line -- and furthermore, having made that change and tested it out, I can report that the driver seems to work just fine on NCT7362 hardware as well.
>>
>
>"if we're going to have a public driver for one, why not also do so for the other"
>
>If you are trying to say that there should be two separate drivers, sorry, that
>would be absolutely unacceptable.
>

Sorry if that was unclear -- it was very much *not* my intent to suggest 
adding a separate driver, merely that we make the nct7363 driver also 
support the nct7362.


Zev

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Ban Feng]

HI Guenter and Zev,

If there's no concern about supporting nct7362 in nct7363 driver,
I'll add it to the of_device_id and i2c_device_id table in v5.

Thanks,
Ban

On Wed, Mar 13, 2024 at 8:21 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Tue, Mar 12, 2024 at 04:58:12PM PDT, Guenter Roeck wrote:
> >On 3/12/24 16:18, Zev Weiss wrote:
> >>On Wed, Mar 06, 2024 at 11:35:31PM PST, Ban Feng wrote:
> >>>Hi Zev,
> >>>
> >>>On Sat, Mar 2, 2024 at 4:19 PM Zev Weiss <zev@bewilderbeest.net> wrote:
> >>>>
> >>>>On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
> >>>>>From: Ban Feng <kcfeng0@nuvoton.com>
> >>>>>
> >>>>>NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
> >>>>>
> >>>>>Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> >>>>>---
> >>
> >><snip>
> >>
> >>>>>+
> >>>>>+static const struct of_device_id nct7363_of_match[] = {
> >>>>>+      { .compatible = "nuvoton,nct7363" },
> >>>>
> >>>>As far as I can see from the code in this driver, it looks like this
> >>>>driver should also be compatible with the nct7362; shall we add the ID
> >>>>table entry for it now?  (Though I only have a datasheet for the
> >>>>nct7362, not the nct7363, so I don't know exactly how they differ.)
> >>>
> >>>As far as I know, the difference between these two ICs is 0x2A~0x2C
> >>>Fading LED for 7362, and 0x2A Watchdog Timer for 7363.
> >>>In my v1 patch, I indeed add the nct7362 to the ID table, however,
> >>>this makes it more complicated and our datasheet isn't public yet.
> >>>I think maybe supporting more chips will be done in the future, but not now.
> >>>
> >>
> >>If the only differences are in features the driver doesn't utilize, I'm not clear on how it adds any complexity.  As far as I'm aware, neither datasheet is public (NCT7363 nor NCT7362), so if we're going to have a public driver for one, why not also do so for the other?  It's a single additional line -- and furthermore, having made that change and tested it out, I can report that the driver seems to work just fine on NCT7362 hardware as well.
> >>
> >
> >"if we're going to have a public driver for one, why not also do so for the other"
> >
> >If you are trying to say that there should be two separate drivers, sorry, that
> >would be absolutely unacceptable.
> >
>
> Sorry if that was unclear -- it was very much *not* my intent to suggest
> adding a separate driver, merely that we make the nct7363 driver also
> support the nct7362.
>
>
> Zev
>

Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y

[Zev Weiss]

On Sun, Mar 17, 2024 at 06:02:42PM PDT, Ban Feng wrote:
>HI Guenter and Zev,
>
>If there's no concern about supporting nct7362 in nct7363 driver,
>I'll add it to the of_device_id and i2c_device_id table in v5.
>
>Thanks,
>Ban
>

That sounds good to me, thanks.


Zev