Message-ID: <20210628162407.dxxt6hqfzeokdtxa@pengutronix.de>
Date:   Mon, 28 Jun 2021 18:24:07 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Sean Anderson <sean.anderson@...o.com>
Cc:     linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Emil Lenngren <emil.lenngren@...il.com>,
        michal.simek@...inx.com, Alvaro Gamez <alvaro.gamez@...ent.com>,
        Thierry Reding <thierry.reding@...il.com>,
        kernel@...gutronix.de, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer

Hello Sean,

On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> On 6/27/21 2:19 PM, Uwe Kleine-König wrote:
> > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> > > So for the moment, why not give an error? This will be legal code both
> > > now and after round_state is implemented.
> > 
> > The problem is where to draw the line. To stay with your example: If a
> > request for period = 150 ns comes in, and let X be the biggest period <=
> > 150 ns that the hardware can configure. For which values of X should an
> > error be returned and for which values the setting should be
> > implemented.
> > 
> > In my eyes the only sensible thing to implement here is to tell the
> > consumer about X and let it decide if it's good enough. If you have a
> > better idea let me hear about it.
> 
> Sure. And I think it's ok to tell the consumer that X is the best we can
> do. But if they go along and request an unconfigurable state anyway, we
> should tell them as much.

I have the impression you didn't understand where I see the problem. If
you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
expects that it can configure the duty_cycle in 1/256 steps of the
period, and then maybe only steps 27 and 213 of the 256 possible steps
work. (This example doesn't really match because the led-pwm driver
varies duty_cycle and not period, but the principle becomes clear I
assume.) If no: Should we accept 151 ns? Isn't that ridiculous?

> IMO, this is the best way to prevent surprising results in the API.

I think it's not possible in practise to refuse "near" misses and every
definition of "near" is in some case ridiculous. Also if you consider
the pwm_round_state() case you don't want to refuse any request to tell
as much as possible about your controller's capabilities. And then it's
straight forward to let apply behave in the same way to keep complexity
low.

> The real issue here is that it is impossible to determine the correct
> way to round the PWM a priori, and in particular, without considering
> both duty_cycle and period. If a consumer requests very small
> period/duty cycle which we cannot produce, how should it be rounded?

Yeah, because there is no obviously right one, I picked one that is as
wrong as the other possibilities but is easy to work with.

> Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
> the least period? Or should we try and increase the period to better
> approximate the % duty cycle? And both of these decisions must be made
> knowing both parameters. We cannot (for example) just always round up,
> since we may produce a configuration with TLR0 == TLR1, which would
> produce 0% duty cycle instead of whatever was requested. Rounding rate
> will introduce significant complexity into the driver. Most of the time
> if a consumer requests an invalid rate, it is due to misconfiguration
> which is best solved by fixing the configuration.

In the first step pick the biggest period not bigger than the requested
and then pick the biggest duty cycle that is not bigger than the
requested and that can be set with the just picked period. That is the
behaviour that all new drivers should do. This is somewhat arbitrary but
after quite some thought the most sensible in my eyes.

> > > Perhaps I should add
> > > 
> > > 	if (tlr0 <= tlr1)
> > > 		return -EINVAL;
> > > 
> > > here to prevent accidentally getting 0% duty cycle.
> > 
> > You can assume that duty_cycle <= period when .apply is called.
> 
> Ok, I will only check for == then.

You just have to pay attention to the case that you had to decrement
.period to the next possible value. Then .duty_cycle might be bigger
than the corrected period.

Best regards
Uwe

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists