Message-ID: <ZTT9fvEF+lqfzGJ/@gofer.mess.org>
Date:   Sun, 22 Oct 2023 11:46:22 +0100
From:   Sean Young <sean@...s.org>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, linux-media@...r.kernel.org,
        linux-pwm@...r.kernel.org,
        Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Corbet <corbet@....net>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Javier Martinez Canillas <javierm@...hat.com>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Support Opensource <support.opensource@...semi.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Mark Gross <markgross@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Helge Deller <deller@....de>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
        dri-devel@...ts.freedesktop.org, linux-hwmon@...r.kernel.org,
        linux-input@...r.kernel.org, linux-leds@...r.kernel.org,
        platform-driver-x86@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in
 atomic context

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@...s.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
> > 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.

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

> > 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@...hat.com>

Thanks,

Sean

Powered by blists - more mailing lists