On Thu, May 12, 2016 at 01:49:12PM +0200, Guillermo Rodriguez Garcia wrote: > Hello, > > [...] > >>> One thing that I'd request is that instead of the cpu_relax() you use a > >>> usleep_range() within the loop instead. I assume it can potentially take > >>> a long time for the current period to finish, so busy looping isn't such > >>> a great idea. You could possibly use the current period_ns to derive a > >>> meaningful value to pass to usleep_range(). > >> > >> I am not sure yet but I believe disabling does not really need to wait for > >> the > >> current period to finish (at least the datasheets do not mention this > >> anywhere). > >> I think that the after writing to PWM_DIS, the actual disable operation is > >> initiated immediately in the PWM subsystem, but is executed asynchronously > >> and requires the pwm_clk to complete. If this assumption is correct, > >> perhaps > >> it is enough to do one single read from PWM_SR so that the disable > >> operation > >> has had the chance to propagate. This is again assuming that all operations > >> are executed sequentially within the PWM subsystem. > >> > >> If the above is correct, then we would not need a loop at all. > > > > I was wrong. The required delay indeed seems to depend on the current PWM > > frequency, suggesting that indeed disabling does not take effect until > > the current > > period is finished. > > > > I will prepare a patch using usleep_range instead of cpu_relax. > > I have found a problem while preparing this. If I use usleep_range I > keep running > into "BUG: scheduling while atomic". This is because I am using the PWM to > drive a buzzer with pwm-beeper, and pwm-beeper currently crashes if the PWM > driver sleeps. Apparently this patch is needed: > > https://lkml.org/lkml/2016/2/22/757 > > However this has not been merged yet. > > How should I proceed ?
The PWM API really shouldn't be used within atomic contexts. There was a change recently that marked all of the PWM devices as "might sleep". The reason for the change was that we introduced a mutex in pwm_enable() and hence every user would have to deal with this eventually. That mutex has since been removed again, but the fact remains that users shouldn't assume that a PWM can be used in atomic context, because the PWM chip could equally well be behind a slow bus such as I2C and hence sleep for every register access. So the correct thing to do would be to follow what leds-pwm did and implement a workqueue. Also might as well make it the only code path as Dmitry suggested in the linked thread, I don't see any point in any kind of fast path here. Thierry
signature.asc
Description: PGP signature
