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

[Laurent Pinchart]

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 
> ---
>  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 000..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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 

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_REG0xA00

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, 

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

[Chris Brandt]

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

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

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

[Chris Brandt]

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

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

[Geert Uytterhoeven]

Hi Jacopo,

On Wed, Jan 25, 2017 at 7:09 PM, 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 

> --- 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 000..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 -- ge...@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