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

[Geert Uytterhoeven]

Hi Marek,

On Sat, 5 Sept 2026 at 23:57, Marek Vasut <marek.vasut@xxxxxxxxxxx> wrote:
> On 9/4/26 1:15 PM, Geert Uytterhoeven wrote:
> >> +++ 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?
>
> I want to avoid that, since if I only return error value, I can then do
> simple:
>
> ret = gpio_rcar_remap_offset(...);
> if (ret)
>    return ret;
>
> in gpio_rcar_read() and gpio_rcar_write(), which are the only two call
> sites of this function.

gpio_rcar_read() and gpio_rcar_write() do not return error codes.
I was thinking of

    offs = gpio_rcar_remap_offset(p, offs);
    if (offs < 0)
            return 0;

which is almost the same, but avoids passing offs by address.

> >> +{
> >> +       /* 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.
> I had one more idea -- how about we convert the driver to mmio regmap,
> use opaque register numbers throughout the driver to identify registers
> to the regmap (maybe not a great idea), and then implement .read/.write
> callbacks in the regmap_config which instead of doing plain
> readl()/writel() for register IO would instead do this remapping ?
> Regmap could validate that the opaque register numbers are only the
> expected register numbers and reject all the others. Maybe the opaque
> register numbers could instead of Gen4 register offsets. What do you think ?

That's similar (but more complex?) than the array look-up
in drivers/tty/serial/sh-sci.c I pointed to before.
drivers/i2c/busses/i2c-riic.c uses the same method.

I.e. just convert the existing register defines into an enum, and use
that to index a table with the family-specific offsets?

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