Re: [PATCH v3 4/7] pwm: sun4i: Add support to output source clock directly

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

Hello Clément,

On Wed, Nov 06, 2019 at 10:24:39PM +0100, Clément Péron wrote:
> On Tue, 5 Nov 2019 at 15:57, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Nov 05, 2019 at 02:14:53PM +0100, Clément Péron wrote:
> > > +     bypass = state->enabled &&
> > > +              (state->period * clk_rate >= NSEC_PER_SEC) &&
> >
> > This is too coarse. With state->period = 1000000 this is fulfilled
> > (unless the multiplication overflows).
> 
> Sorry, misunderstood the previous mail
> 
> What about something like this ?
> ((state->period - 1) * clk_rate <= NSEC_PER_SEC) &&
> ((state->period + 1) * clk_rate >= NSEC_PER_SEC) &&
>  ((state->duty_cycle - 1) * 2 <= state->period) &&
>  ((state->duty_cycle + 1) * 2 >= state->period);
> 
> We are sure that the user is looking for a PWM around the OSC with a
> 50% duty cycle ?

This again is too strict. The general policy to fulfill a request is:

 1) provide the longest possible period not bigger than requested
 2) provide the longest possible duty cycle not bigger than requested
 3) if possible complete the currently running period before switching
    and don't return to the user before the new setting is active.
    Document the behaviour prominently because the code (usually)
    doesn't allow to understand the hardware's features here.
 4) A disabled PWM should output the inactive level

And then there is a corner case: If the user requests .duty_cycle = 0,
.enabled = 1 it is ok to provide .enabled = 0 iff otherwise 0% isn't
possible.

So the right check for bypass is:

  state->period * clk_rate >= NSEC_PER_SEC &&
  state->period * clk_rate < whatevercanbereachedwithoutbypass &&
  state->duty_cycle * clk_rate * 2 >= NSEC_PER_SEC

Best regards
Uwe

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