Hi Sean,

On 10/22/23 12:46, Sean Young wrote:

Hi Hans,

On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:

quoted

On 10/19/23 12:51, Uwe Kleine-König wrote:

quoted

On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:

quoted

On 10/17/23 11:17, Sean Young wrote:

quoted

Some drivers require sleeping, for example if the pwm device is connected
over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc
with the generated IR signal when sleeping occurs.

This patch makes it possible to use pwm when the driver does not sleep,
by introducing the pwm_can_sleep() function.

Signed-off-by: Sean Young <sean@mess.org>

I have no objection to this patch by itself, but it seems a bit
of unnecessary churn to change all current callers of pwm_apply_state()
to a new API.

The idea is to improve the semantic of the function name, see
https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rlymzz@pengutronix.de (local)
for more context.

Hmm, so the argument here is that the GPIO API has this, but GPIOs
generally speaking can be set atomically, so there not being able
to set it atomically is special.

OTOH we have many many many other kernel functions which may sleep
and we don't all postfix them with _can_sleep.

And for PWM controllers pwm_apply_state is IMHO sorta expected to
sleep. Many of these are attached over I2C so things will sleep,
others have a handshake to wait for the current dutycycle to
end before you can apply a second change on top of an earlier
change during the current dutycycle which often also involves
sleeping.

So the natural/expeected thing for pwm_apply_state() is to sleep
and thus it does not need a postfix for this IMHO.

Most pwm drivers look like they can be made to work in atomic context,
I think. Like you say this is not the case for all of them. Whatever
we choose to be the default for pwm_apply_state(), we should have a
clear function name for the alternative. This is essentially why
pam_apply_cansleep() was picked.

The alternative to pwm_apply_cansleep() is to have a function name
which implies it can be used from atomic context. However, 
pwm_apply_atomic() is not great because the "atomic" could be
confused with the PWM atomic API, not the kernel process/atomic
context.

Well pwm_apply_state() is the atomic PWM interface right?

So to me pwm_apply_state_atomic() would be clearly about
running atomically.

So what should the non-sleeping function be called then? 
 - pwm_apply_cannotsleep() 
 - pwm_apply_nosleep()
 - pwm_apply_nonsleeping()
 - pwm_apply_atomic_context()

I would just go with:

pwm_apply_state_atomic()

but if this is disliked by others then lets just rename

pwm_apply_state() to pwm_apply_state_cansleep() as
is done in this patch and use plain pwm_apply_state()
for the new atomic-context version.

Regards,

Hans

quoted

quoted

I think it's very subjective if you consider this
churn or not.

I consider it churn because I don't think adding a postfix
for what is the default/expected behavior is a good idea
(with GPIOs not sleeping is the expected behavior).

I agree that this is very subjective and very much goes
into the territory of bikeshedding. So please consider
the above my 2 cents on this and lets leave it at that.

You have a valid point. Let's focus on having descriptive function names.

quoted

quoted

While it's nice to have every caller converted in a single
step, I'd go for

	#define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state)

, keep that macro for a while and convert all users step by step. This
way we don't needlessly break oot code and the changes to convert to the
new API can go via their usual trees without time pressure.

I don't think there are enough users of pwm_apply_state() to warrant
such an exercise.

So if people want to move ahead with the _can_sleep postfix addition
(still not a fan) here is my acked-by for the drivers/platform/x86
changes, for merging this through the PWM tree in a single commit:

Acked-by: Hans de Goede <redacted>

Thanks,

Sean