Re: [PATCH 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

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

Hello,

On Wed, Jan 16, 2019 at 04:40:42PM +0530, Yash Shah wrote:
> On Wed, Jan 16, 2019 at 3:30 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Jan 11, 2019 at 01:52:44PM +0530, Yash Shah wrote:
> > > Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
> > >
> > > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > > [Atish: Various fixes and code cleanup]
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > > ---
> > >  drivers/pwm/Kconfig      |  10 ++
> > >  drivers/pwm/Makefile     |   1 +
> > >  drivers/pwm/pwm-sifive.c | 246 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 257 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-sifive.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a8f47df..3bcaf6a 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -380,6 +380,16 @@ config PWM_SAMSUNG
> > >         To compile this driver as a module, choose M here: the module
> > >         will be called pwm-samsung.
> > >
> > > +config PWM_SIFIVE
> > > +     tristate "SiFive PWM support"
> > > +     depends on OF
> > > +     depends on COMMON_CLK
> >
> > I'd say add:
> >
> >         depends on MACH_SIFIVE || COMPILE_TEST
> >
> > (I guess "MACH_SIFIVE" is wrong, but I assume you get what I mean.)
> 
> As of now, MACH_SIFIVE/ARCH_SIFIVE isn't available.
> @Paul, Do you have any comments on this?

If this is not going to be available at least protect it by

	depends RISCV || COMPILE_TEST
 
> > I wonder if such an instance should be only a single PWM instead of
> > four. Then you were more flexible with the period lengths (using
> > pwmcfg.pwmzerocmp) and could do stuff like inverted and uninverted mode.
> >
> > I didn't understand how the deglitch logic works yet. Currently it is
> > not used which might result in four edges in a single period (which is
> > bad).
> 
> I can enable deglitch logic by just setting a bit (BIT_PWM_DEGLITCH) in
> REG_PWMCFG. Will do that.

This only works for the first pwm output though.

> > > +struct sifive_pwm_device {
> > > +     struct pwm_chip chip;
> > > +     struct notifier_block notifier;
> > > +     struct clk *clk;
> > > +     void __iomem *regs;
> > > +     unsigned int approx_period;

When thinking about this a bit more, I think the better approach would
be to let the consumer change the period iff there is only one consumer.
Then you can drop that approx-period stuff and the result is more
flexible. (Having said that I still prefer making the driver provide
only a single PWM with the ability to have periods other than powers of
two.)

> > > +     writel(frac, pwm->regs + REG_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP);
> >
> > If you get a constant inactive output with frac=0 and a constant active
> > output with frac=0xffff the calculation above seems wrong to me.
> > (A value i written to the pwmcmpX register means a duty cycle of
> > (i * period / 0xffff). Your calculation assumes a divisor of 0x10000
> > however.)
> 
> Not sure if I get you completely. But, if divisor of 0xffff is your concern then
> does the below look ok?
> 
> frac = div_u64(((u64)duty_cycle << 16) - duty_cycle, state->period);

This works (I think, didn't redo the maths), but I would prefer

	frac = div_u64((u64)duty_cycle * 0xffff, state->period);

for better readability. (Maybe the compiler is even clever enough to see
this can be calculated as you suggested.)

> > > +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> > > +                                  unsigned long event, void *data)
> > > +{
> > > +     struct clk_notifier_data *ndata = data;
> > > +     struct sifive_pwm_device *pwm =
> > > +             container_of(nb, struct sifive_pwm_device, notifier);
> > > +
> > > +     if (event == POST_RATE_CHANGE)
> > > +             sifive_pwm_update_clock(pwm, ndata->new_rate);
> >
> > Does this need locking? (Maybe not with the current state.)
> 
> No. We can add it when required.

My thought was, that the clk freq might change while .apply is active.
But given that you only configure the relative duty cycle this is
independent of the actual clk freq.

Best regards
Uwe

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