Patch 1 adds dt binding document in YAML format.
Patch 2 add PWM fan controller driver for LGM SoC.
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 | 244 +++++++++++++++++++++
4 files changed, 300 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
create mode 100644 drivers/pwm/pwm-intel-lgm.c
--
2.11.0
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 | 44 ++++++++++++++++++++++
1 file changed, 44 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..11a606536169
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml
@@ -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>;
+ };
--
2.11.0
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 | 11 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-intel-lgm.c | 244 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 256 insertions(+)
create mode 100644 drivers/pwm/pwm-intel-lgm.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7dbcf6973d33..f7a832268c79 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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 OF && HAS_IOMEM
+ depends on 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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 2c2ba0a03557..e9431b151694 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..3d0f79b69532
--- /dev/null
+++ b/drivers/pwm/pwm-intel-lgm.c
@@ -0,0 +1,244 @@
+// 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.
+ * - 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.
+ * - 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 LGM_PWM_FAN_CON0 0x0
+#define LGM_PWM_FAN_EN_EN BIT(0)
+#define LGM_PWM_FAN_EN_DIS 0x0
+#define LGM_PWM_FAN_EN_MSK BIT(0)
+#define LGM_PWM_FAN_MODE_2WIRE 0x0
+#define LGM_PWM_FAN_MODE_MSK BIT(1)
+#define LGM_PWM_FAN_DC_MSK GENMASK(23, 16)
+
+#define LGM_PWM_FAN_CON1 0x4
+#define LGM_PWM_FAN_MAX_RPM_MSK GENMASK(15, 0)
+
+#define LGM_PWM_MAX_RPM (BIT(16) - 1)
+#define LGM_PWM_DEFAULT_RPM 4000
+#define LGM_PWM_MAX_DUTY_CYCLE (BIT(8) - 1)
+
+#define LGM_PWM_DC_BITS 8
+
+#define LGM_PWM_PERIOD_2WIRE_NSECS 40000000
+
+struct lgm_pwm_chip {
+ struct pwm_chip chip;
+ struct regmap *regmap;
+ struct clk *clk;
+ struct reset_control *rst;
+ u32 period;
+};
+
+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_enable(struct pwm_chip *chip, bool enable)
+{
+ struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
+ struct regmap *regmap = pc->regmap;
+
+ return regmap_update_bits(regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_EN_MSK,
+ enable ? LGM_PWM_FAN_EN_EN : LGM_PWM_FAN_EN_DIS);
+}
+
+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);
+ u32 duty_cycle, val;
+ int ret;
+
+ /*
+ * The hardware only supports
+ * normal polarity and fixed period.
+ */
+ if (state->polarity != PWM_POLARITY_NORMAL ||
+ state->period < pc->period)
+ return -EINVAL;
+
+ if (!state->enabled) {
+ ret = lgm_pwm_enable(chip, 0);
+ return ret;
+ }
+
+ duty_cycle = min_t(u64, state->duty_cycle, pc->period);
+ val = duty_cycle * LGM_PWM_MAX_DUTY_CYCLE / pc->period;
+
+ ret = regmap_update_bits(pc->regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_DC_MSK,
+ FIELD_PREP(LGM_PWM_FAN_DC_MSK, val));
+ if (ret)
+ return ret;
+
+ ret = lgm_pwm_enable(chip, 1);
+
+ 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, LGM_PWM_FAN_CON0,
+ LGM_PWM_FAN_EN_EN);
+ state->polarity = PWM_POLARITY_NORMAL;
+ state->period = pc->period; /* fixed period */
+
+ regmap_read(pc->regmap, LGM_PWM_FAN_CON0, &val);
+ duty = FIELD_GET(LGM_PWM_FAN_DC_MSK, val);
+ state->duty_cycle = DIV_ROUND_UP(duty * pc->period,
+ LGM_PWM_MAX_DUTY_CYCLE);
+}
+
+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_init(struct lgm_pwm_chip *pc)
+{
+ struct regmap *regmap = pc->regmap;
+ u32 con0_val;
+
+ con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK, LGM_PWM_FAN_MODE_2WIRE);
+ pc->period = LGM_PWM_PERIOD_2WIRE_NSECS;
+ regmap_update_bits(regmap, LGM_PWM_FAN_CON1, LGM_PWM_FAN_MAX_RPM_MSK,
+ LGM_PWM_DEFAULT_RPM);
+ regmap_update_bits(regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_MODE_MSK,
+ con0_val);
+}
+
+static const struct regmap_config lgm_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, &lgm_pwm_regmap_config);
+ if (IS_ERR(pc->regmap)) {
+ ret = PTR_ERR(pc->regmap);
+ if (ret != -EPROBE_DEFER)
+ 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);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to get clock: %pe\n", pc->clk);
+ 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(dev, "failed to get reset control: %pe\n",
+ pc->rst);
+ return ret;
+ }
+
+ ret = reset_control_deassert(pc->rst);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ 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");
+ reset_control_assert(pc->rst);
+ 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: %pe\n", ERR_PTR(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;
+
+ 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
On Thu, Aug 20, 2020 at 12:50:46PM +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.
...
> +config PWM_INTEL_LGM
> + tristate "Intel LGM PWM support"
> + depends on OF && HAS_IOMEM
> + depends on X86 || COMPILE_TEST
For better test coverage you may rewrite this
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.
...
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
This should be mod_devicetable.h.
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
...
> +#define LGM_PWM_PERIOD_2WIRE_NSECS 40000000
NSECS -> NS
40000000 -> (40 * NSEC_PER_MSEC)
...
> + if (state->polarity != PWM_POLARITY_NORMAL ||
> + state->period < pc->period)
It can be one line.
> + return -EINVAL;
...
> + if (!state->enabled) {
> + ret = lgm_pwm_enable(chip, 0);
> + return ret;
What is the point?
> + }
...
> + ret = lgm_pwm_enable(chip, 1);
> +
> + return ret;
Ditto.
...
> + state->duty_cycle = DIV_ROUND_UP(duty * pc->period,
> + LGM_PWM_MAX_DUTY_CYCLE);
One line?
...
> + struct lgm_pwm_chip *pc;
> + struct device *dev = &pdev->dev;
Use reversed xmas tree order.
> + void __iomem *io_base;
> + int ret;
...
> + 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(dev, "failed to init register map: %pe\n",
> + pc->regmap);
> + return ret;
dev_err_probe()
> + }
...
> + pc->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pc->clk)) {
> + ret = PTR_ERR(pc->clk);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to get clock: %pe\n", pc->clk);
> + return ret;
Ditto.
> + }
> +
> + 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(dev, "failed to get reset control: %pe\n",
> + pc->rst);
> + return ret;
Ditto.
> + }
> +
> + ret = reset_control_deassert(pc->rst);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "cannot deassert reset control: %pe\n",
> + ERR_PTR(ret));
> + return ret;
Ditto.
> + }
...
> + ret = clk_prepare_enable(pc->clk);
Wrap it with devm_add_action_or_reset(). Same for reset_control_deassert().
You probably can even put them under one function.
> + if (ret) {
> + dev_err(dev, "failed to enable clock\n");
> + reset_control_assert(pc->rst);
> + return ret;
> + }
...
> + ret = pwmchip_add(&pc->chip);
> + if (ret < 0) {
Does ' < 0' have any meaning?
> + dev_err(dev, "failed to add PWM chip: %pe\n", ERR_PTR(ret));
> + clk_disable_unprepare(pc->clk);
> + reset_control_assert(pc->rst);
> + return ret;
> + }
...
> + ret = pwmchip_remove(&pc->chip);
> + if (ret < 0)
Ditto.
> + return ret;
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 20/8/2020 6:52 pm, Andy Shevchenko wrote:
> On Thu, Aug 20, 2020 at 12:50:46PM +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.
> ...
>
>> +config PWM_INTEL_LGM
>> + tristate "Intel LGM PWM support"
>> + depends on OF && HAS_IOMEM
>> + depends on X86 || COMPILE_TEST
> For better test coverage you may rewrite this
>
> depends on HAS_IOMEM
> depends on (OF && X86) || COMPILE_TEST
Sure, will update.
>> + 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.
> ...
>
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
> This should be mod_devicetable.h.
Well noted.
>> +#include <linux/pwm.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
> ...
>
>> +#define LGM_PWM_PERIOD_2WIRE_NSECS 40000000
> NSECS -> NS
> 40000000 -> (40 * NSEC_PER_MSEC)
Well noted.
> ...
>
>> + if (state->polarity != PWM_POLARITY_NORMAL ||
>> + state->period < pc->period)
> It can be one line.
Ok.
>> + return -EINVAL;
> ...
>
>> + if (!state->enabled) {
>> + ret = lgm_pwm_enable(chip, 0);
>> + return ret;
> What is the point?
I guess you mean to change it to return lgm_pwm_enable(chip, 0);
Will do, thanks.
>> + }
> ...
>
>> + ret = lgm_pwm_enable(chip, 1);
>> +
>> + return ret;
> Ditto.
>
> ...
>
>> + state->duty_cycle = DIV_ROUND_UP(duty * pc->period,
>> + LGM_PWM_MAX_DUTY_CYCLE);
> One line?
Ok.
> ...
>
>> + struct lgm_pwm_chip *pc;
>> + struct device *dev = &pdev->dev;
> Use reversed xmas tree order.
Sure, will update.
>> + void __iomem *io_base;
>> + int ret;
> ...
>
>> + 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(dev, "failed to init register map: %pe\n",
>> + pc->regmap);
>> + return ret;
> dev_err_probe()
Will update. Thanks.
>> + }
> ...
>
>> + pc->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(pc->clk)) {
>> + ret = PTR_ERR(pc->clk);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "failed to get clock: %pe\n", pc->clk);
>> + return ret;
> Ditto.
>
>> + }
>> +
>> + 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(dev, "failed to get reset control: %pe\n",
>> + pc->rst);
>> + return ret;
> Ditto.
>
>> + }
>> +
>> + ret = reset_control_deassert(pc->rst);
>> + if (ret) {
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "cannot deassert reset control: %pe\n",
>> + ERR_PTR(ret));
>> + return ret;
> Ditto.
>
>> + }
> ...
>
>> + ret = clk_prepare_enable(pc->clk);
> Wrap it with devm_add_action_or_reset(). Same for reset_control_deassert().
> You probably can even put them under one function.
I did some study and research for using devm_add_action_or_reset(). But
still i have some doubts. Below steps is what i intend to do in order to
switch to using this API. Please do review and let me know it is ok and
i am not missing anything else. Thanks.
1. Call reset_control_assert()
2. Call clk_prepare_enable()
3. Call pwmchip_add()
4. Call devm_add_action_or_reset(dev, my_action, pc)
5. Remove explicit calls to unprepare/reset_control_assert from probe in
failure cases.
6. static void my_action(void *pc)
{
pwmchip_remove(&pc->chip);
clk_disable_upprepare(pc->clk);
reset_control_assert(pc->rst);
}
7. Remove platform_driver.remove() entirely.
>> + if (ret) {
>> + dev_err(dev, "failed to enable clock\n");
>> + reset_control_assert(pc->rst);
>> + return ret;
>> + }
> ...
>
>> + ret = pwmchip_add(&pc->chip);
>> + if (ret < 0) {
> Does ' < 0' have any meaning?
I use < 0 because this API's return code is mentioned as below:
Returns: 0 on success or a negative error code on failure.
Also, all other PWM drivers check for <0 for this call.
>> + dev_err(dev, "failed to add PWM chip: %pe\n", ERR_PTR(ret));
>> + clk_disable_unprepare(pc->clk);
>> + reset_control_assert(pc->rst);
>> + return ret;
>> + }
> ...
>
>> + ret = pwmchip_remove(&pc->chip);
>> + if (ret < 0)
> Ditto.
Same as above.
>> + return ret;
Thanks.
Regards,
Rahul
On Fri, Aug 21, 2020 at 02:07:49PM +0800, Tanwar, Rahul wrote:
> On 20/8/2020 6:52 pm, Andy Shevchenko wrote:
> > On Thu, Aug 20, 2020 at 12:50:46PM +0800, Rahul Tanwar wrote:
...
> >> + ret = clk_prepare_enable(pc->clk);
> > Wrap it with devm_add_action_or_reset(). Same for reset_control_deassert().
> > You probably can even put them under one function.
>
> I did some study and research for using devm_add_action_or_reset(). But
> still i have some doubts. Below steps is what i intend to do in order to
> switch to using this API. Please do review and let me know it is ok and
> i am not missing anything else. Thanks.
>
> 1. Call reset_control_assert()
> 2. Call clk_prepare_enable()
> 3. Call pwmchip_add()
First of all, I haven't told anything about this.
> 4. Call devm_add_action_or_reset(dev, my_action, pc)
> 5. Remove explicit calls to unprepare/reset_control_assert from probe in
> failure cases.
> 6. static void my_action(void *pc)
> ?? {
> ????? pwmchip_remove(&pc->chip);
> ????? clk_disable_upprepare(pc->clk);
> ????? reset_control_assert(pc->rst);
> ?? }
> 7. Remove platform_driver.remove() entirely.
Nope, pwmchip_add() and pwmchip_remove() stay as is now.
Only wrap clock and reset resources.
> >> + if (ret) {
> >> + dev_err(dev, "failed to enable clock\n");
> >> + reset_control_assert(pc->rst);
> >> + return ret;
> >> + }
--
With Best Regards,
Andy Shevchenko
On 20/8/2020 6:52 pm, Andy Shevchenko wrote:
> On Thu, Aug 20, 2020 at 12:50:46PM +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.
> ...
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
>> +#include <linux/of_device.h>
> This should be mod_devicetable.h.
Inlcuding mod_devicetable.h instead of of_device.h results in
build failure related to all platform calls. Build is ok if
i remove it. Just FYI..
Regards,
Rahul
On Fri, Aug 21, 2020 at 05:14:29PM +0800, Tanwar, Rahul wrote:
> On 20/8/2020 6:52 pm, Andy Shevchenko wrote:
> > On Thu, Aug 20, 2020 at 12:50:46PM +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.
> > ...
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/module.h>
> >> +#include <linux/of_device.h>
> > This should be mod_devicetable.h.
>
> Inlcuding mod_devicetable.h instead of of_device.h results in
> build failure related to all platform calls. Build is ok if
> i remove it. Just FYI..
I didn't get what the change you performed, by there is no users of of_device.h
in your code. If build fails it means you need to figure out proper list of
what has to be included.
--
With Best Regards,
Andy Shevchenko