linux-kernel - Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Apr 2021 22:21:10 +0200
From:   Clemens Gruber <clemens.gruber@...ruber.com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     linux-pwm@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Sven Van Asbroeck <TheSven73@...il.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

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