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

[Uwe Kleine-König]

Hello Clément,

On Thu, Nov 14, 2019 at 11:47:00PM +0100, Clément Péron wrote:
> On Wed, 13 Nov 2019 at 09:58, Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > On Fri, Nov 08, 2019 at 09:45:14AM +0100, Clément Péron wrote:
> > > From: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > > +     /*
> > > +      * Although it would make much more sense to check for bypass in
> > > +      * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> >
> > I don't understand this reasoning. sun4i_pwm_calculate knows about
> > .enabled and also sun4i_pwm->data->has_direct_mod_clk_output. Maybe just
> > add a bool *bypass as parameter and move the logic there?
> 
> I asked myself the same question, however the sun4i_pwm_calculate is
> only called when period or duty_cycle is updated not when state is
> toggled between disabled / enabled.
> 
> Should we also call it when the state is switched to enabled ?

Given that the check

	if ((cstate.period != state->period) ||
	    (cstate.duty_cycle != state->duty_cycle)) {

is not perfect anyhow (because if I toggle between

	pwm_apply_state(pwm, { .period = 50000001, .duty_cycle = 25000000, .enabled = true });

and

	pwm_apply_state(pwm, { .period = 50000000, .duty_cycle = 25000000, .enabled = true });

the code recalculates every parameter while it (probably) doesn't make a
difference.) And also given that cstate is filled by pwm_get_state which
might change its semantic slightly in the future I would say calculating
the needed parameter always is also cleaner. (But I'm aware this isn't
objective enough to overrule different opinions.) While I'm a fan of
avoid unneeded calculations, I think having a simple driver is more
important.

(And apart from that I don't like lowlevel drivers calling the pwm API
that is designed for consumers.)

Best regards
Uwe

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