Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Thierry Reding]

On Wed, Apr 07, 2021 at 11:34:03PM +0200, Uwe Kleine-König wrote:
> On Wed, Apr 07, 2021 at 10:21:10PM +0200, Clemens Gruber wrote:
> > On Wed, Apr 07, 2021 at 07:46:58AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote:
> > > > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver
> > > > may (if supported by the HW) delay the ON time of the channel relative
> > > > to the channel number.
> > > > This does not alter the duty cycle ratio and is only relevant for PWM
> > > > chips with less prescalers than channels, which would otherwise assert
> > > > multiple or even all enabled channels at the same time.
> > > > 
> > > > If this feature is supported by the driver and the flag is set on
> > > > multiple channels, their ON times are spread out to improve EMI and
> > > > reduce current spikes.
> > > 
> > > As said in reply to patch 4/8 already: I don't like this idea and
> > > think this should be made explicit using a new offset member in struct
> > > pwm_state instead. That's because I think that the wave form a PWM
> > > generates should be (completely) defined by the consumer and not by a
> > > mix between consumer and device tree. Also the consumer has no (sane)
> > > way to determine if staggering is in use or not.
> > 
> > I don't think offsets are ideal for this feature: It makes it more
> > cumbersome for the user, because he has to allocate the offsets
> > himself instead of a simple on/off switch.
> > The envisioned usecase is: "I want better EMI behavior and don't care
> > about the individual channels no longer being asserted at the exact same
> > time".
> 
> The formal thing is: "I want better EMI behavior and don't care if
> periods start with the active phase, it might be anywhere, even over a
> period boundary." Being asserted at the exact same time is just a detail
> for the pca9685.
>  
> > > One side effect (at least for the pca9685) is that when programming a
> > > new duty cycle it takes a bit longer than without staggering until the
> > > new setting is active. 
> > 
> > Yes, but it can be turned off if this is a problem, now even per-PWM.
> 
> Yes and that is a good thing. (BTW: I'd call it per-PWM-consumer, but
> details.)
> 
> > > Another objection I have is that we already have some technical debt
> > > because there are already two different types of drivers (.apply vs
> > > .config+.set_polarity+.enable+.disable) and I would like to unify this
> > > first before introducing new stuff.
> > 
> > But there is already PWM_POLARITY_INVERTED, which can be set in the DT.
> > I am only adding another flag.
> 
> I understand your reasoning, and similar to "This diplay backlight needs
> an inverted PWM (as a low duty-cycle results in a high brightness" this
> semantic "This consumer doesn't care if the active cycle is anywhere in
> the period". Hmm, maybe I just have to think about it a bit more to
> become friends with that thought.

Yes, I think that's basically what this is saying. I think we're perhaps
getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
the impression that we're dealing with some provider-specific feature,
whereas what we really want to express is that the PWM doesn't care
exactly when the active cycle starts and based on that a provider that
can support it may optimize the EMI behavior.

Maybe we can find a better name for this? Ultimately what this means is
that the consumer is primarily interested in the power output of the PWM
rather than the exact shape of the signal. So perhaps something like
PWM_USAGE_POWER would be more appropriate.

Come to think of it, a flag like that might even be useful to implement
the common case of emulating inverted polarity with reversing the duty
cycle. That is, if PWM_USAGE_POWER | PWM_POLARITY_INVERSED was specified
and the PWM provider did not support polarity inversion, the inversion
could still be implemented using emulation. Currently we push that logic
down into consumers, but this could be a way to bring that up into
drivers, or perhaps even the core.

Thierry

Attachment: signature.asc
Description: PGP signature