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

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

On 9/4/26 1:15 PM, Geert Uytterhoeven wrote:

Hello Geert,

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

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