Hi Quentin,
On Feb 4, 2025 at 09:30:33, Quentin Schulz <quentin.sch...@cherry.de> wrote:
> 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 <jus...@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.
>
My preference was to keep these changes together. Without the modification
to the C file, enabling CONFIG_SPL_REGULATOR_RK8XX would not have the
expected behavior since most of the driver code would still be disabled. If
you feel strongly otherwise, I am happy to split into separate patches.
> In any case, looks good to me so:
>
> Reviewed-by: Quentin Schulz <quentin.sch...@cherry.de>
>
> Thanks!
> Quentin
>
>
Thanks,
Justin
