Hello Sean,
On Fri, Sep 29, 2023 at 05:20:09PM +0100, Sean Young wrote:
> On Fri, Sep 29, 2023 at 11:49:52AM +0300, Ivaylo Dimitrov wrote:
> > On 26.09.23 г. 23:18 ч., Sean Young wrote:
> > > I've never known of a solution to the pwm-ir-tx driver. If using hrtimers
> > > directly improves the situation even a bit, then that would be great.
> >
> > The issue with hrtimers is that we cannot use them directly, as
> > pwm_apply_state() may sleep, but hrtimer function is called in atomic
> > context.
>
> I've also been looking at this problem and came to same conclusion: the
> fact that pwm_apply_state() sleeps is a huge problem.
>
> 1) The vast majority of pwm drivers don't sleep, or could even be converted
> to spinlocks (e.g pwm-sifive.c could use spinlocks, as far as I can see).
>
> 2) Sure, some pwm devices are on i2c busses, so the driver needs to sleep.
> Those devices aren't great for what we're trying to do here, since the
> sleeping may cause delays and affect the generated signal.
>
> What would be ideal here is to have pwm-ir-tx work in atomic context if
> a non-sleeping pwm device is used, and another (non-optimal) code path
> for sleeping pwm drivers. We could even just refuse to run on sleeping pwm
> drivers.
>
> Uwe what do you think of this idea? The pwm api could have a
> bool pwm_may_sleep(struct pwm *pwm) function,
It's certainly possible. The idea of introducing the might_sleep() was
to catch atomic users and if some appear to be able to evaluate if
something needs to be done. See commit 4ad91a227817 ("pwm: Make it
explicit that pwm_apply_state() might sleep").
It complicates things concerning my last bigger pwm series, see
https://lore.kernel.org/linux-pwm/20230808171931.944154-102-u.kleine-koenig@xxxxxxxxxxxxxx/
which introduces a mutex_lock() in pwm_apply_state(). Hmm.
> and pwm_apply_state() does
> not contain might_sleep() - only the driver-specific apply calls might_sleep().
I'd replace the might_sleep() by something like
might_sleep_if(pwm_may_sleep(pwm)); but that's an implementation detail.
> It would be nice if this could all be done at compile time through e.g. a
> device tree attribute.
I wouldn't have something like "linux,slow-pwm" or similar in the device
tree, and I'd expect the dt maintainers to shoot down something like
that, too. What is the problem with a pwm_can_sleep() function only?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
