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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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