Re: [PATCH v2 3/4] gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3744 bytes --]

Hi,

On Sun Apr 19, 2026 at 11:19 PM CEST, Linus Walleij wrote:
> Hi Yu-Chun,
>
> On Fri, Apr 10, 2026 at 11:39 AM Yu-Chun Lin [林祐君]
> <eleanor.lin@realtek.com> wrote:
>
>> We did look into gpio-mmio and gpio-regmap, but they are not quite suitable for
>> our platform due to the specific hardware design:
>>
>> 1. Per-GPIO Dedicated Registers: Unlike typical GPIO controllers that pack 32 pins
>> into a single 32-bit register (1 bit per pin), our hardware uses a dedicated 32-bit
>> register for each individual GPIO. This single register controls the
>> input/output state, direction, and interrupt trigger type for that specific pin.
>
> Isn't that attainable by:
>
> - setting .ngpio_per_reg to 1 in struct gpio_regmap_config

Which is just used by the gpio_regmap_simple_xlate() anyway. So it
doesn't really matter. But yeah, 1 would be the correct value here,
assuming that the registers are consecutive.

> - extend .reg_mask_xlate callback with an enum for each operation
>   (need to change all users of the .reg_mask_xlate callback but
>   who cares, they are not many):
>
> e.g.
>
> enum gpio_regmap_operation {
>     GPIO_REGMAP_GET_OP,
>     GPIO_REGMAP_SET_OP,
>     GPIO_REGMAP_SET_WITH_CLEAR_OP,
>     GPIO_REGMAP_GET_DIR_OP,
>     GPIO_REGMAP_SET_DIR_OP,
> };
>
>  int (*reg_mask_xlate)(struct gpio_regmap *gpio,
>                               enum_gpio_regmap_operation op,
>                               unsigned int base,
>                               unsigned int offset, unsigned int *reg,
>                               unsigned int *mask);
>
> This way .reg_mask_xlate() can hit different bits in the returned
> *mask depending on operation and it will be find to pack all of
> the bits into one 32bit register.
>
> Added Michael Walle to the the thread, he will know if this is a
> good idea.

Nice idea, though the information is then redundant in the usual
case, i.e. drivers which need to translate specific registers
will do a "switch (base)" at the moment. These should be converted
to "switch (op)" just to keep all the drivers aligned and prevent
new drivers from using the old method. You'd need to touch them
anyway.

I was briefly thinking about making it somewhat possible to embed
the op into the base, if it would otherwise be all the same. That
way, you could gpio-regmap as is. A special case like
GPIO_REGMAP_ADDR_ZERO, that could be used by these kind of drivers,
but that is probably too hacky.

I'm fine with either way.

>> 2. Write-Enable (WREN) Mask Mechanism: Our hardware requires a specific Write-Enable
>> mask to be written simultaneously when updating the register values.
>
> Which is to just set bit 31.
>
> With the above scheme your .reg_mask_xlate callback can just set bit 31
> no matter what operating you're doing. Piece of cake.

Keep in mind, that this will make reading and writing somewhat
different. reading assumes there is only one bit set in mask,
because of the "!!(val & mask)" op, which is hardcoded. I'm not
against using the write like that though.

-michael

>> 3. Hardware Debounce: We also need to support hardware debounce settings per pin,
>> which requires custom configuration via set_config mapped to these specific per-pin
>> registers.
>
> Just add a version of an optional .set_config() call to gpio-regmap.c
> to handle this using .reg_mask_xlate() per above and add a new
> GPIO_REGMAP_CONFIG_OP to the above enum, problem solved.
>
> If it seems too hard I can write patch 1 & 2 adding this infrastructure
> but I bet you can easily see what can be done with gpio-regmap.c
> here provided Michael W approves the idea.
>
> Yours,
> Linus Walleij


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]