* [PATCH v12 0/2] Add PWM support for Intel Keem Bay SoC @ 2020-10-14 19:36 vijayakannan.ayyathurai 2020-10-14 19:36 ` [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay vijayakannan.ayyathurai 2020-10-14 19:36 ` [PATCH v12 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM vijayakannan.ayyathurai 0 siblings, 2 replies; 17+ messages in thread From: vijayakannan.ayyathurai @ 2020-10-14 19:36 UTC (permalink / raw) To: thierry.reding, u.kleine-koenig, robh+dt Cc: linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko, mgross, lakshmi.bai.raja.subramanian, vijayakannan.ayyathurai From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> Hi, This patch set enables the support for PWM in the Intel Keem Bay SoC. Keem Bay is an ARM based SoC, and the GPIO module allows configuration of 6 PWM outputs. Patch 1 adds the PWM driver and Patch 2 is for the required Device Tree bindings documentation. This driver was tested on the Keem Bay evaluation module board. Thank you. Regards, Vijay Changes since v11: - Minor variable name change to match the api needs. - Setup polarity as PWM_POLARITY_NORMAL. - Add comment for LEADIN register usage. Changes since v10: - Update low time calculation formula as per Uwe. - During distruct remove pwmchip first then disable the clock. Changes since v9: - Remove Reported-by tag from the commit log. Changes since v8: - Fix the compilation error reported by kernel test robot. - Add the tag Reported-by: kernel test robot <lkp@intel.com> - Minor correction in the pwm low time calculation formula. - Rebase with 5.9-rc7 Changes since v7: - Change the dependency as ARCH_KEEMBAY instead of ARM64 in Kconfig. - Use DIV_ROUND_DOWN_ULL instead of DIV_ROUND_CLOSEST_ULL. - Update the right formula as per Uwe. - List the tags in chronological order. - Add clk_disable_unprepare in the error paths. Changes since v6: - Add reviewed-by tag Changes since v5: - Reorder symbols/Kconfig in drivers/pwm/Kconfig and drivers/pwm/Makefile - Use "Limitations" for consistency - Add clk_prepare_enable() - Reorder keembay_pwm_get_state() function call - Rework if conditional for channel disablement in .apply() - Remove channel disabling from .probe(), and clear LEADIN register bits in .apply instead - Update commit message for Patch 1 Changes since v4: - Add co-developed-by tag - Include mod_devicetable.h and remove of.h - Update comment with correct calulation for high/low time - Fix missing return from dev_err_probe Changes since v3: - Removed variable for address and calculate in place instead - Utilized u32_replace_bits() when updating KMB_PWM_LEADIN_OFFSET - Utilized dev_err_probe() for error reporting - Updated comments to use physical units - Updated error check for pwmchip_add() Changes since v2: - Include documentation about HW limitation/behaviour - Use hex values for KMB_PWM_COUNT_MAX - Redefine register macros - Utilize FIELD_GET/FIELD_PREP for calculating pwm_l/h_count and pwm_count - Round up duty cycle/period values - Get current hardware state in .apply instead of cached values - Do a polarity check before .enabled - Round high time/low time to closest value - Set enable bit in KMB_PWM_LEADIN_OFFSET to 0 in probe - Correct the naming for MODULE_ALIAS - Add additionalProperties: false in DT bindings Changes since v1: - Updated licensing info, "clocks" property and example in DT bindings - Updated name of DT bindings document to match compatible string - Removed 1 patch for addition of new sysfs attribute "count" - Added support for COMPILE_TEST in Kconfig - Updated naming of defines and regmap attribute - Updated calculation of waveform high time and low time - Added range checking for waveform high/low time - Implemented .get_state - Removed register writes for lead-in and count values (left to default) - Updated register access to single-access - Folded keembay_pwm_enable/disable_channel, keembay_pwm_config_period/duty_cycle, and keembay_pwm_config into keembay_pwm_apply - Updated error messages/error codes - Removed pwm_disable from keembay_pwm_remove - Removed clk_prepare/clk_enable/clk_disable from driver Lai, Poey Seng (1): pwm: Add PWM driver for Intel Keem Bay Vineetha G. Jaya Kumaran (1): dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM .../bindings/pwm/intel,keembay-pwm.yaml | 47 ++++ drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-keembay.c | 233 ++++++++++++++++++ 4 files changed, 290 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml create mode 100644 drivers/pwm/pwm-keembay.c base-commit: 549738f15da0e5a00275977623be199fbbf7df50 prerequisite-patch-id: 0a348762b660d0d817b8e70cc71647e83173c78c prerequisite-patch-id: 0c6072cfe492b078c44ec864b8f9d1c76eada93b prerequisite-patch-id: 12b93428ee51a3d92ca973b928c0e0989f5d585e -- 2.17.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-14 19:36 [PATCH v12 0/2] Add PWM support for Intel Keem Bay SoC vijayakannan.ayyathurai @ 2020-10-14 19:36 ` vijayakannan.ayyathurai 2020-10-14 20:04 ` Uwe Kleine-König 2020-10-15 10:42 ` Andy Shevchenko 2020-10-14 19:36 ` [PATCH v12 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM vijayakannan.ayyathurai 1 sibling, 2 replies; 17+ messages in thread From: vijayakannan.ayyathurai @ 2020-10-14 19:36 UTC (permalink / raw) To: thierry.reding, u.kleine-koenig, robh+dt Cc: linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko, mgross, lakshmi.bai.raja.subramanian, vijayakannan.ayyathurai From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> The Intel Keem Bay SoC requires PWM support. Add the pwm-keembay driver to enable this. Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> Co-developed-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> --- drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-keembay.c | 233 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 243 insertions(+) create mode 100644 drivers/pwm/pwm-keembay.c diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 7dbcf6973d33..6129a9dbbfa8 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -254,6 +254,15 @@ config PWM_JZ4740 To compile this driver as a module, choose M here: the module will be called pwm-jz4740. +config PWM_KEEMBAY + tristate "Intel Keem Bay PWM driver" + depends on ARCH_KEEMBAY || COMPILE_TEST + help + The platform driver for Intel Keem Bay PWM controller. + + To compile this driver as a module, choose M here: the module + will be called pwm-keembay. + config PWM_LP3943 tristate "TI/National Semiconductor LP3943 PWM support" depends on MFD_LP3943 diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 2c2ba0a03557..a1051122eb07 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c new file mode 100644 index 000000000000..ced6d4010add --- /dev/null +++ b/drivers/pwm/pwm-keembay.c @@ -0,0 +1,233 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Intel Keem Bay PWM driver + * + * Copyright (C) 2020 Intel Corporation + * Authors: Lai Poey Seng <poey.seng.lai@intel.com> + * Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> + * + * Limitations: + * - Upon disabling a channel, the currently running + * period will not be completed. However, upon + * reconfiguration of the duty cycle/period, the + * currently running period will be completed first. + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> + +#define KMB_TOTAL_PWM_CHANNELS 6 +#define KMB_PWM_COUNT_MAX U16_MAX +#define KMB_PWM_EN_BIT BIT(31) + +/* Mask */ +#define KMB_PWM_HIGH_MASK GENMASK(31, 16) +#define KMB_PWM_LOW_MASK GENMASK(15, 0) +#define KMB_PWM_LEADIN_MASK GENMASK(30, 0) + +/* PWM Register offset */ +#define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) +#define KMB_PWM_HIGHLOW_OFFSET(ch) (0x20 + 4 * (ch)) + +struct keembay_pwm { + struct pwm_chip chip; + struct device *dev; + struct clk *clk; + void __iomem *base; +}; + +static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip) +{ + return container_of(chip, struct keembay_pwm, chip); +} + +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask, + u32 val, u32 offset) +{ + u32 buff = readl(priv->base + offset); + + buff = u32_replace_bits(buff, val, mask); + writel(buff, priv->base + offset); +} + +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) +{ + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, + KMB_PWM_LEADIN_OFFSET(ch)); +} + +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) +{ + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0, + KMB_PWM_LEADIN_OFFSET(ch)); +} + +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); + unsigned long long high, low; + unsigned long clk_rate; + u32 highlow; + + clk_rate = clk_get_rate(priv->clk); + + /* Read channel enabled status */ + highlow = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); + if (highlow & KMB_PWM_EN_BIT) + state->enabled = true; + else + state->enabled = false; + + /* Read period and duty cycle */ + highlow = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); + low = FIELD_GET(KMB_PWM_LOW_MASK, highlow) * NSEC_PER_SEC; + high = FIELD_GET(KMB_PWM_HIGH_MASK, highlow) * NSEC_PER_SEC; + state->duty_cycle = DIV_ROUND_UP_ULL(high, clk_rate); + state->period = DIV_ROUND_UP_ULL(high + low, clk_rate); + state->polarity = PWM_POLARITY_NORMAL; +} + +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); + struct pwm_state current_state; + unsigned long long div; + unsigned long clk_rate; + u32 pwm_count = 0; + u16 high, low; + + if (state->polarity != PWM_POLARITY_NORMAL) + return -ENOSYS; + + /* + * Configure the pwm repeat count as infinite at (15:0) and leadin + * low time as 0 at (30:16), which is in terms of clock cycles. + */ + keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0, + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); + + keembay_pwm_get_state(chip, pwm, ¤t_state); + + if (!state->enabled) { + if (current_state.enabled) + keembay_pwm_disable(priv, pwm->hwpwm); + return 0; + } + + /* + * The upper 16 bits and lower 16 bits of the KMB_PWM_HIGHLOW_OFFSET + * register contain the high time and low time of waveform accordingly. + * All the values are in terms of clock cycles. + */ + + clk_rate = clk_get_rate(priv->clk); + div = clk_rate * state->duty_cycle; + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); + if (div > KMB_PWM_COUNT_MAX) + return -ERANGE; + + high = div; + div = clk_rate * state->period; + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); + div = div - high; + if (div > KMB_PWM_COUNT_MAX) + return -ERANGE; + + low = div; + + pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, high) | + FIELD_PREP(KMB_PWM_LOW_MASK, low); + + writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); + + if (state->enabled && !current_state.enabled) + keembay_pwm_enable(priv, pwm->hwpwm); + + return 0; +} + +static const struct pwm_ops keembay_pwm_ops = { + .owner = THIS_MODULE, + .apply = keembay_pwm_apply, + .get_state = keembay_pwm_get_state, +}; + +static int keembay_pwm_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct keembay_pwm *priv; + int ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->clk = devm_clk_get(dev, NULL); + if (IS_ERR(priv->clk)) + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock\n"); + + ret = clk_prepare_enable(priv->clk); + if (ret) + return ret; + + priv->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(priv->base)) { + clk_disable_unprepare(priv->clk); + return PTR_ERR(priv->base); + } + + priv->chip.base = -1; + priv->chip.dev = dev; + priv->chip.ops = &keembay_pwm_ops; + priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS; + + ret = pwmchip_add(&priv->chip); + if (ret) { + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret)); + clk_disable_unprepare(priv->clk); + return ret; + } + + platform_set_drvdata(pdev, priv); + + return 0; +} + +static int keembay_pwm_remove(struct platform_device *pdev) +{ + struct keembay_pwm *priv = platform_get_drvdata(pdev); + int ret; + + ret = pwmchip_remove(&priv->chip); + clk_disable_unprepare(priv->clk); + + return ret; +} + +static const struct of_device_id keembay_pwm_of_match[] = { + { .compatible = "intel,keembay-pwm" }, + { } +}; +MODULE_DEVICE_TABLE(of, keembay_pwm_of_match); + +static struct platform_driver keembay_pwm_driver = { + .probe = keembay_pwm_probe, + .remove = keembay_pwm_remove, + .driver = { + .name = "pwm-keembay", + .of_match_table = keembay_pwm_of_match, + }, +}; +module_platform_driver(keembay_pwm_driver); + +MODULE_ALIAS("platform:pwm-keembay"); +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); +MODULE_LICENSE("GPL v2"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-14 19:36 ` [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay vijayakannan.ayyathurai @ 2020-10-14 20:04 ` Uwe Kleine-König 2020-10-15 10:42 ` Andy Shevchenko 1 sibling, 0 replies; 17+ messages in thread From: Uwe Kleine-König @ 2020-10-14 20:04 UTC (permalink / raw) To: vijayakannan.ayyathurai Cc: thierry.reding, robh+dt, linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko, mgross, lakshmi.bai.raja.subramanian [-- Attachment #1: Type: text/plain, Size: 923 bytes --] On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote: > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > > The Intel Keem Bay SoC requires PWM support. > Add the pwm-keembay driver to enable this. > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> > Co-developed-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks for your persistence to create a great driver. 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] 17+ messages in thread
* Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-14 19:36 ` [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay vijayakannan.ayyathurai 2020-10-14 20:04 ` Uwe Kleine-König @ 2020-10-15 10:42 ` Andy Shevchenko 2020-10-16 3:18 ` Ayyathurai, Vijayakannan 2020-10-16 6:24 ` Uwe Kleine-König 1 sibling, 2 replies; 17+ messages in thread From: Andy Shevchenko @ 2020-10-15 10:42 UTC (permalink / raw) To: vijayakannan.ayyathurai Cc: thierry.reding, u.kleine-koenig, robh+dt, linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, mgross, lakshmi.bai.raja.subramanian On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote: > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > > The Intel Keem Bay SoC requires PWM support. > Add the pwm-keembay driver to enable this. > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> Missed Co-developed-by? > Co-developed-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-keembay.c | 233 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 243 insertions(+) > create mode 100644 drivers/pwm/pwm-keembay.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 7dbcf6973d33..6129a9dbbfa8 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -254,6 +254,15 @@ config PWM_JZ4740 > To compile this driver as a module, choose M here: the module > will be called pwm-jz4740. > > +config PWM_KEEMBAY > + tristate "Intel Keem Bay PWM driver" > + depends on ARCH_KEEMBAY || COMPILE_TEST > + help > + The platform driver for Intel Keem Bay PWM controller. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-keembay. > + > config PWM_LP3943 > tristate "TI/National Semiconductor LP3943 PWM support" > depends on MFD_LP3943 > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 2c2ba0a03557..a1051122eb07 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c > new file mode 100644 > index 000000000000..ced6d4010add > --- /dev/null > +++ b/drivers/pwm/pwm-keembay.c > @@ -0,0 +1,233 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Intel Keem Bay PWM driver > + * > + * Copyright (C) 2020 Intel Corporation > + * Authors: Lai Poey Seng <poey.seng.lai@intel.com> > + * Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > + * > + * Limitations: > + * - Upon disabling a channel, the currently running > + * period will not be completed. However, upon > + * reconfiguration of the duty cycle/period, the > + * currently running period will be completed first. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > + > +#define KMB_TOTAL_PWM_CHANNELS 6 > +#define KMB_PWM_COUNT_MAX U16_MAX > +#define KMB_PWM_EN_BIT BIT(31) > + > +/* Mask */ > +#define KMB_PWM_HIGH_MASK GENMASK(31, 16) > +#define KMB_PWM_LOW_MASK GENMASK(15, 0) > +#define KMB_PWM_LEADIN_MASK GENMASK(30, 0) > + > +/* PWM Register offset */ > +#define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) > +#define KMB_PWM_HIGHLOW_OFFSET(ch) (0x20 + 4 * (ch)) > + > +struct keembay_pwm { > + struct pwm_chip chip; > + struct device *dev; > + struct clk *clk; > + void __iomem *base; > +}; > + > +static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip) > +{ > + return container_of(chip, struct keembay_pwm, chip); > +} > + > +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask, > + u32 val, u32 offset) > +{ > + u32 buff = readl(priv->base + offset); > + > + buff = u32_replace_bits(buff, val, mask); > + writel(buff, priv->base + offset); > +} > + > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) > +{ > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > + KMB_PWM_LEADIN_OFFSET(ch)); > +} > + > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) > +{ > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0, > + KMB_PWM_LEADIN_OFFSET(ch)); > +} > + > +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + unsigned long long high, low; > + unsigned long clk_rate; > + u32 highlow; > + > + clk_rate = clk_get_rate(priv->clk); > + > + /* Read channel enabled status */ > + highlow = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); > + if (highlow & KMB_PWM_EN_BIT) > + state->enabled = true; > + else > + state->enabled = false; > + > + /* Read period and duty cycle */ > + highlow = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); > + low = FIELD_GET(KMB_PWM_LOW_MASK, highlow) * NSEC_PER_SEC; > + high = FIELD_GET(KMB_PWM_HIGH_MASK, highlow) * NSEC_PER_SEC; > + state->duty_cycle = DIV_ROUND_UP_ULL(high, clk_rate); > + state->period = DIV_ROUND_UP_ULL(high + low, clk_rate); > + state->polarity = PWM_POLARITY_NORMAL; > +} > + > +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + struct pwm_state current_state; > + unsigned long long div; > + unsigned long clk_rate; > + u32 pwm_count = 0; > + u16 high, low; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOSYS; > + > + /* > + * Configure the pwm repeat count as infinite at (15:0) and leadin > + * low time as 0 at (30:16), which is in terms of clock cycles. > + */ > + keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0, > + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); > + > + keembay_pwm_get_state(chip, pwm, ¤t_state); > + > + if (!state->enabled) { > + if (current_state.enabled) > + keembay_pwm_disable(priv, pwm->hwpwm); > + return 0; > + } > + > + /* > + * The upper 16 bits and lower 16 bits of the KMB_PWM_HIGHLOW_OFFSET > + * register contain the high time and low time of waveform accordingly. > + * All the values are in terms of clock cycles. > + */ > + > + clk_rate = clk_get_rate(priv->clk); > + div = clk_rate * state->duty_cycle; > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + high = div; > + div = clk_rate * state->period; > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > + div = div - high; > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + low = div; > + > + pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, high) | > + FIELD_PREP(KMB_PWM_LOW_MASK, low); > + > + writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); > + > + if (state->enabled && !current_state.enabled) > + keembay_pwm_enable(priv, pwm->hwpwm); > + > + return 0; > +} > + > +static const struct pwm_ops keembay_pwm_ops = { > + .owner = THIS_MODULE, > + .apply = keembay_pwm_apply, > + .get_state = keembay_pwm_get_state, > +}; > + > +static int keembay_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct keembay_pwm *priv; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(priv->clk)) > + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock\n"); > + > + ret = clk_prepare_enable(priv->clk); > + if (ret) > + return ret; > + > + priv->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(priv->base)) { > + clk_disable_unprepare(priv->clk); See below. > + return PTR_ERR(priv->base); > + } > + > + priv->chip.base = -1; > + priv->chip.dev = dev; > + priv->chip.ops = &keembay_pwm_ops; > + priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS; > + > + ret = pwmchip_add(&priv->chip); > + if (ret) { > + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret)); > + clk_disable_unprepare(priv->clk); This messes up with ordering of things. That's why devm golden rule is either all or none. You may fix this by switching to devm_add_action_or_reset(). One of possible way is like in below drivers: % git grep -n devm_add_action_or_reset.*disable_unprepare -- drivers/ But it may be fixed in follow up change. Depends on maintainers' wishes. > + return ret; > + } > + > + platform_set_drvdata(pdev, priv); > + > + return 0; > +} > + > +static int keembay_pwm_remove(struct platform_device *pdev) > +{ > + struct keembay_pwm *priv = platform_get_drvdata(pdev); > + int ret; > + > + ret = pwmchip_remove(&priv->chip); > + clk_disable_unprepare(priv->clk); > + > + return ret; ...and this will be simplified to return pwmchip_remove(&priv->chip); > +} > + > +static const struct of_device_id keembay_pwm_of_match[] = { > + { .compatible = "intel,keembay-pwm" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, keembay_pwm_of_match); > + > +static struct platform_driver keembay_pwm_driver = { > + .probe = keembay_pwm_probe, > + .remove = keembay_pwm_remove, > + .driver = { > + .name = "pwm-keembay", > + .of_match_table = keembay_pwm_of_match, > + }, > +}; > +module_platform_driver(keembay_pwm_driver); > + > +MODULE_ALIAS("platform:pwm-keembay"); > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.17.1 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-15 10:42 ` Andy Shevchenko @ 2020-10-16 3:18 ` Ayyathurai, Vijayakannan 2020-10-16 7:34 ` Uwe Kleine-König 2020-10-16 9:21 ` Andy Shevchenko 2020-10-16 6:24 ` Uwe Kleine-König 1 sibling, 2 replies; 17+ messages in thread From: Ayyathurai, Vijayakannan @ 2020-10-16 3:18 UTC (permalink / raw) To: Andy Shevchenko Cc: thierry.reding, u.kleine-koenig, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, mgross, Raja Subramanian, Lakshmi Bai Hi Andy, -----Original Message----- From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Sent: Thursday, 15 October, 2020 4:12 PM To: Ayyathurai, Vijayakannan <vijayakannan.ayyathurai@intel.com> Cc: thierry.reding@gmail.com; u.kleine-koenig@pengutronix.de; robh+dt@kernel.org; linux-pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; mgross@linux.intel.com; Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com> Subject: Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote: > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > > The Intel Keem Bay SoC requires PWM support. > Add the pwm-keembay driver to enable this. > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> Missed Co-developed-by? Sure. I will add my Co-developed-by tag. > Co-developed-by: Vineetha G. Jaya Kumaran > <vineetha.g.jaya.kumaran@intel.com> > Signed-off-by: Vineetha G. Jaya Kumaran > <vineetha.g.jaya.kumaran@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Vijayakannan Ayyathurai > <vijayakannan.ayyathurai@intel.com> > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-keembay.c | 233 > ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 243 insertions(+) > create mode 100644 drivers/pwm/pwm-keembay.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > 7dbcf6973d33..6129a9dbbfa8 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -254,6 +254,15 @@ config PWM_JZ4740 > To compile this driver as a module, choose M here: the module > will be called pwm-jz4740. > > +config PWM_KEEMBAY > + tristate "Intel Keem Bay PWM driver" > + depends on ARCH_KEEMBAY || COMPILE_TEST > + help > + The platform driver for Intel Keem Bay PWM controller. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-keembay. > + > config PWM_LP3943 > tristate "TI/National Semiconductor LP3943 PWM support" > depends on MFD_LP3943 > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index > 2c2ba0a03557..a1051122eb07 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c new > file mode 100644 index 000000000000..ced6d4010add > --- /dev/null > +++ b/drivers/pwm/pwm-keembay.c > @@ -0,0 +1,233 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Intel Keem Bay PWM driver > + * > + * Copyright (C) 2020 Intel Corporation > + * Authors: Lai Poey Seng <poey.seng.lai@intel.com> > + * Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > + * > + * Limitations: > + * - Upon disabling a channel, the currently running > + * period will not be completed. However, upon > + * reconfiguration of the duty cycle/period, the > + * currently running period will be completed first. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > + > +#define KMB_TOTAL_PWM_CHANNELS 6 > +#define KMB_PWM_COUNT_MAX U16_MAX > +#define KMB_PWM_EN_BIT BIT(31) > + > +/* Mask */ > +#define KMB_PWM_HIGH_MASK GENMASK(31, 16) > +#define KMB_PWM_LOW_MASK GENMASK(15, 0) > +#define KMB_PWM_LEADIN_MASK GENMASK(30, 0) > + > +/* PWM Register offset */ > +#define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) > +#define KMB_PWM_HIGHLOW_OFFSET(ch) (0x20 + 4 * (ch)) > + > +struct keembay_pwm { > + struct pwm_chip chip; > + struct device *dev; > + struct clk *clk; > + void __iomem *base; > +}; > + > +static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip > +*chip) { > + return container_of(chip, struct keembay_pwm, chip); } > + > +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask, > + u32 val, u32 offset) > +{ > + u32 buff = readl(priv->base + offset); > + > + buff = u32_replace_bits(buff, val, mask); > + writel(buff, priv->base + offset); > +} > + > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) { > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > + KMB_PWM_LEADIN_OFFSET(ch)); > +} > + > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) { > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0, > + KMB_PWM_LEADIN_OFFSET(ch)); > +} > + > +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + unsigned long long high, low; > + unsigned long clk_rate; > + u32 highlow; > + > + clk_rate = clk_get_rate(priv->clk); > + > + /* Read channel enabled status */ > + highlow = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); > + if (highlow & KMB_PWM_EN_BIT) > + state->enabled = true; > + else > + state->enabled = false; > + > + /* Read period and duty cycle */ > + highlow = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); > + low = FIELD_GET(KMB_PWM_LOW_MASK, highlow) * NSEC_PER_SEC; > + high = FIELD_GET(KMB_PWM_HIGH_MASK, highlow) * NSEC_PER_SEC; > + state->duty_cycle = DIV_ROUND_UP_ULL(high, clk_rate); > + state->period = DIV_ROUND_UP_ULL(high + low, clk_rate); > + state->polarity = PWM_POLARITY_NORMAL; } > + > +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) { > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > + struct pwm_state current_state; > + unsigned long long div; > + unsigned long clk_rate; > + u32 pwm_count = 0; > + u16 high, low; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOSYS; > + > + /* > + * Configure the pwm repeat count as infinite at (15:0) and leadin > + * low time as 0 at (30:16), which is in terms of clock cycles. > + */ > + keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0, > + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); > + > + keembay_pwm_get_state(chip, pwm, ¤t_state); > + > + if (!state->enabled) { > + if (current_state.enabled) > + keembay_pwm_disable(priv, pwm->hwpwm); > + return 0; > + } > + > + /* > + * The upper 16 bits and lower 16 bits of the KMB_PWM_HIGHLOW_OFFSET > + * register contain the high time and low time of waveform accordingly. > + * All the values are in terms of clock cycles. > + */ > + > + clk_rate = clk_get_rate(priv->clk); > + div = clk_rate * state->duty_cycle; > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + high = div; > + div = clk_rate * state->period; > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > + div = div - high; > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + low = div; > + > + pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, high) | > + FIELD_PREP(KMB_PWM_LOW_MASK, low); > + > + writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); > + > + if (state->enabled && !current_state.enabled) > + keembay_pwm_enable(priv, pwm->hwpwm); > + > + return 0; > +} > + > +static const struct pwm_ops keembay_pwm_ops = { > + .owner = THIS_MODULE, > + .apply = keembay_pwm_apply, > + .get_state = keembay_pwm_get_state, > +}; > + > +static int keembay_pwm_probe(struct platform_device *pdev) { > + struct device *dev = &pdev->dev; > + struct keembay_pwm *priv; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(priv->clk)) > + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get > +clock\n"); > + > + ret = clk_prepare_enable(priv->clk); > + if (ret) > + return ret; > + > + priv->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(priv->base)) { > + clk_disable_unprepare(priv->clk); See below. > + return PTR_ERR(priv->base); > + } > + > + priv->chip.base = -1; > + priv->chip.dev = dev; > + priv->chip.ops = &keembay_pwm_ops; > + priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS; > + > + ret = pwmchip_add(&priv->chip); > + if (ret) { > + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret)); > + clk_disable_unprepare(priv->clk); This messes up with ordering of things. That's why devm golden rule is either all or none. You may fix this by switching to devm_add_action_or_reset(). One of possible way is like in below drivers: % git grep -n devm_add_action_or_reset.*disable_unprepare -- drivers/ But it may be fixed in follow up change. Depends on maintainers' wishes. Sure. I shall incorporate and check based on maintainers wish in the next version. > + return ret; > + } > + > + platform_set_drvdata(pdev, priv); > + > + return 0; > +} > + > +static int keembay_pwm_remove(struct platform_device *pdev) { > + struct keembay_pwm *priv = platform_get_drvdata(pdev); > + int ret; > + > + ret = pwmchip_remove(&priv->chip); > + clk_disable_unprepare(priv->clk); > + > + return ret; ...and this will be simplified to return pwmchip_remove(&priv->chip); Until v10, It is as per your suggestion. But I have changed it in v11 to overcome the issue mentioned by Uwe. I have kept the snip of v10 FYR below. //Start snip from v10 review mailing list //> +static int keembay_pwm_remove(struct platform_device *pdev) { //> + struct keembay_pwm *priv = platform_get_drvdata(pdev); //> + //> + clk_disable_unprepare(priv->clk); //> + //> + return pwmchip_remove(&priv->chip); // //You have to call pwmchip_remove first. Otherwise you're stopping the PWM while the framework still believes everything to be fine. // //> +} //End snip from v10 review mailing review > +} > + > +static const struct of_device_id keembay_pwm_of_match[] = { > + { .compatible = "intel,keembay-pwm" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, keembay_pwm_of_match); > + > +static struct platform_driver keembay_pwm_driver = { > + .probe = keembay_pwm_probe, > + .remove = keembay_pwm_remove, > + .driver = { > + .name = "pwm-keembay", > + .of_match_table = keembay_pwm_of_match, > + }, > +}; > +module_platform_driver(keembay_pwm_driver); > + > +MODULE_ALIAS("platform:pwm-keembay"); > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); MODULE_LICENSE("GPL > +v2"); > -- > 2.17.1 > -- With Best Regards, Andy Shevchenko Thanks, Vijay ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-16 3:18 ` Ayyathurai, Vijayakannan @ 2020-10-16 7:34 ` Uwe Kleine-König 2020-10-16 8:32 ` Ayyathurai, Vijayakannan 2020-10-19 5:44 ` Ayyathurai, Vijayakannan 2020-10-16 9:21 ` Andy Shevchenko 1 sibling, 2 replies; 17+ messages in thread From: Uwe Kleine-König @ 2020-10-16 7:34 UTC (permalink / raw) To: Ayyathurai, Vijayakannan Cc: Andy Shevchenko, thierry.reding, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, mgross, Raja Subramanian, Lakshmi Bai [-- Attachment #1: Type: text/plain, Size: 2016 bytes --] Hello Ayyathurai, Can you please fix your MUA to properly quote when replying, this is really annoying. On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote: > > On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote: > > > +static int keembay_pwm_remove(struct platform_device *pdev) { > > > + struct keembay_pwm *priv = platform_get_drvdata(pdev); > > > + int ret; > > > + > > > + ret = pwmchip_remove(&priv->chip); > > > + clk_disable_unprepare(priv->clk); > > > + > > > + return ret; > > > > ...and this will be simplified to > > > > return pwmchip_remove(&priv->chip); > > Until v10, It is as per your suggestion. But I have changed it in v11 > to overcome the issue mentioned by Uwe. I have kept the snip of v10 > FYR below. > > //Start snip from v10 review mailing list > //> +static int keembay_pwm_remove(struct platform_device *pdev) { > //> + struct keembay_pwm *priv = platform_get_drvdata(pdev); > //> + > //> + clk_disable_unprepare(priv->clk); > //> + > //> + return pwmchip_remove(&priv->chip); > // > //You have to call pwmchip_remove first. Otherwise you're stopping the PWM while the framework still believes everything to be fine. > // > //> +} > //End snip from v10 review mailing review Note that we're both (Andy and I) are right. You must not disable the clocks before pwmchip_remove() (otherwise for a short time the PWM looks ready but isn't). And if you use devm-stuff to enable the clock it will be disabled only after the remove callback completed and your .remove may look like: static int keembay_pwm_remove(struct platform_device *pdev) { struct keembay_pwm *priv = platform_get_drvdata(pdev); return pwmchip_remove(&priv->chip); } because you won't have to care for the clock explicitly. 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] 17+ messages in thread
* RE: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-16 7:34 ` Uwe Kleine-König @ 2020-10-16 8:32 ` Ayyathurai, Vijayakannan 2020-10-16 9:25 ` Andy Shevchenko 2020-10-19 5:44 ` Ayyathurai, Vijayakannan 1 sibling, 1 reply; 17+ messages in thread From: Ayyathurai, Vijayakannan @ 2020-10-16 8:32 UTC (permalink / raw) To: Uwe Kleine-König Cc: Andy Shevchenko, thierry.reding, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, mgross, Raja Subramanian, Lakshmi Bai Hi Uwe, -----Original Message----- From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Sent: Friday, 16 October, 2020 1:04 PM To: Ayyathurai, Vijayakannan <vijayakannan.ayyathurai@intel.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; thierry.reding@gmail.com; robh+dt@kernel.org; linux-pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; mgross@linux.intel.com; Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com> Subject: Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay Hello Ayyathurai, Can you please fix your MUA to properly quote when replying, this is really annoying. Sorry for that MUA (Made Up Acronym). On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote: > > On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote: > > > +static int keembay_pwm_remove(struct platform_device *pdev) { > > > + struct keembay_pwm *priv = platform_get_drvdata(pdev); > > > + int ret; > > > + > > > + ret = pwmchip_remove(&priv->chip); > > > + clk_disable_unprepare(priv->clk); > > > + > > > + return ret; > > > > ...and this will be simplified to > > > > return pwmchip_remove(&priv->chip); > > Until v10, It is as per your suggestion. But I have changed it in v11 > to overcome the issue mentioned by Uwe. I have kept the snip of v10 > FYR below. > > //Start snip from v10 review mailing list //> +static int > keembay_pwm_remove(struct platform_device *pdev) { > //> + struct keembay_pwm *priv = platform_get_drvdata(pdev); > //> + > //> + clk_disable_unprepare(priv->clk); > //> + > //> + return pwmchip_remove(&priv->chip); > // > //You have to call pwmchip_remove first. Otherwise you're stopping the PWM while the framework still believes everything to be fine. > // > //> +} > //End snip from v10 review mailing review Note that we're both (Andy and I) are right. You must not disable the clocks before pwmchip_remove() (otherwise for a short time the PWM looks ready but isn't). And if you use devm-stuff to enable the clock it will be disabled only after the remove callback completed and your .remove may look like: static int keembay_pwm_remove(struct platform_device *pdev) { struct keembay_pwm *priv = platform_get_drvdata(pdev); return pwmchip_remove(&priv->chip); } because you won't have to care for the clock explicitly. OK. I will incorporate it in the next version. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | Thanks, Vijay ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-16 8:32 ` Ayyathurai, Vijayakannan @ 2020-10-16 9:25 ` Andy Shevchenko 0 siblings, 0 replies; 17+ messages in thread From: Andy Shevchenko @ 2020-10-16 9:25 UTC (permalink / raw) To: Ayyathurai, Vijayakannan Cc: Uwe Kleine-König, thierry.reding, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, mgross, Raja Subramanian, Lakshmi Bai On Fri, Oct 16, 2020 at 08:32:05AM +0000, Ayyathurai, Vijayakannan wrote: > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Sent: Friday, 16 October, 2020 1:04 PM > Hello Ayyathurai, > > Can you please fix your MUA to properly quote when replying, this is really annoying. > > Sorry for that MUA (Made Up Acronym). MUA = Mail User Agent. And your quoting is a mess. Please, fix this for any replies in OSS mailing lists. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-16 7:34 ` Uwe Kleine-König 2020-10-16 8:32 ` Ayyathurai, Vijayakannan @ 2020-10-19 5:44 ` Ayyathurai, Vijayakannan 2020-10-19 6:42 ` Uwe Kleine-König 1 sibling, 1 reply; 17+ messages in thread From: Ayyathurai, Vijayakannan @ 2020-10-19 5:44 UTC (permalink / raw) To: Uwe Kleine-König Cc: Andy Shevchenko, thierry.reding, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, mgross, Raja Subramanian, Lakshmi Bai Hi Uwe, > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Sent: Friday, 16 October, 2020 1:04 PM > Subject: Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay > > Hello Ayyathurai, > Note that we're both (Andy and I) are right. You must not disable the clocks > before pwmchip_remove() (otherwise for a short time the PWM looks ready > but isn't). And if you use devm-stuff to enable the clock it will be disabled only > after the remove callback completed and your .remove may look like: > > static int keembay_pwm_remove(struct platform_device *pdev) > { > struct keembay_pwm *priv = platform_get_drvdata(pdev); > > return pwmchip_remove(&priv->chip); > } > > because you won't have to care for the clock explicitly. Sure. I will incorporate the same in the next version. Also, Is it fine not to care clock during pwmchip_add like below, ret = pwmchip_add(&priv->chip); if (ret) return dev_err_probe(dev, ret, "Failed to add PWM chip\n"); > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | Thanks, Vijay ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-19 5:44 ` Ayyathurai, Vijayakannan @ 2020-10-19 6:42 ` Uwe Kleine-König 0 siblings, 0 replies; 17+ messages in thread From: Uwe Kleine-König @ 2020-10-19 6:42 UTC (permalink / raw) To: Ayyathurai, Vijayakannan Cc: Andy Shevchenko, thierry.reding, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, mgross, Raja Subramanian, Lakshmi Bai [-- Attachment #1: Type: text/plain, Size: 1543 bytes --] On Mon, Oct 19, 2020 at 05:44:36AM +0000, Ayyathurai, Vijayakannan wrote: > Hi Uwe, > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Sent: Friday, 16 October, 2020 1:04 PM > > Subject: Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay > > > > Hello Ayyathurai, > > Note that we're both (Andy and I) are right. You must not disable the clocks > > before pwmchip_remove() (otherwise for a short time the PWM looks ready > > but isn't). And if you use devm-stuff to enable the clock it will be disabled only > > after the remove callback completed and your .remove may look like: > > > > static int keembay_pwm_remove(struct platform_device *pdev) > > { > > struct keembay_pwm *priv = platform_get_drvdata(pdev); > > > > return pwmchip_remove(&priv->chip); > > } > > > > because you won't have to care for the clock explicitly. > > Sure. I will incorporate the same in the next version. > > Also, Is it fine not to care clock during pwmchip_add like below, > > ret = pwmchip_add(&priv->chip); > if (ret) > return dev_err_probe(dev, ret, "Failed to add PWM chip\n"); Yes, if you cared for disabling the clk using devm_* you don't need (even: must not) disable the clock in the error path because the devm release functions are also called on .probe() failing. 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] 17+ messages in thread
* Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-16 3:18 ` Ayyathurai, Vijayakannan 2020-10-16 7:34 ` Uwe Kleine-König @ 2020-10-16 9:21 ` Andy Shevchenko 2020-10-19 6:46 ` Uwe Kleine-König 1 sibling, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2020-10-16 9:21 UTC (permalink / raw) To: Ayyathurai, Vijayakannan Cc: thierry.reding, u.kleine-koenig, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, mgross, Raja Subramanian, Lakshmi Bai On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote: > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Thursday, 15 October, 2020 4:12 PM > On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote: > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> ... > > + priv->clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(priv->clk)) > > + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get > > +clock\n"); > > + > > + ret = clk_prepare_enable(priv->clk); > > + if (ret) > > + return ret; > > + > > + priv->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(priv->base)) { > > > + clk_disable_unprepare(priv->clk); > > See below. > > > + return PTR_ERR(priv->base); > > + } > > + > > + priv->chip.base = -1; > > + priv->chip.dev = dev; > > + priv->chip.ops = &keembay_pwm_ops; > > + priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS; > > + > > + ret = pwmchip_add(&priv->chip); > > + if (ret) { > > + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret)); > > > + clk_disable_unprepare(priv->clk); > > This messes up with ordering of things. > > That's why devm golden rule is either all or none. You may fix this by switching to devm_add_action_or_reset(). > > One of possible way is like in below drivers: > > % git grep -n devm_add_action_or_reset.*disable_unprepare -- drivers/ > > But it may be fixed in follow up change. Depends on maintainers' wishes. > > Sure. I shall incorporate and check based on maintainers wish in the next version. > > > + return ret; > > + } > > + > > + platform_set_drvdata(pdev, priv); > > + > > + return 0; > > +} > > + > > +static int keembay_pwm_remove(struct platform_device *pdev) { > > + struct keembay_pwm *priv = platform_get_drvdata(pdev); > > + int ret; > > + > > + ret = pwmchip_remove(&priv->chip); > > + clk_disable_unprepare(priv->clk); > > + > > + return ret; > > ...and this will be simplified to > > return pwmchip_remove(&priv->chip); > > Until v10, It is as per your suggestion. But I have changed it in v11 to overcome the issue mentioned by Uwe. I have kept the snip of v10 FYR below. > > //Start snip from v10 review mailing list > //> +static int keembay_pwm_remove(struct platform_device *pdev) { > //> + struct keembay_pwm *priv = platform_get_drvdata(pdev); > //> + > //> + clk_disable_unprepare(priv->clk); > //> + > //> + return pwmchip_remove(&priv->chip); > // > //You have to call pwmchip_remove first. Otherwise you're stopping the PWM while the framework still believes everything to be fine. > // > //> +} > //End snip from v10 review mailing review > > > +} What I said does not contradict with what Uwe said. So, please, fix ordering either by dropping devm_ in the middle or adding devm_ action. Now you moved serious ordering issue in ->remove() (which Uwe noted) to less serious in ->probe(). But issue is still present. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-16 9:21 ` Andy Shevchenko @ 2020-10-19 6:46 ` Uwe Kleine-König 0 siblings, 0 replies; 17+ messages in thread From: Uwe Kleine-König @ 2020-10-19 6:46 UTC (permalink / raw) To: Andy Shevchenko Cc: Ayyathurai, Vijayakannan, thierry.reding, robh+dt, linux-pwm, devicetree, Wan Mohamad, Wan Ahmad Zainie, mgross, Raja Subramanian, Lakshmi Bai [-- Attachment #1: Type: text/plain, Size: 3650 bytes --] On Fri, Oct 16, 2020 at 12:21:27PM +0300, Andy Shevchenko wrote: > On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote: > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Sent: Thursday, 15 October, 2020 4:12 PM > > On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote: > > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > > ... > > > > + priv->clk = devm_clk_get(dev, NULL); > > > + if (IS_ERR(priv->clk)) > > > + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get > > > +clock\n"); > > > + > > > + ret = clk_prepare_enable(priv->clk); > > > + if (ret) > > > + return ret; > > > + > > > + priv->base = devm_platform_ioremap_resource(pdev, 0); > > > + if (IS_ERR(priv->base)) { > > > > > + clk_disable_unprepare(priv->clk); > > > > See below. > > > > > + return PTR_ERR(priv->base); > > > + } > > > + > > > + priv->chip.base = -1; > > > + priv->chip.dev = dev; > > > + priv->chip.ops = &keembay_pwm_ops; > > > + priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS; > > > + > > > + ret = pwmchip_add(&priv->chip); > > > + if (ret) { > > > + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret)); > > > > > + clk_disable_unprepare(priv->clk); > > > > This messes up with ordering of things. > > > > That's why devm golden rule is either all or none. You may fix this by switching to devm_add_action_or_reset(). > > > > One of possible way is like in below drivers: > > > > % git grep -n devm_add_action_or_reset.*disable_unprepare -- drivers/ > > > > But it may be fixed in follow up change. Depends on maintainers' wishes. > > > > Sure. I shall incorporate and check based on maintainers wish in the next version. > > > > > + return ret; > > > + } > > > + > > > + platform_set_drvdata(pdev, priv); > > > + > > > + return 0; > > > +} > > > + > > > +static int keembay_pwm_remove(struct platform_device *pdev) { > > > + struct keembay_pwm *priv = platform_get_drvdata(pdev); > > > + int ret; > > > + > > > + ret = pwmchip_remove(&priv->chip); > > > + clk_disable_unprepare(priv->clk); > > > + > > > + return ret; > > > > ...and this will be simplified to > > > > return pwmchip_remove(&priv->chip); > > > > Until v10, It is as per your suggestion. But I have changed it in v11 to overcome the issue mentioned by Uwe. I have kept the snip of v10 FYR below. > > > > //Start snip from v10 review mailing list > > //> +static int keembay_pwm_remove(struct platform_device *pdev) { > > //> + struct keembay_pwm *priv = platform_get_drvdata(pdev); > > //> + > > //> + clk_disable_unprepare(priv->clk); > > //> + > > //> + return pwmchip_remove(&priv->chip); > > // > > //You have to call pwmchip_remove first. Otherwise you're stopping the PWM while the framework still believes everything to be fine. > > // > > //> +} > > //End snip from v10 review mailing review > > > > > +} > > What I said does not contradict with what Uwe said. So, please, fix ordering > either by dropping devm_ in the middle or adding devm_ action. > > Now you moved serious ordering issue in ->remove() (which Uwe noted) to less > serious in ->probe(). But issue is still present. The easiest way is probably to do the clk_prepare_enable() only after all devm_ stuff in .probe. (But I'm not emphatic about that, it would be fine for me to wait until devm_clk_get_enabled() lands.) 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] 17+ messages in thread
* Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-15 10:42 ` Andy Shevchenko 2020-10-16 3:18 ` Ayyathurai, Vijayakannan @ 2020-10-16 6:24 ` Uwe Kleine-König 2020-10-16 9:24 ` Andy Shevchenko 1 sibling, 1 reply; 17+ messages in thread From: Uwe Kleine-König @ 2020-10-16 6:24 UTC (permalink / raw) To: Andy Shevchenko Cc: vijayakannan.ayyathurai, thierry.reding, robh+dt, linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, mgross, lakshmi.bai.raja.subramanian [-- Attachment #1: Type: text/plain, Size: 9989 bytes --] On Thu, Oct 15, 2020 at 01:42:17PM +0300, Andy Shevchenko wrote: > On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote: > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > > > > The Intel Keem Bay SoC requires PWM support. > > Add the pwm-keembay driver to enable this. > > > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> > > Missed Co-developed-by? > > > Co-developed-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > > --- > > drivers/pwm/Kconfig | 9 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-keembay.c | 233 ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 243 insertions(+) > > create mode 100644 drivers/pwm/pwm-keembay.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index 7dbcf6973d33..6129a9dbbfa8 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -254,6 +254,15 @@ config PWM_JZ4740 > > To compile this driver as a module, choose M here: the module > > will be called pwm-jz4740. > > > > +config PWM_KEEMBAY > > + tristate "Intel Keem Bay PWM driver" > > + depends on ARCH_KEEMBAY || COMPILE_TEST > > + help > > + The platform driver for Intel Keem Bay PWM controller. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-keembay. > > + > > config PWM_LP3943 > > tristate "TI/National Semiconductor LP3943 PWM support" > > depends on MFD_LP3943 > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > index 2c2ba0a03557..a1051122eb07 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o > > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c > > new file mode 100644 > > index 000000000000..ced6d4010add > > --- /dev/null > > +++ b/drivers/pwm/pwm-keembay.c > > @@ -0,0 +1,233 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Intel Keem Bay PWM driver > > + * > > + * Copyright (C) 2020 Intel Corporation > > + * Authors: Lai Poey Seng <poey.seng.lai@intel.com> > > + * Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > > + * > > + * Limitations: > > + * - Upon disabling a channel, the currently running > > + * period will not be completed. However, upon > > + * reconfiguration of the duty cycle/period, the > > + * currently running period will be completed first. > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/clk.h> > > +#include <linux/io.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/regmap.h> > > + > > +#define KMB_TOTAL_PWM_CHANNELS 6 > > +#define KMB_PWM_COUNT_MAX U16_MAX > > +#define KMB_PWM_EN_BIT BIT(31) > > + > > +/* Mask */ > > +#define KMB_PWM_HIGH_MASK GENMASK(31, 16) > > +#define KMB_PWM_LOW_MASK GENMASK(15, 0) > > +#define KMB_PWM_LEADIN_MASK GENMASK(30, 0) > > + > > +/* PWM Register offset */ > > +#define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) > > +#define KMB_PWM_HIGHLOW_OFFSET(ch) (0x20 + 4 * (ch)) > > + > > +struct keembay_pwm { > > + struct pwm_chip chip; > > + struct device *dev; > > + struct clk *clk; > > + void __iomem *base; > > +}; > > + > > +static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip) > > +{ > > + return container_of(chip, struct keembay_pwm, chip); > > +} > > + > > +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask, > > + u32 val, u32 offset) > > +{ > > + u32 buff = readl(priv->base + offset); > > + > > + buff = u32_replace_bits(buff, val, mask); > > + writel(buff, priv->base + offset); > > +} > > + > > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) > > +{ > > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > > + KMB_PWM_LEADIN_OFFSET(ch)); > > +} > > + > > +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch) > > +{ > > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0, > > + KMB_PWM_LEADIN_OFFSET(ch)); > > +} > > + > > +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > > + unsigned long long high, low; > > + unsigned long clk_rate; > > + u32 highlow; > > + > > + clk_rate = clk_get_rate(priv->clk); > > + > > + /* Read channel enabled status */ > > + highlow = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); > > + if (highlow & KMB_PWM_EN_BIT) > > + state->enabled = true; > > + else > > + state->enabled = false; > > + > > + /* Read period and duty cycle */ > > + highlow = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); > > + low = FIELD_GET(KMB_PWM_LOW_MASK, highlow) * NSEC_PER_SEC; > > + high = FIELD_GET(KMB_PWM_HIGH_MASK, highlow) * NSEC_PER_SEC; > > + state->duty_cycle = DIV_ROUND_UP_ULL(high, clk_rate); > > + state->period = DIV_ROUND_UP_ULL(high + low, clk_rate); > > + state->polarity = PWM_POLARITY_NORMAL; > > +} > > + > > +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + const struct pwm_state *state) > > +{ > > + struct keembay_pwm *priv = to_keembay_pwm_dev(chip); > > + struct pwm_state current_state; > > + unsigned long long div; > > + unsigned long clk_rate; > > + u32 pwm_count = 0; > > + u16 high, low; > > + > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -ENOSYS; > > + > > + /* > > + * Configure the pwm repeat count as infinite at (15:0) and leadin > > + * low time as 0 at (30:16), which is in terms of clock cycles. > > + */ > > + keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0, > > + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm)); > > + > > + keembay_pwm_get_state(chip, pwm, ¤t_state); > > + > > + if (!state->enabled) { > > + if (current_state.enabled) > > + keembay_pwm_disable(priv, pwm->hwpwm); > > + return 0; > > + } > > + > > + /* > > + * The upper 16 bits and lower 16 bits of the KMB_PWM_HIGHLOW_OFFSET > > + * register contain the high time and low time of waveform accordingly. > > + * All the values are in terms of clock cycles. > > + */ > > + > > + clk_rate = clk_get_rate(priv->clk); > > + div = clk_rate * state->duty_cycle; > > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > > + if (div > KMB_PWM_COUNT_MAX) > > + return -ERANGE; > > + > > + high = div; > > + div = clk_rate * state->period; > > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > > + div = div - high; > > + if (div > KMB_PWM_COUNT_MAX) > > + return -ERANGE; > > + > > + low = div; > > + > > + pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, high) | > > + FIELD_PREP(KMB_PWM_LOW_MASK, low); > > + > > + writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); > > + > > + if (state->enabled && !current_state.enabled) > > + keembay_pwm_enable(priv, pwm->hwpwm); > > + > > + return 0; > > +} > > + > > +static const struct pwm_ops keembay_pwm_ops = { > > + .owner = THIS_MODULE, > > + .apply = keembay_pwm_apply, > > + .get_state = keembay_pwm_get_state, > > +}; > > + > > +static int keembay_pwm_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct keembay_pwm *priv; > > + int ret; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(priv->clk)) > > + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock\n"); > > + > > + ret = clk_prepare_enable(priv->clk); > > + if (ret) > > + return ret; > > + > > + priv->base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(priv->base)) { > > > + clk_disable_unprepare(priv->clk); > > See below. > > > + return PTR_ERR(priv->base); > > + } > > + > > + priv->chip.base = -1; > > + priv->chip.dev = dev; > > + priv->chip.ops = &keembay_pwm_ops; > > + priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS; > > + > > + ret = pwmchip_add(&priv->chip); > > + if (ret) { > > + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret)); > > > + clk_disable_unprepare(priv->clk); > > This messes up with ordering of things. > > That's why devm golden rule is either all or none. You may fix this by > switching to devm_add_action_or_reset(). > > One of possible way is like in below drivers: > > % git grep -n devm_add_action_or_reset.*disable_unprepare -- drivers/ > > But it may be fixed in follow up change. Depends on maintainers' wishes. I recently sent a patch to address this in a still more comfortable way: https://lore.kernel.org/r/20201013082132.661993-1-u.kleine-koenig@pengutronix.de Other than that I don't think there is a real issue to fix here, yes, the unroll order doesn't match the init order, but here this is about ioremap vs clk_prepare_enable, so I'm fine with the status quo. (I assume I didn't miss a real issue here. Please tell if so.) 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] 17+ messages in thread
* Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay 2020-10-16 6:24 ` Uwe Kleine-König @ 2020-10-16 9:24 ` Andy Shevchenko 0 siblings, 0 replies; 17+ messages in thread From: Andy Shevchenko @ 2020-10-16 9:24 UTC (permalink / raw) To: Uwe Kleine-König Cc: vijayakannan.ayyathurai, thierry.reding, robh+dt, linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, mgross, lakshmi.bai.raja.subramanian On Fri, Oct 16, 2020 at 08:24:39AM +0200, Uwe Kleine-König wrote: > On Thu, Oct 15, 2020 at 01:42:17PM +0300, Andy Shevchenko wrote: > > On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote: > > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> ... > > > + ret = pwmchip_add(&priv->chip); > > > + if (ret) { > > > + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret)); > > > > > + clk_disable_unprepare(priv->clk); > > > > This messes up with ordering of things. > > > > That's why devm golden rule is either all or none. You may fix this by > > switching to devm_add_action_or_reset(). > > > > One of possible way is like in below drivers: > > > > % git grep -n devm_add_action_or_reset.*disable_unprepare -- drivers/ > > > > But it may be fixed in follow up change. Depends on maintainers' wishes. > > I recently sent a patch to address this in a still more comfortable way: > > https://lore.kernel.org/r/20201013082132.661993-1-u.kleine-koenig@pengutronix.de Good to know, thanks! > Other than that I don't think there is a real issue to fix here, yes, > the unroll order doesn't match the init order, but here this is about > ioremap vs clk_prepare_enable, so I'm fine with the status quo. > > (I assume I didn't miss a real issue here. Please tell if so.) No, as you said and as I answered in previous reply that the more serious issue (which you pointed out) has been replaced with less serious which can even be addressed separately, but it's up to you and Thierry. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v12 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM 2020-10-14 19:36 [PATCH v12 0/2] Add PWM support for Intel Keem Bay SoC vijayakannan.ayyathurai 2020-10-14 19:36 ` [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay vijayakannan.ayyathurai @ 2020-10-14 19:36 ` vijayakannan.ayyathurai 2020-10-16 16:04 ` Rob Herring 1 sibling, 1 reply; 17+ messages in thread From: vijayakannan.ayyathurai @ 2020-10-14 19:36 UTC (permalink / raw) To: thierry.reding, u.kleine-koenig, robh+dt Cc: linux-pwm, devicetree, wan.ahmad.zainie.wan.mohamad, andriy.shevchenko, mgross, lakshmi.bai.raja.subramanian, vijayakannan.ayyathurai From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC. Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> Reviewed-by: Rob Herring <robh@kernel.org> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> --- .../bindings/pwm/intel,keembay-pwm.yaml | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml diff --git a/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml new file mode 100644 index 000000000000..a37433487632 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml @@ -0,0 +1,47 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) 2020 Intel Corporation +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/intel,keembay-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Keem Bay PWM Device Tree Bindings + +maintainers: + - Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + enum: + - intel,keembay-pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + "#pwm-cells": + const: 2 + +required: + - compatible + - reg + - clocks + - '#pwm-cells' + +additionalProperties: false + +examples: + - | + #define KEEM_BAY_A53_GPIO + + pwm@203200a0 { + compatible = "intel,keembay-pwm"; + reg = <0x203200a0 0xe8>; + clocks = <&scmi_clk KEEM_BAY_A53_GPIO>; + #pwm-cells = <2>; + }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v12 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM 2020-10-14 19:36 ` [PATCH v12 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM vijayakannan.ayyathurai @ 2020-10-16 16:04 ` Rob Herring 2020-10-16 19:34 ` Ayyathurai, Vijayakannan 0 siblings, 1 reply; 17+ messages in thread From: Rob Herring @ 2020-10-16 16:04 UTC (permalink / raw) To: vijayakannan.ayyathurai Cc: robh+dt, thierry.reding, u.kleine-koenig, lakshmi.bai.raja.subramanian, mgross, wan.ahmad.zainie.wan.mohamad, devicetree, andriy.shevchenko, linux-pwm On Thu, 15 Oct 2020 03:36:10 +0800, vijayakannan.ayyathurai@intel.com wrote: > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > > Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC. > > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > Reviewed-by: Rob Herring <robh@kernel.org> > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com> > --- > .../bindings/pwm/intel,keembay-pwm.yaml | 47 +++++++++++++++++++ > 1 file changed, 47 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml > My bot found errors running 'make dt_binding_check' on your patch: ./Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml:31:2: [warning] wrong indentation: expected 2 but found 1 (indentation) See https://patchwork.ozlabs.org/patch/1382326 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit. ^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v12 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM 2020-10-16 16:04 ` Rob Herring @ 2020-10-16 19:34 ` Ayyathurai, Vijayakannan 0 siblings, 0 replies; 17+ messages in thread From: Ayyathurai, Vijayakannan @ 2020-10-16 19:34 UTC (permalink / raw) To: Rob Herring Cc: robh+dt, thierry.reding, u.kleine-koenig, Raja Subramanian, Lakshmi Bai, mgross, Wan Mohamad, Wan Ahmad Zainie, devicetree, andriy.shevchenko, linux-pwm Hi Rob, > From: Rob Herring <robh@kernel.org> > Sent: Friday, 16 October, 2020 9:34 PM > > My bot found errors running 'make dt_binding_check' on your patch: Ok I will check again with upgraded schema and incorporate the fix in the next version. > > ./Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml:31:2: > [warning] wrong indentation: expected 2 but found 1 (indentation) > > > See https://patchwork.ozlabs.org/patch/1382326 > > If you already ran 'make dt_binding_check' and didn't see the above error(s), > then make sure dt-schema is up to date: > > pip3 install git+https://github.com/devicetree-org/dt-schema.git@master -- > upgrade > > Please check and re-submit. Thanks, Vijay ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-10-19 6:46 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-14 19:36 [PATCH v12 0/2] Add PWM support for Intel Keem Bay SoC vijayakannan.ayyathurai 2020-10-14 19:36 ` [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay vijayakannan.ayyathurai 2020-10-14 20:04 ` Uwe Kleine-König 2020-10-15 10:42 ` Andy Shevchenko 2020-10-16 3:18 ` Ayyathurai, Vijayakannan 2020-10-16 7:34 ` Uwe Kleine-König 2020-10-16 8:32 ` Ayyathurai, Vijayakannan 2020-10-16 9:25 ` Andy Shevchenko 2020-10-19 5:44 ` Ayyathurai, Vijayakannan 2020-10-19 6:42 ` Uwe Kleine-König 2020-10-16 9:21 ` Andy Shevchenko 2020-10-19 6:46 ` Uwe Kleine-König 2020-10-16 6:24 ` Uwe Kleine-König 2020-10-16 9:24 ` Andy Shevchenko 2020-10-14 19:36 ` [PATCH v12 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM vijayakannan.ayyathurai 2020-10-16 16:04 ` Rob Herring 2020-10-16 19:34 ` Ayyathurai, Vijayakannan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.