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: >> On 10/19/23 12:51, Uwe Kleine-König wrote: >>> On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote: >>>> On 10/17/23 11:17, Sean Young wrote: >>>>> 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@xxxxxxxx> >>>> >>>> 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@xxxxxxxxxxxxxx >>> 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 > >>> 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. > >>> 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 <hdegoede@xxxxxxxxxx> > > Thanks, > > Sean >
