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

From: Clemens Gruber <clemens.gruber@pqgruber.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: linux-pwm@vger.kernel.org,
	Thierry Reding <thierry.reding@gmail.com>,
	Sven Van Asbroeck <TheSven73@gmail.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag
Date: Wed, 7 Apr 2021 22:21:10 +0200	[thread overview]
Message-ID: <YG4UNoBCQJkEEfwi@workstation.tuxnet> (raw)
In-Reply-To: <20210407054658.qdsjkstqwynxeuxj@pengutronix.de>

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".

> 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.

> 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.

Thierry: What's your take on this?

Thanks,
Clemens