This patch add support for longer pwm period configuration,
which allowing blinking LEDs via pwm interface.
Sam Shih (1):
pwm: mediatek: add longer period support
drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
--
2.17.1
The pwm clock source could be divided by 1625 with PWM_CON
BIT(3) setting in mediatek hardware.
This patch add support for longer pwm period configuration,
which allowing blinking LEDs via pwm interface.
Signed-off-by: Sam Shih <[email protected]>
---
drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index b94e0d09c300..9af309bea01a 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
int duty_ns, int period_ns)
{
struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
- u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
- reg_thres = PWMTHRES;
+ u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty,
+ reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
u64 resolution;
int ret;
@@ -141,9 +141,18 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
clkdiv++;
cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000,
resolution);
+ if (clkdiv > PWM_CLK_DIV_MAX && !clksel) {
+ clksel = 1;
+ clkdiv = 0;
+ resolution = (u64)NSEC_PER_SEC * 1000 * 1625;
+ do_div(resolution,
+ clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
+ cnt_period = DIV_ROUND_CLOSEST_ULL(
+ (u64)period_ns * 1000, resolution);
+ }
}
- if (clkdiv > PWM_CLK_DIV_MAX) {
+ if (clkdiv > PWM_CLK_DIV_MAX && clksel) {
pwm_mediatek_clk_disable(chip, pwm);
dev_err(chip->dev, "period %d not supported\n", period_ns);
return -EINVAL;
@@ -159,7 +168,11 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
}
cnt_duty = DIV_ROUND_CLOSEST_ULL((u64)duty_ns * 1000, resolution);
- pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
+ if (clksel)
+ pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) |
+ clkdiv);
+ else
+ pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period);
pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty);
--
2.17.1
On Thu, Feb 27, 2020 at 03:46:50PM +0800, Sam Shih wrote:
> The pwm clock source could be divided by 1625 with PWM_CON
> BIT(3) setting in mediatek hardware.
>
> This patch add support for longer pwm period configuration,
> which allowing blinking LEDs via pwm interface.
>
> Signed-off-by: Sam Shih <[email protected]>
> ---
> drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index b94e0d09c300..9af309bea01a 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
> int duty_ns, int period_ns)
> {
> struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
> - u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
> - reg_thres = PWMTHRES;
> + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty,
> + reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
> u64 resolution;
> int ret;
>
Adding some more context:
> @@ -139,11 +139,20 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
> while (cnt_period > 8191) {
> resolution *= 2;
> clkdiv++;
> cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000,
> resolution);
> + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) {
> + clksel = 1;
> + clkdiv = 0;
> + resolution = (u64)NSEC_PER_SEC * 1000 * 1625;
> + do_div(resolution,
> + clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
> + cnt_period = DIV_ROUND_CLOSEST_ULL(
> + (u64)period_ns * 1000, resolution);
The assignment is a repetition from just above the if. Maybe just put it
once after this if block?
> + }
> }
>
> - if (clkdiv > PWM_CLK_DIV_MAX) {
> + if (clkdiv > PWM_CLK_DIV_MAX && clksel) {
Is this change actually relevant? If the while loop that starts at line
139 is never run (because cnt_period is <= 8191) clkdiv is zero and so
the condition is false with and without "&& clksel". If however the
while loop is entered and clkdiv becomes bigger than PWM_CLK_DIV_MAX
clksel is 1 and the "&& clksel" doesn't make a difference, too.
The code is hard to follow, I wonder if this could be cleaned up with
some comments added that explain the hardware details enough to be able
to actually understand the code without having the hardware reference
manual handy.
> pwm_mediatek_clk_disable(chip, pwm);
> dev_err(chip->dev, "period %d not supported\n", period_ns);
> return -EINVAL;
> @@ -159,7 +168,11 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
> }
>
> cnt_duty = DIV_ROUND_CLOSEST_ULL((u64)duty_ns * 1000, resolution);
> - pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> + if (clksel)
> + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) |
> + clkdiv);
> + else
> + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period);
> pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty);
>
> --
> 2.17.1
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, 2020-02-27 at 09:04 +0100, Uwe Kleine-K?nig wrote:
On Thu, Feb 27, 2020 at 03:46:50PM +0800, Sam Shih wrote:
> > The pwm clock source could be divided by 1625 with PWM_CON
> > BIT(3) setting in mediatek hardware.
> >
> > This patch add support for longer pwm period configuration,
> > which allowing blinking LEDs via pwm interface.
> >
> > Signed-off-by: Sam Shih <[email protected]>
> > ---
> > drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++----
> > 1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index b94e0d09c300..9af309bea01a 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip
*chip, struct pwm_device *pwm,
> > int duty_ns, int period_ns)
> > {
> > struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
> > - u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
> > - reg_thres = PWMTHRES;
> > + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty,
> > + reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
> > u64 resolution;
> > int ret;
> >
> Adding some more context:
>
+ /* The pwm source clock can be divided by 2^clkdiv. When the clksel +
* bit is set to 1, The final clock output needs to be divided by an + *
extra 1625.
+ */
Is this ok ?
> > @@ -139,11 +139,20 @@ static int pwm_mediatek_config(struct pwm_chip
*chip, struct pwm_device *pwm,
> > while (cnt_period > 8191) {
> > resolution *= 2;
> > clkdiv++;
> > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000,
> > resolution);
> > + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) {
> > + clksel = 1;
> > + clkdiv = 0;
> > + resolution = (u64)NSEC_PER_SEC * 1000 * 1625;
> > + do_div(resolution,
> > + clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
> > + cnt_period = DIV_ROUND_CLOSEST_ULL(
> > + (u64)period_ns * 1000, resolution);
>
> The assignment is a repetition from just above the if. Maybe just put
it
> once after this if block?
The cnt_period represents the effective range of the PWM period counter,
when we need changing the pwm output period to a longer value at the
same clock frequency, we can setting a larger cnt_period, but the width
of the cnt_peroid register is 12 bits,
When the request period is too long, we need to divide the clock source
and then recalculate cnt_period outputs the correct waveform.
As mentioned above, when changing clkdiv, we need to recalculate
cnt_period immediately.
If the request period is very long (for example, LED blinking), clkdiv
may be insufficient.
In this case, we will use clksel to divide the pwm source clock by an
additional 1625, and recalculate clkdiv and cnt_period.
I don't think we can just place assignments after the if block.
>
> > + }
> > }
> >
> > - if (clkdiv > PWM_CLK_DIV_MAX) {
> > + if (clkdiv > PWM_CLK_DIV_MAX && clksel) {
>
> Is this change actually relevant? If the while loop that starts at
line
> 139 is never run (because cnt_period is <= 8191) clkdiv is zero and so
> the condition is false with and without "&& clksel". If however the
> while loop is entered and clkdiv becomes bigger than PWM_CLK_DIV_MAX
> clksel is 1 and the "&& clksel" doesn't make a difference, too.
>
You are right, I will remove this.
> The code is hard to follow, I wonder if this could be cleaned up with
> some comments added that explain the hardware details enough to be
able
> to actually understand the code without having the hardware reference
> manual handy.
>
Is it sufficient to add some context into comment like the response of
the second question?
> > pwm_mediatek_clk_disable(chip, pwm);
> > dev_err(chip->dev, "period %d not supported\n", period_ns);
> > return -EINVAL;
> > @@ -159,7 +168,11 @@ static int pwm_mediatek_config(struct pwm_chip
*chip, struct pwm_device *pwm,
> > }
> >
> > cnt_duty = DIV_ROUND_CLOSEST_ULL((u64)duty_ns * 1000, resolution);
> > - pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> > + if (clksel)
> > + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) |
> > + clkdiv);
> > + else
> > + pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> > pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period);
> > pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty);
> >
> > --
> > 2.17.1
>
> Best regards
> Uwe
>
>
Best Regards,
Sam
Hello,
On Thu, Feb 27, 2020 at 05:59:50PM +0800, Sam Shih wrote:
> On Thu, 2020-02-27 at 09:04 +0100, Uwe Kleine-K?nig wrote:
> On Thu, Feb 27, 2020 at 03:46:50PM +0800, Sam Shih wrote:
> > > The pwm clock source could be divided by 1625 with PWM_CON
> > > BIT(3) setting in mediatek hardware.
> > >
> > > This patch add support for longer pwm period configuration,
> > > which allowing blinking LEDs via pwm interface.
> > >
> > > Signed-off-by: Sam Shih <[email protected]>
> > > ---
> > > drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++----
> > > 1 file changed, 17 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > index b94e0d09c300..9af309bea01a 100644
> > > --- a/drivers/pwm/pwm-mediatek.c
> > > +++ b/drivers/pwm/pwm-mediatek.c
> > > @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip
> *chip, struct pwm_device *pwm,
> > > int duty_ns, int period_ns)
> > > {
> > > struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
> > > - u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
> > > - reg_thres = PWMTHRES;
> > > + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty,
> > > + reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
> > > u64 resolution;
> > > int ret;
> > >
> > Adding some more context:
> >
>
> + /* The pwm source clock can be divided by 2^clkdiv. When the clksel +
> * bit is set to 1, The final clock output needs to be divided by an + *
> extra 1625.
> + */
I'd write:
The source clock is divided by 2^clkdiv or iff the clksel bit is set by
2^clkdiv + 1625.
>
> Is this ok ?
>
>
> > > @@ -139,11 +139,20 @@ static int pwm_mediatek_config(struct pwm_chip
> *chip, struct pwm_device *pwm,
> > > while (cnt_period > 8191) {
> > > resolution *= 2;
> > > clkdiv++;
> > > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000,
> > > resolution);
> > > + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) {
> > > + clksel = 1;
> > > + clkdiv = 0;
> > > + resolution = (u64)NSEC_PER_SEC * 1000 * 1625;
> > > + do_div(resolution,
> > > + clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
> > > + cnt_period = DIV_ROUND_CLOSEST_ULL(
> > > + (u64)period_ns * 1000, resolution);
> >
> > The assignment is a repetition from just above the if. Maybe just put
> it
> > once after this if block?
>
> The cnt_period represents the effective range of the PWM period counter,
> when we need changing the pwm output period to a longer value at the
> same clock frequency, we can setting a larger cnt_period, but the width
> of the cnt_peroid register is 12 bits,
> When the request period is too long, we need to divide the clock source
> and then recalculate cnt_period outputs the correct waveform.
> As mentioned above, when changing clkdiv, we need to recalculate
> cnt_period immediately.
>
> If the request period is very long (for example, LED blinking), clkdiv
> may be insufficient.
> In this case, we will use clksel to divide the pwm source clock by an
> additional 1625, and recalculate clkdiv and cnt_period.
>
> I don't think we can just place assignments after the if block.
I didn't care enough to read your reasoning and retry to convince you
with mine:
With your patch you have:
cnt_period = someexpression;
if (somecondition) {
...
cnt_period = someexpression;
}
As somecondition doesn't make use of cnt_period this is equivalent to:
if (somecondition) {
...
}
cnt_period = someexpression;
isn't it?
> > The code is hard to follow, I wonder if this could be cleaned up with
> > some comments added that explain the hardware details enough to be able
> > to actually understand the code without having the hardware reference
> > manual handy.
>
> Is it sufficient to add some context into comment like the response of
> the second question?
I didn't check but I wouldn't be surprised if the code is more
complicated than necessary. If you don't see something to simplify, go
for adding an explanation as suggested and I will take a look in a quiet
moment.
Not sure I already pointed out that having a link to a publicly
available reference manual in the driver's header is useful. If there is
such a manual, please add a link there. Your benefit is that you
simplify others to improve your driver.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello,
>
> On Thu, Feb 27, 2020 at 05:59:50PM +0800, Sam Shih wrote:
> > On Thu, 2020-02-27 at 09:04 +0100, Uwe Kleine-König wrote:
> > On Thu, Feb 27, 2020 at 03:46:50PM +0800, Sam Shih wrote:
> > > > The pwm clock source could be divided by 1625 with PWM_CON
> > > > BIT(3) setting in mediatek hardware.
> > > >
> > > > This patch add support for longer pwm period configuration,
> > > > which allowing blinking LEDs via pwm interface.
> > > >
> > > > Signed-off-by: Sam Shih <[email protected]>
> > > > ---
> > > > drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++----
> > > > 1 file changed, 17 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-mediatek.c
b/drivers/pwm/pwm-mediatek.c
> > > > index b94e0d09c300..9af309bea01a 100644
> > > > --- a/drivers/pwm/pwm-mediatek.c
> > > > +++ b/drivers/pwm/pwm-mediatek.c
> > > > @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct
pwm_chip
> > *chip, struct pwm_device *pwm,
> > > > int duty_ns, int period_ns)
> > > > {
> > > > struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
> > > > - u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
> > > > - reg_thres = PWMTHRES;
> > > > + u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty,
> > > > + reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
> > > > u64 resolution;
> > > > int ret;
> > > >
> > > Adding some more context:
> > >
> >
> > + /* The pwm source clock can be divided by 2^clkdiv. When the
clksel +
> > * bit is set to 1, The final clock output needs to be divided by an
+ *
> > extra 1625.
> > + */
>
> I'd write:
>
> The source clock is divided by 2^clkdiv or iff the clksel bit is set
by
> 2^clkdiv + 1625.
>
Great, the comment is short and clear.
But maybe change “2^clkdiv + 1625” to “the product of 2^clkdiv and 1625”
is clearer ?
> >
> > Is this ok ?
> >
> >
> > > > @@ -139,11 +139,20 @@ static int pwm_mediatek_config(struct
pwm_chip
> > *chip, struct pwm_device *pwm,
> > > > while (cnt_period > 8191) {
> > > > resolution *= 2;
> > > > clkdiv++;
> > > > cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000,
> > > > resolution);
> > > > + if (clkdiv > PWM_CLK_DIV_MAX && !clksel) {
> > > > + clksel = 1;
> > > > + clkdiv = 0;
> > > > + resolution = (u64)NSEC_PER_SEC * 1000 * 1625;
> > > > + do_div(resolution,
> > > > + clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
> > > > + cnt_period = DIV_ROUND_CLOSEST_ULL(
> > > > + (u64)period_ns * 1000, resolution);
> > >
> > > The assignment is a repetition from just above the if. Maybe just
put
> > it
> > > once after this if block?
> >
> > The cnt_period represents the effective range of the PWM period
counter,
> > when we need changing the pwm output period to a longer value at the
> > same clock frequency, we can setting a larger cnt_period, but the
width
> > of the cnt_peroid register is 12 bits,
> > When the request period is too long, we need to divide the clock
source
> > and then recalculate cnt_period outputs the correct waveform.
> > As mentioned above, when changing clkdiv, we need to recalculate
> > cnt_period immediately.
> >
> > If the request period is very long (for example, LED blinking),
clkdiv
> > may be insufficient.
> > In this case, we will use clksel to divide the pwm source clock by
an
> > additional 1625, and recalculate clkdiv and cnt_period.
> >
> > I don't think we can just place assignments after the if block.
>
> I didn't care enough to read your reasoning and retry to convince you
> with mine:
>
> With your patch you have:
>
> cnt_period = someexpression;
>
> if (somecondition) {
> ...
> cnt_period = someexpression;
> }
>
> As somecondition doesn't make use of cnt_period this is equivalent to:
>
> if (somecondition) {
> ...
> }
> cnt_period = someexpression;
>
> isn't it?
>
Yes, you're right, I misunderstood.
Your code clearly reminded me.
I just want to explain that the re-calculation of the cnt_period is
important.
However, after reading your code, I think the program logic will not be
break and the duplicates can be removed.
> > > The code is hard to follow, I wonder if this could be cleaned up
with
> > > some comments added that explain the hardware details enough to be
able
> > > to actually understand the code without having the hardware
reference
> > > manual handy.
> >
> > Is it sufficient to add some context into comment like the response
of
> > the second question?
>
> I didn't check but I wouldn't be surprised if the code is more
> complicated than necessary. If you don't see something to simplify, go
> for adding an explanation as suggested and I will take a look in a
quiet
> moment.
>
I will send v2 patch to remove the repetition of “cnt_period =
someexpression”, and add some comment to clksel and cnt_period.
> Not sure I already pointed out that having a link to a publicly
> available reference manual in the driver's header is useful. If there
is
> such a manual, please add a link there. Your benefit is that you
> simplify others to improve your driver.
>
>
> Best regards
> Uwe
>
>
Best Regards.
Sam
Hello Sam,
On Thu, Feb 27, 2020 at 08:27:07PM +0800, Sam Shih wrote:
> > >
> > > + /* The pwm source clock can be divided by 2^clkdiv. When the clksel +
> > > * bit is set to 1, The final clock output needs to be divided by an + *
> > > extra 1625.
> > > + */
> >
> > I'd write:
> >
> > The source clock is divided by 2^clkdiv or iff the clksel bit is set by
> > 2^clkdiv + 1625.
> >
>
> Great, the comment is short and clear.
> But maybe change “2^clkdiv + 1625” to “the product of 2^clkdiv and 1625”
> is clearer ?
Writing a formula in words isn't helpful. If my formula was wrong use
the right one. I wrote
2^clkdiv + 1625
(which implicitly means (2^clkdiv) + 1625), if this is wrong write
2^clkdiv * 1625
or whatever is the right one then. And use parenthesis if you doubt
clearness.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |