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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4236 bytes --]

On Tue, Jan 15, 2019 at 11:00:46PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> 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.)
> 
> > +	help
> > +	  Generic PWM framework driver for SiFive SoCs.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-sifive.
> > +
> >  config PWM_SPEAR
> >  	tristate "STMicroelectronics SPEAr PWM support"
> >  	depends on PLAT_SPEAR
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 9c676a0..30089ca 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
> >  obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
> >  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
> >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> > +obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> >  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
> >  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
> >  obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
> > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > new file mode 100644
> > index 0000000..7fee809
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sifive.c
> > @@ -0,0 +1,246 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017-2018 SiFive
> > + * For SiFive's PWM IP block documentation please refer Chapter 14 of
> > + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> 
> 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 thought this IP only allowed a single period for all PWM channels in
the IP. If so, splitting this into four different devices is going to
complicate things because you'd have to coordinate between all four as
to which period is currently set.

> > +struct sifive_pwm_device {
> > +	struct pwm_chip	chip;
> > +	struct notifier_block notifier;
> > +	struct clk *clk;
> > +	void __iomem *regs;
> > +	unsigned int approx_period;
> > +	unsigned int real_period;
> > +};
> 
> I'd call this pwm_sifive_ddata. The prefix because the driver is called
> pwm-sifive and ddata because this is driver data and not a device.

I don't think there's a need to have an extra suffix. Just call this
sifive_pwm or pwm_sifive. There's no ambiguity in that name and it's
short and crisp.

> > +	if (state->enabled)
> > +		sifive_pwm_get_state(chip, dev, state);
> 
> @Thierry: Should we bless this correction of state?

I'm not sure I understand why this correction is necessary. Is it okay
to request a state to be applied and when we're not able to set that
state we just set anything as close as possible? Sounds a bit risky to
me. What if somebody wants to use this in a case where precision
matters?

Now, if you're saying that there can't be such cases and we should
support this, then why restrict the state correction to when the PWM is
enabled? What's wrong with correcting it in either case?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv