diff mbox series

input: misc: max8997: Switch to pwm_apply()

Message ID 20210316203813.48999-1-uwe@kleine-koenig.org
State New
Headers show
Series input: misc: max8997: Switch to pwm_apply() | expand

Commit Message

Uwe Kleine-König March 16, 2021, 8:38 p.m. UTC
max8997_haptic_enable() is the only caller of
max8997_haptic_set_duty_cycle(). For the non-external case the PWM is
already enabled in max8997_haptic_set_duty_cycle(), so this can be done
for the external case, too, and so the pwm_enable() call can be folded into
max8997_haptic_set_duty_cycle()'s call to pwm_apply_state().

With max8997_haptic_set_duty_cycle() now using pwm_init_state() the call to
pwm_apply_args() can be dropped.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/input/misc/max8997_haptic.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

Comments

Dmitry Torokhov March 21, 2021, 10:23 p.m. UTC | #1
Hi Uwe,

On Tue, Mar 16, 2021 at 09:38:13PM +0100, Uwe Kleine-König wrote:
> max8997_haptic_enable() is the only caller of

> max8997_haptic_set_duty_cycle(). For the non-external case the PWM is

> already enabled in max8997_haptic_set_duty_cycle(), so this can be done


Are you sure about that? I think the intent was to enable it in
max8997_haptic_configure(), and only after "inmotor" regulator is
enabled. If the device is enabled earlier then I'd say we need to make
sure we disable it until it is needed.

Thanks.

-- 
Dmitry
Uwe Kleine-König March 22, 2021, 8:16 a.m. UTC | #2
Hi Dmitry,

On 3/21/21 11:23 PM, Dmitry Torokhov wrote:
> On Tue, Mar 16, 2021 at 09:38:13PM +0100, Uwe Kleine-König wrote:

>> max8997_haptic_enable() is the only caller of

>> max8997_haptic_set_duty_cycle(). For the non-external case the PWM is

>> already enabled in max8997_haptic_set_duty_cycle(), so this can be done

> 

> Are you sure about that? I think the intent was to enable it in

> max8997_haptic_configure(), and only after "inmotor" regulator is

> enabled. If the device is enabled earlier then I'd say we need to make

> sure we disable it until it is needed.


If you claim you understand this better, I will well believe that. I 
described my train of thoughts, i.e. how I understood the internal case.

Anyhow, there is little sense in separating configuration and enablement 
of the PWM, because the change of duty_cycle and period for a disabled 
PWM is expected to do nothing to the hardware's output.

So the safer approach is to do the pwm_apply_state at the place, where 
pwm_enable was before, but the more consistent is how I suggested in my 
patch. If it feels better I can do the more conservative change instead 
and if somebody with a deeper understanding of the driver and/or a 
testing possibility can be found, the internal and external cases can be 
unified.

Best regards
Uwe
Dmitry Torokhov March 22, 2021, 11:29 p.m. UTC | #3
Hi Uwe,

On Mon, Mar 22, 2021 at 09:16:43AM +0100, Uwe Kleine-König wrote:
> Hi Dmitry,

> 

> On 3/21/21 11:23 PM, Dmitry Torokhov wrote:

> > On Tue, Mar 16, 2021 at 09:38:13PM +0100, Uwe Kleine-König wrote:

> > > max8997_haptic_enable() is the only caller of

> > > max8997_haptic_set_duty_cycle(). For the non-external case the PWM is

> > > already enabled in max8997_haptic_set_duty_cycle(), so this can be done

> > 

> > Are you sure about that? I think the intent was to enable it in

> > max8997_haptic_configure(), and only after "inmotor" regulator is

> > enabled. If the device is enabled earlier then I'd say we need to make

> > sure we disable it until it is needed.

> 

> If you claim you understand this better, I will well believe that. I

> described my train of thoughts, i.e. how I understood the internal case.

> 

> Anyhow, there is little sense in separating configuration and enablement of

> the PWM, because the change of duty_cycle and period for a disabled PWM is

> expected to do nothing to the hardware's output.

> 

> So the safer approach is to do the pwm_apply_state at the place, where

> pwm_enable was before, but the more consistent is how I suggested in my

> patch. If it feels better I can do the more conservative change instead and

> if somebody with a deeper understanding of the driver and/or a testing

> possibility can be found, the internal and external cases can be unified.


Yes, could we please go with the more conservative approach as I do not
have the hardware to verify the behavior.

Thanks!

-- 
Dmitry
diff mbox series

Patch

diff --git a/drivers/input/misc/max8997_haptic.c b/drivers/input/misc/max8997_haptic.c
index 20ff087b8a44..c86966ea0f16 100644
--- a/drivers/input/misc/max8997_haptic.c
+++ b/drivers/input/misc/max8997_haptic.c
@@ -58,8 +58,12 @@  static int max8997_haptic_set_duty_cycle(struct max8997_haptic *chip)
 	int ret = 0;
 
 	if (chip->mode == MAX8997_EXTERNAL_MODE) {
-		unsigned int duty = chip->pwm_period * chip->level / 100;
-		ret = pwm_config(chip->pwm, duty, chip->pwm_period);
+		struct pwm_state state;
+		pwm_init_state(chip->pwm, &state);
+		state.enabled = true;
+		state.period = chip->pwm_period;
+		pwm_set_relative_duty_cycle(&state, chip->level, 100);
+		ret = pwm_apply_state(chip->pwm, &state);
 	} else {
 		int i;
 		u8 duty_index = 0;
@@ -173,14 +177,6 @@  static void max8997_haptic_enable(struct max8997_haptic *chip)
 			goto out;
 		}
 		max8997_haptic_configure(chip);
-		if (chip->mode == MAX8997_EXTERNAL_MODE) {
-			error = pwm_enable(chip->pwm);
-			if (error) {
-				dev_err(chip->dev, "Failed to enable PWM\n");
-				regulator_disable(chip->regulator);
-				goto out;
-			}
-		}
 		chip->enabled = true;
 	}
 
@@ -293,11 +289,6 @@  static int max8997_haptic_probe(struct platform_device *pdev)
 			goto err_free_mem;
 		}
 
-		/*
-		 * FIXME: pwm_apply_args() should be removed when switching to
-		 * the atomic PWM API.
-		 */
-		pwm_apply_args(chip->pwm);
 		break;
 
 	default: