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

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

On 9/7/26 9:53 AM, Geert Uytterhoeven wrote:

Hello Geert,

> On Sat, 5 Sept 2026 at 23:57, Marek Vasut <marek.vasut@mailbox.org> 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 ?

>>>> +{
>>>> +       /* 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 ?

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