| Message ID | 1595083628-20734-2-git-send-email-vineetha.g.jaya.kumaran@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add PWM support for Intel Keem Bay SoC | expand |
Hi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on robh/for-next v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/vineetha-g-jaya-kumaran-intel-com/Add-PWM-support-for-Intel-Keem-Bay-SoC/20200718-225115
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: m68k-allyesconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
m68k-linux-ld: drivers/pwm/pwm-keembay.o: in function `keembay_pwm_get_state':
>> pwm-keembay.c:(.text+0x8a): undefined reference to `__udivdi3'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on robh/for-next v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/vineetha-g-jaya-kumaran-intel-com/Add-PWM-support-for-Intel-Keem-Bay-SoC/20200718-225115
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__divdi3" [drivers/pwm/pwm-keembay.ko] undefined!
>> ERROR: modpost: "__udivdi3" [drivers/pwm/pwm-keembay.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hello, On Sat, Jul 18, 2020 at 10:47:07PM +0800, vineetha.g.jaya.kumaran@intel.com wrote: > From: "Lai, Poey Seng" <poey.seng.lai@intel.com> > > Enable PWM support for the Intel Keem Bay SoC. > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com> > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-keembay.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 249 insertions(+) > create mode 100644 drivers/pwm/pwm-keembay.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index cb8d739..2b0419b 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -569,4 +569,13 @@ config PWM_ZX > To compile this driver as a module, choose M here: the module > will be called pwm-zx. > > +config PWM_KEEMBAY > + tristate "Intel Keem Bay PWM driver" > + depends on ARM64 || 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. > + > endif > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index a59c710..0c84ff2 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -55,3 +55,4 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o > obj-$(CONFIG_PWM_ZX) += pwm-zx.o > +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c > new file mode 100644 > index 00000000..fa5fe95 > --- /dev/null > +++ b/drivers/pwm/pwm-keembay.c > @@ -0,0 +1,239 @@ > +// 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> > + */ If possible, please add a link here to documentation for this chip. > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.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 65535 If you write this as hexadecimal constant it is more obvious. > +#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_COUNT_MASK GENMASK(31, 0) > + > +/* PWM Register offset */ > +#define KMB_PWM_LEADIN0_OFFSET 0x00 > +#define KMB_PWM_LEADIN1_OFFSET 0x04 > +#define KMB_PWM_LEADIN2_OFFSET 0x08 > +#define KMB_PWM_LEADIN3_OFFSET 0x0c > +#define KMB_PWM_LEADIN4_OFFSET 0x10 > +#define KMB_PWM_LEADIN5_OFFSET 0x14 All but ..LEADIN0.. are unused. Is this maybe more useful to write as: #define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) ? > +#define KMB_PWM_HIGHLOW0_OFFSET 0x20 > +#define KMB_PWM_HIGHLOW1_OFFSET 0x24 > +#define KMB_PWM_HIGHLOW2_OFFSET 0x28 > +#define KMB_PWM_HIGHLOW3_OFFSET 0x2c > +#define KMB_PWM_HIGHLOW4_OFFSET 0x30 > +#define KMB_PWM_HIGHLOW5_OFFSET 0x34 ditto. > +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 reg, int ch) > +{ > + u32 buff, offset, tmp; > + void __iomem *address; > + > + offset = reg + ch * 4; > + address = priv->base + offset; > + buff = readl(address); > + tmp = buff & ~mask; > + tmp |= FIELD_PREP(mask, val); > + writel(tmp, address); > +} > + > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) > +{ > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > + KMB_PWM_LEADIN0_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_LEADIN0_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 pwm_h_count, pwm_l_count; > + unsigned long clk_rate; > + u32 buff; > + > + clk_rate = clk_get_rate(priv->clk); > + > + /* Read channel enabled status */ > + buff = readl(priv->base + KMB_PWM_LEADIN0_OFFSET + pwm->hwpwm * 4); > + if (buff & KMB_PWM_EN_BIT) > + state->enabled = true; > + else > + state->enabled = false; > + > + /* Read period and duty cycle*/ Missing ' ' before closing */ > + buff = readl(priv->base + KMB_PWM_HIGHLOW0_OFFSET + pwm->hwpwm * 4); > + pwm_l_count = (buff & KMB_PWM_LOW_MASK) * NSEC_PER_SEC; > + pwm_h_count = ((buff & KMB_PWM_HIGH_MASK) >> 16) * NSEC_PER_SEC; pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC > + state->duty_cycle = pwm_h_count / clk_rate; > + state->period = (pwm_h_count + pwm_l_count) / clk_rate; Please round up both values. > +} > + > +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; > + u16 pwm_h_count, pwm_l_count; > + unsigned long long div; > + unsigned long clk_rate; > + u32 pwm_count = 0; > + > + pwm_get_state(pwm, ¤t_state); Please check the hardware state, not the value cached in the PWM framework. > + if (!state->enabled && current_state.enabled) { > + keembay_pwm_disable(priv, pwm->hwpwm); > + return 0; > + } > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOSYS; This must be checked before .enabled. That's because the expectation on .enabled = false .polarity = PWM_POLARITY_INVERSED is that the output gets constant high. > + /* > + * The upper 16 bits of the KMB_PWM_HIGHLOWx_OFFSET register contain > + * the high time of the waveform, while the last 16 bits contain > + * the low time of the waveform, in terms of clock cycles. Just to be sure: Each period starts with the high time, right? > + * high time = clock rate * duty cycle / NSEC_PER_SEC > + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC > + * > + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz: > + * high time = (500000000 * 30000) / 1000000000 = 0x3A98 > + * low time = (500000000 * 20000) / 1000000000 = 0x2710 > + * Value written to KMB_PWM_HIGHLOWx_OFFSET = 0x3A982710 For period = 50000ns, duty_cycle = 30000ns and clock rate 266666666 Hz you have to configure: high = 7999 low = 5334 > + */ > + > + clk_rate = clk_get_rate(priv->clk); > + > + /* Configure waveform high time */ > + div = clk_rate * state->duty_cycle; > + do_div(div, NSEC_PER_SEC); Can this overflow? > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_h_count = (u16)div; No need for this cast. > + /* Configure waveform low time */ > + div = clk_rate * (state->period - state->duty_cycle); > + do_div(div, NSEC_PER_SEC); Here the rounding is wrong. (See above example, currently you use low = 5333 here) > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_l_count = (u16)div; > + > + pwm_count |= pwm_h_count << 16; > + pwm_count |= pwm_l_count; pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); > + > + keembay_pwm_update_bits(priv, KMB_PWM_COUNT_MASK, > + pwm_count, KMB_PWM_HIGHLOW0_OFFSET, pwm->hwpwm); here the rwm-procedure is not necessary as all 32 bits are written anyhow. > + if (state->enabled && !current_state.enabled) > + keembay_pwm_enable(priv, pwm->hwpwm); > + > + return 0; > +} > [...] > +MODULE_ALIAS("platform:keembay"); This has to match your driver's name, so use: MODULE_ALIAS("platform:pwm-keembay"); > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); > +MODULE_LICENSE("GPL v2"); In v1 you told that on reconfiguration the hardware completes the currently running period. Please document this in the driver, similar to e.g. pwm-sifive.c. You only ever write the enable bit in the leadin register. If there is something != 0 at boot this influences the correct behaviour of the driver, right? Further things to note there: - What is the behaviour on disable (usual candidates: freezes at current value, completes period and emits low, changes to High-Z)? - Can it do 0% and 100% duty ratio? Best regards Uwe
Hi Uwe, Thank you for the feedback, will work on the changes for V3. > -----Original Message----- > From: linux-pwm-owner@vger.kernel.org <linux-pwm- > owner@vger.kernel.org> On Behalf Of Uwe Kleine-König > Sent: Tuesday, July 21, 2020 5:14 PM > To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com> > Cc: 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>; Shevchenko, > Andriy <andriy.shevchenko@intel.com> > Subject: Re: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay > > Hello, > > On Sat, Jul 18, 2020 at 10:47:07PM +0800, > vineetha.g.jaya.kumaran@intel.com wrote: > > From: "Lai, Poey Seng" <poey.seng.lai@intel.com> > > > > Enable PWM support for the Intel Keem Bay SoC. > > > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> > > Signed-off-by: Vineetha G. Jaya Kumaran > > <vineetha.g.jaya.kumaran@intel.com> > > --- > > drivers/pwm/Kconfig | 9 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-keembay.c | 239 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 249 insertions(+) > > create mode 100644 drivers/pwm/pwm-keembay.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > cb8d739..2b0419b 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -569,4 +569,13 @@ config PWM_ZX > > To compile this driver as a module, choose M here: the module > > will be called pwm-zx. > > > > +config PWM_KEEMBAY > > + tristate "Intel Keem Bay PWM driver" > > + depends on ARM64 || 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. > > + > > endif > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index > > a59c710..0c84ff2 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -55,3 +55,4 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o > > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o > > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o > > obj-$(CONFIG_PWM_ZX) += pwm-zx.o > > +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o > > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c > new > > file mode 100644 index 00000000..fa5fe95 > > --- /dev/null > > +++ b/drivers/pwm/pwm-keembay.c > > @@ -0,0 +1,239 @@ > > +// 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> > > + */ > > If possible, please add a link here to documentation for this chip. > There is no documentation released publicly (yet), but once there is, I will add it in. > > + > > +#include <linux/bitfield.h> > > +#include <linux/clk.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > +#include <linux/of.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 65535 > > If you write this as hexadecimal constant it is more obvious. > > > +#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_COUNT_MASK GENMASK(31, 0) > > + > > +/* PWM Register offset */ > > +#define KMB_PWM_LEADIN0_OFFSET 0x00 > > +#define KMB_PWM_LEADIN1_OFFSET 0x04 > > +#define KMB_PWM_LEADIN2_OFFSET 0x08 > > +#define KMB_PWM_LEADIN3_OFFSET 0x0c > > +#define KMB_PWM_LEADIN4_OFFSET 0x10 > > +#define KMB_PWM_LEADIN5_OFFSET 0x14 > > All but ..LEADIN0.. are unused. Is this maybe more useful to write as: > > #define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) > > ? > Agreed, will change this for both KMB_PWM_HIGHLOW_OFFSET and KMB_PWM_LEADIN_OFFSET. > > +#define KMB_PWM_HIGHLOW0_OFFSET 0x20 > > +#define KMB_PWM_HIGHLOW1_OFFSET 0x24 > > +#define KMB_PWM_HIGHLOW2_OFFSET 0x28 > > +#define KMB_PWM_HIGHLOW3_OFFSET 0x2c > > +#define KMB_PWM_HIGHLOW4_OFFSET 0x30 > > +#define KMB_PWM_HIGHLOW5_OFFSET 0x34 > > ditto. > > > +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 reg, int ch) > > +{ > > + u32 buff, offset, tmp; > > + void __iomem *address; > > + > > + offset = reg + ch * 4; > > + address = priv->base + offset; > > + buff = readl(address); > > + tmp = buff & ~mask; > > + tmp |= FIELD_PREP(mask, val); > > + writel(tmp, address); > > +} > > + > > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) { > > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > > + KMB_PWM_LEADIN0_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_LEADIN0_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 pwm_h_count, pwm_l_count; > > + unsigned long clk_rate; > > + u32 buff; > > + > > + clk_rate = clk_get_rate(priv->clk); > > + > > + /* Read channel enabled status */ > > + buff = readl(priv->base + KMB_PWM_LEADIN0_OFFSET + pwm- > >hwpwm * 4); > > + if (buff & KMB_PWM_EN_BIT) > > + state->enabled = true; > > + else > > + state->enabled = false; > > + > > + /* Read period and duty cycle*/ > > Missing ' ' before closing */ > > > + buff = readl(priv->base + KMB_PWM_HIGHLOW0_OFFSET + pwm- > >hwpwm * 4); > > + pwm_l_count = (buff & KMB_PWM_LOW_MASK) * NSEC_PER_SEC; > > + pwm_h_count = ((buff & KMB_PWM_HIGH_MASK) >> 16) * > NSEC_PER_SEC; > > pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * > NSEC_PER_SEC > > > + state->duty_cycle = pwm_h_count / clk_rate; > > + state->period = (pwm_h_count + pwm_l_count) / clk_rate; > > Please round up both values. > > > +} > > + > > +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; > > + u16 pwm_h_count, pwm_l_count; > > + unsigned long long div; > > + unsigned long clk_rate; > > + u32 pwm_count = 0; > > + > > + pwm_get_state(pwm, ¤t_state); > > Please check the hardware state, not the value cached in the PWM > framework. > > > + if (!state->enabled && current_state.enabled) { > > + keembay_pwm_disable(priv, pwm->hwpwm); > > + return 0; > > + } > > + > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -ENOSYS; > > This must be checked before .enabled. That's because the expectation on > > .enabled = false > .polarity = PWM_POLARITY_INVERSED > > is that the output gets constant high. > > > + /* > > + * The upper 16 bits of the KMB_PWM_HIGHLOWx_OFFSET register > contain > > + * the high time of the waveform, while the last 16 bits contain > > + * the low time of the waveform, in terms of clock cycles. > > Just to be sure: Each period starts with the high time, right? > Yes, each period starts with the high time. > > + * high time = clock rate * duty cycle / NSEC_PER_SEC > > + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC > > + * > > + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz: > > + * high time = (500000000 * 30000) / 1000000000 = 0x3A98 > > + * low time = (500000000 * 20000) / 1000000000 = 0x2710 > > + * Value written to KMB_PWM_HIGHLOWx_OFFSET = 0x3A982710 > > For period = 50000ns, duty_cycle = 30000ns and clock rate 266666666 Hz you > have to configure: > > high = 7999 > low = 5334 > > > + */ > > + > > + clk_rate = clk_get_rate(priv->clk); > > + > > + /* Configure waveform high time */ > > + div = clk_rate * state->duty_cycle; > > + do_div(div, NSEC_PER_SEC); > > Can this overflow? > > > + if (div > KMB_PWM_COUNT_MAX) > > + return -ERANGE; > > + > > + pwm_h_count = (u16)div; > > No need for this cast. > > > + /* Configure waveform low time */ > > + div = clk_rate * (state->period - state->duty_cycle); > > + do_div(div, NSEC_PER_SEC); > > Here the rounding is wrong. (See above example, currently you use low = > 5333 here) > > > + if (div > KMB_PWM_COUNT_MAX) > > + return -ERANGE; > > + > > + pwm_l_count = (u16)div; > > + > > + pwm_count |= pwm_h_count << 16; > > + pwm_count |= pwm_l_count; > > pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | > FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); > > > + > > + keembay_pwm_update_bits(priv, KMB_PWM_COUNT_MASK, > > + pwm_count, > KMB_PWM_HIGHLOW0_OFFSET, pwm->hwpwm); > > here the rwm-procedure is not necessary as all 32 bits are written anyhow. > > > + if (state->enabled && !current_state.enabled) > > + keembay_pwm_enable(priv, pwm->hwpwm); > > + > > + return 0; > > +} > > [...] > > +MODULE_ALIAS("platform:keembay"); > > This has to match your driver's name, so use: > > MODULE_ALIAS("platform:pwm-keembay"); > > > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); > MODULE_LICENSE("GPL > > +v2"); > > In v1 you told that on reconfiguration the hardware completes the currently > running period. Please document this in the driver, similar to e.g. pwm- > sifive.c. > OK, I will refer to that example and add this information in. > You only ever write the enable bit in the leadin register. If there is something > != 0 at boot this influences the correct behaviour of the driver, right? > That's right, I will add a check during probe to ensure the bit is set to 0 as default. > Further things to note there: > > - What is the behaviour on disable (usual candidates: freezes at > current value, completes period and emits low, changes to High-Z)? > For this case, the period does not get completed. As soon as the channel is disabled, the output goes low. > - Can it do 0% and 100% duty ratio? > Yes, we can set duty cycles of 0% and 100%. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
> -----Original Message----- > From: linux-pwm-owner@vger.kernel.org <linux-pwm- > owner@vger.kernel.org> On Behalf Of G Jaya Kumaran, Vineetha > Sent: Monday, July 27, 2020 1:12 PM > To: Uwe Kleine Konig <u.kleine-koenig@pengutronix.de> > Cc: 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>; Shevchenko, > Andriy <andriy.shevchenko@intel.com> > Subject: RE: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay > > Hi Uwe, > > Thank you for the feedback, will work on the changes for V3. > > > -----Original Message----- > > From: linux-pwm-owner@vger.kernel.org <linux-pwm- > > owner@vger.kernel.org> On Behalf Of Uwe Kleine-König > > Sent: Tuesday, July 21, 2020 5:14 PM > > To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@intel.com> > > Cc: 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>; > Shevchenko, > > Andriy <andriy.shevchenko@intel.com> > > Subject: Re: [PATCH v2 1/2] pwm: Add PWM driver for Intel Keem Bay > > > > Hello, > > > > On Sat, Jul 18, 2020 at 10:47:07PM +0800, > > vineetha.g.jaya.kumaran@intel.com wrote: > > > From: "Lai, Poey Seng" <poey.seng.lai@intel.com> > > > > > > Enable PWM support for the Intel Keem Bay SoC. > > > > > > Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com> > > > Signed-off-by: Vineetha G. Jaya Kumaran > > > <vineetha.g.jaya.kumaran@intel.com> > > > --- > > > drivers/pwm/Kconfig | 9 ++ > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-keembay.c | 239 > > > ++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 249 insertions(+) > > > create mode 100644 drivers/pwm/pwm-keembay.c > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > > cb8d739..2b0419b 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -569,4 +569,13 @@ config PWM_ZX > > > To compile this driver as a module, choose M here: the module > > > will be called pwm-zx. > > > > > > +config PWM_KEEMBAY > > > + tristate "Intel Keem Bay PWM driver" > > > + depends on ARM64 || 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. > > > + > > > endif > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index > > > a59c710..0c84ff2 100644 > > > --- a/drivers/pwm/Makefile > > > +++ b/drivers/pwm/Makefile > > > @@ -55,3 +55,4 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o > > > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o > > > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o > > > obj-$(CONFIG_PWM_ZX) += pwm-zx.o > > > +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o > > > diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm- > keembay.c > > new > > > file mode 100644 index 00000000..fa5fe95 > > > --- /dev/null > > > +++ b/drivers/pwm/pwm-keembay.c > > > @@ -0,0 +1,239 @@ > > > +// 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> > > > + */ > > > > If possible, please add a link here to documentation for this chip. > > > > There is no documentation released publicly (yet), but once there is, I will > add it in. > > > > + > > > +#include <linux/bitfield.h> > > > +#include <linux/clk.h> > > > +#include <linux/io.h> > > > +#include <linux/module.h> > > > +#include <linux/of.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 65535 > > > > If you write this as hexadecimal constant it is more obvious. > > > > > +#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_COUNT_MASK GENMASK(31, 0) > > > + > > > +/* PWM Register offset */ > > > +#define KMB_PWM_LEADIN0_OFFSET 0x00 > > > +#define KMB_PWM_LEADIN1_OFFSET 0x04 > > > +#define KMB_PWM_LEADIN2_OFFSET 0x08 > > > +#define KMB_PWM_LEADIN3_OFFSET 0x0c > > > +#define KMB_PWM_LEADIN4_OFFSET 0x10 > > > +#define KMB_PWM_LEADIN5_OFFSET 0x14 > > > > All but ..LEADIN0.. are unused. Is this maybe more useful to write as: > > > > #define KMB_PWM_LEADIN_OFFSET(ch) (0x00 + 4 * (ch)) > > > > ? > > > > Agreed, will change this for both KMB_PWM_HIGHLOW_OFFSET and > KMB_PWM_LEADIN_OFFSET. > > > > +#define KMB_PWM_HIGHLOW0_OFFSET 0x20 > > > +#define KMB_PWM_HIGHLOW1_OFFSET 0x24 > > > +#define KMB_PWM_HIGHLOW2_OFFSET 0x28 > > > +#define KMB_PWM_HIGHLOW3_OFFSET 0x2c > > > +#define KMB_PWM_HIGHLOW4_OFFSET 0x30 > > > +#define KMB_PWM_HIGHLOW5_OFFSET 0x34 > > > > ditto. > > > > > +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 reg, int ch) > > > +{ > > > + u32 buff, offset, tmp; > > > + void __iomem *address; > > > + > > > + offset = reg + ch * 4; > > > + address = priv->base + offset; > > > + buff = readl(address); > > > + tmp = buff & ~mask; > > > + tmp |= FIELD_PREP(mask, val); > > > + writel(tmp, address); > > > +} > > > + > > > +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) { > > > + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, > > > + KMB_PWM_LEADIN0_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_LEADIN0_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 pwm_h_count, pwm_l_count; > > > + unsigned long clk_rate; > > > + u32 buff; > > > + > > > + clk_rate = clk_get_rate(priv->clk); > > > + > > > + /* Read channel enabled status */ > > > + buff = readl(priv->base + KMB_PWM_LEADIN0_OFFSET + pwm- > > >hwpwm * 4); > > > + if (buff & KMB_PWM_EN_BIT) > > > + state->enabled = true; > > > + else > > > + state->enabled = false; > > > + > > > + /* Read period and duty cycle*/ > > > > Missing ' ' before closing */ > > > > > + buff = readl(priv->base + KMB_PWM_HIGHLOW0_OFFSET + pwm- > > >hwpwm * 4); > > > + pwm_l_count = (buff & KMB_PWM_LOW_MASK) * NSEC_PER_SEC; > > > + pwm_h_count = ((buff & KMB_PWM_HIGH_MASK) >> 16) * > > NSEC_PER_SEC; > > > > pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * > NSEC_PER_SEC > > > > > + state->duty_cycle = pwm_h_count / clk_rate; > > > + state->period = (pwm_h_count + pwm_l_count) / clk_rate; > > > > Please round up both values. > > > > > +} > > > + > > > +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; > > > + u16 pwm_h_count, pwm_l_count; > > > + unsigned long long div; > > > + unsigned long clk_rate; > > > + u32 pwm_count = 0; > > > + > > > + pwm_get_state(pwm, ¤t_state); > > > > Please check the hardware state, not the value cached in the PWM > > framework. > > > > > + if (!state->enabled && current_state.enabled) { > > > + keembay_pwm_disable(priv, pwm->hwpwm); > > > + return 0; > > > + } > > > + > > > + if (state->polarity != PWM_POLARITY_NORMAL) > > > + return -ENOSYS; > > > > This must be checked before .enabled. That's because the expectation > > on > > > > .enabled = false > > .polarity = PWM_POLARITY_INVERSED > > > > is that the output gets constant high. > > > > > + /* > > > + * The upper 16 bits of the KMB_PWM_HIGHLOWx_OFFSET register > > contain > > > + * the high time of the waveform, while the last 16 bits contain > > > + * the low time of the waveform, in terms of clock cycles. > > > > Just to be sure: Each period starts with the high time, right? > > > > Yes, each period starts with the high time. > > > > + * high time = clock rate * duty cycle / NSEC_PER_SEC > > > + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC > > > + * > > > + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz: > > > + * high time = (500000000 * 30000) / 1000000000 = 0x3A98 > > > + * low time = (500000000 * 20000) / 1000000000 = 0x2710 > > > + * Value written to KMB_PWM_HIGHLOWx_OFFSET = 0x3A982710 > > > > For period = 50000ns, duty_cycle = 30000ns and clock rate 266666666 Hz > > you have to configure: > > > > high = 7999 > > low = 5334 > > > > > + */ > > > + > > > + clk_rate = clk_get_rate(priv->clk); > > > + > > > + /* Configure waveform high time */ > > > + div = clk_rate * state->duty_cycle; > > > + do_div(div, NSEC_PER_SEC); > > > > Can this overflow? > > Hi Uwe, sorry for missing this in my previous reply. Yes, it may exceed the 16-bit limit for the high/low time, depending on the selected duty cycle. There is a range check after, and -ERANGE will be returned if the high/low time range is exceeded. > > > + if (div > KMB_PWM_COUNT_MAX) > > > + return -ERANGE; > > > + > > > + pwm_h_count = (u16)div; > > > > No need for this cast. > > > > > + /* Configure waveform low time */ > > > + div = clk_rate * (state->period - state->duty_cycle); > > > + do_div(div, NSEC_PER_SEC); > > > > Here the rounding is wrong. (See above example, currently you use low > > = > > 5333 here) > > Will fix the rounding to the nearest integer for the high/low time. > > > + if (div > KMB_PWM_COUNT_MAX) > > > + return -ERANGE; > > > + > > > + pwm_l_count = (u16)div; > > > + > > > + pwm_count |= pwm_h_count << 16; > > > + pwm_count |= pwm_l_count; > > > > pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | > > FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); > > > > > + > > > + keembay_pwm_update_bits(priv, KMB_PWM_COUNT_MASK, > > > + pwm_count, > > KMB_PWM_HIGHLOW0_OFFSET, pwm->hwpwm); > > > > here the rwm-procedure is not necessary as all 32 bits are written anyhow. > > > > > + if (state->enabled && !current_state.enabled) > > > + keembay_pwm_enable(priv, pwm->hwpwm); > > > + > > > + return 0; > > > +} > > > [...] > > > +MODULE_ALIAS("platform:keembay"); > > > > This has to match your driver's name, so use: > > > > MODULE_ALIAS("platform:pwm-keembay"); > > > > > +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); > > MODULE_LICENSE("GPL > > > +v2"); > > > > In v1 you told that on reconfiguration the hardware completes the > > currently running period. Please document this in the driver, similar > > to e.g. pwm- sifive.c. > > > > OK, I will refer to that example and add this information in. > > > You only ever write the enable bit in the leadin register. If there is > > something != 0 at boot this influences the correct behaviour of the driver, > right? > > > > That's right, I will add a check during probe to ensure the bit is set to 0 as > default. > > > Further things to note there: > > > > - What is the behaviour on disable (usual candidates: freezes at > > current value, completes period and emits low, changes to High-Z)? > > > > For this case, the period does not get completed. > As soon as the channel is disabled, the output goes low. > > > - Can it do 0% and 100% duty ratio? > > > > Yes, we can set duty cycles of 0% and 100%. > > > Best regards > > Uwe > > > > -- > > Pengutronix e.K. | Uwe Kleine-König | > > Industrial Linux Solutions | https://www.pengutronix.de/ |
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index cb8d739..2b0419b 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -569,4 +569,13 @@ config PWM_ZX To compile this driver as a module, choose M here: the module will be called pwm-zx. +config PWM_KEEMBAY + tristate "Intel Keem Bay PWM driver" + depends on ARM64 || 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. + endif diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index a59c710..0c84ff2 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -55,3 +55,4 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o obj-$(CONFIG_PWM_ZX) += pwm-zx.o +obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c new file mode 100644 index 00000000..fa5fe95 --- /dev/null +++ b/drivers/pwm/pwm-keembay.c @@ -0,0 +1,239 @@ +// 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> + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.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 65535 +#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_COUNT_MASK GENMASK(31, 0) + +/* PWM Register offset */ +#define KMB_PWM_LEADIN0_OFFSET 0x00 +#define KMB_PWM_LEADIN1_OFFSET 0x04 +#define KMB_PWM_LEADIN2_OFFSET 0x08 +#define KMB_PWM_LEADIN3_OFFSET 0x0c +#define KMB_PWM_LEADIN4_OFFSET 0x10 +#define KMB_PWM_LEADIN5_OFFSET 0x14 + +#define KMB_PWM_HIGHLOW0_OFFSET 0x20 +#define KMB_PWM_HIGHLOW1_OFFSET 0x24 +#define KMB_PWM_HIGHLOW2_OFFSET 0x28 +#define KMB_PWM_HIGHLOW3_OFFSET 0x2c +#define KMB_PWM_HIGHLOW4_OFFSET 0x30 +#define KMB_PWM_HIGHLOW5_OFFSET 0x34 + +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 reg, int ch) +{ + u32 buff, offset, tmp; + void __iomem *address; + + offset = reg + ch * 4; + address = priv->base + offset; + buff = readl(address); + tmp = buff & ~mask; + tmp |= FIELD_PREP(mask, val); + writel(tmp, address); +} + +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch) +{ + keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1, + KMB_PWM_LEADIN0_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_LEADIN0_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 pwm_h_count, pwm_l_count; + unsigned long clk_rate; + u32 buff; + + clk_rate = clk_get_rate(priv->clk); + + /* Read channel enabled status */ + buff = readl(priv->base + KMB_PWM_LEADIN0_OFFSET + pwm->hwpwm * 4); + if (buff & KMB_PWM_EN_BIT) + state->enabled = true; + else + state->enabled = false; + + /* Read period and duty cycle*/ + buff = readl(priv->base + KMB_PWM_HIGHLOW0_OFFSET + pwm->hwpwm * 4); + pwm_l_count = (buff & KMB_PWM_LOW_MASK) * NSEC_PER_SEC; + pwm_h_count = ((buff & KMB_PWM_HIGH_MASK) >> 16) * NSEC_PER_SEC; + state->duty_cycle = pwm_h_count / clk_rate; + state->period = (pwm_h_count + pwm_l_count) / clk_rate; +} + +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; + u16 pwm_h_count, pwm_l_count; + unsigned long long div; + unsigned long clk_rate; + u32 pwm_count = 0; + + pwm_get_state(pwm, ¤t_state); + + if (!state->enabled && current_state.enabled) { + keembay_pwm_disable(priv, pwm->hwpwm); + return 0; + } + + if (state->polarity != PWM_POLARITY_NORMAL) + return -ENOSYS; + + /* + * The upper 16 bits of the KMB_PWM_HIGHLOWx_OFFSET register contain + * the high time of the waveform, while the last 16 bits contain + * the low time of the waveform, in terms of clock cycles. + * + * high time = clock rate * duty cycle / NSEC_PER_SEC + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC + * + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz: + * high time = (500000000 * 30000) / 1000000000 = 0x3A98 + * low time = (500000000 * 20000) / 1000000000 = 0x2710 + * Value written to KMB_PWM_HIGHLOWx_OFFSET = 0x3A982710 + */ + + clk_rate = clk_get_rate(priv->clk); + + /* Configure waveform high time */ + div = clk_rate * state->duty_cycle; + do_div(div, NSEC_PER_SEC); + if (div > KMB_PWM_COUNT_MAX) + return -ERANGE; + + pwm_h_count = (u16)div; + + /* Configure waveform low time */ + div = clk_rate * (state->period - state->duty_cycle); + do_div(div, NSEC_PER_SEC); + if (div > KMB_PWM_COUNT_MAX) + return -ERANGE; + + pwm_l_count = (u16)div; + + pwm_count |= pwm_h_count << 16; + pwm_count |= pwm_l_count; + + keembay_pwm_update_bits(priv, KMB_PWM_COUNT_MASK, + pwm_count, KMB_PWM_HIGHLOW0_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(&pdev->dev, NULL); + if (IS_ERR(priv->clk)) { + if (PTR_ERR(priv->clk) != -EPROBE_DEFER) + dev_err(dev, "Failed to get clock: %pe", priv->clk); + + return PTR_ERR(priv->clk); + } + + priv->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(priv->base)) + 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 < 0) { + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret)); + 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); + + 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:keembay"); +MODULE_DESCRIPTION("Intel Keem Bay PWM driver"); +MODULE_LICENSE("GPL v2");