[PATCH] Input: max77693 - Convert to atomic pwm operation

[PATCH] Input: max77693 - Convert to atomic pwm operation

[Uwe Kleine-König]

The driver called pwm_config() and pwm_enable() separately. Today both
are wrappers for pwm_apply_might_sleep() and it's more effective to call
this function directly and only once. Also don't configure the
duty_cycle and period if the next operation is to disable the PWM so
configure the PWM in max77693_haptic_enable().

With the direct use of pwm_apply_might_sleep() the need to call
pwm_apply_args() in .probe() is now gone, too, so drop this one.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
Hello,

the motivation for this patch is getting rid of pwm_config(),
pwm_enable() and pwm_apply_args(). I plan to remove these once all
callers are fixed to use pwm_apply_might_sleep().

Best regards
Uwe

 drivers/input/misc/max77693-haptic.c | 41 ++++++----------------------
 1 file changed, 8 insertions(+), 33 deletions(-)

diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c
index 1dfd7b95a4ce..ecb3e8d541c3 100644
--- a/drivers/input/misc/max77693-haptic.c
+++ b/drivers/input/misc/max77693-haptic.c
@@ -66,23 +66,6 @@ struct max77693_haptic {
 	struct work_struct work;
 };
 
-static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic)
-{
-	struct pwm_args pargs;
-	int delta;
-	int error;
-
-	pwm_get_args(haptic->pwm_dev, &pargs);
-	delta = (pargs.period + haptic->pwm_duty) / 2;
-	error = pwm_config(haptic->pwm_dev, delta, pargs.period);
-	if (error) {
-		dev_err(haptic->dev, "failed to configure pwm: %d\n", error);
-		return error;
-	}
-
-	return 0;
-}
-
 static int max77843_haptic_bias(struct max77693_haptic *haptic, bool on)
 {
 	int error;
@@ -167,17 +150,22 @@ static int max77693_haptic_lowsys(struct max77693_haptic *haptic, bool enable)
 static void max77693_haptic_enable(struct max77693_haptic *haptic)
 {
 	int error;
+	struct pwm_state state;
 
-	if (haptic->enabled)
-		return;
+	pwm_init_state(haptic->pwm_dev, &state);
+	state.duty_cycle = (state.period + haptic->pwm_duty) / 2;
+	state.enabled = true;
 
-	error = pwm_enable(haptic->pwm_dev);
+	error = pwm_apply_might_sleep(haptic->pwm_dev, &state);
 	if (error) {
 		dev_err(haptic->dev,
 			"failed to enable haptic pwm device: %d\n", error);
 		return;
 	}
 
+	if (haptic->enabled)
+		return;
+
 	error = max77693_haptic_lowsys(haptic, true);
 	if (error)
 		goto err_enable_lowsys;
@@ -224,13 +212,6 @@ static void max77693_haptic_play_work(struct work_struct *work)
 {
 	struct max77693_haptic *haptic =
 			container_of(work, struct max77693_haptic, work);
-	int error;
-
-	error = max77693_haptic_set_duty_cycle(haptic);
-	if (error) {
-		dev_err(haptic->dev, "failed to set duty cycle: %d\n", error);
-		return;
-	}
 
 	if (haptic->magnitude)
 		max77693_haptic_enable(haptic);
@@ -340,12 +321,6 @@ static int max77693_haptic_probe(struct platform_device *pdev)
 		return PTR_ERR(haptic->pwm_dev);
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to the
-	 * atomic PWM API.
-	 */
-	pwm_apply_args(haptic->pwm_dev);
-
 	haptic->motor_reg = devm_regulator_get(&pdev->dev, "haptic");
 	if (IS_ERR(haptic->motor_reg)) {
 		dev_err(&pdev->dev, "failed to get regulator\n");

base-commit: 1343433ed38923a21425c602e92120a1f1db5f7a
-- 
2.49.0

Re: [PATCH] Input: max77693 - Convert to atomic pwm operation

[Dmitry Torokhov]

Hi Uwe,

On Mon, Jun 30, 2025 at 12:38:50PM +0200, Uwe Kleine-König wrote:
> @@ -167,17 +150,22 @@ static int max77693_haptic_lowsys(struct max77693_haptic *haptic, bool enable)
>  static void max77693_haptic_enable(struct max77693_haptic *haptic)
>  {
>  	int error;
> +	struct pwm_state state;
>  
> -	if (haptic->enabled)
> -		return;
> +	pwm_init_state(haptic->pwm_dev, &state);
> +	state.duty_cycle = (state.period + haptic->pwm_duty) / 2;
> +	state.enabled = true;
>  
> -	error = pwm_enable(haptic->pwm_dev);
> +	error = pwm_apply_might_sleep(haptic->pwm_dev, &state);
>  	if (error) {
>  		dev_err(haptic->dev,
>  			"failed to enable haptic pwm device: %d\n", error);
>  		return;
>  	}
>  
> +	if (haptic->enabled)
> +		return;
> +
>  	error = max77693_haptic_lowsys(haptic, true);
>  	if (error)
>  		goto err_enable_lowsys;

I do not quite like that max77693_haptic_enable() now has split brain:
there is part of it that does update unconditionally and part that is
protected by the "enabled" flag. How about we keep max77693_haptic_set_duty_cycle() but make max77693_haptic_play_work() 
a bit smarter, like in the version below:


Input: max77693 - convert to atomic pwm operation

From: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

The driver called pwm_config() and pwm_enable() separately. Today both
are wrappers for pwm_apply_might_sleep() and it's more effective to call
this function directly and only once. Also don't configure the
duty_cycle and period if the next operation is to disable the PWM so
configure the PWM in max77693_haptic_enable().

With the direct use of pwm_apply_might_sleep() the need to call
pwm_apply_args() in .probe() is now gone, too, so drop this one.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Link: https://lore.kernel.org/r/20250630103851.2069952-2-u.kleine-koenig@baylibre.com
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/max77693-haptic.c |   41 +++++++++++++++-------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c
index 1dfd7b95a4ce..5d45680d74a4 100644
--- a/drivers/input/misc/max77693-haptic.c
+++ b/drivers/input/misc/max77693-haptic.c
@@ -68,15 +68,16 @@ struct max77693_haptic {
 
 static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic)
 {
-	struct pwm_args pargs;
-	int delta;
+	struct pwm_state state;
 	int error;
 
-	pwm_get_args(haptic->pwm_dev, &pargs);
-	delta = (pargs.period + haptic->pwm_duty) / 2;
-	error = pwm_config(haptic->pwm_dev, delta, pargs.period);
+	pwm_init_state(haptic->pwm_dev, &state);
+	state.duty_cycle = (state.period + haptic->pwm_duty) / 2;
+
+	error = pwm_apply_might_sleep(haptic->pwm_dev, &state);
 	if (error) {
-		dev_err(haptic->dev, "failed to configure pwm: %d\n", error);
+		dev_err(haptic->dev,
+			"failed to set pwm duty cycle: %d\n", error);
 		return error;
 	}
 
@@ -166,12 +167,17 @@ static int max77693_haptic_lowsys(struct max77693_haptic *haptic, bool enable)
 
 static void max77693_haptic_enable(struct max77693_haptic *haptic)
 {
+	struct pwm_state state;
 	int error;
 
 	if (haptic->enabled)
 		return;
 
-	error = pwm_enable(haptic->pwm_dev);
+	pwm_init_state(haptic->pwm_dev, &state);
+	state.duty_cycle = (state.period + haptic->pwm_duty) / 2;
+	state.enabled = true;
+
+	error = pwm_apply_might_sleep(haptic->pwm_dev, &state);
 	if (error) {
 		dev_err(haptic->dev,
 			"failed to enable haptic pwm device: %d\n", error);
@@ -224,18 +230,13 @@ static void max77693_haptic_play_work(struct work_struct *work)
 {
 	struct max77693_haptic *haptic =
 			container_of(work, struct max77693_haptic, work);
-	int error;
-
-	error = max77693_haptic_set_duty_cycle(haptic);
-	if (error) {
-		dev_err(haptic->dev, "failed to set duty cycle: %d\n", error);
-		return;
-	}
 
-	if (haptic->magnitude)
-		max77693_haptic_enable(haptic);
-	else
+	if (!haptic->magnitude)
 		max77693_haptic_disable(haptic);
+	else if (haptic->enabled)
+		max77693_haptic_set_duty_cycle(haptic);
+	else
+		max77693_haptic_enable(haptic);
 }
 
 static int max77693_haptic_play_effect(struct input_dev *dev, void *data,
@@ -340,12 +341,6 @@ static int max77693_haptic_probe(struct platform_device *pdev)
 		return PTR_ERR(haptic->pwm_dev);
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to the
-	 * atomic PWM API.
-	 */
-	pwm_apply_args(haptic->pwm_dev);
-
 	haptic->motor_reg = devm_regulator_get(&pdev->dev, "haptic");
 	if (IS_ERR(haptic->motor_reg)) {
 		dev_err(&pdev->dev, "failed to get regulator\n");

-- 
Dmitry

Re: [PATCH] Input: max77693 - Convert to atomic pwm operation

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5205 bytes --]

On Mon, Jun 30, 2025 at 12:23:43PM -0700, Dmitry Torokhov wrote:
> Hi Uwe,
> 
> On Mon, Jun 30, 2025 at 12:38:50PM +0200, Uwe Kleine-König wrote:
> > @@ -167,17 +150,22 @@ static int max77693_haptic_lowsys(struct max77693_haptic *haptic, bool enable)
> >  static void max77693_haptic_enable(struct max77693_haptic *haptic)
> >  {
> >  	int error;
> > +	struct pwm_state state;
> >  
> > -	if (haptic->enabled)
> > -		return;
> > +	pwm_init_state(haptic->pwm_dev, &state);
> > +	state.duty_cycle = (state.period + haptic->pwm_duty) / 2;
> > +	state.enabled = true;
> >  
> > -	error = pwm_enable(haptic->pwm_dev);
> > +	error = pwm_apply_might_sleep(haptic->pwm_dev, &state);
> >  	if (error) {
> >  		dev_err(haptic->dev,
> >  			"failed to enable haptic pwm device: %d\n", error);
> >  		return;
> >  	}
> >  
> > +	if (haptic->enabled)
> > +		return;
> > +
> >  	error = max77693_haptic_lowsys(haptic, true);
> >  	if (error)
> >  		goto err_enable_lowsys;
> 
> I do not quite like that max77693_haptic_enable() now has split brain:
> there is part of it that does update unconditionally and part that is
> protected by the "enabled" flag. How about we keep max77693_haptic_set_duty_cycle() but make max77693_haptic_play_work() 
> a bit smarter, like in the version below:
> 
> 
> Input: max77693 - convert to atomic pwm operation
> 
> From: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> 
> The driver called pwm_config() and pwm_enable() separately. Today both
> are wrappers for pwm_apply_might_sleep() and it's more effective to call
> this function directly and only once. Also don't configure the
> duty_cycle and period if the next operation is to disable the PWM so
> configure the PWM in max77693_haptic_enable().
> 
> With the direct use of pwm_apply_might_sleep() the need to call
> pwm_apply_args() in .probe() is now gone, too, so drop this one.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Link: https://lore.kernel.org/r/20250630103851.2069952-2-u.kleine-koenig@baylibre.com
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/max77693-haptic.c |   41 +++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c
> index 1dfd7b95a4ce..5d45680d74a4 100644
> --- a/drivers/input/misc/max77693-haptic.c
> +++ b/drivers/input/misc/max77693-haptic.c
> @@ -68,15 +68,16 @@ struct max77693_haptic {
>  
>  static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic)
>  {
> -	struct pwm_args pargs;
> -	int delta;
> +	struct pwm_state state;
>  	int error;
>  
> -	pwm_get_args(haptic->pwm_dev, &pargs);
> -	delta = (pargs.period + haptic->pwm_duty) / 2;
> -	error = pwm_config(haptic->pwm_dev, delta, pargs.period);
> +	pwm_init_state(haptic->pwm_dev, &state);
> +	state.duty_cycle = (state.period + haptic->pwm_duty) / 2;
> +
> +	error = pwm_apply_might_sleep(haptic->pwm_dev, &state);
>  	if (error) {
> -		dev_err(haptic->dev, "failed to configure pwm: %d\n", error);
> +		dev_err(haptic->dev,
> +			"failed to set pwm duty cycle: %d\n", error);
>  		return error;
>  	}
>  
> @@ -166,12 +167,17 @@ static int max77693_haptic_lowsys(struct max77693_haptic *haptic, bool enable)
>  
>  static void max77693_haptic_enable(struct max77693_haptic *haptic)
>  {
> +	struct pwm_state state;
>  	int error;
>  
>  	if (haptic->enabled)
>  		return;
>  
> -	error = pwm_enable(haptic->pwm_dev);
> +	pwm_init_state(haptic->pwm_dev, &state);
> +	state.duty_cycle = (state.period + haptic->pwm_duty) / 2;
> +	state.enabled = true;
> +
> +	error = pwm_apply_might_sleep(haptic->pwm_dev, &state);
>  	if (error) {
>  		dev_err(haptic->dev,
>  			"failed to enable haptic pwm device: %d\n", error);
> @@ -224,18 +230,13 @@ static void max77693_haptic_play_work(struct work_struct *work)
>  {
>  	struct max77693_haptic *haptic =
>  			container_of(work, struct max77693_haptic, work);
> -	int error;
> -
> -	error = max77693_haptic_set_duty_cycle(haptic);
> -	if (error) {
> -		dev_err(haptic->dev, "failed to set duty cycle: %d\n", error);
> -		return;
> -	}
>  
> -	if (haptic->magnitude)
> -		max77693_haptic_enable(haptic);
> -	else
> +	if (!haptic->magnitude)
>  		max77693_haptic_disable(haptic);
> +	else if (haptic->enabled)
> +		max77693_haptic_set_duty_cycle(haptic);
> +	else
> +		max77693_haptic_enable(haptic);
>  }
>  
>  static int max77693_haptic_play_effect(struct input_dev *dev, void *data,

I had something like that at first, but didn't like it. With that
approach you have two places that have to know how to set the PWM's
duty_cycle. Also I think the control flow is more complicated.

I considered renaming max77693_haptic_enable() to something that better
matches what it does in my variant, but max77693_haptic_configure() was
already taken.

But that might all be subjective? If you like your version better,
that's fine, it still gets rid of pwm_config(), pwm_enable() and
pwm_apply_args() which is my main objective.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Input: max77693 - Convert to atomic pwm operation

[Dmitry Torokhov]

On Tue, Jul 01, 2025 at 07:49:22AM +0200, Uwe Kleine-König wrote:
> 
> I had something like that at first, but didn't like it. With that
> approach you have two places that have to know how to set the PWM's
> duty_cycle. Also I think the control flow is more complicated.
> 
> I considered renaming max77693_haptic_enable() to something that better
> matches what it does in my variant, but max77693_haptic_configure() was
> already taken.
> 
> But that might all be subjective? If you like your version better,
> that's fine, it still gets rid of pwm_config(), pwm_enable() and
> pwm_apply_args() which is my main objective.

Yes, I agree that it is subjective. I know that you do not quite like
the version I posted, still will you be OK if it is attributed to you?

Thanks.

-- 
Dmitry

Re: [PATCH] Input: max77693 - Convert to atomic pwm operation

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

On Tue, Jul 01, 2025 at 11:06:50AM -0700, Dmitry Torokhov wrote:
> On Tue, Jul 01, 2025 at 07:49:22AM +0200, Uwe Kleine-König wrote:
> > 
> > I had something like that at first, but didn't like it. With that
> > approach you have two places that have to know how to set the PWM's
> > duty_cycle. Also I think the control flow is more complicated.
> > 
> > I considered renaming max77693_haptic_enable() to something that better
> > matches what it does in my variant, but max77693_haptic_configure() was
> > already taken.
> > 
> > But that might all be subjective? If you like your version better,
> > that's fine, it still gets rid of pwm_config(), pwm_enable() and
> > pwm_apply_args() which is my main objective.
> 
> Yes, I agree that it is subjective. I know that you do not quite like
> the version I posted, still will you be OK if it is attributed to you?

Yes, feel free to apply it as you suggested.

Thanks
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Input: max77693 - Convert to atomic pwm operation

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

Hello Dmitry,

On Wed, Jul 02, 2025 at 08:02:33AM +0200, Uwe Kleine-König wrote:
> On Tue, Jul 01, 2025 at 11:06:50AM -0700, Dmitry Torokhov wrote:
> > On Tue, Jul 01, 2025 at 07:49:22AM +0200, Uwe Kleine-König wrote:
> > > 
> > > I had something like that at first, but didn't like it. With that
> > > approach you have two places that have to know how to set the PWM's
> > > duty_cycle. Also I think the control flow is more complicated.
> > > 
> > > I considered renaming max77693_haptic_enable() to something that better
> > > matches what it does in my variant, but max77693_haptic_configure() was
> > > already taken.
> > > 
> > > But that might all be subjective? If you like your version better,
> > > that's fine, it still gets rid of pwm_config(), pwm_enable() and
> > > pwm_apply_args() which is my main objective.
> > 
> > Yes, I agree that it is subjective. I know that you do not quite like
> > the version I posted, still will you be OK if it is attributed to you?
> 
> Yes, feel free to apply it as you suggested.

As of today's next that didn't happen. Do you have this patch still on
your radar?

This is the last driver making use of pwm_config(), it would be great to
get rid of that.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Input: max77693 - Convert to atomic pwm operation

[Dmitry Torokhov]

Hi Uwe,

On Tue, Jul 29, 2025 at 10:04:06PM +0200, Uwe Kleine-König wrote:
> Hello Dmitry,
> 
> On Wed, Jul 02, 2025 at 08:02:33AM +0200, Uwe Kleine-König wrote:
> > On Tue, Jul 01, 2025 at 11:06:50AM -0700, Dmitry Torokhov wrote:
> > > On Tue, Jul 01, 2025 at 07:49:22AM +0200, Uwe Kleine-König wrote:
> > > > 
> > > > I had something like that at first, but didn't like it. With that
> > > > approach you have two places that have to know how to set the PWM's
> > > > duty_cycle. Also I think the control flow is more complicated.
> > > > 
> > > > I considered renaming max77693_haptic_enable() to something that better
> > > > matches what it does in my variant, but max77693_haptic_configure() was
> > > > already taken.
> > > > 
> > > > But that might all be subjective? If you like your version better,
> > > > that's fine, it still gets rid of pwm_config(), pwm_enable() and
> > > > pwm_apply_args() which is my main objective.
> > > 
> > > Yes, I agree that it is subjective. I know that you do not quite like
> > > the version I posted, still will you be OK if it is attributed to you?
> > 
> > Yes, feel free to apply it as you suggested.
> 
> As of today's next that didn't happen. Do you have this patch still on
> your radar?

My bad, I lost track of it when I reshuffled my queue. 

> 
> This is the last driver making use of pwm_config(), it would be great to
> get rid of that.

The patch is in the pull request I just sent to Linus, hope this
unblocks you.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: max77693 - Convert to atomic pwm operation

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 251 bytes --]

Hello Dmitry,

On Wed, Aug 06, 2025 at 09:33:25PM -0700, Dmitry Torokhov wrote:
> The patch is in the pull request I just sent to Linus, hope this
> unblocks you.

I'm not under great pressure, but if I know it's not stalled that's
great.

Thanks
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]