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

[Uwe Kleine-König]

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature