[PATCH linux v1 0/2] Support for ASPEED AST2400/AST2500 PWM and Fan Tach driver

[PATCH linux v1 0/2] Support for ASPEED AST2400/AST2500 PWM and Fan Tach driver

[Jaghathiswari Rankappagounder Natarajan]

Support for ASPEED AST2400/AST2500 PWM and Fan Tach driver

Support for ASPEED AST2400/AST2500 PWM and Fan Tach driver.
The AST2400/AST2500 PWM controller can support 8 PWM output ports.
The AST2400/AST2500 Fan Tach controller can support 16 tachometer inputs.
There can be three different PWM clock types (Type M, N and O); Those three
types just to have three independent PWM sources.
The driver provides sysfs entries through which the user can configure the
duty cycle for the particular PWM output port and read the fan rpm value for
the particular tachometer input.
Added devicetree binding documentation for AST2400/AST2500 PWM and Fan Tach
support.

Tested on Zaius board (which has AST2500 chip) and observed that when
duty cycle is lowered then the fan speed is lowered and lower fan rpm value(
corresponding to the duty cycle) is reported and when the duty cycle is
increased then the fan speed increases and higher fan rpm value(corresponding
to the duty cycle) is reported.

Tested on AST2500 EVB board and observed the PWM output pulses come
correctly based on the given settings using Logic Saleae Analyzer.

Jaghathiswari Rankappagounder Natarajan (2):
  Documentation: dt-bindings: Document bindings for aspeed pwm and fan
    tach controller device driver
  drivers: hwmon: hwmon driver for ASPEED AST2400/2500 PWM and Fan tach
    controller

 .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt | 153 ++++
 Documentation/hwmon/aspeed-pwm-tacho               |  22 +
 drivers/hwmon/Kconfig                              |   9 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/aspeed-pwm-tacho.c                   | 884 +++++++++++++++++++++
 5 files changed, 1069 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
 create mode 100644 Documentation/hwmon/aspeed-pwm-tacho
 create mode 100644 drivers/hwmon/aspeed-pwm-tacho.c

--
2.11.0.390.gc69c2f50cf-goog

[PATCH linux v1 1/2] Documentation: dt-bindings: Document bindings for ASPEED AST2400/AST2500 pwm and fan tach controller device driver

[Jaghathiswari Rankappagounder Natarajan]

This binding provides interface for adding values related to ASPEED
AST2400/2500 PWM and Fan tach controller support.
The PWM controller can support upto 8 PWM output ports.
The Fan tach controller can support upto 16 tachometer inputs.
PWM clock types M, N and 0 are three types just to have three independent
PWM sources.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt | 153 +++++++++++++++++++++
 1 file changed, 153 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
