Re: [PATCH 2/2] gpio: rcar: Add R-Car X5H (R8A78000) support

[Geert Uytterhoeven]

Hi Marek,

On Sat, 4 Jul 2026 at 17:15, Marek Vasut
<marek.vasut+renesas@xxxxxxxxxxx> wrote:
> R-Car X5H (R8A78000) is the first member of the R-Car Gen5 family.
> Add support for R-Car X5H, which has slightly different GPIO block
> register layout compared to previous generations. Introduce offset
> remap function which performs 1:1 remap for R-Car Gen1..4 and a bit
> more complex remap for R-Car Gen5.
>
> The GPIO block register offsets on R-Car Gen5 changed and the change
> can be divided into five groups, registers which remained at the
> same offset, INDT register shifted by +0x10, OUTDTSEL register
> shifted by -0x34, INEN register shifted by -0x38 and the rest of
> the registers used by the driver shifted by +0x70 .
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/gpio/gpio-rcar.c
> +++ b/drivers/gpio/gpio-rcar.c

> @@ -65,14 +66,59 @@ struct gpio_rcar_priv {
>
>  #define RCAR_MAX_GPIO_PER_BANK         32
>
> +static inline int gpio_rcar_remap_offset(struct gpio_rcar_priv *p, int *offs)

IMO passing a pointer to offs complicates the code.  Perhaps pass offs
by value, and return the adjusted offset or a negative error code?

> +{
> +       /* R-Car Gen4 and older do not need any offset remap. */
> +       if (!p->info.has_layout_gen5)
> +               return 0;
> +
> +       /*
> +        * R-Car Gen5 register layout is slightly different and the offsets
> +        * that have to be added to or subtracted from each register offset
> +        * can be divided into five groups, listed below.
> +        */
> +       switch (*offs) {
> +       case IOINTSEL...OUTDT:
> +               return 0;
> +       case INDT:
> +               *offs += 0x10;
> +               return 0;
> +       case INTDT...EDGLEVEL:
> +               fallthrough;
> +       case BOTHEDGE:
> +               *offs += 0x70;
> +               return 0;
> +       case OUTDTSEL:
> +               *offs -= 0x34;
> +               return 0;
> +       case INEN:
> +               *offs -= 0x38;
> +               return 0;
> +       default:
> +               /*
> +                * This here must never be reached, if this is reached, that
> +                * means there is a catastrophic failure in the driver. Skip
> +                * any IO read/write to prevent further damage.
> +                */
> +               WARN_ON(1);

A build-time failure would be better.  I tried BUILD_BUG() instead,
but unfortunately gcc is not smart enough to notice this case is
never reached.  __always_inline doesn't seem to help either.

> +               return -EINVAL;
> +       }
> +}

Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

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