[PATCH v3 0/4] hwmon: aspeed2600-pwm-tacho: Add driver support

[PATCH v3 0/4] hwmon: aspeed2600-pwm-tacho: Add driver support

[Troy Lee]

Aspeed AST2600 is a server management SoC which has 16 PWM channels and
16 fan tacho channel.

This series of patch provides AST2600 PWM/Fan tacho support in hwmon
class.

The driver provides a sysfs interface, and user can configure PWM duty
cycle and read current FAN speed in RPM.

Changes since v2:
 - declare local function as static function
 - fixed dt binding yamllint warnings/errors
 - rename dt property property name from pwm-freq to pwm-freq-hz

Changes since v1:
 - dt-binding rewrote with dt schema format
 - register hwmon driver with devm_hwmon_device_register_with_info()
 - moving default configurations to device tree

Troy Lee (4):
  dt-bindings: hwmon: Add Aspeed AST2600 PWM/Fan
  hwmon: Add Aspeed AST2600 support
  ARM: dts: aspeed: Add Aspeed AST2600 PWM/Fan node in devicetree
  hwmon: Support Aspeed AST2600 PWM/Fan tachometer

 .../hwmon/aspeed,ast2600-pwm-tachometer.yaml  | 131 +++
 .../hwmon/aspeed2600-pwm-tachometer.rst       |  27 +
 Documentation/hwmon/index.rst                 |   1 +
 arch/arm/boot/dts/aspeed-ast2600-evb.dts      | 152 ++++
 arch/arm/boot/dts/aspeed-g6.dtsi              |  10 +
 drivers/hwmon/Kconfig                         |  10 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/aspeed2600-pwm-tacho.c          | 756 ++++++++++++++++++
 8 files changed, 1088 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
 create mode 100644 Documentation/hwmon/aspeed2600-pwm-tachometer.rst
 create mode 100644 drivers/hwmon/aspeed2600-pwm-tacho.c


base-commit: 1d011777cdbe7ae38a854a0cbeb6bdfbf724cce0
-- 
2.25.1

[PATCH v3 1/4] dt-bindings: hwmon: Add Aspeed AST2600 PWM/Fan

[Troy Lee]

We add binding for supporting a new AST2600 PWM/Fan hwmon driver.

Changes since v2:
 - Fixed yamllint warnings/errors

Changes since v1:
 - dt binding with DT schema format

Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
Reported-by: Rob Herring <robh@kernel.org>
---
 .../hwmon/aspeed,ast2600-pwm-tachometer.yaml  | 131 ++++++++++++++++++
 1 file changed, 131 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
new file mode 100644
index 000000000000..fa5340f5a43f
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
@@ -0,0 +1,131 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-pwm-tachometer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED AST2600 PWM and Fan Tacho controller device driver
+
+maintainers:
+  - Ryan Chen <ryan_chen@aspeedtech.com>
+
+description: |
+  The ASPEED PWM controller can support upto 16 PWM outputs. The ASPEED Fan Tacho
+  controller can support upto 16 Fan tachometer inputs.
+  There can be upto 16 fans supported. Each fan can have one PWM output and
+  one Fan tach inputs.
+
+properties:
+  compatible:
+    const: aspeed,ast2600-pwm-tachometer
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  "#cooling-cells":
+    const: 2
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+patternProperties:
+  "^fan@[0-9a-f]$":
+    type: object
+    description:
+      Under fan subnode there can upto 16 child nodes, with each child node
+      representing a fan. There are 16 fans each fan can have one PWM port and one
+      Fan tach inputs.
+      For PWM port can be configured cooling-levels to create cooling device.
+      Cooling device could be bound to a thermal zone for the thermal control.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 15
+        description:
+          This property identify the PWM control channel of this fan.
+
+      fan-tach-ch:
+        $ref: /schemas/types.yaml#/definitions/uint8
+        minimum: 0
+        maximum: 15
+        description:
+          This property identify the fan tach input channel.
+
+      pulses-per-revolution:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        default: 2
+        minimum: 1
+        description:
+          Specify tacho pulse per revolution of the fan.
+
+      cooling-levels:
+        description:
+          PWM duty cycle values in a range from 0 to 255
+          which correspond to thermal cooling states.
+
+      aspeed,pwm-freq-hz:
+        default: 25000
+        minimum: 24
+        maximum: 780000
+        description:
+          Specify the frequency of PWM.
+
+      aspeed,inverse-pin:
+        type: boolean
+        description:
+          Inverse PWM output signal.
+
+      aspeed,falling-point:
+        $ref: /schemas/types.yaml#/definitions/uint8
+        default: 10
+        description:
+          Initialize the pulse width.
+
+    required:
+      - fan-tach-ch
+      - reg
+
+    additionalProperties: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm_tacho: pwm-tacho-controller@1e610000 {
+        compatible = "aspeed,ast2600-pwm-tachometer";
+        #address-cells = <1>;
+        #size-cells = <0>;
+        reg = <0x1e610000 0x100>;
+
+        fan@1 {
+            reg = <0x00>;
+            aspeed,pwm-freq-hz = <25000>;
+            cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+            fan-tach-ch = /bits/ 8 <0x00>;
+            pulses-per-revolution = <2>;
+        };
+
+        fan@2 {
+            reg = <0x01>;
+            aspeed,pwm-freq-hz = <25000>;
+            cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+            fan-tach-ch = /bits/ 8 <0x01>;
+            pulses-per-revolution = <2>;
+        };
+    };
+...
-- 
2.25.1

Re: [PATCH v3 1/4] dt-bindings: hwmon: Add Aspeed AST2600 PWM/Fan

[Andrew Jeffery]

On Mon, 18 Jan 2021, at 17:20, Troy Lee wrote:
> We add binding for supporting a new AST2600 PWM/Fan hwmon driver.
> 
> Changes since v2:
>  - Fixed yamllint warnings/errors
> 
> Changes since v1:
>  - dt binding with DT schema format
> 
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> Reported-by: Rob Herring <robh@kernel.org>
> ---
>  .../hwmon/aspeed,ast2600-pwm-tachometer.yaml  | 131 ++++++++++++++++++
>  1 file changed, 131 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> 
> diff --git 
> a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> new file mode 100644
> index 000000000000..fa5340f5a43f
> --- /dev/null
> +++ 
> b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-pwm-tachometer.yaml
> @@ -0,0 +1,131 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: 
> http://devicetree.org/schemas/hwmon/aspeed,ast2600-pwm-tachometer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED AST2600 PWM and Fan Tacho controller device driver
> +
> +maintainers:
> +  - Ryan Chen <ryan_chen@aspeedtech.com>
> +
> +description: |
> +  The ASPEED PWM controller can support upto 16 PWM outputs. The 
> ASPEED Fan Tacho
> +  controller can support upto 16 Fan tachometer inputs.
> +  There can be upto 16 fans supported. Each fan can have one PWM 
> output and
> +  one Fan tach inputs.
> +
> +properties:
> +  compatible:
> +    const: aspeed,ast2600-pwm-tachometer
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  "#cooling-cells":
> +    const: 2
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +patternProperties:
> +  "^fan@[0-9a-f]$":
> +    type: object
> +    description:
> +      Under fan subnode there can upto 16 child nodes, with each child 
> node
> +      representing a fan. There are 16 fans each fan can have one PWM 
> port and one
> +      Fan tach inputs.
> +      For PWM port can be configured cooling-levels to create cooling 
> device.
> +      Cooling device could be bound to a thermal zone for the thermal 
> control.
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 15
> +        description:
> +          This property identify the PWM control channel of this fan.
> +
> +      fan-tach-ch:
> +        $ref: /schemas/types.yaml#/definitions/uint8
> +        minimum: 0
> +        maximum: 15
> +        description:
> +          This property identify the fan tach input channel.
> +
> +      pulses-per-revolution:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        default: 2
> +        minimum: 1
> +        description:
> +          Specify tacho pulse per revolution of the fan.
> +
> +      cooling-levels:
> +        description:
> +          PWM duty cycle values in a range from 0 to 255
> +          which correspond to thermal cooling states.
> +
> +      aspeed,pwm-freq-hz:
> +        default: 25000
> +        minimum: 24
> +        maximum: 780000
> +        description:
> +          Specify the frequency of PWM.

What about using 'bus-frequency' instead?

> +
> +      aspeed,inverse-pin:
> +        type: boolean
> +        description:
> +          Inverse PWM output signal.

What's the benefit? I know the hardware provides the function, but do we need to expose it?

> +
> +      aspeed,falling-point:
> +        $ref: /schemas/types.yaml#/definitions/uint8
> +        default: 10

I feel like it probably shouldn't have a default value.

> +        description:
> +          Initialize the pulse width.

Then can we make the property name a bit more intuitive? 'pwm-duty'? Alternatively, [1] calls the duty "cooling-levels", can we use something in that vein instead?

[1] Documentation/devicetree/bindings/hwmon/pwm-fan.txt

Andrew

[PATCH v3 2/4] hwmon: Add Aspeed AST2600 support

[Troy Lee]

Updating index.rst and adding aspeed2600-pwm-tachometer.rst to address
the driver.

Changes since v1:
 - rename to aspeed2600-pwm-tachometer.rst
 - add license identifier

Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
---
 .../hwmon/aspeed2600-pwm-tachometer.rst       | 27 +++++++++++++++++++
 Documentation/hwmon/index.rst                 |  1 +
 2 files changed, 28 insertions(+)
 create mode 100644 Documentation/hwmon/aspeed2600-pwm-tachometer.rst

diff --git a/Documentation/hwmon/aspeed2600-pwm-tachometer.rst b/Documentation/hwmon/aspeed2600-pwm-tachometer.rst
new file mode 100644
index 000000000000..cf0d31a19597
--- /dev/null
+++ b/Documentation/hwmon/aspeed2600-pwm-tachometer.rst
@@ -0,0 +1,27 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===================================
+Kernel driver aspeed2600-pwm-tachometer
+===================================
+
+Supported chips:
+	ASPEED AST2600
+
+Authors:
+	Ryan Chen <ryan_chen@aspeedtech.com>
+
+Description:
+------------
+This driver implements support for ASPEED AST2600 PWM and Fan Tacho
+controller. The PWM controller supports upto 16 PWM outputs. The Fan tacho
+controller supports up to 16 tachometer inputs.
+
+The driver provides the following sensor accesses in sysfs:
+
+=============== ======= =====================================================
+fanX_input	ro	provide current fan rotation value in RPM as reported
+			by the fan to the device.
+
+pwmX		rw	get or set PWM fan control value. This is an integer
+			value between 0(off) and 255(full speed).
+=============== ======= =====================================================
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index fcb870ce6286..02020c282549 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -44,6 +44,7 @@ Hardware Monitoring Kernel Drivers
    asb100
    asc7621
    aspeed-pwm-tacho
+   aspeed2600-pwm-tachometer
    bcm54140
    bel-pfe
    bt1-pvt
-- 
2.25.1

[PATCH v3 3/4] ARM: dts: aspeed: Add Aspeed AST2600 PWM/Fan node in devicetree

[Troy Lee]

Create a common node in aspeed-g6.dtsi and add fan nodes for ast2600-evb
dts file.

Changes since v2:
 - Change property name pwm-freq to pwm-freq-hz

Changes since v1:
 - rename properties name in child node

Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed-ast2600-evb.dts | 152 +++++++++++++++++++++++
 arch/arm/boot/dts/aspeed-g6.dtsi         |  10 ++
 2 files changed, 162 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 89be13197780..4d24c051eeae 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -23,6 +23,158 @@ memory@80000000 {
 	};
 };
 
+&pwm_tacho {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_tach0_default
+			&pinctrl_pwm1_default &pinctrl_tach1_default
+			&pinctrl_pwm2_default &pinctrl_tach2_default
+			&pinctrl_pwm3_default &pinctrl_tach3_default
+			&pinctrl_pwm4_default &pinctrl_tach4_default
+			&pinctrl_pwm5_default &pinctrl_tach5_default
+			&pinctrl_pwm6_default &pinctrl_tach6_default
+			&pinctrl_pwm7_default &pinctrl_tach7_default
+			&pinctrl_pwm8g1_default &pinctrl_tach8_default
+			&pinctrl_pwm9g1_default &pinctrl_tach9_default
+			&pinctrl_pwm10g1_default &pinctrl_tach10_default
+			&pinctrl_pwm11g1_default &pinctrl_tach11_default
+			&pinctrl_pwm12g1_default &pinctrl_tach12_default
+			&pinctrl_pwm13g1_default &pinctrl_tach13_default
+			&pinctrl_pwm14g1_default &pinctrl_tach14_default
+			&pinctrl_pwm15g1_default &pinctrl_tach15_default>;
+
+	fan@0 {
+		reg = <0x00>;
+		aspeed,pwm-freq-hz = <25000>;
+		aspeed,falling-point = /bits/ 8 <100>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x00>;
+		aspeed,tacho-div = <3>;
+		pulses-per-revolution = <1>;
+	};
+
+	fan@1 {
+		reg = <0x01>;
+		aspeed,pwm-freq-hz = <25000>;
+		aspeed,falling-point = /bits/ 8 <100>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x01>;
+		pulses-per-revolution = <1>;
+	};
+
+	fan@2 {
+		reg = <0x02>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x02>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@3 {
+		reg = <0x03>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x03>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@4 {
+		reg = <0x04>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x04>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@5 {
+		reg = <0x05>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x05>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@6 {
+		reg = <0x06>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x06>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@7 {
+		reg = <0x07>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x07>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@8 {
+		reg = <0x08>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x08>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@9 {
+		reg = <0x09>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x09>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@a {
+		reg = <0x0a>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x0a>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@b {
+		reg = <0x0b>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x0b>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@c {
+		reg = <0x0c>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x0c>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@d {
+		reg = <0x0d>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x0d>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@e {
+		reg = <0x0e>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x0e>;
+		pulses-per-revolution = <2>;
+	};
+
+	fan@f {
+		reg = <0x0f>;
+		aspeed,pwm-freq-hz = <25000>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		fan-tach-ch = /bits/ 8 <0x0f>;
+		pulses-per-revolution = <2>;
+	};
+};
+
 &mdio0 {
 	status = "okay";
 
diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi
index 810b0676ab03..0369f8db123a 100644
--- a/arch/arm/boot/dts/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6.dtsi
@@ -304,6 +304,16 @@ apb {
 			#size-cells = <1>;
 			ranges;
 
+			pwm_tacho: pwm-tacho-controller@1e610000 {
+				compatible = "aspeed,ast2600-pwm-tachometer";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0x1e610000 0x100>;
+				clocks = <&syscon ASPEED_CLK_AHB>;
+				resets = <&syscon ASPEED_RESET_PWM>;
+				status = "disabled";
+			};
+
 			syscon: syscon@1e6e2000 {
 				compatible = "aspeed,ast2600-scu", "syscon", "simple-mfd";
 				reg = <0x1e6e2000 0x1000>;
-- 
2.25.1

[PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan tachometer

[Troy Lee]

Add Aspeed AST2600 PWM/Fan tacho driver. AST2600 has 16 PWM channel and
16 FAN tacho channel.

Changes since v2:
 - declare local function as static function

Changes since v1:
 - fixed review comments
 - fixed double-looped calculation of div_h and div_l
 - moving configuration to device tree
 - register hwmon driver with devm_hwmon_device_register_with_info()

Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/hwmon/Kconfig                |  10 +
 drivers/hwmon/Makefile               |   1 +
 drivers/hwmon/aspeed2600-pwm-tacho.c | 756 +++++++++++++++++++++++++++
 3 files changed, 767 insertions(+)
 create mode 100644 drivers/hwmon/aspeed2600-pwm-tacho.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 1ecf697d8d99..98f89f703161 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -400,6 +400,16 @@ config SENSORS_ASPEED
 	  This driver can also be built as a module. If so, the module
 	  will be called aspeed_pwm_tacho.
 
+config SENSORS_ASPEED2600_PWM_TACHO
+	tristate "ASPEED AST2600 PWM and Fan Tachometer"
+	depends on THERMAL || THERMAL=n
+	help
+	  This driver provides support for ASPEED AST2600 PWM
+	  and Fan Tacho controllers.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called aspeed2600-pwm-tacho.
+
 config SENSORS_ATXP1
 	tristate "Attansic ATXP1 VID controller"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 09a86c5e1d29..1a415d493ffc 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
 obj-$(CONFIG_SENSORS_AS370)	+= as370-hwmon.o
 obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
 obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
+obj-$(CONFIG_SENSORS_ASPEED2600_PWM_TACHO)	+= aspeed2600-pwm-tacho.o
 obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
 obj-$(CONFIG_SENSORS_BT1_PVT)	+= bt1-pvt.o
diff --git a/drivers/hwmon/aspeed2600-pwm-tacho.c b/drivers/hwmon/aspeed2600-pwm-tacho.c
new file mode 100644
index 000000000000..00ff100db92f
--- /dev/null
+++ b/drivers/hwmon/aspeed2600-pwm-tacho.c
@@ -0,0 +1,756 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) ASPEED Technology Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or later as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/sysfs.h>
+#include <linux/reset.h>
+#include <linux/regmap.h>
+#include <linux/thermal.h>
+/**********************************************************
+ * PWM HW register offset define
+ *********************************************************/
+/* PWM Control Register */
+#define ASPEED_PWM_CTRL_CH(ch)			(((ch) * 0x10) + 0x00)
+/* PWM Duty Cycle Register */
+#define ASPEED_PWM_DUTY_CYCLE_CH(ch)		(((ch) * 0x10) + 0x04)
+/* TACH Control Register */
+#define ASPEED_TACHO_CTRL_CH(ch)		(((ch) * 0x10) + 0x08)
+/* TACH Status Register */
+#define ASPEED_TACHO_STS_CH(x)			(((x) * 0x10) + 0x0C)
+
+/**********************************************************
+ * PWM register Bit field
+ *********************************************************/
+/*PWM_CTRL */
+#define PWM_LOAD_SEL_AS_WDT_BIT		(19)	/* load selection as WDT */
+#define PWM_DUTY_LOAD_AS_WDT_EN		BIT(18)	/* enable PWM duty load as WDT */
+#define PWM_DUTY_SYNC_DIS		BIT(17)	/* disable PWM duty sync */
+#define PWM_CLK_ENABLE			BIT(16)	/* enable PWM clock */
+#define PWM_LEVEL_OUTPUT		BIT(15)	/* output PWM level */
+#define PWM_INVERSE			BIT(14) /* inverse PWM pin */
+#define PWM_OPEN_DRAIN_EN		BIT(13)	/* enable open-drain */
+#define PWM_PIN_EN			BIT(12) /* enable PWM pin */
+#define PWM_CLK_DIV_H_MASK		(0xf << 8) /* PWM clock division H bit [3:0] */
+#define PWM_CLK_DIV_L_MASK		(0xff)	/* PWM clock division H bit [3:0] */
+/* [19] */
+#define LOAD_SEL_FALLING 0
+#define LOAD_SEL_RIGING  1
+
+/*PWM_DUTY_CYCLE */
+#define PWM_PERIOD_BIT			(24)		/* pwm period bit [7:0] */
+#define PWM_PERIOD_BIT_MASK		(0xff << 24)	/* pwm period bit [7:0] */
+#define PWM_RISING_FALLING_AS_WDT_BIT	(16)
+#define PWM_RISING_FALLING_AS_WDT_MASK	(0xff << 16)	/* rising/falling point bit [7:0] as WDT */
+#define PWM_RISING_FALLING_MASK		(0xffff)
+#define PWM_FALLING_POINT_BIT		(8)		/* pwm falling point bit [7:0] */
+#define PWM_RISING_POINT_BIT		(0)		/* pwm rising point bit [7:0] */
+/* [31:24] */
+#define DEFAULT_PWM_PERIOD 0xff
+
+/*PWM_TACHO_CTRL */
+#define TACHO_IER				BIT(31)	/* enable tacho interrupt */
+#define TACHO_INVERS_LIMIT			BIT(30) /* inverse tacho limit comparison */
+#define TACHO_LOOPBACK				BIT(29) /* tacho loopback */
+#define TACHO_ENABLE				BIT(28)	/* enable tacho */
+#define TACHO_DEBOUNCE_MASK			(0x3 << 26) /* tacho de-bounce */
+#define TACHO_DEBOUNCE_BIT			(26) /* tacho de-bounce */
+#define TECHIO_EDGE_MASK			(0x3 << 24) /* tacho edge */
+#define TECHIO_EDGE_BIT				(24) /* tacho edge */
+#define TACHO_CLK_DIV_T_MASK			(0xf << 20)
+#define TACHO_CLK_DIV_BIT			(20)
+#define TACHO_THRESHOLD_MASK			(0xfffff)	/* tacho threshold bit */
+/* [27:26] */
+#define DEBOUNCE_3_CLK 0x00 /* 10b */
+#define DEBOUNCE_2_CLK 0x01 /* 10b */
+#define DEBOUNCE_1_CLK 0x02 /* 10b */
+#define DEBOUNCE_0_CLK 0x03 /* 10b */
+/* [25:24] */
+#define F2F_EDGES 0x00 /* 10b */
+#define R2R_EDGES 0x01 /* 10b */
+#define BOTH_EDGES 0x02 /* 10b */
+/* [23:20] */
+/* Cover rpm range 5~5859375 */
+#define  DEFAULT_TACHO_DIV 5
+
+/*PWM_TACHO_STS */
+#define  TACHO_ISR		BIT(31)	/* interrupt status and clear */
+#define  PWM_OUT		BIT(25)	/* pwm_out */
+#define  PWM_OEN		BIT(24)	/* pwm_oeN */
+#define  TACHO_DEB_INPUT	BIT(23)	/* tacho deB input */
+#define  TACHO_RAW_INPUT	BIT(22) /* tacho raw input */
+#define  TACHO_VALUE_UPDATE	BIT(21)	/* tacho value updated since the last read */
+#define  TACHO_FULL_MEASUREMENT	BIT(20) /* tacho full measurement */
+#define  TACHO_VALUE_MASK	0xfffff	/* tacho value bit [19:0] */
+/**********************************************************
+ * Software setting
+ *********************************************************/
+#define DEFAULT_TARGET_PWM_FREQ		25000
+#define DEFAULT_FAN_PULSE_PR		2
+#define DEFAULT_WDT_RISING_FALLING_PT	0x10
+#define DEFAULT_RISING_PT		0x00
+#define DEFAULT_FALLING_PT		0x0a
+#define MAX_CDEV_NAME_LEN		16
+
+struct aspeed_pwm_channel_params {
+	int target_freq;
+	int pwm_freq;
+	int load_wdt_rising_falling_pt;
+	int load_wdt_selection;		/* 0: rising , 1: falling */
+	int load_wdt_enable;
+	int duty_sync_disable;
+	int inverse_pin;
+	u8 falling;
+};
+
+static struct aspeed_pwm_channel_params default_pwm_params[16];
+
+struct aspeed_tacho_channel_params {
+	int limited_inverse;
+	u16 threshold;
+	u8 tacho_edge;
+	u8 tacho_debounce;
+	u8 pulse_pr;
+	u32 divide;
+};
+
+static struct aspeed_tacho_channel_params default_tacho_params[16];
+
+struct aspeed_pwm_tachometer_data {
+	struct regmap *regmap;
+	unsigned long clk_freq;
+	struct reset_control *reset;
+	bool pwm_present[16];
+	bool fan_tach_present[16];
+	struct aspeed_pwm_channel_params *pwm_channel;
+	struct aspeed_tacho_channel_params *tacho_channel;
+	/* for thermal */
+	struct aspeed_cooling_device *cdev[8];
+};
+
+struct aspeed_cooling_device {
+	char name[16];
+	struct aspeed_pwm_tachometer_data *priv;
+	struct thermal_cooling_device *tcdev;
+	int pwm_channel;
+	u8 *cooling_levels;
+	u8 max_state;
+	u8 cur_state;
+};
+
+static int regmap_aspeed_pwm_tachometer_reg_write(void *context, unsigned int reg,
+					     unsigned int val)
+{
+	void __iomem *regs = (void __iomem *)context;
+
+	writel(val, regs + reg);
+	return 0;
+}
+
+static int regmap_aspeed_pwm_tachometer_reg_read(void *context, unsigned int reg,
+					    unsigned int *val)
+{
+	void __iomem *regs = (void __iomem *)context;
+
+	*val = readl(regs + reg);
+	return 0;
+}
+
+static const struct regmap_config aspeed_pwm_tachometer_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = 0x100,
+	.reg_write = regmap_aspeed_pwm_tachometer_reg_write,
+	.reg_read = regmap_aspeed_pwm_tachometer_reg_read,
+	.fast_io = true,
+};
+
+static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 pwm_channel,
+				       bool enable)
+{
+	regmap_update_bits(regmap, ASPEED_PWM_CTRL_CH(pwm_channel),
+			   PWM_CLK_ENABLE | PWM_PIN_EN,
+			   enable ? PWM_CLK_ENABLE | PWM_PIN_EN : 0);
+}
+
+static void aspeed_set_fan_tach_ch_enable(struct aspeed_pwm_tachometer_data *priv, u8 fan_tach_ch,
+					  bool enable, u32 tacho_div)
+{
+	u32 reg_value;
+
+	if (enable) {
+		/* divide = 2^(tacho_div*2) */
+		priv->tacho_channel[fan_tach_ch].divide = 1 << (tacho_div << 1);
+
+		reg_value = TACHO_ENABLE |
+				(priv->tacho_channel[fan_tach_ch].tacho_edge << TECHIO_EDGE_BIT) |
+				(tacho_div << TACHO_CLK_DIV_BIT) |
+				(priv->tacho_channel[fan_tach_ch].tacho_debounce << TACHO_DEBOUNCE_BIT);
+
+		if (priv->tacho_channel[fan_tach_ch].limited_inverse)
+			reg_value |= TACHO_INVERS_LIMIT;
+
+		if (priv->tacho_channel[fan_tach_ch].threshold)
+			reg_value |= (TACHO_IER | priv->tacho_channel[fan_tach_ch].threshold);
+
+		regmap_write(priv->regmap, ASPEED_TACHO_CTRL_CH(fan_tach_ch), reg_value);
+	} else
+		regmap_update_bits(priv->regmap, ASPEED_TACHO_CTRL_CH(fan_tach_ch),  TACHO_ENABLE, 0);
+}
+
+/*
+ * The PWM frequency = HCLK(200Mhz) / (clock division L bit *
+ * clock division H bit * (period bit + 1))
+ */
+static void aspeed_set_pwm_channel_fan_ctrl(struct device *dev,
+					   struct aspeed_pwm_tachometer_data *priv,
+					   u8 index, u8 fan_ctrl)
+{
+	u32 duty_value,	ctrl_value;
+	u32 target_div, div_h, div_l, cal_freq;
+	u32 tmp_div_h, tmp_div_l, diff, min_diff = INT_MAX;
+
+	if (fan_ctrl == 0) {
+		aspeed_set_pwm_channel_enable(priv->regmap, index, false);
+		return;
+	}
+
+	cal_freq = priv->clk_freq / (DEFAULT_PWM_PERIOD + 1);
+	target_div = DIV_ROUND_UP(cal_freq, priv->pwm_channel[index].target_freq);
+
+	/* calculate for target frequence */
+	for (tmp_div_h = 0; tmp_div_h < 0x10; tmp_div_h++) {
+		tmp_div_l = target_div / BIT(tmp_div_h) - 1;
+
+		if (tmp_div_l < 0 || tmp_div_l > 255)
+			continue;
+
+		diff = priv->pwm_channel[index].target_freq - cal_freq / (BIT(tmp_div_h) * (tmp_div_l + 1));
+		if (abs(diff) < abs(min_diff)) {
+			min_diff = diff;
+			div_l = tmp_div_l;
+			div_h = tmp_div_h;
+
+			if (diff == 0)
+				break;
+		}
+	}
+
+	priv->pwm_channel[index].pwm_freq = cal_freq / (BIT(div_h) * (div_l + 1));
+	dev_info(dev, "div h %x, l : %x pwm out clk %d\n", div_h, div_l,
+			priv->pwm_channel[index].pwm_freq);
+	dev_info(dev, "hclk %ld, target pwm freq %d, real pwm freq %d\n", priv->clk_freq,
+			priv->pwm_channel[index].target_freq, priv->pwm_channel[index].pwm_freq);
+
+	ctrl_value = (div_h << 8) | div_l;
+
+	duty_value = (DEFAULT_PWM_PERIOD << PWM_PERIOD_BIT) |
+		(0 << PWM_RISING_POINT_BIT) | (fan_ctrl << PWM_FALLING_POINT_BIT);
+
+	if (priv->pwm_channel[index].load_wdt_enable) {
+		ctrl_value |= PWM_DUTY_LOAD_AS_WDT_EN;
+		ctrl_value |= priv->pwm_channel[index].load_wdt_selection << PWM_LOAD_SEL_AS_WDT_BIT;
+		duty_value |= (priv->pwm_channel[index].load_wdt_rising_falling_pt << PWM_RISING_FALLING_AS_WDT_BIT);
+	}
+
+	if (priv->pwm_channel[index].inverse_pin)
+		ctrl_value |= PWM_INVERSE;
+	if (priv->pwm_channel[index].duty_sync_disable)
+		ctrl_value |= PWM_DUTY_SYNC_DIS;
+
+	regmap_write(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index), duty_value);
+	regmap_write(priv->regmap, ASPEED_PWM_CTRL_CH(index), ctrl_value);
+
+	aspeed_set_pwm_channel_enable(priv->regmap, index, true);
+}
+
+static int aspeed_get_fan_tach_ch_rpm(struct device *dev, struct aspeed_pwm_tachometer_data *priv,
+				      u8 fan_tach_ch)
+{
+	u32 raw_data, tach_div, clk_source, val;
+	int ret;
+
+	ret = regmap_read_poll_timeout(priv->regmap,
+				ASPEED_TACHO_STS_CH(fan_tach_ch),
+				val,
+				(val & TACHO_FULL_MEASUREMENT),
+				1000,
+				10000);
+
+	/* return -ETIMEDOUT if we didn't get an answer. */
+	if (ret)
+		return ret;
+
+	raw_data = val & TACHO_VALUE_MASK;
+	if (raw_data == 0xfffff)
+		return 0;
+
+	raw_data += 1;
+
+	/*
+	 * We need the mode to determine if the raw_data is double (from
+	 * counting both edges).
+	 */
+	tach_div = raw_data * (priv->tacho_channel[fan_tach_ch].divide) * (priv->tacho_channel[fan_tach_ch].pulse_pr);
+
+	dev_dbg(dev, "clk %ld, raw_data %d , tach_div %d\n", priv->clk_freq, raw_data, tach_div);
+	clk_source = priv->clk_freq;
+
+	return ((clk_source / tach_div) * 60);
+
+}
+
+static int set_pwm(struct device *dev, int index, long fan_ctrl)
+{
+	struct aspeed_pwm_tachometer_data *priv = dev_get_drvdata(dev);
+	u8 org_falling = priv->pwm_channel[index].falling;
+
+	if (fan_ctrl > DEFAULT_PWM_PERIOD || fan_ctrl < 0)
+		return -EINVAL;
+
+	if (priv->pwm_channel[index].falling == fan_ctrl)
+		return 0;
+
+	priv->pwm_channel[index].falling = fan_ctrl;
+
+	if (fan_ctrl == 0)
+		aspeed_set_pwm_channel_enable(priv->regmap, index, false);
+	else {
+		if (fan_ctrl == DEFAULT_PWM_PERIOD)
+			regmap_update_bits(priv->regmap,
+					   ASPEED_PWM_DUTY_CYCLE_CH(index),
+					   GENMASK(15, 0), 0);
+		else
+			regmap_update_bits(priv->regmap,
+					   ASPEED_PWM_DUTY_CYCLE_CH(index),
+					   GENMASK(15, 8),
+					   (fan_ctrl << PWM_FALLING_POINT_BIT));
+	}
+
+	if (org_falling == 0)
+		aspeed_set_pwm_channel_enable(priv->regmap, index, true);
+
+	return 0;
+}
+
+static void aspeed_create_pwm_channel(struct device *dev, struct aspeed_pwm_tachometer_data *priv,
+			   u8 pwm_channel)
+{
+	priv->pwm_present[pwm_channel] = true;
+
+	/* use default */
+	aspeed_set_pwm_channel_fan_ctrl(dev,
+					priv,
+					pwm_channel,
+					priv->pwm_channel[pwm_channel].falling);
+}
+
+static void aspeed_create_fan_tach_channel(struct aspeed_pwm_tachometer_data *priv,
+					   u8 *fan_tach_ch, int count,
+					   u32 fan_pulse_pr, u32 tacho_div)
+{
+	u8 val, index;
+
+	for (val = 0; val < count; val++) {
+		index = fan_tach_ch[val];
+		priv->fan_tach_present[index] = true;
+		priv->tacho_channel[index].pulse_pr = fan_pulse_pr;
+		priv->tacho_channel[index].limited_inverse = 0;
+		priv->tacho_channel[index].threshold = 0;
+		priv->tacho_channel[index].tacho_edge = F2F_EDGES;
+		priv->tacho_channel[index].tacho_debounce = DEBOUNCE_3_CLK;
+		aspeed_set_fan_tach_ch_enable(priv, index, true, tacho_div);
+	}
+}
+
+static int
+aspeed_pwm_cz_get_max_state(struct thermal_cooling_device *tcdev,
+			    unsigned long *state)
+{
+	struct aspeed_cooling_device *cdev = tcdev->devdata;
+
+	*state = cdev->max_state;
+
+	return 0;
+}
+
+static int
+aspeed_pwm_cz_get_cur_state(struct thermal_cooling_device *tcdev,
+			    unsigned long *state)
+{
+	struct aspeed_cooling_device *cdev = tcdev->devdata;
+
+	*state = cdev->cur_state;
+
+	return 0;
+}
+
+static int
+aspeed_pwm_cz_set_cur_state(struct thermal_cooling_device *tcdev,
+			    unsigned long state)
+{
+	struct aspeed_cooling_device *cdev = tcdev->devdata;
+
+	if (state > cdev->max_state)
+		return -EINVAL;
+
+	cdev->cur_state = state;
+	cdev->priv->pwm_channel[cdev->pwm_channel].falling =
+					cdev->cooling_levels[cdev->cur_state];
+	aspeed_set_pwm_channel_fan_ctrl(&tcdev->device, cdev->priv, cdev->pwm_channel,
+				     cdev->cooling_levels[cdev->cur_state]);
+
+	return 0;
+}
+
+static const struct thermal_cooling_device_ops aspeed_pwm_cool_ops = {
+	.get_max_state = aspeed_pwm_cz_get_max_state,
+	.get_cur_state = aspeed_pwm_cz_get_cur_state,
+	.set_cur_state = aspeed_pwm_cz_set_cur_state,
+};
+
+static int aspeed_create_pwm_cooling(struct device *dev,
+				     struct device_node *child,
+				     struct aspeed_pwm_tachometer_data *priv,
+				     u32 pwm_channel, u8 num_levels)
+{
+	int ret;
+	struct aspeed_cooling_device *cdev;
+
+	cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
+	if (!cdev)
+		return -ENOMEM;
+
+	cdev->cooling_levels = devm_kzalloc(dev, num_levels, GFP_KERNEL);
+	if (!cdev->cooling_levels)
+		return -ENOMEM;
+
+	cdev->max_state = num_levels - 1;
+	ret = of_property_read_u8_array(child, "cooling-levels",
+					cdev->cooling_levels,
+					num_levels);
+	if (ret) {
+		dev_err(dev, "Property 'cooling-levels' cannot be read.\n");
+		return ret;
+	}
+	snprintf(cdev->name, MAX_CDEV_NAME_LEN, "%s%d", child->name, pwm_channel);
+
+	cdev->tcdev = thermal_of_cooling_device_register(child,
+							 cdev->name,
+							 cdev,
+							 &aspeed_pwm_cool_ops);
+	if (IS_ERR(cdev->tcdev))
+		return PTR_ERR(cdev->tcdev);
+
+	cdev->priv = priv;
+	cdev->pwm_channel = pwm_channel;
+
+	priv->cdev[pwm_channel] = cdev;
+
+	return 0;
+}
+
+static int aspeed_pwm_create_fan(struct device *dev,
+			     struct device_node *child,
+			     struct aspeed_pwm_tachometer_data *priv)
+{
+	u8 *fan_tach_ch;
+	u32 fan_pulse_pr;
+	u32 tacho_div;
+	u32 pwm_channel;
+	u32 target_pwm_freq = 0;
+	u8 val;
+	int ret, count;
+
+	ret = of_property_read_u32(child, "reg", &pwm_channel);
+	if (ret)
+		return ret;
+	else if (pwm_channel > 0x0f)
+		return -EINVAL;
+
+	ret = of_property_read_u32(child, "aspeed,pwm-freq-hz", &target_pwm_freq);
+	if (ret)
+		target_pwm_freq = DEFAULT_TARGET_PWM_FREQ;
+
+	priv->pwm_channel[pwm_channel].target_freq = target_pwm_freq;
+
+	priv->pwm_channel[pwm_channel].load_wdt_enable = of_property_read_bool(child,
+			"aspeed,enable-pwm-duty-load-wdt");
+	priv->pwm_channel[pwm_channel].load_wdt_selection = of_property_read_bool(child,
+			"aspeed,wdt-selection-rising");
+	priv->pwm_channel[pwm_channel].duty_sync_disable = of_property_read_bool(child,
+			"aspeed,disable-duty-instant-change");
+	priv->pwm_channel[pwm_channel].inverse_pin = of_property_read_bool(child,
+			"aspeed,inverse-pin");
+
+	ret = of_property_read_u8(child, "aspeed,wdt-rising-falling-point", &val);
+	if (ret)
+		val = DEFAULT_WDT_RISING_FALLING_PT;
+	priv->pwm_channel[pwm_channel].load_wdt_rising_falling_pt = val;
+
+	ret = of_property_read_u8(child, "aspeed,falling-point", &val);
+	if (ret)
+		val = DEFAULT_FALLING_PT;
+	priv->pwm_channel[pwm_channel].falling = val;
+
+	aspeed_create_pwm_channel(dev, priv, (u8)pwm_channel);
+
+	ret = of_property_count_u8_elems(child, "cooling-levels");
+	if (ret > 0) {
+		if (IS_ENABLED(CONFIG_THERMAL)) {
+			ret = aspeed_create_pwm_cooling(dev, child, priv, pwm_channel,
+							ret);
+			if (ret)
+				return ret;
+		}
+	}
+
+	count = of_property_count_u8_elems(child, "fan-tach-ch");
+	if (count < 1)
+		return -EINVAL;
+
+	fan_tach_ch = devm_kzalloc(dev, sizeof(*fan_tach_ch) * count,
+				   GFP_KERNEL);
+	if (!fan_tach_ch)
+		return -ENOMEM;
+	ret = of_property_read_u8_array(child, "fan-tach-ch",
+					fan_tach_ch, count);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32(child, "pulses-per-revolution", &fan_pulse_pr);
+	if (ret)
+		fan_pulse_pr = DEFAULT_FAN_PULSE_PR;
+
+	ret = of_property_read_u32(child, "aspeed,tacho-div", &tacho_div);
+	if (ret)
+		tacho_div = DEFAULT_TACHO_DIV;
+
+	aspeed_create_fan_tach_channel(priv, fan_tach_ch, count, fan_pulse_pr, tacho_div);
+
+	return 0;
+}
+
+static umode_t aspeed_pwm_tacho_is_visible(const void *data, enum hwmon_sensor_types type,
+					u32 attr, int channel)
+{
+	const struct aspeed_pwm_tachometer_data *priv = data;
+
+	switch (type) {
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			if (!priv->pwm_present[channel])
+				return 0;
+			return 0644;
+		default:
+			return 0;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+			if (!priv->fan_tach_present[channel])
+				return 0;
+			return 0444;
+		default:
+			return 0;
+		}
+	default:
+		return 0;
+	}
+}
+
+static int aspeed_pwm_tacho_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct aspeed_pwm_tachometer_data *priv = dev_get_drvdata(dev);
+	long rpm;
+
+	switch (type) {
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			*val = priv->pwm_channel[channel].falling;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+			rpm = aspeed_get_fan_tach_ch_rpm(dev, priv, channel);
+			if (rpm < 0)
+				return rpm;
+			*val = rpm;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int aspeed_pwm_tacho_write(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long val)
+{
+	switch (type) {
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			return set_pwm(dev, channel, val);
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_ops aspeed_pwm_hwmon_ops = {
+	.is_visible = aspeed_pwm_tacho_is_visible,
+	.read = aspeed_pwm_tacho_read,
+	.write = aspeed_pwm_tacho_write,
+};
+
+static const struct hwmon_channel_info *aspeed_pwm_hwmon_info[] = {
+	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),
+	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),
+	NULL,
+};
+
+static const struct hwmon_chip_info aspeed_pwm_chip_info = {
+	.ops = &aspeed_pwm_hwmon_ops,
+	.info = aspeed_pwm_hwmon_info,
+};
+
+static int aspeed_pwm_tachometer_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np, *child;
+	struct aspeed_pwm_tachometer_data *priv;
+	void __iomem *regs;
+	struct device *hwmon;
+	struct clk *clk;
+	int ret;
+
+	np = dev->of_node;
+
+	regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->pwm_channel = default_pwm_params;
+	priv->tacho_channel = default_tacho_params;
+	priv->regmap = devm_regmap_init(dev, NULL, (__force void *)regs,
+			&aspeed_pwm_tachometer_regmap_config);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		return -ENODEV;
+	priv->clk_freq = clk_get_rate(clk);
+
+	priv->reset = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->reset)) {
+		dev_err(&pdev->dev, "can't get aspeed_pwm_tacho reset\n");
+		return PTR_ERR(priv->reset);
+	}
+
+	/* scu init */
+	reset_control_assert(priv->reset);
+	reset_control_deassert(priv->reset);
+
+	for_each_child_of_node(np, child) {
+		ret = aspeed_pwm_create_fan(dev, child, priv);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
+	}
+
+	dev_info(dev, "pwm tach probe done\n");
+	hwmon = devm_hwmon_device_register_with_info(dev, "aspeed_pwm_tachometer",
+			priv, &aspeed_pwm_chip_info, NULL);
+
+	return PTR_ERR_OR_ZERO(hwmon);
+}
+
+static const struct of_device_id of_pwm_tachometer_match_table[] = {
+	{ .compatible = "aspeed,ast2600-pwm-tachometer", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_pwm_tachometer_match_table);
+
+static struct platform_driver aspeed_pwm_tachometer_driver = {
+	.probe		= aspeed_pwm_tachometer_probe,
+	.driver		= {
+		.name	= "aspeed_pwm_tachometer",
+		.of_match_table = of_pwm_tachometer_match_table,
+	},
+};
+
+module_platform_driver(aspeed_pwm_tachometer_driver);
+
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED PWM and Fan Tachometer device driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan tachometer

[Andrew Jeffery]

Hi Troy,

On Mon, 18 Jan 2021, at 17:20, Troy Lee wrote:
> Add Aspeed AST2600 PWM/Fan tacho driver. AST2600 has 16 PWM channel and
> 16 FAN tacho channel.
> 
> Changes since v2:
>  - declare local function as static function
> 
> Changes since v1:
>  - fixed review comments
>  - fixed double-looped calculation of div_h and div_l
>  - moving configuration to device tree
>  - register hwmon driver with devm_hwmon_device_register_with_info()
> 
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  drivers/hwmon/Kconfig                |  10 +
>  drivers/hwmon/Makefile               |   1 +
>  drivers/hwmon/aspeed2600-pwm-tacho.c | 756 +++++++++++++++++++++++++++
>  3 files changed, 767 insertions(+)
>  create mode 100644 drivers/hwmon/aspeed2600-pwm-tacho.c

...

> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 09a86c5e1d29..1a415d493ffc 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>  obj-$(CONFIG_SENSORS_AS370)	+= as370-hwmon.o
>  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
>  obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
> +obj-$(CONFIG_SENSORS_ASPEED2600_PWM_TACHO)	+= aspeed2600-pwm-tacho.o

Why does this have to be a separate implementation from the existing ASPEED 
PWM/Tacho driver? Is there really nothing in common?

Andrew

RE: [PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan tachometer

[Ryan Chen]

> -----Original Message-----
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Wednesday, January 20, 2021 1:16 PM
> To: Troy Lee <troy_lee@aspeedtech.com>; openbmc@lists.ozlabs.org; Joel
> Stanley <joel@jms.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; open list
> <linux-kernel@vger.kernel.org>; moderated list:ARM/ASPEED MACHINE
> SUPPORT <linux-arm-kernel@lists.infradead.org>; moderated
> list:ARM/ASPEED MACHINE SUPPORT <linux-aspeed@lists.ozlabs.org>
> Cc: Ryan Chen <ryan_chen@aspeedtech.com>; ChiaWei Wang
> <chiawei_wang@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; kbuild
> test robot <lkp@intel.com>
> Subject: Re: [PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan
> tachometer
> 
> Hi Troy,
> 
> On Mon, 18 Jan 2021, at 17:20, Troy Lee wrote:
> > Add Aspeed AST2600 PWM/Fan tacho driver. AST2600 has 16 PWM channel
> > and
> > 16 FAN tacho channel.
> >
> > Changes since v2:
> >  - declare local function as static function
> >
> > Changes since v1:
> >  - fixed review comments
> >  - fixed double-looped calculation of div_h and div_l
> >  - moving configuration to device tree
> >  - register hwmon driver with devm_hwmon_device_register_with_info()
> >
> > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > ---
> >  drivers/hwmon/Kconfig                |  10 +
> >  drivers/hwmon/Makefile               |   1 +
> >  drivers/hwmon/aspeed2600-pwm-tacho.c | 756
> > +++++++++++++++++++++++++++
> >  3 files changed, 767 insertions(+)
> >  create mode 100644 drivers/hwmon/aspeed2600-pwm-tacho.c
> 
> ...
> 
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index
> > 09a86c5e1d29..1a415d493ffc 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -52,6 +52,7 @@ obj-$(CONFIG_SENSORS_ARM_SCPI)	+=
> scpi-hwmon.o
> >  obj-$(CONFIG_SENSORS_AS370)	+= as370-hwmon.o
> >  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
> >  obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
> > +obj-$(CONFIG_SENSORS_ASPEED2600_PWM_TACHO)	+=
> aspeed2600-pwm-tacho.o
> 
> Why does this have to be a separate implementation from the existing ASPEED
> PWM/Tacho driver? Is there really nothing in common?
> 
Hello Andrew,
	The register set is fully re-arrange. And it is new design at AST2600, can't be compatible with older PWM.  

Re: [PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan tachometer

[Andrew Jeffery]

On Wed, 20 Jan 2021, at 15:53, Ryan Chen wrote:
> > -----Original Message-----
> > From: Andrew Jeffery <andrew@aj.id.au>
> > Sent: Wednesday, January 20, 2021 1:16 PM
> > To: Troy Lee <troy_lee@aspeedtech.com>; openbmc@lists.ozlabs.org; Joel
> > Stanley <joel@jms.id.au>; Philipp Zabel <p.zabel@pengutronix.de>; open list
> > <linux-kernel@vger.kernel.org>; moderated list:ARM/ASPEED MACHINE
> > SUPPORT <linux-arm-kernel@lists.infradead.org>; moderated
> > list:ARM/ASPEED MACHINE SUPPORT <linux-aspeed@lists.ozlabs.org>
> > Cc: Ryan Chen <ryan_chen@aspeedtech.com>; ChiaWei Wang
> > <chiawei_wang@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; kbuild
> > test robot <lkp@intel.com>
> > Subject: Re: [PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan
> > tachometer
> > 
> > Hi Troy,
> > 
> > On Mon, 18 Jan 2021, at 17:20, Troy Lee wrote:
> > > Add Aspeed AST2600 PWM/Fan tacho driver. AST2600 has 16 PWM channel
> > > and
> > > 16 FAN tacho channel.
> > >
> > > Changes since v2:
> > >  - declare local function as static function
> > >
> > > Changes since v1:
> > >  - fixed review comments
> > >  - fixed double-looped calculation of div_h and div_l
> > >  - moving configuration to device tree
> > >  - register hwmon driver with devm_hwmon_device_register_with_info()
> > >
> > > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > ---
> > >  drivers/hwmon/Kconfig                |  10 +
> > >  drivers/hwmon/Makefile               |   1 +
> > >  drivers/hwmon/aspeed2600-pwm-tacho.c | 756
> > > +++++++++++++++++++++++++++
> > >  3 files changed, 767 insertions(+)
> > >  create mode 100644 drivers/hwmon/aspeed2600-pwm-tacho.c
> > 
> > ...
> > 
> > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index
> > > 09a86c5e1d29..1a415d493ffc 100644
> > > --- a/drivers/hwmon/Makefile
> > > +++ b/drivers/hwmon/Makefile
> > > @@ -52,6 +52,7 @@ obj-$(CONFIG_SENSORS_ARM_SCPI)	+=
> > scpi-hwmon.o
> > >  obj-$(CONFIG_SENSORS_AS370)	+= as370-hwmon.o
> > >  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
> > >  obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
> > > +obj-$(CONFIG_SENSORS_ASPEED2600_PWM_TACHO)	+=
> > aspeed2600-pwm-tacho.o
> > 
> > Why does this have to be a separate implementation from the existing ASPEED
> > PWM/Tacho driver? Is there really nothing in common?
> > 
> Hello Andrew,
> 	The register set is fully re-arrange. And it is new design at AST2600, 
> can't be compatible with older PWM.  
>

Ah, okay. I hadn't looked. Thanks Ryan.

Andrew

Re: [PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan tachometer

[Andrew Jeffery]

Hi Troy,

On Mon, 18 Jan 2021, at 17:20, Troy Lee wrote:
> Add Aspeed AST2600 PWM/Fan tacho driver. AST2600 has 16 PWM channel and
> 16 FAN tacho channel.
> 
> Changes since v2:
>  - declare local function as static function
> 
> Changes since v1:
>  - fixed review comments
>  - fixed double-looped calculation of div_h and div_l
>  - moving configuration to device tree
>  - register hwmon driver with devm_hwmon_device_register_with_info()
> 
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  drivers/hwmon/Kconfig                |  10 +
>  drivers/hwmon/Makefile               |   1 +
>  drivers/hwmon/aspeed2600-pwm-tacho.c | 756 +++++++++++++++++++++++++++
>  3 files changed, 767 insertions(+)
>  create mode 100644 drivers/hwmon/aspeed2600-pwm-tacho.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 1ecf697d8d99..98f89f703161 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -400,6 +400,16 @@ config SENSORS_ASPEED
>  	  This driver can also be built as a module. If so, the module
>  	  will be called aspeed_pwm_tacho.
>  
> +config SENSORS_ASPEED2600_PWM_TACHO
> +	tristate "ASPEED AST2600 PWM and Fan Tachometer"
> +	depends on THERMAL || THERMAL=n
> +	help
> +	  This driver provides support for ASPEED AST2600 PWM
> +	  and Fan Tacho controllers.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called aspeed2600-pwm-tacho.
> +
>  config SENSORS_ATXP1
>  	tristate "Attansic ATXP1 VID controller"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 09a86c5e1d29..1a415d493ffc 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>  obj-$(CONFIG_SENSORS_AS370)	+= as370-hwmon.o
>  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
>  obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
> +obj-$(CONFIG_SENSORS_ASPEED2600_PWM_TACHO)	+= aspeed2600-pwm-tacho.o
>  obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
>  obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
>  obj-$(CONFIG_SENSORS_BT1_PVT)	+= bt1-pvt.o
> diff --git a/drivers/hwmon/aspeed2600-pwm-tacho.c 
> b/drivers/hwmon/aspeed2600-pwm-tacho.c
> new file mode 100644
> index 000000000000..00ff100db92f
> --- /dev/null
> +++ b/drivers/hwmon/aspeed2600-pwm-tacho.c
> @@ -0,0 +1,756 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) ASPEED Technology Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or 
> later as
> + * published by the Free Software Foundation.
> + */

The license is captured by the SPDX marker above. This summary text should be dropped (though retain the copyright line).

> +
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/reset.h>
> +#include <linux/regmap.h>
> +#include <linux/thermal.h>
> +/**********************************************************
> + * PWM HW register offset define
> + *********************************************************/

Banner comments like this generally aren't preferred. If you think the comment text itself is necessary, just do:

/* PWM HW register offset define */

However, the macro names should indicate what we're defining anyway, so I feel we can remove it entirely without too much loss

> +/* PWM Control Register */
> +#define ASPEED_PWM_CTRL_CH(ch)			(((ch) * 0x10) + 0x00)
> +/* PWM Duty Cycle Register */
> +#define ASPEED_PWM_DUTY_CYCLE_CH(ch)		(((ch) * 0x10) + 0x04)
> +/* TACH Control Register */
> +#define ASPEED_TACHO_CTRL_CH(ch)		(((ch) * 0x10) + 0x08)
> +/* TACH Status Register */
> +#define ASPEED_TACHO_STS_CH(x)			(((x) * 0x10) + 0x0C)
> +
> +/**********************************************************
> + * PWM register Bit field
> + *********************************************************/
> +/*PWM_CTRL */
> +#define PWM_LOAD_SEL_AS_WDT_BIT		(19)	/* load selection as WDT */

The trailing comments don't really add any further information in the context of the macro name. Let's remove these as well.

> +#define PWM_DUTY_LOAD_AS_WDT_EN		BIT(18)	/* enable PWM duty load as 
> WDT */
> +#define PWM_DUTY_SYNC_DIS		BIT(17)	/* disable PWM duty sync */
> +#define PWM_CLK_ENABLE			BIT(16)	/* enable PWM clock */
> +#define PWM_LEVEL_OUTPUT		BIT(15)	/* output PWM level */
> +#define PWM_INVERSE			BIT(14) /* inverse PWM pin */
> +#define PWM_OPEN_DRAIN_EN		BIT(13)	/* enable open-drain */
> +#define PWM_PIN_EN			BIT(12) /* enable PWM pin */
> +#define PWM_CLK_DIV_H_MASK		(0xf << 8) /* PWM clock division H bit 
> [3:0] */
> +#define PWM_CLK_DIV_L_MASK		(0xff)	/* PWM clock division H bit [3:0] */
> +/* [19] */
> +#define LOAD_SEL_FALLING 0
> +#define LOAD_SEL_RIGING  1

It's much easier to track these macros if you place them alongside the associated register, and in a way that makes them self-documenting (i.e. we remove the need for the banner comments). Often the bit/mask macros are indented to help with visuals, for example:

#define ASPEED_PWM_CTRL_CH(ch)			(((ch) * 0x10) + 0x00)
#define     PWM_DUTY_LOAD_AS_WDT_EN		BIT(18)
#define     PWM_DUTY_SYNC_DIS                             BIT(17)
...

#define ASPEED_PWM_DUTY_CYCLE_CH(ch)			(((ch) * 0x10) + 0x04)
#define     PWM_PERIOD_BIT						(24)
#define     PWM_PERIOD_BIT_MASK					(0xff << 24)
...

etcetera. It also helps to make the name of the bitfield macros use a similar prefix to name of the associated register macro.

> +
> +/*PWM_DUTY_CYCLE */
> +#define PWM_PERIOD_BIT			(24)		/* pwm period bit [7:0] */
> +#define PWM_PERIOD_BIT_MASK		(0xff << 24)	/* pwm period bit [7:0] */
> +#define PWM_RISING_FALLING_AS_WDT_BIT	(16)
> +#define PWM_RISING_FALLING_AS_WDT_MASK	(0xff << 16)	/* rising/falling 
> point bit [7:0] as WDT */
> +#define PWM_RISING_FALLING_MASK		(0xffff)
> +#define PWM_FALLING_POINT_BIT		(8)		/* pwm falling point bit [7:0] */
> +#define PWM_RISING_POINT_BIT		(0)		/* pwm rising point bit [7:0] */
> +/* [31:24] */
> +#define DEFAULT_PWM_PERIOD 0xff
> +
> +/*PWM_TACHO_CTRL */
> +#define TACHO_IER				BIT(31)	/* enable tacho interrupt */
> +#define TACHO_INVERS_LIMIT			BIT(30) /* inverse tacho limit comparison 
> */
> +#define TACHO_LOOPBACK				BIT(29) /* tacho loopback */
> +#define TACHO_ENABLE				BIT(28)	/* enable tacho */
> +#define TACHO_DEBOUNCE_MASK			(0x3 << 26) /* tacho de-bounce */
> +#define TACHO_DEBOUNCE_BIT			(26) /* tacho de-bounce */
> +#define TECHIO_EDGE_MASK			(0x3 << 24) /* tacho edge */
> +#define TECHIO_EDGE_BIT				(24) /* tacho edge */
> +#define TACHO_CLK_DIV_T_MASK			(0xf << 20)
> +#define TACHO_CLK_DIV_BIT			(20)
> +#define TACHO_THRESHOLD_MASK			(0xfffff)	/* tacho threshold bit */
> +/* [27:26] */
> +#define DEBOUNCE_3_CLK 0x00 /* 10b */
> +#define DEBOUNCE_2_CLK 0x01 /* 10b */
> +#define DEBOUNCE_1_CLK 0x02 /* 10b */
> +#define DEBOUNCE_0_CLK 0x03 /* 10b */
> +/* [25:24] */
> +#define F2F_EDGES 0x00 /* 10b */
> +#define R2R_EDGES 0x01 /* 10b */
> +#define BOTH_EDGES 0x02 /* 10b */
> +/* [23:20] */
> +/* Cover rpm range 5~5859375 */
> +#define  DEFAULT_TACHO_DIV 5
> +
> +/*PWM_TACHO_STS */
> +#define  TACHO_ISR		BIT(31)	/* interrupt status and clear */
> +#define  PWM_OUT		BIT(25)	/* pwm_out */
> +#define  PWM_OEN		BIT(24)	/* pwm_oeN */
> +#define  TACHO_DEB_INPUT	BIT(23)	/* tacho deB input */
> +#define  TACHO_RAW_INPUT	BIT(22) /* tacho raw input */
> +#define  TACHO_VALUE_UPDATE	BIT(21)	/* tacho value updated since the 
> last read */
> +#define  TACHO_FULL_MEASUREMENT	BIT(20) /* tacho full measurement */
> +#define  TACHO_VALUE_MASK	0xfffff	/* tacho value bit [19:0] */
> +/**********************************************************
> + * Software setting
> + *********************************************************/
> +#define DEFAULT_TARGET_PWM_FREQ		25000
> +#define DEFAULT_FAN_PULSE_PR		2
> +#define DEFAULT_WDT_RISING_FALLING_PT	0x10
> +#define DEFAULT_RISING_PT		0x00
> +#define DEFAULT_FALLING_PT		0x0a
> +#define MAX_CDEV_NAME_LEN		16
> +
> +struct aspeed_pwm_channel_params {
> +	int target_freq;
> +	int pwm_freq;
> +	int load_wdt_rising_falling_pt;
> +	int load_wdt_selection;		/* 0: rising , 1: falling */
> +	int load_wdt_enable;
> +	int duty_sync_disable;
> +	int inverse_pin;
> +	u8 falling;
> +};
> +
> +static struct aspeed_pwm_channel_params default_pwm_params[16];
> +
> +struct aspeed_tacho_channel_params {
> +	int limited_inverse;
> +	u16 threshold;
> +	u8 tacho_edge;
> +	u8 tacho_debounce;
> +	u8 pulse_pr;
> +	u32 divide;
> +};
> +
> +static struct aspeed_tacho_channel_params default_tacho_params[16];
> +
> +struct aspeed_pwm_tachometer_data {
> +	struct regmap *regmap;

What's the reason for using a regmap? For MMIO the benefit is that the API provides some locking for when regmap is used as a syscon, but that doesn't appear to be how it's used here? regmap generates a bunch of overhead compared to readl()/writel().

> +	unsigned long clk_freq;
> +	struct reset_control *reset;
> +	bool pwm_present[16];
> +	bool fan_tach_present[16];
> +	struct aspeed_pwm_channel_params *pwm_channel;
> +	struct aspeed_tacho_channel_params *tacho_channel;
> +	/* for thermal */
> +	struct aspeed_cooling_device *cdev[8];
> +};
> +
> +struct aspeed_cooling_device {
> +	char name[16];
> +	struct aspeed_pwm_tachometer_data *priv;
> +	struct thermal_cooling_device *tcdev;
> +	int pwm_channel;
> +	u8 *cooling_levels;
> +	u8 max_state;
> +	u8 cur_state;
> +};
> +
> +static int regmap_aspeed_pwm_tachometer_reg_write(void *context, 
> unsigned int reg,
> +					     unsigned int val)
> +{
> +	void __iomem *regs = (void __iomem *)context;
> +
> +	writel(val, regs + reg);
> +	return 0;
> +}
> +
> +static int regmap_aspeed_pwm_tachometer_reg_read(void *context, 
> unsigned int reg,
> +					    unsigned int *val)
> +{
> +	void __iomem *regs = (void __iomem *)context;
> +
> +	*val = readl(regs + reg);
> +	return 0;
> +}
> +
> +static const struct regmap_config aspeed_pwm_tachometer_regmap_config 
> = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = 0x100,
> +	.reg_write = regmap_aspeed_pwm_tachometer_reg_write,
> +	.reg_read = regmap_aspeed_pwm_tachometer_reg_read,

These just do an MMIO access, do we really need our own read/write definitions?

> +	.fast_io = true,
> +};
> +
> +static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 
> pwm_channel,
> +				       bool enable)
> +{
> +	regmap_update_bits(regmap, ASPEED_PWM_CTRL_CH(pwm_channel),
> +			   PWM_CLK_ENABLE | PWM_PIN_EN,
> +			   enable ? PWM_CLK_ENABLE | PWM_PIN_EN : 0);

If the motivation for regmap is something like regmap_update_bits(), you can alternatively use the FIELD_*() macros from linux/bitfield.h

> +}
> +
> +static void aspeed_set_fan_tach_ch_enable(struct 
> aspeed_pwm_tachometer_data *priv, u8 fan_tach_ch,
> +					  bool enable, u32 tacho_div)
> +{
> +	u32 reg_value;
> +
> +	if (enable) {
> +		/* divide = 2^(tacho_div*2) */
> +		priv->tacho_channel[fan_tach_ch].divide = 1 << (tacho_div << 1);
> +
> +		reg_value = TACHO_ENABLE |
> +				(priv->tacho_channel[fan_tach_ch].tacho_edge << TECHIO_EDGE_BIT) |
> +				(tacho_div << TACHO_CLK_DIV_BIT) |
> +				(priv->tacho_channel[fan_tach_ch].tacho_debounce << 
> TACHO_DEBOUNCE_BIT);
> +
> +		if (priv->tacho_channel[fan_tach_ch].limited_inverse)
> +			reg_value |= TACHO_INVERS_LIMIT;
> +
> +		if (priv->tacho_channel[fan_tach_ch].threshold)
> +			reg_value |= (TACHO_IER | 
> priv->tacho_channel[fan_tach_ch].threshold);

I feel like this could be cleaned up a bit with:

const struct aspeed_tacho_channel_params *tacho = &priv->tacho_channel[fan_tach_ch];

and replacing the expression with tacho throughout.

> +
> +		regmap_write(priv->regmap, ASPEED_TACHO_CTRL_CH(fan_tach_ch), 
> reg_value);
> +	} else
> +		regmap_update_bits(priv->regmap, ASPEED_TACHO_CTRL_CH(fan_tach_ch),  
> TACHO_ENABLE, 0);
> +}
> +
> +/*
> + * The PWM frequency = HCLK(200Mhz) / (clock division L bit *
> + * clock division H bit * (period bit + 1))
> + */
> +static void aspeed_set_pwm_channel_fan_ctrl(struct device *dev,
> +					   struct aspeed_pwm_tachometer_data *priv,
> +					   u8 index, u8 fan_ctrl)
> +{
> +	u32 duty_value,	ctrl_value;
> +	u32 target_div, div_h, div_l, cal_freq;
> +	u32 tmp_div_h, tmp_div_l, diff, min_diff = INT_MAX;
> +
> +	if (fan_ctrl == 0) {
> +		aspeed_set_pwm_channel_enable(priv->regmap, index, false);
> +		return;
> +	}
> +
> +	cal_freq = priv->clk_freq / (DEFAULT_PWM_PERIOD + 1);

Is this always correct if we're relying on a default value? Or are we using DEFAULT_PWM_PERIOD because its value is the maximum period? Using a macro with DEFAULT in the name certainly makes me feel less sure that the code is right.

> +	target_div = DIV_ROUND_UP(cal_freq, 
> priv->pwm_channel[index].target_freq);
> +
> +	/* calculate for target frequence */
> +	for (tmp_div_h = 0; tmp_div_h < 0x10; tmp_div_h++) {
> +		tmp_div_l = target_div / BIT(tmp_div_h) - 1;
> +
> +		if (tmp_div_l < 0 || tmp_div_l > 255)
> +			continue;
> +
> +		diff = priv->pwm_channel[index].target_freq - cal_freq / 
> (BIT(tmp_div_h) * (tmp_div_l + 1));
> +		if (abs(diff) < abs(min_diff)) {
> +			min_diff = diff;
> +			div_l = tmp_div_l;
> +			div_h = tmp_div_h;
> +
> +			if (diff == 0)
> +				break;
> +		}
> +	}

I've spent some time thinking about this and haven't come up with anything that's concretely better. It wasn't fun to read though :(

> +
> +	priv->pwm_channel[index].pwm_freq = cal_freq / (BIT(div_h) * (div_l + 
> 1));
> +	dev_info(dev, "div h %x, l : %x pwm out clk %d\n", div_h, div_l,
> +			priv->pwm_channel[index].pwm_freq);
> +	dev_info(dev, "hclk %ld, target pwm freq %d, real pwm freq %d\n", 
> priv->clk_freq,
> +			priv->pwm_channel[index].target_freq, 
> priv->pwm_channel[index].pwm_freq);

These should be dev_dbg()

> +
> +	ctrl_value = (div_h << 8) | div_l;
> +
> +	duty_value = (DEFAULT_PWM_PERIOD << PWM_PERIOD_BIT) |
> +		(0 << PWM_RISING_POINT_BIT) | (fan_ctrl << PWM_FALLING_POINT_BIT);
> +
> +	if (priv->pwm_channel[index].load_wdt_enable) {
> +		ctrl_value |= PWM_DUTY_LOAD_AS_WDT_EN;
> +		ctrl_value |= priv->pwm_channel[index].load_wdt_selection << 
> PWM_LOAD_SEL_AS_WDT_BIT;
> +		duty_value |= (priv->pwm_channel[index].load_wdt_rising_falling_pt 
> << PWM_RISING_FALLING_AS_WDT_BIT);
> +	}
> +
> +	if (priv->pwm_channel[index].inverse_pin)
> +		ctrl_value |= PWM_INVERSE;
> +	if (priv->pwm_channel[index].duty_sync_disable)
> +		ctrl_value |= PWM_DUTY_SYNC_DIS;

Similar to my comment above about tacho, I think this can be cleaned up a bit with:

const struct aspeed_pwm_channel_params *pwm = &priv->pwm_channel[index];

and replacing the expression with pwm throughout.

> +
> +	regmap_write(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index), 
> duty_value);
> +	regmap_write(priv->regmap, ASPEED_PWM_CTRL_CH(index), ctrl_value);
> +
> +	aspeed_set_pwm_channel_enable(priv->regmap, index, true);
> +}
> +
> +static int aspeed_get_fan_tach_ch_rpm(struct device *dev, struct 
> aspeed_pwm_tachometer_data *priv,
> +				      u8 fan_tach_ch)
> +{
> +	u32 raw_data, tach_div, clk_source, val;
> +	int ret;
> +
> +	ret = regmap_read_poll_timeout(priv->regmap,
> +				ASPEED_TACHO_STS_CH(fan_tach_ch),
> +				val,
> +				(val & TACHO_FULL_MEASUREMENT),
> +				1000,
> +				10000);

Could just use readl_poll_timeout()?

> +
> +	/* return -ETIMEDOUT if we didn't get an answer. */
> +	if (ret)
> +		return ret;
> +
> +	raw_data = val & TACHO_VALUE_MASK;
> +	if (raw_data == 0xfffff)
> +		return 0;
> +
> +	raw_data += 1;
> +
> +	/*
> +	 * We need the mode to determine if the raw_data is double (from
> +	 * counting both edges).
> +	 */
> +	tach_div = raw_data * (priv->tacho_channel[fan_tach_ch].divide) * 
> (priv->tacho_channel[fan_tach_ch].pulse_pr);

Again, lets extract the channel pointer to clean this up a little.

> +
> +	dev_dbg(dev, "clk %ld, raw_data %d , tach_div %d\n", priv->clk_freq, 
> raw_data, tach_div);
> +	clk_source = priv->clk_freq;
> +
> +	return ((clk_source / tach_div) * 60);
> +
> +}
> +
> +static int set_pwm(struct device *dev, int index, long fan_ctrl)
> +{
> +	struct aspeed_pwm_tachometer_data *priv = dev_get_drvdata(dev);
> +	u8 org_falling = priv->pwm_channel[index].falling;
> +
> +	if (fan_ctrl > DEFAULT_PWM_PERIOD || fan_ctrl < 0)
> +		return -EINVAL;
> +
> +	if (priv->pwm_channel[index].falling == fan_ctrl)
> +		return 0;
> +
> +	priv->pwm_channel[index].falling = fan_ctrl;

My understanding is the value of fan_ctrl is the value that userspace has written to the pwm[1-*] attribute in sysfs. The name 'falling' in struct aspeed_pwm_channel_params is not at all intuitive to me. I understand the hardware calls it something along these lines, but is there a better name? "period"? "duty"?

> +
> +	if (fan_ctrl == 0)
> +		aspeed_set_pwm_channel_enable(priv->regmap, index, false);
> +	else {
> +		if (fan_ctrl == DEFAULT_PWM_PERIOD)
> +			regmap_update_bits(priv->regmap,
> +					   ASPEED_PWM_DUTY_CYCLE_CH(index),
> +					   GENMASK(15, 0), 0);
> +		else
> +			regmap_update_bits(priv->regmap,
> +					   ASPEED_PWM_DUTY_CYCLE_CH(index),
> +					   GENMASK(15, 8),

The asymmetry between these masks caught my eye, and I guess I understand what's going on a bit more with 'falling' at least. The hardware is definitely flexible :)

> +					   (fan_ctrl << PWM_FALLING_POINT_BIT));
> +	}
> +
> +	if (org_falling == 0)

Do we have to assign .falling member before this point? Can we get rid of org_falling by assigning .falling after this test?

> +		aspeed_set_pwm_channel_enable(priv->regmap, index, true);
> +
> +	return 0;
> +}
> +
> +static void aspeed_create_pwm_channel(struct device *dev, struct 
> aspeed_pwm_tachometer_data *priv,
> +			   u8 pwm_channel)
> +{
> +	priv->pwm_present[pwm_channel] = true;

Is there a reason we can't have the present state as a member of the params struct?

> +
> +	/* use default */
> +	aspeed_set_pwm_channel_fan_ctrl(dev,
> +					priv,
> +					pwm_channel,
> +					priv->pwm_channel[pwm_channel].falling);
> +}
> +
> +static void aspeed_create_fan_tach_channel(struct 
> aspeed_pwm_tachometer_data *priv,
> +					   u8 *fan_tach_ch, int count,
> +					   u32 fan_pulse_pr, u32 tacho_div)
> +{
> +	u8 val, index;
> +
> +	for (val = 0; val < count; val++) {
> +		index = fan_tach_ch[val];
> +		priv->fan_tach_present[index] = true;

Same query as pwm_present: Can we not have this as member of the params struct?

> +		priv->tacho_channel[index].pulse_pr = fan_pulse_pr;
> +		priv->tacho_channel[index].limited_inverse = 0;
> +		priv->tacho_channel[index].threshold = 0;
> +		priv->tacho_channel[index].tacho_edge = F2F_EDGES;
> +		priv->tacho_channel[index].tacho_debounce = DEBOUNCE_3_CLK;
> +		aspeed_set_fan_tach_ch_enable(priv, index, true, tacho_div);
> +	}
> +}
> +
> +static int
> +aspeed_pwm_cz_get_max_state(struct thermal_cooling_device *tcdev,
> +			    unsigned long *state)
> +{
> +	struct aspeed_cooling_device *cdev = tcdev->devdata;
> +
> +	*state = cdev->max_state;
> +
> +	return 0;
> +}
> +
> +static int
> +aspeed_pwm_cz_get_cur_state(struct thermal_cooling_device *tcdev,
> +			    unsigned long *state)
> +{
> +	struct aspeed_cooling_device *cdev = tcdev->devdata;
> +
> +	*state = cdev->cur_state;
> +
> +	return 0;
> +}
> +
> +static int
> +aspeed_pwm_cz_set_cur_state(struct thermal_cooling_device *tcdev,
> +			    unsigned long state)
> +{
> +	struct aspeed_cooling_device *cdev = tcdev->devdata;
> +
> +	if (state > cdev->max_state)
> +		return -EINVAL;
> +
> +	cdev->cur_state = state;
> +	cdev->priv->pwm_channel[cdev->pwm_channel].falling =
> +					cdev->cooling_levels[cdev->cur_state];
> +	aspeed_set_pwm_channel_fan_ctrl(&tcdev->device, cdev->priv, 
> cdev->pwm_channel,
> +				     cdev->cooling_levels[cdev->cur_state]);
> +
> +	return 0;
> +}
> +
> +static const struct thermal_cooling_device_ops aspeed_pwm_cool_ops = {
> +	.get_max_state = aspeed_pwm_cz_get_max_state,
> +	.get_cur_state = aspeed_pwm_cz_get_cur_state,
> +	.set_cur_state = aspeed_pwm_cz_set_cur_state,
> +};
> +
> +static int aspeed_create_pwm_cooling(struct device *dev,
> +				     struct device_node *child,
> +				     struct aspeed_pwm_tachometer_data *priv,
> +				     u32 pwm_channel, u8 num_levels)
> +{
> +	int ret;
> +	struct aspeed_cooling_device *cdev;
> +
> +	cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
> +	if (!cdev)
> +		return -ENOMEM;
> +
> +	cdev->cooling_levels = devm_kzalloc(dev, num_levels, GFP_KERNEL);
> +	if (!cdev->cooling_levels)
> +		return -ENOMEM;
> +
> +	cdev->max_state = num_levels - 1;
> +	ret = of_property_read_u8_array(child, "cooling-levels",
> +					cdev->cooling_levels,
> +					num_levels);
> +	if (ret) {
> +		dev_err(dev, "Property 'cooling-levels' cannot be read.\n");
> +		return ret;
> +	}
> +	snprintf(cdev->name, MAX_CDEV_NAME_LEN, "%s%d", child->name, 
> pwm_channel);
> +
> +	cdev->tcdev = thermal_of_cooling_device_register(child,
> +							 cdev->name,
> +							 cdev,
> +							 &aspeed_pwm_cool_ops);
> +	if (IS_ERR(cdev->tcdev))
> +		return PTR_ERR(cdev->tcdev);
> +
> +	cdev->priv = priv;
> +	cdev->pwm_channel = pwm_channel;
> +
> +	priv->cdev[pwm_channel] = cdev;
> +
> +	return 0;
> +}

I'm not super familiar with the cooling zones stuff, so I'll leave that for someone else to comment on :)

> +
> +static int aspeed_pwm_create_fan(struct device *dev,
> +			     struct device_node *child,
> +			     struct aspeed_pwm_tachometer_data *priv)
> +{
> +	u8 *fan_tach_ch;
> +	u32 fan_pulse_pr;
> +	u32 tacho_div;
> +	u32 pwm_channel;
> +	u32 target_pwm_freq = 0;
> +	u8 val;
> +	int ret, count;
> +
> +	ret = of_property_read_u32(child, "reg", &pwm_channel);
> +	if (ret)
> +		return ret;
> +	else if (pwm_channel > 0x0f)
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32(child, "aspeed,pwm-freq-hz", 
> &target_pwm_freq);
> +	if (ret)
> +		target_pwm_freq = DEFAULT_TARGET_PWM_FREQ;
> +
> +	priv->pwm_channel[pwm_channel].target_freq = target_pwm_freq;

I made some comments on the binding patch, but I'll repeat some of that here.

What are your thoughts on reusing a property like 'bus-frequency' here?

> +
> +	priv->pwm_channel[pwm_channel].load_wdt_enable = 
> of_property_read_bool(child,
> +			"aspeed,enable-pwm-duty-load-wdt");

This isn't documented in the binding.

> +	priv->pwm_channel[pwm_channel].load_wdt_selection = 
> of_property_read_bool(child,
> +			"aspeed,wdt-selection-rising");

This isn't documented in the binding.

> +	priv->pwm_channel[pwm_channel].duty_sync_disable = 
> of_property_read_bool(child,
> +			"aspeed,disable-duty-instant-change");

This isn't documented in the binding.

However, does slowly increase/decrease the duty instead? Might it be better to name the property in terms of the positive case (the property name doesn't have to directly reflect the name of the hardware setting), e.g. 'aspeed,ramp-pwm-duty'?

> +	priv->pwm_channel[pwm_channel].inverse_pin = 
> of_property_read_bool(child,
> +			"aspeed,inverse-pin");
> +
> +	ret = of_property_read_u8(child, "aspeed,wdt-rising-falling-point", 
> &val);

This isn't documented in the binding.

> +	if (ret)
> +		val = DEFAULT_WDT_RISING_FALLING_PT;
> +	priv->pwm_channel[pwm_channel].load_wdt_rising_falling_pt = val;
> +
> +	ret = of_property_read_u8(child, "aspeed,falling-point", &val);
> +	if (ret)
> +		val = DEFAULT_FALLING_PT;
> +	priv->pwm_channel[pwm_channel].falling = val;
> +
> +	aspeed_create_pwm_channel(dev, priv, (u8)pwm_channel);
> +
> +	ret = of_property_count_u8_elems(child, "cooling-levels");
> +	if (ret > 0) {
> +		if (IS_ENABLED(CONFIG_THERMAL)) {
> +			ret = aspeed_create_pwm_cooling(dev, child, priv, pwm_channel,
> +							ret);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	count = of_property_count_u8_elems(child, "fan-tach-ch");
> +	if (count < 1)
> +		return -EINVAL;
> +
> +	fan_tach_ch = devm_kzalloc(dev, sizeof(*fan_tach_ch) * count,
> +				   GFP_KERNEL);
> +	if (!fan_tach_ch)
> +		return -ENOMEM;
> +	ret = of_property_read_u8_array(child, "fan-tach-ch",
> +					fan_tach_ch, count);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u32(child, "pulses-per-revolution", 
> &fan_pulse_pr);
> +	if (ret)
> +		fan_pulse_pr = DEFAULT_FAN_PULSE_PR;
> +
> +	ret = of_property_read_u32(child, "aspeed,tacho-div", &tacho_div);

This isn't documented in the binding.

> +	if (ret)
> +		tacho_div = DEFAULT_TACHO_DIV;
> +
> +	aspeed_create_fan_tach_channel(priv, fan_tach_ch, count, 
> fan_pulse_pr, tacho_div);
> +
> +	return 0;
> +}
> +
> +static umode_t aspeed_pwm_tacho_is_visible(const void *data, enum 
> hwmon_sensor_types type,
> +					u32 attr, int channel)
> +{
> +	const struct aspeed_pwm_tachometer_data *priv = data;
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			if (!priv->pwm_present[channel])
> +				return 0;
> +			return 0644;
> +		default:
> +			return 0;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			if (!priv->fan_tach_present[channel])
> +				return 0;
> +			return 0444;
> +		default:
> +			return 0;
> +		}
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int aspeed_pwm_tacho_read(struct device *dev, enum 
> hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	struct aspeed_pwm_tachometer_data *priv = dev_get_drvdata(dev);
> +	long rpm;
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			*val = priv->pwm_channel[channel].falling;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			rpm = aspeed_get_fan_tach_ch_rpm(dev, priv, channel);
> +			if (rpm < 0)
> +				return rpm;
> +			*val = rpm;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int aspeed_pwm_tacho_write(struct device *dev, enum 
> hwmon_sensor_types type,
> +			u32 attr, int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			return set_pwm(dev, channel, val);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_ops aspeed_pwm_hwmon_ops = {
> +	.is_visible = aspeed_pwm_tacho_is_visible,
> +	.read = aspeed_pwm_tacho_read,
> +	.write = aspeed_pwm_tacho_write,
> +};
> +
> +static const struct hwmon_channel_info *aspeed_pwm_hwmon_info[] = {
> +	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),
> +	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),
> +	NULL,
> +};
> +
> +static const struct hwmon_chip_info aspeed_pwm_chip_info = {
> +	.ops = &aspeed_pwm_hwmon_ops,
> +	.info = aspeed_pwm_hwmon_info,
> +};
> +
> +static int aspeed_pwm_tachometer_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np, *child;
> +	struct aspeed_pwm_tachometer_data *priv;
> +	void __iomem *regs;
> +	struct device *hwmon;
> +	struct clk *clk;
> +	int ret;
> +
> +	np = dev->of_node;
> +
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->pwm_channel = default_pwm_params;
> +	priv->tacho_channel = default_tacho_params;
> +	priv->regmap = devm_regmap_init(dev, NULL, (__force void *)regs,
> +			&aspeed_pwm_tachometer_regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +
> +	clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(clk))
> +		return -ENODEV;
> +	priv->clk_freq = clk_get_rate(clk);
> +
> +	priv->reset = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->reset)) {
> +		dev_err(&pdev->dev, "can't get aspeed_pwm_tacho reset\n");
> +		return PTR_ERR(priv->reset);
> +	}
> +
> +	/* scu init */
> +	reset_control_assert(priv->reset);
> +	reset_control_deassert(priv->reset);
> +
> +	for_each_child_of_node(np, child) {
> +		ret = aspeed_pwm_create_fan(dev, child, priv);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;

Should you reassert the reset here? Or is that handled by the devm infra?

> +		}
> +	}
> +
> +	dev_info(dev, "pwm tach probe done\n");

This is a bit misleading because you're yet to register with hwmon :) I'd put it after the call below.

Anyway, I think it should probably print something more informative if we're going to keep it. Print the number of PWMs/tachos configured as well?

> +	hwmon = devm_hwmon_device_register_with_info(dev, 
> "aspeed_pwm_tachometer",
> +			priv, &aspeed_pwm_chip_info, NULL);
> +
> +	return PTR_ERR_OR_ZERO(hwmon);
> +}
> +
> +static const struct of_device_id of_pwm_tachometer_match_table[] = {
> +	{ .compatible = "aspeed,ast2600-pwm-tachometer", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_tachometer_match_table);
> +
> +static struct platform_driver aspeed_pwm_tachometer_driver = {
> +	.probe		= aspeed_pwm_tachometer_probe,
> +	.driver		= {
> +		.name	= "aspeed_pwm_tachometer",
> +		.of_match_table = of_pwm_tachometer_match_table,
> +	},
> +};
> +
> +module_platform_driver(aspeed_pwm_tachometer_driver);
> +
> +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> +MODULE_DESCRIPTION("ASPEED PWM and Fan Tachometer device driver");

Given it's AST2600-specific:

"AST2600 PWM and Fan Tachometer device driver"

I've run out of steam, so that's enough for now :)

Andrew

Re: [PATCH v3 4/4] hwmon: Support Aspeed AST2600 PWM/Fan tachometer

[Troy Lee]

Hi Andrew,

The 01/22/2021 12:38, Andrew Jeffery wrote:
> Hi Troy,
> 
> On Mon, 18 Jan 2021, at 17:20, Troy Lee wrote:
> > Add Aspeed AST2600 PWM/Fan tacho driver. AST2600 has 16 PWM channel and
> > 16 FAN tacho channel.
> > 
> > Changes since v2:
> >  - declare local function as static function
> > 
> > Changes since v1:
> >  - fixed review comments
> >  - fixed double-looped calculation of div_h and div_l
> >  - moving configuration to device tree
> >  - register hwmon driver with devm_hwmon_device_register_with_info()
> > 
> > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > ---
...

> > diff --git a/drivers/hwmon/aspeed2600-pwm-tacho.c 
> > b/drivers/hwmon/aspeed2600-pwm-tacho.c
> > new file mode 100644
> > index 000000000000..00ff100db92f
> > --- /dev/null
> > +++ b/drivers/hwmon/aspeed2600-pwm-tacho.c
> > @@ -0,0 +1,756 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) ASPEED Technology Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 or 
> > later as
> > + * published by the Free Software Foundation.
> > + */
> 
> The license is captured by the SPDX marker above. This summary text should be dropped (though retain the copyright line).
> 
> > +
> > +#include <linux/clk.h>
> > +#include <linux/errno.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/delay.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/reset.h>
> > +#include <linux/regmap.h>
> > +#include <linux/thermal.h>
> > +/**********************************************************
> > + * PWM HW register offset define
> > + *********************************************************/
> 
> Banner comments like this generally aren't preferred. If you think the comment text itself is necessary, just do:
> 
> /* PWM HW register offset define */
> 
> However, the macro names should indicate what we're defining anyway, so I feel we can remove it entirely without too much loss
> 
I'll remove the redundant description and comments.

> > +/* PWM Control Register */
> > +#define ASPEED_PWM_CTRL_CH(ch)			(((ch) * 0x10) + 0x00)
> > +/* PWM Duty Cycle Register */
> > +#define ASPEED_PWM_DUTY_CYCLE_CH(ch)		(((ch) * 0x10) + 0x04)
> > +/* TACH Control Register */
> > +#define ASPEED_TACHO_CTRL_CH(ch)		(((ch) * 0x10) + 0x08)
> > +/* TACH Status Register */
> > +#define ASPEED_TACHO_STS_CH(x)			(((x) * 0x10) + 0x0C)
> > +
> > +/**********************************************************
> > + * PWM register Bit field
> > + *********************************************************/
> > +/*PWM_CTRL */
> > +#define PWM_LOAD_SEL_AS_WDT_BIT		(19)	/* load selection as WDT */
> 
> The trailing comments don't really add any further information in the context of the macro name. Let's remove these as well.
> 
> > +#define PWM_DUTY_LOAD_AS_WDT_EN		BIT(18)	/* enable PWM duty load as 
> > WDT */
> > +#define PWM_DUTY_SYNC_DIS		BIT(17)	/* disable PWM duty sync */
> > +#define PWM_CLK_ENABLE			BIT(16)	/* enable PWM clock */
> > +#define PWM_LEVEL_OUTPUT		BIT(15)	/* output PWM level */
> > +#define PWM_INVERSE			BIT(14) /* inverse PWM pin */
> > +#define PWM_OPEN_DRAIN_EN		BIT(13)	/* enable open-drain */
> > +#define PWM_PIN_EN			BIT(12) /* enable PWM pin */
> > +#define PWM_CLK_DIV_H_MASK		(0xf << 8) /* PWM clock division H bit 
> > [3:0] */
> > +#define PWM_CLK_DIV_L_MASK		(0xff)	/* PWM clock division H bit [3:0] */
> > +/* [19] */
> > +#define LOAD_SEL_FALLING 0
> > +#define LOAD_SEL_RIGING  1
> 
> It's much easier to track these macros if you place them alongside the associated register, and in a way that makes them self-documenting (i.e. we remove the need for the banner comments). Often the bit/mask macros are indented to help with visuals, for example:
> 
> #define ASPEED_PWM_CTRL_CH(ch)			(((ch) * 0x10) + 0x00)
> #define     PWM_DUTY_LOAD_AS_WDT_EN		BIT(18)
> #define     PWM_DUTY_SYNC_DIS                             BIT(17)
> ...
> 
> #define ASPEED_PWM_DUTY_CYCLE_CH(ch)			(((ch) * 0x10) + 0x04)
> #define     PWM_PERIOD_BIT						(24)
> #define     PWM_PERIOD_BIT_MASK					(0xff << 24)
> ...
> 
> etcetera. It also helps to make the name of the bitfield macros use a similar prefix to name of the associated register macro.
> 
Yes, this will increase readibility, I'll re-order these in v4.

> > +
> > +/*PWM_DUTY_CYCLE */
> > +#define PWM_PERIOD_BIT			(24)		/* pwm period bit [7:0] */
> > +#define PWM_PERIOD_BIT_MASK		(0xff << 24)	/* pwm period bit [7:0] */
> > +#define PWM_RISING_FALLING_AS_WDT_BIT	(16)
> > +#define PWM_RISING_FALLING_AS_WDT_MASK	(0xff << 16)	/* rising/falling 
> > point bit [7:0] as WDT */
> > +#define PWM_RISING_FALLING_MASK		(0xffff)
> > +#define PWM_FALLING_POINT_BIT		(8)		/* pwm falling point bit [7:0] */
> > +#define PWM_RISING_POINT_BIT		(0)		/* pwm rising point bit [7:0] */
> > +/* [31:24] */
> > +#define DEFAULT_PWM_PERIOD 0xff
> > +
> > +/*PWM_TACHO_CTRL */
> > +#define TACHO_IER				BIT(31)	/* enable tacho interrupt */
> > +#define TACHO_INVERS_LIMIT			BIT(30) /* inverse tacho limit comparison 
> > */
> > +#define TACHO_LOOPBACK				BIT(29) /* tacho loopback */
> > +#define TACHO_ENABLE				BIT(28)	/* enable tacho */
> > +#define TACHO_DEBOUNCE_MASK			(0x3 << 26) /* tacho de-bounce */
> > +#define TACHO_DEBOUNCE_BIT			(26) /* tacho de-bounce */
> > +#define TECHIO_EDGE_MASK			(0x3 << 24) /* tacho edge */
> > +#define TECHIO_EDGE_BIT				(24) /* tacho edge */
> > +#define TACHO_CLK_DIV_T_MASK			(0xf << 20)
> > +#define TACHO_CLK_DIV_BIT			(20)
> > +#define TACHO_THRESHOLD_MASK			(0xfffff)	/* tacho threshold bit */
> > +/* [27:26] */
> > +#define DEBOUNCE_3_CLK 0x00 /* 10b */
> > +#define DEBOUNCE_2_CLK 0x01 /* 10b */
> > +#define DEBOUNCE_1_CLK 0x02 /* 10b */
> > +#define DEBOUNCE_0_CLK 0x03 /* 10b */
> > +/* [25:24] */
> > +#define F2F_EDGES 0x00 /* 10b */
> > +#define R2R_EDGES 0x01 /* 10b */
> > +#define BOTH_EDGES 0x02 /* 10b */
> > +/* [23:20] */
> > +/* Cover rpm range 5~5859375 */
> > +#define  DEFAULT_TACHO_DIV 5
> > +
> > +/*PWM_TACHO_STS */
> > +#define  TACHO_ISR		BIT(31)	/* interrupt status and clear */
> > +#define  PWM_OUT		BIT(25)	/* pwm_out */
> > +#define  PWM_OEN		BIT(24)	/* pwm_oeN */
> > +#define  TACHO_DEB_INPUT	BIT(23)	/* tacho deB input */
> > +#define  TACHO_RAW_INPUT	BIT(22) /* tacho raw input */
> > +#define  TACHO_VALUE_UPDATE	BIT(21)	/* tacho value updated since the 
> > last read */
> > +#define  TACHO_FULL_MEASUREMENT	BIT(20) /* tacho full measurement */
> > +#define  TACHO_VALUE_MASK	0xfffff	/* tacho value bit [19:0] */
> > +/**********************************************************
> > + * Software setting
> > + *********************************************************/
> > +#define DEFAULT_TARGET_PWM_FREQ		25000
> > +#define DEFAULT_FAN_PULSE_PR		2
> > +#define DEFAULT_WDT_RISING_FALLING_PT	0x10
> > +#define DEFAULT_RISING_PT		0x00
> > +#define DEFAULT_FALLING_PT		0x0a
> > +#define MAX_CDEV_NAME_LEN		16
> > +
> > +struct aspeed_pwm_channel_params {
> > +	int target_freq;
> > +	int pwm_freq;
> > +	int load_wdt_rising_falling_pt;
> > +	int load_wdt_selection;		/* 0: rising , 1: falling */
> > +	int load_wdt_enable;
> > +	int duty_sync_disable;
> > +	int inverse_pin;
> > +	u8 falling;
> > +};
> > +
> > +static struct aspeed_pwm_channel_params default_pwm_params[16];
> > +
> > +struct aspeed_tacho_channel_params {
> > +	int limited_inverse;
> > +	u16 threshold;
> > +	u8 tacho_edge;
> > +	u8 tacho_debounce;
> > +	u8 pulse_pr;
> > +	u32 divide;
> > +};
> > +
> > +static struct aspeed_tacho_channel_params default_tacho_params[16];
> > +
> > +struct aspeed_pwm_tachometer_data {
> > +	struct regmap *regmap;
> 
> What's the reason for using a regmap? For MMIO the benefit is that the API provides some locking for when regmap is used as a syscon, but that doesn't appear to be how it's used here? regmap generates a bunch of overhead compared to readl()/writel().
> 
Nope, the readl()/writel() will work as well.

> > +	unsigned long clk_freq;
> > +	struct reset_control *reset;
> > +	bool pwm_present[16];
> > +	bool fan_tach_present[16];
> > +	struct aspeed_pwm_channel_params *pwm_channel;
> > +	struct aspeed_tacho_channel_params *tacho_channel;
> > +	/* for thermal */
> > +	struct aspeed_cooling_device *cdev[8];
> > +};
> > +
> > +struct aspeed_cooling_device {
> > +	char name[16];
> > +	struct aspeed_pwm_tachometer_data *priv;
> > +	struct thermal_cooling_device *tcdev;
> > +	int pwm_channel;
> > +	u8 *cooling_levels;
> > +	u8 max_state;
> > +	u8 cur_state;
> > +};
> > +
> > +static int regmap_aspeed_pwm_tachometer_reg_write(void *context, 
> > unsigned int reg,
> > +					     unsigned int val)
> > +{
> > +	void __iomem *regs = (void __iomem *)context;
> > +
> > +	writel(val, regs + reg);
> > +	return 0;
> > +}
> > +
> > +static int regmap_aspeed_pwm_tachometer_reg_read(void *context, 
> > unsigned int reg,
> > +					    unsigned int *val)
> > +{
> > +	void __iomem *regs = (void __iomem *)context;
> > +
> > +	*val = readl(regs + reg);
> > +	return 0;
> > +}
> > +
> > +static const struct regmap_config aspeed_pwm_tachometer_regmap_config 
> > = {
> > +	.reg_bits = 32,
> > +	.val_bits = 32,
> > +	.reg_stride = 4,
> > +	.max_register = 0x100,
> > +	.reg_write = regmap_aspeed_pwm_tachometer_reg_write,
> > +	.reg_read = regmap_aspeed_pwm_tachometer_reg_read,
> 
> These just do an MMIO access, do we really need our own read/write definitions?
> 
> > +	.fast_io = true,
> > +};
> > +
> > +static void aspeed_set_pwm_channel_enable(struct regmap *regmap, u8 
> > pwm_channel,
> > +				       bool enable)
> > +{
> > +	regmap_update_bits(regmap, ASPEED_PWM_CTRL_CH(pwm_channel),
> > +			   PWM_CLK_ENABLE | PWM_PIN_EN,
> > +			   enable ? PWM_CLK_ENABLE | PWM_PIN_EN : 0);
> 
> If the motivation for regmap is something like regmap_update_bits(), you can alternatively use the FIELD_*() macros from linux/bitfield.h
> 
Got it.

> > +}
> > +
> > +static void aspeed_set_fan_tach_ch_enable(struct 
> > aspeed_pwm_tachometer_data *priv, u8 fan_tach_ch,
> > +					  bool enable, u32 tacho_div)
> > +{
> > +	u32 reg_value;
> > +
> > +	if (enable) {
> > +		/* divide = 2^(tacho_div*2) */
> > +		priv->tacho_channel[fan_tach_ch].divide = 1 << (tacho_div << 1);
> > +
> > +		reg_value = TACHO_ENABLE |
> > +				(priv->tacho_channel[fan_tach_ch].tacho_edge << TECHIO_EDGE_BIT) |
> > +				(tacho_div << TACHO_CLK_DIV_BIT) |
> > +				(priv->tacho_channel[fan_tach_ch].tacho_debounce << 
> > TACHO_DEBOUNCE_BIT);
> > +
> > +		if (priv->tacho_channel[fan_tach_ch].limited_inverse)
> > +			reg_value |= TACHO_INVERS_LIMIT;
> > +
> > +		if (priv->tacho_channel[fan_tach_ch].threshold)
> > +			reg_value |= (TACHO_IER | 
> > priv->tacho_channel[fan_tach_ch].threshold);
> 
> I feel like this could be cleaned up a bit with:
> 
> const struct aspeed_tacho_channel_params *tacho = &priv->tacho_channel[fan_tach_ch];
> 
> and replacing the expression with tacho throughout.
> 
After replacing the expression, the code looks cleaner.

> > +
> > +		regmap_write(priv->regmap, ASPEED_TACHO_CTRL_CH(fan_tach_ch), 
> > reg_value);
> > +	} else
> > +		regmap_update_bits(priv->regmap, ASPEED_TACHO_CTRL_CH(fan_tach_ch),  
> > TACHO_ENABLE, 0);
> > +}
> > +
> > +/*
> > + * The PWM frequency = HCLK(200Mhz) / (clock division L bit *
> > + * clock division H bit * (period bit + 1))
> > + */
> > +static void aspeed_set_pwm_channel_fan_ctrl(struct device *dev,
> > +					   struct aspeed_pwm_tachometer_data *priv,
> > +					   u8 index, u8 fan_ctrl)
> > +{
> > +	u32 duty_value,	ctrl_value;
> > +	u32 target_div, div_h, div_l, cal_freq;
> > +	u32 tmp_div_h, tmp_div_l, diff, min_diff = INT_MAX;
> > +
> > +	if (fan_ctrl == 0) {
> > +		aspeed_set_pwm_channel_enable(priv->regmap, index, false);
> > +		return;
> > +	}
> > +
> > +	cal_freq = priv->clk_freq / (DEFAULT_PWM_PERIOD + 1);
> 
> Is this always correct if we're relying on a default value? Or are we using DEFAULT_PWM_PERIOD because its value is the maximum period? Using a macro with DEFAULT in the name certainly makes me feel less sure that the code is right.
> 
I'll check again about this.

> > +	target_div = DIV_ROUND_UP(cal_freq, 
> > priv->pwm_channel[index].target_freq);
> > +
> > +	/* calculate for target frequence */
> > +	for (tmp_div_h = 0; tmp_div_h < 0x10; tmp_div_h++) {
> > +		tmp_div_l = target_div / BIT(tmp_div_h) - 1;
> > +
> > +		if (tmp_div_l < 0 || tmp_div_l > 255)
> > +			continue;
> > +
> > +		diff = priv->pwm_channel[index].target_freq - cal_freq / 
> > (BIT(tmp_div_h) * (tmp_div_l + 1));
> > +		if (abs(diff) < abs(min_diff)) {
> > +			min_diff = diff;
> > +			div_l = tmp_div_l;
> > +			div_h = tmp_div_h;
> > +
> > +			if (diff == 0)
> > +				break;
> > +		}
> > +	}
> 
> I've spent some time thinking about this and haven't come up with anything that's concretely better. It wasn't fun to read though :(
> 
Yep, me too. The intention of this segment is finding a valid pair of DIV_H and DIV_L, and it will be used as clock prescaler. 

> > +
> > +	priv->pwm_channel[index].pwm_freq = cal_freq / (BIT(div_h) * (div_l + 
> > 1));
> > +	dev_info(dev, "div h %x, l : %x pwm out clk %d\n", div_h, div_l,
> > +			priv->pwm_channel[index].pwm_freq);
> > +	dev_info(dev, "hclk %ld, target pwm freq %d, real pwm freq %d\n", 
> > priv->clk_freq,
> > +			priv->pwm_channel[index].target_freq, 
> > priv->pwm_channel[index].pwm_freq);
> 
> These should be dev_dbg()
> 
Updated.

> > +
> > +	ctrl_value = (div_h << 8) | div_l;
> > +
> > +	duty_value = (DEFAULT_PWM_PERIOD << PWM_PERIOD_BIT) |
> > +		(0 << PWM_RISING_POINT_BIT) | (fan_ctrl << PWM_FALLING_POINT_BIT);
> > +
> > +	if (priv->pwm_channel[index].load_wdt_enable) {
> > +		ctrl_value |= PWM_DUTY_LOAD_AS_WDT_EN;
> > +		ctrl_value |= priv->pwm_channel[index].load_wdt_selection << 
> > PWM_LOAD_SEL_AS_WDT_BIT;
> > +		duty_value |= (priv->pwm_channel[index].load_wdt_rising_falling_pt 
> > << PWM_RISING_FALLING_AS_WDT_BIT);
> > +	}
> > +
> > +	if (priv->pwm_channel[index].inverse_pin)
> > +		ctrl_value |= PWM_INVERSE;
> > +	if (priv->pwm_channel[index].duty_sync_disable)
> > +		ctrl_value |= PWM_DUTY_SYNC_DIS;
> 
> Similar to my comment above about tacho, I think this can be cleaned up a bit with:
> 
> const struct aspeed_pwm_channel_params *pwm = &priv->pwm_channel[index];
> 
> and replacing the expression with pwm throughout.
> 
Updated.

> > +
> > +	regmap_write(priv->regmap, ASPEED_PWM_DUTY_CYCLE_CH(index), 
> > duty_value);
> > +	regmap_write(priv->regmap, ASPEED_PWM_CTRL_CH(index), ctrl_value);
> > +
> > +	aspeed_set_pwm_channel_enable(priv->regmap, index, true);
> > +}
> > +
> > +static int aspeed_get_fan_tach_ch_rpm(struct device *dev, struct 
> > aspeed_pwm_tachometer_data *priv,
> > +				      u8 fan_tach_ch)
> > +{
> > +	u32 raw_data, tach_div, clk_source, val;
> > +	int ret;
> > +
> > +	ret = regmap_read_poll_timeout(priv->regmap,
> > +				ASPEED_TACHO_STS_CH(fan_tach_ch),
> > +				val,
> > +				(val & TACHO_FULL_MEASUREMENT),
> > +				1000,
> > +				10000);
> 
> Could just use readl_poll_timeout()?
> 
Updated accordingly.

> > +
> > +	/* return -ETIMEDOUT if we didn't get an answer. */
> > +	if (ret)
> > +		return ret;
> > +
> > +	raw_data = val & TACHO_VALUE_MASK;
> > +	if (raw_data == 0xfffff)
> > +		return 0;
> > +
> > +	raw_data += 1;
> > +
> > +	/*
> > +	 * We need the mode to determine if the raw_data is double (from
> > +	 * counting both edges).
> > +	 */
> > +	tach_div = raw_data * (priv->tacho_channel[fan_tach_ch].divide) * 
> > (priv->tacho_channel[fan_tach_ch].pulse_pr);
> 
> Again, lets extract the channel pointer to clean this up a little.
> 
Updated.

> > +
> > +	dev_dbg(dev, "clk %ld, raw_data %d , tach_div %d\n", priv->clk_freq, 
> > raw_data, tach_div);
> > +	clk_source = priv->clk_freq;
> > +
> > +	return ((clk_source / tach_div) * 60);
> > +
> > +}
> > +
> > +static int set_pwm(struct device *dev, int index, long fan_ctrl)
> > +{
> > +	struct aspeed_pwm_tachometer_data *priv = dev_get_drvdata(dev);
> > +	u8 org_falling = priv->pwm_channel[index].falling;
> > +
> > +	if (fan_ctrl > DEFAULT_PWM_PERIOD || fan_ctrl < 0)
> > +		return -EINVAL;
> > +
> > +	if (priv->pwm_channel[index].falling == fan_ctrl)
> > +		return 0;
> > +
> > +	priv->pwm_channel[index].falling = fan_ctrl;
> 
> My understanding is the value of fan_ctrl is the value that userspace has written to the pwm[1-*] attribute in sysfs. The name 'falling' in struct aspeed_pwm_channel_params is not at all intuitive to me. I understand the hardware calls it something along these lines, but is there a better name? "period"? "duty"?
> 
Good suggestion, I'll update accordingly.

> > +
> > +	if (fan_ctrl == 0)
> > +		aspeed_set_pwm_channel_enable(priv->regmap, index, false);
> > +	else {
> > +		if (fan_ctrl == DEFAULT_PWM_PERIOD)
> > +			regmap_update_bits(priv->regmap,
> > +					   ASPEED_PWM_DUTY_CYCLE_CH(index),
> > +					   GENMASK(15, 0), 0);
> > +		else
> > +			regmap_update_bits(priv->regmap,
> > +					   ASPEED_PWM_DUTY_CYCLE_CH(index),
> > +					   GENMASK(15, 8),
> 
> The asymmetry between these masks caught my eye, and I guess I understand what's going on a bit more with 'falling' at least. The hardware is definitely flexible :)
> 
> > +					   (fan_ctrl << PWM_FALLING_POINT_BIT));
> > +	}
> > +
> > +	if (org_falling == 0)
> 
> Do we have to assign .falling member before this point? Can we get rid of org_falling by assigning .falling after this test?
> 
Yes, good suggestion.

> > +		aspeed_set_pwm_channel_enable(priv->regmap, index, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static void aspeed_create_pwm_channel(struct device *dev, struct 
> > aspeed_pwm_tachometer_data *priv,
> > +			   u8 pwm_channel)
> > +{
> > +	priv->pwm_present[pwm_channel] = true;
> 
> Is there a reason we can't have the present state as a member of the params struct?
> 
Nope. I'll move the present into param struct.
> > +
> > +	/* use default */
> > +	aspeed_set_pwm_channel_fan_ctrl(dev,
> > +					priv,
> > +					pwm_channel,
> > +					priv->pwm_channel[pwm_channel].falling);
> > +}
> > +
> > +static void aspeed_create_fan_tach_channel(struct 
> > aspeed_pwm_tachometer_data *priv,
> > +					   u8 *fan_tach_ch, int count,
> > +					   u32 fan_pulse_pr, u32 tacho_div)
> > +{
> > +	u8 val, index;
> > +
> > +	for (val = 0; val < count; val++) {
> > +		index = fan_tach_ch[val];
> > +		priv->fan_tach_present[index] = true;
> 
> Same query as pwm_present: Can we not have this as member of the params struct?
> 
Same as pwm.
> > +		priv->tacho_channel[index].pulse_pr = fan_pulse_pr;
> > +		priv->tacho_channel[index].limited_inverse = 0;
> > +		priv->tacho_channel[index].threshold = 0;
> > +		priv->tacho_channel[index].tacho_edge = F2F_EDGES;
> > +		priv->tacho_channel[index].tacho_debounce = DEBOUNCE_3_CLK;
> > +		aspeed_set_fan_tach_ch_enable(priv, index, true, tacho_div);
> > +	}
> > +}
> > +
> > +static int
> > +aspeed_pwm_cz_get_max_state(struct thermal_cooling_device *tcdev,
> > +			    unsigned long *state)
> > +{
> > +	struct aspeed_cooling_device *cdev = tcdev->devdata;
> > +
> > +	*state = cdev->max_state;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +aspeed_pwm_cz_get_cur_state(struct thermal_cooling_device *tcdev,
> > +			    unsigned long *state)
> > +{
> > +	struct aspeed_cooling_device *cdev = tcdev->devdata;
> > +
> > +	*state = cdev->cur_state;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +aspeed_pwm_cz_set_cur_state(struct thermal_cooling_device *tcdev,
> > +			    unsigned long state)
> > +{
> > +	struct aspeed_cooling_device *cdev = tcdev->devdata;
> > +
> > +	if (state > cdev->max_state)
> > +		return -EINVAL;
> > +
> > +	cdev->cur_state = state;
> > +	cdev->priv->pwm_channel[cdev->pwm_channel].falling =
> > +					cdev->cooling_levels[cdev->cur_state];
> > +	aspeed_set_pwm_channel_fan_ctrl(&tcdev->device, cdev->priv, 
> > cdev->pwm_channel,
> > +				     cdev->cooling_levels[cdev->cur_state]);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct thermal_cooling_device_ops aspeed_pwm_cool_ops = {
> > +	.get_max_state = aspeed_pwm_cz_get_max_state,
> > +	.get_cur_state = aspeed_pwm_cz_get_cur_state,
> > +	.set_cur_state = aspeed_pwm_cz_set_cur_state,
> > +};
> > +
> > +static int aspeed_create_pwm_cooling(struct device *dev,
> > +				     struct device_node *child,
> > +				     struct aspeed_pwm_tachometer_data *priv,
> > +				     u32 pwm_channel, u8 num_levels)
> > +{
> > +	int ret;
> > +	struct aspeed_cooling_device *cdev;
> > +
> > +	cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
> > +	if (!cdev)
> > +		return -ENOMEM;
> > +
> > +	cdev->cooling_levels = devm_kzalloc(dev, num_levels, GFP_KERNEL);
> > +	if (!cdev->cooling_levels)
> > +		return -ENOMEM;
> > +
> > +	cdev->max_state = num_levels - 1;
> > +	ret = of_property_read_u8_array(child, "cooling-levels",
> > +					cdev->cooling_levels,
> > +					num_levels);
> > +	if (ret) {
> > +		dev_err(dev, "Property 'cooling-levels' cannot be read.\n");
> > +		return ret;
> > +	}
> > +	snprintf(cdev->name, MAX_CDEV_NAME_LEN, "%s%d", child->name, 
> > pwm_channel);
> > +
> > +	cdev->tcdev = thermal_of_cooling_device_register(child,
> > +							 cdev->name,
> > +							 cdev,
> > +							 &aspeed_pwm_cool_ops);
> > +	if (IS_ERR(cdev->tcdev))
> > +		return PTR_ERR(cdev->tcdev);
> > +
> > +	cdev->priv = priv;
> > +	cdev->pwm_channel = pwm_channel;
> > +
> > +	priv->cdev[pwm_channel] = cdev;
> > +
> > +	return 0;
> > +}
> 
> I'm not super familiar with the cooling zones stuff, so I'll leave that for someone else to comment on :)
> 
> > +
> > +static int aspeed_pwm_create_fan(struct device *dev,
> > +			     struct device_node *child,
> > +			     struct aspeed_pwm_tachometer_data *priv)
> > +{
> > +	u8 *fan_tach_ch;
> > +	u32 fan_pulse_pr;
> > +	u32 tacho_div;
> > +	u32 pwm_channel;
> > +	u32 target_pwm_freq = 0;
> > +	u8 val;
> > +	int ret, count;
> > +
> > +	ret = of_property_read_u32(child, "reg", &pwm_channel);
> > +	if (ret)
> > +		return ret;
> > +	else if (pwm_channel > 0x0f)
> > +		return -EINVAL;
> > +
> > +	ret = of_property_read_u32(child, "aspeed,pwm-freq-hz", 
> > &target_pwm_freq);
> > +	if (ret)
> > +		target_pwm_freq = DEFAULT_TARGET_PWM_FREQ;
> > +
> > +	priv->pwm_channel[pwm_channel].target_freq = target_pwm_freq;
> 
> I made some comments on the binding patch, but I'll repeat some of that here.
> 
> What are your thoughts on reusing a property like 'bus-frequency' here?
> 
> > +
> > +	priv->pwm_channel[pwm_channel].load_wdt_enable = 
> > of_property_read_bool(child,
> > +			"aspeed,enable-pwm-duty-load-wdt");
> 
> This isn't documented in the binding.
> 
> > +	priv->pwm_channel[pwm_channel].load_wdt_selection = 
> > of_property_read_bool(child,
> > +			"aspeed,wdt-selection-rising");
> 
> This isn't documented in the binding.
> 
> > +	priv->pwm_channel[pwm_channel].duty_sync_disable = 
> > of_property_read_bool(child,
> > +			"aspeed,disable-duty-instant-change");
> 
> This isn't documented in the binding.
> 
> However, does slowly increase/decrease the duty instead? Might it be better to name the property in terms of the positive case (the property name doesn't have to directly reflect the name of the hardware setting), e.g. 'aspeed,ramp-pwm-duty'?
> 
I'll check about the functionaility.

> > +	priv->pwm_channel[pwm_channel].inverse_pin = 
> > of_property_read_bool(child,
> > +			"aspeed,inverse-pin");
> > +
> > +	ret = of_property_read_u8(child, "aspeed,wdt-rising-falling-point", 
> > &val);
> 
> This isn't documented in the binding.
> 
> > +	if (ret)
> > +		val = DEFAULT_WDT_RISING_FALLING_PT;
> > +	priv->pwm_channel[pwm_channel].load_wdt_rising_falling_pt = val;
> > +
> > +	ret = of_property_read_u8(child, "aspeed,falling-point", &val);
> > +	if (ret)
> > +		val = DEFAULT_FALLING_PT;
> > +	priv->pwm_channel[pwm_channel].falling = val;
> > +
> > +	aspeed_create_pwm_channel(dev, priv, (u8)pwm_channel);
> > +
> > +	ret = of_property_count_u8_elems(child, "cooling-levels");
> > +	if (ret > 0) {
> > +		if (IS_ENABLED(CONFIG_THERMAL)) {
> > +			ret = aspeed_create_pwm_cooling(dev, child, priv, pwm_channel,
> > +							ret);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +	}
> > +
> > +	count = of_property_count_u8_elems(child, "fan-tach-ch");
> > +	if (count < 1)
> > +		return -EINVAL;
> > +
> > +	fan_tach_ch = devm_kzalloc(dev, sizeof(*fan_tach_ch) * count,
> > +				   GFP_KERNEL);
> > +	if (!fan_tach_ch)
> > +		return -ENOMEM;
> > +	ret = of_property_read_u8_array(child, "fan-tach-ch",
> > +					fan_tach_ch, count);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = of_property_read_u32(child, "pulses-per-revolution", 
> > &fan_pulse_pr);
> > +	if (ret)
> > +		fan_pulse_pr = DEFAULT_FAN_PULSE_PR;
> > +
> > +	ret = of_property_read_u32(child, "aspeed,tacho-div", &tacho_div);
> 
> This isn't documented in the binding.
> 
Missing properties will be added in v4 document.

> > +	if (ret)
> > +		tacho_div = DEFAULT_TACHO_DIV;
> > +
> > +	aspeed_create_fan_tach_channel(priv, fan_tach_ch, count, 
> > fan_pulse_pr, tacho_div);
> > +
> > +	return 0;
> > +}
> > +
> > +static umode_t aspeed_pwm_tacho_is_visible(const void *data, enum 
> > hwmon_sensor_types type,
> > +					u32 attr, int channel)
> > +{
> > +	const struct aspeed_pwm_tachometer_data *priv = data;
> > +
> > +	switch (type) {
> > +	case hwmon_pwm:
> > +		switch (attr) {
> > +		case hwmon_pwm_input:
> > +			if (!priv->pwm_present[channel])
> > +				return 0;
> > +			return 0644;
> > +		default:
> > +			return 0;
> > +		}
> > +		break;
> > +	case hwmon_fan:
> > +		switch (attr) {
> > +		case hwmon_fan_input:
> > +			if (!priv->fan_tach_present[channel])
> > +				return 0;
> > +			return 0444;
> > +		default:
> > +			return 0;
> > +		}
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static int aspeed_pwm_tacho_read(struct device *dev, enum 
> > hwmon_sensor_types type,
> > +			u32 attr, int channel, long *val)
> > +{
> > +	struct aspeed_pwm_tachometer_data *priv = dev_get_drvdata(dev);
> > +	long rpm;
> > +
> > +	switch (type) {
> > +	case hwmon_pwm:
> > +		switch (attr) {
> > +		case hwmon_pwm_input:
> > +			*val = priv->pwm_channel[channel].falling;
> > +			break;
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> > +		break;
> > +	case hwmon_fan:
> > +		switch (attr) {
> > +		case hwmon_fan_input:
> > +			rpm = aspeed_get_fan_tach_ch_rpm(dev, priv, channel);
> > +			if (rpm < 0)
> > +				return rpm;
> > +			*val = rpm;
> > +			break;
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int aspeed_pwm_tacho_write(struct device *dev, enum 
> > hwmon_sensor_types type,
> > +			u32 attr, int channel, long val)
> > +{
> > +	switch (type) {
> > +	case hwmon_pwm:
> > +		switch (attr) {
> > +		case hwmon_pwm_input:
> > +			return set_pwm(dev, channel, val);
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static const struct hwmon_ops aspeed_pwm_hwmon_ops = {
> > +	.is_visible = aspeed_pwm_tacho_is_visible,
> > +	.read = aspeed_pwm_tacho_read,
> > +	.write = aspeed_pwm_tacho_write,
> > +};
> > +
> > +static const struct hwmon_channel_info *aspeed_pwm_hwmon_info[] = {
> > +	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),
> > +	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),
> > +	NULL,
> > +};
> > +
> > +static const struct hwmon_chip_info aspeed_pwm_chip_info = {
> > +	.ops = &aspeed_pwm_hwmon_ops,
> > +	.info = aspeed_pwm_hwmon_info,
> > +};
> > +
> > +static int aspeed_pwm_tachometer_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np, *child;
> > +	struct aspeed_pwm_tachometer_data *priv;
> > +	void __iomem *regs;
> > +	struct device *hwmon;
> > +	struct clk *clk;
> > +	int ret;
> > +
> > +	np = dev->of_node;
> > +
> > +	regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->pwm_channel = default_pwm_params;
> > +	priv->tacho_channel = default_tacho_params;
> > +	priv->regmap = devm_regmap_init(dev, NULL, (__force void *)regs,
> > +			&aspeed_pwm_tachometer_regmap_config);
> > +	if (IS_ERR(priv->regmap))
> > +		return PTR_ERR(priv->regmap);
> > +
> > +	clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(clk))
> > +		return -ENODEV;
> > +	priv->clk_freq = clk_get_rate(clk);
> > +
> > +	priv->reset = devm_reset_control_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->reset)) {
> > +		dev_err(&pdev->dev, "can't get aspeed_pwm_tacho reset\n");
> > +		return PTR_ERR(priv->reset);
> > +	}
> > +
> > +	/* scu init */
> > +	reset_control_assert(priv->reset);
> > +	reset_control_deassert(priv->reset);
> > +
> > +	for_each_child_of_node(np, child) {
> > +		ret = aspeed_pwm_create_fan(dev, child, priv);
> > +		if (ret) {
> > +			of_node_put(child);
> > +			return ret;
> 
> Should you reassert the reset here? Or is that handled by the devm infra?
> 
I don't think we need to reassert here. It should reset once at the
begining.

> > +		}
> > +	}
> > +
> > +	dev_info(dev, "pwm tach probe done\n");
> 
> This is a bit misleading because you're yet to register with hwmon :) I'd put it after the call below.
> 
> Anyway, I think it should probably print something more informative if we're going to keep it. Print the number of PWMs/tachos configured as well?
Good catch. I think this message can be removed. How many pwm/tacho channels configured can be found in sysfs.

> 
> > +	hwmon = devm_hwmon_device_register_with_info(dev, 
> > "aspeed_pwm_tachometer",
> > +			priv, &aspeed_pwm_chip_info, NULL);
> > +
> > +	return PTR_ERR_OR_ZERO(hwmon);
> > +}
> > +
> > +static const struct of_device_id of_pwm_tachometer_match_table[] = {
> > +	{ .compatible = "aspeed,ast2600-pwm-tachometer", },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, of_pwm_tachometer_match_table);
> > +
> > +static struct platform_driver aspeed_pwm_tachometer_driver = {
> > +	.probe		= aspeed_pwm_tachometer_probe,
> > +	.driver		= {
> > +		.name	= "aspeed_pwm_tachometer",
> > +		.of_match_table = of_pwm_tachometer_match_table,
> > +	},
> > +};
> > +
> > +module_platform_driver(aspeed_pwm_tachometer_driver);
> > +
> > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> > +MODULE_DESCRIPTION("ASPEED PWM and Fan Tachometer device driver");
> 
> Given it's AST2600-specific:
> 
> "AST2600 PWM and Fan Tachometer device driver"
> 
> I've run out of steam, so that's enough for now :)
> 
> Andrew

Thanks for reviewing, I'll update and submit a v4 patch.

Troy Lee