Re: [PATCH v2] pwm: sunxi: Add Allwinner SoC PWM controller driver

[Maxime Ripard]

Hi,

On Thu, Feb 04, 2021 at 11:47:34AM +0800, 班涛 wrote:
> Maxime Ripard <maxime@xxxxxxxxxx> 于2021年2月3日周三 下午11:46写道：
> 
> > Hi,
> >
> > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> > > From: Ban Tao <fengzheng923@xxxxxxxxx>
> > >
> > > The Allwinner R818, A133, R329, V536 and V833 has a new PWM controller
> > > IP compared to the older Allwinner SoCs.
> > >
> > > Signed-off-by: Ban Tao <fengzheng923@xxxxxxxxx>
> >
> > Thanks for your patch. There's a bunch of warnings reported by
> > checkpatch --strict, they should be addressed.
> >
> > > ---
> > > v1->v2:
> > > 1.delete unnecessary code.
> > > 2.using a named define for some constants.
> > > 3.Add comment in sun50i_pwm_config function.
> > > 4.using dev_err_probe() for error handling.
> > > 5.disable the clock after pwmchip_remove().
> > > ---
> > >  MAINTAINERS              |   6 +
> > >  drivers/pwm/Kconfig      |  11 ++
> > >  drivers/pwm/Makefile     |   1 +
> > >  drivers/pwm/pwm-sun50i.c | 348 +++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 366 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-sun50i.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index e73636b75f29..d33cf1b69b43 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -737,6 +737,12 @@ L:       linux-media@xxxxxxxxxxxxxxx
> > >  S:   Maintained
> > >  F:   drivers/staging/media/sunxi/cedrus/
> > >
> > > +ALLWINNER PWM DRIVER
> > > +M:   Ban Tao <fengzheng923@xxxxxxxxx>
> > > +L:   linux-pwm@xxxxxxxxxxxxxxx
> > > +S:   Maintained
> > > +F:   drivers/pwm/pwm-sun50i.c
> > > +
> > >  ALPHA PORT
> > >  M:   Richard Henderson <rth@xxxxxxxxxxx>
> > >  M:   Ivan Kokshaysky <ink@xxxxxxxxxxxxxxxxxxxx>
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 9a4f66ae8070..17635a8f2ed3 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -552,6 +552,17 @@ config PWM_SUN4I
> > >         To compile this driver as a module, choose M here: the module
> > >         will be called pwm-sun4i.
> > >
> > > +config PWM_SUN50I
> > > +     tristate "Allwinner enhanced PWM support"
> > > +     depends on ARCH_SUNXI || COMPILE_TEST
> > > +     depends on HAS_IOMEM && COMMON_CLK
> > > +     help
> > > +       Enhanced PWM framework driver for Allwinner R818, A133, R329,
> > > +       V536 and V833 SoCs.
> > > +
> > > +       To compile this driver as a module, choose M here: the module
> > > +       will be called pwm-sun50i.
> > > +
> >
> > Even though it's unfortunate, there's a bunch of other SoCs part of the
> > sun50i family that are supported by the sun4i driver.
> >
> > Which SoC introduced that new design? It's usually the name we pick up
> > then.
> >
> 
> In fact, some SoCs has not been supported by the sun4i driver, such as v833,
> v536, r818, a133 and r329.
> v833 and v536 are part of the sun8i family, but r818, a133 and r329 are
> part of the sun50i family.

Indeed, I missed that sorry. 

> So,I'm confused, how do choose the name of this driver?

Just go for the earliest SoC that introduced that design. What was the
first SoC to use it?

> > > +static const struct sun50i_pwm_data sun8i_pwm_data_c9 = {
> > > +     .npwm = 9,
> > > +};
> > > +
> > > +static const struct sun50i_pwm_data sun50i_pwm_data_c16 = {
> > > +     .npwm = 16,
> > > +};
> > > +
> > > +static const struct of_device_id sun50i_pwm_dt_ids[] = {
> > > +     {
> > > +             .compatible = "allwinner,sun8i-v833-pwm",
> > > +             .data = &sun8i_pwm_data_c9,
> > > +     }, {
> > > +             .compatible = "allwinner,sun8i-v536-pwm",
> > > +             .data = &sun8i_pwm_data_c9,
> > > +     }, {
> > > +             .compatible = "allwinner,sun50i-r818-pwm",
> > > +             .data = &sun50i_pwm_data_c16,
> > > +     }, {
> > > +             .compatible = "allwinner,sun50i-a133-pwm",
> > > +             .data = &sun50i_pwm_data_c16,
> > > +     }, {
> > > +             .compatible = "allwinner,sun50i-r329-pwm",
> > > +             .data = &sun8i_pwm_data_c9,
> > > +     }, {
> > > +             /* sentinel */
> > > +     },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, sun50i_pwm_dt_ids);
> >
> > What are the differences between all these SoCs? If there's none between
> > the v833, v536 and R329, and between the r818 and the A133, you should
> > use the same compatible.
> 
> Compared with the sun4i driver, this patch is a completely different PWM IP
> controller.

Sure, I didn't mean to compare it to sun4i. What I meant was that as far
as these struct goes, the A133 and the R818 look to have the same PWM
controller. Just like the v833, v536 and R329.

If the PWM controllers are indeed identical across those SoCs, you can
just use two compatibles, one for the PWM with 9 channels (again, the
earliest SoC among the V833, v536 and r329), and one for the PWM with 16
channels.

None of those SoCs are supported at the moment in Linux though, so it
would make more sense to support them first before adding a new driver
for those SoCs, it's gonna be dead code otherwise.

Maxime