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

[Uwe Kleine-König]

Hello,

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().

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.)

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.

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).

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.))

So in sum: I think this concept is too inchoate and we shouldn't apply
these patches. I would prefer to go for allow-phase-shift (if at all)
for now. There the concept is clear what is allowed (for a lowlevel
driver) resp. can be expected (for a consumer).

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature