[PATCH] input: misc: max8997: Switch to pwm_apply()

[PATCH] input: misc: max8997: Switch to pwm_apply()

[Uwe Kleine-König]

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(-)

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:
-- 
2.30.1

Re: [PATCH] input: misc: max8997: Switch to pwm_apply()

[Dmitry Torokhov]

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

Re: [PATCH] input: misc: max8997: Switch to pwm_apply()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1341 bytes --]

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



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

Re: [PATCH] input: misc: max8997: Switch to pwm_apply()

[Dmitry Torokhov]

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