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

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 6373 bytes --]

On Sat, Feb 06, 2021 at 09:27:30PM +0800, 班涛 wrote:
> Maxime Ripard <maxime@cerno.tech> 于2021年2月6日周六 上午12:12写道：
> 
> > Hi,
> >
> > On Thu, Feb 04, 2021 at 11:47:34AM +0800, 班涛 wrote:
> > > Maxime Ripard <maxime@cerno.tech> 于2021年2月3日周三 下午11:46写道：
> > >
> > > > Hi,
> > > >
> > > > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> > > > > From: Ban Tao <fengzheng923@gmail.com>
> > > > >
> > > > > 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@gmail.com>
> > > >
> > > > 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@vger.kernel.org
> > > > >  S:   Maintained
> > > > >  F:   drivers/staging/media/sunxi/cedrus/
> > > > >
> > > > > +ALLWINNER PWM DRIVER
> > > > > +M:   Ban Tao <fengzheng923@gmail.com>
> > > > > +L:   linux-pwm@vger.kernel.org
> > > > > +S:   Maintained
> > > > > +F:   drivers/pwm/pwm-sun50i.c
> > > > > +
> > > > >  ALPHA PORT
> > > > >  M:   Richard Henderson <rth@twiddle.net>
> > > > >  M:   Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> > > > > 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?
> >
>
> The V536 SOC first used this design, so, we should use the name
> "sun8i-pwm.c"?

sun8i-pwm would be too generic, but sun8i-v536-pwm would be great then

> 
> > > > > +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.
> >
> Ok i see what you mean.
>  static const struct of_device_id sun50i_pwm_dt_ids[] = {
>         {
>                 .compatible = "allwinner,sun8i-v536-pwm",
>                 .data = &sun8i_pwm_data_c9,
>         }, {
>                 .compatible = "allwinner,sun50i-r818-pwm",
>                 .data = &sun50i_pwm_data_c16,
>         }, {
>                 /* sentinel */
>         },
> };
> is this OK? Just write two SoC for the " compatible "？

Yes, this is perfect :)

Maxime

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