Re: [PATCH 1/2] power: regulator: rk8xx: add RK818 LDO9 support

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Shigeaki Asai,

On 10/10/26 3:32 AM, Shigeaki Asai wrote:
> The RK818 has nine LDOs, but rk818_ldo[] only describes eight, so
> LDO_REG9 indexes past the end of the table. Its voltage reads back as
> garbage and it cannot be enabled or set.
> 
> Unlike LDO1-8, LDO9 is switched from the DCDC enable and sleep
> registers (bit 5), and its voltage selector lives in
> BOOST_LDO9_ON_VSEL/SLP_VSEL with the usual 1.8-3.4V, 100mV range.
> 
> Add the table entry and handle LDO9 in the enable and suspend enable
> helpers.
> 
> This patch was written with the help of Claude (claude-opus-5-5).
> 
> Signed-off-by: Shigeaki Asai <y-asai.0816@docomonet.jp>
> ---
>   drivers/power/regulator/rk8xx.c | 46 ++++++++++++++++++++++++++-------
>   1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
> index 88453bb7bdb..cb402c96ec8 100644
> --- a/drivers/power/regulator/rk8xx.c
> +++ b/drivers/power/regulator/rk8xx.c
> @@ -60,6 +60,8 @@
>   #define RK818_BUCK4_VSEL_MASK		0x1f
>   #define RK818_LDO_VSEL_MASK		0x1f
>   #define RK818_LDO3_ON_VSEL_MASK	0xf
> +/* LDO9 is switched from REG_DCDC_EN and REG_SLEEP_SET_OFF1 */
> +#define RK818_LDO9_EN_MASK		BIT(5)
>   #define RK818_BOOST_ON_VSEL_MASK	0xe0
>   #define RK818_USB_ILIM_SEL_MASK		0x0f
>   #define RK818_USB_CHG_SD_VSEL_MASK	0x70
> @@ -297,6 +299,8 @@ static const struct rk8xx_reg_info rk818_ldo[] = {
>   	{  800000, 100000, REG_LDO6_ON_VSEL, REG_LDO6_SLP_VSEL, NA, RK818_LDO_VSEL_MASK, },
>   	{  800000, 100000, REG_LDO7_ON_VSEL, REG_LDO7_SLP_VSEL, NA, RK818_LDO_VSEL_MASK, },
>   	{ 1800000, 100000, REG_LDO8_ON_VSEL, REG_LDO8_SLP_VSEL, NA, RK818_LDO_VSEL_MASK, },
> +	{ 1800000, 100000, REG_BOOST_LDO9_ON_VSEL, REG_BOOST_LDO9_SLP_VSEL, NA,
> +	  RK818_LDO_VSEL_MASK, },

Please stay consistent with the rest of the file where we have 
everything on the same line.

>   };
>   #endif
>   
> @@ -695,8 +699,13 @@ static int _ldo_get_enable(struct udevice *pmic, int ldo)
>   		break;
>   	case RK808_ID:
>   	case RK818_ID:
> -		mask = 1 << ldo;
> -		ret = pmic_reg_read(pmic, REG_LDO_EN);
> +		if (priv->variant == RK818_ID && ldo == 8) {
> +			mask = RK818_LDO9_EN_MASK;
> +			ret = pmic_reg_read(pmic, REG_DCDC_EN);
> +		} else {
> +			mask = 1 << ldo;
> +			ret = pmic_reg_read(pmic, REG_LDO_EN);
> +		}

This is not great, I would prefer to have some additional configuration 
in rk8xx_reg_info instead, but since we do it for many other cases 
already, nothing for you to do on this issue in this patch.

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin