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/ |
