Re: [PATCH 2/2] gpio: rcar: Add R-Car X5H (R8A78000) support
From: Marek Vasut
Date: Tue Sep 08 2026 - 21:34:57 EST
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 ?
I had one more idea -- how about we convert the driver to mmio regmap,+{
+ /* 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.
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 tablePlease see the actual-regmap suggestion I proposed above, that solves this problem too, without mixing signed and unsigned types.
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);