* [PATCH v4 0/2] pwm: intel: Add PWM driver for a new SoC @ 2020-06-30 7:55 Rahul Tanwar 2020-06-30 7:55 ` [PATCH v4 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC Rahul Tanwar 2020-06-30 7:55 ` [PATCH v4 2/2] Add PWM fan controller driver for " Rahul Tanwar 0 siblings, 2 replies; 10+ messages in thread From: Rahul Tanwar @ 2020-06-30 7:55 UTC (permalink / raw) To: u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ, linux-pwm-u79uwXL29TY76Z2rM5mHXA Cc: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w, songjun.Wu-ral2JQCrhuEAvxtiuMwx3w, cheol.yong.kim-ral2JQCrhuEAvxtiuMwx3w, qi-ming.wu-ral2JQCrhuEAvxtiuMwx3w, rahul.tanwar.linux-Re5JQEeQqe8AvxtiuMwx3w, Rahul Tanwar [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 1977 bytes --] Patch 1 adds dt binding document in YAML format. Patch 2 add PWM fan controller driver for LGM SoC. Patch series is baselined on linux 5.8-rc2. v3: - 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 | 50 ++++ drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 266 +++++++++++++++++++++ 4 files changed, 328 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml create mode 100644 drivers/pwm/pwm-intel-lgm.c -- 2.11.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC 2020-06-30 7:55 [PATCH v4 0/2] pwm: intel: Add PWM driver for a new SoC Rahul Tanwar @ 2020-06-30 7:55 ` Rahul Tanwar 2020-07-13 16:46 ` Rob Herring 2020-06-30 7:55 ` [PATCH v4 2/2] Add PWM fan controller driver for " Rahul Tanwar 1 sibling, 1 reply; 10+ messages in thread From: Rahul Tanwar @ 2020-06-30 7:55 UTC (permalink / raw) To: u.kleine-koenig, linux-pwm Cc: thierry.reding, p.zabel, robh+dt, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, rahul.tanwar.linux, Rahul Tanwar Intel's LGM(Lightning Mountain) SoC contains a PWM fan controller which is only used to control the fan attached to the system. This PWM controller does not have any other consumer other than fan. Add DT bindings documentation for this PWM fan controller. Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com> --- .../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 50 ++++++++++++++++++++++ 1 file changed, 50 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..120bf3d85a24 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml @@ -0,0 +1,50 @@ +# 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 <rahul.tanwar@intel.com> + +properties: + compatible: + const: intel,lgm-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + intel,fan-wire: + $ref: '/schemas/types.yaml#/definitions/uint32' + description: Specifies fan mode. Default when unspecified is 2. + + intel,max-rpm: + $ref: '/schemas/types.yaml#/definitions/uint32' + description: + Specifies maximum RPM of fan attached to the system. + Default when unspecified is 4000. + +required: + - compatible + - reg + - clocks + - resets + +additionalProperties: false + +examples: + - | + pwm: pwm@e0d00000 { + compatible = "intel,lgm-pwm"; + reg = <0xe0d00000 0x30>; + clocks = <&cgu0 126>; + resets = <&rcu0 0x30 21>; + }; -- 2.11.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC 2020-06-30 7:55 ` [PATCH v4 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC Rahul Tanwar @ 2020-07-13 16:46 ` Rob Herring 2020-07-14 5:32 ` Tanwar, Rahul 0 siblings, 1 reply; 10+ messages in thread From: Rob Herring @ 2020-07-13 16:46 UTC (permalink / raw) To: Rahul Tanwar Cc: u.kleine-koenig, linux-pwm, thierry.reding, p.zabel, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, rahul.tanwar.linux On Tue, Jun 30, 2020 at 03:55:31PM +0800, Rahul Tanwar wrote: > 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 <rahul.tanwar@linux.intel.com> > --- > .../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 50 ++++++++++++++++++++++ > 1 file changed, 50 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..120bf3d85a24 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml > @@ -0,0 +1,50 @@ > +# 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 <rahul.tanwar@intel.com> > + > +properties: > + compatible: > + const: intel,lgm-pwm > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + resets: > + maxItems: 1 > + > + intel,fan-wire: > + $ref: '/schemas/types.yaml#/definitions/uint32' > + description: Specifies fan mode. Default when unspecified is 2. > + > + intel,max-rpm: > + $ref: '/schemas/types.yaml#/definitions/uint32' > + description: > + Specifies maximum RPM of fan attached to the system. > + Default when unspecified is 4000. These are properties of the fan, not the PWM. And probably if you need these properties then others would too, so they should be common. Look at the pwm-fan.txt binding. Rob ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC 2020-07-13 16:46 ` Rob Herring @ 2020-07-14 5:32 ` Tanwar, Rahul 0 siblings, 0 replies; 10+ messages in thread From: Tanwar, Rahul @ 2020-07-14 5:32 UTC (permalink / raw) To: Rob Herring Cc: u.kleine-koenig, linux-pwm, thierry.reding, p.zabel, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, rahul.tanwar.linux Hi Rob, On 14/7/2020 12:46 am, Rob Herring wrote: > On Tue, Jun 30, 2020 at 03:55:31PM +0800, Rahul Tanwar wrote: >> 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 <rahul.tanwar@linux.intel.com> >> --- >> .../devicetree/bindings/pwm/intel,lgm-pwm.yaml | 50 ++++++++++++++++++++++ >> 1 file changed, 50 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..120bf3d85a24 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/intel,lgm-pwm.yaml >> @@ -0,0 +1,50 @@ >> +# 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 <rahul.tanwar@intel.com> >> + >> +properties: >> + compatible: >> + const: intel,lgm-pwm >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + resets: >> + maxItems: 1 >> + >> + intel,fan-wire: >> + $ref: '/schemas/types.yaml#/definitions/uint32' >> + description: Specifies fan mode. Default when unspecified is 2. >> + >> + intel,max-rpm: >> + $ref: '/schemas/types.yaml#/definitions/uint32' >> + description: >> + Specifies maximum RPM of fan attached to the system. >> + Default when unspecified is 4000. > These are properties of the fan, not the PWM. And probably if you > need these properties then others would too, so they should be > common. Look at the pwm-fan.txt binding. I checked pwm-fan.txt. I don't find any common property which matches our fan properties of fan wire mode & max-rpm. Are you suggesting to add these properties additionally in pwm-fan.txt as new common properties and then use newly added generic name in our driver? Also, we have a dedicated PWM fan controller. So do you think this driver belongs to drivers/hwmon instead of drivers/pwm? Thanks. Regards, Rahul ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC 2020-06-30 7:55 [PATCH v4 0/2] pwm: intel: Add PWM driver for a new SoC Rahul Tanwar 2020-06-30 7:55 ` [PATCH v4 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC Rahul Tanwar @ 2020-06-30 7:55 ` Rahul Tanwar 2020-07-13 19:10 ` Uwe Kleine-König 1 sibling, 1 reply; 10+ messages in thread From: Rahul Tanwar @ 2020-06-30 7:55 UTC (permalink / raw) To: u.kleine-koenig, linux-pwm Cc: thierry.reding, p.zabel, robh+dt, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, rahul.tanwar.linux, 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 <rahul.tanwar@linux.intel.com> --- drivers/pwm/Kconfig | 11 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-intel-lgm.c | 266 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 278 insertions(+) create mode 100644 drivers/pwm/pwm-intel-lgm.c diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index cb8d739067d2..3486edab09c4 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 a59c710e98c7..db154a6b4f51 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..fddfedd56fc3 --- /dev/null +++ b/drivers/pwm/pwm-intel-lgm.c @@ -0,0 +1,266 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 Intel Corporation. + * + * Notes & Limitations: + * - The hardware supports fixed period which is dependent on 2/3 or 4 + * wire fan mode. + * - Supports normal polarity. Does not support changing polarity. + * - When PWM is disabled, output of PWM will become 0(inactive). It doesn't + * keep track of running period. + * - When duty cycle is changed, PWM output may be a mix of previous setting + * and new setting for the first period. From second period, the output is + * based on new setting. + * - 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_4WIRE 0x1 +#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 +#define LGM_PWM_PERIOD_4WIRE_NSECS 40000 + +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; + + regmap_update_bits(regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_EN_MSK, + enable ? LGM_PWM_FAN_EN_EN : LGM_PWM_FAN_EN_DIS); + + return 0; +} + +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip); + u32 duty_cycle, val; + unsigned int period; + + if (!state->enabled) { + lgm_pwm_enable(chip, 0); + return 0; + } + + period = min_t(u64, state->period, pc->period); + + if (state->polarity != PWM_POLARITY_NORMAL || + period < pc->period) + return -EINVAL; + + duty_cycle = min_t(u32, state->duty_cycle, period); + /* reg_value = duty_ns * LGM_PWM_MAX_DUTY_CYCLE(0xff) / period_ns */ + val = duty_cycle * LGM_PWM_MAX_DUTY_CYCLE / period; + + regmap_update_bits(pc->regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_DC_MSK, + FIELD_PREP(LGM_PWM_FAN_DC_MSK, val)); + + if (state->enabled) + lgm_pwm_enable(chip, 1); + + return 0; +} + +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 device *dev = pc->chip.dev; + struct regmap *regmap = pc->regmap; + u32 max_rpm, fan_wire, con0_val, con0_mask; + + if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire)) + fan_wire = 2; /* default is 2 wire mode */ + + con0_mask = LGM_PWM_FAN_MODE_MSK; + + switch (fan_wire) { + case 4: + con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK, LGM_PWM_FAN_MODE_4WIRE); + pc->period = LGM_PWM_PERIOD_4WIRE_NSECS; + break; + default: + /* default is 2wire mode */ + con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK, LGM_PWM_FAN_MODE_2WIRE); + pc->period = LGM_PWM_PERIOD_2WIRE_NSECS; + break; + } + + if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm)) + max_rpm = LGM_PWM_DEFAULT_RPM; + + max_rpm = min_t(u32, max_rpm, LGM_PWM_MAX_RPM); + if (max_rpm == 0) + max_rpm = LGM_PWM_DEFAULT_RPM; + + regmap_update_bits(regmap, LGM_PWM_FAN_CON1, LGM_PWM_FAN_MAX_RPM_MSK, max_rpm); + regmap_update_bits(regmap, LGM_PWM_FAN_CON0, con0_mask, 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"); + 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC 2020-06-30 7:55 ` [PATCH v4 2/2] Add PWM fan controller driver for " Rahul Tanwar @ 2020-07-13 19:10 ` Uwe Kleine-König 2020-07-14 5:35 ` Tanwar, Rahul [not found] ` <20200713191059.zsokzvv3k2hyaxcl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 0 siblings, 2 replies; 10+ messages in thread From: Uwe Kleine-König @ 2020-07-13 19:10 UTC (permalink / raw) To: Rahul Tanwar Cc: linux-pwm, thierry.reding, p.zabel, robh+dt, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, rahul.tanwar.linux [-- Attachment #1: Type: text/plain, Size: 10229 bytes --] Hello, On Tue, Jun 30, 2020 at 03:55:32PM +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> > --- > drivers/pwm/Kconfig | 11 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-intel-lgm.c | 266 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 278 insertions(+) > create mode 100644 drivers/pwm/pwm-intel-lgm.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index cb8d739067d2..3486edab09c4 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 a59c710e98c7..db154a6b4f51 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..fddfedd56fc3 > --- /dev/null > +++ b/drivers/pwm/pwm-intel-lgm.c > @@ -0,0 +1,266 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2020 Intel Corporation. > + * > + * Notes & Limitations: I'd like to have this "Limitations:" only to make it easily greppable. > + * - 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_4WIRE 0x1 > +#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 > +#define LGM_PWM_PERIOD_4WIRE_NSECS 40000 > + > +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; > + > + regmap_update_bits(regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_EN_MSK, > + enable ? LGM_PWM_FAN_EN_EN : LGM_PWM_FAN_EN_DIS); regmap_update_bits has a return value. I think it is supposed to be checked. > + > + return 0; > +} > + > +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip); > + u32 duty_cycle, val; > + unsigned int period; > + > + if (!state->enabled) { > + lgm_pwm_enable(chip, 0); > + return 0; > + } > + > + period = min_t(u64, state->period, pc->period); > + > + if (state->polarity != PWM_POLARITY_NORMAL || > + period < pc->period) > + return -EINVAL; This check looks wrong. If you refuse period < pc->period there isn't much configuration possible. > + duty_cycle = min_t(u32, state->duty_cycle, period); This is wrong. Consider state->duty_cycle = 0x100000001 (once it is an u64). > + /* reg_value = duty_ns * LGM_PWM_MAX_DUTY_CYCLE(0xff) / period_ns */ > + val = duty_cycle * LGM_PWM_MAX_DUTY_CYCLE / period; The comment is little helpful. > + regmap_update_bits(pc->regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_DC_MSK, > + FIELD_PREP(LGM_PWM_FAN_DC_MSK, val)); > + > + if (state->enabled) > + lgm_pwm_enable(chip, 1); > + > + return 0; > +} > + > +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 device *dev = pc->chip.dev; > + struct regmap *regmap = pc->regmap; > + u32 max_rpm, fan_wire, con0_val, con0_mask; > + > + if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire)) > + fan_wire = 2; /* default is 2 wire mode */ > + > + con0_mask = LGM_PWM_FAN_MODE_MSK; > + > + switch (fan_wire) { > + case 4: > + con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK, LGM_PWM_FAN_MODE_4WIRE); > + pc->period = LGM_PWM_PERIOD_4WIRE_NSECS; > + break; > + default: > + /* default is 2wire mode */ > + con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK, LGM_PWM_FAN_MODE_2WIRE); > + pc->period = LGM_PWM_PERIOD_2WIRE_NSECS; > + break; > + } > + > + if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm)) > + max_rpm = LGM_PWM_DEFAULT_RPM; > + > + max_rpm = min_t(u32, max_rpm, LGM_PWM_MAX_RPM); > + if (max_rpm == 0) > + max_rpm = LGM_PWM_DEFAULT_RPM; > + > + regmap_update_bits(regmap, LGM_PWM_FAN_CON1, LGM_PWM_FAN_MAX_RPM_MSK, max_rpm); > + regmap_update_bits(regmap, LGM_PWM_FAN_CON0, con0_mask, 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 missing here. > + 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); Please swap the two previous lines to match the error patch of .probe. > + > + return 0; > +} Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC 2020-07-13 19:10 ` Uwe Kleine-König @ 2020-07-14 5:35 ` Tanwar, Rahul 2020-07-14 7:18 ` Uwe Kleine-König [not found] ` <20200713191059.zsokzvv3k2hyaxcl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 1 sibling, 1 reply; 10+ messages in thread From: Tanwar, Rahul @ 2020-07-14 5:35 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm, thierry.reding, p.zabel, robh+dt, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, rahul.tanwar.linux Hi Uwe, On 14/7/2020 3:10 am, Uwe Kleine-König wrote: > Hello, > > On Tue, Jun 30, 2020 at 03:55:32PM +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> >> --- >> drivers/pwm/Kconfig | 11 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-intel-lgm.c | 266 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 278 insertions(+) >> create mode 100644 drivers/pwm/pwm-intel-lgm.c >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index cb8d739067d2..3486edab09c4 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 a59c710e98c7..db154a6b4f51 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..fddfedd56fc3 >> --- /dev/null >> +++ b/drivers/pwm/pwm-intel-lgm.c >> @@ -0,0 +1,266 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2020 Intel Corporation. >> + * >> + * Notes & Limitations: > I'd like to have this "Limitations:" only to make it easily greppable. > >> + * - 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_4WIRE 0x1 >> +#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 >> +#define LGM_PWM_PERIOD_4WIRE_NSECS 40000 >> + >> +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; >> + >> + regmap_update_bits(regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_EN_MSK, >> + enable ? LGM_PWM_FAN_EN_EN : LGM_PWM_FAN_EN_DIS); > regmap_update_bits has a return value. I think it is supposed to be > checked. > >> + >> + return 0; >> +} >> + >> +static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, >> + const struct pwm_state *state) >> +{ >> + struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip); >> + u32 duty_cycle, val; >> + unsigned int period; >> + >> + if (!state->enabled) { >> + lgm_pwm_enable(chip, 0); >> + return 0; >> + } >> + >> + period = min_t(u64, state->period, pc->period); >> + >> + if (state->polarity != PWM_POLARITY_NORMAL || >> + period < pc->period) >> + return -EINVAL; > This check looks wrong. If you refuse period < pc->period there isn't > much configuration possible. > >> + duty_cycle = min_t(u32, state->duty_cycle, period); > This is wrong. Consider state->duty_cycle = 0x100000001 (once it is an > u64). > >> + /* reg_value = duty_ns * LGM_PWM_MAX_DUTY_CYCLE(0xff) / period_ns */ >> + val = duty_cycle * LGM_PWM_MAX_DUTY_CYCLE / period; > The comment is little helpful. > >> + regmap_update_bits(pc->regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_DC_MSK, >> + FIELD_PREP(LGM_PWM_FAN_DC_MSK, val)); >> + >> + if (state->enabled) >> + lgm_pwm_enable(chip, 1); >> + >> + return 0; >> +} >> + >> +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 device *dev = pc->chip.dev; >> + struct regmap *regmap = pc->regmap; >> + u32 max_rpm, fan_wire, con0_val, con0_mask; >> + >> + if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire)) >> + fan_wire = 2; /* default is 2 wire mode */ >> + >> + con0_mask = LGM_PWM_FAN_MODE_MSK; >> + >> + switch (fan_wire) { >> + case 4: >> + con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK, LGM_PWM_FAN_MODE_4WIRE); >> + pc->period = LGM_PWM_PERIOD_4WIRE_NSECS; >> + break; >> + default: >> + /* default is 2wire mode */ >> + con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK, LGM_PWM_FAN_MODE_2WIRE); >> + pc->period = LGM_PWM_PERIOD_2WIRE_NSECS; >> + break; >> + } >> + >> + if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm)) >> + max_rpm = LGM_PWM_DEFAULT_RPM; >> + >> + max_rpm = min_t(u32, max_rpm, LGM_PWM_MAX_RPM); >> + if (max_rpm == 0) >> + max_rpm = LGM_PWM_DEFAULT_RPM; >> + >> + regmap_update_bits(regmap, LGM_PWM_FAN_CON1, LGM_PWM_FAN_MAX_RPM_MSK, max_rpm); >> + regmap_update_bits(regmap, LGM_PWM_FAN_CON0, con0_mask, 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 missing here. > >> + 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); > Please swap the two previous lines to match the error patch of .probe. > >> + >> + return 0; >> +} > Given the fact that this is a dedicated PWM fan controller with no other PWM consumer, do you think that this driver belongs to drivers/hwmon instead of drivers/pwm? Thanks. Regards, Rahul ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC 2020-07-14 5:35 ` Tanwar, Rahul @ 2020-07-14 7:18 ` Uwe Kleine-König 0 siblings, 0 replies; 10+ messages in thread From: Uwe Kleine-König @ 2020-07-14 7:18 UTC (permalink / raw) To: Tanwar, Rahul Cc: linux-pwm, thierry.reding, p.zabel, robh+dt, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, rahul.tanwar.linux [-- Attachment #1: Type: text/plain, Size: 800 bytes --] Hello Rahul, On Tue, Jul 14, 2020 at 01:35:14PM +0800, Tanwar, Rahul wrote: > On 14/7/2020 3:10 am, Uwe Kleine-König wrote: > > On Tue, Jun 30, 2020 at 03:55:32PM +0800, Rahul Tanwar wrote: > Given the fact that this is a dedicated PWM fan controller with no other > PWM consumer, do you think that this driver belongs to drivers/hwmon > instead of drivers/pwm? Thanks. If you implement the PWM API it belongs to drivers/pwm I'd say. (There are some "mixed" drivers that implement both GPIO and PWM that live in drivers/gpio, but if the support can be separated in a sane way, such mixing isn't welcome.) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20200713191059.zsokzvv3k2hyaxcl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]
* Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC [not found] ` <20200713191059.zsokzvv3k2hyaxcl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> @ 2020-07-23 4:16 ` Tanwar, Rahul 2020-07-23 6:34 ` Uwe Kleine-König 0 siblings, 1 reply; 10+ messages in thread From: Tanwar, Rahul @ 2020-07-23 4:16 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA, thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, linux-kernel-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w, songjun.Wu-ral2JQCrhuEAvxtiuMwx3w, cheol.yong.kim-ral2JQCrhuEAvxtiuMwx3w, qi-ming.wu-ral2JQCrhuEAvxtiuMwx3w, rahul.tanwar.linux-Re5JQEeQqe8AvxtiuMwx3w Hi Uwe, Thanks for the feedback. On 14/7/2020 3:10 am, Uwe Kleine-König wrote: > Hello, > > On Tue, Jun 30, 2020 at 03:55:32PM +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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> >> --- >> drivers/pwm/Kconfig | 11 ++ >> drivers/pwm/Makefile | 1 + >> drivers/pwm/pwm-intel-lgm.c | 266 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 278 insertions(+) >> create mode 100644 drivers/pwm/pwm-intel-lgm.c [...] >> + >> +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; >> + unsigned int period; >> + >> + if (!state->enabled) { >> + lgm_pwm_enable(chip, 0); >> + return 0; >> + } >> + >> + period = min_t(u64, state->period, pc->period); >> + >> + if (state->polarity != PWM_POLARITY_NORMAL || >> + period < pc->period) >> + return -EINVAL; > This check looks wrong. If you refuse period < pc->period there isn't > much configuration possible. I am kind of stuck here. I made this change of adding a check period < pc->period based on your feedback on v2 patch. In fact, you had specified this code in v2 review feedback and i used the same exact code. How should we handle it when the hardware supports fixed period. We don't want user to change period and allow just changing duty_cycle. With that intention, i had first added a strict check which refused configuration if period != pc->period. Period is intended to be a read only value. How do you suggest we handle the fixed period hardware support? Would you have any reference example of other drivers which also supports fixed period? Thanks. [...] >> +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); > Please swap the two previous lines to match the error patch of .probe. Again, i had made this change based on your below review feedback for v1. IMO, reverse of probe makes more sense. "In .probe() you first release reset and then enable the clock. It's good style to do it the other way round in .remove()." Regards, Rahul ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC 2020-07-23 4:16 ` Tanwar, Rahul @ 2020-07-23 6:34 ` Uwe Kleine-König 0 siblings, 0 replies; 10+ messages in thread From: Uwe Kleine-König @ 2020-07-23 6:34 UTC (permalink / raw) To: Tanwar, Rahul Cc: linux-pwm, thierry.reding, p.zabel, robh+dt, linux-kernel, devicetree, andriy.shevchenko, songjun.Wu, cheol.yong.kim, qi-ming.wu, rahul.tanwar.linux [-- Attachment #1: Type: text/plain, Size: 3571 bytes --] Hello, On Thu, Jul 23, 2020 at 12:16:18PM +0800, Tanwar, Rahul wrote: > On 14/7/2020 3:10 am, Uwe Kleine-König wrote: > > Hello, > > > > On Tue, Jun 30, 2020 at 03:55:32PM +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> > >> --- > >> drivers/pwm/Kconfig | 11 ++ > >> drivers/pwm/Makefile | 1 + > >> drivers/pwm/pwm-intel-lgm.c | 266 ++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 278 insertions(+) > >> create mode 100644 drivers/pwm/pwm-intel-lgm.c > > [...] > > >> + > >> +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; > >> + unsigned int period; > >> + > >> + if (!state->enabled) { > >> + lgm_pwm_enable(chip, 0); > >> + return 0; > >> + } > >> + > >> + period = min_t(u64, state->period, pc->period); > >> + > >> + if (state->polarity != PWM_POLARITY_NORMAL || > >> + period < pc->period) > >> + return -EINVAL; > > This check looks wrong. If you refuse period < pc->period there isn't > > much configuration possible. > > I am kind of stuck here. I made this change of adding a check > period < pc->period based on your feedback on v2 patch. > In fact, you had specified this code in v2 review feedback > and i used the same exact code. My feedback was nonsense, sorry. Now I don't understand anymore what I thought. The check you proposed in v2 is perfectly fine. > How should we handle it when the hardware supports fixed period. > We don't want user to change period and allow just changing > duty_cycle. With that intention, i had first added a strict check > which refused configuration if period != pc->period. Period is > intended to be a read only value. > > How do you suggest we handle the fixed period hardware support? > Would you have any reference example of other drivers which also > supports fixed period? Thanks. > > [...] > >> +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); > > Please swap the two previous lines to match the error patch of .probe. > > Again, i had made this change based on your below review feedback > for v1. IMO, reverse of probe makes more sense. > > "In .probe() you first release reset and then enable the clock. It's good > style to do it the other way round in .remove()." Again my comment was nonsense, I'm sorry. I think I was irritated by the fact that reset handling happens in between the clk stuff in probe. You do there: devm_clk_get devm_reset_control_get_exclusive reset_control_deassert clk_prepare_enable and I noticed that as "in probe clk handling is done first". Looking at the other feedback I think my other comments are not BS. Best regards and sorry again, Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-07-23 6:34 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-30 7:55 [PATCH v4 0/2] pwm: intel: Add PWM driver for a new SoC Rahul Tanwar 2020-06-30 7:55 ` [PATCH v4 1/2] Add DT bindings YAML schema for PWM fan controller of LGM SoC Rahul Tanwar 2020-07-13 16:46 ` Rob Herring 2020-07-14 5:32 ` Tanwar, Rahul 2020-06-30 7:55 ` [PATCH v4 2/2] Add PWM fan controller driver for " Rahul Tanwar 2020-07-13 19:10 ` Uwe Kleine-König 2020-07-14 5:35 ` Tanwar, Rahul 2020-07-14 7:18 ` Uwe Kleine-König [not found] ` <20200713191059.zsokzvv3k2hyaxcl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 2020-07-23 4:16 ` Tanwar, Rahul 2020-07-23 6:34 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).