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
