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

[Uwe Kleine-König]

Hello Thierry,

On Fri, Apr 09, 2021 at 01:25:36PM +0200, Thierry Reding wrote:
> On Thu, Apr 08, 2021 at 07:36:37PM +0200, Uwe Kleine-König wrote:
> > On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote:
> > > On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote:
> > > > Yes, I think that's basically what this is saying. I think we're perhaps
> > > > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
> > > > the impression that we're dealing with some provider-specific feature,
> > > > whereas what we really want to express is that the PWM doesn't care
> > > > exactly when the active cycle starts and based on that a provider that
> > > > can support it may optimize the EMI behavior.
> > > > 
> > > > Maybe we can find a better name for this? Ultimately what this means is
> > > > that the consumer is primarily interested in the power output of the PWM
> > > > rather than the exact shape of the signal. So perhaps something like
> > > > PWM_USAGE_POWER would be more appropriate.
> > > 
> > > Yes, although it would then no longer be obvious that this feature leads
> > > to improved EMI behavior, as long as we mention that in the docs, I
> > > think it's a good idea
> > > 
> > > Maybe document it as follows?
> > > PWM_USAGE_POWER - Allow the driver to delay the start of the cycle
> > > for EMI improvements, as long as the power output stays the same
> > 
> > I don't like both names, because for someone who is only halfway into
> > PWM stuff it is not understandable. Maybe ALLOW_PHASE_SHIFT?
> 
> Heh... how's that any more understandable?

The questions that come to (my) mind when reading PWM_USAGE_POWER are:
So the PWM is allowed to use some power? The PWM is used to provide a
power source (like a regulator)? Has this something to do with a
permission to use the PWM (the power to use it)?

Please try googling for "usage power", and compare it with the results
you get for "phase shift".

> > When a consumer is only interested in the power output than
> > 
> > 	.period = 20
> > 	.duty_cycle = 5
> > 
> > would also be an allowed response for the request
> > 
> > 	.period = 200
> > 	.duty_cycle = 50
> > 
> > and this is not what is in the focus here.
> 
> Actually, that's *exactly* what's important here. From a consumer point
> of view the output power is the key in this case.

OK, so if I understand you correctly, you want indeed allow

 	.period = 200
 	.duty_cycle = 50

when

	.period = 20
	.duty_cycle = 5

was requested? Do you want also allow .period = 20000 + .duty_cycle =
5000? How would you limit what is allowed? I'd expect we don't want to
allow .period = 20000000000 + .duty_cycle = 5000000000? What should a
driver for a PWM backlight pass to pwm_apply_state if the PWM period
should be between 4000000 ns and 16666666 ns? (This comes from the first
backlight datasheet I found where the valid range for the brightness
input is 60 to 250Hz.) Maybe saying that only making the period smaller
would be an idea; but the motor bridge I recently worked with[1] limits
the PWM frequency to "up to 20 kHz", so this isn't an universally good
idea either.

[1] https://www.st.com/resource/en/datasheet/vnh5019a-e.pdf

> The specifier is a description of a particular PWM in the consumer
> context. And the consumer not going to care what exactly the PWM
> controller might end up configuring to achieve best results. If the
> controller allows the phase shift to be changed and the constraints
> allow it, then that's great, but it isn't something that the consumer
> has to know if all it wants is that the power output is as requested.

Yes, if ALLOW_PHASE_SHIFT isn't what the consumer actually wants, they
shouldn't use it. Agreed.

> Put another way, the more generically we can describe the constraints
> or use cases, the more flexibility we get for drivers to fulfill those
> constraints.

Yes, from the POV of a lowlevel driver the more general the better. From
the POV of a consumer this isn't universally true, because the consumer
might only accept a subset of the freedom this general flag gives to the
lowlevel driver.

> For example one controller might support phase shifting
> and use that for PWM_USAGE_POWER for better EMI behaviour. But another
> PWM controller may not support it. But it could perhaps want to
> optimize the PWM signal by reversing the polarity of one channel or
> whatever other mechanism there may be.
> 
> If we add a flag such as ALLOW_PHASE_SHIFT, then only controllers that
> support programmable phase shift will be able to support this.

That's wrong, drivers that support the polarity that was not requested
can make use of it, too. That's something you already pointed out
yourself (or I misunderstood you). (Then

	.period = X
	.duty_cycle = Y
	.polarity = PWM_POLARITY_NORMAL

is "equivalent" to

	.period = X
	.duty_cycle = X - Y
	.polarity = PWM_POLARITY_INVERSED

as the signals only differ by a phase shift.)

> If some other mechanism can also be used to support "equivalent power"
> use cases, that would have to be described as some other flag, which
> has essentially the same meaning. So you can get into a situation
> where you have multiple flags used for the same thing.

If they are for the same thing, you don't need another flag. Your
concern is only valid if all consumers that are ok to accept a phase
shifted PWM signal really only care about the relative duty cycle. I
think that's wrong. (All consumers that use the PWM as something like a
clock signal where something happens on the rising and/or falling edge
and want this something happen with a certain frequency care about the
period, but not the phase shift.)

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature