Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

[Thierry Reding]

On Mon, May 31, 2021 at 06:12:32PM +0200, Clemens Gruber wrote:
> On Sat, May 08, 2021 at 01:18:31AM +0200, Uwe Kleine-König wrote:
> > Hello Clemens,
> > 
> > On Fri, May 07, 2021 at 05:47:58PM +0200, Clemens Gruber wrote:
> > > On Fri, May 07, 2021 at 05:03:17PM +0200, Uwe Kleine-König wrote:
> > > > On Fri, May 07, 2021 at 03:18:42PM +0200, Clemens Gruber wrote:
> > > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > > > index 5bb90af4997e..5a73251d28e3 100644
> > > > > --- a/include/linux/pwm.h
> > > > > +++ b/include/linux/pwm.h
> > > > > @@ -54,12 +54,17 @@ enum {
> > > > >   * @duty_cycle: PWM duty cycle (in nanoseconds)
> > > > >   * @polarity: PWM polarity
> > > > >   * @enabled: PWM enabled status
> > > > > + * @usage_power: If set, the PWM driver is only required to maintain the power
> > > > > + *               output but has more freedom regarding signal form.
> > > > > + *               If supported, the signal can be optimized, for example to
> > > > > + *               improve EMI by phase shifting individual channels.
> > > > >   */
> > > > >  struct pwm_state {
> > > > >  	u64 period;
> > > > >  	u64 duty_cycle;
> > > > >  	enum pwm_polarity polarity;
> > > > >  	bool enabled;
> > > > > +	bool usage_power;
> > > > >  };
> > > > >  
> > > > >  /**
> > > > 
> > > > If we really want to support this usecase, I would prefer to not have it
> > > > in pwm_state because this concept is not a property of the wave form. So
> > > > for a driver it doesn't really make sense to set this flag in
> > > > .get_state().
> > > 
> > > It is related to the wave form in so far as it allows the driver to
> > > change the wave form as long as the power output remains the same.
> > 
> > Yes, the thing I wanted to express is: usage_power is a software thing.
> > Just from reading the hardware registers you can never know if
> > usage_power is set or not. So it is conceptually slightly different than
> > the other members of pwm_state which all are represented 1:1 in
> > hardware.
> > 
> > > > Maybe it makes more sense to put this in a separate argument for a
> > > > variant of pwm_apply_state? something like:
> > > > 
> > > > 	int pwm_apply_state_transition(struct pwm_device *pwm, const struct pwm_state *state, const struct pwm_state_transition *transition);
> > > > 
> > > > and pwm_state_transition can then contain something like this usage
> > > > power concept and maybe also a sync flag that requests that the call
> > > > should only return when the new setting is active and maybe also a
> > > > complete_period flag that requests that the currently running period
> > > > must be completed before the requested setting is implemented.
> > > > 
> > > > OTOH the information "I only care about the relative duty cycle" is
> > > > relevant longer than during the transition to a new setting. (So if
> > > > there are two consumers and one specified to be only interested in the
> > > > relative duty cycle, the other might be allowed to change the common
> > > > period.)
> > > 
> > > As you said, usage_power does not only apply to one state transition.
> > > 
> > > > Having said that, I don't like the proposed names. Neither "usage_power"
> > > > nor "pwm_apply_state_transition".
> > > > 
> > > > In a non-representative survey among two hardware engineers and one
> > > > software engineer who already contributed to PWM (!= me) I found out
> > > > that these three have an intuitive right understanding of
> > > > "allow-phase-shift" but have no idea what "usage-power" could mean.
> > > 
> > > One advantage of usage_power is that it is not limited to phase
> > > shifting. Drivers could do other optimizations as long as the power
> > > output remains the same.
> > 
> > Freedom you give to the lowlevel driver might be a burden to the
> > consumer. I think it makes sense to split the concept into:
> > 
> > 	PWM_ALLOW_PHASE_SHIFT		1
> > 	PWM_SET_RELATIVE_DUTY		2
> > 	PWM_SET_POWER			(PWM_ALLOW_PHASE_SHIFT | PWM_SET_RELATIVE_DUTY)
> > 
> > This way a consumer (e.g. a clock driver) who doesn't care about the
> > phase shift but wants a fixed period can specify that, and if a consumer
> > really only cares about the emitted power that's possible, too.
> > 
> > And given that your driver actually only implements a phase shift I
> > suggest not to generalize more than necessary here; also because the
> > semantic of usage-power isn't well defined. So this is something where I
> > agree to Thierry: Let's not solve a problem we don't have. (Though he
> > comes to a different conclusion here.)
> > 
> > > > On a side note: The hardware guys noted that it might make sense to
> > > > align the shifted phases. i.e. instead of shifting channel i by i *
> > > > period/16, it might be better to let the 2nd channel get active when the
> > > > first gets inactive. (i.e. try to keep the count of active channels
> > > > constant).
> > > 
> > > I am not sure what you mean exactly, because the duty cycles of the
> > > 16 outputs are not necessarily the same and can all be active at the
> > > same time. The idea is to spread the edges out as evenly as possible.
> > > Shifting them by period/16 is the best way to smoothen the current
> > > spikes in my opinion and the implementation is also very simple.
> > 
> > Yes, the algorithm needed to satisfy this wish is more complicated. And
> > maybe it even isn't possible to implement it in a sane way, I didn't
> > think about it much. I just believed them that if you have two channels
> > that run at 50% it's better to have a phase shift of 50% between them
> > than 6.25%. Maybe it makes sense to align the start of channel #i+1 (if
> > allowed) to the end of channel #i to already get a better behaviour on
> > average than the fixed offset depending on the channel number.
> > 
> > > > And as already mentioned earlier I still think we should define the
> > > > concept of "usage power" better. It should be clearly defined for a
> > > > driver author which setting they should pick for a given request. This
> > > > removes surprises for consumers and guessing for lowlevel driver
> > > > authors. Also a uniform policy brings conflicts better to light.
> > > > (Something like driver A does the right thing for consumer C and driver
> > > > B makes it right for D. But once D tries to use A things break. (And
> > > > then maybe A is changed to fit D, and C doesn't object because they
> > > > don't read the pwm list resulting in a breakage for C later.))
> > > 
> > > I added documentation and comments to the header file as a first step
> > > but we can always improve them.
> > 
> > In my book the documentation is inadequate because it doesn't define how
> > a driver should behave and so doesn't define what the consumer can
> > expect. With your description all settings I suggested in
> > https://lore.kernel.org/r/20210413175631.pwbynvwmnn7oog4m@xxxxxxxxxxxxxx
> > are allowed. I don't think this is a good idea.
> > 
> > And given that all people I talked to about "usage-power" were unable to
> > correctly guess its semantic, I'm convinced that we need a better name.
> > This is something you cannot outweigh with documentation -- most people
> > won't read it anyway.
> 
> Thierry: Would be great to get your input on this.

In the interest of making forward progress, I've applied this series.
The good thing is that with the current proposal none of this leaks into
ABI, so nothing is set in stone. If ever somebody comes up with better
names we can change them.

Thierry

Attachment: signature.asc
Description: PGP signature