| Message ID | 20250623112742.1372312-2-u.kleine-koenig@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: max8997_haptic - Optimize PWM configuration | expand |
Hi Uwe, On Mon, Jun 23, 2025 at 01:27:42PM +0200, Uwe Kleine-König wrote: > Both pwm_config() and pwm_enable() are wrappers around > pwm_apply_might_sleep(). Instead of calling this function twice only > call it once without an intermediate step. > > max8997_haptic_enable() is the only user of > max8997_haptic_set_duty_cycle(), so it's known in the latter that the > PWM should be enabled. Right, but the question is: is it OK for the PWM to be enabled before we enable the regulator/power up the motor? I'm afraid we need to use 2 distinct steps... Thanks.
On Sun, Jun 29, 2025 at 10:30:47PM -0700, Dmitry Torokhov wrote: > Hi Uwe, > > On Mon, Jun 23, 2025 at 01:27:42PM +0200, Uwe Kleine-König wrote: > > Both pwm_config() and pwm_enable() are wrappers around > > pwm_apply_might_sleep(). Instead of calling this function twice only > > call it once without an intermediate step. > > > > max8997_haptic_enable() is the only user of > > max8997_haptic_set_duty_cycle(), so it's known in the latter that the > > PWM should be enabled. > > Right, but the question is: is it OK for the PWM to be enabled before we > enable the regulator/power up the motor? > > I'm afraid we need to use 2 distinct steps... In that case the status quo is wrong (at least in general), too, because the behaviour of a PWM that isn't enabled isn't known/defined. Some have their output active, some have it inactive, some have it high-z. And even continuing to toggle would be a valid implementation if there is no chance to save some power. So if the device relies on e.g. a constant inactive output, it must enable it (with .duty_cycle = 0). Anyhow, I will try a more conservative approach to get rid of pwm_config(). Expect a v2 soon. Best regards Uwe
diff --git a/drivers/input/misc/max8997_haptic.c b/drivers/input/misc/max8997_haptic.c index f97f341ee0bb..6549df0b0919 100644 --- a/drivers/input/misc/max8997_haptic.c +++ b/drivers/input/misc/max8997_haptic.c @@ -58,8 +58,14 @@ 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.period = chip->pwm_period; + state.duty_cycle = chip->pwm_period * chip->level / 100; + state.enabled = true; + + ret = pwm_apply_might_sleep(chip->pwm, &state); } else { u8 duty_index = 0; @@ -168,14 +174,6 @@ static void max8997_haptic_enable(struct max8997_haptic *chip) return; } 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); - return; - } - } chip->enabled = true; } }
Both pwm_config() and pwm_enable() are wrappers around pwm_apply_might_sleep(). Instead of calling this function twice only call it once without an intermediate step. max8997_haptic_enable() is the only user of max8997_haptic_set_duty_cycle(), so it's known in the latter that the PWM should be enabled. There is a slight change in semantic because without this change the polarity configured before (which probably is at its bootup state) was kept, while with this change it is set to its default polarity. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- Hello, the motivation for this patch is to get rid of pwm_config(). This driver is one of the remaining two users of this function. Best regards Uwe drivers/input/misc/max8997_haptic.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) base-commit: f817b6dd2b62d921a6cdc0a3ac599cd1851f343c