new file mode 100644
index 000000000000..8f346409ee8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
@@ -0,0 +1,153 @@
+ASPEED AST2400/AST2500 PWM and Fan Tacho controller device driver
+
+The ASPEED PWM controller can support upto 8 PWM outputs. The ASPEED Fan Tacho
+controller can support upto 16 tachometer inputs. The PWM controller supports
+3 types of frequency mode PWM for fan speed control. PWM clock types M, N and 0
+are 3 types of frequency mode PWM just to have 3 independent PWM sources.
+
+Required properties for pwm_tacho node:
+- #address-cells : should be 1.
+
+- #size-cells : should be 1.
+
+- reg : address and length of the register set for the device.
+
+- pinctrl-names : a pinctrl state named "default" must be defined.
+
+- pinctrl-0 : phandle referencing pin configuration of the AST2400/AST2500 PWM
+	      ports.
+
+- compatible : should be "aspeed,aspeed2400-pwm-tacho" for AST2400 or
+	       "aspeed,aspeed2500-pwm-tacho" for AST2500.
+
+- clocks : a fixed clock providing input clock frequency(PWM
+	   and Fan Tach clock)
+
+type_values subnode format:
+===========================
+Under type_values subnode there can be upto 3 child nodes indicating type M/N/O
+values. Atleast one child node is required.
+
+Required properties for the child node(type M/N/O):
+- pwm_period : indicates type M/N/O PWM period, as per the AST2400/AST2500
+	       datasheet. integer value in the range 0 to 255.
+
+- pwm_clock_division_l : indicates type M/N/O PWM clock division L value,
+			 as per the AST2400/AST2500 datasheet.
+			 integer value in the range 0 to 15.
+			 0 here indicates divide 1, 1 indicates divide 2,
+			 2 indicates divide 4, 3 indicates divide 6, and so on
+			 till 15 indicates divide 30.
+
+- pwm_clock_division_h : indicates type M/N/O PWM clock division H value,
+			 as per the AST2400/AST2500 datasheet.
+			 integer value in the range 0 to 15.
+			 0 here indicates divide 1, 1 indicates divide 2,
+			 2 indicates divide 4, 3 indicates divide 8, and so on
+			 till 15 indicates divide 32768.
+
+- fan_tach_enable : indicates fan tach enable of type M/N/O as per the
+		    AST2400/AST2500 datasheet. boolean value.
+
+- fan_tach_clock_division : indicates fan tach clock division as per the
+			    AST2400/AST2500 datasheet.
+			    integer value in the range 0 to 7.
+			    0 indicates divide 4, 1 indicates divide 16,
+			    2 indicates divide 64, 3 indicates divide 256
+			    and so on till 7 indicates divide 65536.
+
+- fan_tach_mode_selection : indicates fan tach mode mode selection as per the
+			    AST2400/AST2500 datasheet. integer value in the
+			    range 0 to 2. 0 indicates falling edge, 1 indicates
+			    rising edge and 2 indicates both edges.
+
+- fan_tach_period : indicates fan tach period as per the AST2400/AST2500
+		    datasheet. integer value (can be upto 16 bits long).
+
+pwm_port subnode format:
+========================
+Under pwm_port subnode there can upto 8 child nodes each indicating values
+for one of the 8 PWM output ports.
+
+Required properties for each child node(starting from PWM A through PWM H):
+- enable : enable PWM #X port, X ranges from A through H. boolean value.
+
+- type : indicates type selection value of PWM #X port, X ranges from A
+	 through H. integer value in the range 0 to 2;
+	 0 indicates type M, 1 indicates type N, 2 indicates type O.
+
+- fan_ctrl : set the PWM fan control initial value. integer value between
+	     0(off) and 255(full speed).
+
+fan_tach_channel subnode format:
+================================
+Under fan_tach_channel subnode there can be upto 16 child nodes each indicating
+values for one of the 16 fan tach channels.
+
+Required properties for each child node(starting from fan tach #0 through
+fan tach #16):
+- fan-ctrl-gpios : should specify the tachometer input pin on the hardware.
+
+- enable : enable fan tach #X, X ranges from 0 through 16. boolean value.
+
+- pwm_source : indicates PWM source of fan tach #X, X ranges from 0 through 16.
+	       integer value in the range 0 to 7. 0 indicates PWM port A,
+	       1 indicates PWM port B and so on till 7 indicates PWM port H.
+
+Examples:
+
+pwm_tacho_fixed_clk: fixedclk {
+	compatible = "fixed-clock";
+	#clock-cells = <0>;
+	clock-frequency = <24000000>;
+}
+
+pwm_tacho: pwm-tacho-controller@1e786000 {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	reg = <0x1E786000 0x1000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default>;
+	compatible = "aspeed,aspeed2500-pwm-tacho";
+	clocks = <&pwm_tacho_fixed_clk>;
+	type_values {
+		typem {
+			pwm_period = /bits/ 8 <0x5F>;
+			pwm_clock_division_h = /bits/ 8 <0x00>;
+			pwm_clock_division_l = /bits/ 8 <0x05>;
+			fan_tach_enable;
+			fan_tach_clock_division = /bits/ 8 <0x00>;
+			fan_tach_mode_selection = /bits/ 8 <0x00>;
+			fan_tach_period = /bits/ 16 <0x1000>;
+		};
+	};
+
+	pwm_port {
+		pwm_port0 {
+			enable;
+			type = /bits/ 8 <0x00>;
+			fan_ctrl = /bits/ 8 <0xFF>;
+		};
+
+		pwm_port1 {
+			enable;
+			type = /bits/ 8 <0x00>;
+			fan_ctrl = /bits/ 8 <0xFF>;
+		};
+        };
+
+	fan_tach_channel {
+		fan_tach0 {
+			fan-ctrl-gpios = <&gpio ASPEED_GPIO(O, 0) GPIO_ACTIVE_HIGH>;
+			enable;
+			pwm_source = /bits/ 8 <0x00>;
+		};
+
+		fan_tach1 {
+			fan-ctrl-gpios = <&gpio ASPEED_GPIO(O, 1) GPIO_ACTIVE_HIGH>;
+			enable;
+			pwm_source = /bits/ 8 <0x01>;
+		};
+
+	};
+};
--
2.11.0.390.gc69c2f50cf-goog

[PATCH linux v1 2/2] drivers: hwmon: hwmon driver for ASPEED AST2400/2500 PWM and Fan tach controller

[Jaghathiswari Rankappagounder Natarajan]

The ASPEED AST2400/2500 PWM controller supports 8 PWM output ports.
The ASPEED AST2400/2500 Fan tach controller supports 16 tachometer inputs.
PWM clock types M, N and 0 are three types just to have three independent PWM
sources.
The device driver matches on the device tree node. The configuration values
are read from the device tree and written to the respective registers.
The driver provides a sysfs entries through which the user can
configure the duty-cycle value (ranging from 0 to 100 percent) and read the
fan tach rpm value.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 Documentation/hwmon/aspeed-pwm-tacho |  22 +
 drivers/hwmon/Kconfig                |   9 +
 drivers/hwmon/Makefile               |   1 +
 drivers/hwmon/aspeed-pwm-tacho.c     | 884 +++++++++++++++++++++++++++++++++++
 4 files changed, 916 insertions(+)
 create mode 100644 Documentation/hwmon/aspeed-pwm-tacho
 create mode 100644 drivers/hwmon/aspeed-pwm-tacho.c

diff --git a/Documentation/hwmon/aspeed-pwm-tacho b/Documentation/hwmon/aspeed-pwm-tacho
new file mode 100644
index 000000000000..0e9ec6d5f900
--- /dev/null
+++ b/Documentation/hwmon/aspeed-pwm-tacho
@@ -0,0 +1,22 @@
+Kernel driver aspeed-pwm-tacho
+==============================
+
+Supported chips:
+	ASPEED AST2400/2500
+
+Authors:
+	<jaghu@google.com>
+
+Description:
+------------
+This driver implements support for ASPEED AST2400/2500 PWM and Fan Tacho
+controller. The PWM controller supports upto 8 PWM outputs. The Fan tacho
+controller supports upto 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/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 45cef3d2c75c..757b5b0705bf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -341,6 +341,15 @@ config SENSORS_ASB100
 	  This driver can also be built as a module.  If so, the module
 	  will be called asb100.

+config SENSORS_ASPEED
+	tristate "ASPEED AST2400/AST2500 PWM and Fan tach driver"
+	help
+	  This driver provides support for ASPEED AST2400/AST2500 PWM
+	  and Fan Tacho controllers.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called aspeed_pwm_tacho.
+
 config SENSORS_ATXP1
 	tristate "Attansic ATXP1 VID controller"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index aecf4ba17460..83025cc9bb45 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
 obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
 obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
 obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
+obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
 obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
new file mode 100644
index 000000000000..93eb3be2e506
--- /dev/null
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -0,0 +1,884 @@
+/*
+ * Copyright (c) 2016 Google, 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/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>
+
+/* ASPEED PWM & FAN Tach Register Definition */
+#define ASPEED_PTCR_CTRL		0x00
+#define ASPEED_PTCR_CLK_CTRL		0x04
+#define ASPEED_PTCR_DUTY0_CTRL		0x08
+#define ASPEED_PTCR_DUTY1_CTRL		0x0c
+#define ASPEED_PTCR_TYPEM_CTRL		0x10
+#define ASPEED_PTCR_TYPEM_CTRL1		0x14
+#define ASPEED_PTCR_TYPEN_CTRL		0x18
+#define ASPEED_PTCR_TYPEN_CTRL1		0x1c
+#define ASPEED_PTCR_TACH_SOURCE		0x20
+#define ASPEED_PTCR_TRIGGER		0x28
+#define ASPEED_PTCR_RESULT		0x2c
+#define ASPEED_PTCR_INTR_CTRL		0x30
+#define ASPEED_PTCR_INTR_STS		0x34
+#define ASPEED_PTCR_TYPEM_LIMIT		0x38
+#define ASPEED_PTCR_TYPEN_LIMIT		0x3C
+#define ASPEED_PTCR_CTRL_EXT		0x40
+#define ASPEED_PTCR_CLK_CTRL_EXT	0x44
+#define ASPEED_PTCR_DUTY2_CTRL		0x48
+#define ASPEED_PTCR_DUTY3_CTRL		0x4c
+#define ASPEED_PTCR_TYPEO_CTRL		0x50
+#define ASPEED_PTCR_TYPEO_CTRL1		0x54
+#define ASPEED_PTCR_TACH_SOURCE_EXT	0x60
+#define ASPEED_PTCR_TYPEO_LIMIT		0x78
+
+/* ASPEED_PTCR_CTRL : 0x00 - General Control Register */
+#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART1	15
+#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART2	6
+#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_MASK	(BIT(7) | BIT(15))
+
+#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART1	14
+#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART2	5
+#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_MASK	(BIT(6) | BIT(14))
+
+#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART1	13
+#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART2	4
+#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_MASK	(BIT(5) | BIT(13))
+
+#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1	12
+#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2	3
+#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK	(BIT(4) | BIT(12))
+
+#define	ASPEED_PTCR_CTRL_FAN_NUM_EN(x)	(0x1 << (16 + (x)))
+
+#define	ASPEED_PTCR_CTRL_PWMD_EN	(0x1 << 11)
+#define	ASPEED_PTCR_CTRL_PWMC_EN	(0x1 << 10)
+#define	ASPEED_PTCR_CTRL_PWMB_EN	(0x1 << 9)
+#define	ASPEED_PTCR_CTRL_PWMA_EN	(0x1 << 8)
+
+#define	ASPEED_PTCR_CTRL_CLK_SRC	0x2
+#define	ASPEED_PTCR_CTRL_CLK_EN		0x1
+
+/* ASPEED_PTCR_CLK_CTRL : 0x04 - Clock Control Register */
+/* TYPE N */
+#define ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT		24
+#define ASPEED_PTCR_CLK_CTRL_TYPEN_H		20
+#define ASPEED_PTCR_CLK_CTRL_TYPEN_L		16
+/* TYPE M */
+#define ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT		8
+#define ASPEED_PTCR_CLK_CTRL_TYPEM_H		4
+#define ASPEED_PTCR_CLK_CTRL_TYPEM_L		0
+
+/*
+ * ASPEED_PTCR_DUTY_CTRL/1/2/3 : 0x08/0x0C/0x48/0x4C - PWM-FAN duty control
+ * 0/1/2/3 register
+ */
+#define DUTY_CTRL_PWM2_FALL_POINT	24
+#define DUTY_CTRL_PWM2_RISE_POINT	16
+#define DUTY_CTRL_PWM1_FALL_POINT	8
+#define DUTY_CTRL_PWM1_RISE_POINT	0
+
+/* ASPEED_PTCR_TYPEM_CTRL : 0x10/0x18/0x50 - Type M/N/O Ctrl 0 Register */
+#define TYPE_CTRL_FAN_PERIOD		16
+#define TYPE_CTRL_FAN_MODE		4
+#define TYPE_CTRL_FAN_DIVISION		1
+#define TYPE_CTRL_FAN_TYPE_EN		1
+
+/* ASPEED_PTCR_TACH_SOURCE : 0x20/0x60 - Tach Source Register */
+/* bit [0,1] at 0x20, bit [2] at 0x60 */
+#define TACH_PWM_SOURCE_BIT01(x)	((x) * 2)
+#define TACH_PWM_SOURCE_BIT2(x)		((x) * 2)
+#define TACH_PWM_SOURCE_MASK_BIT01(x)	(0x3 << ((x) * 2))
+#define TACH_PWM_SOURCE_MASK_BIT2(x)	(0x1 << ((x) * 2))
+
+/* ASPEED_PTCR_TRIGGER : 0x28 - Trigger Register */
+#define TRIGGER_READ_FAN_NUM(x)		(0x1 << (x))
+
+/* ASPEED_PTCR_RESULT : 0x2c - Result Register */
+#define RESULT_STATUS			31
+#define RESULT_VALUE_MASK		0xfffff
+
+/* ASPEED_PTCR_CTRL_EXT : 0x40 - General Control Extension #1 Register */
+#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART1	15
+#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART2	6
+#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_MASK	(BIT(7) | BIT(15))
+
+#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART1	14
+#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART2	5
+#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_MASK	(BIT(6) | BIT(14))
+
+#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART1	13
+#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART2	4
+#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_MASK	(BIT(5) | BIT(13))
+
+#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART1	12
+#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART2	3
+#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_MASK	(BIT(4) | BIT(12))
+
+#define	ASPEED_PTCR_CTRL_PWMH_EN	(0x1 << 11)
+#define	ASPEED_PTCR_CTRL_PWMG_EN	(0x1 << 10)
+#define	ASPEED_PTCR_CTRL_PWMF_EN	(0x1 << 9)
+#define	ASPEED_PTCR_CTRL_PWME_EN	(0x1 << 8)
+
+/* ASPEED_PTCR_CLK_EXT_CTRL : 0x44 - Clock Control Extension #1 Register */
+/* TYPE O */
+#define ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT		8
+#define ASPEED_PTCR_CLK_CTRL_TYPEO_H		4
+#define ASPEED_PTCR_CLK_CTRL_TYPEO_L		0
+
+#define MCLK 1
+#define PWM_MAX 255
+#define MAX_HIGH_LOW_BIT 15
+
+struct aspeed_pwm_tacho_data {
+	void __iomem *base;
+	unsigned long clk_freq;
+	const struct attribute_group *groups[24];
+	u8 type_pwm_clock_unit[3];
+	u8 type_pwm_clock_division_h[3];
+	u8 type_pwm_clock_division_l[3];
+	u8 type_fan_tach_clock_division[3];
+	u16 type_fan_tach_unit[3];
+	u8 pwm_port_type[8];
+	u8 pwm_port_fan_ctrl[8];
+	u8 fan_tach_ch_source[16];
+};
+
+enum type { TYPEM, TYPEN, TYPEO };
+
+struct type_params {
+	u32 l_value;
+	u32 h_value;
+	u32 unit_value;
+	u32 clk_ctrl_reg;
+	u32 ctrl_reg;
+	u32 ctrl_reg1;
+};
+
+static const struct type_params type_params[] = {
+	[TYPEM] = {
+		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEM_L,
+		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEM_H,
+		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT,
+		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL,
+		.ctrl_reg = ASPEED_PTCR_TYPEM_CTRL,
+		.ctrl_reg1 = ASPEED_PTCR_TYPEM_CTRL1,
+	},
+	[TYPEN] = {
+		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEN_L,
+		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEN_H,
+		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT,
+		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL,
+		.ctrl_reg = ASPEED_PTCR_TYPEN_CTRL,
+		.ctrl_reg1 = ASPEED_PTCR_TYPEN_CTRL1,
+	},
+	[TYPEO] = {
+		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEO_L,
+		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEO_H,
+		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT,
+		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL_EXT,
+		.ctrl_reg = ASPEED_PTCR_TYPEO_CTRL,
+		.ctrl_reg1 = ASPEED_PTCR_TYPEO_CTRL1,
+	}
+};
+
+enum pwm_port { PWMA, PWMB, PWMC, PWMD, PWME, PWMF, PWMG, PWMH };
+
+struct pwm_port_params {
+	u32 pwm_en;
+	u32 ctrl_reg;
+	u32 type_part1;
+	u32 type_part2;
+	u32 type_mask;
+	u32 duty_ctrl_rise_point;
+	u32 duty_ctrl_fall_point;
+	u32 duty_ctrl_reg;
+	u8 duty_ctrl_calc_type;
+};
+
+static const struct pwm_port_params pwm_port_params[] = {
+	[PWMA] = {
+		.pwm_en = ASPEED_PTCR_CTRL_PWMA_EN,
+		.ctrl_reg = ASPEED_PTCR_CTRL,
+		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1,
+		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2,
+		.type_mask = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK,
+		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
+		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
+		.duty_ctrl_reg = ASPEED_PTCR_DUTY0_CTRL,
+		.duty_ctrl_calc_type = 0,
+	},
+	[PWMB] = {
+		.pwm_en = ASPEED_PTCR_CTRL_PWMB_EN,
+		.ctrl_reg = ASPEED_PTCR_CTRL,
+		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART1,
+		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART2,
+		.type_mask = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_MASK,
+		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
+		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
+		.duty_ctrl_reg = ASPEED_PTCR_DUTY0_CTRL,
+		.duty_ctrl_calc_type = 1,
+	},
+	[PWMC] = {
+		.pwm_en = ASPEED_PTCR_CTRL_PWMC_EN,
+		.ctrl_reg = ASPEED_PTCR_CTRL,
+		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART1,
+		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART2,
+		.type_mask = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_MASK,
+		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
+		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
+		.duty_ctrl_reg = ASPEED_PTCR_DUTY1_CTRL,
+		.duty_ctrl_calc_type = 0,
+	},
+	[PWMD] = {
+		.pwm_en = ASPEED_PTCR_CTRL_PWMD_EN,
+		.ctrl_reg = ASPEED_PTCR_CTRL,
+		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART1,
+		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART2,
+		.type_mask = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_MASK,
+		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
+		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
+		.duty_ctrl_reg = ASPEED_PTCR_DUTY1_CTRL,
+		.duty_ctrl_calc_type = 1,
+	},
+	[PWME] = {
+		.pwm_en = ASPEED_PTCR_CTRL_PWME_EN,
+		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
+		.type_part1 = ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART1,
+		.type_part2 = ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART2,
+		.type_mask = ASPEED_PTCR_CTRL_SET_PWME_TYPE_MASK,
+		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
+		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
+		.duty_ctrl_reg = ASPEED_PTCR_DUTY2_CTRL,
+		.duty_ctrl_calc_type = 0,
+	},
+	[PWMF] = {
+		.pwm_en = ASPEED_PTCR_CTRL_PWMF_EN,
+		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
+		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART1,
+		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART2,
+		.type_mask = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_MASK,
+		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
+		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
+		.duty_ctrl_reg = ASPEED_PTCR_DUTY2_CTRL,
+		.duty_ctrl_calc_type = 1,
+	},
+	[PWMG] = {
+		.pwm_en = ASPEED_PTCR_CTRL_PWMG_EN,
+		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
+		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART1,
+		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART2,
+		.type_mask = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_MASK,
+		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
+		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
+		.duty_ctrl_reg = ASPEED_PTCR_DUTY3_CTRL,
+		.duty_ctrl_calc_type = 0,
+	},
+	[PWMH] = {
+		.pwm_en = ASPEED_PTCR_CTRL_PWMH_EN,
+		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
+		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART1,
+		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART2,
+		.type_mask = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_MASK,
+		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
+		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
+		.duty_ctrl_reg = ASPEED_PTCR_DUTY3_CTRL,
+		.duty_ctrl_calc_type = 1,
+	}
+};
+
+static void aspeed_set_clock_enable(void __iomem *base, bool val)
+{
+	u32 reg_value = ioread32(base + ASPEED_PTCR_CTRL);
+
+	if (val)
+		reg_value |= ASPEED_PTCR_CTRL_CLK_EN;
+	else
+		reg_value &= ~ASPEED_PTCR_CTRL_CLK_EN;
+
+	iowrite32(reg_value, base + ASPEED_PTCR_CTRL);
+}
+
+static void aspeed_set_clock_source(void __iomem *base, int val)
+{
+	u32 reg_value = ioread32(base + ASPEED_PTCR_CTRL);
+
+	if (val == MCLK)
+		reg_value |= ASPEED_PTCR_CTRL_CLK_SRC;
+	else
+		reg_value &= ~ASPEED_PTCR_CTRL_CLK_SRC;
+
+	iowrite32(reg_value, base + ASPEED_PTCR_CTRL);
+}
+
+static void aspeed_set_pwm_clock_values(void __iomem *base, u8 type,
+					u8 div_high, u8 div_low, u8 unit)
+{
+	u32 reg_offset = type_params[type].clk_ctrl_reg;
+	u32 reg_value = ioread32(base + reg_offset);
+
+	reg_value &= ~((0xF << type_params[type].h_value) |
+	(0xF << type_params[type].l_value) |
+	(0xFF << type_params[type].unit_value));
+	reg_value |= ((div_high << type_params[type].h_value) |
+	(div_low << type_params[type].l_value) |
+	(unit << type_params[type].unit_value));
+
+	iowrite32(reg_value, base + reg_offset);
+}
+
+static void aspeed_set_pwm_port_enable(void __iomem *base, u8 pwm_port,
+				       bool enable)
+{
+	u32 reg_offset = pwm_port_params[pwm_port].ctrl_reg;
+	u32 reg_value = ioread32(base + reg_offset);
+
+	if (enable)
+		reg_value |= pwm_port_params[pwm_port].pwm_en;
+	else
+		reg_value &= ~pwm_port_params[pwm_port].pwm_en;
+	iowrite32(reg_value, base + reg_offset);
+}
+
+static void aspeed_set_pwm_port_type(void __iomem *base, u8 pwm_port, u8 type)
+{
+	u32 reg_offset = pwm_port_params[pwm_port].ctrl_reg;
+	u32 reg_value = ioread32(base + reg_offset);
+
+	reg_value &= ~pwm_port_params[pwm_port].type_mask;
+	reg_value |= (type & 0x1) <<
+		pwm_port_params[pwm_port].type_part1;
+	reg_value |= (type & 0x2) <<
+		pwm_port_params[pwm_port].type_part2;
+
+	iowrite32(reg_value, base + reg_offset);
+}
+
+static void aspeed_set_pwm_port_duty_rising_falling(void __iomem *base,
+						    u8 pwm_port, u8 rising,
+						    u8 falling)
+{
+	u32 reg_offset = pwm_port_params[pwm_port].duty_ctrl_reg;
+	u32 reg_value = ioread32(base + reg_offset);
+
+	reg_value &= ~(0xFF << pwm_port_params[pwm_port].duty_ctrl_rise_point);
+	reg_value &= ~(0xFF << pwm_port_params[pwm_port].duty_ctrl_fall_point);
+
+	if (pwm_port_params[pwm_port].duty_ctrl_calc_type == 0) {
+		reg_value |= rising;
+	} else if (pwm_port_params[pwm_port].duty_ctrl_calc_type == 1) {
+		reg_value |= (rising <<
+			pwm_port_params[pwm_port].duty_ctrl_rise_point);
+	}
+	reg_value |= (falling <<
+			pwm_port_params[pwm_port].duty_ctrl_fall_point);
+
+	iowrite32(reg_value, base + reg_offset);
+}
+
+static void aspeed_set_tacho_type_enable(void __iomem *base, u8 type,
+					 bool enable)
+{
+	u32 reg_offset = type_params[type].ctrl_reg;
+	u32 reg_value = ioread32(base + reg_offset);
+
+	if (enable)
+		reg_value |= TYPE_CTRL_FAN_TYPE_EN;
+	else
+		reg_value &= ~TYPE_CTRL_FAN_TYPE_EN;
+
+	iowrite32(reg_value, base + reg_offset);
+}
+
+static void aspeed_set_tacho_type_values(void __iomem *base, u8 type, u8 mode,
+					 u16 unit, u8 division)
+{
+	u32 reg_offset = type_params[type].ctrl_reg;
+	u32 reg_offset1 = type_params[type].ctrl_reg1;
+	u32 reg_value = ioread32(base + reg_offset);
+
+	reg_value &= ~((0x3 << TYPE_CTRL_FAN_MODE) |
+			(0xFFFF << TYPE_CTRL_FAN_PERIOD) |
+			(0x7 << TYPE_CTRL_FAN_DIVISION));
+	reg_value |= ((mode << TYPE_CTRL_FAN_MODE) |
+			(unit << TYPE_CTRL_FAN_PERIOD) |
+			(division << TYPE_CTRL_FAN_DIVISION));
+
+	iowrite32(reg_value, base + reg_offset);
+
+	iowrite32(unit << 16, base + reg_offset1);
+}
+
+static void aspeed_set_fan_tach_ch_enable(void __iomem *base, u8 fan_tach_ch,
+					  bool enable)
+{
+	u32 reg_value = ioread32(base + ASPEED_PTCR_CTRL);
+
+	if (enable)
+		reg_value |= ASPEED_PTCR_CTRL_FAN_NUM_EN(fan_tach_ch);
+	else
+		reg_value &= ~ASPEED_PTCR_CTRL_FAN_NUM_EN(fan_tach_ch);
+
+	iowrite32(reg_value, base + ASPEED_PTCR_CTRL);
+}
+
+static void aspeed_set_fan_tach_ch_source(void __iomem *base, u8 fan_tach_ch,
+					  u8 fan_tach_ch_source)
+{
+	u32 reg_value1 = ioread32(base + ASPEED_PTCR_TACH_SOURCE);
+	u32 reg_value2 = ioread32(base + ASPEED_PTCR_TACH_SOURCE_EXT);
+
+	reg_value1 &= ~(TACH_PWM_SOURCE_MASK_BIT01(fan_tach_ch));
+	reg_value1 |= ((fan_tach_ch_source & 0x3) <<
+			(TACH_PWM_SOURCE_BIT01(fan_tach_ch)));
+
+	reg_value2 &= ~(TACH_PWM_SOURCE_MASK_BIT2(fan_tach_ch));
+	reg_value2 |= (((fan_tach_ch_source & 0x4) >> 2) <<
+			(TACH_PWM_SOURCE_BIT2(fan_tach_ch)));
+
+	iowrite32(reg_value1, base + ASPEED_PTCR_TACH_SOURCE);
+	iowrite32(reg_value2, base + ASPEED_PTCR_TACH_SOURCE_EXT);
+}
+
+static void aspeed_set_pwm_port_fan_ctrl(struct aspeed_pwm_tacho_data *priv,
+					 u8 index, u8 fan_ctrl)
+{
+	u16 period;
+	u16 dc_time_on;
+
+	period = priv->type_pwm_clock_unit[priv->pwm_port_type[index]];
+	period += 1;
+	dc_time_on = (fan_ctrl * period) / PWM_MAX;
+
+	if (dc_time_on == 0) {
+		aspeed_set_pwm_port_enable(priv->base, index, false);
+	} else {
+		if (dc_time_on == period)
+			dc_time_on = 0;
+
+		aspeed_set_pwm_port_duty_rising_falling(priv->base, index, 0,
+							dc_time_on);
+		aspeed_set_pwm_port_enable(priv->base, index, true);
+	}
+}
+
+static u32 aspeed_get_fan_tach_ch_measure_period(struct aspeed_pwm_tacho_data
+						 *priv, u8 type)
+{
+	u32 clk;
+	u16 tacho_unit;
+	u8 clk_unit, div_h, div_l, tacho_div;
+
+	clk = priv->clk_freq;
+
+	clk_unit = priv->type_pwm_clock_unit[type];
+	div_h = priv->type_pwm_clock_division_h[type];
+	div_h = 0x1 << div_h;
+	div_l = priv->type_pwm_clock_division_l[type];
+	if (div_l == 0)
+		div_l = 1;
+	else
+		div_l = div_l * 2;
+
+	tacho_unit = priv->type_fan_tach_unit[type];
+	tacho_div = priv->type_fan_tach_clock_division[type];
+
+	tacho_div = 0x4 << (tacho_div * 2);
+	return clk / (clk_unit * div_h * div_l * tacho_div * tacho_unit);
+}
+
+static u32 aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv,
+				      u8 fan_tach_ch)
+{
+	u32 raw_data, rpm, tach_div, clk_source, timeout = 0, sec;
+	u8 fan_tach_ch_source, type;
+	void __iomem *base = priv->base;
+
+	iowrite32(0, priv->base + ASPEED_PTCR_TRIGGER);
+	iowrite32(0x1 << fan_tach_ch, priv->base + ASPEED_PTCR_TRIGGER);
+
+	fan_tach_ch_source = priv->fan_tach_ch_source[fan_tach_ch];
+	type = priv->pwm_port_type[fan_tach_ch_source];
+
+	sec = (1000 / aspeed_get_fan_tach_ch_measure_period(priv, type));
+
+	msleep(sec);
+
+	while (!(ioread32(priv->base + ASPEED_PTCR_RESULT) &
+				(0x1 << RESULT_STATUS))) {
+		timeout++;
+		if (timeout > 1)
+			return 0;
+		msleep(sec);
+	};
+
+	raw_data = (ioread32(base + ASPEED_PTCR_RESULT)) &
+				RESULT_VALUE_MASK;
+	tach_div = priv->type_fan_tach_clock_division[type];
+
+	tach_div = 0x4 << (tach_div * 2);
+	clk_source = priv->clk_freq;
+	rpm = (clk_source * 60) / (2 * raw_data * tach_div);
+	return rpm;
+}
+
+static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+	int index = sensor_attr->index;
+	int ret;
+
+	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
+	long fan_ctrl;
+
+	ret = kstrtol(buf, 10, &fan_ctrl);
+	if (ret != 0)
+		return ret;
+
+	if (fan_ctrl < 0 || fan_ctrl > PWM_MAX)
+		return -EINVAL;
+
+	if (priv->pwm_port_fan_ctrl[index] == fan_ctrl)
+		return count;
+
+	priv->pwm_port_fan_ctrl[index] = fan_ctrl;
+	aspeed_set_pwm_port_fan_ctrl(priv, index, fan_ctrl);
+
+	return count;
+}
+
+static ssize_t show_pwm(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+	int index = sensor_attr->index;
+
+	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", priv->pwm_port_fan_ctrl[index]);
+}
+
+static ssize_t show_rpm(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+	int index = sensor_attr->index;
+	u32 rpm;
+
+	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
+
+	rpm = aspeed_get_fan_tach_ch_rpm(priv, index);
+
+	return sprintf(buf, "%u\n", rpm);
+}
+
+#define pwm_index(index)					\
+static SENSOR_DEVICE_ATTR(pwm##index, 0644,	\
+			show_pwm, set_pwm, index - 1);		\
+								\
+static struct attribute *pwm##index##_dev_attrs[] = {		\
+	&sensor_dev_attr_pwm##index.dev_attr.attr,		\
+	NULL,							\
+};								\
+static const struct attribute_group pwm##index##_dev_groups = {	\
+	.attrs = pwm##index##_dev_attrs,			\
+}
+
+#define fan_index(index)					\
+static SENSOR_DEVICE_ATTR(fan##index##_input, 0444,		\
+		show_rpm, NULL, index - 1);			\
+								\
+static struct attribute *fan##index##_dev_attrs[] = {		\
+	&sensor_dev_attr_fan##index##_input.dev_attr.attr,	\
+	NULL,							\
+};								\
+static const struct attribute_group fan##index##_dev_groups = {	\
+	.attrs = fan##index##_dev_attrs,			\
+}
+
+pwm_index(1);
+pwm_index(2);
+pwm_index(3);
+pwm_index(4);
+pwm_index(5);
+pwm_index(6);
+pwm_index(7);
+pwm_index(8);
+
+fan_index(1);
+fan_index(2);
+fan_index(3);
+fan_index(4);
+fan_index(5);
+fan_index(6);
+fan_index(7);
+fan_index(8);
+fan_index(9);
+fan_index(10);
+fan_index(11);
+fan_index(12);
+fan_index(13);
+fan_index(14);
+fan_index(15);
+fan_index(16);
+
+/*
+ * If the clock type is type M then :
+ * The PWM frequency = 24MHz / (type M clock division L bit *
+ * type M clock division H bit * (type M PWM period bit + 1))
+ * Calculate type M clock division L bit and H bits given the other values
+ */
+static int aspeed_create_type(struct device_node *child,
+			      struct aspeed_pwm_tacho_data *priv,
+			      u8 index)
+{
+	u8 period, div_l, div_h;
+	bool enable;
+	u8 mode, div;
+	u16 unit;
+
+	of_property_read_u8(child, "pwm_period", &period);
+	of_property_read_u8(child, "pwm_clock_division_h", &div_h);
+	of_property_read_u8(child, "pwm_clock_division_l", &div_l);
+	priv->type_pwm_clock_division_h[index] = div_h;
+	priv->type_pwm_clock_division_l[index] = div_l;
+	priv->type_pwm_clock_unit[index] = period;
+	aspeed_set_pwm_clock_values(priv->base, index, div_h, div_l, period);
+
+	enable = of_property_read_bool(child, "fan_tach_enable");
+	aspeed_set_tacho_type_enable(priv->base, index, enable);
+
+	of_property_read_u8(child, "fan_tach_clock_division", &div);
+	priv->type_fan_tach_clock_division[index] = div;
+
+	of_property_read_u8(child, "fan_tach_mode_selection", &mode);
+
+	of_property_read_u16(child, "fan_tach_period", &unit);
+	priv->type_fan_tach_unit[index] = unit;
+	aspeed_set_tacho_type_values(priv->base, index, mode, unit, div);
+
+	return 0;
+}
+
+static int aspeed_create_pwm_port(struct device_node *child,
+				  struct aspeed_pwm_tacho_data *priv, u8 index,
+				  u8 group_index)
+{
+	u8 val;
+	bool enable;
+
+	switch (index) {
+	case 1:
+		priv->groups[group_index] = &pwm1_dev_groups;
+		break;
+	case 2:
+		priv->groups[group_index] = &pwm2_dev_groups;
+		break;
+	case 3:
+		priv->groups[group_index] = &pwm3_dev_groups;
+		break;
+	case 4:
+		priv->groups[group_index] = &pwm4_dev_groups;
+		break;
+	case 5:
+		priv->groups[group_index] = &pwm5_dev_groups;
+		break;
+	case 6:
+		priv->groups[group_index] = &pwm6_dev_groups;
+		break;
+	case 7:
+		priv->groups[group_index] = &pwm7_dev_groups;
+		break;
+	case 8:
+		priv->groups[group_index] = &pwm8_dev_groups;
+		break;
+	}
+
+	enable = of_property_read_bool(child, "enable");
+	aspeed_set_pwm_port_enable(priv->base, index - 1, enable);
+
+	of_property_read_u8(child, "type", &val);
+	priv->pwm_port_type[index - 1] = val;
+	aspeed_set_pwm_port_type(priv->base, index - 1, val);
+
+	of_property_read_u8(child, "fan_ctrl", &val);
+	priv->pwm_port_fan_ctrl[index - 1] = val;
+	aspeed_set_pwm_port_fan_ctrl(priv, index - 1, val);
+
+	return 0;
+}
+
+static int aspeed_create_fan_tach_channel(struct device *dev,
+					  struct device_node *child,
+					  struct aspeed_pwm_tacho_data *priv,
+					  u8 index, u8 group_index)
+{
+	u8 val;
+	bool enable;
+	struct gpio_desc *fan_ctrl = devm_gpiod_get(dev, "fan-ctrl", GPIOD_IN);
+
+	if (IS_ERR(fan_ctrl))
+		return PTR_ERR(fan_ctrl);
+
+	switch (index) {
+	case 1:
+		priv->groups[group_index] = &fan1_dev_groups;
+		break;
+	case 2:
+		priv->groups[group_index] = &fan2_dev_groups;
+		break;
+	case 3:
+		priv->groups[group_index] = &fan3_dev_groups;
+		break;
+	case 4:
+		priv->groups[group_index] = &fan4_dev_groups;
+		break;
+	case 5:
+		priv->groups[group_index] = &fan5_dev_groups;
+		break;
+	case 6:
+		priv->groups[group_index] = &fan6_dev_groups;
+		break;
+	case 7:
+		priv->groups[group_index] = &fan7_dev_groups;
+		break;
+	case 8:
+		priv->groups[group_index] = &fan8_dev_groups;
+		break;
+	case 9:
+		priv->groups[group_index] = &fan9_dev_groups;
+		break;
+	case 10:
+		priv->groups[group_index] = &fan10_dev_groups;
+		break;
+	case 11:
+		priv->groups[group_index] = &fan11_dev_groups;
+		break;
+	case 12:
+		priv->groups[group_index] = &fan12_dev_groups;
+		break;
+	case 13:
+		priv->groups[group_index] = &fan13_dev_groups;
+		break;
+	case 14:
+		priv->groups[group_index] = &fan14_dev_groups;
+		break;
+	case 15:
+		priv->groups[group_index] = &fan15_dev_groups;
+		break;
+	case 16:
+		priv->groups[group_index] = &fan16_dev_groups;
+		break;
+	}
+
+	enable = of_property_read_bool(child, "enable");
+	aspeed_set_fan_tach_ch_enable(priv->base, index - 1, enable);
+
+	of_property_read_u8(child, "pwm_source", &val);
+	priv->fan_tach_ch_source[index - 1] = val;
+	aspeed_set_fan_tach_ch_source(priv->base, index - 1, val);
+
+	return 0;
+}
+
+static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
+{
+	struct device_node *np, *type_np, *pwm_np, *fan_tach_np, *child;
+	u8 pwm_index = 1, fan_index = 1, type_index = 0, group_index = 0;
+	struct aspeed_pwm_tacho_data *priv;
+	struct resource *res;
+	struct device *hwmon;
+	void __iomem *base;
+	struct clk *clk;
+
+	np = pdev->dev.of_node;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENOENT;
+
+	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!base)
+		return -ENOMEM;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->base = base;
+
+	iowrite32(0, base + ASPEED_PTCR_TACH_SOURCE);
+	iowrite32(0, base + ASPEED_PTCR_TACH_SOURCE_EXT);
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return -ENODEV;
+
+	priv->clk_freq = clk_get_rate(clk);
+	aspeed_set_clock_enable(base, true);
+	aspeed_set_clock_source(base, 0);
+
+	type_np = of_get_child_by_name(np, "type_values");
+	for_each_child_of_node(type_np, child) {
+		aspeed_create_type(child, priv, type_index++);
+		of_node_put(child);
+	}
+	of_node_put(type_np);
+	pwm_np = of_get_child_by_name(np, "pwm_port");
+	for_each_child_of_node(pwm_np, child) {
+		aspeed_create_pwm_port(child, priv, pwm_index++,
+				       group_index++);
+		of_node_put(child);
+	}
+	of_node_put(pwm_np);
+	fan_tach_np = of_get_child_by_name(np, "fan_tach_channel");
+	for_each_child_of_node(fan_tach_np, child) {
+		aspeed_create_fan_tach_channel(&pdev->dev, child, priv,
+					       fan_index++, group_index++);
+		of_node_put(child);
+	}
+	of_node_put(fan_tach_np);
+	of_node_put(np);
+
+	hwmon = devm_hwmon_device_register_with_groups(&pdev->dev,
+						       "aspeed_pwm_tacho",
+						       priv, priv->groups);
+	if (IS_ERR(hwmon))
+		return PTR_ERR(hwmon);
+
+	return 0;
+}
+
+static const struct of_device_id of_pwm_tacho_match_table[] = {
+	{ .compatible = "aspeed,aspeed2400-pwm-tacho", },
+	{ .compatible = "aspeed,aspeed2500-pwm-tacho", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_pwm_tacho_match_table);
+
+static struct platform_driver aspeed_pwm_tacho_driver = {
+	.probe		= aspeed_pwm_tacho_probe,
+	.driver		= {
+		.name	= "aspeed_pwm_tacho",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_pwm_tacho_match_table,
+	},
+};
+
+module_platform_driver(aspeed_pwm_tacho_driver);
+
+MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>");
+MODULE_DESCRIPTION("ASPEED PWM and Fan Tacho device driver");
+MODULE_LICENSE("GPL");
--
2.11.0.390.gc69c2f50cf-goog

Re: [PATCH linux v1 1/2] Documentation: dt-bindings: Document bindings for ASPEED AST2400/AST2500 pwm and fan tach controller device driver

[Rob Herring]

On Mon, Jan 09, 2017 at 01:59:34PM -0800, Jaghathiswari Rankappagounder Natarajan wrote:
> This binding provides interface for adding values related to ASPEED
> AST2400/2500 PWM and Fan tach controller support.
> The PWM controller can support upto 8 PWM output ports.
> The Fan tach controller can support upto 16 tachometer inputs.
> PWM clock types M, N and 0 are three types just to have three independent
> PWM sources.
> 
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> ---
>  .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt | 153 +++++++++++++++++++++

Perhaps bindings/pwm/... even though this is more than just PWM.

>  1 file changed, 153 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> new file mode 100644
> index 000000000000..8f346409ee8c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> @@ -0,0 +1,153 @@
> +ASPEED AST2400/AST2500 PWM and Fan Tacho controller device driver
> +
> +The ASPEED PWM controller can support upto 8 PWM outputs. The ASPEED Fan Tacho
> +controller can support upto 16 tachometer inputs. The PWM controller supports
> +3 types of frequency mode PWM for fan speed control. PWM clock types M, N and 0
> +are 3 types of frequency mode PWM just to have 3 independent PWM sources.
> +
> +Required properties for pwm_tacho node:
> +- #address-cells : should be 1.
> +
> +- #size-cells : should be 1.
> +
> +- reg : address and length of the register set for the device.
> +
> +- pinctrl-names : a pinctrl state named "default" must be defined.
> +
> +- pinctrl-0 : phandle referencing pin configuration of the AST2400/AST2500 PWM
> +	      ports.
> +
> +- compatible : should be "aspeed,aspeed2400-pwm-tacho" for AST2400 or
> +	       "aspeed,aspeed2500-pwm-tacho" for AST2500.
> +
> +- clocks : a fixed clock providing input clock frequency(PWM
> +	   and Fan Tach clock)
> +
> +type_values subnode format:

Don't use '_' in node or property names.

> +===========================
> +Under type_values subnode there can be upto 3 child nodes indicating type M/N/O
> +values. Atleast one child node is required.
> +
> +Required properties for the child node(type M/N/O):
> +- pwm_period : indicates type M/N/O PWM period, as per the AST2400/AST2500
> +	       datasheet. integer value in the range 0 to 255.


> +
> +- pwm_clock_division_l : indicates type M/N/O PWM clock division L value,
> +			 as per the AST2400/AST2500 datasheet.
> +			 integer value in the range 0 to 15.
> +			 0 here indicates divide 1, 1 indicates divide 2,
> +			 2 indicates divide 4, 3 indicates divide 6, and so on
> +			 till 15 indicates divide 30.
> +
> +- pwm_clock_division_h : indicates type M/N/O PWM clock division H value,
> +			 as per the AST2400/AST2500 datasheet.
> +			 integer value in the range 0 to 15.
> +			 0 here indicates divide 1, 1 indicates divide 2,
> +			 2 indicates divide 4, 3 indicates divide 8, and so on
> +			 till 15 indicates divide 32768.

Can't you have a single divider value and driver convert to register 
values? Really, you should specify the PWM period in ns/us and calculate 
the divider based on the input clock freq. (i.e. use the clock binding). 
There's already PWM binding to specify the period.

I think you should have a node for the fan using the PWM binding and 
perhaps moving some of these properties to the fan node.

> +
> +- fan_tach_enable : indicates fan tach enable of type M/N/O as per the
> +		    AST2400/AST2500 datasheet. boolean value.
> +
> +- fan_tach_clock_division : indicates fan tach clock division as per the
> +			    AST2400/AST2500 datasheet.
> +			    integer value in the range 0 to 7.
> +			    0 indicates divide 4, 1 indicates divide 16,
> +			    2 indicates divide 64, 3 indicates divide 256
> +			    and so on till 7 indicates divide 65536.
> +
> +- fan_tach_mode_selection : indicates fan tach mode mode selection as per the
> +			    AST2400/AST2500 datasheet. integer value in the
> +			    range 0 to 2. 0 indicates falling edge, 1 indicates
> +			    rising edge and 2 indicates both edges.
> +
> +- fan_tach_period : indicates fan tach period as per the AST2400/AST2500
> +		    datasheet. integer value (can be upto 16 bits long).
> +
> +pwm_port subnode format:
> +========================
> +Under pwm_port subnode there can upto 8 child nodes each indicating values
> +for one of the 8 PWM output ports.
> +
> +Required properties for each child node(starting from PWM A through PWM H):
> +- enable : enable PWM #X port, X ranges from A through H. boolean value.

A connection in the PWM binding would imply this.

> +
> +- type : indicates type selection value of PWM #X port, X ranges from A
> +	 through H. integer value in the range 0 to 2;
> +	 0 indicates type M, 1 indicates type N, 2 indicates type O.

This is meaningless to me.

> +- fan_ctrl : set the PWM fan control initial value. integer value between
> +	     0(off) and 255(full speed).
> +
> +fan_tach_channel subnode format:
> +================================
> +Under fan_tach_channel subnode there can be upto 16 child nodes each indicating
> +values for one of the 16 fan tach channels.
> +
> +Required properties for each child node(starting from fan tach #0 through
> +fan tach #16):
> +- fan-ctrl-gpios : should specify the tachometer input pin on the hardware.

I don't understand the connection here. You have a tach block with 16 
inputs on the SoC, so why the GPIOs? If the GPIOs go to the fan, then a 
fan node should have the GPIO properties.

> +
> +- enable : enable fan tach #X, X ranges from 0 through 16. boolean value.

'status' property is the standard way to enable a node or not. Not sure 
if that makes sense here though.

> +
> +- pwm_source : indicates PWM source of fan tach #X, X ranges from 0 through 16.
> +	       integer value in the range 0 to 7. 0 indicates PWM port A,
> +	       1 indicates PWM port B and so on till 7 indicates PWM port H.

Re: [PATCH linux v1 2/2] drivers: hwmon: hwmon driver for ASPEED AST2400/2500 PWM and Fan tach controller

[Guenter Roeck]

On 01/09/2017 01:59 PM, Jaghathiswari Rankappagounder Natarajan wrote:
> The ASPEED AST2400/2500 PWM controller supports 8 PWM output ports.
> The ASPEED AST2400/2500 Fan tach controller supports 16 tachometer inputs.
> PWM clock types M, N and 0 are three types just to have three independent PWM
> sources.
> The device driver matches on the device tree node. The configuration values
> are read from the device tree and written to the respective registers.
> The driver provides a sysfs entries through which the user can
> configure the duty-cycle value (ranging from 0 to 100 percent) and read the
> fan tach rpm value.
>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> ---
>  Documentation/hwmon/aspeed-pwm-tacho |  22 +
>  drivers/hwmon/Kconfig                |   9 +
>  drivers/hwmon/Makefile               |   1 +
>  drivers/hwmon/aspeed-pwm-tacho.c     | 884 +++++++++++++++++++++++++++++++++++
>  4 files changed, 916 insertions(+)
>  create mode 100644 Documentation/hwmon/aspeed-pwm-tacho
>  create mode 100644 drivers/hwmon/aspeed-pwm-tacho.c
>
> diff --git a/Documentation/hwmon/aspeed-pwm-tacho b/Documentation/hwmon/aspeed-pwm-tacho
> new file mode 100644
> index 000000000000..0e9ec6d5f900
> --- /dev/null
> +++ b/Documentation/hwmon/aspeed-pwm-tacho
> @@ -0,0 +1,22 @@
> +Kernel driver aspeed-pwm-tacho
> +==============================
> +
> +Supported chips:
> +	ASPEED AST2400/2500
> +
> +Authors:
> +	<jaghu@google.com>
> +
> +Description:
> +------------
> +This driver implements support for ASPEED AST2400/2500 PWM and Fan Tacho
> +controller. The PWM controller supports upto 8 PWM outputs. The Fan tacho
> +controller supports upto 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/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 45cef3d2c75c..757b5b0705bf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -341,6 +341,15 @@ config SENSORS_ASB100
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called asb100.
>
> +config SENSORS_ASPEED
> +	tristate "ASPEED AST2400/AST2500 PWM and Fan tach driver"
> +	help
> +	  This driver provides support for ASPEED AST2400/AST2500 PWM
> +	  and Fan Tacho controllers.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called aspeed_pwm_tacho.
> +
>  config SENSORS_ATXP1
>  	tristate "Attansic ATXP1 VID controller"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index aecf4ba17460..83025cc9bb45 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
>  obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
>  obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
> +obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
>  obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
>  obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
>  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> new file mode 100644
> index 000000000000..93eb3be2e506
> --- /dev/null
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -0,0 +1,884 @@
> +/*
> + * Copyright (c) 2016 Google, 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/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>
> +
> +/* ASPEED PWM & FAN Tach Register Definition */
> +#define ASPEED_PTCR_CTRL		0x00
> +#define ASPEED_PTCR_CLK_CTRL		0x04
> +#define ASPEED_PTCR_DUTY0_CTRL		0x08
> +#define ASPEED_PTCR_DUTY1_CTRL		0x0c
> +#define ASPEED_PTCR_TYPEM_CTRL		0x10
> +#define ASPEED_PTCR_TYPEM_CTRL1		0x14
> +#define ASPEED_PTCR_TYPEN_CTRL		0x18
> +#define ASPEED_PTCR_TYPEN_CTRL1		0x1c
> +#define ASPEED_PTCR_TACH_SOURCE		0x20
> +#define ASPEED_PTCR_TRIGGER		0x28
> +#define ASPEED_PTCR_RESULT		0x2c
> +#define ASPEED_PTCR_INTR_CTRL		0x30
> +#define ASPEED_PTCR_INTR_STS		0x34
> +#define ASPEED_PTCR_TYPEM_LIMIT		0x38
> +#define ASPEED_PTCR_TYPEN_LIMIT		0x3C
> +#define ASPEED_PTCR_CTRL_EXT		0x40
> +#define ASPEED_PTCR_CLK_CTRL_EXT	0x44
> +#define ASPEED_PTCR_DUTY2_CTRL		0x48
> +#define ASPEED_PTCR_DUTY3_CTRL		0x4c
> +#define ASPEED_PTCR_TYPEO_CTRL		0x50
> +#define ASPEED_PTCR_TYPEO_CTRL1		0x54
> +#define ASPEED_PTCR_TACH_SOURCE_EXT	0x60
> +#define ASPEED_PTCR_TYPEO_LIMIT		0x78
> +
> +/* ASPEED_PTCR_CTRL : 0x00 - General Control Register */
> +#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART1	15
> +#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART2	6
> +#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_MASK	(BIT(7) | BIT(15))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART1	14
> +#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART2	5
> +#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_MASK	(BIT(6) | BIT(14))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART1	13
> +#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART2	4
> +#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_MASK	(BIT(5) | BIT(13))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1	12
> +#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2	3
> +#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK	(BIT(4) | BIT(12))
> +
> +#define	ASPEED_PTCR_CTRL_FAN_NUM_EN(x)	(0x1 << (16 + (x)))
> +
> +#define	ASPEED_PTCR_CTRL_PWMD_EN	(0x1 << 11)
> +#define	ASPEED_PTCR_CTRL_PWMC_EN	(0x1 << 10)
> +#define	ASPEED_PTCR_CTRL_PWMB_EN	(0x1 << 9)
> +#define	ASPEED_PTCR_CTRL_PWMA_EN	(0x1 << 8)
> +
Why not BIT() ?

> +#define	ASPEED_PTCR_CTRL_CLK_SRC	0x2
> +#define	ASPEED_PTCR_CTRL_CLK_EN		0x1

BIT(0) and BIT(1) ?

> +
> +/* ASPEED_PTCR_CLK_CTRL : 0x04 - Clock Control Register */
> +/* TYPE N */
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT		24
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_H		20
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_L		16
> +/* TYPE M */
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT		8
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_H		4
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_L		0
> +
> +/*
> + * ASPEED_PTCR_DUTY_CTRL/1/2/3 : 0x08/0x0C/0x48/0x4C - PWM-FAN duty control
> + * 0/1/2/3 register
> + */
> +#define DUTY_CTRL_PWM2_FALL_POINT	24
> +#define DUTY_CTRL_PWM2_RISE_POINT	16
> +#define DUTY_CTRL_PWM1_FALL_POINT	8
> +#define DUTY_CTRL_PWM1_RISE_POINT	0
> +
> +/* ASPEED_PTCR_TYPEM_CTRL : 0x10/0x18/0x50 - Type M/N/O Ctrl 0 Register */
> +#define TYPE_CTRL_FAN_PERIOD		16
> +#define TYPE_CTRL_FAN_MODE		4
> +#define TYPE_CTRL_FAN_DIVISION		1
> +#define TYPE_CTRL_FAN_TYPE_EN		1
> +
> +/* ASPEED_PTCR_TACH_SOURCE : 0x20/0x60 - Tach Source Register */
> +/* bit [0,1] at 0x20, bit [2] at 0x60 */
> +#define TACH_PWM_SOURCE_BIT01(x)	((x) * 2)
> +#define TACH_PWM_SOURCE_BIT2(x)		((x) * 2)
> +#define TACH_PWM_SOURCE_MASK_BIT01(x)	(0x3 << ((x) * 2))
> +#define TACH_PWM_SOURCE_MASK_BIT2(x)	(0x1 << ((x) * 2))
> +
> +/* ASPEED_PTCR_TRIGGER : 0x28 - Trigger Register */
> +#define TRIGGER_READ_FAN_NUM(x)		(0x1 << (x))

Why not BIT() ?

> +
> +/* ASPEED_PTCR_RESULT : 0x2c - Result Register */
> +#define RESULT_STATUS			31
> +#define RESULT_VALUE_MASK		0xfffff
> +
> +/* ASPEED_PTCR_CTRL_EXT : 0x40 - General Control Extension #1 Register */
> +#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART1	15
> +#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART2	6
> +#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_MASK	(BIT(7) | BIT(15))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART1	14
> +#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART2	5
> +#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_MASK	(BIT(6) | BIT(14))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART1	13
> +#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART2	4
> +#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_MASK	(BIT(5) | BIT(13))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART1	12
> +#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART2	3
> +#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_MASK	(BIT(4) | BIT(12))
> +
> +#define	ASPEED_PTCR_CTRL_PWMH_EN	(0x1 << 11)
> +#define	ASPEED_PTCR_CTRL_PWMG_EN	(0x1 << 10)
> +#define	ASPEED_PTCR_CTRL_PWMF_EN	(0x1 << 9)
> +#define	ASPEED_PTCR_CTRL_PWME_EN	(0x1 << 8)

Why not BIT() ?

> +
> +/* ASPEED_PTCR_CLK_EXT_CTRL : 0x44 - Clock Control Extension #1 Register */
> +/* TYPE O */
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT		8
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_H		4
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_L		0
> +
> +#define MCLK 1
> +#define PWM_MAX 255
> +#define MAX_HIGH_LOW_BIT 15
> +
> +struct aspeed_pwm_tacho_data {
> +	void __iomem *base;
> +	unsigned long clk_freq;
> +	const struct attribute_group *groups[24];
> +	u8 type_pwm_clock_unit[3];
> +	u8 type_pwm_clock_division_h[3];
> +	u8 type_pwm_clock_division_l[3];
> +	u8 type_fan_tach_clock_division[3];
> +	u16 type_fan_tach_unit[3];
> +	u8 pwm_port_type[8];
> +	u8 pwm_port_fan_ctrl[8];
> +	u8 fan_tach_ch_source[16];
> +};
> +
> +enum type { TYPEM, TYPEN, TYPEO };
> +
> +struct type_params {
> +	u32 l_value;
> +	u32 h_value;
> +	u32 unit_value;
> +	u32 clk_ctrl_reg;
> +	u32 ctrl_reg;
> +	u32 ctrl_reg1;
> +};
> +
> +static const struct type_params type_params[] = {
> +	[TYPEM] = {
> +		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEM_L,
> +		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEM_H,
> +		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT,
> +		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL,
> +		.ctrl_reg = ASPEED_PTCR_TYPEM_CTRL,
> +		.ctrl_reg1 = ASPEED_PTCR_TYPEM_CTRL1,
> +	},
> +	[TYPEN] = {
> +		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEN_L,
> +		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEN_H,
> +		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT,
> +		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL,
> +		.ctrl_reg = ASPEED_PTCR_TYPEN_CTRL,
> +		.ctrl_reg1 = ASPEED_PTCR_TYPEN_CTRL1,
> +	},
> +	[TYPEO] = {
> +		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEO_L,
> +		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEO_H,
> +		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT,
> +		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL_EXT,
> +		.ctrl_reg = ASPEED_PTCR_TYPEO_CTRL,
> +		.ctrl_reg1 = ASPEED_PTCR_TYPEO_CTRL1,
> +	}
> +};
> +
> +enum pwm_port { PWMA, PWMB, PWMC, PWMD, PWME, PWMF, PWMG, PWMH };
> +
> +struct pwm_port_params {
> +	u32 pwm_en;
> +	u32 ctrl_reg;
> +	u32 type_part1;
> +	u32 type_part2;
> +	u32 type_mask;
> +	u32 duty_ctrl_rise_point;
> +	u32 duty_ctrl_fall_point;
> +	u32 duty_ctrl_reg;
> +	u8 duty_ctrl_calc_type;
> +};
> +
> +static const struct pwm_port_params pwm_port_params[] = {
> +	[PWMA] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMA_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY0_CTRL,
> +		.duty_ctrl_calc_type = 0,
> +	},
> +	[PWMB] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMB_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY0_CTRL,
> +		.duty_ctrl_calc_type = 1,
> +	},
> +	[PWMC] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMC_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY1_CTRL,
> +		.duty_ctrl_calc_type = 0,
> +	},
> +	[PWMD] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMD_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY1_CTRL,
> +		.duty_ctrl_calc_type = 1,
> +	},
> +	[PWME] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWME_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWME_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY2_CTRL,
> +		.duty_ctrl_calc_type = 0,
> +	},
> +	[PWMF] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMF_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY2_CTRL,
> +		.duty_ctrl_calc_type = 1,
> +	},
> +	[PWMG] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMG_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY3_CTRL,
> +		.duty_ctrl_calc_type = 0,
> +	},
> +	[PWMH] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMH_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY3_CTRL,
> +		.duty_ctrl_calc_type = 1,
> +	}
> +};
> +
> +static void aspeed_set_clock_enable(void __iomem *base, bool val)
> +{
> +	u32 reg_value = ioread32(base + ASPEED_PTCR_CTRL);
> +
> +	if (val)
> +		reg_value |= ASPEED_PTCR_CTRL_CLK_EN;
> +	else
> +		reg_value &= ~ASPEED_PTCR_CTRL_CLK_EN;
> +
> +	iowrite32(reg_value, base + ASPEED_PTCR_CTRL);

Those functions make me wonder if it would make sense to use regmap.

> +}
> +
> +static void aspeed_set_clock_source(void __iomem *base, int val)
> +{
> +	u32 reg_value = ioread32(base + ASPEED_PTCR_CTRL);
> +
> +	if (val == MCLK)
> +		reg_value |= ASPEED_PTCR_CTRL_CLK_SRC;
> +	else
> +		reg_value &= ~ASPEED_PTCR_CTRL_CLK_SRC;
> +
> +	iowrite32(reg_value, base + ASPEED_PTCR_CTRL);
> +}
> +
> +static void aspeed_set_pwm_clock_values(void __iomem *base, u8 type,
> +					u8 div_high, u8 div_low, u8 unit)
> +{
> +	u32 reg_offset = type_params[type].clk_ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	reg_value &= ~((0xF << type_params[type].h_value) |
> +	(0xF << type_params[type].l_value) |
> +	(0xFF << type_params[type].unit_value));
> +	reg_value |= ((div_high << type_params[type].h_value) |
> +	(div_low << type_params[type].l_value) |
> +	(unit << type_params[type].unit_value));
> +
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_pwm_port_enable(void __iomem *base, u8 pwm_port,
> +				       bool enable)
> +{
> +	u32 reg_offset = pwm_port_params[pwm_port].ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	if (enable)
> +		reg_value |= pwm_port_params[pwm_port].pwm_en;
> +	else
> +		reg_value &= ~pwm_port_params[pwm_port].pwm_en;
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_pwm_port_type(void __iomem *base, u8 pwm_port, u8 type)
> +{
> +	u32 reg_offset = pwm_port_params[pwm_port].ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	reg_value &= ~pwm_port_params[pwm_port].type_mask;
> +	reg_value |= (type & 0x1) <<
> +		pwm_port_params[pwm_port].type_part1;
> +	reg_value |= (type & 0x2) <<
> +		pwm_port_params[pwm_port].type_part2;
> +
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_pwm_port_duty_rising_falling(void __iomem *base,
> +						    u8 pwm_port, u8 rising,
> +						    u8 falling)
> +{
> +	u32 reg_offset = pwm_port_params[pwm_port].duty_ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	reg_value &= ~(0xFF << pwm_port_params[pwm_port].duty_ctrl_rise_point);
> +	reg_value &= ~(0xFF << pwm_port_params[pwm_port].duty_ctrl_fall_point);
> +
> +	if (pwm_port_params[pwm_port].duty_ctrl_calc_type == 0) {
> +		reg_value |= rising;
> +	} else if (pwm_port_params[pwm_port].duty_ctrl_calc_type == 1) {
> +		reg_value |= (rising <<
> +			pwm_port_params[pwm_port].duty_ctrl_rise_point);
> +	}
> +	reg_value |= (falling <<
> +			pwm_port_params[pwm_port].duty_ctrl_fall_point);
> +
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_tacho_type_enable(void __iomem *base, u8 type,
> +					 bool enable)
> +{
> +	u32 reg_offset = type_params[type].ctrl_reg;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	if (enable)
> +		reg_value |= TYPE_CTRL_FAN_TYPE_EN;
> +	else
> +		reg_value &= ~TYPE_CTRL_FAN_TYPE_EN;
> +
> +	iowrite32(reg_value, base + reg_offset);
> +}
> +
> +static void aspeed_set_tacho_type_values(void __iomem *base, u8 type, u8 mode,
> +					 u16 unit, u8 division)
> +{
> +	u32 reg_offset = type_params[type].ctrl_reg;
> +	u32 reg_offset1 = type_params[type].ctrl_reg1;
> +	u32 reg_value = ioread32(base + reg_offset);
> +
> +	reg_value &= ~((0x3 << TYPE_CTRL_FAN_MODE) |
> +			(0xFFFF << TYPE_CTRL_FAN_PERIOD) |
> +			(0x7 << TYPE_CTRL_FAN_DIVISION));
> +	reg_value |= ((mode << TYPE_CTRL_FAN_MODE) |
> +			(unit << TYPE_CTRL_FAN_PERIOD) |
> +			(division << TYPE_CTRL_FAN_DIVISION));
> +
> +	iowrite32(reg_value, base + reg_offset);
> +
> +	iowrite32(unit << 16, base + reg_offset1);
> +}
> +
> +static void aspeed_set_fan_tach_ch_enable(void __iomem *base, u8 fan_tach_ch,
> +					  bool enable)
> +{
> +	u32 reg_value = ioread32(base + ASPEED_PTCR_CTRL);
> +
> +	if (enable)
> +		reg_value |= ASPEED_PTCR_CTRL_FAN_NUM_EN(fan_tach_ch);
> +	else
> +		reg_value &= ~ASPEED_PTCR_CTRL_FAN_NUM_EN(fan_tach_ch);
> +
> +	iowrite32(reg_value, base + ASPEED_PTCR_CTRL);
> +}
> +
> +static void aspeed_set_fan_tach_ch_source(void __iomem *base, u8 fan_tach_ch,
> +					  u8 fan_tach_ch_source)
> +{
> +	u32 reg_value1 = ioread32(base + ASPEED_PTCR_TACH_SOURCE);
> +	u32 reg_value2 = ioread32(base + ASPEED_PTCR_TACH_SOURCE_EXT);
> +
> +	reg_value1 &= ~(TACH_PWM_SOURCE_MASK_BIT01(fan_tach_ch));
> +	reg_value1 |= ((fan_tach_ch_source & 0x3) <<
> +			(TACH_PWM_SOURCE_BIT01(fan_tach_ch)));
> +
> +	reg_value2 &= ~(TACH_PWM_SOURCE_MASK_BIT2(fan_tach_ch));
> +	reg_value2 |= (((fan_tach_ch_source & 0x4) >> 2) <<
> +			(TACH_PWM_SOURCE_BIT2(fan_tach_ch)));
> +
> +	iowrite32(reg_value1, base + ASPEED_PTCR_TACH_SOURCE);
> +	iowrite32(reg_value2, base + ASPEED_PTCR_TACH_SOURCE_EXT);
> +}
> +
> +static void aspeed_set_pwm_port_fan_ctrl(struct aspeed_pwm_tacho_data *priv,
> +					 u8 index, u8 fan_ctrl)
> +{
> +	u16 period;
> +	u16 dc_time_on;
> +
> +	period = priv->type_pwm_clock_unit[priv->pwm_port_type[index]];
> +	period += 1;
> +	dc_time_on = (fan_ctrl * period) / PWM_MAX;
> +
> +	if (dc_time_on == 0) {
> +		aspeed_set_pwm_port_enable(priv->base, index, false);
> +	} else {
> +		if (dc_time_on == period)
> +			dc_time_on = 0;
> +
> +		aspeed_set_pwm_port_duty_rising_falling(priv->base, index, 0,
> +							dc_time_on);
> +		aspeed_set_pwm_port_enable(priv->base, index, true);
> +	}
> +}
> +
> +static u32 aspeed_get_fan_tach_ch_measure_period(struct aspeed_pwm_tacho_data
> +						 *priv, u8 type)
> +{
> +	u32 clk;
> +	u16 tacho_unit;
> +	u8 clk_unit, div_h, div_l, tacho_div;
> +
> +	clk = priv->clk_freq;
> +
> +	clk_unit = priv->type_pwm_clock_unit[type];
> +	div_h = priv->type_pwm_clock_division_h[type];
> +	div_h = 0x1 << div_h;
> +	div_l = priv->type_pwm_clock_division_l[type];
> +	if (div_l == 0)
> +		div_l = 1;
> +	else
> +		div_l = div_l * 2;
> +
> +	tacho_unit = priv->type_fan_tach_unit[type];
> +	tacho_div = priv->type_fan_tach_clock_division[type];
> +
> +	tacho_div = 0x4 << (tacho_div * 2);
> +	return clk / (clk_unit * div_h * div_l * tacho_div * tacho_unit);
> +}
> +
> +static u32 aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv,
> +				      u8 fan_tach_ch)
> +{
> +	u32 raw_data, rpm, tach_div, clk_source, timeout = 0, sec;
> +	u8 fan_tach_ch_source, type;
> +	void __iomem *base = priv->base;
> +
> +	iowrite32(0, priv->base + ASPEED_PTCR_TRIGGER);
> +	iowrite32(0x1 << fan_tach_ch, priv->base + ASPEED_PTCR_TRIGGER);
> +
> +	fan_tach_ch_source = priv->fan_tach_ch_source[fan_tach_ch];
> +	type = priv->pwm_port_type[fan_tach_ch_source];
> +
> +	sec = (1000 / aspeed_get_fan_tach_ch_measure_period(priv, type));
> +
> +	msleep(sec);
> +
> +	while (!(ioread32(priv->base + ASPEED_PTCR_RESULT) &
> +				(0x1 << RESULT_STATUS))) {
> +		timeout++;
> +		if (timeout > 1)
> +			return 0;
> +		msleep(sec);
> +	};
> +
> +	raw_data = (ioread32(base + ASPEED_PTCR_RESULT)) &
> +				RESULT_VALUE_MASK;
> +	tach_div = priv->type_fan_tach_clock_division[type];
> +
> +	tach_div = 0x4 << (tach_div * 2);
> +	clk_source = priv->clk_freq;
> +	rpm = (clk_source * 60) / (2 * raw_data * tach_div);

Can raw_data be 0 ?

> +	return rpm;
> +}
> +
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int index = sensor_attr->index;
> +	int ret;
> +
No empty line here, please.

> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +	long fan_ctrl;
> +
> +	ret = kstrtol(buf, 10, &fan_ctrl);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (fan_ctrl < 0 || fan_ctrl > PWM_MAX)
> +		return -EINVAL;
> +
> +	if (priv->pwm_port_fan_ctrl[index] == fan_ctrl)
> +		return count;
> +
> +	priv->pwm_port_fan_ctrl[index] = fan_ctrl;
> +	aspeed_set_pwm_port_fan_ctrl(priv, index, fan_ctrl);
> +
> +	return count;
> +}
> +
> +static ssize_t show_pwm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int index = sensor_attr->index;
> +
In general, please keep variable declarations together with no empty line in between.

> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", priv->pwm_port_fan_ctrl[index]);
> +}
> +
> +static ssize_t show_rpm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int index = sensor_attr->index;
> +	u32 rpm;
> +
> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +
> +	rpm = aspeed_get_fan_tach_ch_rpm(priv, index);
> +
> +	return sprintf(buf, "%u\n", rpm);
> +}
> +
> +#define pwm_index(index)					\
> +static SENSOR_DEVICE_ATTR(pwm##index, 0644,	\
> +			show_pwm, set_pwm, index - 1);		\
> +								\
> +static struct attribute *pwm##index##_dev_attrs[] = {		\
> +	&sensor_dev_attr_pwm##index.dev_attr.attr,		\
> +	NULL,							\
> +};								\
> +static const struct attribute_group pwm##index##_dev_groups = {	\
> +	.attrs = pwm##index##_dev_attrs,			\
> +}
> +
> +#define fan_index(index)					\
> +static SENSOR_DEVICE_ATTR(fan##index##_input, 0444,		\
> +		show_rpm, NULL, index - 1);			\
> +								\
> +static struct attribute *fan##index##_dev_attrs[] = {		\
> +	&sensor_dev_attr_fan##index##_input.dev_attr.attr,	\
> +	NULL,							\
> +};								\
> +static const struct attribute_group fan##index##_dev_groups = {	\
> +	.attrs = fan##index##_dev_attrs,			\
> +}
> +
> +pwm_index(1);
> +pwm_index(2);
> +pwm_index(3);
> +pwm_index(4);
> +pwm_index(5);
> +pwm_index(6);
> +pwm_index(7);
> +pwm_index(8);
> +
> +fan_index(1);
> +fan_index(2);
> +fan_index(3);
> +fan_index(4);
> +fan_index(5);
> +fan_index(6);
> +fan_index(7);
> +fan_index(8);
> +fan_index(9);
> +fan_index(10);
> +fan_index(11);
> +fan_index(12);
> +fan_index(13);
> +fan_index(14);
> +fan_index(15);
> +fan_index(16);
> +
Please no such macros, and not that many groups. One or at most two groups
(one for pwm, one for fans), then use the is_visible callback to determine
if an attribute is visible.


> +/*
> + * If the clock type is type M then :
> + * The PWM frequency = 24MHz / (type M clock division L bit *
> + * type M clock division H bit * (type M PWM period bit + 1))
> + * Calculate type M clock division L bit and H bits given the other values
> + */
> +static int aspeed_create_type(struct device_node *child,
> +			      struct aspeed_pwm_tacho_data *priv,
> +			      u8 index)
> +{
> +	u8 period, div_l, div_h;
> +	bool enable;
> +	u8 mode, div;
> +	u16 unit;
> +
> +	of_property_read_u8(child, "pwm_period", &period);
> +	of_property_read_u8(child, "pwm_clock_division_h", &div_h);
> +	of_property_read_u8(child, "pwm_clock_division_l", &div_l);

What happens if those properties are undefined ?

> +	priv->type_pwm_clock_division_h[index] = div_h;
> +	priv->type_pwm_clock_division_l[index] = div_l;
> +	priv->type_pwm_clock_unit[index] = period;
> +	aspeed_set_pwm_clock_values(priv->base, index, div_h, div_l, period);
> +
> +	enable = of_property_read_bool(child, "fan_tach_enable");
> +	aspeed_set_tacho_type_enable(priv->base, index, enable);
> +
> +	of_property_read_u8(child, "fan_tach_clock_division", &div);
> +	priv->type_fan_tach_clock_division[index] = div;
> +
> +	of_property_read_u8(child, "fan_tach_mode_selection", &mode);
> +
> +	of_property_read_u16(child, "fan_tach_period", &unit);
> +	priv->type_fan_tach_unit[index] = unit;
> +	aspeed_set_tacho_type_values(priv->base, index, mode, unit, div);
> +
> +	return 0;
> +}
> +
> +static int aspeed_create_pwm_port(struct device_node *child,
> +				  struct aspeed_pwm_tacho_data *priv, u8 index,
> +				  u8 group_index)
> +{
> +	u8 val;
> +	bool enable;
> +
> +	switch (index) {
> +	case 1:
> +		priv->groups[group_index] = &pwm1_dev_groups;
> +		break;
> +	case 2:
> +		priv->groups[group_index] = &pwm2_dev_groups;
> +		break;
> +	case 3:
> +		priv->groups[group_index] = &pwm3_dev_groups;
> +		break;
> +	case 4:
> +		priv->groups[group_index] = &pwm4_dev_groups;
> +		break;
> +	case 5:
> +		priv->groups[group_index] = &pwm5_dev_groups;
> +		break;
> +	case 6:
> +		priv->groups[group_index] = &pwm6_dev_groups;
> +		break;
> +	case 7:
> +		priv->groups[group_index] = &pwm7_dev_groups;
> +		break;
> +	case 8:
> +		priv->groups[group_index] = &pwm8_dev_groups;
> +		break;
> +	}
> +
> +	enable = of_property_read_bool(child, "enable");
> +	aspeed_set_pwm_port_enable(priv->base, index - 1, enable);
> +
This looks wrong. If the child is not enabled in the devicetree,
per standard devicetree convention, we should not get here.

Overall I am a bit guessing here, but it seems that you want consecutive
attributes with no gap. This isn't the way to go, though. The attribute
names associate with a given port should not change if another port is
enabled or disabled. pwmX should always point to a well defined pwm
pin on the chip. So, again, please use one group, and enable the individual
attributes with is_visible.

> +	of_property_read_u8(child, "type", &val);

What happens if this property is undefined ?

> +	priv->pwm_port_type[index - 1] = val;
> +	aspeed_set_pwm_port_type(priv->base, index - 1, val);
> +
> +	of_property_read_u8(child, "fan_ctrl", &val);

and if undefined ?

> +	priv->pwm_port_fan_ctrl[index - 1] = val;
> +	aspeed_set_pwm_port_fan_ctrl(priv, index - 1, val);
> +
> +	return 0;
> +}
> +
> +static int aspeed_create_fan_tach_channel(struct device *dev,
> +					  struct device_node *child,
> +					  struct aspeed_pwm_tacho_data *priv,
> +					  u8 index, u8 group_index)
> +{
> +	u8 val;
> +	bool enable;
> +	struct gpio_desc *fan_ctrl = devm_gpiod_get(dev, "fan-ctrl", GPIOD_IN);
> +
> +	if (IS_ERR(fan_ctrl))
> +		return PTR_ERR(fan_ctrl);
> +
> +	switch (index) {
> +	case 1:
> +		priv->groups[group_index] = &fan1_dev_groups;
> +		break;
> +	case 2:
> +		priv->groups[group_index] = &fan2_dev_groups;
> +		break;
> +	case 3:
> +		priv->groups[group_index] = &fan3_dev_groups;
> +		break;
> +	case 4:
> +		priv->groups[group_index] = &fan4_dev_groups;
> +		break;
> +	case 5:
> +		priv->groups[group_index] = &fan5_dev_groups;
> +		break;
> +	case 6:
> +		priv->groups[group_index] = &fan6_dev_groups;
> +		break;
> +	case 7:
> +		priv->groups[group_index] = &fan7_dev_groups;
> +		break;
> +	case 8:
> +		priv->groups[group_index] = &fan8_dev_groups;
> +		break;
> +	case 9:
> +		priv->groups[group_index] = &fan9_dev_groups;
> +		break;
> +	case 10:
> +		priv->groups[group_index] = &fan10_dev_groups;
> +		break;
> +	case 11:
> +		priv->groups[group_index] = &fan11_dev_groups;
> +		break;
> +	case 12:
> +		priv->groups[group_index] = &fan12_dev_groups;
> +		break;
> +	case 13:
> +		priv->groups[group_index] = &fan13_dev_groups;
> +		break;
> +	case 14:
> +		priv->groups[group_index] = &fan14_dev_groups;
> +		break;
> +	case 15:
> +		priv->groups[group_index] = &fan15_dev_groups;
> +		break;
> +	case 16:
> +		priv->groups[group_index] = &fan16_dev_groups;
> +		break;
> +	}
> +
> +	enable = of_property_read_bool(child, "enable");
> +	aspeed_set_fan_tach_ch_enable(priv->base, index - 1, enable);
> +
> +	of_property_read_u8(child, "pwm_source", &val);

if undefined ?

> +	priv->fan_tach_ch_source[index - 1] = val;
> +	aspeed_set_fan_tach_ch_source(priv->base, index - 1, val);
> +
> +	return 0;
> +}
> +
> +static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
> +{
	struct device *dev = &pdev->dev;

might make the rest of the code easier to read.

> +	struct device_node *np, *type_np, *pwm_np, *fan_tach_np, *child;
> +	u8 pwm_index = 1, fan_index = 1, type_index = 0, group_index = 0;
> +	struct aspeed_pwm_tacho_data *priv;
> +	struct resource *res;
> +	struct device *hwmon;
> +	void __iomem *base;
> +	struct clk *clk;
> +
> +	np = pdev->dev.of_node;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENOENT;
> +
> +	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (!base)
> +		return -ENOMEM;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->base = base;
> +
> +	iowrite32(0, base + ASPEED_PTCR_TACH_SOURCE);
> +	iowrite32(0, base + ASPEED_PTCR_TACH_SOURCE_EXT);
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return -ENODEV;
> +
> +	priv->clk_freq = clk_get_rate(clk);
> +	aspeed_set_clock_enable(base, true);
> +	aspeed_set_clock_source(base, 0);
> +
> +	type_np = of_get_child_by_name(np, "type_values");
> +	for_each_child_of_node(type_np, child) {
> +		aspeed_create_type(child, priv, type_index++);
> +		of_node_put(child);

Wrong.

> +	}
> +	of_node_put(type_np);
> +	pwm_np = of_get_child_by_name(np, "pwm_port");
> +	for_each_child_of_node(pwm_np, child) {
> +		aspeed_create_pwm_port(child, priv, pwm_index++,
> +				       group_index++);
> +		of_node_put(child);

This is wrong (was there a 0day about it ? I dont recall).

> +	}
> +	of_node_put(pwm_np);
> +	fan_tach_np = of_get_child_by_name(np, "fan_tach_channel");
> +	for_each_child_of_node(fan_tach_np, child) {
> +		aspeed_create_fan_tach_channel(&pdev->dev, child, priv,
> +					       fan_index++, group_index++);
> +		of_node_put(child);

Wrong.

> +	}
> +	of_node_put(fan_tach_np);
> +	of_node_put(np);

I may be wrong, but I don't see why this would be needed.

> +
> +	hwmon = devm_hwmon_device_register_with_groups(&pdev->dev,
> +						       "aspeed_pwm_tacho",
> +						       priv, priv->groups);
> +	if (IS_ERR(hwmon))
> +		return PTR_ERR(hwmon);

return PTR_ERR_OR_ZERO(hwmon);

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_pwm_tacho_match_table[] = {
> +	{ .compatible = "aspeed,aspeed2400-pwm-tacho", },
> +	{ .compatible = "aspeed,aspeed2500-pwm-tacho", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_tacho_match_table);
> +
> +static struct platform_driver aspeed_pwm_tacho_driver = {
> +	.probe		= aspeed_pwm_tacho_probe,
> +	.driver		= {
> +		.name	= "aspeed_pwm_tacho",
> +		.owner	= THIS_MODULE,

not needed.

> +		.of_match_table = of_pwm_tacho_match_table,
> +	},
> +};
> +
> +module_platform_driver(aspeed_pwm_tacho_driver);
> +
> +MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>");
> +MODULE_DESCRIPTION("ASPEED PWM and Fan Tacho device driver");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0.390.gc69c2f50cf-goog
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH linux v1 2/2] drivers: hwmon: hwmon driver for ASPEED AST2400/2500 PWM and Fan tach controller

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2374 bytes --]

Hi Jaghathiswari,

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v4.10-rc5 next-20170120]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jaghathiswari-Rankappagounder-Natarajan/Support-for-ASPEED-AST2400-AST2500-PWM-and-Fan-Tach-driver/20170110-093628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: i386-randconfig-i1-01230650 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/hwmon/aspeed-pwm-tacho.c: In function 'aspeed_create_type':
>> drivers/hwmon/aspeed-pwm-tacho.c:656:41: warning: 'div_h' is used uninitialized in this function [-Wuninitialized]
     priv->type_pwm_clock_division_h[index] = div_h;
                                            ^

vim +/div_h +656 drivers/hwmon/aspeed-pwm-tacho.c

   640	 * The PWM frequency = 24MHz / (type M clock division L bit *
   641	 * type M clock division H bit * (type M PWM period bit + 1))
   642	 * Calculate type M clock division L bit and H bits given the other values
   643	 */
   644	static int aspeed_create_type(struct device_node *child,
   645				      struct aspeed_pwm_tacho_data *priv,
   646				      u8 index)
   647	{
   648		u8 period, div_l, div_h;
   649		bool enable;
   650		u8 mode, div;
   651		u16 unit;
   652	
   653		of_property_read_u8(child, "pwm_period", &period);
   654		of_property_read_u8(child, "pwm_clock_division_h", &div_h);
   655		of_property_read_u8(child, "pwm_clock_division_l", &div_l);
 > 656		priv->type_pwm_clock_division_h[index] = div_h;
   657		priv->type_pwm_clock_division_l[index] = div_l;
   658		priv->type_pwm_clock_unit[index] = period;
   659		aspeed_set_pwm_clock_values(priv->base, index, div_h, div_l, period);
   660	
   661		enable = of_property_read_bool(child, "fan_tach_enable");
   662		aspeed_set_tacho_type_enable(priv->base, index, enable);
   663	
   664		of_property_read_u8(child, "fan_tach_clock_division", &div);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27607 bytes --]

