pwm: intel: Add PWM driver for a new SoC
Subsystems (2)
v10 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.
v10:
- Removed unused of_device.h and added platform_device.h & mod_devicetable.h
v9:
- Address code quality related review concerns (Andy Shevchenko)
- Use devm_add_action_or_reset() instead of explicit unwind calls.
v8:
- Remove fan related optional properties usage, keep them as default. If needed, change pwm-fan driver separately in future to add them as generic properties.
v7:
- Address code quality related review concerns.
- Rename fan related property to pwm-*.
- Fix one make dt_binding_check reported error.
v6:
- Readjust .apply op as per review feedback.
- Add back pwm-cells property to resolve make dt_binding_check error. pwm-cells is a required property for PWM driver.
- Add back fan related optional properties.
v5:
- Address below review concerns from Uwe Kleine-K�nig.
* Improve comments about Limitations.
* Use return value of regmap_update_bits if container function returns error code.
* Modify .apply op to have strict checking for fixed period supported by PWM HW.
* Use u64 as type when use min_t for duty_cycle.
* Add reset_control_assert() in failure case in probe where it was missing earlier.
- Remove fan specific optional properties from pwm dt binding document (Rob Herring)
v4:
- Address below review concerns from Uwe Kleine-K�nig.
* Improve notes and limitations comments.
* Add common prefixes for all #defines.
* Modify/Improve logic in .apply & .get_state ops as advised.
* Skip error messages in probe when error is -EPROBE_DEFER.
* Add dependencies in Kconfig (OF & HAS_IOMEM) and add select REGMAP_MMIO.
* Address other code quality related review concerns.
- Fix make dt_binding_check reported error in YAML file.
v3:
- Address below review concerns from Uwe Kleine-K�nig.
* Remove fan rpm calibration task from the driver.
* Modify apply op as per the review feedback.
* Add roundup & round down where necessary.
* Address other misc code quality related review concerns.
* Use devm_reset_control_get_exclusive(). (Philipp Zabel)
* Improve dt binding document.
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 | 44 ++++ drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 242 +++++++++++++++++++++ 4 files changed, 298 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 v10 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 | 44 ++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml new file +44 -0
@@ -0,0 +1,44 @@ +# 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 + +required: + - compatible + - reg + - 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
PATCH v10 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 | 11 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 242 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 254 insertions(+) create mode 100644 drivers/pwm/pwm-intel-lgm.c
drivers/pwm/Kconfig +11 -0
@@ -232,6 +232,17 @@ 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 HAS_IOMEM + depends on (OF && X86) || COMPILE_TEST + select REGMAP_MMIO + 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 +242 -0
242 lines not on this page · raw message
Signature · 1 line
2.11.0
Re: [PATCH v10 2/2] Add PWM fan controller driver for LGM SoC
Quoted 4 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.
...
Quoted 20 lines
drivers/pwm/pwm-intel-lgm.c new file +20
-0+ pc->regmap =devm_regmap_init_mmio (dev, io_base, &lgm_pwm_regmap_config); +if (IS_ERR (pc->regmap)) { + ret =PTR_ERR (pc->regmap); +if (ret != -EPROBE_DEFER ) +dev_err_probe (dev, ret,"failed to init register map\n" ); +return ret; + } + + pc->clk =devm_clk_get (dev,NULL ); +if (IS_ERR (pc->clk)) { + ret =PTR_ERR (pc->clk); +if (ret != -EPROBE_DEFER ) +dev_err_probe (dev, ret,"failed to get clock\n" ); +return ret; + } + + pc->rst =devm_reset_control_get_exclusive (dev,NULL ); +if (IS_ERR (pc->rst)) { + ret =PTR_ERR (pc->rst); +if (ret != -EPROBE_DEFER )
drivers/pwm/pwm-intel-lgm.c line 188
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+dev_err_probe (dev, ret,"failed to get reset control\n" ); +return ret; + } + + ret =reset_control_deassert (pc->rst); +if (ret) { +if (ret != -EPROBE_DEFER ) +dev_err_probe (dev, ret,"cannot deassert reset control\n" ); +return ret; + }
Please, spend a bit of time to understand the changes you are doing. There are already few examples how to use dev_err_probe() properly.
drivers/pwm/pwm-intel-lgm.c line 198
Quoted 9 lines
drivers/pwm/pwm-intel-lgm.c new file +9
-0+ ret =clk_prepare_enable (pc->clk); +if (ret) { +dev_err (dev,"failed to enable clock\n" ); +return ret; + } + + ret =devm_add_action_or_reset (dev, lgm_pwm_action, pc); +if (ret) +return ret;
You have also ordering issues here.
So, what I can see about implementation is that
static void ..._clk_disable(void *data)
{
clk_disable_unprepare(data);
}
static int ..._clk_enable(...)
{
int ret;
ret = clk_preare_enable(...);
if (ret)
return ret;
return devm_add_action_or_reset(..., ..._clk_disable);
}Similar for reset control.
Then in the ->probe() something like this:
ret = devm_reset_control_get...; if (ret) return ret; ret = ..._reset_deassert(...); if (ret) return ret;
followed by similar section for the clock.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v10 2/2] Add PWM fan controller driver for LGM SoC
Hi Andy,
Quoted 31 lines · Andy Shevchenko
Quoted 4 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.
...
Quoted 24 lines
drivers/pwm/pwm-intel-lgm.c new file +24
-0+ pc->regmap =devm_regmap_init_mmio (dev, io_base, &lgm_pwm_regmap_config); +if (IS_ERR (pc->regmap)) { + ret =PTR_ERR (pc->regmap); +if (ret != -EPROBE_DEFER ) +dev_err_probe (dev, ret,"failed to init register map\n" ); +return ret; + } + + pc->clk =devm_clk_get (dev,NULL ); +if (IS_ERR (pc->clk)) { + ret =PTR_ERR (pc->clk); +if (ret != -EPROBE_DEFER ) +dev_err_probe (dev, ret,"failed to get clock\n" ); +return ret; + } + + pc->rst =devm_reset_control_get_exclusive (dev,NULL ); +if (IS_ERR (pc->rst)) { + ret =PTR_ERR (pc->rst); +if (ret != -EPROBE_DEFER ) +dev_err_probe (dev, ret,"failed to get reset control\n" ); +return ret; + } +
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 188
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +6
-0+ ret =reset_control_deassert (pc->rst); +if (ret) { +if (ret != -EPROBE_DEFER ) +dev_err_probe (dev, ret,"cannot deassert reset control\n" ); +return ret; + }Please, spend a bit of time to understand the changes you are doing. There are already few examples how to use
dev_err_probe()properly.
I guess your point is that the check of (ret !- -EPROBE_DEFER) is not needed when using dev_err_probe() as it encapsulates it. Sorry, i missed it. Will fix it. I am not able to find any other missing point after referring to two driver examples which uses dev_err_probe() ?
Quoted 42 lines
drivers/pwm/pwm-intel-lgm.c line 198
Quoted 9 lines
drivers/pwm/pwm-intel-lgm.c new file +9
-0+ ret =clk_prepare_enable (pc->clk); +if (ret) { +dev_err (dev,"failed to enable clock\n" ); +return ret; + } + + ret =devm_add_action_or_reset (dev, lgm_pwm_action, pc); +if (ret) +return ret;You have also ordering issues here.
So, what I can see about implementation is that
static void ..._clk_disable(void *data) { clk_disable_unprepare(data); } static int ..._clk_enable(...) { int ret; ret = clk_preare_enable(...); if (ret) return ret; return devm_add_action_or_reset(..., ..._clk_disable); }Similar for reset control.
Then in the ->probe() something like this:
ret = devm_reset_control_get...; if (ret) return ret; ret = ..._reset_deassert(...); if (ret) return ret;
Quoted 2 lines
followed by similar section for the clock.
Regarding ordering: In early rounds of review, feedback about ordering was that it is recommended to be reverse of the sequence in probe i.e.
if in probe:
1. reset_control_deassert()
2. clk_prepare_enable()
then in remove:
1. clk_disable_uprepare()
2. reset_control_assert()
That's the reason i added a generic action() which reverses order.
I understand your suggested way as explained above but not sure if that would ensure reverse ordering during unwind.
Thanks.
Regards,
Rahul
Re: [PATCH v10 2/2] Add PWM fan controller driver for LGM SoC
Quoted 2 lines · Rahul
Quoted 1 line · Andy Shevchenko
On Mon, Aug 24, 2020 at 11:36:37AM +0800, Rahul Tanwar wrote:
...
Quoted 11 lines
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 188
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +6
-0+ ret =reset_control_deassert (pc->rst); +if (ret) { +if (ret != -EPROBE_DEFER ) +dev_err_probe (dev, ret,"cannot deassert reset control\n" ); +return ret; + }Please, spend a bit of time to understand the changes you are doing. There are already few examples how to use
dev_err_probe()properly.I guess your point is that the check of (ret !- -EPROBE_DEFER) is not needed when using
dev_err_probe()as it encapsulates it.
It does even more. Look at the existing examples.
Quoted 3 lines
Sorry, i missed it. Will
fix it. I am not able to find any other missing point after referring to two driver examples which usesdev_err_probe()?
There are three drivers that are using it in Linux Next. All of them utilizing it correctly, look at them.
Quoted 54 lines
Quoted 42 lines
drivers/pwm/pwm-intel-lgm.c line 198
Quoted 9 lines
drivers/pwm/pwm-intel-lgm.c new file +9
-0+ ret =clk_prepare_enable (pc->clk); +if (ret) { +dev_err (dev,"failed to enable clock\n" ); +return ret; + } + + ret =devm_add_action_or_reset (dev, lgm_pwm_action, pc); +if (ret) +return ret;You have also ordering issues here.
So, what I can see about implementation is that
static void ..._clk_disable(void *data) { clk_disable_unprepare(data); } static int ..._clk_enable(...) { int ret; ret = clk_preare_enable(...); if (ret) return ret; return devm_add_action_or_reset(..., ..._clk_disable); }Similar for reset control.
Then in the ->probe() something like this:
ret = devm_reset_control_get...; if (ret) return ret; ret = ..._reset_deassert(...); if (ret) return ret;Quoted 2 lines
followed by similar section for the clock.
Regarding ordering: In early rounds of review, feedback about ordering was that it is recommended to be reverse of the sequence in probe i.e.
if in probe:
1.reset_control_deassert()
2.clk_prepare_enable()
then in remove:
1.clk_disable_uprepare()
2.reset_control_assert()
Quoted 1 line
That's the reason i added a generic
action()which reverses order.
Yes, and my suggestion follows this.
Quoted 2 lines
I understand your suggested way as explained above but not sure if that would ensure reverse ordering during unwind.
You have:
devm r1
devm r2
enable r1
enable r2 (and here you have broken error path)
My suggestion has it like this (and no broken error path):
devm r1
enable r1
devm r2
enable r2
Signature · 2 lines
With Best Regards, Andy Shevchenko