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

[Marek Vasut <marek.vasut@mailbox.org>]

On 9/8/26 9:49 AM, Geert Uytterhoeven wrote:

Hello Geert,

>>>> 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.

> : 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.

> As a bonus, storing -1 for a non-existing register in the look-up table
> would let us get rid of the four existing .has_<reg> booleans, e.g.
> 
>     -        if (p->info.has_both_edge_trigger)
>     +        if (p->info.regs[BOTHEDGE] >= 0)
>                      gpio_rcar_modify_bit(p, BOTHEDGE, hwirq, both);
Please see the actual-regmap suggestion I proposed above, that solves 
this problem too, without mixing signed and unsigned types.