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