| Message ID | a8cb129092283cb6415e56b928293ef7121a851b.1602090900.git.vijayakannan.ayyathurai@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add PWM support for Intel Keem Bay SoC | expand |
Hello, On Thu, Oct 08, 2020 at 01:40:30AM +0800, vijayakannan.ayyathurai@intel.com wrote: > +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; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOSYS; > + > + 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 of the KMB_PWM_HIGHLOW_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 > + * low time = clock rate * (period - duty cycle) > + */ > + > + clk_rate = clk_get_rate(priv->clk); > + /* Configure waveform high time */ > + div = clk_rate * state->duty_cycle; > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_h_count = div; > + /* Configure waveform low time */ > + div = clk_rate * (state->period - state->duty_cycle); > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC - pwm_h_count); In reply to your v7 I suggested the example: clk_rate = 333334 [Hz] state.duty_cycle = 1000500 [ns] state.period = 2001000 [ns] where the expected outcome is pwm_h_count = 333 pwm_l_count = 334 . Please reread my feedback there. I tried to construct an example where the value is more wrong, but with the constraint that pwm_h_count must be <= KMB_PWM_COUNT_MAX this isn't easy. The best I could come up with is: clk_rate = 1000000000 state.duty_cycle = 65535 [ns] state.period = 131070 [ns] where the right value for pwm_l_count is 65535 while you calculate 65539 (and then quit with -ERANGE). > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_l_count = div; > + > + pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | > + FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); > + > + 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); > + > + 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. > +} Best regards Uwe
Hi Uwe, Thank you for constructing example to explain the review comment. -----Original Message----- From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Sent: Thursday, 8 October, 2020 2:28 AM Subject: Re: [PATCH v10 1/2] pwm: Add PWM driver for Intel Keem Bay Hello, On Thu, Oct 08, 2020 at 01:40:30AM +0800, vijayakannan.ayyathurai@intel.com wrote: > +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; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOSYS; > + > + 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 of the KMB_PWM_HIGHLOW_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 > + * low time = clock rate * (period - duty cycle) > + */ > + > + clk_rate = clk_get_rate(priv->clk); > + /* Configure waveform high time */ > + div = clk_rate * state->duty_cycle; > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_h_count = div; > + /* Configure waveform low time */ > + div = clk_rate * (state->period - state->duty_cycle); > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC - pwm_h_count); In reply to your v7 I suggested the example: clk_rate = 333334 [Hz] state.duty_cycle = 1000500 [ns] state.period = 2001000 [ns] where the expected outcome is pwm_h_count = 333 pwm_l_count = 334 . Please reread my feedback there. I tried to construct an example where the value is more wrong, but with the constraint that pwm_h_count must be <= KMB_PWM_COUNT_MAX this isn't easy. The best I could come up with is: clk_rate = 1000000000 state.duty_cycle = 65535 [ns] state.period = 131070 [ns] where the right value for pwm_l_count is 65535 while you calculate 65539 (and then quit with -ERANGE). I have applied the formula mentioned in v7 and got different duty cycle result then what was expected. Formula referred by Uwe at v7: pwm_l_count = (clk_rate * state->period) / NSEC_PER_SEC - pwm_h_count % clk_rate period duty_cycle NSEC_PER_SEC pwm_h_count pwm_l_count 50% 333334 2001000 1000500 1000000000 333 667 25% 500000000 20000 5000 1000000000 2500 10000 50% 100000000 131070 65535 1000000000 6553 13107 Whereas the below table is the result of minor modification from your formula and getting the right result. pwm_l_count = clk_rate * (state->period - state->duty_cycle) / NSEC_PER_SEC - pwm_h_count % clk_rate period duty_cycle NSEC_PER_SEC pwm_h_count pwm_l_count 50% 333334 2001000 1000500 1000000000 333 333 25% 500000000 20000 5000 1000000000 2500 7500 50% 100000000 131070 65535 1000000000 6553 6553 Please review this and confirm. > + if (div > KMB_PWM_COUNT_MAX) > + return -ERANGE; > + > + pwm_l_count = div; > + > + pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | > + FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); > + > + 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); > + > + 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. I will incorporate the change in next version. > +} Best regards Uwe
Hello Ayyathurai, you're quoting style is strange. I fixed it up and hope I got it right. On Mon, Oct 12, 2020 at 08:04:47PM +0000, Ayyathurai, Vijayakannan wrote: > > On Thu, Oct 08, 2020 at 01:40:30AM +0800, vijayakannan.ayyathurai@intel.com wrote: > > > +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; > > > + > > > + if (state->polarity != PWM_POLARITY_NORMAL) > > > + return -ENOSYS; > > > + > > > + 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 of the KMB_PWM_HIGHLOW_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 > > > + * low time = clock rate * (period - duty cycle) > > > + */ > > > + > > > + clk_rate = clk_get_rate(priv->clk); > > > + /* Configure waveform high time */ > > > + div = clk_rate * state->duty_cycle; > > > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > > > + if (div > KMB_PWM_COUNT_MAX) > > > + return -ERANGE; > > > + > > > + pwm_h_count = div; > > > + /* Configure waveform low time */ > > > + div = clk_rate * (state->period - state->duty_cycle); > > > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC - pwm_h_count); > > > > In reply to your v7 I suggested the example: > > > > clk_rate = 333334 [Hz] > > state.duty_cycle = 1000500 [ns] > > state.period = 2001000 [ns] > > > > where the expected outcome is > > > > pwm_h_count = 333 > > pwm_l_count = 334 > > > > . Please reread my feedback there. I tried to construct an example > > where the value is more wrong, but with the constraint that > > pwm_h_count must be <= KMB_PWM_COUNT_MAX this isn't easy. The best I > > could come up with is: > > > > clk_rate = 1000000000 > > state.duty_cycle = 65535 [ns] > > state.period = 131070 [ns] > > > > where the right value for pwm_l_count is 65535 while you calculate > > 65539 (and then quit with -ERANGE). > > I have applied the formula mentioned in v7 and got different duty > cycle result then what was expected. > > Formula referred by Uwe at v7: > pwm_l_count = (clk_rate * state->period) / NSEC_PER_SEC - pwm_h_count > > % clk_rate period duty_cycle NSEC_PER_SEC pwm_h_count pwm_l_count > 50% 333334 2001000 1000500 1000000000 333 667 > 25% 500000000 20000 5000 1000000000 2500 10000 > 50% 100000000 131070 65535 1000000000 6553 13107 For the first line: (clk_rate * state->period) // NSEC_PER_SEC - pwm_h_count = (333334 * 2001000) // 1000000000 - 333 = 667.001334 - 333 = 334 This gives duty cycle = 333 * 1000000000 / 333334 = 998998.0020039959 ns and a period = (333 + 334) * 1000000000 / 333334 = 2000995.998008004 ns which is well in the limits. I guess you assumed my formula to be (clk_rate * state->period) / (NSEC_PER_SEC - pwm_h_count), but that's not what I meant. > Whereas the below table is the result of minor modification from your formula and getting the right result. > pwm_l_count = clk_rate * (state->period - state->duty_cycle) / NSEC_PER_SEC - pwm_h_count > > % clk_rate period duty_cycle NSEC_PER_SEC pwm_h_count pwm_l_count > 50% 333334 2001000 1000500 1000000000 333 333 > 25% 500000000 20000 5000 1000000000 2500 7500 > 50% 100000000 131070 65535 1000000000 6553 6553 > > Please review this and confirm. In the code you used clk_rate * (state->period - state->duty_cycle) / (NSEC_PER_SEC - pwm_h_count) which is notably different. For the example in the first line again you then get 333, which is a less optimal result than 334 I get with my (fixed) formula. I'm still convinced my formula is the right and optimal one. Best regards Uwe
Hi Uwe, -----Original Message----- From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Sent: Tuesday, 13 October, 2020 2:31 AM Subject: Re: [PATCH v10 1/2] pwm: Add PWM driver for Intel Keem Bay Hello Ayyathurai, you're quoting style is strange. I fixed it up and hope I got it right. On Mon, Oct 12, 2020 at 08:04:47PM +0000, Ayyathurai, Vijayakannan wrote: > > On Thu, Oct 08, 2020 at 01:40:30AM +0800, vijayakannan.ayyathurai@intel.com wrote: > > > +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; > > > + > > > + if (state->polarity != PWM_POLARITY_NORMAL) > > > + return -ENOSYS; > > > + > > > + 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 of the KMB_PWM_HIGHLOW_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 > > > + * low time = clock rate * (period - duty cycle) > > > + */ > > > + > > > + clk_rate = clk_get_rate(priv->clk); > > > + /* Configure waveform high time */ > > > + div = clk_rate * state->duty_cycle; > > > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); > > > + if (div > KMB_PWM_COUNT_MAX) > > > + return -ERANGE; > > > + > > > + pwm_h_count = div; > > > + /* Configure waveform low time */ > > > + div = clk_rate * (state->period - state->duty_cycle); > > > + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC - pwm_h_count); > > > > In reply to your v7 I suggested the example: > > > > clk_rate = 333334 [Hz] > > state.duty_cycle = 1000500 [ns] > > state.period = 2001000 [ns] > > > > where the expected outcome is > > > > pwm_h_count = 333 > > pwm_l_count = 334 > > > > . Please reread my feedback there. I tried to construct an example > > where the value is more wrong, but with the constraint that > > pwm_h_count must be <= KMB_PWM_COUNT_MAX this isn't easy. The best I > > could come up with is: > > > > clk_rate = 1000000000 > > state.duty_cycle = 65535 [ns] > > state.period = 131070 [ns] > > > > where the right value for pwm_l_count is 65535 while you calculate > > 65539 (and then quit with -ERANGE). > > I have applied the formula mentioned in v7 and got different duty > cycle result then what was expected. > > Formula referred by Uwe at v7: > pwm_l_count = (clk_rate * state->period) / NSEC_PER_SEC - pwm_h_count > > % clk_rate period duty_cycle NSEC_PER_SEC pwm_h_count pwm_l_count > 50% 333334 2001000 1000500 1000000000 333 667 > 25% 500000000 20000 5000 1000000000 2500 10000 > 50% 100000000 131070 65535 1000000000 6553 13107 For the first line: (clk_rate * state->period) // NSEC_PER_SEC - pwm_h_count = (333334 * 2001000) // 1000000000 - 333 = 667.001334 - 333 = 334 This gives duty cycle = 333 * 1000000000 / 333334 = 998998.0020039959 ns and a period = (333 + 334) * 1000000000 / 333334 = 2000995.998008004 ns which is well in the limits. Thank you for this clarification and I am clear in incorporating it in my next version. Is there any other feedback in this version v10? I guess you assumed my formula to be (clk_rate * state->period) / (NSEC_PER_SEC - pwm_h_count), but that's not what I meant. > Whereas the below table is the result of minor modification from your formula and getting the right result. > pwm_l_count = clk_rate * (state->period - state->duty_cycle) / > NSEC_PER_SEC - pwm_h_count > > % clk_rate period duty_cycle NSEC_PER_SEC pwm_h_count pwm_l_count > 50% 333334 2001000 1000500 1000000000 333 333 > 25% 500000000 20000 5000 1000000000 2500 7500 > 50% 100000000 131070 65535 1000000000 6553 6553 > > Please review this and confirm. In the code you used clk_rate * (state->period - state->duty_cycle) / (NSEC_PER_SEC - pwm_h_count) which is notably different. For the example in the first line again you then get 333, which is a less optimal result than 334 I get with my (fixed) formula. I'm still convinced my formula is the right and optimal one. Best regards Uwe
Hello Ayyathurai, can you please fix your mailer to quote properly? On Tue, Oct 13, 2020 at 02:54:31AM +0000, Ayyathurai, Vijayakannan wrote: > Thank you for this clarification and I am clear in incorporating it in > my next version. Is there any other feedback in this version v10? I won't promise that I will not spot something in your v11, but currently I told you about all issues I saw with v10. Best regards Uwe
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..ff362520d3cd --- /dev/null +++ b/drivers/pwm/pwm-keembay.c @@ -0,0 +1,230 @@ +// 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 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_LEADIN_OFFSET(pwm->hwpwm)); + if (buff & KMB_PWM_EN_BIT) + state->enabled = true; + else + state->enabled = false; + + /* Read period and duty cycle */ + buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm)); + pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC; + pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC; + state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate); + state->period = DIV_ROUND_UP_ULL(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; + + if (state->polarity != PWM_POLARITY_NORMAL) + return -ENOSYS; + + 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 of the KMB_PWM_HIGHLOW_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 + * low time = clock rate * (period - duty cycle) + */ + + clk_rate = clk_get_rate(priv->clk); + /* Configure waveform high time */ + div = clk_rate * state->duty_cycle; + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC); + if (div > KMB_PWM_COUNT_MAX) + return -ERANGE; + + pwm_h_count = div; + /* Configure waveform low time */ + div = clk_rate * (state->period - state->duty_cycle); + div = DIV_ROUND_DOWN_ULL(div, NSEC_PER_SEC - pwm_h_count); + if (div > KMB_PWM_COUNT_MAX) + return -ERANGE; + + pwm_l_count = div; + + pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) | + FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count); + + 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); + + clk_disable_unprepare(priv->clk); + + 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");