pwm: intel: Add PWM driver for a new SoC
Subsystems (2)
v2 cover pwm: intel: Add PWM driver for a new SoC
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
Signature · 1 line
2.11.0
PATCH v2 1/2 Add DT bindings YAML schema for PWM fan controller of LGM SoC
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.
--- .../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 57 ++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml new file +57 -0
@@ -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 <[email protected]> + +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>; + };
Signature · 1 line
2.11.0
Re: [PATCH v2 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC
Hello,
Quoted 37 lines · 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 <[email protected]>--- .../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 57 ++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yamlDocumentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml new file +20
-0@@ -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 <[email protected]> + +properties: + compatible: + const: intel,lgm-pwm + + reg: + maxItems: 1 + + "#pwm-cells": + const: 2
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml line 30
Quoted 10 lines
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml new file +10
-0+ + 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.
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml line 34
Quoted 4 lines
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml new file +4
-0+ + intel,tach-plus: + $ref: '/schemas/types.yaml#/definitions/uint32' + description: Specifies fan tach pulse periods
Only used with fan-wire = 4, default = 2.
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml line 38
Quoted 4 lines
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml new file +4
-0+ + 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.
Quoted 8 lines
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml new file +8
-0+required: + - compatible + - reg + - "#pwm-cells" + - clocks + - resets + +additionalProperties: false
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml line 57
Quoted 10 lines
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml new file +10
-0+ +examples: + - | + pwm: pwm@e0d00000 { + compatible = "intel,lgm-pwm"; + reg = <0xe0d00000 0x30>; + #pwm-cells = <2>; + clocks = <&cgu0 126>; + resets = <&rcu0 0x30 21>; + };
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
PATCH v2 2/2 Add PWM fan controller driver for LGM SoC
In reply to Rahul TanwarIntel 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.
--- 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
drivers/pwm/Kconfig +9 -0
@@ -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
drivers/pwm/Makefile +1 -0
@@ -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
drivers/pwm/pwm-intel-lgm.c new file +400 -0
400 lines not on this page · raw message
Signature · 1 line
2.11.0
Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC
Hi Rahul,
Quoted 13 lines · 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 <[email protected]>--- 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
[...]
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +0
-0@@ -0,0 +1,400 @@
[...]
Quoted 25 lines
drivers/pwm/pwm-intel-lgm.c new file +25
-0+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);
drivers/pwm/pwm-intel-lgm.c line 335
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+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
Hi Philipp,
Quoted 56 lines · Philipp Zabel
Hi Rahul,
Quoted 13 lines · 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 <[email protected]>--- 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[...]
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +0
-0@@ -0,0 +1,400 @@[...]
Quoted 32 lines
drivers/pwm/pwm-intel-lgm.c new file +32
-0+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);
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 335
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+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 usedevm_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
Quoted 69 lines · Rahul
Hi Philipp,
Quoted 56 lines · Philipp Zabel
Hi Rahul,
Quoted 13 lines · 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 <[email protected]>--- 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[...]
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +0
-0@@ -0,0 +1,400 @@[...]
Quoted 32 lines
drivers/pwm/pwm-intel-lgm.c new file +32
-0+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);Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 335
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+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 usedevm_reset_control_get_shared()to potentially allow sharing a reset line with other devices.
Quoted 8 lines
devm_reset_control_get()is a wrapper fordevm_reset_control_get_exclusive(). Code as below:
static inline structreset_control*devm_reset_control_get( struct device *dev, const char *id) {
returndevm_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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC
Quoted 76 lines · Uwe Kleine-König
Quoted 67 lines · Rahul
Hi Philipp,
Quoted 56 lines · Philipp Zabel
Hi Rahul,
Quoted 13 lines · 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.
--- 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[...]
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +0
-0@@ -0,0 +1,400 @@[...]
Quoted 32 lines
drivers/pwm/pwm-intel-lgm.c new file +32
-0+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);Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 335
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+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 usedevm_reset_control_get_shared()to potentially allow sharing a reset line with other devices.Quoted 8 lines
devm_reset_control_get()is a wrapper fordevm_reset_control_get_exclusive(). Code as below:
static inline structreset_control*devm_reset_control_get( struct device *dev, const char *id) {
returndevm_reset_control_get_exclusive(dev, id);
}
Am i missing something else?
Quoted 2 lines
Obviously you're missing the comment above
of_reset_control_getabout 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
In reply to Rahul TanwarHello Rahul,
Quoted 158 lines · 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 <[email protected]>--- 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.cdrivers/pwm/Kconfig +9
-0@@ -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_TESTdrivers/pwm/Makefile +1
-0@@ -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.odrivers/pwm/pwm-intel-lgm.c new file +107
-0@@ -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 ; +} +
drivers/pwm/pwm-intel-lgm.c line 117
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-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)
drivers/pwm/pwm-intel-lgm.c line 121
Quoted 4 lines
drivers/pwm/pwm-intel-lgm.c new file +4
-0+return -EINVAL ; + + cur_state = pwm->state; + duty_cycle = state->duty_cycle;
This would then be:
duty_cycle = min(state->duty_cycle, period);
drivers/pwm/pwm-intel-lgm.c line 123
Quoted 2 lines
drivers/pwm/pwm-intel-lgm.c new file +2
-0+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?
drivers/pwm/pwm-intel-lgm.c line 126
Quoted 2 lines
drivers/pwm/pwm-intel-lgm.c new file +2
-0+ 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.
drivers/pwm/pwm-intel-lgm.c line 127
Quoted 1 line
drivers/pwm/pwm-intel-lgm.c new file +1
-0+ 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.
drivers/pwm/pwm-intel-lgm.c line 137
Quoted 9 lines
drivers/pwm/pwm-intel-lgm.c new file +9
-0+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.
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+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);
drivers/pwm/pwm-intel-lgm.c line 155
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+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.
Quoted 68 lines
drivers/pwm/pwm-intel-lgm.c new file +68
-0+} + +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 ;
drivers/pwm/pwm-intel-lgm.c line 233
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-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.
drivers/pwm/pwm-intel-lgm.c line 246
Quoted 13 lines
drivers/pwm/pwm-intel-lgm.c new file +13
-0+} + +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()
Quoted 39 lines
drivers/pwm/pwm-intel-lgm.c new file +39
-0+ +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 ; +
drivers/pwm/pwm-intel-lgm.c line 295
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+ 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.
Quoted 53 lines
drivers/pwm/pwm-intel-lgm.c new file +53
-0+ .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; + }
drivers/pwm/pwm-intel-lgm.c line 358
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+ + 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.
drivers/pwm/pwm-intel-lgm.c line 366
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c new file +8
-0+clk_disable_unprepare (pc->clk); +reset_control_assert (pc->rst); +return ret; + } + +platform_set_drvdata (pdev, pc); +return 0 ; +}
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC
Hello again,
Quoted 3 lines · Uwe Kleine-König
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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v2 2/2] Add PWM fan controller driver for LGM SoC
In reply to Uwe Kleine-KönigHi Uwe,
Thanks for your valuable feedback.
Quoted 189 lines · Uwe Kleine-König
Hello Rahul,
Quoted 158 lines · 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.
--- 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.cdrivers/pwm/Kconfig +9
-0@@ -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_TESTdrivers/pwm/Makefile +1
-0@@ -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.odrivers/pwm/pwm-intel-lgm.c new file +107
-0@@ -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 ; +} +drivers/pwm/pwm-intel-lgm.c line 117
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-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->periodin the following)drivers/pwm/pwm-intel-lgm.c line 121
Quoted 4 lines
drivers/pwm/pwm-intel-lgm.c new file +4
-0+return -EINVAL ; + + cur_state = pwm->state; + duty_cycle = state->duty_cycle;This would then be:
Quoted 8 lines
duty_cycle = min(state->duty_cycle, period);drivers/pwm/pwm-intel-lgm.c line 123
Quoted 2 lines
drivers/pwm/pwm-intel-lgm.c new file +2
-0+if (!state->enabled) + duty_cycle =0 ;What happens if you don't set
duty_cycleto 0? Is it just to prevent a spike inlgm_pwm_update_dcbefore callinglgm_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.
Quoted 9 lines
drivers/pwm/pwm-intel-lgm.c line 126
Quoted 2 lines
drivers/pwm/pwm-intel-lgm.c new file +2
-0+ 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.
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c line 127
Quoted 1 line
drivers/pwm/pwm-intel-lgm.c new file +1
-0+ 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.
Quoted 4 lines
Quoted 4 lines
drivers/pwm/pwm-intel-lgm.c new file +4
-0+if (pc->tach_en) { + pc->set_dc = val; + pc->set_rpm = val * pc->max_rpm /MAX_DUTY_CYCLE ; + }
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 137
Quoted 5 lines
drivers/pwm/pwm-intel-lgm.c new file +5
-0+ + 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_MSKbit instead ofpwm->state. This way the driver is more independent from pwm API internals.
Well noted.
Quoted 11 lines
Quoted 11 lines
drivers/pwm/pwm-intel-lgm.c new file +11
-0+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 );
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 155
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +6
-0+ 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.
Quoted 73 lines
Quoted 73 lines
drivers/pwm/pwm-intel-lgm.c new file +73
-0+} + +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:
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 233
Quoted 5 lines
drivers/pwm/pwm-intel-lgm.c new file +5
-0+/* +* 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.
Quoted 6 lines
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +6
-0+} + +static void lgm_pwm_init (struct lgm_pwm_chip *pc) +{ +struct device *dev = pc->chip.dev; +struct regmap *regmap = pc->regmap;
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 246
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+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.
Quoted 107 lines
Quoted 39 lines
drivers/pwm/pwm-intel-lgm.c new file +39
-0+ +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 ; +drivers/pwm/pwm-intel-lgm.c line 295
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+ 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.Quoted 56 lines
drivers/pwm/pwm-intel-lgm.c new file +56
-0+ .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;
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 358
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+ 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