lkml.org 
[lkml]   [2019]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/ |

\
 
 \ /
  Last update: 2019-11-15 08:36    [W:0.059 / U:1.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site