Re: [PATCH v5 2/2] media: rc: remove ir-rx51 in favour of generic pwm-ir-tx

[Uwe Kleine-König]

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