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

[Uwe Kleine-König]

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.

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. 

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.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature