linux-kernel - Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times

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]

Message-ID: <20210402194805.rj65qcdmzfgcbgri@pengutronix.de>
Date:   Fri, 2 Apr 2021 21:48:05 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Thierry Reding <thierry.reding@...il.com>,
        Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     linux-pwm@...r.kernel.org, Sven Van Asbroeck <TheSven73@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 4/7] pwm: pca9685: Support staggered output ON times

Hello,

On Wed, Mar 31, 2021 at 06:21:32PM +0200, Thierry Reding wrote:
> However, I'm a bit hesitant about this staggering output mode. From what
> I understand what's going to happen for these is basically that overall
> each PWM will be running at the requested duty cycle, but the on/off
> times will be evenly spread out over the whole period. In other words,
> the output *power* of the PWM signal will be the same as if the signal
> was a single on/off cycle. That's not technically a PWM signal as the
> PWM framework defines it. See the kerneldoc for enum pwm_polarity for
> what signals are expected to look like.

After reading this thread I had the impression that there is no
(externally visible) difference between using ON = 0 plus programming a
new setting when the counter is say 70 and using ON = 30 plus
programming a new setting when the counter is 100. But that's not the
case and I agree that defaulting to staggering is a bad idea.

Having said that I doubt that adding a property to the device tree is a
good solution, because it changes behaviour without the consumer being
aware and additionally it's not really a hardware description.

The solution I'd prefer is to change struct pwm_state to include the
delay in it. (This would then make the polarity obsolete, because

	.duty_cycle = 30
	.period = 100
	.polarity = POLARITY_INVERTED
	.offset = 0

is equivalent to

	.duty_cycle = 30
	.period = 100
	.polarity = POLARITY_NORMAL
	.offset = 70

. Other inverted states can be modified similarily.) Then consumers can
be coordinated to use different offsets.

I'm aware changing this isn't trivial, and it's not thought out
completely, but I think the end result is rechnically superior to the
approach suggested in the patch under discussion.

Best regards
Uwe

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)