RE: [cip-dev] [PATCH 6.12.y-cip 10/23] pinctrl: renesas: rzg2l: Add support for selecting power source for {WDT,AWO,ISO}

Hi Pavel,

Thanks for the feedback.

> -----Original Message-----
> From: [email protected] <[email protected]> On Be=
half Of Pavel Machek via
> lists.cip-project.org
> Sent: 15 July 2026 11:18
> Subject: Re: [cip-dev] [PATCH 6.12.y-cip 10/23] pinctrl: renesas: rzg2l: =
Add support for selecting power
> source for {WDT,AWO,ISO}
>=20
> Hi!
>=20
> > The RZ/G3L SoC has support for setting power source that are not
> > controlled by the following voltage control registers:
> >   - SD_CH{0,1,2}_POC, XSPI_POC, ETH{0,1}_POC, I3C_SET.POC
> >
> > Add support for selecting voltages using OTHER_POC register for
> > setting I/O domain voltage for WDT, ISO and AWO by extending
> > rzg2l_caps_to_pwr_reg() with a mask output parameter so that callers
> > callers can identify which bit(s) within OTHER_POC correspond to the
> > requested domain. Update rzg2l_get_power_source() to extract the
> > relevant bit field via field_get() when reading OTHER_POC, and update
> > rzg2l_set_power_source() to perform a read-modify-write under the
> > spinlock when writing to OTHER_POC, since multiple domains share the
> > same register.
>=20
> I believe more robustness is needed here.
>=20
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -958,25 +983,37 @@ static int rzg2l_set_power_source(struct
> > rzg2l_pinctrl *pctrl, u32 pin, u32 caps
> >
> >  	switch (ps) {
> >  	case 1800:
> > -		val =3D PVDD_1800;
> > +		poc_val =3D PVDD_1800;
> >  		break;
> >  	case 2500:
> >  		if (!(caps & (PIN_CFG_IO_VMC_ETH0 | PIN_CFG_IO_VMC_ETH1)))
> >  			return -EINVAL;
> > -		val =3D PVDD_2500;
> > +		poc_val =3D PVDD_2500;
> >  		break;
> >  	case 3300:
> > -		val =3D PVDD_3300;
> > +		poc_val =3D PVDD_3300;
> >  		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> >
> > -	pwr_reg =3D rzg2l_caps_to_pwr_reg(regs, caps);
> > +	pwr_reg =3D rzg2l_caps_to_pwr_reg(regs, caps, &mask);
> >  	if (pwr_reg < 0)
> >  		return pwr_reg;
> >
> > -	writeb(val, pctrl->base + pwr_reg);
> > +	if (pwr_reg =3D=3D OTHER_POC) {
> > +		scoped_guard(raw_spinlock, &pctrl->lock) {
> > +			val =3D readb(pctrl->base + pwr_reg);
> > +			if (poc_val)
> > +				val |=3D mask;
> > +			else
> > +				val &=3D ~mask;
> > +			writeb(val, pctrl->base + pwr_reg);
> > +		}
>=20
> This is strange. You test that ps is right ammount of milivolts, but here=
 you just ignore poc_val, no
> matter if user selected 1800, 2500 or 3300mV.

Poc_val is used to set the desired voltage. For OTHER_POC register there ar=
e 3 bits

For each bit,=20

#define PVDD_1800		1	/* I/O domain voltage <=3D 1.8V */
#define PVDD_3300		0	/* I/O domain voltage >=3D 3.3V */

Cheers,
Biju

>=20
> I believe you should cross-check values here, so that if user requests 18=
00mV, he does not silenly get
> 3300mV (or vice versa, I'm not sure how the hardware work).
>=20
> Best regards,
> 								Pavel