* [PATCH] input: misc: max8997: Switch to pwm_apply()
@ 2021-03-16 20:38 Uwe Kleine-König
2021-03-21 22:23 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Uwe Kleine-König @ 2021-03-16 20:38 UTC (permalink / raw
To: Dmitry Torokhov; +Cc: Thierry Reding, Lee Jones, linux-input, linux-pwm
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] input: misc: max8997: Switch to pwm_apply()
2021-03-16 20:38 [PATCH] input: misc: max8997: Switch to pwm_apply() Uwe Kleine-König
@ 2021-03-21 22:23 ` Dmitry Torokhov
2021-03-22 8:16 ` Uwe Kleine-König
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Torokhov @ 2021-03-21 22:23 UTC (permalink / raw
To: Uwe Kleine-König; +Cc: Thierry Reding, Lee Jones, linux-input, linux-pwm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] input: misc: max8997: Switch to pwm_apply()
2021-03-21 22:23 ` Dmitry Torokhov
@ 2021-03-22 8:16 ` Uwe Kleine-König
2021-03-22 23:29 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Uwe Kleine-König @ 2021-03-22 8:16 UTC (permalink / raw
To: Dmitry Torokhov; +Cc: Thierry Reding, Lee Jones, linux-input, linux-pwm
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] input: misc: max8997: Switch to pwm_apply()
2021-03-22 8:16 ` Uwe Kleine-König
@ 2021-03-22 23:29 ` Dmitry Torokhov
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Torokhov @ 2021-03-22 23:29 UTC (permalink / raw
To: Uwe Kleine-König; +Cc: Thierry Reding, Lee Jones, linux-input, linux-pwm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-22 23:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-16 20:38 [PATCH] input: misc: max8997: Switch to pwm_apply() Uwe Kleine-König
2021-03-21 22:23 ` Dmitry Torokhov
2021-03-22 8:16 ` Uwe Kleine-König
2021-03-22 23:29 ` Dmitry Torokhov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.