Re: [PATCH v2 1/3] pwm: rockchip: Don't update the state for the caller of pwm_apply_state()

Uwe Kleine-König
2019-05-03T19:59:05Z

Hello Thierry,

On Fri, May 03, 2019 at 12:59:15PM +0200, Thierry Reding wrote:
> On Thu, May 02, 2019 at 10:42:43AM +0200, Uwe Kleine-König wrote:
> > On Thu, May 02, 2019 at 10:09:51AM +0200, Boris Brezillon wrote:
> > > On Thu, 2 May 2019 09:33:26 +0200
> > > Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > On Thu, May 02, 2019 at 09:16:38AM +0200, Boris Brezillon wrote:
> > > > > I do understand that some users might want to check how the HW will
> > > > > adjust the period/duty before applying the new setup, and in that
> > > > > regard, having pwm_round_rate() is a good thing. But in any case, it's
> > > > > hard for the user to decide how to adjust things to get what it wants
> > > > > (should he increase/decrease the period/duty?).

Whenever I wrote pwm_round_rate I actually meant pwm_round_state.

> > > > It depends on the use case. For one of them I suggested an algorithm.
> > >
> > > Yes, I was just trying to say that passing a PWM state to
> > > pwm_round_state() is not enough, we need extra info if we want to make
> > > it useful, like the rounding policy, the accepted deviation on period,
> > > duty or the duty/period ratio, ....
> >
> > Ack. My suggestion is that round_rate should do:
> >
> > if polarity is unsupported:
> > polarity = !polarity
> > duty_cycle = period - duty_cycle
>
> This should really be up to the consumer. The PWM framework or driver
> doesn't know whether or not the consumer cares about the polarity or
> whether it only cares about the power output.

You don't understand my idea. If the hardware cannot implement the
requested state the round_state function has to return a best
approximation according to some rules. After that the consumer can
decide if they want that or not. As the hardware isn't touched nothing
bad happens.

(Well, apart from that I still cannot imagine a use case where with the
current possibilities of the PWM API a consumer can really care about
the polarity.)

> > period = biggest supportable period <= requested period, 0 if no
> > such period exists.
> >
> > duty_cycle = biggest supportable duty cycle <= requested
> > duty_cycle, 0 if no such value exists
>
> This doesn't really work. We need some way to detect "value does not
> exist" that is different from value == 0, because value == 0 is a
> legitimate use-case.

Same as above. If I asked for duty_cycle = 5 ns and get back duty_cycle
= 0 ns this means the driver cannot implement a duty_cycle in the
interval (0, 5] ns, nothing more.

> > This would allow to let the consumer (or framework helper function)
> > decide which deviation is ok.
>
> So what's the consumer supposed to do if it gets back these values?

If the value is then known to be the best (or good enough if the
consumer doesn't really care), it is used. Otherwise a different state
is rounded or the consumer gives up if it becomes clear that the
hardware cannot satisfy their needs.

> How does it know how to scale them if the deviation is not okay?

It depends on what the consumer considers optimal. I already gave one
example code how I think pwm_round_state should be used. Another is:

If the consumer wants a period as near as possible to a given target
period the algorithm with the round_state function as suggested would
look as follows (duty_cycle ignored here for the ease of discussion):

mystate.period = target_period
...
pwm_round_state(pwm, &mystate)

if (mystate.period == target_period)
return mystate.period

mystate.period = 2 * target_period - mystate.period
pwm_round_state(pwm, &mystate)

if (mystate.period < target_period)
return mystate.period;

do {
best_bigger = mystate.period
mystate.period -= 1
pwm_round_state(pwm, &mystate)
} while (mystate.period > target_period)

return best_bigger

> What in case the hardware can do achieve a good period that is
> slightly bigger than the requested period and which would give a very
> good result?

The above algorithm answers this question.

Theoretically the requirements for pwm_round_state could be changed such
that it is supposed to yield the nearest hit instead of the next smaller
one, but this only shifts the complexity to a different area. (I think
my suggestion is the better one though because the math for the lowlevel
drivers and also the helper functions is easier then. Also the search
terminates earlier if the consumer wants something bigger than the
driver can implement. Moreover "biggest value not bigger than requested"
is (IMHO) easier to understand for the affected coders than "nearest
value" because there are more ugly special cases. (Consider I ask for
(duty_cycle, period) = (10, 20) and the driver can implement (9, 18) and
(12, 21). Which of these is nearer?))

Best regards
Uwe

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