| Message ID | 20180806155129.cjcc7okmwtaujf43@pengutronix.de |
|---|---|
| State | Rejected |
| Headers | show |
| Series | RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period) | expand |
On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > (If you have a déjà vu: Yes, I tried to argue this case already in the > past, it's just that I'm just faced with a new use case that's broken > with the current state.) > > Currently the idiom > > pwm_config(mypwm, value, period); > if (!value) > pwm_disable(mypwm); > else > pwm_enable(mypwm); > > (and it's equivalent: > > state.duty_cycle = ... > state.enable = state.duty_cycle ? true : false; > pwm_apply_state(...); > > ) is quite present in the kernel. > > In my eyes this is bad for two reasons: > > a) if the caller is supposed to call pwm_disable() after configuring the > duty cycle to zero, then why doesn't pwm_config() contains this to > unburden pwm-API users and so reduce open coding this assumption? That's because it's not true in all cases. pwm_disable() is not equivalent to setting the duty cycle to zero. Many users don't care about the difference, but that doesn't mean it should be assumed to be always the case. > b) it is actually wrong in at least two cases: > - On i.MX28 pwm_config(mypwm, 0, period) only programs a register > and only after the current period is elapsed this is clocked into > the hardware. So it is very likely that when pwm_disable() is > called the period is still not over, and then the clk is disabled > and the period never ends resulting in the pwm pin being frozen in > whatever state it happend to be when the clk was stopped. So that's a driver bug then. By definition, after the call to pwm_config() completes, the hardware should be in the configured state. If a device doesn't allow the programming to happen immediately, then it is up to the driver to ensure it waits long enough for the settings to have been properly applied. > - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period) > correctly ensures the PWM pin to stay at 1. But then pwm_disable() > puts it to 0. In the current case this implies that the backlight > is fully enabled instead of off. Again, that's a driver bug. pwm_disable() isn't supposed to change the pin polarity. It's not supposed to change the pin level. > Both issues are hard (or at least ugly) to fix. The IMHO most sane > approach is to not let PWM-API-users care about power saving when they > still care about the output level and update the samantic of > pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop > toggling, I don't care about the level of the output." in contrast to > the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was > called before, stop the pin at (maybe inverted) low level (for most pwms > at least).". I don't see how you could make that work. In practically all cases a pwm_disable() would have to be assumed to cause the pin level to become undefined. That makes it pretty much useless. > This is in my eyes more clear, removes code duplication and power saving > can (and should) be implemented in the pwm backend drivers that have the > domain specific knowledge about the effects of pwm_disable and > eventually unwanted side effects when duty cycle is 0 and just do the > corresponding action if its safe. We could even put something like: > > if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0) > pwm_disable(pwm); > > in pwm_config to help backend driver authors of "sane" PWMs. I agree that this is simpler and clearer. However it also significantly reduces the flexibility of the API, and I'm not sure that it is enough. Again, yes, for many cases this is sufficient, but it doesn't fully expose the capabilities of hardware. Thierry
Hello Thierry, On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote: > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: > > (If you have a déjà vu: Yes, I tried to argue this case already in the > > past, it's just that I'm just faced with a new use case that's broken > > with the current state.) > > > > Currently the idiom > > > > pwm_config(mypwm, value, period); > > if (!value) > > pwm_disable(mypwm); > > else > > pwm_enable(mypwm); > > > > (and it's equivalent: > > > > state.duty_cycle = ... > > state.enable = state.duty_cycle ? true : false; > > pwm_apply_state(...); > > > > ) is quite present in the kernel. > > > > In my eyes this is bad for two reasons: > > > > a) if the caller is supposed to call pwm_disable() after configuring the > > duty cycle to zero, then why doesn't pwm_config() contains this to > > unburden pwm-API users and so reduce open coding this assumption? > > That's because it's not true in all cases. pwm_disable() is not > equivalent to setting the duty cycle to zero. Many users don't care > about the difference, but that doesn't mean it should be assumed to > be always the case. I cannot follow you. You're saying that pwm_disable() is not the same as duty-cycle := 0. That's different from what I said. I only talked about pwm_disable() after config(pwm, duty=0, period). Actually the current semantic of pwm_disable isn't clear to me. Is it: "Freeze the pin in the current state"? If so, this isn't possible to implement for some hardware I know. If it's like that, the pin state is undefined after pwm_disable unless the duty-cycle was 0% or 100% before, right? > > b) it is actually wrong in at least two cases: > > - On i.MX28 pwm_config(mypwm, 0, period) only programs a register > > and only after the current period is elapsed this is clocked into > > the hardware. So it is very likely that when pwm_disable() is > > called the period is still not over, and then the clk is disabled > > and the period never ends resulting in the pwm pin being frozen in > > whatever state it happend to be when the clk was stopped. > > So that's a driver bug then. By definition, after the call to > pwm_config() completes, the hardware should be in the configured state. > If a device doesn't allow the programming to happen immediately, then it > is up to the driver to ensure it waits long enough for the settings to > have been properly applied. Unfortunately there is no way (that I know of at least) to notice when a period is over, so only an unconditional delay/sleep is the way out here. > > - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period) > > correctly ensures the PWM pin to stay at 1. But then pwm_disable() > > puts it to 0. In the current case this implies that the backlight > > is fully enabled instead of off. > > Again, that's a driver bug. pwm_disable() isn't supposed to change the > pin polarity. It's not supposed to change the pin level. So the disable callback must look as follows: def imx_pwm_disable(pwm): state = pwm_get_state(...) if state.polarity == PWM_POLARITY_INVERSED: return hw_access_to_disable_pwm() (Alternatively the pin could be configured to gpio, the right level set and the hw disable unconditionally.) And actually because for an inverted polarity after setting the duty_cycle to 100% it is possible to already disable the hardware in the config callback. And for the users that config for duty=0 but don't call pwm_disable() the symmetric case could be handled, too, so the config callback optimally looks like: def imx_pwm_config(chip, duty, period): state = pwm_get_state(...) if (duty == 0 and not state.polarity == PWM_POLARITY_INVERSED) || (duty == period and state.polarity == PWM_POLARITY_INVERSED): hw_access_to_disable_pwm() else: hw_access_to_config(duty, period) The obvious downsides are: a) if pwm_disable() was called because the user doesn't need the pwm to toggle any more, it stays enabled. b) pwm_disable() after pwm_config() that allows to disable the hardware, is a noop So it would be much saner from the backend implementations POV if the PWM-API user doesn't call pwm_disable() if he cares about the output level of the pin. And I cannot imagine any pwm implementation where this requirement would make the implementation worse. Unless I miss something currently there is no user-visible effect of calling pwm_disable after duty was set to 0. So as an API-user I cannot differentiate between - set the output to low; and - don't care about the pwm pin state, turn off and save power All downsides could be fixed if the only way to set the output to low would be pwm_config(pwm, 0, period); and pwm_disable() would imply to turn off with no guarantees about the pin state. This would simplify some drivers but not complicate any. This would make the API more expressive because it allows to differentiate between "output low" and "power off". The only downside I see is that all users must be reviewed and eventually fixed for the slightly different semantic. Parts of that was already done in my patch in the previous mail. > > Both issues are hard (or at least ugly) to fix. The IMHO most sane > > approach is to not let PWM-API-users care about power saving when they > > still care about the output level and update the samantic of > > pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop > > toggling, I don't care about the level of the output." in contrast to > > the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was > > called before, stop the pin at (maybe inverted) low level (for most pwms > > at least).". > > I don't see how you could make that work. In practically all cases a > pwm_disable() would have to be assumed to cause the pin level to become > undefined. That makes it pretty much useless. Right, that's what I want. pwm_disable() should make no guarantees about the pin level. So only call it if you don't care about the pin state. If you care, only do pwm_config(pwm, duty=0, period); and let the backend driver save power if it possible already in the corresponding callback. That is actually easier and more expressive! Currently the semantic of pwm_disable() is ambiguous. The two possible meanings are: - save power and keep the pin state - save power and the backend driver cannot know which one the caller actually wants. So the only way is to assume the stronger one and keep the pin state which results in more power usage for some hardware. > > This is in my eyes more clear, removes code duplication and power saving > > can (and should) be implemented in the pwm backend drivers that have the > > domain specific knowledge about the effects of pwm_disable and > > eventually unwanted side effects when duty cycle is 0 and just do the > > corresponding action if its safe. We could even put something like: > > > > if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0) > > pwm_disable(pwm); > > > > in pwm_config to help backend driver authors of "sane" PWMs. > > I agree that this is simpler and clearer. However it also significantly > reduces the flexibility of the API, and I'm not sure that it is enough. > Again, yes, for many cases this is sufficient, but it doesn't fully > expose the capabilities of hardware. Can you show me a use case that isn't possible to express with the suggested change in semantic? Best regards Uwe
Hello Thierry, On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote: > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote: > > I don't see how you could make that work. In practically all cases a > > pwm_disable() would have to be assumed to cause the pin level to become > > undefined. That makes it pretty much useless. > > Right, that's what I want. pwm_disable() should make no guarantees about > the pin level. [...] > > > I agree that this is simpler and clearer. However it also significantly > > reduces the flexibility of the API, and I'm not sure that it is enough. > > Again, yes, for many cases this is sufficient, but it doesn't fully > > expose the capabilities of hardware. > > Can you show me a use case that isn't possible to express with the > suggested change in semantic? To get forward here the only missing points are: a) Your claim that this simplification looses expressive power. b) Actually convert the users (assuming a) can be resolved) I cannot find a usecase that cannot be handled with the suggested change in semantic. So can I lure you in explaining what setup you have in mind? Best regards Uwe
Hello Thierry, On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote: > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote: > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote: > > > I don't see how you could make that work. In practically all cases a > > > pwm_disable() would have to be assumed to cause the pin level to become > > > undefined. That makes it pretty much useless. > > > > Right, that's what I want. pwm_disable() should make no guarantees about > > the pin level. [...] > > > > > I agree that this is simpler and clearer. However it also significantly > > > reduces the flexibility of the API, and I'm not sure that it is enough. > > > Again, yes, for many cases this is sufficient, but it doesn't fully > > > expose the capabilities of hardware. > > > > Can you show me a use case that isn't possible to express with the > > suggested change in semantic? > > To get forward here the only missing points are: > > a) Your claim that this simplification looses expressive power. > b) Actually convert the users (assuming a) can be resolved) > > I cannot find a usecase that cannot be handled with the suggested change > in semantic. So can I lure you in explaining what setup you have in > mind? I'd really like to get forward here, so you feedback would be very welcome. What is the use case you have in mind when writing "it also significantly reduces the flexibility of the API, and I'm not sure that it is enough"? Best regards Uwe
Hello Thierry, On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote: > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote: > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote: > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote: > > > > I don't see how you could make that work. In practically all cases a > > > > pwm_disable() would have to be assumed to cause the pin level to become > > > > undefined. That makes it pretty much useless. > > > > > > Right, that's what I want. pwm_disable() should make no guarantees about > > > the pin level. [...] > > > > > > > I agree that this is simpler and clearer. However it also significantly > > > > reduces the flexibility of the API, and I'm not sure that it is enough. > > > > Again, yes, for many cases this is sufficient, but it doesn't fully > > > > expose the capabilities of hardware. > > > > > > Can you show me a use case that isn't possible to express with the > > > suggested change in semantic? > > > > To get forward here the only missing points are: > > > > a) Your claim that this simplification looses expressive power. > > b) Actually convert the users (assuming a) can be resolved) > > > > I cannot find a usecase that cannot be handled with the suggested change > > in semantic. So can I lure you in explaining what setup you have in > > mind? > > I'd really like to get forward here, so you feedback would be very > welcome. What is the use case you have in mind when writing "it also > significantly reduces the flexibility of the API, and I'm not sure that > it is enough"? I'm a bit irritated that you don't continue to communicate here. On August 9 I had the impression that we can discuss this topic to an end. But as you stopped replying we unfortunately cannot :-| Best regards Uwe
Hello Thierry, On Fri, Sep 21, 2018 at 06:02:30PM +0200, Uwe Kleine-König wrote: > On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote: > > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote: > > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote: > > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote: > > > > > I don't see how you could make that work. In practically all cases a > > > > > pwm_disable() would have to be assumed to cause the pin level to become > > > > > undefined. That makes it pretty much useless. > > > > > > > > Right, that's what I want. pwm_disable() should make no guarantees about > > > > the pin level. [...] > > > > > > > > > I agree that this is simpler and clearer. However it also significantly > > > > > reduces the flexibility of the API, and I'm not sure that it is enough. > > > > > Again, yes, for many cases this is sufficient, but it doesn't fully > > > > > expose the capabilities of hardware. > > > > > > > > Can you show me a use case that isn't possible to express with the > > > > suggested change in semantic? > > > > > > To get forward here the only missing points are: > > > > > > a) Your claim that this simplification looses expressive power. > > > b) Actually convert the users (assuming a) can be resolved) > > > > > > I cannot find a usecase that cannot be handled with the suggested change > > > in semantic. So can I lure you in explaining what setup you have in > > > mind? > > > > I'd really like to get forward here, so you feedback would be very > > welcome. What is the use case you have in mind when writing "it also > > significantly reduces the flexibility of the API, and I'm not sure that > > it is enough"? > > I'm a bit irritated that you don't continue to communicate here. On > August 9 I had the impression that we can discuss this topic to an end. > But as you stopped replying we unfortunately cannot :-| Seeing you reply on other topics I wonder what is the problem here. Can you at least quickly confirm that you received my mails? Best regards Uwe
On Thu, Sep 27, 2018 at 08:26:07PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Fri, Sep 21, 2018 at 06:02:30PM +0200, Uwe Kleine-König wrote: > > On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote: > > > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote: > > > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote: > > > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote: > > > > > > I don't see how you could make that work. In practically all cases a > > > > > > pwm_disable() would have to be assumed to cause the pin level to become > > > > > > undefined. That makes it pretty much useless. > > > > > > > > > > Right, that's what I want. pwm_disable() should make no guarantees about > > > > > the pin level. [...] > > > > > > > > > > > I agree that this is simpler and clearer. However it also significantly > > > > > > reduces the flexibility of the API, and I'm not sure that it is enough. > > > > > > Again, yes, for many cases this is sufficient, but it doesn't fully > > > > > > expose the capabilities of hardware. > > > > > > > > > > Can you show me a use case that isn't possible to express with the > > > > > suggested change in semantic? > > > > > > > > To get forward here the only missing points are: > > > > > > > > a) Your claim that this simplification looses expressive power. > > > > b) Actually convert the users (assuming a) can be resolved) > > > > > > > > I cannot find a usecase that cannot be handled with the suggested change > > > > in semantic. So can I lure you in explaining what setup you have in > > > > mind? > > > > > > I'd really like to get forward here, so you feedback would be very > > > welcome. What is the use case you have in mind when writing "it also > > > significantly reduces the flexibility of the API, and I'm not sure that > > > it is enough"? > > > > I'm a bit irritated that you don't continue to communicate here. On > > August 9 I had the impression that we can discuss this topic to an end. > > But as you stopped replying we unfortunately cannot :-| > > Seeing you reply on other topics I wonder what is the problem here. Can > you at least quickly confirm that you received my mails? Confirming that I'm receiving your emails. Sorry, but I haven't found the necessary time to look at the details here any closer to continue the discussion in a meaningful way. I will hopefully get around to it within the next couple of days. Thierry
Hello Thierry, On Thu, Sep 27, 2018 at 10:29:55PM +0200, Thierry Reding wrote: > On Thu, Sep 27, 2018 at 08:26:07PM +0200, Uwe Kleine-König wrote: > > On Fri, Sep 21, 2018 at 06:02:30PM +0200, Uwe Kleine-König wrote: > > > On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote: > > > > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote: > > > > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote: > > > > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote: > > > > > > > I don't see how you could make that work. In practically all cases a > > > > > > > pwm_disable() would have to be assumed to cause the pin level to become > > > > > > > undefined. That makes it pretty much useless. > > > > > > > > > > > > Right, that's what I want. pwm_disable() should make no guarantees about > > > > > > the pin level. [...] > > > > > > > > > > > > > I agree that this is simpler and clearer. However it also significantly > > > > > > > reduces the flexibility of the API, and I'm not sure that it is enough. > > > > > > > Again, yes, for many cases this is sufficient, but it doesn't fully > > > > > > > expose the capabilities of hardware. > > > > > > > > > > > > Can you show me a use case that isn't possible to express with the > > > > > > suggested change in semantic? > > > > > > > > > > To get forward here the only missing points are: > > > > > > > > > > a) Your claim that this simplification looses expressive power. > > > > > b) Actually convert the users (assuming a) can be resolved) > > > > > > > > > > I cannot find a usecase that cannot be handled with the suggested change > > > > > in semantic. So can I lure you in explaining what setup you have in > > > > > mind? > > > > > > > > I'd really like to get forward here, so you feedback would be very > > > > welcome. What is the use case you have in mind when writing "it also > > > > significantly reduces the flexibility of the API, and I'm not sure that > > > > it is enough"? > > > > > > I'm a bit irritated that you don't continue to communicate here. On > > > August 9 I had the impression that we can discuss this topic to an end. > > > But as you stopped replying we unfortunately cannot :-| > > > > Seeing you reply on other topics I wonder what is the problem here. Can > > you at least quickly confirm that you received my mails? > > Confirming that I'm receiving your emails. Sorry, but I haven't found > the necessary time to look at the details here any closer to continue > the discussion in a meaningful way. > > I will hopefully get around to it within the next couple of days. I would really like to continue this discussion before it is swapped out of my mind again. Did you find the time to consider my thoughts? Best regards Uwe
On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > (If you have a déjà vu: Yes, I tried to argue this case already in the > past, it's just that I'm just faced with a new use case that's broken > with the current state.) > > Currently the idiom > > pwm_config(mypwm, value, period); > if (!value) > pwm_disable(mypwm); > else > pwm_enable(mypwm); > > (and it's equivalent: > > state.duty_cycle = ... > state.enable = state.duty_cycle ? true : false; > pwm_apply_state(...); > > ) is quite present in the kernel. > > In my eyes this is bad for two reasons: > > a) if the caller is supposed to call pwm_disable() after configuring the > duty cycle to zero, then why doesn't pwm_config() contains this to > unburden pwm-API users and so reduce open coding this assumption? Just to reiterate my thoughts on this: while the config/enable/disable split may seem arbitrary for some use-cases (you may argue most use-cases, and you'd probably be right), I think there's value in having the additional flexibility to explicitly turn off the PWM. Also, duty cycle of zero doesn't always mean "off". If your PWM is inverted, then the duty cycle of zero actually means "on". And that of course also still depends on other components on your board. You may have an inverter somewhere, or you may actually be driving a circuit that expects an inverted PWM. So saying that duty-cycle of 0 equals disable is simplifying things too much, in my opinion. > b) it is actually wrong in at least two cases: > - On i.MX28 pwm_config(mypwm, 0, period) only programs a register > and only after the current period is elapsed this is clocked into > the hardware. So it is very likely that when pwm_disable() is > called the period is still not over, and then the clk is disabled > and the period never ends resulting in the pwm pin being frozen in > whatever state it happend to be when the clk was stopped. > - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period) > correctly ensures the PWM pin to stay at 1. But then pwm_disable() > puts it to 0. In the current case this implies that the backlight > is fully enabled instead of off. I think I've explained this in the past: these are i.MX specific issues that you need to work around in the driver. It's also mostly orthogonal to the API issue because consider the case where you want to unload the PWM driver on your board. Your remove function would need disable all PWM channels in order to properly clean up the device, but if you do that, then your backlight would turn fully on, right? On that note, I'm not sure I understand how this would even work, since you should have the very same state on boot. I'm assuming here that the PWM will be off at boot time by default, in which case, given that it's disabled, it should cause the backlight to turn on. How do you solve that problem? Or if it's not a problem at boot, why does it become a problem on PWM disable or driver removal? What you're currently proposing is unjustified at this point: you're trying to work around an issue specific to i.MX by changing an API that has worked fine for everyone else for a decade. Thierry
Hello Thierry, On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote: > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: > > Hello Thierry, > > > > (If you have a déjà vu: Yes, I tried to argue this case already in the > > past, it's just that I'm just faced with a new use case that's broken > > with the current state.) > > > > Currently the idiom > > > > pwm_config(mypwm, value, period); > > if (!value) > > pwm_disable(mypwm); > > else > > pwm_enable(mypwm); > > > > (and it's equivalent: > > > > state.duty_cycle = ... > > state.enable = state.duty_cycle ? true : false; > > pwm_apply_state(...); > > > > ) is quite present in the kernel. > > > > In my eyes this is bad for two reasons: > > > > a) if the caller is supposed to call pwm_disable() after configuring the > > duty cycle to zero, then why doesn't pwm_config() contains this to > > unburden pwm-API users and so reduce open coding this assumption? > > Just to reiterate my thoughts on this: while the config/enable/disable > split may seem arbitrary for some use-cases (you may argue most > use-cases, and you'd probably be right), I think there's value in having > the additional flexibility to explicitly turn off the PWM. Also, duty > cycle of zero doesn't always mean "off". If your PWM is inverted, then > the duty cycle of zero actually means "on". And that of course also > still depends on other components on your board. Did you notice you didn't argue against me here? We seem to agree that calling disabling a PWM after setting the duty cycle to zero is bad. > You may have an inverter somewhere, or you may actually be driving a > circuit that expects an inverted PWM. > > So saying that duty-cycle of 0 equals disable is simplifying things too > much, in my opinion. Full ack. So let's fix the users to not call pwm_disable after setting the duty cycle to zero! Really, only the driver knows when it's safe to disable the clock while it's expected to keep the pin at a given state. So better don't expect a certain state after pwm_disable(). > > b) it is actually wrong in at least two cases: > > - On i.MX28 pwm_config(mypwm, 0, period) only programs a register > > and only after the current period is elapsed this is clocked into > > the hardware. So it is very likely that when pwm_disable() is > > called the period is still not over, and then the clk is disabled > > and the period never ends resulting in the pwm pin being frozen in > > whatever state it happend to be when the clk was stopped. > > - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period) > > correctly ensures the PWM pin to stay at 1. But then pwm_disable() > > puts it to 0. In the current case this implies that the backlight > > is fully enabled instead of off. > > I think I've explained this in the past: these are i.MX specific issues > that you need to work around in the driver. I'd say: The i.MX PWM doesn't match the idealised idea of a PWM that can (in a sane way) be mapped by the current PWM API. That's because there are assumptions in the API that are wrong for i.MX. Still the hardware allows to implement all use cases. That's a sign that the API is bad and is in need for generalisation. > It's also mostly orthogonal > to the API issue because consider the case where you want to unload the > PWM driver on your board. Your remove function would need disable all > PWM channels in order to properly clean up the device, but if you do > that, then your backlight would turn fully on, right? Right. I cannot fix all hardware problems in software. But with my suggested change I can at least make it as good as possible in software without having to resort to ugly hacks. > On that note, I'm not sure I understand how this would even work, since > you should have the very same state on boot. I'm assuming here that the > PWM will be off at boot time by default, in which case, given that it's > disabled, it should cause the backlight to turn on. How do you solve > that problem? Or if it's not a problem at boot, why does it become a > problem on PWM disable or driver removal? Most of the time it's not a problem during boot because the pin is muxed as input. But when the clock of the i.MX PWM is disabled it actively drives a zero. And I already had cases where there really was a problem during boot until the bootloader fixed the stuff in software. For these cases I need patches ("hacks") that remove calls to pwm_disable in the backlight driver to at least work around the issue during runtime, which is the best that can be done in software. > What you're currently proposing is unjustified at this point: you're > trying to work around an issue specific to i.MX by changing an API that > has worked fine for everyone else for a decade. I don't agree here. The PWM API as it is now would make it necessary that the imx driver becomes ugly. By changing the semantic of the API - the i.MX driver can be implemented nicely; - the other drivers for "saner" hardware can stay as they are; - the API users can still express all their needs; - the API stops having two ways to express the same needs; - most API users can be simplified because they can drop if (duty == 0) pwm_disable() - on i.MX the clock can be disabled when not needed saving some energy; Regarding you claim that the API worked fine for a decade: I tried already in 2013 to address this problem. And even independent of that, that is not a good reason to not improve stuff. Best regards Uwe
On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote: > > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: > > > Hello Thierry, > > > > > > (If you have a déjà vu: Yes, I tried to argue this case already in the > > > past, it's just that I'm just faced with a new use case that's broken > > > with the current state.) > > > > > > Currently the idiom > > > > > > pwm_config(mypwm, value, period); > > > if (!value) > > > pwm_disable(mypwm); > > > else > > > pwm_enable(mypwm); > > > > > > (and it's equivalent: > > > > > > state.duty_cycle = ... > > > state.enable = state.duty_cycle ? true : false; > > > pwm_apply_state(...); > > > > > > ) is quite present in the kernel. > > > > > > In my eyes this is bad for two reasons: > > > > > > a) if the caller is supposed to call pwm_disable() after configuring the > > > duty cycle to zero, then why doesn't pwm_config() contains this to > > > unburden pwm-API users and so reduce open coding this assumption? > > > > Just to reiterate my thoughts on this: while the config/enable/disable > > split may seem arbitrary for some use-cases (you may argue most > > use-cases, and you'd probably be right), I think there's value in having > > the additional flexibility to explicitly turn off the PWM. Also, duty > > cycle of zero doesn't always mean "off". If your PWM is inverted, then > > the duty cycle of zero actually means "on". And that of course also > > still depends on other components on your board. > > Did you notice you didn't argue against me here? We seem to agree that > calling disabling a PWM after setting the duty cycle to zero is bad. I don't see why you think I agree with that. The only thing I'm saying is that pwm_config() shouldn't assume that a duty-cycle of 0 means the same as the PWM being disabled. > > You may have an inverter somewhere, or you may actually be driving a > > circuit that expects an inverted PWM. > > > > So saying that duty-cycle of 0 equals disable is simplifying things too > > much, in my opinion. > > Full ack. So let's fix the users to not call pwm_disable after setting > the duty cycle to zero! I don't understand why you come to that conclusion. It seems like we at least agree on what the problem is and what potential issues there could be with different setups. The point that we don't agree with is how to fix this. > Really, only the driver knows when it's safe to disable the clock while > it's expected to keep the pin at a given state. So better don't expect a > certain state after pwm_disable(). Yes, I agree that only the driver knows when it is safe to do so, but I still fail to understand why we should change the API in order to accomodate the specifics of a particular driver. So we already know that currently all users expect that the pin state after disable will be low (because they previously set duty cycle to 0, which is the active equivalent of low). Given that stricter definition, the i.MX PWM driver becomes buggy because it doesn't guarantee that pin state. I think the fix for that is for the i.MX PWM driver to make sure the pin state doesn't actually change on ->disable(). If that means the clock needs to remain on, then that's exactly what the driver should be implementing. > > > b) it is actually wrong in at least two cases: > > > - On i.MX28 pwm_config(mypwm, 0, period) only programs a register > > > and only after the current period is elapsed this is clocked into > > > the hardware. So it is very likely that when pwm_disable() is > > > called the period is still not over, and then the clk is disabled > > > and the period never ends resulting in the pwm pin being frozen in > > > whatever state it happend to be when the clk was stopped. > > > - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period) > > > correctly ensures the PWM pin to stay at 1. But then pwm_disable() > > > puts it to 0. In the current case this implies that the backlight > > > is fully enabled instead of off. > > > > I think I've explained this in the past: these are i.MX specific issues > > that you need to work around in the driver. > > I'd say: The i.MX PWM doesn't match the idealised idea of a PWM that can > (in a sane way) be mapped by the current PWM API. That's because there > are assumptions in the API that are wrong for i.MX. Still the hardware > allows to implement all use cases. That's a sign that the API is bad and > is in need for generalisation. It's perfectly fine for the driver to make any of the PWM operations a no-op if that's what it takes to make it conform with the API. > > It's also mostly orthogonal > > to the API issue because consider the case where you want to unload the > > PWM driver on your board. Your remove function would need disable all > > PWM channels in order to properly clean up the device, but if you do > > that, then your backlight would turn fully on, right? > > Right. I cannot fix all hardware problems in software. But with my > suggested change I can at least make it as good as possible in software > without having to resort to ugly hacks. > > > On that note, I'm not sure I understand how this would even work, since > > you should have the very same state on boot. I'm assuming here that the > > PWM will be off at boot time by default, in which case, given that it's > > disabled, it should cause the backlight to turn on. How do you solve > > that problem? Or if it's not a problem at boot, why does it become a > > problem on PWM disable or driver removal? > > Most of the time it's not a problem during boot because the pin is muxed > as input. But when the clock of the i.MX PWM is disabled it actively > drives a zero. And I already had cases where there really was a problem > during boot until the bootloader fixed the stuff in software. For these > cases I need patches ("hacks") that remove calls to pwm_disable in the > backlight driver to at least work around the issue during runtime, which > is the best that can be done in software. Are you aware of this: http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987 This seems to address exactly the problem that you're describing. The solution matches what I had expected a fix for this problem to look like. Can you try it and see if that fixes the issue? It looks as if that also fixes the shutdown problem, so it's better than what you're proposing here yet it doesn't require any API change at all to make it work. Cc'ing Michal for visibility. > > What you're currently proposing is unjustified at this point: you're > > trying to work around an issue specific to i.MX by changing an API that > > has worked fine for everyone else for a decade. > > I don't agree here. The PWM API as it is now would make it necessary > that the imx driver becomes ugly. Beauty is very subjective. This doesn't count as an argument. Michal's proposed solution looks perfectly fine. > By changing the semantic of the API > > - the i.MX driver can be implemented nicely; > - the other drivers for "saner" hardware can stay as they are; > - the API users can still express all their needs; > - the API stops having two ways to express the same needs; Again, duty-cycle = 0 is not the same as setting the state to disabled. Practically they usually have the same effect, but that's just because in the typical use-case we don't care about the subtle differences. > - most API users can be simplified because they can drop > if (duty == 0) pwm_disable() > - on i.MX the clock can be disabled when not needed saving some energy; That's one reason why we have pwm_disable(). It's what you use to save some energy if you don't need the PWM. Also, you claim that if the clock is disabled the attached backlight will turn on at full brightness, so I fail to see how this would work. Also, you leave out the fact that even after all this work that touches all users and all PWM drivers you still don't get fully correct functionality on your devices because when the driver is removed or shut down you run into the same problem. > Regarding you claim that the API worked fine for a decade: I tried > already in 2013 to address this problem. And even independent of that, > that is not a good reason to not improve stuff. My claim was that it has worked fine "for everyone else". I know that this has been an issue on i.MX for a long time and I've done my best to lay out why I think your proposal is not the right way to fix it. The proposal from Michal shows that there is another way that properly fixes the issue, so let's focus on getting that to work for your use-cases. Thierry
Ahoj, thank you for bringing me to the discussion Thierry. On 10.10.2018 14:26, Thierry Reding wrote: > On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote: >> Hello Thierry, >> >> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote: >>> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: >>>> Hello Thierry, >>>> >>>> (If you have a déjà vu: Yes, I tried to argue this case already in the >>>> past, it's just that I'm just faced with a new use case that's broken >>>> with the current state.) >>>> >>>> Currently the idiom >>>> >>>> pwm_config(mypwm, value, period); >>>> if (!value) >>>> pwm_disable(mypwm); >>>> else >>>> pwm_enable(mypwm); >>>> >>>> (and it's equivalent: >>>> >>>> state.duty_cycle = ... >>>> state.enable = state.duty_cycle ? true : false; >>>> pwm_apply_state(...); >>>> >>>> ) is quite present in the kernel. >>>> >>>> In my eyes this is bad for two reasons: >>>> >>>> a) if the caller is supposed to call pwm_disable() after configuring the >>>> duty cycle to zero, then why doesn't pwm_config() contains this to >>>> unburden pwm-API users and so reduce open coding this assumption? >>> >>> Just to reiterate my thoughts on this: while the config/enable/disable >>> split may seem arbitrary for some use-cases (you may argue most >>> use-cases, and you'd probably be right), I think there's value in having >>> the additional flexibility to explicitly turn off the PWM. Also, duty >>> cycle of zero doesn't always mean "off". If your PWM is inverted, then >>> the duty cycle of zero actually means "on". And that of course also >>> still depends on other components on your board. >> >> Did you notice you didn't argue against me here? We seem to agree that >> calling disabling a PWM after setting the duty cycle to zero is bad. > > I don't see why you think I agree with that. The only thing I'm saying > is that pwm_config() shouldn't assume that a duty-cycle of 0 means the > same as the PWM being disabled. > >>> You may have an inverter somewhere, or you may actually be driving a >>> circuit that expects an inverted PWM. >>> >>> So saying that duty-cycle of 0 equals disable is simplifying things too >>> much, in my opinion. >> >> Full ack. So let's fix the users to not call pwm_disable after setting >> the duty cycle to zero! > > I don't understand why you come to that conclusion. It seems like we at > least agree on what the problem is and what potential issues there could > be with different setups. > > The point that we don't agree with is how to fix this. > >> Really, only the driver knows when it's safe to disable the clock while >> it's expected to keep the pin at a given state. So better don't expect a >> certain state after pwm_disable(). > > Yes, I agree that only the driver knows when it is safe to do so, but I > still fail to understand why we should change the API in order to > accomodate the specifics of a particular driver. > > So we already know that currently all users expect that the pin state > after disable will be low (because they previously set duty cycle to 0, > which is the active equivalent of low). Given that stricter definition, > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin > state. I think the fix for that is for the i.MX PWM driver to make sure > the pin state doesn't actually change on ->disable(). If that means the > clock needs to remain on, then that's exactly what the driver should be > implementing. > >>>> b) it is actually wrong in at least two cases: >>>> - On i.MX28 pwm_config(mypwm, 0, period) only programs a register >>>> and only after the current period is elapsed this is clocked into >>>> the hardware. So it is very likely that when pwm_disable() is >>>> called the period is still not over, and then the clk is disabled >>>> and the period never ends resulting in the pwm pin being frozen in >>>> whatever state it happend to be when the clk was stopped. >>>> - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period) >>>> correctly ensures the PWM pin to stay at 1. But then pwm_disable() >>>> puts it to 0. In the current case this implies that the backlight >>>> is fully enabled instead of off. >>> >>> I think I've explained this in the past: these are i.MX specific issues >>> that you need to work around in the driver. >> >> I'd say: The i.MX PWM doesn't match the idealised idea of a PWM that can >> (in a sane way) be mapped by the current PWM API. That's because there >> are assumptions in the API that are wrong for i.MX. Still the hardware >> allows to implement all use cases. That's a sign that the API is bad and >> is in need for generalisation. > > It's perfectly fine for the driver to make any of the PWM operations a > no-op if that's what it takes to make it conform with the API. > >>> It's also mostly orthogonal >>> to the API issue because consider the case where you want to unload the >>> PWM driver on your board. Your remove function would need disable all >>> PWM channels in order to properly clean up the device, but if you do >>> that, then your backlight would turn fully on, right? >> >> Right. I cannot fix all hardware problems in software. But with my >> suggested change I can at least make it as good as possible in software >> without having to resort to ugly hacks. >> >>> On that note, I'm not sure I understand how this would even work, since >>> you should have the very same state on boot. I'm assuming here that the >>> PWM will be off at boot time by default, in which case, given that it's >>> disabled, it should cause the backlight to turn on. How do you solve >>> that problem? Or if it's not a problem at boot, why does it become a >>> problem on PWM disable or driver removal? >> >> Most of the time it's not a problem during boot because the pin is muxed >> as input. But when the clock of the i.MX PWM is disabled it actively >> drives a zero. And I already had cases where there really was a problem >> during boot until the bootloader fixed the stuff in software. For these >> cases I need patches ("hacks") that remove calls to pwm_disable in the >> backlight driver to at least work around the issue during runtime, which >> is the best that can be done in software. > > Are you aware of this: > > http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987 > > This seems to address exactly the problem that you're describing. The > solution matches what I had expected a fix for this problem to look > like. Can you try it and see if that fixes the issue? > > It looks as if that also fixes the shutdown problem, so it's better than > what you're proposing here yet it doesn't require any API change at all > to make it work. > > Cc'ing Michal for visibility From my very brief skim of your previous discussion it looks like the exact same problem. So I think it should hopefully solve Uwe's issue. I would also like to bring attention to an other series I submitted recently: http://patchwork.ozlabs.org/project/linux-pwm/list/?series=68445 It implements the get_state() function that is call from PWM core. It allows to read the actual state of the PWM HW at probe time. So if you enable PWM in bootloader (I mean you configure it to produce a PWM signal) and you also use it in Linux (just enabling the pwm chip in DT is enough) you will end up with running PWM in userspace. And you can read the actual period/duty cycle form sysfs. And vice versa. If the PWM chip is disabled when Linux is booting, you will end up with disabled PWM in userspace. In combination with the RFC series this should produce undistorted PWM signal from power-up to userspace for normal and inverted PWMs for both cases: if you want it enabled from bootloader to userspace or if you want it disabled from bootloader to userspace. >>> What you're currently proposing is unjustified at this point: you're >>> trying to work around an issue specific to i.MX by changing an API that >>> has worked fine for everyone else for a decade. >> >> I don't agree here. The PWM API as it is now would make it necessary >> that the imx driver becomes ugly. > > Beauty is very subjective. This doesn't count as an argument. Michal's > proposed solution looks perfectly fine. Good to hear that, thanks ;) >> By changing the semantic of the API >> >> - the i.MX driver can be implemented nicely; >> - the other drivers for "saner" hardware can stay as they are; >> - the API users can still express all their needs; >> - the API stops having two ways to express the same needs; > > Again, duty-cycle = 0 is not the same as setting the state to disabled. > Practically they usually have the same effect, but that's just because > in the typical use-case we don't care about the subtle differences. > >> - most API users can be simplified because they can drop >> if (duty == 0) pwm_disable() >> - on i.MX the clock can be disabled when not needed saving some energy; > > That's one reason why we have pwm_disable(). It's what you use to save > some energy if you don't need the PWM. Also, you claim that if the clock > is disabled the attached backlight will turn on at full brightness, so I > fail to see how this would work. > > Also, you leave out the fact that even after all this work that touches > all users and all PWM drivers you still don't get fully correct > functionality on your devices because when the driver is removed or shut > down you run into the same problem. > >> Regarding you claim that the API worked fine for a decade: I tried >> already in 2013 to address this problem. And even independent of that, >> that is not a good reason to not improve stuff. > > My claim was that it has worked fine "for everyone else". I know that > this has been an issue on i.MX for a long time and I've done my best to > lay out why I think your proposal is not the right way to fix it. The > proposal from Michal shows that there is another way that properly fixes > the issue, so let's focus on getting that to work for your use-cases. > > Thierry >
Hello Thierry, On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote: > On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote: > > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote: > > > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: > > > > Hello Thierry, > > > > > > > > (If you have a déjà vu: Yes, I tried to argue this case already in the > > > > past, it's just that I'm just faced with a new use case that's broken > > > > with the current state.) > > > > > > > > Currently the idiom > > > > > > > > pwm_config(mypwm, value, period); > > > > if (!value) > > > > pwm_disable(mypwm); > > > > else > > > > pwm_enable(mypwm); > > > > > > > > (and it's equivalent: > > > > > > > > state.duty_cycle = ... > > > > state.enable = state.duty_cycle ? true : false; > > > > pwm_apply_state(...); > > > > > > > > ) is quite present in the kernel. > > > > > > > > In my eyes this is bad for two reasons: > > > > > > > > a) if the caller is supposed to call pwm_disable() after configuring the > > > > duty cycle to zero, then why doesn't pwm_config() contains this to > > > > unburden pwm-API users and so reduce open coding this assumption? > > > > > > Just to reiterate my thoughts on this: while the config/enable/disable > > > split may seem arbitrary for some use-cases (you may argue most > > > use-cases, and you'd probably be right), I think there's value in having > > > the additional flexibility to explicitly turn off the PWM. Also, duty > > > cycle of zero doesn't always mean "off". If your PWM is inverted, then > > > the duty cycle of zero actually means "on". And that of course also > > > still depends on other components on your board. > > > > Did you notice you didn't argue against me here? We seem to agree that > > calling disabling a PWM after setting the duty cycle to zero is bad. You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but also don't seem to refuse to change pwm users that do: pwm_config(pwm, 0, 100); pwm_disable(pwm); I interpreted your sentence as confirmation that this sequence isn't correct in general. Where is the difference you care about between duty=0 and disabled? > I don't see why you think I agree with that. The only thing I'm saying > is that pwm_config() shouldn't assume that a duty-cycle of 0 means the > same as the PWM being disabled. What is in your view the current difference for the hardware behind the PWM? > > > You may have an inverter somewhere, or you may actually be driving a > > > circuit that expects an inverted PWM. > > > > > > So saying that duty-cycle of 0 equals disable is simplifying things too > > > much, in my opinion. > > > > Full ack. So let's fix the users to not call pwm_disable after setting > > the duty cycle to zero! > > I don't understand why you come to that conclusion. It seems like we at > least agree on what the problem is and what potential issues there could > be with different setups. > > The point that we don't agree with is how to fix this. ack. > > Really, only the driver knows when it's safe to disable the clock while > > it's expected to keep the pin at a given state. So better don't expect a > > certain state after pwm_disable(). > > Yes, I agree that only the driver knows when it is safe to do so, but I > still fail to understand why we should change the API in order to > accomodate the specifics of a particular driver. The incentive to change the API is a combination of: - The restriction to keep the pin driving at a certain level after disable isn't a natural and obvious property of the hardware. The designers of the imx-pwm for example did it differently. - The change removes ambiguity from from the demands on the hardware drivers. - The change doesn't make the API less expressive. - The change doesn't doesn't complicate anmy pwm hardware driver. - The change allows to simplify most pwm users. - The change simplifies the imx-pwm driver. In my experience this is enough to justify such a change. > So we already know that currently all users expect that the pin state > after disable will be low (because they previously set duty cycle to 0, > which is the active equivalent of low). My theory is that most users don't expect this and/or don't make use of it. As of 9dcd936c5312 we have 50 pwm drivers ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l), and I think there are a few false positive matches, pwm-sun4i.c for example). I'd be willing to bet that as of now imx isn't the only driver with this problem. > Given that stricter definition, > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin > state. I think the fix for that is for the i.MX PWM driver to make sure > the pin state doesn't actually change on ->disable(). If that means the > clock needs to remain on, then that's exactly what the driver should be > implementing. Yes, if we keep the API "stricter" this is what should be done. This doesn't justify to keep the API strict though. > Are you aware of this: > > http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987 > > This seems to address exactly the problem that you're describing. The > solution matches what I had expected a fix for this problem to look > like. Can you try it and see if that fixes the issue? Right. Apart from bugs in it (for example http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio as output) this would solve the issue for sure. This doesn't void my arguments to relax the PWM API though. Also look at the costs: The patch adds 86 lines to the imx driver complicating it considerably. Each machine that wants to profit of this change has to adapt it's device tree to make the additional pinmuxing and gpio known. To be fair, my change doesn't fix the gap between pinctrl being active and the actual enablement of the pwm. But this could be better solved in the pwm framework without touching any hardware driver with pinctrl mechanism that are already in place. (Make use of the "init" pinctrl state and only switch to "default" when the pwm is configured.) There is nothing imx-pwm specific in hooking a gpio and adapting the pinmuxing in it. So if you want to go this path, please implement it in a generic way such that all pwm devices could benefit. > It looks as if that also fixes the shutdown problem, so it's better than > what you're proposing here yet it doesn't require any API change at all > to make it work. I don't care much about the shutdown problem. (Also, if the imx-pwm driver is unbound, the gpio is freed, too, so it's only by chance if the level is kept.) > > > What you're currently proposing is unjustified at this point: you're > > > trying to work around an issue specific to i.MX by changing an API that > > > has worked fine for everyone else for a decade. > > > > I don't agree here. The PWM API as it is now would make it necessary > > that the imx driver becomes ugly. > > Beauty is very subjective. This doesn't count as an argument. Michal's > proposed solution looks perfectly fine. In this case the opposite of ugly is "simple". This is a real argument. And this is a reason to apply my idea. It simplifies stuff and solves some problems. A framework is good if it is general enough to cover all use cases for its users and demands very little from it's implementors. Each callback to implement by a hardware driver should be as simple as possible. Implying "keep the pin level constant" in .disable makes it more complicated for some drivers. So this is a bad restriction that should better be dropped if there are no relvant downsides. Sidenote: With the current capabilities of the pwm framework there is no technical reason to expose to the hardware drivers that the pwm user uses inverted logic. The framework could just apply duty = period - duty; before calling the hardware specific .config callback and so allow to simplify some drivers, reducing complexity to a single place. > > By changing the semantic of the API > > > > - the i.MX driver can be implemented nicely; > > - the other drivers for "saner" hardware can stay as they are; > > - the API users can still express all their needs; > > - the API stops having two ways to express the same needs; > > Again, duty-cycle = 0 is not the same as setting the state to disabled. > > Practically they usually have the same effect, but that's just because > in the typical use-case we don't care about the subtle differences. I fail to see the subtle difference. > > - most API users can be simplified because they can drop > > if (duty == 0) pwm_disable() > > - on i.MX the clock can be disabled when not needed saving some energy; > > That's one reason why we have pwm_disable(). It's what you use to save > some energy if you don't need the PWM. Also, you claim that if the clock > is disabled the attached backlight will turn on at full brightness, so I > fail to see how this would work. As long as you care about the backlight to be off, don't disable the pwm. And if in this state the clk can be disabled, then this should be done without an additional poke by the hardware driver which is the only place that can know for sure that it is safe to do so. There is no good reason to let the pwm user have to know that the currently configured state might make some energy saving possible and impose the burden on him to then call pwm_disable(). > Also, you leave out the fact that even after all this work that touches > all users and all PWM drivers you still don't get fully correct > functionality on your devices because when the driver is removed or shut > down you run into the same problem. It's ok for me that all bets are off when the driver is removed. I'm happy when I fix the problem during active operation because on the machine I currently care about the shutdown problem isn't relevant. (Also as noted above, even Michal's patch doesn't solve the problem formally.) > > Regarding you claim that the API worked fine for a decade: I tried > > already in 2013 to address this problem. And even independent of that, > > that is not a good reason to not improve stuff. > > My claim was that it has worked fine "for everyone else". I know that > this has been an issue on i.MX for a long time and I've done my best to > lay out why I think your proposal is not the right way to fix it. I still fail to see a single technical reason to not evolve the API. I read from your mails: - it has been like that for ages - it works for everyone but i.MX - it could be made working for i.MX by involving pinctrl and gpio in the hardware specific pwm driver. Did I miss anything? None of these justifies to keep the status quo in my eyes. Best regards Uwe
On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote: > > On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote: > > > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote: > > > > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: > > > > > Hello Thierry, > > > > > > > > > > (If you have a déjà vu: Yes, I tried to argue this case already in the > > > > > past, it's just that I'm just faced with a new use case that's broken > > > > > with the current state.) > > > > > > > > > > Currently the idiom > > > > > > > > > > pwm_config(mypwm, value, period); > > > > > if (!value) > > > > > pwm_disable(mypwm); > > > > > else > > > > > pwm_enable(mypwm); > > > > > > > > > > (and it's equivalent: > > > > > > > > > > state.duty_cycle = ... > > > > > state.enable = state.duty_cycle ? true : false; > > > > > pwm_apply_state(...); > > > > > > > > > > ) is quite present in the kernel. > > > > > > > > > > In my eyes this is bad for two reasons: > > > > > > > > > > a) if the caller is supposed to call pwm_disable() after configuring the > > > > > duty cycle to zero, then why doesn't pwm_config() contains this to > > > > > unburden pwm-API users and so reduce open coding this assumption? > > > > > > > > Just to reiterate my thoughts on this: while the config/enable/disable > > > > split may seem arbitrary for some use-cases (you may argue most > > > > use-cases, and you'd probably be right), I think there's value in having > > > > the additional flexibility to explicitly turn off the PWM. Also, duty > > > > cycle of zero doesn't always mean "off". If your PWM is inverted, then > > > > the duty cycle of zero actually means "on". And that of course also > > > > still depends on other components on your board. > > > > > > Did you notice you didn't argue against me here? We seem to agree that > > > calling disabling a PWM after setting the duty cycle to zero is bad. > > You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but > also don't seem to refuse to change pwm users that do: > > pwm_config(pwm, 0, 100); > pwm_disable(pwm); > > I interpreted your sentence as confirmation that this sequence isn't > correct in general. Yes, it is not correct in general, though it is correct in many cases. > Where is the difference you care about between duty=0 and disabled? This is described in the kerneldoc of the corresponding functions. In the case of pwm_enable() it starts the output toggling, whereas pwm_enable() disables the output toggling. So strictly by going according to the PWM API a driver should only need to call pwm_config() if the duty cycle actually changes. If all you do is disable and enable, then pwm_enable() and pwm_disable() should be good enough. The implicit assumption is that the configuration will persist across pwm_disable()/pwm_enable(). In practice this doesn't matter that much because most users will be setting the duty cycle and enable/disable at the same time. I can easily imagine other use-cases where you would want to make the different. One other difference could be that pwm_enable() and pwm_disable() require a more involved sequence of operations than pwm_config() or vice versa, in which cases you may want to avoid one or the other if possible. In addition, we do expose these details to userspace via sysfs, so not allowing an explicit disable after config(duty=0) would break that ABI. > > I don't see why you think I agree with that. The only thing I'm saying > > is that pwm_config() shouldn't assume that a duty-cycle of 0 means the > > same as the PWM being disabled. > > What is in your view the current difference for the hardware behind the > PWM? It depends on the driver implementation, but the kerneldoc is pretty specific. I many cases I would expect pwm_disable() to allow a bit of extra power to be saved because clocks could be turned off. > > > > You may have an inverter somewhere, or you may actually be driving a > > > > circuit that expects an inverted PWM. > > > > > > > > So saying that duty-cycle of 0 equals disable is simplifying things too > > > > much, in my opinion. > > > > > > Full ack. So let's fix the users to not call pwm_disable after setting > > > the duty cycle to zero! > > > > I don't understand why you come to that conclusion. It seems like we at > > least agree on what the problem is and what potential issues there could > > be with different setups. > > > > The point that we don't agree with is how to fix this. > > ack. > > > > Really, only the driver knows when it's safe to disable the clock while > > > it's expected to keep the pin at a given state. So better don't expect a > > > certain state after pwm_disable(). > > > > Yes, I agree that only the driver knows when it is safe to do so, but I > > still fail to understand why we should change the API in order to > > accomodate the specifics of a particular driver. > > The incentive to change the API is a combination of: > > - The restriction to keep the pin driving at a certain level after > disable isn't a natural and obvious property of the hardware. > The designers of the imx-pwm for example did it differently. I would disagree. A pin that you can't keep at a defined level if it isn't actively driven is a fundamentally useless concept. You said yourself that the hardware design is problematic because it causes the pin to change unexpectedly when you disable the PWM. One could argue that that's a bug in the hardware. However, given that there is a way to avoid the unexpected change, there is a mechanism outside of PWM to deal with this use-case. And that's also perfectly fine. Dealing with pins that aren't actively driven is part of the job of a pinmux controller. > - The change removes ambiguity from from the demands on the hardware > drivers. I don't think there's currently any ambiguity. We may not be stating explicitly that the assumption is for a PWM to be "off" after pwm_disable(), but it's certainly how everyone has interpreted it. Also if there's an ambiguity, then that's what should be addressed. > - The change doesn't make the API less expressive. Yes it does. You remove the distinction between duty cycle == 0 and the PWM being disabled. > - The change doesn't doesn't complicate anmy pwm hardware driver. True. It also doesn't simplify any PWM hardware driver. > - The change allows to simplify most pwm users. Yes, the subset that doesn't care about the difference is marginally simplified. Although if the API was more rigorous about the definition of pwm_disable(), then PWM users would be even more simplified. > - The change simplifies the imx-pwm driver. True. But you leave out that the change doesn't actually fix your problem. If your PWM user goes away, it will still want to disable the PWM because it is no longer in use. However, you don't want that to mean that the backlight goes to full brightness again. If you unbind the backlight driver, the backlight should remain off (or turn off if it isn't already). > In my experience this is enough to justify such a change. I would consider the change if it was actually going to fix the issue that you're seeing. As it is, all your proposal does is paper over one specific symptom, and I'm sorry, but that's just not good enough. > > So we already know that currently all users expect that the pin state > > after disable will be low (because they previously set duty cycle to 0, > > which is the active equivalent of low). > > My theory is that most users don't expect this and/or don't make use of > it. As of 9dcd936c5312 we have 50 pwm drivers > ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion > at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l), > and I think there are a few false positive matches, pwm-sun4i.c for > example). I'd be willing to bet that as of now imx isn't the only driver > with this problem. We can speculate about that as much as we want. i.MX is the only one that this issue has been reported against, so I'm going to have to assume that it's working fine for all of the others. If it isn't then people should come forward and report it. > > Given that stricter definition, > > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin > > state. I think the fix for that is for the i.MX PWM driver to make sure > > the pin state doesn't actually change on ->disable(). If that means the > > clock needs to remain on, then that's exactly what the driver should be > > implementing. > > Yes, if we keep the API "stricter" this is what should be done. This > doesn't justify to keep the API strict though. But an API that isn't strict is useless. If the behaviour of a PWM isn't predictable, how can you use it to drive a backlight. If at any point it could just go 100% and turn on the backlight to full brightness, how can anyone use it for anything sensible? > > Are you aware of this: > > > > http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987 > > > > This seems to address exactly the problem that you're describing. The > > solution matches what I had expected a fix for this problem to look > > like. Can you try it and see if that fixes the issue? > > Right. Apart from bugs in it (for example > http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio > as output) this would solve the issue for sure. This doesn't void my > arguments to relax the PWM API though. Also look at the costs: The patch > adds 86 lines to the imx driver complicating it considerably. Now you're just being dramatic. 86 lines of code is not a lot. And I don't think it's complicated. I'm willing to carry the burden of those 86 lines of code if it fixes the issue for good. > Each > machine that wants to profit of this change has to adapt it's device > tree to make the additional pinmuxing and gpio known. > To be fair, my change doesn't fix the gap between pinctrl being active > and the actual enablement of the pwm. But this could be better solved in > the pwm framework without touching any hardware driver with pinctrl > mechanism that are already in place. (Make use of the "init" pinctrl > state and only switch to "default" when the pwm is configured.) > There is nothing imx-pwm specific in hooking a gpio and adapting the > pinmuxing in it. So if you want to go this path, please implement it in > a generic way such that all pwm devices could benefit. You're wrong. Judging by all the evidence that we have at this point, this is exactly an imx-pwm specific issue, therefore the burden is on the imx-pwm driver to implement a solution for it. If there ever is a second driver that has this issue we can revisit moving this into the core. I don't see any reason to do so at this point in time with the information that we have. > > It looks as if that also fixes the shutdown problem, so it's better than > > what you're proposing here yet it doesn't require any API change at all > > to make it work. > > I don't care much about the shutdown problem. (Also, if the imx-pwm > driver is unbound, the gpio is freed, too, so it's only by chance if the > level is kept.) Who else do you think would use the GPIO and change the level? If you've got multiple concurrent users of the pin that's a whole other problem. I also don't see why you wouldn't want to care about the shutdown problem. It's annoying if you shut down the device and all of a sudden your backlight comes back up again, perhaps in the middle of the display controller shutting down and producing garbage on the screen. > > > > > What you're currently proposing is unjustified at this point: you're > > > > trying to work around an issue specific to i.MX by changing an API that > > > > has worked fine for everyone else for a decade. > > > > > > I don't agree here. The PWM API as it is now would make it necessary > > > that the imx driver becomes ugly. > > > > Beauty is very subjective. This doesn't count as an argument. Michal's > > proposed solution looks perfectly fine. > > In this case the opposite of ugly is "simple". This is a real argument. > And this is a reason to apply my idea. It simplifies stuff and solves > some problems. > > A framework is good if it is general enough to cover all use cases for > its users and demands very little from it's implementors. Each callback > to implement by a hardware driver should be as simple as possible. > Implying "keep the pin level constant" in .disable makes it more > complicated for some drivers. So this is a bad restriction that should > better be dropped if there are no relvant downsides. Okay, so I'm not suggesting that pwm_disable() keep the pin level constant. What I was proposing is that it be required to set the pin level to "off" (whatever that means). And the reason for doing that is because there are relevant downsides if we leave this undefined, as I described at length above. > Sidenote: With the current capabilities of the pwm framework there is no > technical reason to expose to the hardware drivers that the pwm user uses > inverted logic. The framework could just apply > > duty = period - duty; > > before calling the hardware specific .config callback and so allow to > simplify some drivers, reducing complexity to a single place. No, you're wrong. If you only consider the case of a backlight or an LED, then yes, you could simplify it that way. However there are other use-cases that require more fine-grained control and in those cases the actual edges of the PWM are important. Also, this has been discussed before and I have said elsewhere that I have no objection to adding a helper that would simplify this for consumers that don't care about the difference. > > > By changing the semantic of the API > > > > > > - the i.MX driver can be implemented nicely; > > > - the other drivers for "saner" hardware can stay as they are; > > > - the API users can still express all their needs; > > > - the API stops having two ways to express the same needs; > > > > Again, duty-cycle = 0 is not the same as setting the state to disabled. > > > > Practically they usually have the same effect, but that's just because > > in the typical use-case we don't care about the subtle differences. > > I fail to see the subtle difference. That's okay. I've tried my best to describe it to you in different ways. If I still haven't been able to convey this, I don't know what else to say. > > > - most API users can be simplified because they can drop > > > if (duty == 0) pwm_disable() > > > - on i.MX the clock can be disabled when not needed saving some energy; > > > > That's one reason why we have pwm_disable(). It's what you use to save > > some energy if you don't need the PWM. Also, you claim that if the clock > > is disabled the attached backlight will turn on at full brightness, so I > > fail to see how this would work. > > As long as you care about the backlight to be off, don't disable the > pwm. And if in this state the clk can be disabled, then this should be > done without an additional poke by the hardware driver which is the only > place that can know for sure that it is safe to do so. There is no good > reason to let the pwm user have to know that the currently configured > state might make some energy saving possible and impose the burden on > him to then call pwm_disable(). This doesn't have anything to do with consumers knowing about potential energy savings. All we do is give the consumers an opportunity to say: I need the PWM to be enabled because I want to use the PWM signal now or I don't really need the PWM signal any more, just stop sending a signal. This is merely a hint from the consumer. What the driver does with it is ultimately up to the driver. If the driver wants to disable the clock when the duty cycle goes to zero because it knows that it is safe to do so, then by all means, feel free to do that. > > Also, you leave out the fact that even after all this work that touches > > all users and all PWM drivers you still don't get fully correct > > functionality on your devices because when the driver is removed or shut > > down you run into the same problem. > > It's ok for me that all bets are off when the driver is removed. Well, it's not okay for me. Especially if we can do better. > I'm > happy when I fix the problem during active operation because on the > machine I currently care about the shutdown problem isn't relevant. Why is it not relevant? Surely at some point you will want to shut down that machine. And even if not, imx-pwm is used by others and they do seem to care about shutdown, so they should have a say in the matter as well. > (Also as noted above, even Michal's patch doesn't solve the problem > formally.) Now I'm confused, you said above that "Apart from bugs in it ... this would solve the issue for sure". So which one is it? > > > Regarding you claim that the API worked fine for a decade: I tried > > > already in 2013 to address this problem. And even independent of that, > > > that is not a good reason to not improve stuff. > > > > My claim was that it has worked fine "for everyone else". I know that > > this has been an issue on i.MX for a long time and I've done my best to > > lay out why I think your proposal is not the right way to fix it. > > I still fail to see a single technical reason to not evolve the API. I > read from your mails: > > - it has been like that for ages > - it works for everyone but i.MX > - it could be made working for i.MX by involving pinctrl and gpio in > the hardware specific pwm driver. > > Did I miss anything? None of these justifies to keep the status quo in > my eyes. I don't understand your logic here. You want to change the API, so the burden is on you to convince me *why* it needs to be changed. So far you have not been able to do that, so don't try to turn this around and make me come up with reasons why it shouldn't change. Thierry
Uwe, Thierry, just a few comments as I am not able to dive deeper into your discussion. On 11.10.2018 14:00, Thierry Reding wrote: > On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote: >> Hello Thierry, >> >> On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote: >>> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote: >>>> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote: >>>>> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: >>>>>> Hello Thierry, >>>>>> >>>>>> (If you have a déjà vu: Yes, I tried to argue this case already in the >>>>>> past, it's just that I'm just faced with a new use case that's broken >>>>>> with the current state.) >>>>>> >>>>>> Currently the idiom >>>>>> >>>>>> pwm_config(mypwm, value, period); >>>>>> if (!value) >>>>>> pwm_disable(mypwm); >>>>>> else >>>>>> pwm_enable(mypwm); >>>>>> >>>>>> (and it's equivalent: >>>>>> >>>>>> state.duty_cycle = ... >>>>>> state.enable = state.duty_cycle ? true : false; >>>>>> pwm_apply_state(...); >>>>>> >>>>>> ) is quite present in the kernel. >>>>>> >>>>>> In my eyes this is bad for two reasons: >>>>>> >>>>>> a) if the caller is supposed to call pwm_disable() after configuring the >>>>>> duty cycle to zero, then why doesn't pwm_config() contains this to >>>>>> unburden pwm-API users and so reduce open coding this assumption? >>>>> >>>>> Just to reiterate my thoughts on this: while the config/enable/disable >>>>> split may seem arbitrary for some use-cases (you may argue most >>>>> use-cases, and you'd probably be right), I think there's value in having >>>>> the additional flexibility to explicitly turn off the PWM. Also, duty >>>>> cycle of zero doesn't always mean "off". If your PWM is inverted, then >>>>> the duty cycle of zero actually means "on". And that of course also >>>>> still depends on other components on your board. >>>> >>>> Did you notice you didn't argue against me here? We seem to agree that >>>> calling disabling a PWM after setting the duty cycle to zero is bad. >> >> You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but >> also don't seem to refuse to change pwm users that do: >> >> pwm_config(pwm, 0, 100); >> pwm_disable(pwm); >> >> I interpreted your sentence as confirmation that this sequence isn't >> correct in general. > > Yes, it is not correct in general, though it is correct in many cases. > >> Where is the difference you care about between duty=0 and disabled? > > This is described in the kerneldoc of the corresponding functions. In the > case of pwm_enable() it starts the output toggling, whereas pwm_enable() > disables the output toggling. So strictly by going according to the PWM > API a driver should only need to call pwm_config() if the duty cycle > actually changes. If all you do is disable and enable, then pwm_enable() > and pwm_disable() should be good enough. The implicit assumption is that > the configuration will persist across pwm_disable()/pwm_enable(). In > practice this doesn't matter that much because most users will be > setting the duty cycle and enable/disable at the same time. I can easily > imagine other use-cases where you would want to make the different. One > other difference could be that pwm_enable() and pwm_disable() require a > more involved sequence of operations than pwm_config() or vice versa, in > which cases you may want to avoid one or the other if possible. > > In addition, we do expose these details to userspace via sysfs, so not > allowing an explicit disable after config(duty=0) would break that ABI. > >>> I don't see why you think I agree with that. The only thing I'm saying >>> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the >>> same as the PWM being disabled. >> >> What is in your view the current difference for the hardware behind the >> PWM? > > It depends on the driver implementation, but the kerneldoc is pretty > specific. I many cases I would expect pwm_disable() to allow a bit of > extra power to be saved because clocks could be turned off. > >>>>> You may have an inverter somewhere, or you may actually be driving a >>>>> circuit that expects an inverted PWM. >>>>> >>>>> So saying that duty-cycle of 0 equals disable is simplifying things too >>>>> much, in my opinion. >>>> >>>> Full ack. So let's fix the users to not call pwm_disable after setting >>>> the duty cycle to zero! >>> >>> I don't understand why you come to that conclusion. It seems like we at >>> least agree on what the problem is and what potential issues there could >>> be with different setups. >>> >>> The point that we don't agree with is how to fix this. >> >> ack. >> >>>> Really, only the driver knows when it's safe to disable the clock while >>>> it's expected to keep the pin at a given state. So better don't expect a >>>> certain state after pwm_disable(). >>> >>> Yes, I agree that only the driver knows when it is safe to do so, but I >>> still fail to understand why we should change the API in order to >>> accomodate the specifics of a particular driver. >> >> The incentive to change the API is a combination of: >> >> - The restriction to keep the pin driving at a certain level after >> disable isn't a natural and obvious property of the hardware. >> The designers of the imx-pwm for example did it differently. > > I would disagree. A pin that you can't keep at a defined level if it > isn't actively driven is a fundamentally useless concept. You said > yourself that the hardware design is problematic because it causes the > pin to change unexpectedly when you disable the PWM. One could argue > that that's a bug in the hardware. However, given that there is a way to > avoid the unexpected change, there is a mechanism outside of PWM to deal > with this use-case. And that's also perfectly fine. Dealing with pins > that aren't actively driven is part of the job of a pinmux controller. > >> - The change removes ambiguity from from the demands on the hardware >> drivers. > > I don't think there's currently any ambiguity. We may not be stating > explicitly that the assumption is for a PWM to be "off" after > pwm_disable(), but it's certainly how everyone has interpreted it. Also > if there's an ambiguity, then that's what should be addressed. > >> - The change doesn't make the API less expressive. > > Yes it does. You remove the distinction between duty cycle == 0 and > the PWM being disabled. > >> - The change doesn't doesn't complicate anmy pwm hardware driver. > > True. It also doesn't simplify any PWM hardware driver. > >> - The change allows to simplify most pwm users. > > Yes, the subset that doesn't care about the difference is marginally > simplified. Although if the API was more rigorous about the definition > of pwm_disable(), then PWM users would be even more simplified. > >> - The change simplifies the imx-pwm driver. > > True. But you leave out that the change doesn't actually fix your > problem. If your PWM user goes away, it will still want to disable the > PWM because it is no longer in use. However, you don't want that to > mean that the backlight goes to full brightness again. If you unbind > the backlight driver, the backlight should remain off (or turn off if > it isn't already). > >> In my experience this is enough to justify such a change. > > I would consider the change if it was actually going to fix the issue > that you're seeing. As it is, all your proposal does is paper over one > specific symptom, and I'm sorry, but that's just not good enough. > >>> So we already know that currently all users expect that the pin state >>> after disable will be low (because they previously set duty cycle to 0, >>> which is the active equivalent of low). >> >> My theory is that most users don't expect this and/or don't make use of >> it. As of 9dcd936c5312 we have 50 pwm drivers >> ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion >> at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l), >> and I think there are a few false positive matches, pwm-sun4i.c for >> example). I'd be willing to bet that as of now imx isn't the only driver >> with this problem. > > We can speculate about that as much as we want. i.MX is the only one > that this issue has been reported against, so I'm going to have to > assume that it's working fine for all of the others. If it isn't then > people should come forward and report it. > >>> Given that stricter definition, >>> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin >>> state. I think the fix for that is for the i.MX PWM driver to make sure >>> the pin state doesn't actually change on ->disable(). If that means the >>> clock needs to remain on, then that's exactly what the driver should be >>> implementing. >> >> Yes, if we keep the API "stricter" this is what should be done. This >> doesn't justify to keep the API strict though. > > But an API that isn't strict is useless. If the behaviour of a PWM isn't > predictable, how can you use it to drive a backlight. If at any point it > could just go 100% and turn on the backlight to full brightness, how can > anyone use it for anything sensible? > >>> Are you aware of this: >>> >>> http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987 >>> >>> This seems to address exactly the problem that you're describing. The >>> solution matches what I had expected a fix for this problem to look >>> like. Can you try it and see if that fixes the issue? >> >> Right. Apart from bugs in it (for example >> http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio >> as output) It is a RFC and the past discussion about it was mostly theoretical than about technical details. So I thought that nobody would really expect excellent technical solution at this point. I just tried to do my best and tested dozens of variations of [enabled/disabled/inverted/non- inverted/ bootloader/device-tree/sysfs] configurations to be sure how it affects current users and how it improves the situation. Regarding the GPIO output configuration. Yes, the pin is configured as input in the imx_pwm_init_pinctrl_info() function. At this stage you do not really know how to configure the pin. Configuring it as input is the safest option I think. I thought that the pin is configured as output whenever gpiod_set_value() is called. None of the other examples/usages in kernel I looked at used the gpiod_direction_output(). So thanks for pointing that out, I can fix that. >> this would solve the issue for sure. This doesn't void my >> arguments to relax the PWM API though. Also look at the costs: The patch >> adds 86 lines to the imx driver complicating it considerably. > > Now you're just being dramatic. 86 lines of code is not a lot. And I > don't think it's complicated. I'm willing to carry the burden of those > 86 lines of code if it fixes the issue for good. > >> Each >> machine that wants to profit of this change has to adapt it's device >> tree to make the additional pinmuxing and gpio known. >> To be fair, my change doesn't fix the gap between pinctrl being active >> and the actual enablement of the pwm. But this could be better solved in >> the pwm framework without touching any hardware driver with pinctrl >> mechanism that are already in place. (Make use of the "init" pinctrl >> state and only switch to "default" when the pwm is configured.) >> There is nothing imx-pwm specific in hooking a gpio and adapting the >> pinmuxing in it. So if you want to go this path, please implement it in >> a generic way such that all pwm devices could benefit. > > You're wrong. Judging by all the evidence that we have at this point, > this is exactly an imx-pwm specific issue, therefore the burden is on > the imx-pwm driver to implement a solution for it. If there ever is a > second driver that has this issue we can revisit moving this into the > core. I don't see any reason to do so at this point in time with the > information that we have. > >>> It looks as if that also fixes the shutdown problem, so it's better than >>> what you're proposing here yet it doesn't require any API change at all >>> to make it work. >> >> I don't care much about the shutdown problem. (Also, if the imx-pwm >> driver is unbound, the gpio is freed, too, so it's only by chance if the >> level is kept.) > > Who else do you think would use the GPIO and change the level? If you've > got multiple concurrent users of the pin that's a whole other problem. I > also don't see why you wouldn't want to care about the shutdown problem. > It's annoying if you shut down the device and all of a sudden your > backlight comes back up again, perhaps in the middle of the display > controller shutting down and producing garbage on the screen. > >> >>>>> What you're currently proposing is unjustified at this point: you're >>>>> trying to work around an issue specific to i.MX by changing an API that >>>>> has worked fine for everyone else for a decade. >>>> >>>> I don't agree here. The PWM API as it is now would make it necessary >>>> that the imx driver becomes ugly. >>> >>> Beauty is very subjective. This doesn't count as an argument. Michal's >>> proposed solution looks perfectly fine. >> >> In this case the opposite of ugly is "simple". This is a real argument. >> And this is a reason to apply my idea. It simplifies stuff and solves >> some problems. >> >> A framework is good if it is general enough to cover all use cases for >> its users and demands very little from it's implementors. Each callback >> to implement by a hardware driver should be as simple as possible. >> Implying "keep the pin level constant" in .disable makes it more >> complicated for some drivers. So this is a bad restriction that should >> better be dropped if there are no relvant downsides. > > Okay, so I'm not suggesting that pwm_disable() keep the pin level > constant. What I was proposing is that it be required to set the pin > level to "off" (whatever that means). And the reason for doing that is > because there are relevant downsides if we leave this undefined, as I > described at length above. > >> Sidenote: With the current capabilities of the pwm framework there is no >> technical reason to expose to the hardware drivers that the pwm user uses >> inverted logic. The framework could just apply >> >> duty = period - duty; >> I prefer to utilize as much HW capabilities as possible. And it seems like most PWM chips support HW output inversion (to some extent) so to me it seems perfectly OK that that information can be propagated from the upper SW levels to the lowest one. >> before calling the hardware specific .config callback and so allow to >> simplify some drivers, reducing complexity to a single place. > > No, you're wrong. If you only consider the case of a backlight or an > LED, then yes, you could simplify it that way. However there are other > use-cases that require more fine-grained control and in those cases the > actual edges of the PWM are important. > > Also, this has been discussed before and I have said elsewhere that I > have no objection to adding a helper that would simplify this for > consumers that don't care about the difference. > >>>> By changing the semantic of the API >>>> >>>> - the i.MX driver can be implemented nicely; >>>> - the other drivers for "saner" hardware can stay as they are; >>>> - the API users can still express all their needs; >>>> - the API stops having two ways to express the same needs; >>> >>> Again, duty-cycle = 0 is not the same as setting the state to disabled. >>> >>> Practically they usually have the same effect, but that's just because >>> in the typical use-case we don't care about the subtle differences. >> >> I fail to see the subtle difference. > > That's okay. I've tried my best to describe it to you in different ways. > If I still haven't been able to convey this, I don't know what else to > say. > >>>> - most API users can be simplified because they can drop >>>> if (duty == 0) pwm_disable() >>>> - on i.MX the clock can be disabled when not needed saving some energy; >>> >>> That's one reason why we have pwm_disable(). It's what you use to save >>> some energy if you don't need the PWM. Also, you claim that if the clock >>> is disabled the attached backlight will turn on at full brightness, so I >>> fail to see how this would work. >> >> As long as you care about the backlight to be off, don't disable the >> pwm. And if in this state the clk can be disabled, then this should be >> done without an additional poke by the hardware driver which is the only >> place that can know for sure that it is safe to do so. There is no good >> reason to let the pwm user have to know that the currently configured >> state might make some energy saving possible and impose the burden on >> him to then call pwm_disable(). > > This doesn't have anything to do with consumers knowing about potential > energy savings. All we do is give the consumers an opportunity to say: I > need the PWM to be enabled because I want to use the PWM signal now or I > don't really need the PWM signal any more, just stop sending a signal. > This is merely a hint from the consumer. What the driver does with it is > ultimately up to the driver. If the driver wants to disable the clock > when the duty cycle goes to zero because it knows that it is safe to do > so, then by all means, feel free to do that. > >>> Also, you leave out the fact that even after all this work that touches >>> all users and all PWM drivers you still don't get fully correct >>> functionality on your devices because when the driver is removed or shut >>> down you run into the same problem. >> >> It's ok for me that all bets are off when the driver is removed. > > Well, it's not okay for me. Especially if we can do better. > >> I'm >> happy when I fix the problem during active operation because on the >> machine I currently care about the shutdown problem isn't relevant. > > Why is it not relevant? Surely at some point you will want to shut down > that machine. And even if not, imx-pwm is used by others and they do > seem to care about shutdown, so they should have a say in the matter as > well. > >> (Also as noted above, even Michal's patch doesn't solve the problem >> formally.) > > Now I'm confused, you said above that "Apart from bugs in it ... this > would solve the issue for sure". So which one is it? > >>>> Regarding you claim that the API worked fine for a decade: I tried >>>> already in 2013 to address this problem. And even independent of that, >>>> that is not a good reason to not improve stuff. >>> >>> My claim was that it has worked fine "for everyone else". I know that >>> this has been an issue on i.MX for a long time and I've done my best to >>> lay out why I think your proposal is not the right way to fix it. >> >> I still fail to see a single technical reason to not evolve the API. I >> read from your mails: >> >> - it has been like that for ages >> - it works for everyone but i.MX >> - it could be made working for i.MX by involving pinctrl and gpio in >> the hardware specific pwm driver. >> >> Did I miss anything? None of these justifies to keep the status quo in >> my eyes. > > I don't understand your logic here. You want to change the API, so the > burden is on you to convince me *why* it needs to be changed. So far you > have not been able to do that, so don't try to turn this around and make > me come up with reasons why it shouldn't change. > > Thierry >
Hello Thierry, On Thu, Oct 11, 2018 at 02:00:07PM +0200, Thierry Reding wrote: > On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote: > > On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote: > > > On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote: > > > > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote: > > > > > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: > > You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but > > also don't seem to refuse to change pwm users that do: Just noticed the above has a "n't" to much. I don't think you misunderstood my point because of that though. > > pwm_config(pwm, 0, 100); > > pwm_disable(pwm); > > > > I interpreted your sentence as confirmation that this sequence isn't > > correct in general. > > Yes, it is not correct in general, though it is correct in many cases. Can you point out where it would not be correct? > > Where is the difference you care about between duty=0 and disabled? > > This is described in the kerneldoc of the corresponding functions. In the > case of pwm_enable() it starts the output toggling, whereas pwm_enable() s/pwm_enable()$/pwm_disable/ I assume. > disables the output toggling. So strictly by going according to the PWM > API a driver should only need to call pwm_config() if the duty cycle > actually changes. If all you do is disable and enable, then pwm_enable() > and pwm_disable() should be good enough. Not sure we're looking on the same kerneldoc. pwm_disable only specifies: pwm_disable() - stop a PWM output toggling . If you call pwm_disable() for an i.MX device it doesn't toggle any more. OK, after configuring the pwm as inverted and 0% duty (or not inverted and 100% duty) you get an additional edge, but I do think it's a bit bold to deduce that the pin should keep it's state from the above specification. > The implicit assumption is that the configuration will persist across > pwm_disable()/pwm_enable(). In practice this doesn't matter that much > because most users will be setting the duty cycle and enable/disable > at the same time. I can easily imagine other use-cases where you would > want to make the different. One other difference could be that > pwm_enable() and pwm_disable() require a more involved sequence of > operations than pwm_config() or vice versa, in which cases you may > want to avoid one or the other if possible. OK, A PWM user that has to switch between say 50% duty cycle and a constant value can use pwm_enable() and pwm_disable() to change between these states (after calling pwm_config(pwm, period / 2, period) once). Similarly it can use pwm_config(pwm, 0, period); and pwm_config(pwm, period / 2, period); to toggle. I confirm, with my suggestion the first alternative would go away. This isn't a big loss though because the second stays around. Now as you refuse to remove this first alternative there should be a relevant semantic difference between the two. There are only a single difference I see: With the first way (using pwm_enable() and pwm_disable()) the PWM user doesn't control if the PWM stops at 0 or 1, using the 2nd approach the PWM has to stop at 0. Currently power consumption might be another difference, but you stated below this wasn't relevant. > In addition, we do expose these details to userspace via sysfs, so not > allowing an explicit disable after config(duty=0) would break that ABI. I wouldn't have a big problem with that even if the "strict" expectation would be documented explicitly. The documentation for /sys/class/pwm/pwmchipN/pwmX/enable only states: Enable/disable the PWM signal. which I wouldn't consider to break with my suggestion. > > What is in your view the current difference for the hardware behind the > > PWM? > > It depends on the driver implementation, but the kerneldoc is pretty > specific. I many cases I would expect pwm_disable() to allow a bit of > extra power to be saved because clocks could be turned off. With my approach that expectation is given. With your's it is not in every case because of the ambiguity of pwm_disable. On i.MX the clock can only be disabled if the caller doesn't care about the resulting pin state. The hardware driver however cannot know if this is the case and so must keep the clock on even in the cases where it would be ok to disable it. > > > > Really, only the driver knows when it's safe to disable the clock while > > > > it's expected to keep the pin at a given state. So better don't expect a > > > > certain state after pwm_disable(). > > > > > > Yes, I agree that only the driver knows when it is safe to do so, but I > > > still fail to understand why we should change the API in order to > > > accomodate the specifics of a particular driver. > > > > The incentive to change the API is a combination of: > > > > - The restriction to keep the pin driving at a certain level after > > disable isn't a natural and obvious property of the hardware. > > The designers of the imx-pwm for example did it differently. > > I would disagree. A pin that you can't keep at a defined level if it > isn't actively driven is a fundamentally useless concept. You said > yourself that the hardware design is problematic because it causes the > pin to change unexpectedly when you disable the PWM. It's only unexpected if you are used to the strict API design. And if you don't agree to this point, I'm missing ambition on your side. If the strict API is only suitable for "sane" pwm hardware instead of being general enough to also cover some of the "stranger" designs, that's what I call useless. > One could argue that that's a bug in the hardware. And one could argue further that it's a shortcoming of the PWM framework that it is unable to handle it better, though it easily could. > However, given that there is a way to avoid the unexpected change, > there is a mechanism outside of PWM to deal with this use-case. And > that's also perfectly fine. Dealing with pins that aren't actively > driven is part of the job of a pinmux controller. > > > - The change removes ambiguity from from the demands on the hardware > > drivers. > > I don't think there's currently any ambiguity. We may not be stating > explicitly that the assumption is for a PWM to be "off" after > pwm_disable(), but it's certainly how everyone has interpreted it. Also > if there's an ambiguity, then that's what should be addressed. "off" isn't a clear definition. For you it is (assuming I understood you right) "stop immediately and keep driving the current level". For the imx hardware designer it is, "stop toggling, output a 0". For others it might be: "Stop driving the pin". The ambiguity is that sometimes the PWM is disabled with the need to keep the output driving at a certain level. And sometimes the PWM is disabled and the output doesn't matter. In both cases pwm_disable() is used and the driver cannot know in which of the two cases we are and if it's ok to release the pin or not. > > - The change doesn't make the API less expressive. > > Yes it does. You remove the distinction between duty cycle == 0 and > the PWM being disabled. Currently there is no distinction that is relevant for PWM users. (If you don't agree, please provide an example.) In both cases the output is constant 0 (assuming a non-inverted PWM). The PWM user can still request that constant 0 with the laxer API. The only difference in the strict API is that pwm_config(pwm, 0, 100) doesn't disable the clock and pwm_disable(pwm) does. There is no good reason however for a driver of a "sane" PWM to not disable the clock in reply to pwm_config(pwm, 0, 100) though. If you don't agree, can you show me a single PWM user that cares about the distinction? > > - The change doesn't doesn't complicate any pwm hardware driver. > > True. It also doesn't simplify any PWM hardware driver. see "The change simplifies the imx-pwm driver." below that you confirmed. > > - The change allows to simplify most pwm users. > > Yes, the subset that doesn't care about the difference is marginally > simplified. Although if the API was more rigorous about the definition > of pwm_disable(), then PWM users would be even more simplified. I cannot follow. How should/could the API be more rigorous to simplify PWM users? Is this off-topic for the current discussion? > > - The change simplifies the imx-pwm driver. > > True. But you leave out that the change doesn't actually fix your > problem. If your PWM user goes away, it will still want to disable the > PWM because it is no longer in use. However, you don't want that to > mean that the backlight goes to full brightness again. If you unbind > the backlight driver, the backlight should remain off (or turn off if > it isn't already). This advantage isn't about "solving my problems". It is about simplification (as I wrote). > > In my experience this is enough to justify such a change. > > I would consider the change if it was actually going to fix the issue > that you're seeing. As it is, all your proposal does is paper over one > specific symptom, and I'm sorry, but that's just not good enough. Correct, it doesn't completely fix the issue. It improves the situation considerably though and has the advantages that you confirmed above. > > > So we already know that currently all users expect that the pin state > > > after disable will be low (because they previously set duty cycle to 0, > > > which is the active equivalent of low). > > > > My theory is that most users don't expect this and/or don't make use of > > it. As of 9dcd936c5312 we have 50 pwm drivers > > ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion > > at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l), > > and I think there are a few false positive matches, pwm-sun4i.c for > > example). I'd be willing to bet that as of now imx isn't the only driver > > with this problem. > > We can speculate about that as much as we want. i.MX is the only one > that this issue has been reported against, so I'm going to have to > assume that it's working fine for all of the others. If it isn't then > people should come forward and report it. You really assume that all the drivers that don't make use of PWM_POLARITY_INVERSED (33 of 50 + imx at least, probably more) behave as you expect when the PWM user does: pwm_apply_state(pwm, { .period = 100; .duty_cycle=0; polarity=PWM_POLARITY_INVERSED; enabled=false; }) ? I'd say your assumption that all are working fine is treading on thin ice. The effect maybe is only that the i.MX users use more corners of the PWM API and/or are more open to report actual problems. Also if there are two ways to implement something, one works for 49 different hardware implementations and one for 50 this is already a good reason to stick to the latter. > > > Given that stricter definition, > > > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin > > > state. I think the fix for that is for the i.MX PWM driver to make sure > > > the pin state doesn't actually change on ->disable(). If that means the > > > clock needs to remain on, then that's exactly what the driver should be > > > implementing. > > > > Yes, if we keep the API "stricter" this is what should be done. This > > doesn't justify to keep the API strict though. > > But an API that isn't strict is useless. If the behaviour of a PWM isn't > predictable, how can you use it to drive a backlight. If at any point it > could just go 100% and turn on the backlight to full brightness, how can > anyone use it for anything sensible? As long as I don't call pwm_disable() (or pwm_config(pwm, period, period)) it must not go to 100%. > > > Are you aware of this: > > > > > > http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987 > > > > > > This seems to address exactly the problem that you're describing. The > > > solution matches what I had expected a fix for this problem to look > > > like. Can you try it and see if that fixes the issue? > > > > Right. Apart from bugs in it (for example > > http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio > > as output) this would solve the issue for sure. This doesn't void my > > arguments to relax the PWM API though. Also look at the costs: The patch > > adds 86 lines to the imx driver complicating it considerably. > > Now you're just being dramatic. 86 lines of code is not a lot. And I > don't think it's complicated. I'm willing to carry the burden of those > 86 lines of code if it fixes the issue for good. Right, the complexity doesn't come with the 86 lines of code. With my approach the pwm-imx driver only has to care about its 6 registers. With Michal's patch it suddenly has to care about gpio and pinmuxing. > > Each > > machine that wants to profit of this change has to adapt it's device > > tree to make the additional pinmuxing and gpio known. > > To be fair, my change doesn't fix the gap between pinctrl being active > > and the actual enablement of the pwm. But this could be better solved in > > the pwm framework without touching any hardware driver with pinctrl > > mechanism that are already in place. (Make use of the "init" pinctrl > > state and only switch to "default" when the pwm is configured.) > > There is nothing imx-pwm specific in hooking a gpio and adapting the > > pinmuxing in it. So if you want to go this path, please implement it in > > a generic way such that all pwm devices could benefit. > > You're wrong. Judging by all the evidence that we have at this point, > this is exactly an imx-pwm specific issue, therefore the burden is on > the imx-pwm driver to implement a solution for it. You're the first maintainer I met with this attitude. Whenever I implemented something in a driver specific way fixing an issue that other drivers might have to, I was requested to find a generic solution without the need to analyse if other drivers are affected or not. There are some subsystems (e.g. tty/serial) where there is much stuff implemented in each driver even though some things could well be implemented in a generic way (e.g. rs485 handling). This is really a big mess. > > > It looks as if that also fixes the shutdown problem, so it's better than > > > what you're proposing here yet it doesn't require any API change at all > > > to make it work. > > > > I don't care much about the shutdown problem. (Also, if the imx-pwm > > driver is unbound, the gpio is freed, too, so it's only by chance if the > > level is kept.) > > Who else do you think would use the GPIO and change the level? If you've > got multiple concurrent users of the pin that's a whole other problem. I > also don't see why you wouldn't want to care about the shutdown problem. > It's annoying if you shut down the device and all of a sudden your > backlight comes back up again, perhaps in the middle of the display > controller shutting down and producing garbage on the screen. If someone on my machine is able to unbind the backlight driver making the display flicker I have much more and graver problems than the flickering display. And I didn't test (as I currently don't have an affected device on my desk) but I think machine shutdown is no problem. > > Sidenote: With the current capabilities of the pwm framework there is no > > technical reason to expose to the hardware drivers that the pwm user uses > > inverted logic. The framework could just apply > > > > duty = period - duty; > > > > before calling the hardware specific .config callback and so allow to > > simplify some drivers, reducing complexity to a single place. > > No, you're wrong. If you only consider the case of a backlight or an > LED, then yes, you could simplify it that way. However there are other > use-cases that require more fine-grained control and in those cases the > actual edges of the PWM are important. Can you please point out such a use case that is also possible to implement with the current API? > Also, this has been discussed before and I have said elsewhere that I > have no objection to adding a helper that would simplify this for > consumers that don't care about the difference. I suggest a compromise below, maybe we can agree on that one. > > > > By changing the semantic of the API > > > > > > > > - the i.MX driver can be implemented nicely; > > > > - the other drivers for "saner" hardware can stay as they are; > > > > - the API users can still express all their needs; > > > > - the API stops having two ways to express the same needs; > > > > > > Again, duty-cycle = 0 is not the same as setting the state to disabled. > > > > > > Practically they usually have the same effect, but that's just because > > > in the typical use-case we don't care about the subtle differences. > > > > I fail to see the subtle difference. > > That's okay. I've tried my best to describe it to you in different ways. > If I still haven't been able to convey this, I don't know what else to > say. I reread your explanation several times and fail to understand it. It would be very helpful if you could come up with a scenario where it's obvious that the distinction is needed. You're writing about "typical use-cases" and "value in having the additional flexibility" without providing a use-case that is atypical or actually need the flexibility. Maybe I don't understand the semantic you're giving to the calls or we're using the same words to describe different things. I try to describe the semantic of pwm_disable of the strict API in my words, can you please confirm or correct that?: pwm_disable stops toggling the PWM pin at whatever state the pin currently is. Given a duty cycle between 0% and 100% (exclusive) that means the pin stops at an undefined level. If the duty cycle is 0% or 100% it stops (for an uninverted PWM) at 0 or 1 respectively (and vice versa for an inverted one). The semantic of pwm_enable in my understanding is: pwm_enable for an uninverted PWM starts with $duty_cycle nanoseconds at 0 and then is 1 for $period - $duty_cycle nanoseconds. Then repeat. For an inverted it is first 1 for $duty_cycle nanoseconds and then 0 for the rest of the period. I think pwm_config is to be defined like this: pwm_config changes period and duty cycle without any specification about how the transition should be done. All calls should be synchronous, that is should only return when the change is actually "on the pin". > > > > - most API users can be simplified because they can drop > > > > if (duty == 0) pwm_disable() > > > > - on i.MX the clock can be disabled when not needed saving some energy; > > > > > > That's one reason why we have pwm_disable(). It's what you use to save > > > some energy if you don't need the PWM. Also, you claim that if the clock > > > is disabled the attached backlight will turn on at full brightness, so I > > > fail to see how this would work. > > > > As long as you care about the backlight to be off, don't disable the > > pwm. And if in this state the clk can be disabled, then this should be > > done without an additional poke by the hardware driver which is the only > > place that can know for sure that it is safe to do so. There is no good > > reason to let the pwm user have to know that the currently configured > > state might make some energy saving possible and impose the burden on > > him to then call pwm_disable(). > > This doesn't have anything to do with consumers knowing about potential > energy savings. All we do is give the consumers an opportunity to say: I > need the PWM to be enabled because I want to use the PWM signal now or I > don't really need the PWM signal any more, just stop sending a signal. There are two different degrees of "I don't need the PWM signal any more". In the first case you still care about the output level and in the other you don't. Currently the API doesn't give the PWM user the possibility to specify which of the two are intended. > This is merely a hint from the consumer. What the driver does with it is > ultimately up to the driver. If the driver wants to disable the clock > when the duty cycle goes to zero because it knows that it is safe to do > so, then by all means, feel free to do that. How should the driver know it is safe if the PWM user doesn't have a way to tell if he cares about the output level? > > > Also, you leave out the fact that even after all this work that touches > > > all users and all PWM drivers you still don't get fully correct > > > functionality on your devices because when the driver is removed or shut > > > down you run into the same problem. > > > > It's ok for me that all bets are off when the driver is removed. > > Well, it's not okay for me. Especially if we can do better. Fine, then lets give a way to PWM users to specify if we have to care about the pin state after disable or not. > > I'm > > happy when I fix the problem during active operation because on the > > machine I currently care about the shutdown problem isn't relevant. > > Why is it not relevant? Surely at some point you will want to shut down > that machine. Does a shutdown do anything with a running pwm software wise? The leds-pwm driver doesn't have a shutdown hook. The pwm_bl driver calls pwm_free in some legacy configuration but otherwise does nothing that would undo it's pwm_backlight_power_off() in the shutdown hook. So I'm convinced shutdown is fine here. > And even if not, imx-pwm is used by others and they do > seem to care about shutdown, so they should have a say in the matter as > well. Note that my suggestion doesn't make things worse for those users who care. I'm used to that it's good enough if a patch improves the situation for some users and doesn't make it worse for the others. > > (Also as noted above, even Michal's patch doesn't solve the problem > > formally.) > > Now I'm confused, you said above that "Apart from bugs in it ... this > would solve the issue for sure". So which one is it? I'm sure that you must not assume that after gpio_direction_output(gpio, 0); gpio_free(gpio); the line still drives the 0. In most cases this will be the case, but if this makes the GPIO switch to being an input that should be correct behaviour, too. That's why I wrote "formally" above. Michal's patch solves the gap at startup and when the PWM user calls disable() formally. At unbind when the gpio is freed it's "only" in practise. > > > > Regarding you claim that the API worked fine for a decade: I tried > > > > already in 2013 to address this problem. And even independent of that, > > > > that is not a good reason to not improve stuff. > > > > > > My claim was that it has worked fine "for everyone else". I know that > > > this has been an issue on i.MX for a long time and I've done my best to > > > lay out why I think your proposal is not the right way to fix it. > > > > I still fail to see a single technical reason to not evolve the API. I > > read from your mails: > > > > - it has been like that for ages > > - it works for everyone but i.MX > > - it could be made working for i.MX by involving pinctrl and gpio in > > the hardware specific pwm driver. > > > > Did I miss anything? None of these justifies to keep the status quo in > > my eyes. > > I don't understand your logic here. You want to change the API, so the > burden is on you to convince me *why* it needs to be changed. Changing the API isn't *needed*. But it's the simplest way to fix some (not all) issues I see. Additionally it simplifies some things. > So far you have not been able to do that, so don't try to turn this > around and make me come up with reasons why it shouldn't change. I listed several reasons to change the API, for some you confirmed them to be valid. You listed some for keeping the API as is. As I'm convinced my suggestion is good and your reasons are smaller (or even bad), I try to argue against them. The above question was my try to summarize your reasons and to make sure the list is complete. I didn't try to turn anything around and if that was your impression that's a misunderstanding. With my current understanding the list of reasons to stick to the status quo is: - it has been like that for ages (which is a poor reason to oppose to change); - it works for everyone but i.MX (which I doubt and with my replacement it works for all drivers and users as now including i.MX and maybe others that have the same issue without us knowing it currently); - it could be made working for i.MX by involving pinctrl and gpio in the hardware specific pwm driver (which is ridiculous because my solution is simpler); and - we're changing API (which I'd say is ok given that the specification isn't very specific and breaking the API for good reasons is not nice but acceptable IMHO) In my eyes in the above list there is one valid reason to not change (the last one). If you don't agree to change the userspace API we could keep that constant and only change the kernel-internal API (which is a bit ugly though). The list of reasons in favour of a change are: - the pwm-imx driver can stay/become easier as with the stricter API; - ambiguity removed (hw drivers know in .disable if they can put the pin at a previously "forbidden" value) - users can be simplified by dropping some pwm_disable() So now how can we come to a conclusion? Here are my suggestions for a compromise: What about adding a new callback that we could for example call "disable_lax" that implements disabling the PWM without the guarantee to put the pin in the "off" state? We could also introduce a callback "freeze" (or disable_strict) that is supposed to implement the "strict" disable. Then there is an objective way to determine when all drivers are adapted to the updated model. Do you agree to my suggestion that pwm_config(pwm, 0, period) or similar should already imply disabling clocks if possible without the need to explicitly call pwm_disable() afterwards? Best regards Uwe
On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote: > Uwe, Thierry, > just a few comments as I am not able to dive deeper into your discussion. > > On 11.10.2018 14:00, Thierry Reding wrote: > > On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote: > >> Hello Thierry, > >> > >> On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote: > >>> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote: > >>>> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote: > >>>>> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote: > >>>>>> Hello Thierry, > >>>>>> > >>>>>> (If you have a déjà vu: Yes, I tried to argue this case already in the > >>>>>> past, it's just that I'm just faced with a new use case that's broken > >>>>>> with the current state.) > >>>>>> > >>>>>> Currently the idiom > >>>>>> > >>>>>> pwm_config(mypwm, value, period); > >>>>>> if (!value) > >>>>>> pwm_disable(mypwm); > >>>>>> else > >>>>>> pwm_enable(mypwm); > >>>>>> > >>>>>> (and it's equivalent: > >>>>>> > >>>>>> state.duty_cycle = ... > >>>>>> state.enable = state.duty_cycle ? true : false; > >>>>>> pwm_apply_state(...); > >>>>>> > >>>>>> ) is quite present in the kernel. > >>>>>> > >>>>>> In my eyes this is bad for two reasons: > >>>>>> > >>>>>> a) if the caller is supposed to call pwm_disable() after configuring the > >>>>>> duty cycle to zero, then why doesn't pwm_config() contains this to > >>>>>> unburden pwm-API users and so reduce open coding this assumption? > >>>>> > >>>>> Just to reiterate my thoughts on this: while the config/enable/disable > >>>>> split may seem arbitrary for some use-cases (you may argue most > >>>>> use-cases, and you'd probably be right), I think there's value in having > >>>>> the additional flexibility to explicitly turn off the PWM. Also, duty > >>>>> cycle of zero doesn't always mean "off". If your PWM is inverted, then > >>>>> the duty cycle of zero actually means "on". And that of course also > >>>>> still depends on other components on your board. > >>>> > >>>> Did you notice you didn't argue against me here? We seem to agree that > >>>> calling disabling a PWM after setting the duty cycle to zero is bad. > >> > >> You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but > >> also don't seem to refuse to change pwm users that do: > >> > >> pwm_config(pwm, 0, 100); > >> pwm_disable(pwm); > >> > >> I interpreted your sentence as confirmation that this sequence isn't > >> correct in general. > > > > Yes, it is not correct in general, though it is correct in many cases. > > > >> Where is the difference you care about between duty=0 and disabled? > > > > This is described in the kerneldoc of the corresponding functions. In the > > case of pwm_enable() it starts the output toggling, whereas pwm_enable() > > disables the output toggling. So strictly by going according to the PWM > > API a driver should only need to call pwm_config() if the duty cycle > > actually changes. If all you do is disable and enable, then pwm_enable() > > and pwm_disable() should be good enough. The implicit assumption is that > > the configuration will persist across pwm_disable()/pwm_enable(). In > > practice this doesn't matter that much because most users will be > > setting the duty cycle and enable/disable at the same time. I can easily > > imagine other use-cases where you would want to make the different. One > > other difference could be that pwm_enable() and pwm_disable() require a > > more involved sequence of operations than pwm_config() or vice versa, in > > which cases you may want to avoid one or the other if possible. > > > > In addition, we do expose these details to userspace via sysfs, so not > > allowing an explicit disable after config(duty=0) would break that ABI. > > > >>> I don't see why you think I agree with that. The only thing I'm saying > >>> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the > >>> same as the PWM being disabled. > >> > >> What is in your view the current difference for the hardware behind the > >> PWM? > > > > It depends on the driver implementation, but the kerneldoc is pretty > > specific. I many cases I would expect pwm_disable() to allow a bit of > > extra power to be saved because clocks could be turned off. > > > >>>>> You may have an inverter somewhere, or you may actually be driving a > >>>>> circuit that expects an inverted PWM. > >>>>> > >>>>> So saying that duty-cycle of 0 equals disable is simplifying things too > >>>>> much, in my opinion. > >>>> > >>>> Full ack. So let's fix the users to not call pwm_disable after setting > >>>> the duty cycle to zero! > >>> > >>> I don't understand why you come to that conclusion. It seems like we at > >>> least agree on what the problem is and what potential issues there could > >>> be with different setups. > >>> > >>> The point that we don't agree with is how to fix this. > >> > >> ack. > >> > >>>> Really, only the driver knows when it's safe to disable the clock while > >>>> it's expected to keep the pin at a given state. So better don't expect a > >>>> certain state after pwm_disable(). > >>> > >>> Yes, I agree that only the driver knows when it is safe to do so, but I > >>> still fail to understand why we should change the API in order to > >>> accomodate the specifics of a particular driver. > >> > >> The incentive to change the API is a combination of: > >> > >> - The restriction to keep the pin driving at a certain level after > >> disable isn't a natural and obvious property of the hardware. > >> The designers of the imx-pwm for example did it differently. > > > > I would disagree. A pin that you can't keep at a defined level if it > > isn't actively driven is a fundamentally useless concept. You said > > yourself that the hardware design is problematic because it causes the > > pin to change unexpectedly when you disable the PWM. One could argue > > that that's a bug in the hardware. However, given that there is a way to > > avoid the unexpected change, there is a mechanism outside of PWM to deal > > with this use-case. And that's also perfectly fine. Dealing with pins > > that aren't actively driven is part of the job of a pinmux controller. > > > >> - The change removes ambiguity from from the demands on the hardware > >> drivers. > > > > I don't think there's currently any ambiguity. We may not be stating > > explicitly that the assumption is for a PWM to be "off" after > > pwm_disable(), but it's certainly how everyone has interpreted it. Also > > if there's an ambiguity, then that's what should be addressed. > > > >> - The change doesn't make the API less expressive. > > > > Yes it does. You remove the distinction between duty cycle == 0 and > > the PWM being disabled. > > > >> - The change doesn't doesn't complicate anmy pwm hardware driver. > > > > True. It also doesn't simplify any PWM hardware driver. > > > >> - The change allows to simplify most pwm users. > > > > Yes, the subset that doesn't care about the difference is marginally > > simplified. Although if the API was more rigorous about the definition > > of pwm_disable(), then PWM users would be even more simplified. > > > >> - The change simplifies the imx-pwm driver. > > > > True. But you leave out that the change doesn't actually fix your > > problem. If your PWM user goes away, it will still want to disable the > > PWM because it is no longer in use. However, you don't want that to > > mean that the backlight goes to full brightness again. If you unbind > > the backlight driver, the backlight should remain off (or turn off if > > it isn't already). > > > >> In my experience this is enough to justify such a change. > > > > I would consider the change if it was actually going to fix the issue > > that you're seeing. As it is, all your proposal does is paper over one > > specific symptom, and I'm sorry, but that's just not good enough. > > > >>> So we already know that currently all users expect that the pin state > >>> after disable will be low (because they previously set duty cycle to 0, > >>> which is the active equivalent of low). > >> > >> My theory is that most users don't expect this and/or don't make use of > >> it. As of 9dcd936c5312 we have 50 pwm drivers > >> ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion > >> at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l), > >> and I think there are a few false positive matches, pwm-sun4i.c for > >> example). I'd be willing to bet that as of now imx isn't the only driver > >> with this problem. > > > > We can speculate about that as much as we want. i.MX is the only one > > that this issue has been reported against, so I'm going to have to > > assume that it's working fine for all of the others. If it isn't then > > people should come forward and report it. > > > >>> Given that stricter definition, > >>> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin > >>> state. I think the fix for that is for the i.MX PWM driver to make sure > >>> the pin state doesn't actually change on ->disable(). If that means the > >>> clock needs to remain on, then that's exactly what the driver should be > >>> implementing. > >> > >> Yes, if we keep the API "stricter" this is what should be done. This > >> doesn't justify to keep the API strict though. > > > > But an API that isn't strict is useless. If the behaviour of a PWM isn't > > predictable, how can you use it to drive a backlight. If at any point it > > could just go 100% and turn on the backlight to full brightness, how can > > anyone use it for anything sensible? > > > >>> Are you aware of this: > >>> > >>> http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987 > >>> > >>> This seems to address exactly the problem that you're describing. The > >>> solution matches what I had expected a fix for this problem to look > >>> like. Can you try it and see if that fixes the issue? > >> > >> Right. Apart from bugs in it (for example > >> http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio > >> as output) > > It is a RFC and the past discussion about it was mostly theoretical than > about technical details. So I thought that nobody would really expect > excellent technical solution at this point. I just tried to do my best > and tested dozens of variations of [enabled/disabled/inverted/non- > inverted/ bootloader/device-tree/sysfs] configurations to be sure how it > affects current users and how it improves the situation. I replied to the mail and pointed out a few other things now. This is easier than to talk about your patch in this discussion. > Regarding the GPIO output configuration. Yes, the pin is configured as > input in the imx_pwm_init_pinctrl_info() function. At this stage you > do not really know how to configure the pin. Configuring it as input > is the safest option I think. Did it work without setting the gpio as output in your tests? > I thought that the pin is configured as output whenever gpiod_set_value() > is called. None of the other examples/usages in kernel I looked at used > the gpiod_direction_output() Maybe they use devm_gpio_request_one or similar that return your gpio's direction already configured? > >> Sidenote: With the current capabilities of the pwm framework there is no > >> technical reason to expose to the hardware drivers that the pwm user uses > >> inverted logic. The framework could just apply > >> > >> duty = period - duty; > >> > > I prefer to utilize as much HW capabilities as possible. And it seems > like most PWM chips support HW output inversion (to some extent) so to > me it seems perfectly OK that that information can be propagated from > the upper SW levels to the lowest one. If the hardware capability is useful I fully agree. But if inversion can be accomplished by a chip that doesn't support inversion in hardware by just using duty = period - duty (and so can be accomplished by a chip that has inversion support without making use of it) then the drivers shouldn't be confronted with it. (I'm not sure if inversion is really relevant with the current status quo, as the expectations are not documented in a place I'm aware of.) Best regards Uwe
On Thu, Oct 11, 2018 at 10:34:50PM +0200, Uwe Kleine-König wrote: [...] > > We can speculate about that as much as we want. i.MX is the only one > > that this issue has been reported against, so I'm going to have to > > assume that it's working fine for all of the others. If it isn't then > > people should come forward and report it. > > You really assume that all the drivers that don't make use of > PWM_POLARITY_INVERSED (33 of 50 + imx at least, probably more) behave as > you expect when the PWM user does: > > pwm_apply_state(pwm, { .period = 100; .duty_cycle=0; polarity=PWM_POLARITY_INVERSED; enabled=false; }) > > ? I'd say your assumption that all are working fine is treading on thin > ice. The effect maybe is only that the i.MX users use more corners of > the PWM API and/or are more open to report actual problems. Look, it's unrealistic to expect me to actually test or verify all of the different PWM controllers out there. The only thing I can go by is by what the maintainers of the individual drivers tell me, or what has been reported by users. So I'm going to assume that a driver works until somebody reports that it is broken. So again, if somebody notices that any of the other drivers are broken, please do report it so that it can be fixed. Otherwise I don't have a choice but assume that they still work. > Also if there are two ways to implement something, one works for 49 > different hardware implementations and one for 50 this is already a good > reason to stick to the latter. This discussion isn't going anywhere, we're going in circles. I was not suggesting that we don't find a solution for i.MX, I'm just saying that changing the API is papering over some more fundamental issue on i.MX. You can't magically fix it by changing the API, you have to get pinctrl involved to properly solve this problem. > > > > Given that stricter definition, > > > > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin > > > > state. I think the fix for that is for the i.MX PWM driver to make sure > > > > the pin state doesn't actually change on ->disable(). If that means the > > > > clock needs to remain on, then that's exactly what the driver should be > > > > implementing. > > > > > > Yes, if we keep the API "stricter" this is what should be done. This > > > doesn't justify to keep the API strict though. > > > > But an API that isn't strict is useless. If the behaviour of a PWM isn't > > predictable, how can you use it to drive a backlight. If at any point it > > could just go 100% and turn on the backlight to full brightness, how can > > anyone use it for anything sensible? > > As long as I don't call pwm_disable() (or pwm_config(pwm, period, > period)) it must not go to 100%. I've been arguing that even if you call pwm_disable() it shouldn't go to 100%. So to summarize the problem for i.MX here: at reset time, the pin driving the PWM is configured as GPIO and is configured as input with a high impedance, so that it will be high on boot, prior to any software running. This is reasonable in order to make sure that the backlight stays off until it is properly initialized. What I've been trying to convey is that when you do pwm_disable() the pin should go into exactly that initial configuration to make sure the backlight doesn't behave in an undefined way. > > > > Are you aware of this: > > > > > > > > http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987 > > > > > > > > This seems to address exactly the problem that you're describing. The > > > > solution matches what I had expected a fix for this problem to look > > > > like. Can you try it and see if that fixes the issue? > > > > > > Right. Apart from bugs in it (for example > > > http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio > > > as output) this would solve the issue for sure. This doesn't void my > > > arguments to relax the PWM API though. Also look at the costs: The patch > > > adds 86 lines to the imx driver complicating it considerably. > > > > Now you're just being dramatic. 86 lines of code is not a lot. And I > > don't think it's complicated. I'm willing to carry the burden of those > > 86 lines of code if it fixes the issue for good. > > Right, the complexity doesn't come with the 86 lines of code. With my > approach the pwm-imx driver only has to care about its 6 registers. With > Michal's patch it suddenly has to care about gpio and pinmuxing. Why is that a bad thing? They are fundamentally entangled here, so why do you think that's inappropriate? > > > Each > > > machine that wants to profit of this change has to adapt it's device > > > tree to make the additional pinmuxing and gpio known. > > > To be fair, my change doesn't fix the gap between pinctrl being active > > > and the actual enablement of the pwm. But this could be better solved in > > > the pwm framework without touching any hardware driver with pinctrl > > > mechanism that are already in place. (Make use of the "init" pinctrl > > > state and only switch to "default" when the pwm is configured.) > > > There is nothing imx-pwm specific in hooking a gpio and adapting the > > > pinmuxing in it. So if you want to go this path, please implement it in > > > a generic way such that all pwm devices could benefit. > > > > You're wrong. Judging by all the evidence that we have at this point, > > this is exactly an imx-pwm specific issue, therefore the burden is on > > the imx-pwm driver to implement a solution for it. > > You're the first maintainer I met with this attitude. Whenever I > implemented something in a driver specific way fixing an issue that > other drivers might have to, I was requested to find a generic solution > without the need to analyse if other drivers are affected or not. Well, you should be happy. Dealing with this at a driver level is really much easier than trying to come up with a generic solution. And maybe you have met maintainers with a different attitude, so what? People have different opinions and experiences. This shouldn't be a surprise. In my opinion it's better to not try and generalize upfront. If you try to create a generic solution if all you have is a single use-case, you are bound to get it wrong. You're "generic" solution is likely to tailor to the particular use-case and make it unusable for any others. So I try not to attempt genericity until there are at least two or more use-cases that can be the basis for a generic solution. > There are some subsystems (e.g. tty/serial) where there is much stuff > implemented in each driver even though some things could well be > implemented in a generic way (e.g. rs485 handling). This is really a big > mess. One person's mess is merely another person's filing system. > I reread your explanation several times and fail to understand it. It > would be very helpful if you could come up with a scenario where it's > obvious that the distinction is needed. You're writing about "typical > use-cases" and "value in having the additional flexibility" without > providing a use-case that is atypical or actually need the flexibility. > > Maybe I don't understand the semantic you're giving to the calls or > we're using the same words to describe different things. I try > to describe the semantic of pwm_disable of the strict API in my words, > can you please confirm or correct that?: > > pwm_disable stops toggling the PWM pin at whatever state the pin > currently is. Given a duty cycle between 0% and 100% (exclusive) > that means the pin stops at an undefined level. If the duty > cycle is 0% or 100% it stops (for an uninverted PWM) at 0 or 1 > respectively (and vice versa for an inverted one). No, I don't think that's entirely correct. The current assumption by all users is more that the pin would stop at the no power state, which would be 0 for normal PWM and 1 for inverted. This is what's reasonable for most or all cases. It's pretty much the same as for a clock, or a regulator or any other number of devices. > The semantic of pwm_enable in my understanding is: > > pwm_enable for an uninverted PWM starts with $duty_cycle > nanoseconds at 0 and then is 1 for $period - $duty_cycle > nanoseconds. Then repeat. > For an inverted it is first 1 for $duty_cycle nanoseconds and > then 0 for the rest of the period. > > I think pwm_config is to be defined like this: > > pwm_config changes period and duty cycle without any > specification about how the transition should be done. > > All calls should be synchronous, that is should only return when the > change is actually "on the pin". Correct. > > > > > - most API users can be simplified because they can drop > > > > > if (duty == 0) pwm_disable() > > > > > - on i.MX the clock can be disabled when not needed saving some energy; > > > > > > > > That's one reason why we have pwm_disable(). It's what you use to save > > > > some energy if you don't need the PWM. Also, you claim that if the clock > > > > is disabled the attached backlight will turn on at full brightness, so I > > > > fail to see how this would work. > > > > > > As long as you care about the backlight to be off, don't disable the > > > pwm. And if in this state the clk can be disabled, then this should be > > > done without an additional poke by the hardware driver which is the only > > > place that can know for sure that it is safe to do so. There is no good > > > reason to let the pwm user have to know that the currently configured > > > state might make some energy saving possible and impose the burden on > > > him to then call pwm_disable(). > > > > This doesn't have anything to do with consumers knowing about potential > > energy savings. All we do is give the consumers an opportunity to say: I > > need the PWM to be enabled because I want to use the PWM signal now or I > > don't really need the PWM signal any more, just stop sending a signal. > > There are two different degrees of "I don't need the PWM signal any > more". In the first case you still care about the output level and in > the other you don't. Currently the API doesn't give the PWM user the > possibility to specify which of the two are intended. I've said before that "don't care about the output level" is completely useless. In most cases what you really want is the "no power" state and that's what pwm_disable() should ensure. You're PWM could be controlling a fan, and when you disable the PWM you want the fan to stop spinning, right? You wouldn't want the fan to keep spinning after you've disabled it. This is something that most hardware engineers will also understand. They tend to design their systems in such a way that the reset defaults will have sensible effects. I think it's good in general to attempt, if possible, to restore those reset defaults when you disable the device that you're driving. > > > happy when I fix the problem during active operation because on the > > > machine I currently care about the shutdown problem isn't relevant. > > > > Why is it not relevant? Surely at some point you will want to shut down > > that machine. > > Does a shutdown do anything with a running pwm software wise? The > leds-pwm driver doesn't have a shutdown hook. The pwm_bl driver calls > pwm_free in some legacy configuration but otherwise does nothing that > would undo it's pwm_backlight_power_off() in the shutdown hook. So I'm > convinced shutdown is fine here. But pwm_backlight_power_off() will also end up calling pwm_disable(), which by your description of the problem would fully light up the backlight. That is, unless your driver implement ->apply(), but in that case you wouldn't have a problem anyway. > > And even if not, imx-pwm is used by others and they do > > seem to care about shutdown, so they should have a say in the matter as > > well. > > Note that my suggestion doesn't make things worse for those users who > care. I'm used to that it's good enough if a patch improves the > situation for some users and doesn't make it worse for the others. > > > > (Also as noted above, even Michal's patch doesn't solve the problem > > > formally.) > > > > Now I'm confused, you said above that "Apart from bugs in it ... this > > would solve the issue for sure". So which one is it? > > I'm sure that you must not assume that after > > gpio_direction_output(gpio, 0); > gpio_free(gpio); > > the line still drives the 0. In most cases this will be the case, but if > this makes the GPIO switch to being an input that should be correct > behaviour, too. That's why I wrote "formally" above. > > Michal's patch solves the gap at startup and when the PWM user calls > disable() formally. At unbind when the gpio is freed it's "only" in > practise. That's why pinctrl is also involved. That should ensure that the correct level is driven to the pin even if it isn't actively driven by either GPIO or PWM. > I listed several reasons to change the API, for some you confirmed them > to be valid. You listed some for keeping the API as is. As I'm > convinced my suggestion is good and your reasons are smaller (or even > bad), I try to argue against them. > > The above question was my try to summarize your reasons and to make sure > the list is complete. I didn't try to turn anything around and if that > was your impression that's a misunderstanding. > > With my current understanding the list of reasons to stick to the status > quo is: > > - it has been like that for ages > (which is a poor reason to oppose to change); > - it works for everyone but i.MX > (which I doubt and with my replacement it works for all drivers and > users as now including i.MX and maybe others that have the same issue > without us knowing it currently); > - it could be made working for i.MX by involving pinctrl and gpio in > the hardware specific pwm driver (which is ridiculous because my > solution is simpler); and > - we're changing API > (which I'd say is ok given that the specification isn't very specific > and breaking the API for good reasons is not nice but acceptable IMHO) > > In my eyes in the above list there is one valid reason to not change > (the last one). If you don't agree to change the userspace API we could > keep that constant and only change the kernel-internal API (which is a > bit ugly though). > > The list of reasons in favour of a change are: > > - the pwm-imx driver can stay/become easier as with the stricter API; > - ambiguity removed > (hw drivers know in .disable if they can put the pin at a previously > "forbidden" value) > - users can be simplified by dropping some pwm_disable() > > So now how can we come to a conclusion? Here are my suggestions for a > compromise: > > What about adding a new callback that we could for example call > "disable_lax" that implements disabling the PWM without the guarantee to > put the pin in the "off" state? > > We could also introduce a callback "freeze" (or disable_strict) that is > supposed to implement the "strict" disable. Then there is an objective > way to determine when all drivers are adapted to the updated model. > > Do you agree to my suggestion that pwm_config(pwm, 0, period) or similar > should already imply disabling clocks if possible without the need to > explicitly call pwm_disable() afterwards? I don't think it has to be that explicit. Drivers should certainly feel free to do so if it works and is reasonable. To be honest, the whole discussion here is somewhat unproductive. You're arguing about changing an API that's deprecated anyway, even if not officially. We already introduced the atomic API a few years ago that is meant to address most of these issues. So instead of trying to "fix" a legacy API, have you considered moving the i.MX driver to the atomic API? That should give you much better control over when and how to program the registers. Thierry
On Fri, Oct 12, 2018 at 11:53:49AM +0200, Thierry Reding wrote: > On Thu, Oct 11, 2018 at 10:34:50PM +0200, Uwe Kleine-König wrote: [...] > > The semantic of pwm_enable in my understanding is: > > > > pwm_enable for an uninverted PWM starts with $duty_cycle > > nanoseconds at 0 and then is 1 for $period - $duty_cycle > > nanoseconds. Then repeat. > > For an inverted it is first 1 for $duty_cycle nanoseconds and > > then 0 for the rest of the period. Slight correction here: these are actually reversed. If you look at the kerneldoc for enum pwm_polarity it explains what the expectations are. Thierry
On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote: > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote: [...] > > >> Sidenote: With the current capabilities of the pwm framework there is no > > >> technical reason to expose to the hardware drivers that the pwm user uses > > >> inverted logic. The framework could just apply > > >> > > >> duty = period - duty; > > >> > > > > I prefer to utilize as much HW capabilities as possible. And it seems > > like most PWM chips support HW output inversion (to some extent) so to > > me it seems perfectly OK that that information can be propagated from > > the upper SW levels to the lowest one. > > If the hardware capability is useful I fully agree. But if inversion can > be accomplished by a chip that doesn't support inversion in hardware by > just using duty = period - duty (and so can be accomplished by a chip > that has inversion support without making use of it) then the drivers > shouldn't be confronted with it. These two are orthogonal problems. If all you care about is the power output of the PWM (as is the case for backlights, LEDs or fans, for example), then inverted polarity has the same effect as duty = period - duty. However, the two don't result in identical waveforms. Again, a backlight or LED doesn't care about the exact waveform, but there are other uses for PWMs where these actually are important. I have admittedly never used any of them myself, but this was brought to my attention a long time ago when polarity support was introduced. I'm sure you can find the discussions in some archives if you are inclined to do so. > (I'm not sure if inversion is really relevant with the current status > quo, as the expectations are not documented in a place I'm aware of.) See the kerneldoc for enum pwm_polarity which does document this. Thierry
On 12.10.2018 11:45, Uwe Kleine-König wrote: > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote: >> Uwe, Thierry, [...] >> It is a RFC and the past discussion about it was mostly theoretical than >> about technical details. So I thought that nobody would really expect >> excellent technical solution at this point. I just tried to do my best >> and tested dozens of variations of [enabled/disabled/inverted/non- >> inverted/ bootloader/device-tree/sysfs] configurations to be sure how it >> affects current users and how it improves the situation. > > I replied to the mail and pointed out a few other things now. This is > easier than to talk about your patch in this discussion. Ack. >> Regarding the GPIO output configuration. Yes, the pin is configured as >> input in the imx_pwm_init_pinctrl_info() function. At this stage you >> do not really know how to configure the pin. Configuring it as input >> is the safest option I think. > > Did it work without setting the gpio as output in your tests? Yes, I am using it like that for aprox. 2 months without issues. >> I thought that the pin is configured as output whenever gpiod_set_value() >> is called. None of the other examples/usages in kernel I looked at used >> the gpiod_direction_output() > > Maybe they use devm_gpio_request_one or similar that return your gpio's > direction already configured? Nope. Examples I looked at (mainly I2C recovery) use only the gpiolib functions. Michal
Hello Michal, On Fri, Oct 12, 2018 at 12:25:01PM +0000, Vokáč Michal wrote: > >> Regarding the GPIO output configuration. Yes, the pin is configured as > >> input in the imx_pwm_init_pinctrl_info() function. At this stage you > >> do not really know how to configure the pin. Configuring it as input > >> is the safest option I think. > > > > Did it work without setting the gpio as output in your tests? > > Yes, I am using it like that for aprox. 2 months without issues. and you're sure the gpio is configured as output? Strange. > >> I thought that the pin is configured as output whenever gpiod_set_value() > >> is called. None of the other examples/usages in kernel I looked at used > >> the gpiod_direction_output() > > > > Maybe they use devm_gpio_request_one or similar that return your gpio's > > direction already configured? > > Nope. Examples I looked at (mainly I2C recovery) use only the gpiolib > functions. For example drivers/i2c/busses/i2c-imx.c uses devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN); which configures as output (somewhat). Best regards Uwe
Hello Thierry, On Fri, Oct 12, 2018 at 11:53:49AM +0200, Thierry Reding wrote: > On Thu, Oct 11, 2018 at 10:34:50PM +0200, Uwe Kleine-König wrote: > [...] > > > We can speculate about that as much as we want. i.MX is the only one > > > that this issue has been reported against, so I'm going to have to > > > assume that it's working fine for all of the others. If it isn't then > > > people should come forward and report it. > > > > You really assume that all the drivers that don't make use of > > PWM_POLARITY_INVERSED (33 of 50 + imx at least, probably more) behave as > > you expect when the PWM user does: > > > > pwm_apply_state(pwm, { .period = 100; .duty_cycle=0; polarity=PWM_POLARITY_INVERSED; enabled=false; }) > > > > ? I'd say your assumption that all are working fine is treading on thin > > ice. The effect maybe is only that the i.MX users use more corners of > > the PWM API and/or are more open to report actual problems. > > Look, it's unrealistic to expect me to actually test or verify all of > the different PWM controllers out there. The only thing I can go by is > by what the maintainers of the individual drivers tell me, or what has > been reported by users. So I'm going to assume that a driver works until > somebody reports that it is broken. I didn't expect from you to test everything. I just pointed out that your argument "I assume all drivers to behave correctly because there is no report that any of them doesn't work" doesn't build on solid facts. But lets make this explicit and official: I looked into all 48 pwm drivers (at commit 6b3944e42e2e) and sorted them into categories: a) not ported to pwm_state and no support .set_polarity: - pwm-ab8500.c - pwm-brcmstb.c - pwm-clps711x.c - pwm-crc.c - pwm-img.c - pwm-imx.c (1 of 2 pwm_ops in this driver) - pwm-lp3943.c - pwm-lpc32xx.c - pwm-mediatek.c - pwm-mtk-disp.c - pwm-mxs.c - pwm-pca9685.c - pwm-puv3.c - pwm-pxa.c - pwm-rcar.c - pwm-spear.c - pwm-sti.c - pwm-stmpe.c - pwm-twl-led.c - pwm-twl.c - pwm-tegra.c b) supports inversion in hardware and pwm_state, polarity handling looks fine: - pwm-bcm-iproc.c - pwm-hibvt.c - pwm-rockchip.c - pwm-sun4i.c - pwm-zx.c c) not ported to pwm_state, supports .set_polarity in hardware: - pwm-atmel-tcb.c - pwm-bcm2835.c - pwm-bcm-kona.c - pwm-berlin.c - pwm-ep93xx.c - pwm-fsl-ftm.c - pwm-lpc18xx-sct.c - pwm-jz4740.c - pwm-omap-dmtimer.c (not sure here, didn't find the actual implementation) - pwm-renesas-tpu.c - pwm-samsung.c - pwm-vt8500.c - pwm-tiehrpwm.c - pwm-tiecap.c d) implements pwm_state, but ignores polarity: - pwm-cros-ec.c - pwm-lpss.c (with pwm-lpss-pci.c, pwm-lpss-platform.c) e) supports state, but doesn't check .polarity on disable - pwm-atmel.c - pwm-atmel-hlcdc.c - pwm-imx.c (1 of 2 pwm_ops in this driver) - pwm-meson.c - pwm-stm32.c - pwm-stm32-lp.c f) not actually a pwm driver (huh?): - pwm-tipwmss.c So it's not as worse as I expected, mainly because some drivers don't mention PWM_POLARITY_INVERSED but test for PWM_POLARITY_NORMAL. The drivers in categories d) and e) are definitively broken and need fixing. Some in the categories b) and c) might have the same problem as imx (i.e. the output doesn't behave in pwm_disable(pwm) as in pwm_config(pwm, 0, period)), but I'm unable to research that without the respective hardware on my desk. (Reading the HW manual probably isn't enough because most of them won't be explicit what happens in "off" state. At least that's the case for the imx manual.) > So again, if somebody notices that any of the other drivers are broken, > please do report it so that it can be fixed. Otherwise I don't have a > choice but assume that they still work. done. :-) > [...] I deleted a big part of your mail and the repeating arguments I already wrote in reply to it. I hope to work towards a compromise this way and taking some heat out of the discussion. > To be honest, the whole discussion here is somewhat unproductive. I think the problem in this discussion is that there is a misunderstanding between us. In my eyes the current state is: I want to implement a change to the PWM API because I see a benefit in it. I listed several advantages, some of them you accepted. You claimed there was a disadvantage that makes the API less expressive. I tried to understand that, asked several times for an example where the difference you see actually matters but I don't get a reply to this question. In this state I cannot invalidate your claim and I stay dissatisfied because my incentive for a change is blocked by an argument I think is void. So I'll try to only discuss in the remaining part of this mail the single issue that I think is key to our discussion: You state there was a difference between pwm_disable(pwm); and pwm_config(pwm, 0, period); In my eyes they are equivalent. Both are expected to set the output to inactive (that is 0 for an uninverted PWM, 1 for an inverted one). The only differences I see are: - hardware driver details Some driver might not disable some resources after pwm_config and so failing to save some power. You stated however this is not the relevant issue. - after pwm_config(pwm, 0, period) pwm_enable(pwm) doesn't restore the old duty cycle. So with my suggested change the PWM framework stops caching the duty cycle. This appears too tiny for a reason to me to actually matter. I assume I failed to list the detail you consider relevant. Can you please make an effort to describe a use-case where the difference you care about actually matters? Best regards Uwe
Hello Thierry, On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote: > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote: > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote: > [...] > > > >> Sidenote: With the current capabilities of the pwm framework there is no > > > >> technical reason to expose to the hardware drivers that the pwm user uses > > > >> inverted logic. The framework could just apply > > > >> > > > >> duty = period - duty; > > > >> > > > > > > I prefer to utilize as much HW capabilities as possible. And it seems > > > like most PWM chips support HW output inversion (to some extent) so to > > > me it seems perfectly OK that that information can be propagated from > > > the upper SW levels to the lowest one. > > > > If the hardware capability is useful I fully agree. But if inversion can > > be accomplished by a chip that doesn't support inversion in hardware by > > just using duty = period - duty (and so can be accomplished by a chip > > that has inversion support without making use of it) then the drivers > > shouldn't be confronted with it. > > These two are orthogonal problems. If all you care about is the power > output of the PWM (as is the case for backlights, LEDs or fans, for > example), then inverted polarity has the same effect as duty = period - > duty. > > However, the two don't result in identical waveforms. OK, let's find the difference then. Consider a pwm that is off at start, then it's configured to run at 66% duty cycle at time A and then it's disabled again at time B. The resulting wave form is: ______________ __ __ __ _____________________ \_____/ \_____/ \_____/ \_____/ A B ^ ^ ^ ^ With the ^ markers pointing out where the periods started. Correct? Now with the duty = period - duty trick the wave would look as follows: _________________ __ __ __ __________________ \_____/ \_____/ \_____/ \_____/ A B ^ ^ ^ ^ Apart from a shift I cannot see any difference. I confirm that this might be bad if there are two (or more) PWMs that are expected to run in sync, but given that the current API doesn't provide the possibility to map such a use case this is irrelevant. Another difference I confirm there might be is that this might result in one more (or less) periods depending on timing and the capabilities of the hardware. Is this bad? I'd say it isn't. What am I missing? Best regards Uwe
Hello, adding the author of the commit that introduced the .set_polarity callback (0aa0869c3c9b10338dd92a20fa4a9b6959f177b5), maybe he can add some value to this discussion. On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote: > On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote: > > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote: > > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote: > > [...] > > > > >> Sidenote: With the current capabilities of the pwm framework there is no > > > > >> technical reason to expose to the hardware drivers that the pwm user uses > > > > >> inverted logic. The framework could just apply > > > > >> > > > > >> duty = period - duty; > > > > >> > > > > > > > > I prefer to utilize as much HW capabilities as possible. And it seems > > > > like most PWM chips support HW output inversion (to some extent) so to > > > > me it seems perfectly OK that that information can be propagated from > > > > the upper SW levels to the lowest one. > > > > > > If the hardware capability is useful I fully agree. But if inversion can > > > be accomplished by a chip that doesn't support inversion in hardware by > > > just using duty = period - duty (and so can be accomplished by a chip > > > that has inversion support without making use of it) then the drivers > > > shouldn't be confronted with it. > > > > These two are orthogonal problems. If all you care about is the power > > output of the PWM (as is the case for backlights, LEDs or fans, for > > example), then inverted polarity has the same effect as duty = period - > > duty. > > > > However, the two don't result in identical waveforms. > > OK, let's find the difference then. Consider a pwm that is off at start, > then it's configured to run at 66% duty cycle at time A and then it's > disabled again at time B. > > The resulting wave form is: > > ______________ __ __ __ _____________________ > \_____/ \_____/ \_____/ \_____/ > A B > ^ ^ ^ ^ > > With the ^ markers pointing out where the periods started. > > Correct? > > Now with the duty = period - duty trick the wave would look as follows: > > > _________________ __ __ __ __________________ > \_____/ \_____/ \_____/ \_____/ > A B > ^ ^ ^ ^ > > Apart from a shift I cannot see any difference. I confirm that this > might be bad if there are two (or more) PWMs that are expected to run in > sync, but given that the current API doesn't provide the possibility to > map such a use case this is irrelevant. > > Another difference I confirm there might be is that this might result in > one more (or less) periods depending on timing and the capabilities of > the hardware. Is this bad? I'd say it isn't. > > What am I missing? The commit log of 0aa0869c3c9b10338dd92a20fa4a9b6959f177b5 mentions: A practical example where [changing polarity] can prove useful is to simulate inversion of the duty cycle. While inversion of polarity and duty cycle are not exactly the same, the differences for most use-cases are negligible. This doesn't help me to understand why the polarity stuff is useful though. Does this log say that the motivation to support changing polarity was to simulate "inversion of the duty cycle"? What exactly is "inversion of the duty cycle"? Just duty_cycle = period - duty_cycle; ? Why does this have to be simulated, the API was able to do this without any simulation before? Is there a single use-case that can be done with polarity support in the API that isn't possible without it? I cannot imagine there is any but would like to learn and understand why this is valuable. Currently there is no user of pwm_set_polarity(), I suggest to remove it to not give users an incentive to still rely on the non-atomic API. Also among the users of pwm_apply_state there is none which makes use of the polarity setting. Best regards Uwe
Hello, On Mon, Oct 15, 2018 at 10:14:21AM +0200, Uwe Kleine-König wrote: > adding the author of the commit that introduced the .set_polarity > callback (0aa0869c3c9b10338dd92a20fa4a9b6959f177b5), maybe he can add > some value to this discussion. there is no insight to be expected from that, the email address doesn't exist any more. Best regards Uwe
On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote: > > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote: > > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote: > > [...] > > > > >> Sidenote: With the current capabilities of the pwm framework there is no > > > > >> technical reason to expose to the hardware drivers that the pwm user uses > > > > >> inverted logic. The framework could just apply > > > > >> > > > > >> duty = period - duty; > > > > >> > > > > > > > > I prefer to utilize as much HW capabilities as possible. And it seems > > > > like most PWM chips support HW output inversion (to some extent) so to > > > > me it seems perfectly OK that that information can be propagated from > > > > the upper SW levels to the lowest one. > > > > > > If the hardware capability is useful I fully agree. But if inversion can > > > be accomplished by a chip that doesn't support inversion in hardware by > > > just using duty = period - duty (and so can be accomplished by a chip > > > that has inversion support without making use of it) then the drivers > > > shouldn't be confronted with it. > > > > These two are orthogonal problems. If all you care about is the power > > output of the PWM (as is the case for backlights, LEDs or fans, for > > example), then inverted polarity has the same effect as duty = period - > > duty. > > > > However, the two don't result in identical waveforms. > > OK, let's find the difference then. Consider a pwm that is off at start, > then it's configured to run at 66% duty cycle at time A and then it's > disabled again at time B. > > The resulting wave form is: > > ______________ __ __ __ _____________________ > \_____/ \_____/ \_____/ \_____/ > A B > ^ ^ ^ ^ > > With the ^ markers pointing out where the periods started. > > Correct? > > Now with the duty = period - duty trick the wave would look as follows: > > > _________________ __ __ __ __________________ > \_____/ \_____/ \_____/ \_____/ > A B > ^ ^ ^ ^ > > Apart from a shift I cannot see any difference. I confirm that this > might be bad if there are two (or more) PWMs that are expected to run in > sync, but given that the current API doesn't provide the possibility to > map such a use case this is irrelevant. > > Another difference I confirm there might be is that this might result in > one more (or less) periods depending on timing and the capabilities of > the hardware. Is this bad? I'd say it isn't. > > What am I missing? I don't think that's entirely correct. For normal PWM with 66% duty cycle you'd get something like this: ____ ____ ____ / \__/ \__/ \__ (1) ^ ^ ^ For "emulated" inversion (duty cycle = period - duty cycle, i.e. 33% duty cycle) you'd get this: __ __ __ / \____/ \____/ \____ (2) ^ ^ ^ For a truly inversed PWM (duty cycle = 33%), it would look like this: __ __ __ \____/ \____/ \____/ (3) ^ ^ ^ If you emulate inversion of the truly inversed PWM, you'd get this: ____ ____ ____ \__/ \__/ \__/ (4) ^ ^ ^ As you can see, all of the above are slightly different. As you can see, and you already noted that, the emulated inversion (2) and the true inversion (3) are almost identical, except that they have a phase shift that is equivalent to the duty cycle. The same is true of (1) and (4). The phase shift doesn't affect the power output of the signal, so (1) and (4) as well as (2) and (3) have the same power output. This is the reason why the difference doesn't matter for backlight or LEDs, because brightness is proportional to the power output. However, you can also see that (2) and (3) are different in how the period starts. (2) starts with a rising edge while (3) starts with a falling edge. Now, for any use-cases where the only thing we care about is the power output, my opinion is that consumers should implement the inversion. If the hardware is capable of inversion, then by all means they should use that in the implementation. But, a lot of hardware is not capable of doing the inversion, so they can still fall back to emulating the inversion. Either way, the important point here is that the consumer driver (pwm-backlight, leds-pwm, ...) knows that only power output is relevant, so it is the consumer drivers that should make the decision. On the other hand, other use-cases may rely on the exact shape of the PWM signal, so if the edges are important, then they need to rely on the hardware inversion for proper functionality. My understanding is that we don't have any of those users in the kernel, but my recollection is that people use the sysfs interfaces to make use of this. Like I said elsewhere, if I remember correctly, people use this as a way of synchronizing motors using PWM pulses. My knowledge of those use-cases stops there because I've never used PWMs like that. Thierry
On Mon, Oct 15, 2018 at 10:14:21AM +0200, Uwe Kleine-König wrote: > Hello, > > adding the author of the commit that introduced the .set_polarity > callback (0aa0869c3c9b10338dd92a20fa4a9b6959f177b5), maybe he can add > some value to this discussion. > > On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote: > > On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote: > > > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote: > > > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote: > > > [...] > > > > > >> Sidenote: With the current capabilities of the pwm framework there is no > > > > > >> technical reason to expose to the hardware drivers that the pwm user uses > > > > > >> inverted logic. The framework could just apply > > > > > >> > > > > > >> duty = period - duty; > > > > > >> > > > > > > > > > > I prefer to utilize as much HW capabilities as possible. And it seems > > > > > like most PWM chips support HW output inversion (to some extent) so to > > > > > me it seems perfectly OK that that information can be propagated from > > > > > the upper SW levels to the lowest one. > > > > > > > > If the hardware capability is useful I fully agree. But if inversion can > > > > be accomplished by a chip that doesn't support inversion in hardware by > > > > just using duty = period - duty (and so can be accomplished by a chip > > > > that has inversion support without making use of it) then the drivers > > > > shouldn't be confronted with it. > > > > > > These two are orthogonal problems. If all you care about is the power > > > output of the PWM (as is the case for backlights, LEDs or fans, for > > > example), then inverted polarity has the same effect as duty = period - > > > duty. > > > > > > However, the two don't result in identical waveforms. > > > > OK, let's find the difference then. Consider a pwm that is off at start, > > then it's configured to run at 66% duty cycle at time A and then it's > > disabled again at time B. > > > > The resulting wave form is: > > > > ______________ __ __ __ _____________________ > > \_____/ \_____/ \_____/ \_____/ > > A B > > ^ ^ ^ ^ > > > > With the ^ markers pointing out where the periods started. > > > > Correct? > > > > Now with the duty = period - duty trick the wave would look as follows: > > > > > > _________________ __ __ __ __________________ > > \_____/ \_____/ \_____/ \_____/ > > A B > > ^ ^ ^ ^ > > > > Apart from a shift I cannot see any difference. I confirm that this > > might be bad if there are two (or more) PWMs that are expected to run in > > sync, but given that the current API doesn't provide the possibility to > > map such a use case this is irrelevant. > > > > Another difference I confirm there might be is that this might result in > > one more (or less) periods depending on timing and the capabilities of > > the hardware. Is this bad? I'd say it isn't. > > > > What am I missing? > > The commit log of 0aa0869c3c9b10338dd92a20fa4a9b6959f177b5 mentions: > > A practical example where [changing polarity] can prove useful is to > simulate inversion of the duty cycle. While inversion of polarity > and duty cycle are not exactly the same, the differences for most > use-cases are negligible. > > This doesn't help me to understand why the polarity stuff is useful > though. Does this log say that the motivation to support changing > polarity was to simulate "inversion of the duty cycle"? What exactly is > "inversion of the duty cycle"? Just > > duty_cycle = period - duty_cycle; > > ? Why does this have to be simulated, the API was able to do this > without any simulation before? > > Is there a single use-case that can be done with polarity support in the > API that isn't possible without it? I cannot imagine there is any but > would like to learn and understand why this is valuable. See my earlier mail which will hopefully answer these questions. > Currently there is no user of pwm_set_polarity(), I suggest to remove it > to not give users an incentive to still rely on the non-atomic API. Yeah, that's probably a good idea. The long-term plan was always to get rid of the legacy API, but I haven't gotten around to that yet. > Also among the users of pwm_apply_state there is none which makes use of > the polarity setting. Well, this is primarily because our current users don't care much about the polarity setting, so they just go with whatever initial setting they get (usually via pwm_get_args()). These correspond to whatever was specified in the flags of the PWM specifier in DT or in the lookup table for non-DT devices (see for example of_pwm_xlate_with_flags()). There is one user that allows changing the polarity, and that's sysfs. Thierry
On Mon, Oct 15, 2018 at 11:22:13AM +0200, Thierry Reding wrote: > On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote: > > Hello Thierry, > > > > On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote: > > > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote: > > > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote: > > > [...] > > > > > >> Sidenote: With the current capabilities of the pwm framework there is no > > > > > >> technical reason to expose to the hardware drivers that the pwm user uses > > > > > >> inverted logic. The framework could just apply > > > > > >> > > > > > >> duty = period - duty; > > > > > >> > > > > > > > > > > I prefer to utilize as much HW capabilities as possible. And it seems > > > > > like most PWM chips support HW output inversion (to some extent) so to > > > > > me it seems perfectly OK that that information can be propagated from > > > > > the upper SW levels to the lowest one. > > > > > > > > If the hardware capability is useful I fully agree. But if inversion can > > > > be accomplished by a chip that doesn't support inversion in hardware by > > > > just using duty = period - duty (and so can be accomplished by a chip > > > > that has inversion support without making use of it) then the drivers > > > > shouldn't be confronted with it. > > > > > > These two are orthogonal problems. If all you care about is the power > > > output of the PWM (as is the case for backlights, LEDs or fans, for > > > example), then inverted polarity has the same effect as duty = period - > > > duty. > > > > > > However, the two don't result in identical waveforms. > > > > OK, let's find the difference then. Consider a pwm that is off at start, > > then it's configured to run at 66% duty cycle at time A and then it's > > disabled again at time B. > > > > The resulting wave form is: > > > > ______________ __ __ __ _____________________ > > \_____/ \_____/ \_____/ \_____/ > > A B > > ^ ^ ^ ^ > > > > With the ^ markers pointing out where the periods started. > > > > Correct? > > > > Now with the duty = period - duty trick the wave would look as follows: > > > > > > _________________ __ __ __ __________________ > > \_____/ \_____/ \_____/ \_____/ > > A B > > ^ ^ ^ ^ > > > > Apart from a shift I cannot see any difference. I confirm that this > > might be bad if there are two (or more) PWMs that are expected to run in > > sync, but given that the current API doesn't provide the possibility to > > map such a use case this is irrelevant. > > > > Another difference I confirm there might be is that this might result in > > one more (or less) periods depending on timing and the capabilities of > > the hardware. Is this bad? I'd say it isn't. > > > > What am I missing? > > I don't think that's entirely correct. For normal PWM with 66% duty > cycle you'd get something like this: > > ____ ____ ____ > / \__/ \__/ \__ (1) > ^ ^ ^ > > For "emulated" inversion (duty cycle = period - duty cycle, i.e. 33% > duty cycle) you'd get this: > > __ __ __ > / \____/ \____/ \____ (2) > ^ ^ ^ > > For a truly inversed PWM (duty cycle = 33%), it would look like this: > > __ __ __ > \____/ \____/ \____/ (3) > ^ ^ ^ > > If you emulate inversion of the truly inversed PWM, you'd get this: > > ____ ____ ____ > \__/ \__/ \__/ (4) > ^ ^ ^ > > As you can see, all of the above are slightly different. As you can see, > and you already noted that, the emulated inversion (2) and the true > inversion (3) are almost identical, except that they have a phase shift > that is equivalent to the duty cycle. The same is true of (1) and (4). > The phase shift doesn't affect the power output of the signal, so (1) > and (4) as well as (2) and (3) have the same power output. This is the > reason why the difference doesn't matter for backlight or LEDs, because > brightness is proportional to the power output. > > However, you can also see that (2) and (3) are different in how the > period starts. (2) starts with a rising edge while (3) starts with a > falling edge. > > Now, for any use-cases where the only thing we care about is the power > output, my opinion is that consumers should implement the inversion. If > the hardware is capable of inversion, then by all means they should use > that in the implementation. But, a lot of hardware is not capable of > doing the inversion, so they can still fall back to emulating the > inversion. Either way, the important point here is that the consumer > driver (pwm-backlight, leds-pwm, ...) knows that only power output is > relevant, so it is the consumer drivers that should make the decision. > > On the other hand, other use-cases may rely on the exact shape of the > PWM signal, so if the edges are important, then they need to rely on the > hardware inversion for proper functionality. My understanding is that we > don't have any of those users in the kernel, but my recollection is that > people use the sysfs interfaces to make use of this. Your figures suggest that the first edge is different, too, but that's wrong because you didn't take the idle state into account that was active before the first period. If the output was already high before there cannot be a raising edge. When this is corrected in your figures they look as follows: 1) uninverted ____ ____ ____ ___/ \__/ \__/ \__ ^ ^ ^ 2) emulated inversion ______ __ __ \____/ \____/ \____ ^ ^ ^ 3) hw inversion: __ __ __ \____/ \____/ \____/ ^ ^ ^ 4) hw + emulated inversion: ____ ____ ____ ______/ \__/ \__/ (4) ^ ^ ^ So the order of the edges is identical for hardware inversion and software inversion. The only difference is that with hardware inversion the first edge might come earlier. But with the current PWM API you cannot make use of this "advantage" because there is no way for the user to sync the period of the PWM to anything else (e.g. another PWM). Also typically for a motor control you want two PWMs running with a phase shift like: ___ ___ ___ ____/ \___/ \___/ ^ ___ ^ ___ ^ ______/ \___/ \___/ ^ ^ ^ So independant of inversion or not the periods of the two signals must be shifted by (say) $period/2. How do you achive that? I'd say the best you can do is: local_irq_disable(); pwm_enable(pwm1); delay(period / 2); pwm_enable(pwm2); local_irq_enable(); Now given that it can take some time until pwm_enable() returns, this is poor timing and you'd have to reduce the delay (by experimenting). If you want to do that from userspace via sysfs you cannot even disable irqs and the timing depends on how many ethernet packages are processed between your two syscalls. Trying this without additional hardware capabilities (e.g. a counter shared by two PWMs) is broken. As the PWM API doesn't expose such capabilities changing how inversion is implemented won't break any user that isn't already broken before. Best regards Uwe
diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c index 7f5a18fa305b..55db3b63add8 100644 --- a/arch/arm/mach-s3c24xx/mach-rx1950.c +++ b/arch/arm/mach-s3c24xx/mach-rx1950.c @@ -429,7 +429,6 @@ static void rx1950_lcd_power(int enable) /* GPB1->OUTPUT, GPB1->0 */ gpio_direction_output(S3C2410_GPB(1), 0); pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD); - pwm_disable(lcd_pwm); /* GPC0->0, GPC10->0 */ gpio_direction_output(S3C2410_GPC(0), 0); diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index b443278e569c..a2b0e5f74ac8 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -784,8 +784,6 @@ static void pwm_disable_backlight(const struct drm_connector_state *old_conn_sta /* Disable the backlight */ pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS); - usleep_range(2000, 3000); - pwm_disable(panel->backlight.pwm); } void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_state) diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index 7838af58f92d..0f1b597bb252 100644 --- a/drivers/hwmon/pwm-fan.c +++ b/drivers/hwmon/pwm-fan.c @@ -51,7 +51,7 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) pwm_init_state(ctx->pwm, &state); period = ctx->pwm->args.period; state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM); - state.enabled = pwm ? true : false; + state.enabled = true; ret = pwm_apply_state(ctx->pwm, &state); if (!ret) @@ -244,8 +244,7 @@ static int pwm_fan_probe(struct platform_device *pdev) ctx, pwm_fan_groups); if (IS_ERR(hwmon)) { dev_err(&pdev->dev, "Failed to register hwmon device\n"); - ret = PTR_ERR(hwmon); - goto err_pwm_disable; + return PTR_ERR(hwmon); } ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx); @@ -260,20 +259,13 @@ static int pwm_fan_probe(struct platform_device *pdev) if (IS_ERR(cdev)) { dev_err(&pdev->dev, "Failed to register pwm-fan as cooling device"); - ret = PTR_ERR(cdev); - goto err_pwm_disable; + return PTR_ERR(cdev); } ctx->cdev = cdev; thermal_cdev_update(cdev); } return 0; - -err_pwm_disable: - state.enabled = false; - pwm_apply_state(ctx->pwm, &state); - - return ret; } static int pwm_fan_remove(struct platform_device *pdev) diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c index df80c89ebe7f..755e13721a16 100644 --- a/drivers/leds/leds-pwm.c +++ b/drivers/leds/leds-pwm.c @@ -40,11 +40,6 @@ static void __led_pwm_set(struct led_pwm_data *led_dat) int new_duty = led_dat->duty; pwm_config(led_dat->pwm, new_duty, led_dat->period); - - if (new_duty == 0) - pwm_disable(led_dat->pwm); - else - pwm_enable(led_dat->pwm); } static int led_pwm_set(struct led_classdev *led_cdev, @@ -105,6 +100,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, return ret; } + pwm_enable(led_data->pwm); + led_data->cdev.brightness_set_blocking = led_pwm_set; /* (This should better make use of pwm_state instead.) diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 1581f6ab1b1f..38f17c8b6364 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -864,6 +864,8 @@ void pwm_put(struct pwm_device *pwm) if (!pwm) return; + pwm_disable(pwm); + mutex_lock(&pwm_lock); if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) { diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c index d7f5f7de030d..61125052ac3d 100644 --- a/drivers/pwm/pwm-lpc18xx-sct.c +++ b/drivers/pwm/pwm-lpc18xx-sct.c @@ -306,8 +306,6 @@ static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip); struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm); - pwm_disable(pwm); - pwm_set_duty_cycle(pwm, 0); clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map); } diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c index 2030a6b77a09..293abb823fde 100644 --- a/drivers/video/backlight/lm3630a_bl.c +++ b/drivers/video/backlight/lm3630a_bl.c @@ -165,12 +165,15 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max) { unsigned int period = pchip->pdata->pwm_period; unsigned int duty = br * period / br_max; + struct pwm_state state; - pwm_config(pchip->pwmd, duty, period); - if (duty) - pwm_enable(pchip->pwmd); - else - pwm_disable(pchip->pwmd); + pwm_get_state(pchip->pwmd); + + state.duty_cycle = duty; + state.period = period; + state.enabled = true; + + pwm_apply_state(pchip->pwmd, &state); } /* update and get brightness */ diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c index 73612485ed07..ba4e71b99c45 100644 --- a/drivers/video/backlight/lp855x_bl.c +++ b/drivers/video/backlight/lp855x_bl.c @@ -240,6 +240,7 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br) unsigned int period = lp->pdata->period_ns; unsigned int duty = br * period / max_br; struct pwm_device *pwm; + struct pwm_state state; /* request pwm device with the consumer name */ if (!lp->pwm) { @@ -248,19 +249,15 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br) return; lp->pwm = pwm; - - /* - * FIXME: pwm_apply_args() should be removed when switching to - * the atomic PWM API. - */ - pwm_apply_args(pwm); } - pwm_config(lp->pwm, duty, period); - if (duty) - pwm_enable(lp->pwm); - else - pwm_disable(lp->pwm); + pwm_get_state(lp->pwm, &state); + + state.duty = duty; + state.period = period; + state.enabled = true; + + pwm_apply_state(lp->pwm, &state); } static int lp855x_bl_update_status(struct backlight_device *bl) diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 44ac5bde4e9d..e60831f3e29a 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -46,18 +46,42 @@ struct pwm_bl_data { void (*exit)(struct device *); }; +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) +{ + unsigned int lth = pb->lth_brightness; + u64 duty_cycle; + + if (pb->levels) + duty_cycle = pb->levels[brightness]; + else + duty_cycle = brightness; + + duty_cycle *= pb->period - lth; + do_div(duty_cycle, pb->scale); + + return duty_cycle + lth; +} + static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness) { int err; + int duty_cycle; + struct pwm_state state; if (pb->enabled) return; + duty_cycle = compute_duty_cycle(pb, brightness); + err = regulator_enable(pb->power_supply); if (err < 0) dev_err(pb->dev, "failed to enable power supply\n"); - pwm_enable(pb->pwm); + pwm_get_state(pb->pwm, &state); + state.duty_cycle = duty_cycle; + state.period = pb->period; + state.enabled = true; + pwm_apply_state(pb->pwm, &state); if (pb->post_pwm_on_delay) msleep(pb->post_pwm_on_delay); @@ -80,33 +104,15 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb) msleep(pb->pwm_off_delay); pwm_config(pb->pwm, 0, pb->period); - pwm_disable(pb->pwm); regulator_disable(pb->power_supply); pb->enabled = false; } -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) -{ - unsigned int lth = pb->lth_brightness; - u64 duty_cycle; - - if (pb->levels) - duty_cycle = pb->levels[brightness]; - else - duty_cycle = brightness; - - duty_cycle *= pb->period - lth; - do_div(duty_cycle, pb->scale); - - return duty_cycle + lth; -} - static int pwm_backlight_update_status(struct backlight_device *bl) { struct pwm_bl_data *pb = bl_get_data(bl); int brightness = bl->props.brightness; - int duty_cycle; if (bl->props.power != FB_BLANK_UNBLANK || bl->props.fb_blank != FB_BLANK_UNBLANK || @@ -116,11 +122,9 @@ static int pwm_backlight_update_status(struct backlight_device *bl) if (pb->notify) brightness = pb->notify(pb->dev, brightness); - if (brightness > 0) { - duty_cycle = compute_duty_cycle(pb, brightness); - pwm_config(pb->pwm, duty_cycle, pb->period); + if (brightness > 0) pwm_backlight_power_on(pb, brightness); - } else + else pwm_backlight_power_off(pb); if (pb->notify_after) @@ -353,12 +357,6 @@ static int pwm_backlight_probe(struct platform_device *pdev) dev_dbg(&pdev->dev, "got pwm for backlight\n"); - /* - * FIXME: pwm_apply_args() should be removed when switching to - * the atomic PWM API. - */ - pwm_apply_args(pb->pwm); - /* * The DT case will set the pwm_period_ns field to 0 and store the * period, parsed from the DT, in the PWM device. For the non-DT case,
Hello Thierry, (If you have a déjà vu: Yes, I tried to argue this case already in the past, it's just that I'm just faced with a new use case that's broken with the current state.) Currently the idiom pwm_config(mypwm, value, period); if (!value) pwm_disable(mypwm); else pwm_enable(mypwm); (and it's equivalent: state.duty_cycle = ... state.enable = state.duty_cycle ? true : false; pwm_apply_state(...); ) is quite present in the kernel. In my eyes this is bad for two reasons: a) if the caller is supposed to call pwm_disable() after configuring the duty cycle to zero, then why doesn't pwm_config() contains this to unburden pwm-API users and so reduce open coding this assumption? b) it is actually wrong in at least two cases: - On i.MX28 pwm_config(mypwm, 0, period) only programs a register and only after the current period is elapsed this is clocked into the hardware. So it is very likely that when pwm_disable() is called the period is still not over, and then the clk is disabled and the period never ends resulting in the pwm pin being frozen in whatever state it happend to be when the clk was stopped. - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period) correctly ensures the PWM pin to stay at 1. But then pwm_disable() puts it to 0. In the current case this implies that the backlight is fully enabled instead of off. Both issues are hard (or at least ugly) to fix. The IMHO most sane approach is to not let PWM-API-users care about power saving when they still care about the output level and update the samantic of pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop toggling, I don't care about the level of the output." in contrast to the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was called before, stop the pin at (maybe inverted) low level (for most pwms at least).". This is in my eyes more clear, removes code duplication and power saving can (and should) be implemented in the pwm backend drivers that have the domain specific knowledge about the effects of pwm_disable and eventually unwanted side effects when duty cycle is 0 and just do the corresponding action if its safe. We could even put something like: if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0) pwm_disable(pwm); in pwm_config to help backend driver authors of "sane" PWMs. I grepped in the kernel for callers of pwm_disable and adapted them for this. I didn't verify all changes are correct, so take with a grain of salt. The only fishy one is drivers/gpu/drm/i915/intel_panel.c in my eyes, I'm not sure about drivers/pwm/pwm-lpc18xx-sct.c. See below for my wip changes. Looking forward to your comments. Best regards Uwe