From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: Baruch Siach <baruch@tkos.co.il> Cc: Thierry Reding <thierry.reding@gmail.com>, Lee Jones <lee.jones@linaro.org>, Andy Gross <agross@kernel.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, Balaji Prakash J <bjagadee@codeaurora.org>, Rob Herring <robh+dt@kernel.org>, Robert Marko <robert.marko@sartura.hr>, Kathiravan T <kathirav@codeaurora.org>, linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kernel@pengutronix.de Subject: Re: [PATCH v8 2/4] pwm: driver for qualcomm ipq6018 pwm block Date: Tue, 14 Sep 2021 14:49:59 +0200 [thread overview] Message-ID: <20210914124959.spwjiifvysposhls@pengutronix.de> (raw) In-Reply-To: <bdc61569e4068490f53f347dcf29ee9539a8bc0b.1630323987.git.baruch@tkos.co.il> [-- Attachment #1: Type: text/plain, Size: 9525 bytes --] Hello Baruch, On Mon, Aug 30, 2021 at 02:46:25PM +0300, Baruch Siach wrote: > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index c76adedd58c9..08add845596f 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -260,6 +260,18 @@ config PWM_INTEL_LGM > To compile this driver as a module, choose M here: the module > will be called pwm-intel-lgm. > > +config PWM_IPQ > + tristate "IPQ PWM support" > + depends on ARCH_QCOM || COMPILE_TEST > + depends on HAVE_CLK && HAS_IOMEM > + help > + Generic PWM framework driver for IPQ PWM block which supports > + 4 pwm channels. Each of the these channels can be configured > + independent of each other. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-ipq. > + > config PWM_IQS620A > tristate "Azoteq IQS620A PWM support" > depends on MFD_IQS62X || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 708840b7fba8..7402feae4b36 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > obj-$(CONFIG_PWM_INTEL_LGM) += pwm-intel-lgm.o > +obj-$(CONFIG_PWM_IPQ) += pwm-ipq.o > obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o > diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c > new file mode 100644 > index 000000000000..8405d0554951 > --- /dev/null > +++ b/drivers/pwm/pwm-ipq.c > @@ -0,0 +1,275 @@ > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 > +/* > + * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/math64.h> > +#include <linux/of_device.h> > +#include <linux/mfd/syscon.h> > +#include <linux/regmap.h> > +#include <linux/bitfield.h> > + > +/* The frequency range supported is 1 Hz to clock rate */ > +#define IPQ_PWM_MAX_PERIOD_NS ((u64)NSEC_PER_SEC) > + > +/* > + * The max value specified for each field is based on the number of bits > + * in the pwm control register for that field > + */ > +#define IPQ_PWM_MAX_DIV 0xFFFF > + > +/* > + * Two 32-bit registers for each PWM: REG0, and REG1. > + * Base offset for PWM #i is at 8 * #i. > + */ > +#define IPQ_PWM_CFG_REG0 0 /*PWM_DIV PWM_HI*/ > +#define IPQ_PWM_REG0_PWM_DIV GENMASK(15, 0) > +#define IPQ_PWM_REG0_HI_DURATION GENMASK(31, 16) Assuming that IPQ_PWM_REG0_PWM_DIV is a field in IPQ_PWM_CFG_REG0: I wonder why the former has not "CFG" in it's name?! Ditto below. > +#define IPQ_PWM_CFG_REG1 4 /*ENABLE UPDATE PWM_PRE_DIV*/ > +#define IPQ_PWM_REG1_PRE_DIV GENMASK(15, 0) > +/* > + * Enable bit is set to enable output toggling in pwm device. > + * Update bit is set to reflect the changed divider and high duration > + * values in register. > + */ > +#define IPQ_PWM_REG1_UPDATE BIT(30) > +#define IPQ_PWM_REG1_ENABLE BIT(31) > + > + > +struct ipq_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + struct regmap *regmap; > + u32 regmap_off; > +}; > + > +static struct ipq_pwm_chip *to_ipq_pwm_chip(struct pwm_chip *chip) I would have called this ipq_pwm_from_chip() to have this function's name use the common prefix, too. (But note that Thierry might not agree here.) > +{ > + return container_of(chip, struct ipq_pwm_chip, chip); > +} > + > +static unsigned int ipq_pwm_reg_read(struct pwm_device *pwm, unsigned reg) checkpatch warns about this line > +{ > + struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip); > + unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg; > + unsigned int val; > + > + regmap_read(ipq_chip->regmap, off, &val); You don't expect regmap_read returning an error? Maybe note that in a comment to prevent someone preparing patches checking the error. Or alternatively add a WARN_ONCE when this fails? > + return val; > +} > + > +static void ipq_pwm_reg_write(struct pwm_device *pwm, unsigned reg, > + unsigned val) I expected that checkpatch warns here, too, and advises to align follow up lines to the opening ( in the previous line. So it's me who has to criticize that. > +{ > + struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip); > + unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg; > + > + regmap_write(ipq_chip->regmap, off, val); > +} > + > +static void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div, > + unsigned int pwm_div, unsigned long rate, u64 duty_ns, > + bool enable) > +{ > + unsigned long hi_dur; > + unsigned long val = 0; > + > + /* > + * high duration = pwm duty * (pwm div + 1) > + * pwm duty = duty_ns / period_ns > + */ > + hi_dur = div64_u64(duty_ns * rate, (pre_div + 1) * NSEC_PER_SEC); > + > + val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) | > + FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div); > + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG0, val); > + > + val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div); > + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val); > + > + /* Enable needs a separate write to REG1 */ s/Enable/Updating REG1/ ? > + val |= IPQ_PWM_REG1_UPDATE; > + if (enable) > + val |= IPQ_PWM_REG1_ENABLE; > + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val); > +} > + > +static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip); > + unsigned long freq; > + unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div; > + long long diff; > + unsigned long rate = clk_get_rate(ipq_chip->clk); > + unsigned long min_diff = rate; > + u64 period_ns, duty_ns; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + if (state->period < div64_u64(NSEC_PER_SEC, rate)) > + return -ERANGE; > + > + period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS); > + duty_ns = min(state->duty_cycle, period_ns); > + > + /* freq in Hz for period in nano second */ > + freq = div64_u64(NSEC_PER_SEC, period_ns); You're loosing quite some precision here. Consider a clock rate of 266666667 Hz and period = 500000001 ns. Then we end up with freq = 1 (while the exact result is nearly 2) which results in diff below being too small. > + best_pre_div = IPQ_PWM_MAX_DIV; > + best_pwm_div = IPQ_PWM_MAX_DIV; > + /* Initial pre_div value such that pwm_div < IPQ_PWM_MAX_DIV */ > + pre_div = DIV64_U64_ROUND_UP(period_ns * rate, > + (u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1)); This is wrong, you need to round down here. (Consider cases where you need pre_div = 0.) > + > + for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) { > + pwm_div = DIV64_U64_ROUND_UP(period_ns * rate, > + (u64)NSEC_PER_SEC * (pre_div + 1)); > + pwm_div--; Can it happen that pwm_div is zero before it is decreased by one? Also you need to round down here; with rounding up the resulting period is bigger than the requested period (unless the division yields an exact integer). > + if (pre_div > pwm_div) > + break; A comment here why we can end the search would be good. > + /* > + * Make sure we can do 100% duty cycle where > + * hi_dur == pwm_div + 1 > + */ > + if (pwm_div > IPQ_PWM_MAX_DIV - 1) > + continue; > + > + diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1)) > + - (uint64_t)rate; > + > + if (diff < 0) /* period larger than requested */ > + continue; > + if (diff == 0) { /* bingo */ > + best_pre_div = pre_div; > + best_pwm_div = pwm_div; > + break; > + } > + if (diff < min_diff) { > + min_diff = diff; > + best_pre_div = pre_div; > + best_pwm_div = pwm_div; > + } > + } > + > + /* config divider values for the closest possible frequency */ > + config_div_and_duty(pwm, best_pre_div, best_pwm_div, > + rate, duty_ns, state->enabled); > + > + return 0; > +} > + > +static void ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip); > + unsigned long rate = clk_get_rate(ipq_chip->clk); > + unsigned int pre_div, pwm_div, hi_dur; > + u64 effective_div, hi_div; > + u32 reg0, reg1; > + > + reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG0); > + reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG1); > + > + state->polarity = PWM_POLARITY_NORMAL; > + state->enabled = reg1 & IPQ_PWM_REG1_ENABLE; > + > + pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0); > + hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0); > + pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1); > + > + /* No overflow here, both pre_div and pwm_div <= 0xffff */ > + effective_div = (u64)(pre_div + 1) * (pwm_div + 1); > + state->period = div64_u64(effective_div * NSEC_PER_SEC, rate); You have to round up here to make apply . get_state idempotent. > + hi_div = hi_dur * (pre_div + 1); > + state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate); > +} 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 --]
WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: Baruch Siach <baruch@tkos.co.il> Cc: Thierry Reding <thierry.reding@gmail.com>, Lee Jones <lee.jones@linaro.org>, Andy Gross <agross@kernel.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, Balaji Prakash J <bjagadee@codeaurora.org>, Rob Herring <robh+dt@kernel.org>, Robert Marko <robert.marko@sartura.hr>, Kathiravan T <kathirav@codeaurora.org>, linux-pwm@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kernel@pengutronix.de Subject: Re: [PATCH v8 2/4] pwm: driver for qualcomm ipq6018 pwm block Date: Tue, 14 Sep 2021 14:49:59 +0200 [thread overview] Message-ID: <20210914124959.spwjiifvysposhls@pengutronix.de> (raw) In-Reply-To: <bdc61569e4068490f53f347dcf29ee9539a8bc0b.1630323987.git.baruch@tkos.co.il> [-- Attachment #1.1: Type: text/plain, Size: 9525 bytes --] Hello Baruch, On Mon, Aug 30, 2021 at 02:46:25PM +0300, Baruch Siach wrote: > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index c76adedd58c9..08add845596f 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -260,6 +260,18 @@ config PWM_INTEL_LGM > To compile this driver as a module, choose M here: the module > will be called pwm-intel-lgm. > > +config PWM_IPQ > + tristate "IPQ PWM support" > + depends on ARCH_QCOM || COMPILE_TEST > + depends on HAVE_CLK && HAS_IOMEM > + help > + Generic PWM framework driver for IPQ PWM block which supports > + 4 pwm channels. Each of the these channels can be configured > + independent of each other. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-ipq. > + > config PWM_IQS620A > tristate "Azoteq IQS620A PWM support" > depends on MFD_IQS62X || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 708840b7fba8..7402feae4b36 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > obj-$(CONFIG_PWM_INTEL_LGM) += pwm-intel-lgm.o > +obj-$(CONFIG_PWM_IPQ) += pwm-ipq.o > obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o > diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c > new file mode 100644 > index 000000000000..8405d0554951 > --- /dev/null > +++ b/drivers/pwm/pwm-ipq.c > @@ -0,0 +1,275 @@ > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0 > +/* > + * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/math64.h> > +#include <linux/of_device.h> > +#include <linux/mfd/syscon.h> > +#include <linux/regmap.h> > +#include <linux/bitfield.h> > + > +/* The frequency range supported is 1 Hz to clock rate */ > +#define IPQ_PWM_MAX_PERIOD_NS ((u64)NSEC_PER_SEC) > + > +/* > + * The max value specified for each field is based on the number of bits > + * in the pwm control register for that field > + */ > +#define IPQ_PWM_MAX_DIV 0xFFFF > + > +/* > + * Two 32-bit registers for each PWM: REG0, and REG1. > + * Base offset for PWM #i is at 8 * #i. > + */ > +#define IPQ_PWM_CFG_REG0 0 /*PWM_DIV PWM_HI*/ > +#define IPQ_PWM_REG0_PWM_DIV GENMASK(15, 0) > +#define IPQ_PWM_REG0_HI_DURATION GENMASK(31, 16) Assuming that IPQ_PWM_REG0_PWM_DIV is a field in IPQ_PWM_CFG_REG0: I wonder why the former has not "CFG" in it's name?! Ditto below. > +#define IPQ_PWM_CFG_REG1 4 /*ENABLE UPDATE PWM_PRE_DIV*/ > +#define IPQ_PWM_REG1_PRE_DIV GENMASK(15, 0) > +/* > + * Enable bit is set to enable output toggling in pwm device. > + * Update bit is set to reflect the changed divider and high duration > + * values in register. > + */ > +#define IPQ_PWM_REG1_UPDATE BIT(30) > +#define IPQ_PWM_REG1_ENABLE BIT(31) > + > + > +struct ipq_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + struct regmap *regmap; > + u32 regmap_off; > +}; > + > +static struct ipq_pwm_chip *to_ipq_pwm_chip(struct pwm_chip *chip) I would have called this ipq_pwm_from_chip() to have this function's name use the common prefix, too. (But note that Thierry might not agree here.) > +{ > + return container_of(chip, struct ipq_pwm_chip, chip); > +} > + > +static unsigned int ipq_pwm_reg_read(struct pwm_device *pwm, unsigned reg) checkpatch warns about this line > +{ > + struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip); > + unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg; > + unsigned int val; > + > + regmap_read(ipq_chip->regmap, off, &val); You don't expect regmap_read returning an error? Maybe note that in a comment to prevent someone preparing patches checking the error. Or alternatively add a WARN_ONCE when this fails? > + return val; > +} > + > +static void ipq_pwm_reg_write(struct pwm_device *pwm, unsigned reg, > + unsigned val) I expected that checkpatch warns here, too, and advises to align follow up lines to the opening ( in the previous line. So it's me who has to criticize that. > +{ > + struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip); > + unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg; > + > + regmap_write(ipq_chip->regmap, off, val); > +} > + > +static void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div, > + unsigned int pwm_div, unsigned long rate, u64 duty_ns, > + bool enable) > +{ > + unsigned long hi_dur; > + unsigned long val = 0; > + > + /* > + * high duration = pwm duty * (pwm div + 1) > + * pwm duty = duty_ns / period_ns > + */ > + hi_dur = div64_u64(duty_ns * rate, (pre_div + 1) * NSEC_PER_SEC); > + > + val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) | > + FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div); > + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG0, val); > + > + val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div); > + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val); > + > + /* Enable needs a separate write to REG1 */ s/Enable/Updating REG1/ ? > + val |= IPQ_PWM_REG1_UPDATE; > + if (enable) > + val |= IPQ_PWM_REG1_ENABLE; > + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val); > +} > + > +static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip); > + unsigned long freq; > + unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div; > + long long diff; > + unsigned long rate = clk_get_rate(ipq_chip->clk); > + unsigned long min_diff = rate; > + u64 period_ns, duty_ns; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + if (state->period < div64_u64(NSEC_PER_SEC, rate)) > + return -ERANGE; > + > + period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS); > + duty_ns = min(state->duty_cycle, period_ns); > + > + /* freq in Hz for period in nano second */ > + freq = div64_u64(NSEC_PER_SEC, period_ns); You're loosing quite some precision here. Consider a clock rate of 266666667 Hz and period = 500000001 ns. Then we end up with freq = 1 (while the exact result is nearly 2) which results in diff below being too small. > + best_pre_div = IPQ_PWM_MAX_DIV; > + best_pwm_div = IPQ_PWM_MAX_DIV; > + /* Initial pre_div value such that pwm_div < IPQ_PWM_MAX_DIV */ > + pre_div = DIV64_U64_ROUND_UP(period_ns * rate, > + (u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1)); This is wrong, you need to round down here. (Consider cases where you need pre_div = 0.) > + > + for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) { > + pwm_div = DIV64_U64_ROUND_UP(period_ns * rate, > + (u64)NSEC_PER_SEC * (pre_div + 1)); > + pwm_div--; Can it happen that pwm_div is zero before it is decreased by one? Also you need to round down here; with rounding up the resulting period is bigger than the requested period (unless the division yields an exact integer). > + if (pre_div > pwm_div) > + break; A comment here why we can end the search would be good. > + /* > + * Make sure we can do 100% duty cycle where > + * hi_dur == pwm_div + 1 > + */ > + if (pwm_div > IPQ_PWM_MAX_DIV - 1) > + continue; > + > + diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1)) > + - (uint64_t)rate; > + > + if (diff < 0) /* period larger than requested */ > + continue; > + if (diff == 0) { /* bingo */ > + best_pre_div = pre_div; > + best_pwm_div = pwm_div; > + break; > + } > + if (diff < min_diff) { > + min_diff = diff; > + best_pre_div = pre_div; > + best_pwm_div = pwm_div; > + } > + } > + > + /* config divider values for the closest possible frequency */ > + config_div_and_duty(pwm, best_pre_div, best_pwm_div, > + rate, duty_ns, state->enabled); > + > + return 0; > +} > + > +static void ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip); > + unsigned long rate = clk_get_rate(ipq_chip->clk); > + unsigned int pre_div, pwm_div, hi_dur; > + u64 effective_div, hi_div; > + u32 reg0, reg1; > + > + reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG0); > + reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG1); > + > + state->polarity = PWM_POLARITY_NORMAL; > + state->enabled = reg1 & IPQ_PWM_REG1_ENABLE; > + > + pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0); > + hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0); > + pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1); > + > + /* No overflow here, both pre_div and pwm_div <= 0xffff */ > + effective_div = (u64)(pre_div + 1) * (pwm_div + 1); > + state->period = div64_u64(effective_div * NSEC_PER_SEC, rate); You have to round up here to make apply . get_state idempotent. > + hi_div = hi_dur * (pre_div + 1); > + state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate); > +} Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-09-14 12:50 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-30 11:46 [PATCH v8 1/4] dt-bindings: mfd: qcom,tcsr: document ipq6018 compatible Baruch Siach 2021-08-30 11:46 ` [PATCH v8 1/4] dt-bindings: mfd: qcom, tcsr: " Baruch Siach 2021-08-30 11:46 ` [PATCH v8 2/4] pwm: driver for qualcomm ipq6018 pwm block Baruch Siach 2021-08-30 11:46 ` Baruch Siach 2021-09-14 12:49 ` Uwe Kleine-König [this message] 2021-09-14 12:49 ` Uwe Kleine-König 2021-12-14 16:05 ` Baruch Siach 2021-12-14 16:05 ` Baruch Siach 2021-08-30 11:46 ` [PATCH v8 3/4] dt-bindings: pwm: add IPQ6018 binding Baruch Siach 2021-08-30 11:46 ` Baruch Siach 2021-08-31 19:02 ` Rob Herring 2021-08-31 19:02 ` Rob Herring 2021-08-30 11:46 ` [PATCH v8 4/4] arm64: dts: ipq6018: add pwm node Baruch Siach 2021-08-30 11:46 ` Baruch Siach 2021-09-22 14:10 ` [PATCH v8 1/4] dt-bindings: mfd: qcom,tcsr: document ipq6018 compatible Lee Jones 2021-09-22 14:10 ` Lee Jones 2021-11-04 9:27 ` Uwe Kleine-König 2021-11-04 9:27 ` Uwe Kleine-König
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210914124959.spwjiifvysposhls@pengutronix.de \ --to=u.kleine-koenig@pengutronix.de \ --cc=agross@kernel.org \ --cc=baruch@tkos.co.il \ --cc=bjagadee@codeaurora.org \ --cc=bjorn.andersson@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=kathirav@codeaurora.org \ --cc=kernel@pengutronix.de \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=robert.marko@sartura.hr \ --cc=robh+dt@kernel.org \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.