[PATCH v2 0/2] pwm: intel: Add PWM driver for a new SoC

[PATCH v2 0/2] pwm: intel: Add PWM driver for a new SoC

[Rahul Tanwar]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 1153 bytes --]

Patch 1 adds dt binding document in YAML format.
Patch 2 add PWM fan controller driver for LGM SoC.

Patch series is baselined on linux 5.8-rc1.

v2:
- Address below review concerns from Uwe Kleine-König.
  * Add notes and limitations about PWM HW.
  * Rename all functions and structure to lgm_pwm_* 
  * Readjust space aligninment in structure fields to single space.
  * Switch to using apply instead of config/enable/disable.
  * Address other code quality related concerns.
  * Rebase to 5.8-rc1.
- Address review concerns in dt binding YAML from Rob Herring.

v1:
- Initial version.

Rahul Tanwar (2):
  Add DT bindings YAML schema for PWM fan controller of LGM SoC
  Add PWM fan controller driver for LGM SoC

 .../devicetree/bindings/pwm/intel,lgm-pwm.yaml     |  57 +++
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-intel-lgm.c                        | 400 +++++++++++++++++++++
 4 files changed, 467 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
 create mode 100644 drivers/pwm/pwm-intel-lgm.c

-- 
2.11.0

[PATCH v2 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC

[Rahul Tanwar]

Intel's LGM(Lightning Mountain) SoC contains a PWM fan controller
which is only used to control the fan attached to the system. This
PWM controller does not have any other consumer other than fan.
Add DT bindings documentation for this PWM fan controller.

Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
---
 .../devicetree/bindings/pwm/intel,lgm-pwm.yaml     | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
new file mode 100644
index 000000000000..e71cc25e4e6a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/intel,lgm-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LGM SoC PWM fan controller
+
+maintainers:
+  - Rahul Tanwar <rahul.tanwar@intel.com>
+
+properties:
+  compatible:
+    const: intel,lgm-pwm
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  intel,fan-wire:
+    $ref: '/schemas/types.yaml#/definitions/uint32'
+    description: Specifies fan mode
+
+  intel,tach-plus:
+    $ref: '/schemas/types.yaml#/definitions/uint32'
+    description: Specifies fan tach pulse periods
+
+  intel,max-rpm:
+    $ref: '/schemas/types.yaml#/definitions/uint32'
+    description: Specifies maximum RPM of fan attached to the system
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+  - clocks
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm: pwm@e0d00000 {
+        compatible = "intel,lgm-pwm";
+        reg = <0xe0d00000 0x30>;
+        #pwm-cells = <2>;
+        clocks = <&cgu0 126>;
+        resets = <&rcu0 0x30 21>;
+    };
-- 
2.11.0

Re: [PATCH v2 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC

[Uwe Kleine-König]

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

Hello,

On Thu, Jun 18, 2020 at 08:05:12PM +0800, Rahul Tanwar wrote:
> Intel's LGM(Lightning Mountain) SoC contains a PWM fan controller
> which is only used to control the fan attached to the system. This
> PWM controller does not have any other consumer other than fan.
> Add DT bindings documentation for this PWM fan controller.
> 
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  .../devicetree/bindings/pwm/intel,lgm-pwm.yaml     | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
> new file mode 100644
> index 000000000000..e71cc25e4e6a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/intel,lgm-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LGM SoC PWM fan controller
> +
> +maintainers:
> +  - Rahul Tanwar <rahul.tanwar@intel.com>
> +
> +properties:
> +  compatible:
> +    const: intel,lgm-pwm
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 2
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  intel,fan-wire:
> +    $ref: '/schemas/types.yaml#/definitions/uint32'
> +    description: Specifies fan mode

The default when unspecified is 2.

> +
> +  intel,tach-plus:
> +    $ref: '/schemas/types.yaml#/definitions/uint32'
> +    description: Specifies fan tach pulse periods

Only used with fan-wire = 4, default = 2.

> +
> +  intel,max-rpm:
> +    $ref: '/schemas/types.yaml#/definitions/uint32'
> +    description: Specifies maximum RPM of fan attached to the system

Ditto.

Consistent with my concern that the fan code should not be part of the
PWM driver I wonder if this representation that mixes fan and PWM is
nice enough to be set in stone.

If we decide to keep it as is I wonder if we should drop #pwm-cells as
the hardware doesn't allow other uses for this PWM (IIUC) and so
referencing this node doesn't make sense.

> +required:
> +  - compatible
> +  - reg
> +  - "#pwm-cells"
> +  - clocks
> +  - resets
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pwm: pwm@e0d00000 {
> +        compatible = "intel,lgm-pwm";
> +        reg = <0xe0d00000 0x30>;
> +        #pwm-cells = <2>;
> +        clocks = <&cgu0 126>;
> +        resets = <&rcu0 0x30 21>;
> +    };

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/2] Add PWM fan controller driver for LGM SoC

[Rahul Tanwar]

Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
This PWM controller does not have any other consumer, it is a
dedicated PWM controller for fan attached to the system. Add
driver for this PWM fan controller.

Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
---
 drivers/pwm/Kconfig         |   9 +
 drivers/pwm/Makefile        |   1 +
 drivers/pwm/pwm-intel-lgm.c | 400 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 410 insertions(+)
 create mode 100644 drivers/pwm/pwm-intel-lgm.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index cb8d739067d2..a3303e22d5fa 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -232,6 +232,15 @@ config PWM_IMX_TPM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-imx-tpm.
 
+config PWM_INTEL_LGM
+	tristate "Intel LGM PWM support"
+	depends on X86 || COMPILE_TEST
+	help
+	  Generic PWM fan controller driver for LGM SoC.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-intel-lgm.
+
 config PWM_IQS620A
 	tristate "Azoteq IQS620A PWM support"
 	depends on MFD_IQS62X || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index a59c710e98c7..db154a6b4f51 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
 obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
 obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
 obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
+obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
 obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
new file mode 100644
index 000000000000..3c7077acb161
--- /dev/null
+++ b/drivers/pwm/pwm-intel-lgm.c
@@ -0,0 +1,400 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Intel Corporation.
+ *
+ * Notes & Limitations:
+ * - The hardware supports fixed period which is dependent on 2/3 or 4
+ *   wire fan mode.
+ * - Supports normal polarity. Does not support changing polarity.
+ * - When PWM is disabled, output of PWM will become 0(inactive). It doesn't
+ *   keep track of running period.
+ * - When duty cycle is changed, PWM output may be a mix of previous setting
+ *   and new setting for the first period. From second period, the output is
+ *   based on new setting.
+ * - Supports 100% duty cycle.
+ * - It is a dedicated PWM fan controller. There are no other consumers for
+ *   this PWM controller.
+ */
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#define PWM_FAN_CON0		0x0
+#define PWM_FAN_EN_EN		BIT(0)
+#define PWM_FAN_EN_DIS		0x0
+#define PWM_FAN_EN_MSK		BIT(0)
+#define PWM_FAN_MODE_2WIRE	0x0
+#define PWM_FAN_MODE_4WIRE	0x1
+#define PWM_FAN_MODE_MSK	BIT(1)
+#define PWM_FAN_PWM_DIS_DIS	0x0
+#define PWM_FAN_PWM_DIS_MSK	BIT(2)
+#define PWM_TACH_EN_EN		0x1
+#define PWM_TACH_EN_MSK		BIT(4)
+#define PWM_TACH_PLUS_2		0x0
+#define PWM_TACH_PLUS_4		0x1
+#define PWM_TACH_PLUS_MSK	BIT(5)
+#define PWM_FAN_DC_MSK		GENMASK(23, 16)
+
+#define PWM_FAN_CON1		0x4
+#define PWM_FAN_MAX_RPM_MSK	GENMASK(15, 0)
+
+#define PWM_FAN_STAT		0x10
+#define PWM_FAN_TACH_MASK	GENMASK(15, 0)
+
+#define MAX_RPM			(BIT(16) - 1)
+#define DFAULT_RPM		4000
+#define MAX_DUTY_CYCLE		(BIT(8) - 1)
+
+#define FRAC_BITS		10
+#define DC_BITS			8
+#define TWO_TENTH		204
+
+#define PERIOD_2WIRE_NSECS	40000000
+#define PERIOD_4WIRE_NSECS	40000
+
+#define TWO_SECONDS		2000
+#define IGNORE_FIRST_ERR	1
+#define THIRTY_SECS_WINDOW	15
+#define ERR_CNT_THRESHOLD	6
+
+struct lgm_pwm_chip {
+	struct pwm_chip chip;
+	struct regmap *regmap;
+	struct clk *clk;
+	struct reset_control *rst;
+	u32 tach_en;
+	u32 max_rpm;
+	u32 set_rpm;
+	u32 set_dc;
+	u32 period;
+	struct delayed_work work;
+};
+
+static inline struct lgm_pwm_chip *to_lgm_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct lgm_pwm_chip, chip);
+}
+
+static int lgm_pwm_update_dc(struct lgm_pwm_chip *pc, u32 val)
+{
+	return regmap_update_bits(pc->regmap, PWM_FAN_CON0, PWM_FAN_DC_MSK,
+				  FIELD_PREP(PWM_FAN_DC_MSK, val));
+}
+
+static int lgm_pwm_enable(struct pwm_chip *chip, bool enable)
+{
+	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
+	struct regmap *regmap = pc->regmap;
+
+	if (enable) {
+		regmap_update_bits(regmap, PWM_FAN_CON0,
+				   PWM_FAN_EN_MSK, PWM_FAN_EN_EN);
+		if (pc->tach_en)
+			schedule_delayed_work(&pc->work, msecs_to_jiffies(10000));
+	} else {
+		if (pc->tach_en)
+			cancel_delayed_work_sync(&pc->work);
+		regmap_update_bits(regmap, PWM_FAN_CON0,
+			   	   PWM_FAN_EN_MSK, PWM_FAN_EN_DIS);
+	}
+
+	return 0;
+}
+
+static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
+	struct pwm_state cur_state;
+	u32 duty_cycle, duty, val;
+	int ret = 0;
+
+	if (state->polarity != PWM_POLARITY_NORMAL ||
+	    state->period != pc->period)
+		return -EINVAL;
+
+	cur_state = pwm->state;
+	duty_cycle = state->duty_cycle;
+	if (!state->enabled)
+		duty_cycle = 0;
+
+	duty = duty_cycle * (1U << DC_BITS);
+	val = DIV_ROUND_CLOSEST(duty, state->period);
+	val = min_t(u32, val, MAX_DUTY_CYCLE);
+
+	if (pc->tach_en) {
+		pc->set_dc = val;
+		pc->set_rpm = val * pc->max_rpm / MAX_DUTY_CYCLE;
+	}
+
+	ret = lgm_pwm_update_dc(pc, val);
+
+	if (state->enabled != cur_state.enabled)
+		lgm_pwm_enable(chip, state->enabled);
+
+	return ret;
+}
+
+static void lgm_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			      struct pwm_state *state)
+{
+	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
+	u32 duty, val;
+
+	state->enabled = regmap_test_bits(pc->regmap, PWM_FAN_CON0,
+					  PWM_FAN_EN_EN);
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->period = pc->period; /* fixed period */
+
+	regmap_read(pc->regmap, PWM_FAN_CON0, &val);
+	duty = FIELD_GET(PWM_FAN_DC_MSK, val);
+	state->duty_cycle = duty * pc->period >> DC_BITS;
+}
+
+static const struct pwm_ops lgm_pwm_ops = {
+	.get_state = lgm_pwm_get_state,
+	.apply = lgm_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static void lgm_pwm_tach_work(struct work_struct *work)
+{
+	struct lgm_pwm_chip *pc = container_of(work, struct lgm_pwm_chip,
+						 work.work);
+	struct regmap *regmap = pc->regmap;
+	u32 fan_tach, fan_dc, val;
+	s32 diff;
+	static u32 fanspeed_err_cnt, time_window, delta_dc;
+
+	/*
+	 * Fan speed is tracked by reading the active duty cycle of PWM output
+	 * from the active duty cycle register. Some variance in the duty cycle
+	 * register value is expected. So we set a time window of 30 seconds and
+	 * if we detect inaccurate fan speed 6 times within 30 seconds then we
+	 * mark it as fan speed problem and fix it by readjusting the duty cycle.
+	 */
+
+	if (fanspeed_err_cnt > IGNORE_FIRST_ERR)
+		/*
+		 * Ignore first time we detect inaccurate fan speed
+		 * because it is expected during bootup.
+		 */
+		time_window++;
+
+	if (time_window == THIRTY_SECS_WINDOW) {
+		/*
+		 * This work is scheduled every 2 seconds i.e. each time_window
+		 * counter step roughly mean 2 seconds. When the time window
+		 * reaches 30 seconds, reset all the counters/logic.
+		 */
+		fanspeed_err_cnt = 0;
+		delta_dc = 0;
+		time_window = 0;
+	}
+
+	regmap_read(regmap, PWM_FAN_STAT, &fan_tach);
+	fan_tach &= PWM_FAN_TACH_MASK;
+	if (!fan_tach)
+		goto restart_work;
+
+	val = DIV_ROUND_CLOSEST(pc->set_rpm << FRAC_BITS, fan_tach);
+	diff = val - BIT(FRAC_BITS);
+
+	if (abs(diff) > TWO_TENTH) {
+		/* if duty cycle diff is more than two tenth, detect it as error */
+		if (fanspeed_err_cnt > IGNORE_FIRST_ERR)
+			delta_dc += val;
+		fanspeed_err_cnt++;
+	}
+
+	if (fanspeed_err_cnt == ERR_CNT_THRESHOLD) {
+		/*
+		 * We detected fan speed errors 6 times with 30 seconds.
+		 * Fix the error by readjusting duty cycle and reset
+		 * our counters/logic.
+		 */
+		fan_dc = pc->set_dc * delta_dc >> (FRAC_BITS + 2);
+		fan_dc = min_t(u32, fan_dc, MAX_DUTY_CYCLE);
+		lgm_pwm_update_dc(pc, fan_dc);
+		fanspeed_err_cnt = 0;
+		delta_dc = 0;
+		time_window = 0;
+	}
+
+restart_work:
+	/*
+	 * Fan speed doesn't need continous tracking. Schedule this work
+	 * every two seconds so it doesn't steal too much cpu cycles.
+	 */
+	schedule_delayed_work(&pc->work, msecs_to_jiffies(TWO_SECONDS));
+}
+
+static void lgm_pwm_init(struct lgm_pwm_chip *pc)
+{
+	struct device *dev = pc->chip.dev;
+	struct regmap *regmap = pc->regmap;
+	u32 max_rpm, fan_wire, tach_plus, con0_val, con0_mask;
+
+	if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire))
+		fan_wire = 2; /* default is 2 wire mode */
+
+	con0_val = FIELD_PREP(PWM_FAN_PWM_DIS_MSK, PWM_FAN_PWM_DIS_DIS);
+	con0_mask = PWM_FAN_PWM_DIS_MSK | PWM_FAN_MODE_MSK;
+
+	switch (fan_wire) {
+	case 4:
+		con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_4WIRE) |
+			    FIELD_PREP(PWM_TACH_EN_MSK, PWM_TACH_EN_EN);
+		con0_mask |= PWM_TACH_EN_MSK | PWM_TACH_PLUS_MSK;
+		pc->tach_en = 1;
+		pc->period = PERIOD_4WIRE_NSECS;
+		break;
+	default:
+		/* default is 2wire mode */
+		con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE);
+		pc->period = PERIOD_2WIRE_NSECS;
+		break;
+	}
+
+	if (pc->tach_en) {
+		if (device_property_read_u32(dev, "intel,tach-plus",
+					     &tach_plus))
+			tach_plus = 2;
+
+		switch (tach_plus) {
+		case 4:
+			con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK,
+					       PWM_TACH_PLUS_4);
+			break;
+		default:
+			con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK,
+					       PWM_TACH_PLUS_2);
+			break;
+		}
+
+		if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm))
+			max_rpm = DFAULT_RPM;
+
+		max_rpm = min_t(u32, max_rpm, MAX_RPM);
+		if (max_rpm == 0)
+			max_rpm = DFAULT_RPM;
+
+		pc->max_rpm = max_rpm;
+		INIT_DEFERRABLE_WORK(&pc->work, lgm_pwm_tach_work);
+		regmap_update_bits(regmap, PWM_FAN_CON1,
+				   PWM_FAN_MAX_RPM_MSK, max_rpm);
+	}
+
+	regmap_update_bits(regmap, PWM_FAN_CON0, con0_mask, con0_val);
+}
+
+static const struct regmap_config pwm_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+};
+
+static int lgm_pwm_probe(struct platform_device *pdev)
+{
+	struct lgm_pwm_chip *pc;
+	struct device *dev = &pdev->dev;
+	void __iomem *io_base;
+	int ret;
+
+	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
+	if (!pc)
+		return -ENOMEM;
+
+	io_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
+
+	pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config);
+	if (IS_ERR(pc->regmap)) {
+		ret = PTR_ERR(pc->regmap);
+		dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
+		return ret;
+	}
+
+	pc->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pc->clk)) {
+		ret = PTR_ERR(pc->clk);
+		dev_err(dev, "failed to get clock: %pe\n", pc->clk);
+		return ret;
+	}
+
+	pc->rst = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(pc->rst)) {
+		ret = PTR_ERR(pc->rst);
+		dev_err(dev, "failed to get reset control: %pe\n", pc->rst);
+		return ret;
+	}
+
+	ret = reset_control_deassert(pc->rst);
+	if (ret) {
+		dev_err(dev, "cannot deassert reset control: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	ret = clk_prepare_enable(pc->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clock\n");
+		return ret;
+	}
+
+	pc->chip.dev = dev;
+	pc->chip.ops = &lgm_pwm_ops;
+	pc->chip.npwm = 1;
+
+	lgm_pwm_init(pc);
+
+	ret = pwmchip_add(&pc->chip);
+	if (ret < 0) {
+		dev_err(dev, "failed to add PWM chip: %d\n", ret);
+		clk_disable_unprepare(pc->clk);
+		reset_control_assert(pc->rst);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pc);
+	return 0;
+}
+
+static int lgm_pwm_remove(struct platform_device *pdev)
+{
+	struct lgm_pwm_chip *pc = platform_get_drvdata(pdev);
+	int ret;
+
+	if (pc->tach_en)
+		cancel_delayed_work_sync(&pc->work);
+
+	ret = pwmchip_remove(&pc->chip);
+	if (ret < 0)
+		return ret;
+
+	clk_disable_unprepare(pc->clk);
+	reset_control_assert(pc->rst);
+
+	return 0;
+}
+
+static const struct of_device_id lgm_pwm_of_match[] = {
+	{ .compatible = "intel,lgm-pwm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lgm_pwm_of_match);
+
+static struct platform_driver lgm_pwm_driver = {
+	.driver = {
+		.name = "intel-pwm",
+		.of_match_table = lgm_pwm_of_match,
+	},
+	.probe = lgm_pwm_probe,
+	.remove = lgm_pwm_remove,
+};
+module_platform_driver(lgm_pwm_driver);
-- 
2.11.0

Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC

[Philipp Zabel]

Hi Rahul,

On Thu, 2020-06-18 at 20:05 +0800, Rahul Tanwar wrote:
> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
> This PWM controller does not have any other consumer, it is a
> dedicated PWM controller for fan attached to the system. Add
> driver for this PWM fan controller.
> 
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  drivers/pwm/Kconfig         |   9 +
>  drivers/pwm/Makefile        |   1 +
>  drivers/pwm/pwm-intel-lgm.c | 400 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 410 insertions(+)
>  create mode 100644 drivers/pwm/pwm-intel-lgm.c
> 
[...]
> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
> new file mode 100644
> index 000000000000..3c7077acb161
> --- /dev/null
> +++ b/drivers/pwm/pwm-intel-lgm.c
> @@ -0,0 +1,400 @@
[...]
> +static int lgm_pwm_probe(struct platform_device *pdev)
> +{
> +	struct lgm_pwm_chip *pc;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *io_base;
> +	int ret;
> +
> +	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> +	if (!pc)
> +		return -ENOMEM;
> +
> +	io_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config);
> +	if (IS_ERR(pc->regmap)) {
> +		ret = PTR_ERR(pc->regmap);
> +		dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
> +		return ret;
> +	}
> +
> +	pc->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pc->clk)) {
> +		ret = PTR_ERR(pc->clk);
> +		dev_err(dev, "failed to get clock: %pe\n", pc->clk);
> +		return ret;
> +	}
> +
> +	pc->rst = devm_reset_control_get(dev, NULL);
> +	if (IS_ERR(pc->rst)) {
> +		ret = PTR_ERR(pc->rst);
> +		dev_err(dev, "failed to get reset control: %pe\n", pc->rst);
> +		return ret;
> +	}

