Add DT bindings YAML schema for PWM fan controller of LGM SoC
Subsystems (2)
PATCH v13 1/2 Add DT bindings YAML schema for PWM fan controller of LGM SoC
In reply to a message not in the archiveIntel'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 v13 2/2 Add PWM fan controller driver for LGM SoC
In reply to a message not in the archiveIntel 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 | 246 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 258 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 +246 -0
246 lines not on this page · raw message
Signature · 1 line
2.11.0
Thierry Reding
AppliedRe: [PATCH v13 0/2] pwm: intel: Add PWM driver for a new SoC
In reply to a message not in the archiveQuoted 93 lines · Rahul Tanwar
Patch 1 adds dt binding document in YAML format. Patch 2 add PWM fan controller driver for LGM SoC.
v13:
- Address below review concerns (Philipp Zabel)
* Make unnecessary 2 line comment as 1 line comment.
* Movereset_deassertat the last afterclk_enable.
* Remove unnecessary return ret statement from .remove()
- Moveplatform_set_drvdata()at the top of probe.v12:
- Rebase to linux 5.9-rc4
- Add Reviewed-by tags from Andy Shevchenko & Rob Herring.v11:
- Address below review concerns (Andy Shevchenko)
* Fix a issue withdev_err_probe()usage & improve the usage.
* Fix & improve a ordering issue with clk_enable/disable &reset_controlassert/deassert.v10:
- Removed unusedof_device.hand addedplatform_device.h&mod_devicetable.hv9:
- Address code quality related review concerns (Andy Shevchenko)
- Usedevm_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 makedt_binding_checkreported error.v6:
- Readjust .apply op as per review feedback.
- Add back pwm-cells property to resolve makedt_binding_checkerror. 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 ofregmap_update_bitsif container function returns error code.
* Modify .apply op to have strict checking for fixed period supported by PWM HW.
* Use u64 as type when usemin_tforduty_cycle.
* Addreset_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 inKconfig(OF &HAS_IOMEM) and add selectREGMAP_MMIO.
* Address other code quality related review concerns.
- Fix makedt_binding_checkreported 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.
* Usedevm_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
Quoted 7 lines
.../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 44 ++++ drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 246 +++++++++++++++++++++ 4 files changed, 302 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml create mode 100644 drivers/pwm/pwm-intel-lgm.c
Applied, thanks.
Thierry
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
In reply to Rahul TanwarHello,
(hhm Thierry already announced to have taken this patch, so my review is late.)
Quoted 54 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]>Reviewed-by Andy Shevchenko <[email protected]>--- drivers/pwm/Kconfig | 11 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 246 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 258 insertions(+) create mode 100644 drivers/pwm/pwm-intel-lgm.cdrivers/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_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 +0
-0
drivers/pwm/pwm-intel-lgm.c line 7
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0@@ -0,0 +1,246 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (C) 2020 Intel Corporation. +* +* Limitations: +* - The hardware supports fixed period which is dependent on 2/3 or 4 +* wire fan mode.
The driver now hardcodes 2-wire mode. IMHO that is worth mentioning.
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+static void lgm_clk_disable (void *data) +{ +struct lgm_pwm_chip *pc = data; + +clk_disable_unprepare (pc->clk); +} +
drivers/pwm/pwm-intel-lgm.c line 149
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+static int lgm_clk_enable (struct device *dev,struct lgm_pwm_chip *pc) +{ +int ret; + + ret =clk_prepare_enable (pc->clk); +if (ret) +return ret; + +return devm_add_action_or_reset (dev, lgm_clk_disable, pc); +}
My first reflex here was to point out that lgm_clk_disable() isn't the counter part to lgm_clk_enable() and so lgm_clk_disable() needs adaption. On a second look this is correct and so I think the function names are wrong. The usual naming would be to use _release instead of _disable. Having said that the enable function could be named devm_clk_enable and live in drivers/clk/clk-devres.c. (Or devm_clk_get_enabled()?)
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+static void lgm_reset_control_assert (void *data) +{ +struct lgm_pwm_chip *pc = data; + +reset_control_assert (pc->rst); +} +
drivers/pwm/pwm-intel-lgm.c line 167
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+static int lgm_reset_control_deassert (struct device *dev,struct lgm_pwm_chip *pc) +{ +int ret; + + ret =reset_control_deassert (pc->rst); +if (ret) +return ret; + +return devm_add_action_or_reset (dev, lgm_reset_control_assert, pc); +}
A similar comment applies here.
Quoted 19 lines
drivers/pwm/pwm-intel-lgm.c new file +19
-0+static int lgm_pwm_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct lgm_pwm_chip *pc; +void __iomem *io_base; +int ret; + + pc =devm_kzalloc (dev,sizeof (*pc),GFP_KERNEL ); +if (!pc) +return -ENOMEM ; + +platform_set_drvdata (pdev, pc); + + 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, &lgm_pwm_regmap_config); +if (IS_ERR (pc->regmap))
drivers/pwm/pwm-intel-lgm.c line 197
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+return dev_err_probe (dev,PTR_ERR (pc->regmap), +"failed to init register map\n" ); + + pc->clk =devm_clk_get (dev,NULL ); +if (IS_ERR (pc->clk)) +return dev_err_probe (dev,PTR_ERR (pc->clk),"failed to get clock\n" ); + + ret =lgm_clk_enable (dev, pc); +if (ret) { +dev_err (dev,"failed to enable clock\n" );
You used dev_err_probe four times for six error paths. I wonder why you didn't use it here (and below for a failing pwmchip_add()).
drivers/pwm/pwm-intel-lgm.c line 208
Quoted 11 lines
drivers/pwm/pwm-intel-lgm.c new file +11
-0+return ret; + } + + pc->rst =devm_reset_control_get_exclusive (dev,NULL ); +if (IS_ERR (pc->rst)) +return dev_err_probe (dev,PTR_ERR (pc->rst), +"failed to get reset control\n" ); + + ret =lgm_reset_control_deassert (dev, pc); +if (ret) +return dev_err_probe (dev, ret,"cannot deassert reset control\n" );
After lgm_reset_control_deassert is called pc->rst is unused. So there is no need to have this member in struct lgm_pwm_chip. The same applies to ->clk. (You have to pass rst (or clk) to devm_add_action_or_reset for that to work. Looks like a nice idea anyhow.)
drivers/pwm/pwm-intel-lgm.c line 212
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+ pc->chip.dev = dev; + pc->chip.ops = &lgm_pwm_ops; + pc->chip.npwm =1 ;
pc->chip.base should probably be set to -1.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
Quoted 4 lines · Uwe Kleine-König
Hello,
(hhm Thierry already announced to have taken this patch, so my review is late.)
There's also a build warning in linux-next caused by this patch, so I'm going to back it out.
Rahul, please address Uwe's comments and make sure to fix the build warning as well.
Thierry
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
Quoted 10 lines · Thierry Reding
Quoted 4 lines · Uwe Kleine-König
Hello,
(hhm Thierry already announced to have taken this patch, so my review is late.)
There's also a build warning in linux-next caused by this patch, so I'm going to back it out.
Rahul, please address Uwe's comments and make sure to fix the build warning as well.
Well noted, will do.
Regards,
Rahul
Quoted 1 line
Thierry
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
In reply to Uwe Kleine-KönigQuoted 1 line · Uwe Kleine-König
On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote:
...
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c line 197
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+ ret =lgm_clk_enable (dev, pc); +if (ret) { +dev_err (dev,"failed to enable clock\n" );You used
dev_err_probefour times for six error paths. I wonder why you didn't use it here (and below for a failingpwmchip_add()).
dev_err_probe() makes sense when we might experience deferred probe. In neither of mentioned function this can be the case.
Quoted 2 lines
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }
...
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 208
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+ ret =lgm_reset_control_deassert (dev, pc); +if (ret) +return dev_err_probe (dev, ret,"cannot deassert reset control\n" );After
lgm_reset_control_deassertis calledpc->rstis unused. So there is no need to have this member in structlgm_pwm_chip. The same applies to ->clk. (You have to pass rst (or clk) todevm_add_action_or_resetfor that to work. Looks like a nice idea anyhow.)
True. And above dev_err_probe() is not needed.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
Quoted 29 lines · Andy Shevchenko
Quoted 1 line · Uwe Kleine-König
On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote:
...
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c line 197
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+ ret =lgm_clk_enable (dev, pc); +if (ret) { +dev_err (dev,"failed to enable clock\n" );You used
dev_err_probefour times for six error paths. I wonder why you didn't use it here (and below for a failingpwmchip_add()).
dev_err_probe()makes sense when we might experience deferred probe. In neither of mentioned function this can be the case.Quoted 2 lines
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }...
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 208
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+ ret =lgm_reset_control_deassert (dev, pc); +if (ret) +return dev_err_probe (dev, ret,"cannot deassert reset control\n" );After
lgm_reset_control_deassertis calledpc->rstis unused. So there is no need to have this member in structlgm_pwm_chip. The same applies to ->clk. (You have to pass rst (or clk) todevm_add_action_or_resetfor that to work. Looks like a nice idea anyhow.)
Quoted 1 line
True. And above
dev_err_probe()is not needed.
You argue that dev_err_probe() gives no benefit as lgm_reset_control_deassert won't return -EPROBE_DEFER, right?
Still I consider it a useful function because
a) I (as an author or as a reviewer) don't need to think if the failing function might return -EPROBE_DEFER now or in the future. dev_err_probe does the right thing even for functions that don't return -EPROBE_DEFER.
b) With dev_err_probe() I can accomplish things in a single line that need two lines when open coding it.
c) dev_err_probe() emits the symbolic error name without having to resort to %pe + ERR_PTR.
d) Using dev_err_probe() for all error paths gives a consistency that I like with a maintainer's hat on.
So I still want to request using dev_err_probe() in all error paths.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
In reply to Uwe Kleine-KönigHi Uwe,
Thanks for review & feedback.
Quoted 60 lines · Uwe Kleine-König
Hello,
(hhm Thierry already announced to have taken this patch, so my review is late.)
Quoted 54 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]>Reviewed-by Andy Shevchenko <[email protected]>--- drivers/pwm/Kconfig | 11 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 246 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 258 insertions(+) create mode 100644 drivers/pwm/pwm-intel-lgm.cdrivers/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_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 +0
-0@@ -0,0 +1,246 @@
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 7
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (C) 2020 Intel Corporation. +* +* Limitations: +* - The hardware supports fixed period which is dependent on 2/3 or 4 +* wire fan mode. The driver now hardcodes 2-wire mode. IMHO that is worth mentioning.
Will update.
Quoted 16 lines
Quoted 16 lines
drivers/pwm/pwm-intel-lgm.c new file +16
-0+static void lgm_clk_disable (void *data) +{ +struct lgm_pwm_chip *pc = data; + +clk_disable_unprepare (pc->clk); +} + +static int lgm_clk_enable (struct device *dev,struct lgm_pwm_chip *pc) +{ +int ret; + + ret =clk_prepare_enable (pc->clk); +if (ret) +return ret; + +return devm_add_action_or_reset (dev, lgm_clk_disable, pc);
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 24
Quoted 1 line
drivers/pwm/pwm-intel-lgm.c new file +1
-0+}My first reflex here was to point out that
lgm_clk_disable()isn't the counter part tolgm_clk_enable()and solgm_clk_disable()needs adaption. On a second look this is correct and so I think the function names are wrong. The usual naming would be to use_releaseinstead of_disable. Having said that the enable function could be nameddevm_clk_enableand live indrivers/clk/clk-devres.c. (Ordevm_clk_get_enabled()?)
Will change function name from _disable to _release. But i think addition of a generic devm_clk_enable in drivers/clk/clk-devres.c can be taken as a separate task in a future patch (not clubbed with this driver)..
Quoted 42 lines
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+static void lgm_reset_control_assert (void *data) +{ +struct lgm_pwm_chip *pc = data; + +reset_control_assert (pc->rst); +} +drivers/pwm/pwm-intel-lgm.c line 167
Quoted 10 lines
drivers/pwm/pwm-intel-lgm.c new file +10
-0+static int lgm_reset_control_deassert (struct device *dev,struct lgm_pwm_chip *pc) +{ +int ret; + + ret =reset_control_deassert (pc->rst); +if (ret) +return ret; + +return devm_add_action_or_reset (dev, lgm_reset_control_assert, pc); +}A similar comment applies here.
Quoted 23 lines
drivers/pwm/pwm-intel-lgm.c new file +23
-0+static int lgm_pwm_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct lgm_pwm_chip *pc; +void __iomem *io_base; +int ret; + + pc =devm_kzalloc (dev,sizeof (*pc),GFP_KERNEL ); +if (!pc) +return -ENOMEM ; + +platform_set_drvdata (pdev, pc); + + 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, &lgm_pwm_regmap_config); +if (IS_ERR (pc->regmap)) +return dev_err_probe (dev,PTR_ERR (pc->regmap), +"failed to init register map\n" ); + + pc->clk =devm_clk_get (dev,NULL );
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 197
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c new file +6
-0+if (IS_ERR (pc->clk)) +return dev_err_probe (dev,PTR_ERR (pc->clk),"failed to get clock\n" ); + + ret =lgm_clk_enable (dev, pc); +if (ret) { +dev_err (dev,"failed to enable clock\n" );You used
dev_err_probefour times for six error paths. I wonder why you didn't use it here (and below for a failingpwmchip_add()).
Well noted, will update.
Quoted 7 lines
Quoted 7 lines
drivers/pwm/pwm-intel-lgm.c new file +7
-0+return ret; + } + + pc->rst =devm_reset_control_get_exclusive (dev,NULL ); +if (IS_ERR (pc->rst)) +return dev_err_probe (dev,PTR_ERR (pc->rst), +"failed to get reset control\n" );
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 208
Quoted 4 lines
drivers/pwm/pwm-intel-lgm.c new file +4
-0+ + ret =lgm_reset_control_deassert (dev, pc); +if (ret) +return dev_err_probe (dev, ret,"cannot deassert reset control\n" );After
lgm_reset_control_deassertis calledpc->rstis unused. So there is no need to have this member in structlgm_pwm_chip. The same applies to ->clk. (You have to pass rst (or clk) todevm_add_action_or_resetfor that to work. Looks like a nice idea anyhow.)
Agreed, will update.
Quoted 4 lines
drivers/pwm/pwm-intel-lgm.c line 212
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+ pc->chip.dev = dev; + pc->chip.ops = &lgm_pwm_ops; + pc->chip.npwm =1 ;
pc->chip.baseshould probably be set to -1.
Got it, will update. Thanks.
Regards,
Rahul
Quoted 3 lines
Best regards
Uwe
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
In reply to Uwe Kleine-KönigQuoted 2 lines · Uwe Kleine-König
Quoted 1 line · Andy Shevchenko
On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-König wrote:
...
Quoted 15 lines
Quoted 1 line
True. And above
dev_err_probe()is not needed.You argue that
dev_err_probe()gives no benefit aslgm_reset_control_deassertwon't return -EPROBE_DEFER, right?Still I consider it a useful function because
a) I (as an author or as a reviewer) don't need to think if the failing function might return -EPROBE_DEFER now or in the future.
dev_err_probedoes the right thing even for functions that don't return -EPROBE_DEFER.b) With
dev_err_probe()I can accomplish things in a single line that need two lines when open coding it.
Quoted 7 lines
c)
dev_err_probe()emits the symbolic error name without having to resort to %pe +ERR_PTR.d) Using
dev_err_probe()for all error paths gives a consistency that I like with a maintainer's hat on.So I still want to request using
dev_err_probe()in all error paths.
As a maintainer it is your choice. I really would like to see more consensus among maintainers, some are insisting of what I said, some, like you, on the opposite, some hate that API and some simply don't care.
And on top of that I saw already use of API without taking returned value into account.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
In reply to Uwe Kleine-KönigQuoted 42 lines · Uwe Kleine-König
Quoted 29 lines · Andy Shevchenko
Quoted 1 line · Uwe Kleine-König
On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote:
...
Quoted 6 lines
drivers/pwm/pwm-intel-lgm.c line 197
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+ ret =lgm_clk_enable (dev, pc); +if (ret) { +dev_err (dev,"failed to enable clock\n" );You used
dev_err_probefour times for six error paths. I wonder why you didn't use it here (and below for a failingpwmchip_add()).
dev_err_probe()makes sense when we might experience deferred probe. In neither of mentioned function this can be the case.Quoted 2 lines
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }...
Quoted 8 lines
drivers/pwm/pwm-intel-lgm.c line 208
Quoted 3 lines
drivers/pwm/pwm-intel-lgm.c new file +3
-0+ ret =lgm_reset_control_deassert (dev, pc); +if (ret) +return dev_err_probe (dev, ret,"cannot deassert reset control\n" );After
lgm_reset_control_deassertis calledpc->rstis unused. So there is no need to have this member in structlgm_pwm_chip. The same applies to ->clk. (You have to pass rst (or clk) todevm_add_action_or_resetfor that to work. Looks like a nice idea anyhow.)Quoted 1 line
True. And above
dev_err_probe()is not needed.You argue that
dev_err_probe()gives no benefit aslgm_reset_control_deassertwon't return -EPROBE_DEFER, right?Still I consider it a useful function because
a) I (as an author or as a reviewer) don't need to think if the failing function might return -EPROBE_DEFER now or in the future.
dev_err_probedoes the right thing even for functions that don't return -EPROBE_DEFER.
Quoted 8 lines
b) With
dev_err_probe()I can accomplish things in a single line that need two lines when open coding it.c)
dev_err_probe()emits the symbolic error name without having to resort to %pe +ERR_PTR.d) Using
dev_err_probe()for all error paths gives a consistency that I like with a maintainer's hat on.
That would perhaps be true if all error paths did use dev_err_probe(). And even if that were the case, dev_err_probe() doesn't guarantee that error messages will actually be consistent because developers can still provide whatever format string they like.
Also, the format of the messages that dev_err_probe() prints is unlike anything that I've seen, so introducing dev_err_probe() actually makes things more inconsistent, in my opinion.
I have in fact been advocating for people to use error messages of the form:
"failed to ...: %d\n", err
or:
"unable to ...: %d\n", err
Or some other similar form because that's the most common type that I have come across in the kernel. I think it's also easier to read those error messages because they contain the important data (i.e. the description, which tells you what went wrong) first and then are followed by the error code (which tells you how it failed).
Now I suspect the current format was chosen because we need to have the constant part first, because otherwise the arbitrary format string could be something that doesn't lend itself to have an error code appended.
The current format is arguably also something that's easier to parse from some script because the format is in a somewhat standard format. On the other hand, I think this is a bit misguided because we already have structured log messages, so I wonder if it might have been better to make the error code part of structured log messages to make them truly machine readable but leave the formatting up to developers so that they can use whatever is consistent within the driver or whatever fits best without actually adding a standard string to the log messages.
Thierry