Re: [PATCH linux v1 1/2] Documentation: dt-bindings: Document bindings for ASPEED AST2400/AST2500 pwm and fan tach controller device driver

[Jaghathiswari Rankappagounder Natarajan]

On Fri, Jan 27, 2017 at 1:32 AM, Jaghathiswari Rankappagounder
Natarajan <jaghu@google.com> wrote:
>
>
> On Fri, Jan 13, 2017 at 8:21 AM, Rob Herring <robh@kernel.org> wrote:
>>
>> On Mon, Jan 09, 2017 at 01:59:34PM -0800, Jaghathiswari Rankappagounder
>> Natarajan wrote:
>> > This binding provides interface for adding values related to ASPEED
>> > AST2400/2500 PWM and Fan tach controller support.
>> > The PWM controller can support upto 8 PWM output ports.
>> > The Fan tach controller can support upto 16 tachometer inputs.
>> > PWM clock types M, N and 0 are three types just to have three
>> > independent
>> > PWM sources.
>> >
>> > Signed-off-by: Jaghathiswari Rankappagounder Natarajan
>> > <jaghu@google.com>
>> > ---
>> >  .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt | 153
>> > +++++++++++++++++++++
>>
>> Perhaps bindings/pwm/... even though this is more than just PWM.
>
> The corresponding hwmon driver is present in /drivers/hwmon/. So would it
> make more
>
> sense to include the devicetree document for that driver in
> /devicetree/bindings/hwmon/.
>
> In future if there any more hwmon related functionalities are added to this
> driver then /devicetree/bindings/hwmon/ would be more relevant.
>
>
>>
>>
>> >  1 file changed, 153 insertions(+)
>> >  create mode 100644
>> > Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
>> >
>> > diff --git
>> > a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
>> > b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
>> > new file mode 100644
>> > index 000000000000..8f346409ee8c
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
>> > @@ -0,0 +1,153 @@
>> > +ASPEED AST2400/AST2500 PWM and Fan Tacho controller device driver
>> > +
>> > +The ASPEED PWM controller can support upto 8 PWM outputs. The ASPEED
>> > Fan Tacho
>> > +controller can support upto 16 tachometer inputs. The PWM controller
>> > supports
>> > +3 types of frequency mode PWM for fan speed control. PWM clock types M,
>> > N and 0
>> > +are 3 types of frequency mode PWM just to have 3 independent PWM
>> > sources.
>> > +
>> > +Required properties for pwm_tacho node:
>> > +- #address-cells : should be 1.
>> > +
>> > +- #size-cells : should be 1.
>> > +
>> > +- reg : address and length of the register set for the device.
>> > +
>> > +- pinctrl-names : a pinctrl state named "default" must be defined.
>> > +
>> > +- pinctrl-0 : phandle referencing pin configuration of the
>> > AST2400/AST2500 PWM
>> > +           ports.
>> > +
>> > +- compatible : should be "aspeed,aspeed2400-pwm-tacho" for AST2400 or
>> > +            "aspeed,aspeed2500-pwm-tacho" for AST2500.
>> > +
>> > +- clocks : a fixed clock providing input clock frequency(PWM
>> > +        and Fan Tach clock)
>> > +
>> > +type_values subnode format:
>>
>> Don't use '_' in node or property names.
>
> Done.
>>
>>
>> > +===========================
>> > +Under type_values subnode there can be upto 3 child nodes indicating
>> > type M/N/O
>> > +values. Atleast one child node is required.
>> > +
>> > +Required properties for the child node(type M/N/O):
>> > +- pwm_period : indicates type M/N/O PWM period, as per the
>> > AST2400/AST2500
>> > +            datasheet. integer value in the range 0 to 255.
>>
>>
>> > +
>> > +- pwm_clock_division_l : indicates type M/N/O PWM clock division L
>> > value,
>> > +                      as per the AST2400/AST2500 datasheet.
>> > +                      integer value in the range 0 to 15.
>> > +                      0 here indicates divide 1, 1 indicates divide 2,
>> > +                      2 indicates divide 4, 3 indicates divide 6, and
>> > so on
>> > +                      till 15 indicates divide 30.
>> > +
>> > +- pwm_clock_division_h : indicates type M/N/O PWM clock division H
>> > value,
>> > +                      as per the AST2400/AST2500 datasheet.
>> > +                      integer value in the range 0 to 15.
>> > +                      0 here indicates divide 1, 1 indicates divide 2,
>> > +                      2 indicates divide 4, 3 indicates divide 8, and
>> > so on
>> > +                      till 15 indicates divide 32768.
>>
>> Can't you have a single divider value and driver convert to register
>> values? Really, you should specify the PWM period in ns/us and calculate
>> the divider based on the input clock freq. (i.e. use the clock binding).
>> There's already PWM binding to specify the period.
>>
>> I think you should have a node for the fan using the PWM binding and
>> perhaps moving some of these properties to the fan node.
>
>
> There are three different types M, N, O. These three types are just to have
>
> different PWM/Fan Tach settings. Each type can have the following settings:
>
> PWM related:
>
>               1) PWM period
>
>               2) PWM clock division high
>
>               3) PWM clock division low
>
> Fan Tach related:
>
>               1) Fan Tach type enable
>
>               2) Fan Tach clock division
>
>               3) Fan Tach mode selection
>
>               4) Fan Tach period
>
>
>
> Each PWM port should be assigned a type which can be type M, N or O.
>
>
>
> Each Fan Tach channel should be assigned a PWM source, which can be PWM port
> A
>
> through H. The Fan Tach channel implicitly gets the type (M, N or O), from
> the type assigned to its PWM source.
>
> If the clock type is type M then :
> The PWM frequency = 24MHz / (type M clock division L value * type M clock
> division H value *
>
> (type M PWM period value + 1))
>
> Because of this formula, I thought I could use properties to specify type
> clock division High and Low and
>
>  type period values so that the desired PWM frequency be achieved.
>>
>>
>> > +
>> > +- fan_tach_enable : indicates fan tach enable of type M/N/O as per the
>> > +                 AST2400/AST2500 datasheet. boolean value.
>> > +
>> > +- fan_tach_clock_division : indicates fan tach clock division as per
>> > the
>> > +                         AST2400/AST2500 datasheet.
>> > +                         integer value in the range 0 to 7.
>> > +                         0 indicates divide 4, 1 indicates divide 16,
>> > +                         2 indicates divide 64, 3 indicates divide 256
>> > +                         and so on till 7 indicates divide 65536.
>> > +
>> > +- fan_tach_mode_selection : indicates fan tach mode mode selection as
>> > per the
>> > +                         AST2400/AST2500 datasheet. integer value in
>> > the
>> > +                         range 0 to 2. 0 indicates falling edge, 1
>> > indicates
>> > +                         rising edge and 2 indicates both edges.
>> > +
>> > +- fan_tach_period : indicates fan tach period as per the
>> > AST2400/AST2500
>> > +                 datasheet. integer value (can be upto 16 bits long).
>> > +
>> > +pwm_port subnode format:
>> > +========================
>> > +Under pwm_port subnode there can upto 8 child nodes each indicating
>> > values
>> > +for one of the 8 PWM output ports.
>> > +
>> > +Required properties for each child node(starting from PWM A through PWM
>> > H):
>> > +- enable : enable PWM #X port, X ranges from A through H. boolean
>> > value.
>>
>> A connection in the PWM binding would imply this.
>
> Had to change this property to indicate PWM port number.
>
>
>>
>>
>> > +
>> > +- type : indicates type selection value of PWM #X port, X ranges from A
>> > +      through H. integer value in the range 0 to 2;
>> > +      0 indicates type M, 1 indicates type N, 2 indicates type O.
>>
>> This is meaningless to me.
>
>
> Each PWM port should be assigned a type which can be type M, N or O.
>
>
>>
>>
>> > +- fan_ctrl : set the PWM fan control initial value. integer value
>> > between
>> > +          0(off) and 255(full speed).
>> > +
>> > +fan_tach_channel subnode format:
>> > +================================
>> > +Under fan_tach_channel subnode there can be upto 16 child nodes each
>> > indicating
>> > +values for one of the 16 fan tach channels.
>> > +
>> > +Required properties for each child node(starting from fan tach #0
>> > through
>> > +fan tach #16):
>> > +- fan-ctrl-gpios : should specify the tachometer input pin on the
>> > hardware.
>>
>> I don't understand the connection here. You have a tach block with 16
>> inputs on the SoC, so why the GPIOs? If the GPIOs go to the fan, then a
>> fan node should have the GPIO properties.
>
> This property specifies the tachometer input GPIO pin on the hardware. Fan
> Tachometer function
>
>  can only work when GPIO is in ”input mode” .
>>
>>
>> > +
>> > +- enable : enable fan tach #X, X ranges from 0 through 16. boolean
>> > value.
>>
>> 'status' property is the standard way to enable a node or not. Not sure
>> if that makes sense here though.
>
> Had to change this property to indicate Fan tach channel number.
>
>
>>
>>
>> > +
>> > +- pwm_source : indicates PWM source of fan tach #X, X ranges from 0
>> > through 16.
>> > +            integer value in the range 0 to 7. 0 indicates PWM port A,
>> > +            1 indicates PWM port B and so on till 7 indicates PWM port
>> > H.
>
>