Please use devm_reset_control_get_exclusive() to make it explicit an
that exclusive reset control is requested. Given how the reset control
is used, I think this driver could also use
devm_reset_control_get_shared() to potentially allow sharing a reset
line with other devices.

regards
Philipp

Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC

[Tanwar, Rahul]

Hi Philipp,

On 18/6/2020 8:25 pm, Philipp Zabel wrote:
> Hi Rahul,
>
> On Thu, 2020-06-18 at 20:05 +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>> This PWM controller does not have any other consumer, it is a
>> dedicated PWM controller for fan attached to the system. Add
>> driver for this PWM fan controller.
>>
>> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
>> ---
>>  drivers/pwm/Kconfig         |   9 +
>>  drivers/pwm/Makefile        |   1 +
>>  drivers/pwm/pwm-intel-lgm.c | 400 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 410 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-intel-lgm.c
>>
> [...]
>> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
>> new file mode 100644
>> index 000000000000..3c7077acb161
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-intel-lgm.c
>> @@ -0,0 +1,400 @@
> [...]
>> +static int lgm_pwm_probe(struct platform_device *pdev)
>> +{
>> +	struct lgm_pwm_chip *pc;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *io_base;
>> +	int ret;
>> +
>> +	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
>> +	if (!pc)
>> +		return -ENOMEM;
>> +
>> +	io_base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(io_base))
>> +		return PTR_ERR(io_base);
>> +
>> +	pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config);
>> +	if (IS_ERR(pc->regmap)) {
>> +		ret = PTR_ERR(pc->regmap);
>> +		dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
>> +		return ret;
>> +	}
>> +
>> +	pc->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(pc->clk)) {
>> +		ret = PTR_ERR(pc->clk);
>> +		dev_err(dev, "failed to get clock: %pe\n", pc->clk);
>> +		return ret;
>> +	}
>> +
>> +	pc->rst = devm_reset_control_get(dev, NULL);
>> +	if (IS_ERR(pc->rst)) {
>> +		ret = PTR_ERR(pc->rst);
>> +		dev_err(dev, "failed to get reset control: %pe\n", pc->rst);
>> +		return ret;
>> +	}
> Please use devm_reset_control_get_exclusive() to make it explicit an
> that exclusive reset control is requested. Given how the reset control
> is used, I think this driver could also use
> devm_reset_control_get_shared() to potentially allow sharing a reset
> line with other devices.

