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

[Geert Uytterhoeven]

Hi Marek,

On Wed, 9 Sept 2026 at 03:34, Marek Vasut <marek.vasut@xxxxxxxxxxx> wrote:
> On 9/8/26 9:49 AM, Geert Uytterhoeven 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.
> >>
> >> Is there any benefit to it, compared to keeping the value and return
> >> code separate ?
> >
> > Naive me (I am not a compiler writer) thinks the compiler may have a
> > harder time to optimize the code when addresses are involved.
>
> Will the compiler generate code that is worse in the end ?
>
> >>>>>> +{
> >>>>>> +       /* 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.
> >>
> >> Those do not use regmap (drivers/base/regmap/), do they ?
> >
> > No they don't.
>
> What about my regmap suggestion ?
>
> >>> I.e. just convert the existing register defines into an enum, and use
> >>> that to index a table with the family-specific offsets?
> >>
> >> Are we back to the table look up discussion instead of remap function ?
> >
> > Yes, I think that's the simplest and best-performing solution
>
> The performance benefit of the table look up was never confirmed.

You think a single array element retrieval would be more expensive
than an out-of-line call to regmap, involving function pointers and
(oh yes) a table look-up?

> > : one
> > extra table look-up (array indexing) in gpio_rcar_{read,write}(),
> > compared to an extra function call to gpio_rcar_remap_offset().
>
> The function is inlined by the compiler. The table look up will likely
> suffer due to non-locality of the data in cache.

It is not inlined by gcc 13.3.0 (Ubuntu 13.3.0-6ubuntu2~24.04.1).
The table needs 13x2 bytes, which fits in a cache line on anything
but m68k.

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