| Message ID | 20210316203813.48999-1-uwe@kleine-koenig.org |
|---|---|
| State | Changes Requested |
| 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.
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!
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(-)