devm_reset_control_get() is a wrapper for devm_reset_control_get_exclusive().
Code as below:
static inline struct reset_control *devm_reset_control_get(
                                struct device *dev, const char *id)
{
        return devm_reset_control_get_exclusive(dev, id);
}
Am i missing something else?

Regards,
Rahul

Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC

[Uwe Kleine-König]

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

On Thu, Jun 25, 2020 at 12:23:54PM +0800, Tanwar, Rahul wrote:
> 
> Hi Philipp,
> 
> On 18/6/2020 8:25 pm, Philipp Zabel wrote:
> > Hi Rahul,
> >
> > On Thu, 2020-06-18 at 20:05 +0800, Rahul Tanwar wrote:
> >> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
> >> This PWM controller does not have any other consumer, it is a
> >> dedicated PWM controller for fan attached to the system. Add
> >> driver for this PWM fan controller.
> >>
> >> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> >> ---
> >>  drivers/pwm/Kconfig         |   9 +
> >>  drivers/pwm/Makefile        |   1 +
> >>  drivers/pwm/pwm-intel-lgm.c | 400 ++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 410 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-intel-lgm.c
> >>
> > [...]
> >> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
> >> new file mode 100644
> >> index 000000000000..3c7077acb161
> >> --- /dev/null
> >> +++ b/drivers/pwm/pwm-intel-lgm.c
> >> @@ -0,0 +1,400 @@
> > [...]
> >> +static int lgm_pwm_probe(struct platform_device *pdev)
> >> +{
> >> +	struct lgm_pwm_chip *pc;
> >> +	struct device *dev = &pdev->dev;
> >> +	void __iomem *io_base;
> >> +	int ret;
> >> +
> >> +	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> >> +	if (!pc)
> >> +		return -ENOMEM;
> >> +
> >> +	io_base = devm_platform_ioremap_resource(pdev, 0);
> >> +	if (IS_ERR(io_base))
> >> +		return PTR_ERR(io_base);
> >> +
> >> +	pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config);
> >> +	if (IS_ERR(pc->regmap)) {
> >> +		ret = PTR_ERR(pc->regmap);
> >> +		dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
> >> +		return ret;
> >> +	}
> >> +
> >> +	pc->clk = devm_clk_get(dev, NULL);
> >> +	if (IS_ERR(pc->clk)) {
> >> +		ret = PTR_ERR(pc->clk);
> >> +		dev_err(dev, "failed to get clock: %pe\n", pc->clk);
> >> +		return ret;
> >> +	}
> >> +
> >> +	pc->rst = devm_reset_control_get(dev, NULL);
> >> +	if (IS_ERR(pc->rst)) {
> >> +		ret = PTR_ERR(pc->rst);
> >> +		dev_err(dev, "failed to get reset control: %pe\n", pc->rst);
> >> +		return ret;
> >> +	}
> > Please use devm_reset_control_get_exclusive() to make it explicit an
> > that exclusive reset control is requested. Given how the reset control
> > is used, I think this driver could also use
> > devm_reset_control_get_shared() to potentially allow sharing a reset
> > line with other devices.
> 
> devm_reset_control_get() is a wrapper for devm_reset_control_get_exclusive().
> Code as below:
> static inline struct reset_control *devm_reset_control_get(
>                                 struct device *dev, const char *id)
> {
>         return devm_reset_control_get_exclusive(dev, id);
> }
> Am i missing something else?

Obviously you're missing the comment above of_reset_control_get about
some functions being compatibility wrappers.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC

[Tanwar, Rahul]

On 25/6/2020 1:58 pm, Uwe Kleine-König wrote:
> On Thu, Jun 25, 2020 at 12:23:54PM +0800, Tanwar, Rahul wrote:
>> Hi Philipp,
>>
>> On 18/6/2020 8:25 pm, Philipp Zabel wrote:
>>> Hi Rahul,
>>>
>>> On Thu, 2020-06-18 at 20:05 +0800, Rahul Tanwar wrote:
>>>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>>>> This PWM controller does not have any other consumer, it is a
>>>> dedicated PWM controller for fan attached to the system. Add
>>>> driver for this PWM fan controller.
>>>>
>>>> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
>>>> ---
>>>>  drivers/pwm/Kconfig         |   9 +
>>>>  drivers/pwm/Makefile        |   1 +
>>>>  drivers/pwm/pwm-intel-lgm.c | 400 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 410 insertions(+)
>>>>  create mode 100644 drivers/pwm/pwm-intel-lgm.c
>>>>
>>> [...]
>>>> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
>>>> new file mode 100644
>>>> index 000000000000..3c7077acb161
>>>> --- /dev/null
>>>> +++ b/drivers/pwm/pwm-intel-lgm.c
>>>> @@ -0,0 +1,400 @@
>>> [...]
>>>> +static int lgm_pwm_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct lgm_pwm_chip *pc;
>>>> +	struct device *dev = &pdev->dev;
>>>> +	void __iomem *io_base;
>>>> +	int ret;
>>>> +
>>>> +	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
>>>> +	if (!pc)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	io_base = devm_platform_ioremap_resource(pdev, 0);
>>>> +	if (IS_ERR(io_base))
>>>> +		return PTR_ERR(io_base);
>>>> +
>>>> +	pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config);
>>>> +	if (IS_ERR(pc->regmap)) {
>>>> +		ret = PTR_ERR(pc->regmap);
>>>> +		dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	pc->clk = devm_clk_get(dev, NULL);
>>>> +	if (IS_ERR(pc->clk)) {
>>>> +		ret = PTR_ERR(pc->clk);
>>>> +		dev_err(dev, "failed to get clock: %pe\n", pc->clk);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	pc->rst = devm_reset_control_get(dev, NULL);
>>>> +	if (IS_ERR(pc->rst)) {
>>>> +		ret = PTR_ERR(pc->rst);
>>>> +		dev_err(dev, "failed to get reset control: %pe\n", pc->rst);
>>>> +		return ret;
>>>> +	}
>>> Please use devm_reset_control_get_exclusive() to make it explicit an
>>> that exclusive reset control is requested. Given how the reset control
>>> is used, I think this driver could also use
>>> devm_reset_control_get_shared() to potentially allow sharing a reset
>>> line with other devices.
>> devm_reset_control_get() is a wrapper for devm_reset_control_get_exclusive().
>> Code as below:
>> static inline struct reset_control *devm_reset_control_get(
>>                                 struct device *dev, const char *id)
>> {
>>         return devm_reset_control_get_exclusive(dev, id);
>> }
>> Am i missing something else?
> Obviously you're missing the comment above of_reset_control_get about
> some functions being compatibility wrappers.

Oops, so sorry totally missed/overlooked that. Will update in v3. Thanks.

Regards,
Rahul

Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC

[Uwe Kleine-König]

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

Hello Rahul,

On Thu, Jun 18, 2020 at 08:05:13PM +0800, Rahul Tanwar wrote:
> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
> This PWM controller does not have any other consumer, it is a
> dedicated PWM controller for fan attached to the system. Add
> driver for this PWM fan controller.
> 
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  drivers/pwm/Kconfig         |   9 +
>  drivers/pwm/Makefile        |   1 +
>  drivers/pwm/pwm-intel-lgm.c | 400 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 410 insertions(+)
>  create mode 100644 drivers/pwm/pwm-intel-lgm.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index cb8d739067d2..a3303e22d5fa 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -232,6 +232,15 @@ config PWM_IMX_TPM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-imx-tpm.
>  
> +config PWM_INTEL_LGM
> +	tristate "Intel LGM PWM support"
> +	depends on X86 || COMPILE_TEST
> +	help
> +	  Generic PWM fan controller driver for LGM SoC.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-intel-lgm.
> +
>  config PWM_IQS620A
>  	tristate "Azoteq IQS620A PWM support"
>  	depends on MFD_IQS62X || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index a59c710e98c7..db154a6b4f51 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
>  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
>  obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
>  obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
> +obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
>  obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
> new file mode 100644
> index 000000000000..3c7077acb161
> --- /dev/null
> +++ b/drivers/pwm/pwm-intel-lgm.c
> @@ -0,0 +1,400 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Intel Corporation.
> + *
> + * Notes & Limitations:
> + * - The hardware supports fixed period which is dependent on 2/3 or 4
> + *   wire fan mode.
> + * - Supports normal polarity. Does not support changing polarity.
> + * - When PWM is disabled, output of PWM will become 0(inactive). It doesn't
> + *   keep track of running period.
> + * - When duty cycle is changed, PWM output may be a mix of previous setting
> + *   and new setting for the first period. From second period, the output is
> + *   based on new setting.
> + * - Supports 100% duty cycle.
> + * - It is a dedicated PWM fan controller. There are no other consumers for
> + *   this PWM controller.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +#define PWM_FAN_CON0		0x0
> +#define PWM_FAN_EN_EN		BIT(0)
> +#define PWM_FAN_EN_DIS		0x0
> +#define PWM_FAN_EN_MSK		BIT(0)
> +#define PWM_FAN_MODE_2WIRE	0x0
> +#define PWM_FAN_MODE_4WIRE	0x1
> +#define PWM_FAN_MODE_MSK	BIT(1)
> +#define PWM_FAN_PWM_DIS_DIS	0x0
> +#define PWM_FAN_PWM_DIS_MSK	BIT(2)
> +#define PWM_TACH_EN_EN		0x1
> +#define PWM_TACH_EN_MSK		BIT(4)
> +#define PWM_TACH_PLUS_2		0x0
> +#define PWM_TACH_PLUS_4		0x1
> +#define PWM_TACH_PLUS_MSK	BIT(5)
> +#define PWM_FAN_DC_MSK		GENMASK(23, 16)
> +
> +#define PWM_FAN_CON1		0x4
> +#define PWM_FAN_MAX_RPM_MSK	GENMASK(15, 0)
> +
> +#define PWM_FAN_STAT		0x10
> +#define PWM_FAN_TACH_MASK	GENMASK(15, 0)
> +
> +#define MAX_RPM			(BIT(16) - 1)
> +#define DFAULT_RPM		4000
> +#define MAX_DUTY_CYCLE		(BIT(8) - 1)
> +
> +#define FRAC_BITS		10
> +#define DC_BITS			8
> +#define TWO_TENTH		204
> +
> +#define PERIOD_2WIRE_NSECS	40000000
> +#define PERIOD_4WIRE_NSECS	40000
> +
> +#define TWO_SECONDS		2000
> +#define IGNORE_FIRST_ERR	1
> +#define THIRTY_SECS_WINDOW	15
> +#define ERR_CNT_THRESHOLD	6
> +
> +struct lgm_pwm_chip {
> +	struct pwm_chip chip;
> +	struct regmap *regmap;
> +	struct clk *clk;
> +	struct reset_control *rst;
> +	u32 tach_en;
> +	u32 max_rpm;
> +	u32 set_rpm;
> +	u32 set_dc;
> +	u32 period;
> +	struct delayed_work work;
> +};
> +
> +static inline struct lgm_pwm_chip *to_lgm_pwm_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct lgm_pwm_chip, chip);
> +}
> +
> +static int lgm_pwm_update_dc(struct lgm_pwm_chip *pc, u32 val)
> +{
> +	return regmap_update_bits(pc->regmap, PWM_FAN_CON0, PWM_FAN_DC_MSK,
> +				  FIELD_PREP(PWM_FAN_DC_MSK, val));
> +}
> +
> +static int lgm_pwm_enable(struct pwm_chip *chip, bool enable)
> +{
> +	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
> +	struct regmap *regmap = pc->regmap;
> +
> +	if (enable) {
> +		regmap_update_bits(regmap, PWM_FAN_CON0,
> +				   PWM_FAN_EN_MSK, PWM_FAN_EN_EN);
> +		if (pc->tach_en)
> +			schedule_delayed_work(&pc->work, msecs_to_jiffies(10000));
> +	} else {
> +		if (pc->tach_en)
> +			cancel_delayed_work_sync(&pc->work);
> +		regmap_update_bits(regmap, PWM_FAN_CON0,
> +			   	   PWM_FAN_EN_MSK, PWM_FAN_EN_DIS);
> +	}
> +
> +	return 0;
> +}
> +
> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
> +	struct pwm_state cur_state;
> +	u32 duty_cycle, duty, val;
> +	int ret = 0;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL ||
> +	    state->period != pc->period)

