This series adds a few related fixes to the pwm .apply and .get_state
callbacks.
The first patch was originally part of the series adding Armada 8K/7K pwm
support. I split it out to a separate series following review comments from
Uwe Kleine-König who spotted a few more issues. There is no dependency between
this and the Armada 8K/7K series.
v3:
* Improve patch 3/5 description (Uwe)
* Add more Reviewed-by tags from Uwe
v2:
Address Uwe Kleine-König comments.
* Improve patch 1/5 summary line
* Add more information to patch 1/5 description
* Add more information to patch 2/5 description
* Don't round period/duty_cycle up in .apply (patch 3/5)
* Expand the comment in path 5/5 based on RMK's analysis of hardware
behaviour
* Add Uwe's Reviewed-by tags
Baruch Siach (5):
gpio: mvebu: fix pwm .get_state period calculation
gpio: mvebu: improve pwm period calculation accuracy
gpio: mvebu: make pwm .get_state closer to idempotent
gpio: mvebu: don't limit pwm period/duty_cycle to UINT_MAX
gpio: mvebu: document zero pwm duty cycle limitation
drivers/gpio/gpio-mvebu.c | 33 +++++++++++++++------------------
1 file changed, 15 insertions(+), 18 deletions(-)
--
2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The period is the sum of on and off values. That is, calculate period as
($on + $off) / clkrate
instead of
$off / clkrate - $on / clkrate
that makes no sense.
Reported-by: Russell King <linux@armlinux.org.uk>
Reviewed-by: Uwe Kleine-König <redacted>
Fixes: 757642f9a584e ("gpio: mvebu: Add limited PWM support")
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
drivers/gpio/gpio-mvebu.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,elsestate->duty_cycle=1;+val=(unsignedlonglong)u;/* on duration */regmap_read(mvpwm->regs,mvebu_pwmreg_blink_off_duration(mvpwm),&u);-val=(unsignedlonglong)u*NSEC_PER_SEC;+val+=(unsignedlonglong)u;/* period = on + off duration */+val*=NSEC_PER_SEC;do_div(val,mvpwm->clk_rate);-if(val<state->duty_cycle){+if(val>UINT_MAX)+state->period=UINT_MAX;+elseif(val)+state->period=val;+elsestate->period=1;-}else{-val-=state->duty_cycle;-if(val>UINT_MAX)-state->period=UINT_MAX;-elseif(val)-state->period=val;-else-state->period=1;-}regmap_read(mvchip->regs,GPIO_BLINK_EN_OFF+mvchip->offset,&u);if(u)
--
2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Round up the divisions in .get_state() to make applying the read out
configuration idempotent in most cases as .apply rounds down.
Reported-by: Uwe Kleine-König <redacted>
Reviewed-by: Uwe Kleine-König <redacted>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
drivers/gpio/gpio-mvebu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
PWM on/off registers are limited to UINT_MAX. However the state period
and duty_cycle fields are ns values of type u64. There is no reason to
limit them to UINT_MAX.
Reported-by: Uwe Kleine-König <redacted>
Reviewed-by: Uwe Kleine-König <redacted>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
drivers/gpio/gpio-mvebu.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
This is too easy. The right thing to do is to adapt .apply and
.get_state to use this new information.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
This is too easy. The right thing to do is to adapt .apply and
.get_state to use this new information.
What exactly do you expect the changes to be?
Bear in mind that the hardware is not capable of atomically updating
e.g. the duty cycle without affecting the period, because any change
in duty cycle needs the "on" and "off" durations to be separately
programmed, and there's a chance that the hardware could start using
either value mid-update.
Also, disabling "blink" mode to achieve a steady output (for 0% or 100%
duty cycle) would require further investigation to find out how the
hardware behaves at the moment where blink mode is disabled: does the
output retain its current state (does the bit in the output register
toggle with the blink) or does it revert to the value in the output
register that was programmed before blink mode was enabled.
Again, none of that is documented, so would need experimentation with
the hardware to work out how to achieve it.
And then if you want even more complexity, I suppose we could try and
read the current state of the pin, add a delay, recheck it and try and
work out the optimal place to disable the blink mode.
Exactly how far do you want to go with this?
All of this is likely getting rediculously complicated for the use
cases of it today that don't need it. Yes, it's annoying that we can't
achieve 0% or 100% duty cycle with this hardware that was never
designed as a PWM without jumping through a lot of hoops but currently
settle for a minimum pulse width of 4ns at each end of the range.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
This is too easy. The right thing to do is to adapt .apply and
.get_state to use this new information.
What exactly do you expect the changes to be?
What I expect is:
- let .apply() write 0 if the intention is to configure 2^32 clock
steps for the on or off register; and symmetrically
- let .get_state report 2^32 * NSEC_PER_SEC / clk_rate if the register
value is 0.
Bear in mind that the hardware is not capable of atomically updating
e.g. the duty cycle without affecting the period, because any change
in duty cycle needs the "on" and "off" durations to be separately
programmed, and there's a chance that the hardware could start using
either value mid-update.
Also, disabling "blink" mode to achieve a steady output (for 0% or 100%
duty cycle) would require further investigation to find out how the
hardware behaves at the moment where blink mode is disabled: does the
output retain its current state (does the bit in the output register
toggle with the blink) or does it revert to the value in the output
register that was programmed before blink mode was enabled.
I have some plans here about what is the right behaviour, but this needs
some preparatory work that I didn't do yet. I'll come back to this
eventually.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |