Re: [PATCH v2 3/5] regulator: rk8xx: Add CONFIG_SPL_REGULATOR_RK8XX

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

Hi Justin,

On 1/28/25 10:37 PM, Justin Klaassen wrote:
> Allows use of the regulator functions of the RK8XX PMIC in SPL, which is
> necessary to support the functionality of the Rockchip IO-domain driver
> on relevant platforms.
> 
> Signed-off-by: Justin Klaassen <justin@tidylabs.net>
> ---
> 
> Changes in v2:
> - Added separate patch for added CONFIG_SPL_REGULATOR_RK8XX Kconfig
> 
>   drivers/power/regulator/Kconfig | 9 +++++++++
>   drivers/power/regulator/rk8xx.c | 8 ++------
>   2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
> index 958f337c7e7..9b50128f859 100644
> --- a/drivers/power/regulator/Kconfig
> +++ b/drivers/power/regulator/Kconfig
> @@ -241,6 +241,15 @@ config REGULATOR_RK8XX
>   	by the PMIC device. This driver is controlled by a device tree node
>   	which includes voltage limits.
>   
> +config SPL_REGULATOR_RK8XX
> +	bool "Enable driver for RK8XX regulators in SPL"
> +	depends on SPL_DM_REGULATOR && SPL_PMIC_RK8XX
> +	help
> +	  Enable support for the regulator functions of the RK8XX PMIC in SPL. The
> +	  driver implements get/set api for the various BUCKS and LDOs supported
> +	  by the PMIC device. This driver is controlled by a device tree node
> +	  which includes voltage limits.
> +
>   config DM_REGULATOR_S2MPS11
>   	bool "Enable driver for S2MPS11 regulator"
>   	depends on DM_REGULATOR && PMIC_S2MPS11
> diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
> index 368675ebb9f..88453bb7bdb 100644
> --- a/drivers/power/regulator/rk8xx.c
> +++ b/drivers/power/regulator/rk8xx.c
> @@ -16,10 +16,6 @@
>   #include <power/pmic.h>
>   #include <power/regulator.h>
>   
> -#ifndef CONFIG_XPL_BUILD
> -#define ENABLE_DRIVER
> -#endif
> -
>   /* Not used or exisit register and configure */
>   #define NA			0xff
>   
> @@ -202,7 +198,7 @@ static const struct rk8xx_reg_info rk818_buck[] = {
>   	{ 1800000, 100000, REG_BUCK4_ON_VSEL, REG_BUCK4_SLP_VSEL, REG_BUCK4_CONFIG, RK818_BUCK4_VSEL_MASK, 0x00, 0x1f },
>   };
>   
> -#ifdef ENABLE_DRIVER
> +#if CONFIG_IS_ENABLED(REGULATOR_RK8XX)
>   static const struct rk8xx_reg_info rk806_nldo[] = {
>   	/* nldo 1 */
>   	{  500000, 12500, RK806_NLDO_ON_VSEL(1), RK806_NLDO_SLP_VSEL(1), NA, RK806_NLDO_VSEL_MASK, 0x00, 0xe7},
> @@ -454,7 +450,7 @@ static int _buck_set_enable(struct udevice *pmic, int buck, bool enable)
>   	return ret;
>   }
>   
> -#ifdef ENABLE_DRIVER
> +#if CONFIG_IS_ENABLED(REGULATOR_RK8XX)
>   static int _buck_set_suspend_value(struct udevice *pmic, int buck, int uvolt)
>   {
>   	const struct rk8xx_reg_info *info = get_buck_reg(pmic, buck, uvolt);

I would split the modification of the c file into a separate patch as 
the addition of the symbol and the modification of the C file aren't per 
se co-dependent.

Their order wouldn't even matter in that case.

In any case, looks good to me so:

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

Thanks!
Quentin