diff mbox series

[v2] pwm: Fixes dpm_run_callback() error in pwm_apply_state()

Message ID 20220819043459.32584-1-m.shams@samsung.com
State Deferred
Headers show
Series [v2] pwm: Fixes dpm_run_callback() error in pwm_apply_state() | expand

Commit Message

Tamseel Shams Aug. 19, 2022, 4:34 a.m. UTC
Return invalid argument error from pwm_apply_state()
call when 'period is not set' or 'duty_cycle is greater
than period' only when PWM is enabled, so as to fix the
dpm_run_callback() error seen on exynos SoC during
Suspend

There may be situation when PWM is exported using sysfs,
but at that point period is not set for PWM. At this
point if we do suspend, then during pwm_apply_state
function call from pwm_class_suspend, it checks whether
period is set or not. It is not set now, so it returns
an invalid argument error which issues dpm_run_callback()
error

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Tamseel Shams <m.shams@samsung.com>
---
 drivers/pwm/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Uwe Kleine-König Aug. 19, 2022, 8:07 a.m. UTC | #1
On Fri, Aug 19, 2022 at 10:04:59AM +0530, Tamseel Shams wrote:
> Return invalid argument error from pwm_apply_state()
> call when 'period is not set' or 'duty_cycle is greater
> than period' only when PWM is enabled, so as to fix the
> dpm_run_callback() error seen on exynos SoC during
> Suspend
> 
> There may be situation when PWM is exported using sysfs,
> but at that point period is not set for PWM. At this
> point if we do suspend, then during pwm_apply_state
> function call from pwm_class_suspend, it checks whether
> period is set or not. It is not set now, so it returns
> an invalid argument error which issues dpm_run_callback()
> error
> 
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Tamseel Shams <m.shams@samsung.com>

I still consider this a band aid and I think there is need for prudence
here. Did you verify that all lowlevel drivers handle a state that is
now allowed in a sane way? If you did, you missed at least
pwm-bcm2835.c, I guess there are more but I stopped checking.

So while this change might make sense in the future, I think it's wrong
to do it now.

I stand to the request to find out why pwm->state is strange. Maybe you
just have to fix your .get_state() callback.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index c7552df32082..10b4e39aaac0 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -634,8 +634,8 @@  int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 	 */
 	might_sleep();
 
-	if (!pwm || !state || !state->period ||
-	    state->duty_cycle > state->period)
+	if (!pwm || !state || (state->enabled && (!state->period ||
+	    state->duty_cycle > state->period)))
 		return -EINVAL;
 
 	chip = pwm->chip;