Re: [PATCH v2] pwm: mediatek: support inverted polarity

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2046 bytes --]

On Thu, Apr 06, 2023 at 03:53:58PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Thu, Apr 06, 2023 at 03:38:48PM +0200, Thierry Reding wrote:
> > On Thu, Mar 09, 2023 at 02:04:10AM +0100, Lorenz Brun wrote:
> > > +	 * appear to have the capability to invert the output.
> > > +	 * This means that inverted mode can not be fully supported as the
> > > +	 * waveform will always start with the low period and end with the high
> > > +	 * period. Thus reject non-normal polarity if the shape of the waveform
> > > +	 * matters, i.e. usage_power is not set.
> > > +	 */
> > > +	if (state->polarity != PWM_POLARITY_NORMAL && !state->usage_power)
> > >  		return -EINVAL;
> > >  
> > >  	if (!state->enabled) {
> > > @@ -213,7 +221,11 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  		return 0;
> > >  	}
> > >  
> > > -	err = pwm_mediatek_config(pwm->chip, pwm, state->duty_cycle, state->period);
> > > +	duty_cycle = state->duty_cycle;
> > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > +		duty_cycle = state->period - state->duty_cycle;
> > 
> > That's not really what state->usage_power was meant to address.
> 
> I don't understand your concern here. I don't like .usage_power, but
> AFAICT this is a legitimite use. With .usage_power = true, the lowlevel
> driver is free to shift the phase_offset and even modify the period size
> and the goal is just that the average power-output matches.
> 
> Lorenz's patch does exactly this: It even keeps the period and only
> shifts the phase (by period - duty_cycle). If you consider this not
> legitmate, I think we have to improve the docs about .usage_power.

I realize that I'm being nitpicky here. Setting usage_power = true and
duty = period - duty is a lazy way of achieving what you can easily do
by adjusting the input duty cycle.

If you all really want this, then it should go into the core, because
it's something that can be implemented on basically every single PWM
controller.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]