| Message ID | befa655d8beb326fc8aa405a25a8b3e62b7e6a4a.1600158087.git.rahul.tanwar@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v13,1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC | expand |
Hello, (hhm Thierry already announced to have taken this patch, so my review is late.) On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote: > Intel Lightning Mountain(LGM) SoC contains a PWM fan controller. > This PWM controller does not have any other consumer, it is a > dedicated PWM controller for fan attached to the system. Add > driver for this PWM fan controller. > > Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> > --- > 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 > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 7dbcf6973d33..4949c51fe90b 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 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 > 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..ea3df75a5971 > --- /dev/null > +++ b/drivers/pwm/pwm-intel-lgm.c > @@ -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. > +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); > +} 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()?) > +static void lgm_reset_control_assert(void *data) > +{ > + struct lgm_pwm_chip *pc = data; > + > + reset_control_assert(pc->rst); > +} > + > +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. > +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); > + 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()). > + 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.) > + 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
On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-König wrote: > 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
On 24/9/2020 3:12 pm, Thierry Reding wrote: > On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-König wrote: >> 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 > Thierry
On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-König wrote: > On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote: ... > > + 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()). dev_err_probe() makes sense when we might experience deferred probe. In neither of mentioned function this can be the case. > > + return ret; > > + } ... > > + 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.) True. And above dev_err_probe() is not needed.
On Thu, Sep 24, 2020 at 04:23:34PM +0300, Andy Shevchenko wrote: > On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-König wrote: > > On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote: > > ... > > > > + 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()). > > dev_err_probe() makes sense when we might experience deferred probe. In neither > of mentioned function this can be the case. > > > > + return ret; > > > + } > > ... > > > > + 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.) > > 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
On Thu, Sep 24, 2020 at 04:16:59PM +0200, Uwe Kleine-König wrote: > On Thu, Sep 24, 2020 at 04:23:34PM +0300, Andy Shevchenko wrote: > > On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-König wrote: ... > > 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. 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.
Hi Uwe, Thanks for review & feedback. On 24/9/2020 2:55 pm, Uwe Kleine-König wrote: > Hello, > > (hhm Thierry already announced to have taken this patch, so my review is > late.) > > On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote: >> Intel Lightning Mountain(LGM) SoC contains a PWM fan controller. >> This PWM controller does not have any other consumer, it is a >> dedicated PWM controller for fan attached to the system. Add >> driver for this PWM fan controller. >> >> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> >> --- >> 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 >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index 7dbcf6973d33..4949c51fe90b 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 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 >> 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..ea3df75a5971 >> --- /dev/null >> +++ b/drivers/pwm/pwm-intel-lgm.c >> @@ -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. Will update. >> +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); >> +} > 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()?) 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).. >> +static void lgm_reset_control_assert(void *data) >> +{ >> + struct lgm_pwm_chip *pc = data; >> + >> + reset_control_assert(pc->rst); >> +} >> + >> +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. > >> +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); >> + 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()). Well noted, will update. >> + 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.) Agreed, will update. >> + pc->chip.dev = dev; >> + pc->chip.ops = &lgm_pwm_ops; >> + pc->chip.npwm = 1; > pc->chip.base should probably be set to -1. Got it, will update. Thanks. Regards, Rahul > Best regards > Uwe >
On Thu, Sep 24, 2020 at 04:16:59PM +0200, Uwe Kleine-König wrote: > On Thu, Sep 24, 2020 at 04:23:34PM +0300, Andy Shevchenko wrote: > > On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-König wrote: > > > On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote: > > > > ... > > > > > > + 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()). > > > > dev_err_probe() makes sense when we might experience deferred probe. In neither > > of mentioned function this can be the case. > > > > > > + return ret; > > > > + } > > > > ... > > > > > > + 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.) > > > > 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. 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
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 7dbcf6973d33..4949c51fe90b 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 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 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..ea3df75a5971 --- /dev/null +++ b/drivers/pwm/pwm-intel-lgm.c @@ -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. + * - 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/platform_device.h> +#include <linux/mod_devicetable.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_NS (40 * NSEC_PER_MSEC) + +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) + return lgm_pwm_enable(chip, 0); + + 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; + + return lgm_pwm_enable(chip, 1); +} + +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_NS; + 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 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); +} + +static void lgm_reset_control_assert(void *data) +{ + struct lgm_pwm_chip *pc = data; + + reset_control_assert(pc->rst); +} + +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); +} + +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); + 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"); + 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"); + + 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)); + return ret; + } + + return 0; +} + +static int lgm_pwm_remove(struct platform_device *pdev) +{ + struct lgm_pwm_chip *pc = platform_get_drvdata(pdev); + + return pwmchip_remove(&pc->chip); +} + +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);