Re: [PATCH v5 2/4] pwm: starfive: Add PWM driver support

[Thierry Reding]

On Mon, Sep 25, 2023 at 06:27:16PM +0800, William Qiu wrote:
> 
> 
> On 2023/9/23 20:08, Emil Renner Berthing wrote:
> > William Qiu wrote:
> >> Add Pulse Width Modulation driver support for StarFive
> >> JH7100 and JH7110 SoC.
> >>
> >> Co-developed-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> >> ---
> >>  MAINTAINERS                |   7 ++
> >>  drivers/pwm/Kconfig        |   9 ++
> >>  drivers/pwm/Makefile       |   1 +
> >>  drivers/pwm/pwm-starfive.c | 190 +++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 207 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-starfive.c
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index bf0f54c24f81..bc2155bd2712 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -20495,6 +20495,13 @@ F:	drivers/pinctrl/starfive/pinctrl-starfive-jh71*
> >>  F:	include/dt-bindings/pinctrl/pinctrl-starfive-jh7100.h
> >>  F:	include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> >>
> >> +STARFIVE JH71X0 PWM DRIVERS
> >> +M:	William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> >> +M:	Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> >> +S:	Supported
> >> +F:	Documentation/devicetree/bindings/pwm/starfive,jh7100-pwm.yaml
> >> +F:	drivers/pwm/pwm-starfive-ptc.c
> >> +
> >>  STARFIVE JH71X0 RESET CONTROLLER DRIVERS
> >>  M:	Emil Renner Berthing <kernel@xxxxxxxx>
> >>  M:	Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index 8ebcddf91f7b..e2ee0169f6e4 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -569,6 +569,15 @@ config PWM_SPRD
> >>  	  To compile this driver as a module, choose M here: the module
> >>  	  will be called pwm-sprd.
> >>
> >> +config PWM_STARFIVE
> >> +	tristate "StarFive PWM support"
> >> +	depends on ARCH_STARFIVE || COMPILE_TEST
> >> +	help
> >> +	  Generic PWM framework driver for StarFive SoCs.
> >> +
> >> +	  To compile this driver as a module, choose M here: the module
> >> +	  will be called pwm-starfive.
> >> +
> >>  config PWM_STI
> >>  	tristate "STiH4xx PWM support"
> >>  	depends on ARCH_STI || COMPILE_TEST
> >> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> >> index c822389c2a24..93b954376873 100644
> >> --- a/drivers/pwm/Makefile
> >> +++ b/drivers/pwm/Makefile
> >> @@ -52,6 +52,7 @@ obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> >>  obj-$(CONFIG_PWM_SL28CPLD)	+= pwm-sl28cpld.o
> >>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
> >>  obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
> >> +obj-$(CONFIG_PWM_STARFIVE)	+= pwm-starfive.o
> >>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
> >>  obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
> >>  obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
> >> diff --git a/drivers/pwm/pwm-starfive.c b/drivers/pwm/pwm-starfive.c
> > 
> > Hi William,
> > 
> > You never answered my questions about what PTC is short for and if there are
> > other PWMs on the JH7110. You just removed -ptc from the name of this file..
> > 
> Hi Emil,
> 
> The PTC, short for PWM/TIMER/CONUTER, comes from OpenCore's ip, but only PWM
> mode is used in the JH7110. So the register still has the word "PTC".
> s the best way to change all the prefix to STARFIVE?

This is the first time I see mentioned that this is based on an Open-
Cores IP. It's definitely something you want to note somewhere so that
others can reuse this driver if they've incorporated the same IP into
their device.

Given the above it might be better to name this something different
entirely. The original OpenCores PTC IP seems to be single-instance,
but that's about the only difference here (i.e. the OpenCores IP lists
one clock and one reset, which this driver supports).

So it'd be easy to turn this into a generic OpenCores driver and then
use the starfive compatible string(s) to parameterize (number of
instances, register stride, etc.).

Thierry

Attachment: signature.asc
Description: PGP signature