Re: [PATCH 3/3] pinctrl: renesas: Add pins/groups/functions for I2C, SCIF and USB supported by RZ/G2L SoC

[Geert Uytterhoeven]

Hi Prabhakar,

On Wed, Jun 16, 2021 at 3:27 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> Add pins/groups/functions for I2C, SCIF and USB supported by RZ/G2L SoC and
> bind it with RZ/G2L PFC core.
>
> Based on a patch in the BSP by Hien Huynh <hien.huynh.px@xxxxxxxxxxx>.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/pinctrl/renesas/pfc-r9a07g044.c
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * R9A07G044 processor support - pinctrl GPIO hardware block.
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + */
> +
> +#include "pinctrl-rzg2l.h"
> +
> +#define RZG2L_GPIO_PIN_CONF    (0)
> +
> +static const struct {
> +       struct pinctrl_pin_desc pin_gpio[392];
> +} pinmux_pins = {
> +       .pin_gpio = {
> +               RZ_G2L_PINCTRL_PIN_GPIO(0, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(1, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(2, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(3, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(4, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(5, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(6, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(7, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(8, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(9, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(10, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(11, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(12, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(13, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(14, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(15, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(16, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(17, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(18, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(19, RZG2L_GPIO_PIN_CONF),

RZG2L_GPIO_PIN_CONF is 0, ike all of the below?

> +               RZ_G2L_PINCTRL_PIN_GPIO(20, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(21, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(22, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(23, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(24, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(25, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(26, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(27, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(28, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(29, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(30, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(31, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(32, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(33, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(34, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(35, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(36, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(37, 0),
> +               RZ_G2L_PINCTRL_PIN_GPIO(38, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(39, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(40, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(41, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(42, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(43, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(44, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(45, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(46, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(47, RZG2L_GPIO_PIN_CONF),
> +               RZ_G2L_PINCTRL_PIN_GPIO(48, RZG2L_GPIO_PIN_CONF),
> +       },
> +};

Doesn't the above belong in pinctrl-rzg2l.c?

> +
> +/* - RIIC2 ------------------------------------------------------------------ */
> +static int i2c2_a_pins[] = {
> +       /* SDA, SCL */
> +       RZ_G2L_PIN(3, 0), RZ_G2L_PIN(3, 1),
> +};
> +static int i2c2_b_pins[] = {
> +       /* SDA, SCL */
> +       RZ_G2L_PIN(19, 0), RZ_G2L_PIN(19, 1),
> +};
> +static int i2c2_c_pins[] = {
> +       /* SDA, SCL */
> +       RZ_G2L_PIN(42, 3), RZ_G2L_PIN(42, 4),
> +};
> +static int i2c2_d_pins[] = {
> +       /* SDA, SCL */
> +       RZ_G2L_PIN(46, 0), RZ_G2L_PIN(46, 1),
> +};
> +static int i2c2_e_pins[] = {
> +       /* SDA, SCL */
> +       RZ_G2L_PIN(48, 0), RZ_G2L_PIN(48, 1),
> +};

[...]

> +static struct group_desc pinmux_groups[] = {
> +       RZ_G2L_PINCTRL_PIN_GROUP(i2c2_a, 2),
> +       RZ_G2L_PINCTRL_PIN_GROUP(i2c2_b, 4),
> +       RZ_G2L_PINCTRL_PIN_GROUP(i2c2_c, 1),
> +       RZ_G2L_PINCTRL_PIN_GROUP(i2c2_d, 4),
> +       RZ_G2L_PINCTRL_PIN_GROUP(i2c2_e, 3),

[...]

As RZ/G2L, unlike R-Car, does not have the concept of pin groups, I'm
wondering why you are defining these groups? The pin function list
spreadsheet also doesn't have the "a" to "e" names of the possible
alternatives.
While I agree it makes it a little bit easier to describe in DT the
use of a group with lots of pins, it does prevent other use cases.
As register configuration is per-pin, I believe the hardware supports
the use of pins from multiple groups (e.g. SDA from the first group,
and SCL from the second group), and thus the board designer may decide
to make use of that.

With pinmux_pins[] moved, and the groups removed, this file becomes
empty?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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