The period-check is too strict, please accept periods bigger than the
resulting value. This case however isn't handled correctly yet in the
following code and needs:

	period = min(state->period, pc->period);

	if (state->polarity != PWM_POLARITY_NORMAL ||
	    period < pc->period)
	    	return -EINVAL;

(and then use period instead of state->period in the following)

> +		return -EINVAL;
> +
> +	cur_state = pwm->state;
> +	duty_cycle = state->duty_cycle;

This would then be:

	duty_cycle = min(state->duty_cycle, period);

> +	if (!state->enabled)
> +		duty_cycle = 0;

What happens if you don't set duty_cycle to 0? Is it just to prevent a
spike in lgm_pwm_update_dc before calling lgm_pwm_enable(.., false)? If
so, what about skipping writing (and calculating) the duty register
value at all?

> +	duty = duty_cycle * (1U << DC_BITS);
> +	val = DIV_ROUND_CLOSEST(duty, state->period);

This is equivalent to:

	val = DIV_ROUND_CLOSEST(duty_cycle << DC_BITS, state->period);

which doesn't need two variables with similar name but different
scaling. Having said that using closest rounding is wrong here, please
round down.

> +	val = min_t(u32, val, MAX_DUTY_CYCLE);

Hmm, this looks suspicious. Does the hardware really produce 100% when
val = MAX_DUTY_CYCLE? Either it doesn't or there is a rounding error in
your algorithm. Does the PWM support 0%? It would help to mention the
formula from the reference manual to verify and understand your
algorithm. Please add a comment for that.

> +	if (pc->tach_en) {
> +		pc->set_dc = val;
> +		pc->set_rpm = val * pc->max_rpm / MAX_DUTY_CYCLE;
> +	}
> +
> +	ret = lgm_pwm_update_dc(pc, val);
> +
> +	if (state->enabled != cur_state.enabled)
> +		lgm_pwm_enable(chip, state->enabled);

I would prefer if you would make this call conditional on the (cached)
state of the PWM_FAN_EN_MSK bit instead of pwm->state. This way the
driver is more independent from pwm API internals.

> +	return ret;
> +}
> +
> +static void lgm_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      struct pwm_state *state)
> +{
> +	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
> +	u32 duty, val;
> +
> +	state->enabled = regmap_test_bits(pc->regmap, PWM_FAN_CON0,
> +					  PWM_FAN_EN_EN);
> +	state->polarity = PWM_POLARITY_NORMAL;
> +	state->period = pc->period; /* fixed period */
> +
> +	regmap_read(pc->regmap, PWM_FAN_CON0, &val);
> +	duty = FIELD_GET(PWM_FAN_DC_MSK, val);
> +	state->duty_cycle = duty * pc->period >> DC_BITS;

The rounding here must use the inverse rounding of .apply(). So please
round up here.

> +}
> +
> +static const struct pwm_ops lgm_pwm_ops = {
> +	.get_state = lgm_pwm_get_state,
> +	.apply = lgm_pwm_apply,
> +	.owner = THIS_MODULE,
> +};
> +
> +static void lgm_pwm_tach_work(struct work_struct *work)
> +{
> +	struct lgm_pwm_chip *pc = container_of(work, struct lgm_pwm_chip,
> +						 work.work);
> +	struct regmap *regmap = pc->regmap;
> +	u32 fan_tach, fan_dc, val;
> +	s32 diff;
> +	static u32 fanspeed_err_cnt, time_window, delta_dc;
> +
> +	/*
> +	 * Fan speed is tracked by reading the active duty cycle of PWM output
> +	 * from the active duty cycle register. Some variance in the duty cycle
> +	 * register value is expected. So we set a time window of 30 seconds and
> +	 * if we detect inaccurate fan speed 6 times within 30 seconds then we
> +	 * mark it as fan speed problem and fix it by readjusting the duty cycle.
> +	 */
> +
> +	if (fanspeed_err_cnt > IGNORE_FIRST_ERR)
> +		/*
> +		 * Ignore first time we detect inaccurate fan speed
> +		 * because it is expected during bootup.
> +		 */
> +		time_window++;
> +
> +	if (time_window == THIRTY_SECS_WINDOW) {
> +		/*
> +		 * This work is scheduled every 2 seconds i.e. each time_window
> +		 * counter step roughly mean 2 seconds. When the time window
> +		 * reaches 30 seconds, reset all the counters/logic.
> +		 */
> +		fanspeed_err_cnt = 0;
> +		delta_dc = 0;
> +		time_window = 0;
> +	}
> +
> +	regmap_read(regmap, PWM_FAN_STAT, &fan_tach);
> +	fan_tach &= PWM_FAN_TACH_MASK;
> +	if (!fan_tach)
> +		goto restart_work;
> +
> +	val = DIV_ROUND_CLOSEST(pc->set_rpm << FRAC_BITS, fan_tach);
> +	diff = val - BIT(FRAC_BITS);
> +
> +	if (abs(diff) > TWO_TENTH) {
> +		/* if duty cycle diff is more than two tenth, detect it as error */
> +		if (fanspeed_err_cnt > IGNORE_FIRST_ERR)
> +			delta_dc += val;
> +		fanspeed_err_cnt++;
> +	}
> +
> +	if (fanspeed_err_cnt == ERR_CNT_THRESHOLD) {
> +		/*
> +		 * We detected fan speed errors 6 times with 30 seconds.
> +		 * Fix the error by readjusting duty cycle and reset
> +		 * our counters/logic.
> +		 */
> +		fan_dc = pc->set_dc * delta_dc >> (FRAC_BITS + 2);
> +		fan_dc = min_t(u32, fan_dc, MAX_DUTY_CYCLE);
> +		lgm_pwm_update_dc(pc, fan_dc);
> +		fanspeed_err_cnt = 0;
> +		delta_dc = 0;
> +		time_window = 0;
> +	}
> +
> +restart_work:
> +	/*
> +	 * Fan speed doesn't need continous tracking. Schedule this work
> +	 * every two seconds so it doesn't steal too much cpu cycles.
> +	 */
> +	schedule_delayed_work(&pc->work, msecs_to_jiffies(TWO_SECONDS));

You had concerns about my review feedback that I don't like the fan
stuff in the PWM driver. I still think that the fan part doesn't belong
here.

> +}
> +
> +static void lgm_pwm_init(struct lgm_pwm_chip *pc)
> +{
> +	struct device *dev = pc->chip.dev;
> +	struct regmap *regmap = pc->regmap;
> +	u32 max_rpm, fan_wire, tach_plus, con0_val, con0_mask;
> +
> +	if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire))
> +		fan_wire = 2; /* default is 2 wire mode */
> +
> +	con0_val = FIELD_PREP(PWM_FAN_PWM_DIS_MSK, PWM_FAN_PWM_DIS_DIS);
> +	con0_mask = PWM_FAN_PWM_DIS_MSK | PWM_FAN_MODE_MSK;

