[RFC,2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver

Add pin controller driver for Renesas RZ/A1 SoC.
The SoC driver registers to rz-pfc core module and provides pin
description array and SoC specific pin mux operation.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/pinctrl/rz-pfc/Kconfig        |   7 +
 drivers/pinctrl/rz-pfc/Makefile       |   1 +
 drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 347 ++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+)
 create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rza1.c

Hi Jacopo,

On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add pin controller driver for Renesas RZ/A1 SoC.
> The SoC driver registers to rz-pfc core module and provides pin
> description array and SoC specific pin mux operation.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> --- a/drivers/pinctrl/rz-pfc/Makefile
> +++ b/drivers/pinctrl/rz-pfc/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_PINCTRL_RZ_PINCTRL)               += pinctrl-rz.o
> +obj-$(CONFIG_PINCTRL_RZA1_PINCTRL)             += pinctrl-rza1.o
> diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
> new file mode 100644
> index 0000000..221f048
> --- /dev/null
> +++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c

> +
> +/* ----------------------------------------------------------------------------
> + * SoC operations
> + */
> +
> +static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg,
> +                               int bank, int pin, int set)
> +{
> +       void __iomem *mem = RZA1_ADDR(res->base, reg, bank);
> +       u16 val = set ? ioread16(mem) | 1 << pin :
> +                       ioread16(mem) & ~(1 << pin);
> +#ifdef RZA1_REG_DBG
> +       u16 temp = ioread16(mem);
> +
> +       pr_err("%p %p %p - %4x %4x\n",
> +              (void *)res->start, res->base, mem, temp, val);

Please drop the cast, take the address, and use %pa to format a
resource_size_t, cfr. Documentation/printk-formats.txt.

> + static struct rz_pinctrl_ops rza1_pinctrl_ops = {

const

> +       .set_mux = rza1_set_mux,
> +};
> +
> +static struct rz_pinctrl_info rza1_info = {

const

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Jacopo,


On Wednesday, January 25, 2017, Jacopo Mondi wrote:
> +	/* Port 5 */
> +	RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2),
> +	RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5),
> +	RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8),
> +	RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10),

The RZ/A1L (basically a subset of RZ/A1H to reduce cost) uses
all 16 port pins on "port 5" so I'd like to include them as well.



> +static const struct of_device_id rza1_pinctrl_of_match[] = {
> +	{ .compatible = "renesas,rza1-pinctrl", },
> +	{ }
> +};

Since this PFC driver file is specifically for RZ/A1, I think a
better compatible string would be:

  .compatible = "renesas,r7s72100-renesas-pinctrl",


Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Chris,

On Monday 30 Jan 2017 19:19:18 Chris Brandt wrote:
> On Wednesday, January 25, 2017, Jacopo Mondi wrote:
> > +	/* Port 5 */
> > +	RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2),
> > +	RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5),
> > +	RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8),
> > +	RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10),
> 
> The RZ/A1L (basically a subset of RZ/A1H to reduce cost) uses
> all 16 port pins on "port 5" so I'd like to include them as well.
> 
> > +static const struct of_device_id rza1_pinctrl_of_match[] = {
> > +	{ .compatible = "renesas,rza1-pinctrl", },
> > +	{ }
> > +};
> 
> Since this PFC driver file is specifically for RZ/A1, I think a
> better compatible string would be:
> 
>   .compatible = "renesas,r7s72100-renesas-pinctrl",

Do we need to repeat "renesas" in the name ? And given that the datasheet 
names the hardware "ports", how about "renesas,r7s72100-ports" ? The IP core 
handles both pinctrl and GPIO, so "pinctrl" is a bit restrictive.

Hi Laurent,

On Tuesday, January 31, 2017, Laurent Pinchart wrote:
> On Monday 30 Jan 2017 19:19:18 Chris Brandt wrote:
> > On Wednesday, January 25, 2017, Jacopo Mondi wrote:
> > > +	/* Port 5 */
> > > +	RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2),
> > > +	RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5),
> > > +	RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8),
> > > +	RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10),
> >
> > The RZ/A1L (basically a subset of RZ/A1H to reduce cost) uses all 16
> > port pins on "port 5" so I'd like to include them as well.
> >
> > > +static const struct of_device_id rza1_pinctrl_of_match[] = {
> > > +	{ .compatible = "renesas,rza1-pinctrl", },
> > > +	{ }
> > > +};
> >
> > Since this PFC driver file is specifically for RZ/A1, I think a better
> > compatible string would be:
> >
> >   .compatible = "renesas,r7s72100-renesas-pinctrl",
> 
> Do we need to repeat "renesas" in the name ? And given that the datasheet
> names the hardware "ports", how about "renesas,r7s72100-ports" ? The IP
> core handles both pinctrl and GPIO, so "pinctrl" is a bit restrictive.


Personally, I do not like "renesas,r7s72100-renesas-pinctrl". I was simply
trying to follow the naming guidelines for DT.

I like your suggestion of "renesas,r7s72100-ports" better.


Cheers

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Jacopo,

Thank you for the patch.

On Wednesday 25 Jan 2017 19:09:44 Jacopo Mondi wrote:
> Add pin controller driver for Renesas RZ/A1 SoC.
> The SoC driver registers to rz-pfc core module and provides pin
> description array and SoC specific pin mux operation.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/pinctrl/rz-pfc/Kconfig        |   7 +
>  drivers/pinctrl/rz-pfc/Makefile       |   1 +
>  drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 347 +++++++++++++++++++++++++++++++
>  3 files changed, 355 insertions(+)
>  create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rza1.c
> 
> diff --git a/drivers/pinctrl/rz-pfc/Kconfig b/drivers/pinctrl/rz-pfc/Kconfig
> index 3714c10..2b9c111 100644
> --- a/drivers/pinctrl/rz-pfc/Kconfig
> +++ b/drivers/pinctrl/rz-pfc/Kconfig
> @@ -15,4 +15,11 @@ config PINCTRL_RZ_PINCTRL
>  	help
>  	  This enables pin control drivers for Renesas RZ platforms
> 
> +config PINCTRL_RZA1_PINCTRL
> +    depends on ARCH_R7S72100
> +	select PINCTRL_RZ_PINCTRL
> +	def_bool y
> +	help
> +	  This enables pin control drivers for Renesas RZ/A1 SoC
> +
>  endif
> diff --git a/drivers/pinctrl/rz-pfc/Makefile
> b/drivers/pinctrl/rz-pfc/Makefile index cba8283..e3befa5 100644
> --- a/drivers/pinctrl/rz-pfc/Makefile
> +++ b/drivers/pinctrl/rz-pfc/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_PINCTRL_RZ_PINCTRL)		+= pinctrl-rz.o
> +obj-$(CONFIG_PINCTRL_RZA1_PINCTRL)		+= pinctrl-rza1.o
> diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
> b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c new file mode 100644
> index 0000000..221f048
> --- /dev/null
> +++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c
> @@ -0,0 +1,347 @@
> +/*
> + * Pinctrl support for Renesas RZ/A1 SoC
> + *
> + * Copyright (C) 2017 Jacopo Mondi
> + * Copyright (C) 2017 Renesas Electronics Corporation
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#include <linux/of.h>
> +#include <linux/of_device.h>

You can move those two lines between module.h and platform_device.h, there's 
no need to keep them separate.

> +
> +#include "pinctrl-rz.h"
> +
> +#define RZA1_REG_DBG
> +
> +/* memory resources indexes */
> +#define MEM_RES_LOW	0 /* PORTn_base */
> +#define MEM_RES_HIGH	1 /* PORTn_base + 0x4000 */
> +
> +/* displacements from PORTn_base */
> +#define PMC_REG		0x400
> +#define PFC_REG		0x500
> +#define PFCE_REG	0x600
> +#define PFCEA_REG	0xA00

The kernel mostly uses lowercase for hex values.

> +
> +/* displacements from PORTn_base + 0x4000 */
> +#define PIBC_REG	0x000
> +#define PBDC_REG	0x100
> +#define PIPC_REG	0x200
> +
> +/* build register address using memory base, offset and bank number */
> +#define RZA1_ADDR(mem_base, reg, bank)	\
> +	((mem_base) + (reg) + (bank * 4))

You should put parentheses around bank:

	((mem_base) + (reg) + (bank) * 4)

to avoid side effects if the bank parameter isn't a constant (that's not the 
case here, but it's a good practice anyway).

> +
> +/* ------------------------------------------------------------------------
> + * pin enumeration
> + */
> +enum rza1_pins {
> +	/* Port 0 */
> +	RZ_PIN_NAME(0, 0) = 0, RZ_PIN_NAME(0, 1), RZ_PIN_NAME(0, 2),

Is there a need for = 0 ?

> +	RZ_PIN_NAME(0, 3), RZ_PIN_NAME(0, 4), RZ_PIN_NAME(0, 5),
> +
> +	/* Port 1 */
> +	RZ_PIN_NAME(1, 0), RZ_PIN_NAME(1, 1), RZ_PIN_NAME(1, 2),
> +	RZ_PIN_NAME(1, 3), RZ_PIN_NAME(1, 4), RZ_PIN_NAME(1, 5),
> +	RZ_PIN_NAME(1, 6), RZ_PIN_NAME(1, 7), RZ_PIN_NAME(1, 8),
> +	RZ_PIN_NAME(1, 9), RZ_PIN_NAME(1, 10), RZ_PIN_NAME(1, 11),
> +	RZ_PIN_NAME(1, 12), RZ_PIN_NAME(1, 13), RZ_PIN_NAME(1, 14),
> +	RZ_PIN_NAME(1, 15),
> +
> +	/* Port 2 */
> +	RZ_PIN_NAME(2, 0), RZ_PIN_NAME(2, 1), RZ_PIN_NAME(2, 2),
> +	RZ_PIN_NAME(2, 3), RZ_PIN_NAME(2, 4), RZ_PIN_NAME(2, 5),
> +	RZ_PIN_NAME(2, 6), RZ_PIN_NAME(2, 7), RZ_PIN_NAME(2, 8),
> +	RZ_PIN_NAME(2, 9), RZ_PIN_NAME(2, 10), RZ_PIN_NAME(2, 11),
> +	RZ_PIN_NAME(2, 12), RZ_PIN_NAME(2, 13), RZ_PIN_NAME(2, 14),
> +	RZ_PIN_NAME(2, 15),
> +
> +	/* Port 3 */
> +	RZ_PIN_NAME(3, 0), RZ_PIN_NAME(3, 1), RZ_PIN_NAME(3, 2),
> +	RZ_PIN_NAME(3, 3), RZ_PIN_NAME(3, 4), RZ_PIN_NAME(3, 5),
> +	RZ_PIN_NAME(3, 6), RZ_PIN_NAME(3, 7), RZ_PIN_NAME(3, 8),
> +	RZ_PIN_NAME(3, 9), RZ_PIN_NAME(3, 10), RZ_PIN_NAME(3, 11),
> +	RZ_PIN_NAME(3, 12), RZ_PIN_NAME(3, 13), RZ_PIN_NAME(3, 14),
> +	RZ_PIN_NAME(3, 15),
> +
> +	/* Port 4 */
> +	RZ_PIN_NAME(4, 0), RZ_PIN_NAME(4, 1), RZ_PIN_NAME(4, 2),
> +	RZ_PIN_NAME(4, 3), RZ_PIN_NAME(4, 4), RZ_PIN_NAME(4, 5),
> +	RZ_PIN_NAME(4, 6), RZ_PIN_NAME(4, 7), RZ_PIN_NAME(4, 8),
> +	RZ_PIN_NAME(4, 9), RZ_PIN_NAME(4, 10), RZ_PIN_NAME(4, 11),
> +	RZ_PIN_NAME(4, 12), RZ_PIN_NAME(4, 13), RZ_PIN_NAME(4, 14),
> +	RZ_PIN_NAME(4, 15),
> +
> +	/* Port 5 */
> +	RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2),
> +	RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5),
> +	RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8),
> +	RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10),
> +
> +	/* Port 6 */
> +	RZ_PIN_NAME(6, 0), RZ_PIN_NAME(6, 1), RZ_PIN_NAME(6, 2),
> +	RZ_PIN_NAME(6, 3), RZ_PIN_NAME(6, 4), RZ_PIN_NAME(6, 5),
> +	RZ_PIN_NAME(6, 6), RZ_PIN_NAME(6, 7), RZ_PIN_NAME(6, 8),
> +	RZ_PIN_NAME(6, 9), RZ_PIN_NAME(6, 10), RZ_PIN_NAME(6, 11),
> +	RZ_PIN_NAME(6, 12), RZ_PIN_NAME(6, 13), RZ_PIN_NAME(6, 14),
> +	RZ_PIN_NAME(6, 15),
> +
> +	/* Port 7 */
> +	RZ_PIN_NAME(7, 0), RZ_PIN_NAME(7, 1), RZ_PIN_NAME(7, 2),
> +	RZ_PIN_NAME(7, 3), RZ_PIN_NAME(7, 4), RZ_PIN_NAME(7, 5),
> +	RZ_PIN_NAME(7, 6), RZ_PIN_NAME(7, 7), RZ_PIN_NAME(7, 8),
> +	RZ_PIN_NAME(7, 9), RZ_PIN_NAME(7, 10), RZ_PIN_NAME(7, 11),
> +	RZ_PIN_NAME(7, 12), RZ_PIN_NAME(7, 13), RZ_PIN_NAME(7, 14),
> +	RZ_PIN_NAME(7, 15),
> +
> +	/* Port 8 */
> +	RZ_PIN_NAME(8, 0), RZ_PIN_NAME(8, 1), RZ_PIN_NAME(8, 2),
> +	RZ_PIN_NAME(8, 3), RZ_PIN_NAME(8, 4), RZ_PIN_NAME(8, 5),
> +	RZ_PIN_NAME(8, 6), RZ_PIN_NAME(8, 7), RZ_PIN_NAME(8, 8),
> +	RZ_PIN_NAME(8, 9), RZ_PIN_NAME(8, 10), RZ_PIN_NAME(8, 11),
> +	RZ_PIN_NAME(8, 12), RZ_PIN_NAME(8, 13), RZ_PIN_NAME(8, 14),
> +	RZ_PIN_NAME(8, 15),
> +
> +	/* Port 9 */
> +	RZ_PIN_NAME(9, 0), RZ_PIN_NAME(9, 1), RZ_PIN_NAME(9, 2),
> +	RZ_PIN_NAME(9, 3), RZ_PIN_NAME(9, 4), RZ_PIN_NAME(9, 5),
> +	RZ_PIN_NAME(9, 6), RZ_PIN_NAME(9, 7),
> +
> +	/* Port 10 */
> +	RZ_PIN_NAME(10, 0), RZ_PIN_NAME(10, 1), RZ_PIN_NAME(10, 2),
> +	RZ_PIN_NAME(10, 3), RZ_PIN_NAME(10, 4), RZ_PIN_NAME(10, 5),
> +	RZ_PIN_NAME(10, 6), RZ_PIN_NAME(10, 7), RZ_PIN_NAME(10, 8),
> +	RZ_PIN_NAME(10, 9), RZ_PIN_NAME(10, 10), RZ_PIN_NAME(10, 11),
> +	RZ_PIN_NAME(10, 12), RZ_PIN_NAME(10, 13), RZ_PIN_NAME(10, 14),
> +	RZ_PIN_NAME(10, 15),
> +
> +	/* Port 10 */

s/Port 10/Port 11/

> +	RZ_PIN_NAME(11, 0), RZ_PIN_NAME(11, 1), RZ_PIN_NAME(11, 2),
> +	RZ_PIN_NAME(11, 3), RZ_PIN_NAME(11, 4), RZ_PIN_NAME(11, 5),
> +	RZ_PIN_NAME(11, 6), RZ_PIN_NAME(11, 7), RZ_PIN_NAME(11, 8),
> +	RZ_PIN_NAME(11, 9), RZ_PIN_NAME(11, 10), RZ_PIN_NAME(11, 11),
> +	RZ_PIN_NAME(11, 12), RZ_PIN_NAME(11, 13), RZ_PIN_NAME(11, 14),
> +	RZ_PIN_NAME(11, 15),
> +};
> +
> +struct rz_pin_desc pins[] = {
> +	/* Port 0 */
> +	RZ_PIN_DESC(0, 0), RZ_PIN_DESC(0, 1), RZ_PIN_DESC(0, 2),
> +	RZ_PIN_DESC(0, 3), RZ_PIN_DESC(0, 4), RZ_PIN_DESC(0, 5),
> +
> +	/* Port 1 */
> +	RZ_PIN_DESC(1, 0), RZ_PIN_DESC(1, 1), RZ_PIN_DESC(1, 2),
> +	RZ_PIN_DESC(1, 3), RZ_PIN_DESC(1, 4), RZ_PIN_DESC(1, 5),
> +	RZ_PIN_DESC(1, 6), RZ_PIN_DESC(1, 7), RZ_PIN_DESC(1, 8),
> +	RZ_PIN_DESC(1, 9), RZ_PIN_DESC(1, 10), RZ_PIN_DESC(1, 11),
> +	RZ_PIN_DESC(1, 12), RZ_PIN_DESC(1, 13), RZ_PIN_DESC(1, 14),
> +	RZ_PIN_DESC(1, 15),
> +
> +	/* Port 2 */
> +	RZ_PIN_DESC(2, 0), RZ_PIN_DESC(2, 1), RZ_PIN_DESC(2, 2),
> +	RZ_PIN_DESC(2, 3), RZ_PIN_DESC(2, 4), RZ_PIN_DESC(2, 5),
> +	RZ_PIN_DESC(2, 6), RZ_PIN_DESC(2, 7), RZ_PIN_DESC(2, 8),
> +	RZ_PIN_DESC(2, 9), RZ_PIN_DESC(2, 10), RZ_PIN_DESC(2, 11),
> +	RZ_PIN_DESC(2, 12), RZ_PIN_DESC(2, 13), RZ_PIN_DESC(2, 14),
> +	RZ_PIN_DESC(2, 15),
> +
> +	/* Port 3 */
> +	RZ_PIN_DESC(3, 0), RZ_PIN_DESC(3, 1), RZ_PIN_DESC(3, 2),
> +	RZ_PIN_DESC(3, 3), RZ_PIN_DESC(3, 4), RZ_PIN_DESC(3, 5),
> +	RZ_PIN_DESC(3, 6), RZ_PIN_DESC(3, 7), RZ_PIN_DESC(3, 8),
> +	RZ_PIN_DESC(3, 9), RZ_PIN_DESC(3, 10), RZ_PIN_DESC(3, 11),
> +	RZ_PIN_DESC(3, 12), RZ_PIN_DESC(3, 13), RZ_PIN_DESC(3, 14),
> +	RZ_PIN_DESC(3, 15),
> +
> +	/* Port 4 */
> +	RZ_PIN_DESC(4, 0), RZ_PIN_DESC(4, 1), RZ_PIN_DESC(4, 2),
> +	RZ_PIN_DESC(4, 3), RZ_PIN_DESC(4, 4), RZ_PIN_DESC(4, 5),
> +	RZ_PIN_DESC(4, 6), RZ_PIN_DESC(4, 7), RZ_PIN_DESC(4, 8),
> +	RZ_PIN_DESC(4, 9), RZ_PIN_DESC(4, 10), RZ_PIN_DESC(4, 11),
> +	RZ_PIN_DESC(4, 12), RZ_PIN_DESC(4, 13), RZ_PIN_DESC(4, 14),
> +	RZ_PIN_DESC(4, 15),
> +
> +	/* Port 5 */
> +	RZ_PIN_DESC(5, 0), RZ_PIN_DESC(5, 1), RZ_PIN_DESC(5, 2),
> +	RZ_PIN_DESC(5, 3), RZ_PIN_DESC(5, 4), RZ_PIN_DESC(5, 5),
> +	RZ_PIN_DESC(5, 6), RZ_PIN_DESC(5, 7), RZ_PIN_DESC(5, 8),
> +	RZ_PIN_DESC(5, 9), RZ_PIN_DESC(5, 10),
> +
> +	/* Port 6 */
> +	RZ_PIN_DESC(6, 0), RZ_PIN_DESC(6, 1), RZ_PIN_DESC(6, 2),
> +	RZ_PIN_DESC(6, 3), RZ_PIN_DESC(6, 4), RZ_PIN_DESC(6, 5),
> +	RZ_PIN_DESC(6, 6), RZ_PIN_DESC(6, 7), RZ_PIN_DESC(6, 8),
> +	RZ_PIN_DESC(6, 9), RZ_PIN_DESC(6, 10), RZ_PIN_DESC(6, 11),
> +	RZ_PIN_DESC(6, 12), RZ_PIN_DESC(6, 13), RZ_PIN_DESC(6, 14),
> +	RZ_PIN_DESC(6, 15),
> +
> +	/* Port 7 */
> +	RZ_PIN_DESC(7, 0), RZ_PIN_DESC(7, 1), RZ_PIN_DESC(7, 2),
> +	RZ_PIN_DESC(7, 3), RZ_PIN_DESC(7, 4), RZ_PIN_DESC(7, 5),
> +	RZ_PIN_DESC(7, 6), RZ_PIN_DESC(7, 7), RZ_PIN_DESC(7, 8),
> +	RZ_PIN_DESC(7, 9), RZ_PIN_DESC(7, 10), RZ_PIN_DESC(7, 11),
> +	RZ_PIN_DESC(7, 12), RZ_PIN_DESC(7, 13), RZ_PIN_DESC(7, 14),
> +	RZ_PIN_DESC(7, 15),
> +
> +	/* Port 8 */
> +	RZ_PIN_DESC(8, 0), RZ_PIN_DESC(8, 1), RZ_PIN_DESC(8, 2),
> +	RZ_PIN_DESC(8, 3), RZ_PIN_DESC(8, 4), RZ_PIN_DESC(8, 5),
> +	RZ_PIN_DESC(8, 6), RZ_PIN_DESC(8, 7), RZ_PIN_DESC(8, 8),
> +	RZ_PIN_DESC(8, 9), RZ_PIN_DESC(8, 10), RZ_PIN_DESC(8, 11),
> +	RZ_PIN_DESC(8, 12), RZ_PIN_DESC(8, 13), RZ_PIN_DESC(8, 14),
> +	RZ_PIN_DESC(8, 15),
> +
> +	/* Port 9 */
> +	RZ_PIN_DESC(9, 0), RZ_PIN_DESC(9, 1), RZ_PIN_DESC(9, 2),
> +	RZ_PIN_DESC(9, 3), RZ_PIN_DESC(9, 4), RZ_PIN_DESC(9, 5),
> +	RZ_PIN_DESC(9, 6), RZ_PIN_DESC(9, 7),
> +
> +	/* Port 10 */
> +	RZ_PIN_DESC(10, 0), RZ_PIN_DESC(10, 1), RZ_PIN_DESC(10, 2),
> +	RZ_PIN_DESC(10, 3), RZ_PIN_DESC(10, 4), RZ_PIN_DESC(10, 5),
> +	RZ_PIN_DESC(10, 6), RZ_PIN_DESC(10, 7), RZ_PIN_DESC(10, 8),
> +	RZ_PIN_DESC(10, 9), RZ_PIN_DESC(10, 10), RZ_PIN_DESC(10, 11),
> +	RZ_PIN_DESC(10, 12), RZ_PIN_DESC(10, 13), RZ_PIN_DESC(10, 14),
> +	RZ_PIN_DESC(10, 15),
> +
> +	/* Port 11 */
> +	RZ_PIN_DESC(11, 0), RZ_PIN_DESC(11, 1), RZ_PIN_DESC(11, 2),
> +	RZ_PIN_DESC(11, 3), RZ_PIN_DESC(11, 4), RZ_PIN_DESC(11, 5),
> +	RZ_PIN_DESC(11, 6), RZ_PIN_DESC(11, 7), RZ_PIN_DESC(11, 8),
> +	RZ_PIN_DESC(11, 9), RZ_PIN_DESC(11, 10), RZ_PIN_DESC(11, 11),
> +	RZ_PIN_DESC(11, 12), RZ_PIN_DESC(11, 13), RZ_PIN_DESC(11, 14),
> +	RZ_PIN_DESC(11, 15),
> +};
> +
> +/* ------------------------------------------------------------------------
> + * SoC operations
> + */
> +
> +static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg,
> +				int bank, int pin, int set)
> +{
> +	void __iomem *mem = RZA1_ADDR(res->base, reg, bank);
> +	u16 val = set ? ioread16(mem) | 1 << pin :
> +			ioread16(mem) & ~(1 << pin);

Is the compiler smart enough to avoid two calls to ioread16() ? If not, how 
about

	u16 val = ioread16(mem);

	if (set)
		val |= 1 << pin;
	else
		val &= ~(1 << pin);

> +#ifdef RZA1_REG_DBG
> +	u16 temp = ioread16(mem);
> +
> +	pr_err("%p %p %p - %4x %4x\n",
> +	       (void *)res->start, res->base, mem, temp, val);
> +#endif

Do you really need this in the driver, or was it useful during initial 
development only ?

> +	iowrite16(val, mem);

Is there a lock in the call stack that prevents a race condition here ? If not 
I'd add a spinlock around the read and write.

> +}
> +
> +/**
> + * rza1_set_mux() - Configure alternate function settings for the selected
> pin
> + *
> + * @pinctrl: RZ pincontroller
> + * @pin: Pin to configure

The parameter is named pin_desc. 

> + * @mux_mode: Alternate function number
> + *
> + * @return: 0 for success; != 0 otherwise
> + */
> +static int rza1_set_mux(struct rz_pinctrl_dev *pinctrl,
> +			struct rz_pin_desc *pin_desc, unsigned int mux_mode)
> +{
> +	struct rz_pinctrl_res *res;
> +	unsigned int bank = pin_desc->bank,
> +		     pin = pin_desc->pin;

One declaration per line please, especially when initializing the variables at 
declaration time.

	unsigned int bank = pin_desc->bank;
	unsigned int pin = pin_desc->pin;

> +
> +	/*
> +	 * disable input buffer and bi-control direction before entering
> +	 * alternate mode and let alternate function drive the IO mode by
> +	 * setting PIPCn to 1

Sentences start with a capital letter and end with a period. Or a smiley in e-
mails, but usually not in driver code ;-) This comment applies to the whole 
patch series.

> +	 */
> +	res = &pinctrl->res[MEM_RES_HIGH];
> +	rza1_set_bit(res, PIBC_REG, bank, pin, 0);
> +	rza1_set_bit(res, PBDC_REG, bank, pin, 0);
> +	rza1_set_bit(res, PIPC_REG, bank, pin, 1);
> +
> +	/* TODO:
> +	 * all alternate functions except a few (3) need PIPCn = 1;
> +	 * find a way to identify those 3 functions, do not set PIPCn to 1
> +	 * and set PMn according to some flag passed as parameter from DTS

Nitpicking, I'd say "from DT", DTS is the source format.

> +	 */
> +
> +	/*
> +	 * enable alternate function mode and select it.
> +	 *
> +	 * ----------------------------------------------------
> +	 * Alternate mode selection table:
> +	 *
> +	 * PMC	PFC	PFCE	PFCAE	AlternateMode mux_mode
> +	 * 1	0	0	0	1		0
> +	 * 1	1	0	0	2		1
> +	 * 1	0	1	0	3		2
> +	 * 1	1	1	0	4		3
> +	 * 1	0	0	1	5		4
> +	 * 1	1	0	1	6		5
> +	 * 1	0	1	1	7		6
> +	 * 1	1	1	1	8		7
> +	 * ----------------------------------------------------
> +	 */
> +	res = &pinctrl->res[MEM_RES_LOW];
> +	rza1_set_bit(res, PMC_REG, bank, pin, 1);
> +	rza1_set_bit(res, PFC_REG, bank, pin, mux_mode & 0x1);
> +	rza1_set_bit(res, PFCE_REG, bank, pin, mux_mode & 0x2);
> +	rza1_set_bit(res, PFCEA_REG, bank, pin, mux_mode & 0x4);
> +
> +	return 0;
> +}
> +
> +static struct rz_pinctrl_ops rza1_pinctrl_ops = {
> +	.set_mux = rza1_set_mux,
> +};
> +
> +static struct rz_pinctrl_info rza1_info = {
> +	.ops = &rza1_pinctrl_ops,
> +
> +	.npins = ARRAY_SIZE(pins),
> +	.pins = pins,
> +};

You should make those structures static const (which will require updating 
patch 1/5). It's especially important to make ops structures const for 
security reasons.

> +static const struct of_device_id rza1_pinctrl_of_match[] = {
> +	{ .compatible = "renesas,rza1-pinctrl", },
> +	{ }
> +};

Drivers usually have a corresponding MODULE_DEVICE_TABLE(). As this code can't 
be compiled as a module I suppose it doesn't matter much.

> +static int rza1_pinctrl_probe(struct platform_device *pdev)
> +{
> +	return rz_pinctrl_probe(pdev, &rza1_info);
> +}
> +
> +static int rza1_pinctrl_remove(struct platform_device *pdev)
> +{
> +	rz_pinctrl_remove(pdev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver rza1_pinctrl_driver = {
> +	.driver = {
> +		.name = "rza1_pinctrl_driver",

No need for "_driver" here, "rza1_pinctrl" will do.

I'm also never sure whether we should use _ or - in driver names.

> +		.of_match_table = rza1_pinctrl_of_match,
> +	},
> +	.probe = rza1_pinctrl_probe,
> +	.remove = rza1_pinctrl_remove,
> +};
> +
> +static int __init rza1_pinctrl_init(void)
> +{
> +	return platform_driver_register(&rza1_pinctrl_driver);
> +}
> +core_initcall(rza1_pinctrl_init);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
> +MODULE_DESCRIPTION("Pinctl driver for Reneas RZ/A1 SoC");

s/Pinctl/Pinctrl/ (or better, "Pin controller" ?)

> +MODULE_LICENSE("GPL v2");