Re: [PATCH v4 4/5] pinctrl: renesas: rzg2l: Add RZ/G3S support for selecting the I3C power source

[Geert Uytterhoeven]

Hi Claudiu, Biju,

On Tue, 14 Jul 2026 at 09:26, Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
> On 7/13/26 18:15, Biju Das wrote:
> >> From: claudiu beznea<claudiu.beznea@xxxxxxxxx>
> >> On 7/13/26 16:56, Biju Das wrote:
> >>>> From: claudiu beznea<claudiu.beznea@xxxxxxxxx>
> >>>> On 7/12/26 17:55, Biju Das wrote:
> >>>>>> From: Claudiu Beznea<claudiu.beznea+renesas@xxxxxxxxx>
> >>>>>> The Renesas RZ/G3S I3C pins can be powered at either 1.8V or 1.2V.
> >>>>>> The pin controller provides a register to select between these two options.
> >>>>>> Update the Renesas RZ/G2L pin controller driver to allow selecting the I3C power source on RZ/G3S
> >> SoC.
> >>>>>> Reviewed-by: Wolfram Sang<wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> >>>>>> Tested-by: Wolfram Sang<wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> >>>>>> Signed-off-by: Claudiu Beznea<claudiu.beznea.uj@xxxxxxxxxxxxxx>

> >>>>>> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >>>>>> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> >>>>>> @@ -281,6 +288,11 @@ struct rzg2l_register_masks {
> >>>>>>                          u8 other_poc_pvdd1833_oth_iso_poc;
> >>>>>>                          u8 other_poc_wdtovf_n_poc;
> >>>>>>                  };
> >>>>>> +
> >>>>>> +                /* RZ/G3S masks */
> >>>>>> +                struct {
> >>>>>> +                        u8 i3c_set_poc;
> >>>>> How this POC is different from Ethernet, SDHI and XSPI POC?
> >>>> Different bit mask and offset for I3C SET_POC compared with ETH, SDHI, XSPI.
> >>> RZ/G3L has i3c_set_poc, which has same bitmask as other_poc_wdtovf_n_poc.
> >>> Maybe create register specific masks??
> >>>
> >>> struct other_poc and struct i3c_set instead of union. So that both
> >>> RZ/G3L and RZ/G3S can share the same struct for i3c.
> >> When RZ/G3L I3C POC support will be added the i3c_set_poc member of struct rzg2l_register_masks could be
> >> moved as common member:
> >>
> >> Current code base allows for this extension. You can have:
> >> struct rzg2l_register_masks {
> >> +       /* Common masks. */
> >> +       u8 i3c_set_poc;
> >>           union {
> > OK, This union will go when we add RZ/G3L.
> >
> >>                   /* RZ/G3L masks */
> >>                   struct {
> >>                           u8 other_poc_pvdd1833_oth_awo_poc;
> >>                           u8 other_poc_pvdd1833_oth_iso_poc;
> >>                           u8 other_poc_wdtovf_n_poc;
> >>                   };
> >> -               /* RZ/G3S masks */
> >> -               struct {
> > Looks, this struct not needed now. struct with single member
> > has no value at all.
>
> I'll let Geert decide if he wants me to drop it.

I like having a struct for grouping, even for a single member, for
consistency, as the other union member is a struct.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds