Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
Hello, On Wed, Oct 25, 2023 at 10:53:27AM +0100, Sean Young wrote: > On Mon, Oct 23, 2023 at 02:34:17PM +0100, Daniel Thompson wrote: > > On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote: > > > 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: > > > > > 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. > > > > For a couple of days I've been trying to resist the bikeshedding (esp. > > given the changes to backlight are tiny) so I'll try to keep it as > > brief as I can: > > > > 1. I dislike the do_it() and do_it_cansleep() pairing. It is > >difficult to detect when a client driver calls do_it() by mistake. > >In fact a latent bug of this nature can only be detected by runtime > >testing with the small number of PWMs that do not support > >configuration from an atomic context. > > > >In contrast do_it() and do_it_atomic()[*] means that although > >incorrectly calling do_it() from an atomic context can be pretty > >catastrophic it is also trivially detected (with any PWM driver) > >simply by running with CONFIG_DEBUG_ATOMIC_SLEEP. Wrongly calling the atomic variant (no matter how it's named) in a context where sleeping is possible is only a minor issue. Being faster than necessary is hardly a problem, so it only hurts by not being an preemption point with PREEMPT_VOLUNTARY which might not even be relevant because we're near to a system call anyhow. For me the naming is only very loosely related to the possible bugs. I think calling the wrong function happens mainly because the driver author isn't aware in which context the call happens and not because of wrong assumptions about the sleepiness of a certain function call. If you consider this an argument however, do_it + do_it_cansleep is better than do_it_atomic + do_it as wrongly assuming do_it would sleep is less bad than wrongly assuming do_it wouldn't sleep. (The latter is catched by CONFIG_DEBUG_ATOMIC_SLEEP, but only if it's enabled.) Having said that while my subjective preference ordering is (with first = best): do_it + do_it_cansleep do_it_atomic + do_it_cansleep do_it_atomic + do_it wi(th a _might_sleep or _mightsleep suffix ranging below _cansleep) I wouldn't get sleepless nights when I get overruled here (uwe_cansleep :-). > >No objections (beyond churn) to fully spelt out pairings such as > >do_it_cansleep() and do_it_atomic()[*]! > > I must say I do like the look of this. Uwe, how do you feel about: > pwm_apply_cansleep() and pwm_apply_atomic()? I know we've talked about > pwm_apply_atomic in the past, however I think this this the best > option I've seen so far. > > > 2. If there is an API rename can we make sure the patch contains no > >other changes (e.g. don't introduce any new API in the same patch). > >Seperating renames makes the patches easier to review! > >It makes each one smaller and easier to review! > > Yes, this should have been separated out. Will fix for next version. +1 Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
On Mon, Oct 23, 2023 at 02:34:17PM +0100, Daniel Thompson wrote: > On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote: > > 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: > > > > 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. > > For a couple of days I've been trying to resist the bikeshedding (esp. > given the changes to backlight are tiny) so I'll try to keep it as > brief as I can: > > 1. I dislike the do_it() and do_it_cansleep() pairing. It is >difficult to detect when a client driver calls do_it() by mistake. >In fact a latent bug of this nature can only be detected by runtime >testing with the small number of PWMs that do not support >configuration from an atomic context. > >In contrast do_it() and do_it_atomic()[*] means that although >incorrectly calling do_it() from an atomic context can be pretty >catastrophic it is also trivially detected (with any PWM driver) >simply by running with CONFIG_DEBUG_ATOMIC_SLEEP. > >No objections (beyond churn) to fully spelt out pairings such as >do_it_cansleep() and do_it_atomic()[*]! I must say I do like the look of this. Uwe, how do you feel about: pwm_apply_cansleep() and pwm_apply_atomic()? I know we've talked about pwm_apply_atomic in the past, however I think this this the best option I've seen so far. > 2. If there is an API rename can we make sure the patch contains no >other changes (e.g. don't introduce any new API in the same patch). >Seperating renames makes the patches easier to review! >It makes each one smaller and easier to review! Yes, this should have been separated out. Will fix for next version. Thanks, Sean
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
On Sun, Oct 22, 2023 at 11:46:22AM +0100, 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: > > > 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. For a couple of days I've been trying to resist the bikeshedding (esp. given the changes to backlight are tiny) so I'll try to keep it as brief as I can: 1. I dislike the do_it() and do_it_cansleep() pairing. It is difficult to detect when a client driver calls do_it() by mistake. In fact a latent bug of this nature can only be detected by runtime testing with the small number of PWMs that do not support configuration from an atomic context. In contrast do_it() and do_it_atomic()[*] means that although incorrectly calling do_it() from an atomic context can be pretty catastrophic it is also trivially detected (with any PWM driver) simply by running with CONFIG_DEBUG_ATOMIC_SLEEP. No objections (beyond churn) to fully spelt out pairings such as do_it_cansleep() and do_it_atomic()[*]! 2. If there is an API rename can we make sure the patch contains no other changes (e.g. don't introduce any new API in the same patch). Seperating renames makes the patches easier to review! It makes each one smaller and easier to review! Daniel. [*] or do_it_nosleep()... etc.
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
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 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.mcdmklbsz2rly...@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. 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 > > Thanks, > > Sean >
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
On Tue, 17 Oct 2023, Sean Young wrote: > diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c > b/drivers/gpu/drm/i915/display/intel_backlight.c > index 2e8f17c045222..cf516190cde8f 100644 > --- a/drivers/gpu/drm/i915/display/intel_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_backlight.c > @@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct > drm_connector_state *conn_state, > struct intel_panel *panel = > _intel_connector(conn_state->connector)->panel; > > pwm_set_relative_duty_cycle(>backlight.pwm_state, level, 100); > - pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state); > + pwm_apply_cansleep(panel->backlight.pwm, >backlight.pwm_state); > } > > static void > @@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct > drm_connector_state *old_conn > intel_backlight_set_pwm_level(old_conn_state, level); > > panel->backlight.pwm_state.enabled = false; > - pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state); > + pwm_apply_cansleep(panel->backlight.pwm, >backlight.pwm_state); > } > > void intel_backlight_disable(const struct drm_connector_state > *old_conn_state) > @@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct > intel_crtc_state *crtc_state, > > pwm_set_relative_duty_cycle(>backlight.pwm_state, level, 100); > panel->backlight.pwm_state.enabled = true; > - pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state); > + pwm_apply_cansleep(panel->backlight.pwm, >backlight.pwm_state); > } > > static void __intel_backlight_enable(const struct intel_crtc_state > *crtc_state, The i915 parts are Acked-by: Jani Nikula for merging via whichever tree you find most convenient, and with whatever naming you end up with. -- Jani Nikula, Intel
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 > >> > >> 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.mcdmklbsz2rly...@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 Thanks, Sean
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
Hi Uwe, 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: >> Hi Sean, >> >> 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 >> >> 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.mcdmklbsz2rly...@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. > 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. > 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 Regards, Hans
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote: > Hi Sean, > > 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 > > 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.mcdmklbsz2rly...@pengutronix.de for more context. I think it's very subjective if you consider this churn or not. 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. > Why not just keep pwm_apply_state() as is and introduce a new > pwm_apply_state_atomic() for callers which want to apply state > in a case where sleeping is not allowed ? There is a big spontaneous growth of function name patterns. I didn't claim having done a complete research, but not using a suffix for the fast variant and _cansleep for the sleeping one at least aligns to how the gpio subsystem names things. Grepping a bit more: - regmap has: regmap_might_sleep() and struct regmap::can_sleep The actual API doesn't have different functions to differentiate sleeping and non-sleeping calls. (Though there is regmap_read_poll_timeout_atomic().) - kmap() + kmap_atomic() - set_pte() + set_pte_atomic() - There is scmi_is_transport_atomic() and scmi_handle::is_transport_atomic() (Is this also about sleeping?) - There are quite a lot more symbols ending in _atomic and in _cansleep, but several of them don't exists to differentiate a slow and a fast procedure. (e.g. drm_mode_atomic) Not entirely sure what to read out of that. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
Hi Sean,
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
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.
Why not just keep pwm_apply_state() as is and introduce a new
pwm_apply_state_atomic() for callers which want to apply state
in a case where sleeping is not allowed ?
Regards,
Hans
> ---
> Documentation/driver-api/pwm.rst | 16 +++-
> .../gpu/drm/i915/display/intel_backlight.c| 6 +-
> drivers/gpu/drm/solomon/ssd130x.c | 2 +-
> drivers/hwmon/pwm-fan.c | 8 +-
> drivers/input/misc/da7280.c | 4 +-
> drivers/input/misc/pwm-beeper.c | 4 +-
> drivers/input/misc/pwm-vibra.c| 8 +-
> drivers/leds/leds-pwm.c | 2 +-
> drivers/leds/rgb/leds-pwm-multicolor.c| 4 +-
> drivers/media/rc/pwm-ir-tx.c | 4 +-
> drivers/platform/x86/lenovo-yogabook.c| 2 +-
> drivers/pwm/core.c| 75 ++-
> drivers/pwm/pwm-renesas-tpu.c | 1 -
> drivers/pwm/pwm-twl-led.c | 2 +-
> drivers/pwm/pwm-vt8500.c | 2 +-
> drivers/pwm/sysfs.c | 10 +--
> drivers/regulator/pwm-regulator.c | 4 +-
> drivers/video/backlight/lm3630a_bl.c | 2 +-
> drivers/video/backlight/lp855x_bl.c | 2 +-
> drivers/video/backlight/pwm_bl.c | 6 +-
> drivers/video/fbdev/ssd1307fb.c | 2 +-
> include/linux/pwm.h | 57 ++
> 22 files changed, 147 insertions(+), 76 deletions(-)
>
> diff --git a/Documentation/driver-api/pwm.rst
> b/Documentation/driver-api/pwm.rst
> index 3fdc95f7a1d15..a2fb5f8f6e1f8 100644
> --- a/Documentation/driver-api/pwm.rst
> +++ b/Documentation/driver-api/pwm.rst
> @@ -41,7 +41,15 @@ the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also
> exist.
>
> After being requested, a PWM has to be configured using::
>
> - int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state);
> + int pwm_apply_cansleep(struct pwm_device *pwm, struct pwm_state *state);
> +
> +If the PWM support atomic mode, which can be determined with::
> +
> +bool pwm_is_atomic(struct pwm_device *pwm);
> +
> +Then the PWM can be configured with::
> +
> + int pwm_apply(struct pwm_device *pwm, struct pwm_state *state);
>
> This API controls both the PWM period/duty_cycle config and the
> enable/disable state.
> @@ -57,13 +65,13 @@ If supported by the driver, the signal can be optimized,
> for example to improve
> EMI by phase shifting the individual channels of a chip.
>
> The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers
> -around pwm_apply_state() and should not be used if the user wants to change
> +around pwm_apply_cansleep() and should not be used if the user wants to
> change
> several parameter at once. For example, if you see pwm_config() and
> pwm_{enable,disable}() calls in the same function, this probably means you
> -should switch to pwm_apply_state().
> +should switch to pwm_apply_cansleep().
>
> The PWM user API also allows one to query the PWM state that was passed to
> the
> -last invocation of pwm_apply_state() using pwm_get_state(). Note this is
> +last invocation of pwm_apply_cansleep() using pwm_get_state(). Note this is
> different to what the driver has actually implemented if the request cannot
> be
> satisfied exactly with the hardware in use. There is currently no way for
> consumers to get the actually implemented settings.
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c
> b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 2e8f17c045222..cf516190cde8f 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct
> drm_connector_state *conn_state,
> struct intel_panel *panel =
> _intel_connector(conn_state->connector)->panel;
>
> pwm_set_relative_duty_cycle(>backlight.pwm_state, level, 100);
> - pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state);
> + pwm_apply_cansleep(panel->backlight.pwm, >backlight.pwm_state);
> }
>
> static void
> @@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct
> drm_connector_state *old_conn
> intel_backlight_set_pwm_level(old_conn_state, level);
>
> panel->backlight.pwm_state.enabled =
[PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
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
---
Documentation/driver-api/pwm.rst | 16 +++-
.../gpu/drm/i915/display/intel_backlight.c| 6 +-
drivers/gpu/drm/solomon/ssd130x.c | 2 +-
drivers/hwmon/pwm-fan.c | 8 +-
drivers/input/misc/da7280.c | 4 +-
drivers/input/misc/pwm-beeper.c | 4 +-
drivers/input/misc/pwm-vibra.c| 8 +-
drivers/leds/leds-pwm.c | 2 +-
drivers/leds/rgb/leds-pwm-multicolor.c| 4 +-
drivers/media/rc/pwm-ir-tx.c | 4 +-
drivers/platform/x86/lenovo-yogabook.c| 2 +-
drivers/pwm/core.c| 75 ++-
drivers/pwm/pwm-renesas-tpu.c | 1 -
drivers/pwm/pwm-twl-led.c | 2 +-
drivers/pwm/pwm-vt8500.c | 2 +-
drivers/pwm/sysfs.c | 10 +--
drivers/regulator/pwm-regulator.c | 4 +-
drivers/video/backlight/lm3630a_bl.c | 2 +-
drivers/video/backlight/lp855x_bl.c | 2 +-
drivers/video/backlight/pwm_bl.c | 6 +-
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 57 ++
22 files changed, 147 insertions(+), 76 deletions(-)
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index 3fdc95f7a1d15..a2fb5f8f6e1f8 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -41,7 +41,15 @@ the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also
exist.
After being requested, a PWM has to be configured using::
- int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state);
+ int pwm_apply_cansleep(struct pwm_device *pwm, struct pwm_state *state);
+
+If the PWM support atomic mode, which can be determined with::
+
+bool pwm_is_atomic(struct pwm_device *pwm);
+
+Then the PWM can be configured with::
+
+ int pwm_apply(struct pwm_device *pwm, struct pwm_state *state);
This API controls both the PWM period/duty_cycle config and the
enable/disable state.
@@ -57,13 +65,13 @@ If supported by the driver, the signal can be optimized,
for example to improve
EMI by phase shifting the individual channels of a chip.
The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers
-around pwm_apply_state() and should not be used if the user wants to change
+around pwm_apply_cansleep() and should not be used if the user wants to change
several parameter at once. For example, if you see pwm_config() and
pwm_{enable,disable}() calls in the same function, this probably means you
-should switch to pwm_apply_state().
+should switch to pwm_apply_cansleep().
The PWM user API also allows one to query the PWM state that was passed to the
-last invocation of pwm_apply_state() using pwm_get_state(). Note this is
+last invocation of pwm_apply_cansleep() using pwm_get_state(). Note this is
different to what the driver has actually implemented if the request cannot be
satisfied exactly with the hardware in use. There is currently no way for
consumers to get the actually implemented settings.
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c
b/drivers/gpu/drm/i915/display/intel_backlight.c
index 2e8f17c045222..cf516190cde8f 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct
drm_connector_state *conn_state,
struct intel_panel *panel =
_intel_connector(conn_state->connector)->panel;
pwm_set_relative_duty_cycle(>backlight.pwm_state, level, 100);
- pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state);
+ pwm_apply_cansleep(panel->backlight.pwm, >backlight.pwm_state);
}
static void
@@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct
drm_connector_state *old_conn
intel_backlight_set_pwm_level(old_conn_state, level);
panel->backlight.pwm_state.enabled = false;
- pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state);
+ pwm_apply_cansleep(panel->backlight.pwm, >backlight.pwm_state);
}
void intel_backlight_disable(const struct drm_connector_state *old_conn_state)
@@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct
intel_crtc_state *crtc_state,
pwm_set_relative_duty_cycle(>backlight.pwm_state, level, 100);
panel->backlight.pwm_state.enabled = true;
- pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state);
+
