Re: [RFC PATCH v3 2/2] pwm: imx: Configure output to GPIO in disabled state
From: Michal VokÃÄ
Date: Thu Jan 24 2019 - 05:12:18 EST
On 24.1.2019 10:22, Uwe Kleine-KÃnig wrote:
On Thu, Jan 24, 2019 at 09:59:47AM +0100, Michal VokÃÄ wrote:
On 12.12.2018 13:12, Uwe Kleine-KÃnig wrote:
On Wed, Dec 12, 2018 at 11:42:17AM +0000, VokÃÄ Michal wrote:
On 12.12.2018 09:01, Uwe Kleine-KÃnig wrote:
On Thu, Dec 06, 2018 at 01:41:31PM +0000, VokÃÄ Michal wrote:
Normally the PWM output is held LOW when PWM is disabled. This can cause
problems when inverted PWM signal polarity is needed. With this behavior
the connected circuit is fed by 100% duty cycle instead of being shut-off.
Allow users to define a "pwm" and a "gpio" pinctrl states. The pwm pinctrl
state is selected when PWM is enabled and the gpio pinctrl state is
selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
configured as input and the internal pull-up resistor is used to pull the
output level high.
If all the pinctrl states and the pwm-gpios GPIO are not correctly
specified in DT the PWM work as usual.
As an example, with this patch a PWM controlled backlight with inversed
signal polarity can be used in full brightness range. Without this patch
the backlight can not be turned off as brightness = 0 disables the PWM
and that in turn set PWM output LOW, that is full brightness.
Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
+--------------+------------+---------------+----------- +-------------+
| After reset | Bootloader | PWM probe | PWM | PWM |
| 100k pull-up | | | enable 30% | disable |
+--------------+------------+---------------+------------+-------------+
| pinctrl | none | default | default | default |
| out H __________________ __ __ |
| out L \_________________/ \_/ \_/\____________ |
| ^ ^ ^ |
+--------------+------------+---------------+------------+-------------+
| pinctrl | none | gpio | pwm | gpio |
| out H __________________________________ __ __ _____________ |
| out L \_/ \_/ \_/ |
| ^ ^ ^ |
+----------------------------------------------------------------------+
Signed-off-by: Michal VokÃÄ <michal.vokac@xxxxxxxxx>
---
Changes in v3:
- Commit message update.
- Minor fix in code comment (Uwe)
- Align function arguments to the opening parentheses. (Uwe)
- Do not test devm_pinctrl_get for NULL. (Thierry)
- Convert all messages to dev_dbg() (Thierry)
- Do not actively drive the pin in gpio state. Configure it as input
and rely solely on the internal pull-up. (Thierry)
Changes in v2:
- Utilize the "pwm" and "gpio" pinctrl states.
- Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
- Select the right pinctrl state in probe.
drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
[ snip ]
On thing I noticed while looking at the rcar driver is: This doesn't
wait for the current period to end. Is this supposed to happen? Also for
the enable case the hardware is configured for the desired duty cycle
and only then the pinctrl is switched to pwm. Both might result in a
spike that is not desired.
The behavior should not change from how imx-pwm was working before.
When PWM is disabled the output is immediately gated off (put into
the idle state) independently on the period. I measured this.
Oh really, I wasn't aware of that. This is another bug in the imx pwm
then (I think).
I kind of expect that when hit a disable button the output is immediately
put into the idle state. To me it does not seem appropriate to wait the
whole period, or even just the active part of the period. If duty=100%
and period=4s (current maximum), in the worst case you would have to wait
4s until you stop the PWM. Quite a long time of driving something you
actually wanted to be shut off.
I think it might be beneficial to allow (or require) that disable acts
immediately. But this is not how it used to be and in my discussion with
Thierry (IIRC) he required to complete the currently running period.
I am confused here. IFAIK it always used to be that:
pwm_apply_state(pwm, { .enabled = 0 });
immediately stops the PWM with:
writel(0, imx->mmio_base + MX3_PWMCR);
regardless of the period (for pwm-imx).
Then if a user requires a "smooth disable" (i.e. with completing the
currently running period) they can do:
pwm_apply_state(pwm, { .duty_cycle = 0, ... });
pwm_apply_state(pwm, { .enabled = 0 });
to get the delayed disable.
But having said this, this should be a concious decision with the
requirements properly documented and the drivers users reviewed and adapted.
For the enable case you would certainly not get any additional spikes.
Yes, there is a possibility for a spike: If you configure for say 40%:
_ _
pwm output : ___/ \__/ \__
muxing : GPIO| PWM_
actual output: ____/\__/ \__
OK, you are right.
The worst thing that may happen is that the first period will be
slightly shorter depending on how long it takes to test the pinctrl
and switch the muxing. And this is unavoidable, it would happen even
if you wait for the start of a period. The test + muxing still takes
some time.
You could first configure for duty_cycle 0 and a short period, then mux
to PWM and then configure the correct duty cycle. Ditto for disable.
I agree it can be solved for the enable case but I do not see the
point in doing so for the disable case. Can you elaborate on how it
could be useful?
Assuming we stick to "disable should complete the currently running
period" it is all about preventing spikes. So if you switch the mux from
PWM to GPIO when the output just raised, that might (depending on the
use case) be bad and so it should be possible for the user to prevent
this.
I understand. The issue is I do not tend to call this case "a spike".
To me it means that the first and last period may have a shorter duty
cycle than requested. I tend to think that shorter duty cycle is not
harmful/bad. It actually means that you enable/disable the device
more smoothly.
Off course everything depends on the device that is fed with the signal.
So I totaly agree that being able to produce the signal with all the
periods of the same length and with the same duty is a good thing.
This is what I came up with:
[...]
What do you think?
I'm missing context. Your patch is on top of your last patch I assume.
Sorry, I had to add some context. It is against current linux-next and
it is just the relevant hunk for the pwm_imx27_apply() function.
Would you mind to send the squashed result?
Of course I do not mind. I can respin and send what I have as v5
against linux-next as I already rebased on top of your patches with
the split driver, if you prefer that?
(I'm unsure when I find time (and nerve) for a proper review, but I will
try to at least look over your suggestions.)
Thank you very much Uwe!
Michal