Messages in this thread | | | Date | Fri, 15 Nov 2019 08:35:47 +0100 | From | Uwe Kleine-König <> | Subject | Re: [PATCH v4 4/7] pwm: sun4i: Add support to output source clock directly |
| |
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@pengutronix.de> wrote: > > On Fri, Nov 08, 2019 at 09:45:14AM +0100, Clément Péron wrote: > > > From: Jernej Skrabec <jernej.skrabec@siol.net> > > > + /* > > > + * 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/ |
| |