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

[Uwe Kleine-König]

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

Attachment: signature.asc
Description: PGP signature