Please don't disable the PWM in .probe()

> +
> +	switch (fan_wire) {
> +	case 4:
> +		con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_4WIRE) |
> +			    FIELD_PREP(PWM_TACH_EN_MSK, PWM_TACH_EN_EN);
> +		con0_mask |= PWM_TACH_EN_MSK | PWM_TACH_PLUS_MSK;
> +		pc->tach_en = 1;
> +		pc->period = PERIOD_4WIRE_NSECS;
> +		break;
> +	default:
> +		/* default is 2wire mode */
> +		con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE);
> +		pc->period = PERIOD_2WIRE_NSECS;
> +		break;
> +	}
> +
> +	if (pc->tach_en) {
> +		if (device_property_read_u32(dev, "intel,tach-plus",
> +					     &tach_plus))
> +			tach_plus = 2;
> +
> +		switch (tach_plus) {
> +		case 4:
> +			con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK,
> +					       PWM_TACH_PLUS_4);
> +			break;
> +		default:
> +			con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK,
> +					       PWM_TACH_PLUS_2);
> +			break;
> +		}
> +
> +		if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm))
> +			max_rpm = DFAULT_RPM;
> +
> +		max_rpm = min_t(u32, max_rpm, MAX_RPM);
> +		if (max_rpm == 0)
> +			max_rpm = DFAULT_RPM;
> +
> +		pc->max_rpm = max_rpm;
> +		INIT_DEFERRABLE_WORK(&pc->work, lgm_pwm_tach_work);
> +		regmap_update_bits(regmap, PWM_FAN_CON1,
> +				   PWM_FAN_MAX_RPM_MSK, max_rpm);
> +	}
> +
> +	regmap_update_bits(regmap, PWM_FAN_CON0, con0_mask, con0_val);
> +}
> +
> +static const struct regmap_config pwm_regmap_config = {

Here you missed to add the lgm_ prefix.

> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +};
> +
> +static int lgm_pwm_probe(struct platform_device *pdev)
> +{
> +	struct lgm_pwm_chip *pc;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *io_base;
> +	int ret;
> +
> +	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> +	if (!pc)
> +		return -ENOMEM;
> +
> +	io_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config);
> +	if (IS_ERR(pc->regmap)) {
> +		ret = PTR_ERR(pc->regmap);
> +		dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
> +		return ret;
> +	}
> +
> +	pc->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pc->clk)) {
> +		ret = PTR_ERR(pc->clk);
> +		dev_err(dev, "failed to get clock: %pe\n", pc->clk);
> +		return ret;
> +	}
> +
> +	pc->rst = devm_reset_control_get(dev, NULL);
> +	if (IS_ERR(pc->rst)) {
> +		ret = PTR_ERR(pc->rst);
> +		dev_err(dev, "failed to get reset control: %pe\n", pc->rst);
> +		return ret;
> +	}
> +
> +	ret = reset_control_deassert(pc->rst);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert reset control: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(pc->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	pc->chip.dev = dev;
> +	pc->chip.ops = &lgm_pwm_ops;
> +	pc->chip.npwm = 1;
> +
> +	lgm_pwm_init(pc);
> +
> +	ret = pwmchip_add(&pc->chip);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add PWM chip: %d\n", ret);

%pe please.

> +		clk_disable_unprepare(pc->clk);
> +		reset_control_assert(pc->rst);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pc);
> +	return 0;
> +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC

[Uwe Kleine-König]

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

Hello again,

On Fri, Jun 19, 2020 at 08:02:13AM +0200, Uwe Kleine-König wrote:
> You had concerns about my review feedback that I don't like the fan
> stuff in the PWM driver. I still think that the fan part doesn't belong
> here.

I suggest splitting the patch. First introduce a generic PWM driver and
in a second patch add the fan stuff. This way you can get an ack from me
at least for a part. Also it makes it more obvious that there is going
on something special and we can have a dedicated discussion about it and
a concious decision made at the end.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC

[Tanwar, Rahul]

Hi Uwe,

Thanks for your valuable feedback.

On 19/6/2020 2:02 pm, Uwe Kleine-König wrote:
> Hello Rahul,
>
> On Thu, Jun 18, 2020 at 08:05:13PM +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
>> This PWM controller does not have any other consumer, it is a
>> dedicated PWM controller for fan attached to the system. Add
>> driver for this PWM fan controller.
>>
>> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
>> ---
>>  drivers/pwm/Kconfig         |   9 +
>>  drivers/pwm/Makefile        |   1 +
>>  drivers/pwm/pwm-intel-lgm.c | 400 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 410 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-intel-lgm.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index cb8d739067d2..a3303e22d5fa 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -232,6 +232,15 @@ config PWM_IMX_TPM
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-imx-tpm.
>>  
>> +config PWM_INTEL_LGM
>> +	tristate "Intel LGM PWM support"
>> +	depends on X86 || COMPILE_TEST
>> +	help
>> +	  Generic PWM fan controller driver for LGM SoC.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called pwm-intel-lgm.
>> +
>>  config PWM_IQS620A
>>  	tristate "Azoteq IQS620A PWM support"
>>  	depends on MFD_IQS62X || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index a59c710e98c7..db154a6b4f51 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
>>  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
>>  obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
>>  obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
>> +obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
>>  obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
>>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>>  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
>> diff --git a/drivers/pwm/pwm-intel-lgm.c b/drivers/pwm/pwm-intel-lgm.c
>> new file mode 100644
>> index 000000000000..3c7077acb161
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-intel-lgm.c
>> @@ -0,0 +1,400 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 Intel Corporation.
>> + *
>> + * Notes & Limitations:
>> + * - The hardware supports fixed period which is dependent on 2/3 or 4
>> + *   wire fan mode.
>> + * - Supports normal polarity. Does not support changing polarity.
>> + * - When PWM is disabled, output of PWM will become 0(inactive). It doesn't
>> + *   keep track of running period.
>> + * - When duty cycle is changed, PWM output may be a mix of previous setting
>> + *   and new setting for the first period. From second period, the output is
>> + *   based on new setting.
>> + * - Supports 100% duty cycle.
>> + * - It is a dedicated PWM fan controller. There are no other consumers for
>> + *   this PWM controller.
>> + */
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +#define PWM_FAN_CON0		0x0
>> +#define PWM_FAN_EN_EN		BIT(0)
>> +#define PWM_FAN_EN_DIS		0x0
>> +#define PWM_FAN_EN_MSK		BIT(0)
>> +#define PWM_FAN_MODE_2WIRE	0x0
>> +#define PWM_FAN_MODE_4WIRE	0x1
>> +#define PWM_FAN_MODE_MSK	BIT(1)
>> +#define PWM_FAN_PWM_DIS_DIS	0x0
>> +#define PWM_FAN_PWM_DIS_MSK	BIT(2)
>> +#define PWM_TACH_EN_EN		0x1
>> +#define PWM_TACH_EN_MSK		BIT(4)
>> +#define PWM_TACH_PLUS_2		0x0
>> +#define PWM_TACH_PLUS_4		0x1
>> +#define PWM_TACH_PLUS_MSK	BIT(5)
>> +#define PWM_FAN_DC_MSK		GENMASK(23, 16)
>> +
>> +#define PWM_FAN_CON1		0x4
>> +#define PWM_FAN_MAX_RPM_MSK	GENMASK(15, 0)
>> +
>> +#define PWM_FAN_STAT		0x10
>> +#define PWM_FAN_TACH_MASK	GENMASK(15, 0)
>> +
>> +#define MAX_RPM			(BIT(16) - 1)
>> +#define DFAULT_RPM		4000
>> +#define MAX_DUTY_CYCLE		(BIT(8) - 1)
>> +
>> +#define FRAC_BITS		10
>> +#define DC_BITS			8
>> +#define TWO_TENTH		204
>> +
>> +#define PERIOD_2WIRE_NSECS	40000000
>> +#define PERIOD_4WIRE_NSECS	40000
>> +
>> +#define TWO_SECONDS		2000
>> +#define IGNORE_FIRST_ERR	1
>> +#define THIRTY_SECS_WINDOW	15
>> +#define ERR_CNT_THRESHOLD	6
>> +
>> +struct lgm_pwm_chip {
>> +	struct pwm_chip chip;
>> +	struct regmap *regmap;
>> +	struct clk *clk;
>> +	struct reset_control *rst;
>> +	u32 tach_en;
>> +	u32 max_rpm;
>> +	u32 set_rpm;
>> +	u32 set_dc;
>> +	u32 period;
>> +	struct delayed_work work;
>> +};
>> +
>> +static inline struct lgm_pwm_chip *to_lgm_pwm_chip(struct pwm_chip *chip)
>> +{
>> +	return container_of(chip, struct lgm_pwm_chip, chip);
>> +}
>> +
>> +static int lgm_pwm_update_dc(struct lgm_pwm_chip *pc, u32 val)
>> +{
>> +	return regmap_update_bits(pc->regmap, PWM_FAN_CON0, PWM_FAN_DC_MSK,
>> +				  FIELD_PREP(PWM_FAN_DC_MSK, val));
>> +}
>> +
>> +static int lgm_pwm_enable(struct pwm_chip *chip, bool enable)
>> +{
>> +	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
>> +	struct regmap *regmap = pc->regmap;
>> +
>> +	if (enable) {
>> +		regmap_update_bits(regmap, PWM_FAN_CON0,
>> +				   PWM_FAN_EN_MSK, PWM_FAN_EN_EN);
>> +		if (pc->tach_en)
>> +			schedule_delayed_work(&pc->work, msecs_to_jiffies(10000));
>> +	} else {
>> +		if (pc->tach_en)
>> +			cancel_delayed_work_sync(&pc->work);
>> +		regmap_update_bits(regmap, PWM_FAN_CON0,
>> +			   	   PWM_FAN_EN_MSK, PWM_FAN_EN_DIS);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			 const struct pwm_state *state)
>> +{
>> +	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
>> +	struct pwm_state cur_state;
>> +	u32 duty_cycle, duty, val;
>> +	int ret = 0;
>> +
>> +	if (state->polarity != PWM_POLARITY_NORMAL ||
>> +	    state->period != pc->period)
> The period-check is too strict, please accept periods bigger than the
> resulting value. This case however isn't handled correctly yet in the
> following code and needs:
>
> 	period = min(state->period, pc->period);
>
> 	if (state->polarity != PWM_POLARITY_NORMAL ||
> 	    period < pc->period)
> 	    	return -EINVAL;
>
> (and then use period instead of state->period in the following)
>
>> +		return -EINVAL;
>> +
>> +	cur_state = pwm->state;
>> +	duty_cycle = state->duty_cycle;
> This would then be:
>
> 	duty_cycle = min(state->duty_cycle, period);
>
>> +	if (!state->enabled)
>> +		duty_cycle = 0;
> What happens if you don't set duty_cycle to 0? Is it just to prevent a
> spike in lgm_pwm_update_dc before calling lgm_pwm_enable(.., false)? If
> so, what about skipping writing (and calculating) the duty register
> value at all?

Thanks, will update. Agree that setting duty_cycle to 0 is redundant.
Setting duty_cycle to 0 is equivalent to disabling PWM. Will remove it.

>> +	duty = duty_cycle * (1U << DC_BITS);
>> +	val = DIV_ROUND_CLOSEST(duty, state->period);
> This is equivalent to:
>
> 	val = DIV_ROUND_CLOSEST(duty_cycle << DC_BITS, state->period);
>
> which doesn't need two variables with similar name but different
> scaling. Having said that using closest rounding is wrong here, please
> round down.

Unable to find a 32 bit variant of DIV_ROUND_DOWN API unlike
DIV_ROUND_UP. Do you have any suggestion for such a API? If not,
i will add one new API for this purpose in the driver.

>> +	val = min_t(u32, val, MAX_DUTY_CYCLE);
> Hmm, this looks suspicious. Does the hardware really produce 100% when
> val = MAX_DUTY_CYCLE? Either it doesn't or there is a rounding error in
> your algorithm. Does the PWM support 0%? It would help to mention the
> formula from the reference manual to verify and understand your
> algorithm. Please add a comment for that.

Yes, hardware claims to produce 100% when val = 0xff for 8 bit duty cycle
register field. But it doesn't support 0%. Minimum supported is 20%.
Sorry i am unable to figure out your point here. Can you please elaborate
more? I was just trying to ensure that reg value doesn't exceed max value
of 0xff.

>> +	if (pc->tach_en) {
>> +		pc->set_dc = val;
>> +		pc->set_rpm = val * pc->max_rpm / MAX_DUTY_CYCLE;
>> +	}
>> +
>> +	ret = lgm_pwm_update_dc(pc, val);
>> +
>> +	if (state->enabled != cur_state.enabled)
>> +		lgm_pwm_enable(chip, state->enabled);
> I would prefer if you would make this call conditional on the (cached)
> state of the PWM_FAN_EN_MSK bit instead of pwm->state. This way the
> driver is more independent from pwm API internals.

Well noted.

>> +	return ret;
>> +}
>> +
>> +static void lgm_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			      struct pwm_state *state)
>> +{
>> +	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
>> +	u32 duty, val;
>> +
>> +	state->enabled = regmap_test_bits(pc->regmap, PWM_FAN_CON0,
>> +					  PWM_FAN_EN_EN);
>> +	state->polarity = PWM_POLARITY_NORMAL;
>> +	state->period = pc->period; /* fixed period */
>> +
>> +	regmap_read(pc->regmap, PWM_FAN_CON0, &val);
>> +	duty = FIELD_GET(PWM_FAN_DC_MSK, val);
>> +	state->duty_cycle = duty * pc->period >> DC_BITS;
> The rounding here must use the inverse rounding of .apply(). So please
> round up here.

Well noted.

>> +}
>> +
>> +static const struct pwm_ops lgm_pwm_ops = {
>> +	.get_state = lgm_pwm_get_state,
>> +	.apply = lgm_pwm_apply,
>> +	.owner = THIS_MODULE,
>> +};
>> +
>> +static void lgm_pwm_tach_work(struct work_struct *work)
>> +{
>> +	struct lgm_pwm_chip *pc = container_of(work, struct lgm_pwm_chip,
>> +						 work.work);
>> +	struct regmap *regmap = pc->regmap;
>> +	u32 fan_tach, fan_dc, val;
>> +	s32 diff;
>> +	static u32 fanspeed_err_cnt, time_window, delta_dc;
>> +
>> +	/*
>> +	 * Fan speed is tracked by reading the active duty cycle of PWM output
>> +	 * from the active duty cycle register. Some variance in the duty cycle
>> +	 * register value is expected. So we set a time window of 30 seconds and
>> +	 * if we detect inaccurate fan speed 6 times within 30 seconds then we
>> +	 * mark it as fan speed problem and fix it by readjusting the duty cycle.
>> +	 */
>> +
>> +	if (fanspeed_err_cnt > IGNORE_FIRST_ERR)
>> +		/*
>> +		 * Ignore first time we detect inaccurate fan speed
>> +		 * because it is expected during bootup.
>> +		 */
>> +		time_window++;
>> +
>> +	if (time_window == THIRTY_SECS_WINDOW) {
>> +		/*
>> +		 * This work is scheduled every 2 seconds i.e. each time_window
>> +		 * counter step roughly mean 2 seconds. When the time window
>> +		 * reaches 30 seconds, reset all the counters/logic.
>> +		 */
>> +		fanspeed_err_cnt = 0;
>> +		delta_dc = 0;
>> +		time_window = 0;
>> +	}
>> +
>> +	regmap_read(regmap, PWM_FAN_STAT, &fan_tach);
>> +	fan_tach &= PWM_FAN_TACH_MASK;
>> +	if (!fan_tach)
>> +		goto restart_work;
>> +
>> +	val = DIV_ROUND_CLOSEST(pc->set_rpm << FRAC_BITS, fan_tach);
>> +	diff = val - BIT(FRAC_BITS);
>> +
>> +	if (abs(diff) > TWO_TENTH) {
>> +		/* if duty cycle diff is more than two tenth, detect it as error */
>> +		if (fanspeed_err_cnt > IGNORE_FIRST_ERR)
>> +			delta_dc += val;
>> +		fanspeed_err_cnt++;
>> +	}
>> +
>> +	if (fanspeed_err_cnt == ERR_CNT_THRESHOLD) {
>> +		/*
>> +		 * We detected fan speed errors 6 times with 30 seconds.
>> +		 * Fix the error by readjusting duty cycle and reset
>> +		 * our counters/logic.
>> +		 */
>> +		fan_dc = pc->set_dc * delta_dc >> (FRAC_BITS + 2);
>> +		fan_dc = min_t(u32, fan_dc, MAX_DUTY_CYCLE);
>> +		lgm_pwm_update_dc(pc, fan_dc);
>> +		fanspeed_err_cnt = 0;
>> +		delta_dc = 0;
>> +		time_window = 0;
>> +	}
>> +
>> +restart_work:
>> +	/*
>> +	 * Fan speed doesn't need continous tracking. Schedule this work
>> +	 * every two seconds so it doesn't steal too much cpu cycles.
>> +	 */
>> +	schedule_delayed_work(&pc->work, msecs_to_jiffies(TWO_SECONDS));
> You had concerns about my review feedback that I don't like the fan
> stuff in the PWM driver. I still think that the fan part doesn't belong
> here.

We have decided to remove this fan calibration task after concluding that
the same can be achieved by a user space application using sysfs. Will remove
it in v3.

>> +}
>> +
>> +static void lgm_pwm_init(struct lgm_pwm_chip *pc)
>> +{
>> +	struct device *dev = pc->chip.dev;
>> +	struct regmap *regmap = pc->regmap;
>> +	u32 max_rpm, fan_wire, tach_plus, con0_val, con0_mask;
>> +
>> +	if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire))
>> +		fan_wire = 2; /* default is 2 wire mode */
>> +
>> +	con0_val = FIELD_PREP(PWM_FAN_PWM_DIS_MSK, PWM_FAN_PWM_DIS_DIS);
>> +	con0_mask = PWM_FAN_PWM_DIS_MSK | PWM_FAN_MODE_MSK;
> Please don't disable the PWM in .probe()

Well noted.

>> +
>> +	switch (fan_wire) {
>> +	case 4:
>> +		con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_4WIRE) |
>> +			    FIELD_PREP(PWM_TACH_EN_MSK, PWM_TACH_EN_EN);
>> +		con0_mask |= PWM_TACH_EN_MSK | PWM_TACH_PLUS_MSK;
>> +		pc->tach_en = 1;
>> +		pc->period = PERIOD_4WIRE_NSECS;
>> +		break;
>> +	default:
>> +		/* default is 2wire mode */
>> +		con0_val |= FIELD_PREP(PWM_FAN_MODE_MSK, PWM_FAN_MODE_2WIRE);
>> +		pc->period = PERIOD_2WIRE_NSECS;
>> +		break;
>> +	}
>> +
>> +	if (pc->tach_en) {
>> +		if (device_property_read_u32(dev, "intel,tach-plus",
>> +					     &tach_plus))
>> +			tach_plus = 2;
>> +
>> +		switch (tach_plus) {
>> +		case 4:
>> +			con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK,
>> +					       PWM_TACH_PLUS_4);
>> +			break;
>> +		default:
>> +			con0_val |= FIELD_PREP(PWM_TACH_PLUS_MSK,
>> +					       PWM_TACH_PLUS_2);
>> +			break;
>> +		}
>> +
>> +		if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm))
>> +			max_rpm = DFAULT_RPM;
>> +
>> +		max_rpm = min_t(u32, max_rpm, MAX_RPM);
>> +		if (max_rpm == 0)
>> +			max_rpm = DFAULT_RPM;
>> +
>> +		pc->max_rpm = max_rpm;
>> +		INIT_DEFERRABLE_WORK(&pc->work, lgm_pwm_tach_work);
>> +		regmap_update_bits(regmap, PWM_FAN_CON1,
>> +				   PWM_FAN_MAX_RPM_MSK, max_rpm);
>> +	}
>> +
>> +	regmap_update_bits(regmap, PWM_FAN_CON0, con0_mask, con0_val);
>> +}
>> +
>> +static const struct regmap_config pwm_regmap_config = {
> Here you missed to add the lgm_ prefix.
>
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 32,
>> +};
>> +
>> +static int lgm_pwm_probe(struct platform_device *pdev)
>> +{
>> +	struct lgm_pwm_chip *pc;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *io_base;
>> +	int ret;
>> +
>> +	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
>> +	if (!pc)
>> +		return -ENOMEM;
>> +
>> +	io_base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(io_base))
>> +		return PTR_ERR(io_base);
>> +
>> +	pc->regmap = devm_regmap_init_mmio(dev, io_base, &pwm_regmap_config);
>> +	if (IS_ERR(pc->regmap)) {
>> +		ret = PTR_ERR(pc->regmap);
>> +		dev_err(dev, "failed to init register map: %pe\n", pc->regmap);
>> +		return ret;
>> +	}
>> +
>> +	pc->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(pc->clk)) {
>> +		ret = PTR_ERR(pc->clk);
>> +		dev_err(dev, "failed to get clock: %pe\n", pc->clk);
>> +		return ret;
>> +	}
>> +
>> +	pc->rst = devm_reset_control_get(dev, NULL);
>> +	if (IS_ERR(pc->rst)) {
>> +		ret = PTR_ERR(pc->rst);
>> +		dev_err(dev, "failed to get reset control: %pe\n", pc->rst);
>> +		return ret;
>> +	}
>> +
>> +	ret = reset_control_deassert(pc->rst);
>> +	if (ret) {
>> +		dev_err(dev, "cannot deassert reset control: %pe\n",
>> +			ERR_PTR(ret));
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(pc->clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable clock\n");
>> +		return ret;
>> +	}
>> +
>> +	pc->chip.dev = dev;
>> +	pc->chip.ops = &lgm_pwm_ops;
>> +	pc->chip.npwm = 1;
>> +
>> +	lgm_pwm_init(pc);
>> +
>> +	ret = pwmchip_add(&pc->chip);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to add PWM chip: %d\n", ret);
> %pe please.

Well noted. Thanks.

Regards,
Rahul