mbox series

[v2,0/3] Allwinner R329 {R-,}PIO pinctrl support

Message ID 20220710081853.1699028-1-uwu@icenowy.me
Headers show
Series Allwinner R329 {R-,}PIO pinctrl support | expand

Message

Icenowy Zheng July 10, 2022, 8:18 a.m. UTC
This patchset contains support for two pin controllers on Allwinner R329
one, a CPUX one and a CPUS one (the standby processor on R329 is, in
fact, a Xtensa DSP), in addition to their bindings.

Icenowy Zheng (3):
  dt-bindings: pinctrl: document Allwinner R329 PIO and R-PIO
  pinctrl: sunxi: add support for R329 CPUX pin controller
  pinctrl: sunxi: add support for R329 R-PIO pin controller

 .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml  |   4 +
 drivers/pinctrl/sunxi/Kconfig                 |  10 +
 drivers/pinctrl/sunxi/Makefile                |   2 +
 drivers/pinctrl/sunxi/pinctrl-sun50i-r329-r.c | 293 +++++++++++++
 drivers/pinctrl/sunxi/pinctrl-sun50i-r329.c   | 412 ++++++++++++++++++
 5 files changed, 721 insertions(+)
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sun50i-r329-r.c
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sun50i-r329.c

Comments

Andy Shevchenko July 10, 2022, 7:06 p.m. UTC | #1
On Sun, Jul 10, 2022 at 10:22 AM Icenowy Zheng <uwu@icenowy.me> wrote:
>
> Allwinner R329 SoC has two pin controllers similar to ones on previous
> SoCs, one in CPUX power domain and another in CPUS.
>
> This patch adds support for the CPUX domain pin controller.

...

> +#include <linux/module.h>
> +#include <linux/platform_device.h>

> +#include <linux/of.h>
> +#include <linux/of_device.h>

No use of these.

> +#include <linux/pinctrl/pinctrl.h>

Missed headers:
mod_devicetable.h

> +#include "pinctrl-sunxi.h"
Andy Shevchenko July 10, 2022, 7:08 p.m. UTC | #2
On Sun, Jul 10, 2022 at 10:28 AM Icenowy Zheng <uwu@icenowy.me> wrote:
>
> Allwinner R320 SoC has a pin controller in the CPUS power domain.
>
> Add support for it.

> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/reset.h>

Same comment as per previous patch.
Also I forgot to mention kernel.h from which ARRAY_SIZE() is currently
available.

> +#include "pinctrl-sunxi.h"
Icenowy Zheng July 12, 2022, 9:37 a.m. UTC | #3
在 2022-07-10星期日的 21:06 +0200,Andy Shevchenko写道:
> On Sun, Jul 10, 2022 at 10:22 AM Icenowy Zheng <uwu@icenowy.me>
> wrote:
> > 
> > Allwinner R329 SoC has two pin controllers similar to ones on
> > previous
> > SoCs, one in CPUX power domain and another in CPUS.
> > 
> > This patch adds support for the CPUX domain pin controller.
> 
> ...
> 
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> 
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> 
> No use of these.
> 
> > +#include <linux/pinctrl/pinctrl.h>
> 
> Missed headers:
> mod_devicetable.h

Thanks for these.

In addition, how to decide what header should be included? The code
works properly because of_device.h includes mod_devicetable.h.

> 
> > +#include "pinctrl-sunxi.h"
>
Andy Shevchenko July 12, 2022, 10:10 a.m. UTC | #4
On Tue, Jul 12, 2022 at 11:37 AM Icenowy Zheng <uwu@icenowy.me> wrote:
> 在 2022-07-10星期日的 21:06 +0200,Andy Shevchenko写道:
> > On Sun, Jul 10, 2022 at 10:22 AM Icenowy Zheng <uwu@icenowy.me>
> > wrote:

...

> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> >
> > No use of these.
> >
> > > +#include <linux/pinctrl/pinctrl.h>
> >
> > Missed headers:
> > mod_devicetable.h
>
> Thanks for these.
>
> In addition, how to decide what header should be included? The code
> works properly because of_device.h includes mod_devicetable.h.

The best approach is usually learnt with experience — the more you
code, the more you get. One of the rules is to avoid too many
inclusions and at the same time reduce indirect inclusions, so that
the headers which are guaranteed to be included by others shouldn't
appear. Another rule is that, for the headers (*.h files) the forward
declarations are preferable over the inclusion in case if the opaque
pointers are in use.
Andy Shevchenko July 12, 2022, 10:11 a.m. UTC | #5
On Tue, Jul 12, 2022 at 12:10 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jul 12, 2022 at 11:37 AM Icenowy Zheng <uwu@icenowy.me> wrote:
> > 在 2022-07-10星期日的 21:06 +0200,Andy Shevchenko写道:
> > > On Sun, Jul 10, 2022 at 10:22 AM Icenowy Zheng <uwu@icenowy.me>
> > > wrote:
>
> ...
>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > >
> > > No use of these.
> > >
> > > > +#include <linux/pinctrl/pinctrl.h>
> > >
> > > Missed headers:
> > > mod_devicetable.h
> >
> > Thanks for these.
> >
> > In addition, how to decide what header should be included? The code
> > works properly because of_device.h includes mod_devicetable.h.

Forgot to put in the first place this:

The rule of thumb is Include headers that the header or C module is
direct user of. Additional information is located in the sections
below.

> The best approach is usually learnt with experience — the more you
> code, the more you get. One of the rules is to avoid too many
> inclusions and at the same time reduce indirect inclusions, so that
> the headers which are guaranteed to be included by others shouldn't
> appear. Another rule is that, for the headers (*.h files) the forward
> declarations are preferable over the inclusion in case if the opaque
> pointers are in use.