[PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks

[PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks

[Quentin Schulz]

This is for master branch, merge ASAP as it's known to break at least
Chromebook Jerry.

@Simon, can you please check that this fixes your CB?

The wrong udevice was passed to the functions, making them call the
pmic callbacks on the parent of the pmic udevice instead of the pmic
udevice itself.

While at it, ensure consistency by having all internal functions use
pmic udevice instead of the regulator udevice.

Finally, clarify operator precedence in ternary condition as reported by
my linter.

Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
Quentin Schulz (3):
      regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value
      regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions
      regulator: rk8xx: clarify operator precedence

 drivers/power/regulator/rk8xx.c | 54 ++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 27 deletions(-)
---
base-commit: c0ea27bccfb7d2d37fd36806ac2a2f7389099420
change-id: 20240605-pmic-rk8xx-52f2286be334

Best regards,
-- 
Quentin Schulz <quentin.schulz@cherry.de>

[PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@cherry.de>

_ldo_get_suspend_value and _ldo_set_suspend_value get passed the parent
of the regulator (so the pmic) as first argument, therefore this udevice
should be used for pmic_* callbacks instead of using the parent of the
pmic.

To avoid further confusion, let's rename the argument to pmic instead of
dev, highlighting which kind of device we expect as argument.

Fixes: f047e4ab9762 ("regulator: rk8xx: add indirection level for some ldo callbacks")
Reported-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 drivers/power/regulator/rk8xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
index 1bd4605d43a..cce3502f89c 100644
--- a/drivers/power/regulator/rk8xx.c
+++ b/drivers/power/regulator/rk8xx.c
@@ -1216,7 +1216,7 @@ static int pldo_set_value(struct udevice *dev, int uvolt)
 	return _ldo_set_value(dev, info, uvolt);
 }
 
-static int _ldo_set_suspend_value(struct udevice *dev, const struct rk8xx_reg_info *info, int uvolt)
+static int _ldo_set_suspend_value(struct udevice *pmic, const struct rk8xx_reg_info *info, int uvolt)
 {
 	int mask = info->vsel_mask;
 	int val;
@@ -1232,7 +1232,7 @@ static int _ldo_set_suspend_value(struct udevice *dev, const struct rk8xx_reg_in
 	debug("%s: volt=%d, reg=0x%x, mask=0x%x, val=0x%x\n",
 	      __func__, uvolt, info->vsel_sleep_reg, mask, val);
 
-	return pmic_clrsetbits(dev->parent, info->vsel_sleep_reg, mask, val);
+	return pmic_clrsetbits(pmic, info->vsel_sleep_reg, mask, val);
 }
 
 static int ldo_set_suspend_value(struct udevice *dev, int uvolt)
@@ -1259,7 +1259,7 @@ static int pldo_set_suspend_value(struct udevice *dev, int uvolt)
 	return _ldo_set_suspend_value(dev->parent, info, uvolt);
 }
 
-static int _ldo_get_suspend_value(struct udevice *dev, const struct rk8xx_reg_info *info)
+static int _ldo_get_suspend_value(struct udevice *pmic, const struct rk8xx_reg_info *info)
 {
 	int mask = info->vsel_mask;
 	int val, ret;
@@ -1267,7 +1267,7 @@ static int _ldo_get_suspend_value(struct udevice *dev, const struct rk8xx_reg_in
 	if (info->vsel_sleep_reg == NA)
 		return -ENOSYS;
 
-	ret = pmic_reg_read(dev->parent, info->vsel_sleep_reg);
+	ret = pmic_reg_read(pmic, info->vsel_sleep_reg);
 	if (ret < 0)
 		return ret;
 

-- 
2.45.1

[PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@cherry.de>

For the sake of consistency, make all internal (starting with _)
functions expect a pmic udevice instead of a regulator udevice.

Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 drivers/power/regulator/rk8xx.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
index cce3502f89c..bd5a37e718f 100644
--- a/drivers/power/regulator/rk8xx.c
+++ b/drivers/power/regulator/rk8xx.c
@@ -1134,14 +1134,14 @@ static int buck_get_enable(struct udevice *dev)
 	return _buck_get_enable(dev->parent, buck);
 }
 
-static int _ldo_get_value(struct udevice *dev, const struct rk8xx_reg_info *info)
+static int _ldo_get_value(struct udevice *pmic, const struct rk8xx_reg_info *info)
 {
 	int mask = info->vsel_mask;
 	int ret, val;
 
 	if (info->vsel_reg == NA)
 		return -ENOSYS;
-	ret = pmic_reg_read(dev->parent, info->vsel_reg);
+	ret = pmic_reg_read(pmic, info->vsel_reg);
 	if (ret < 0)
 		return ret;
 	val = ret & mask;
@@ -1154,7 +1154,7 @@ static int ldo_get_value(struct udevice *dev)
 	int ldo = dev->driver_data - 1;
 	const struct rk8xx_reg_info *info = get_ldo_reg(dev->parent, ldo, 0);
 
-	return _ldo_get_value(dev, info);
+	return _ldo_get_value(dev->parent, info);
 }
 
 static int nldo_get_value(struct udevice *dev)
@@ -1162,7 +1162,7 @@ static int nldo_get_value(struct udevice *dev)
 	int nldo = dev->driver_data - 1;
 	const struct rk8xx_reg_info *info = get_nldo_reg(dev->parent, nldo, 0);
 
-	return _ldo_get_value(dev, info);
+	return _ldo_get_value(dev->parent, info);
 }
 
 static int pldo_get_value(struct udevice *dev)
@@ -1170,10 +1170,10 @@ static int pldo_get_value(struct udevice *dev)
 	int pldo = dev->driver_data - 1;
 	const struct rk8xx_reg_info *info = get_pldo_reg(dev->parent, pldo, 0);
 
-	return _ldo_get_value(dev, info);
+	return _ldo_get_value(dev->parent, info);
 }
 
-static int _ldo_set_value(struct udevice *dev, const struct rk8xx_reg_info *info, int uvolt)
+static int _ldo_set_value(struct udevice *pmic, const struct rk8xx_reg_info *info, int uvolt)
 {
 	int mask = info->vsel_mask;
 	int val;
@@ -1189,7 +1189,7 @@ static int _ldo_set_value(struct udevice *dev, const struct rk8xx_reg_info *info
 	debug("%s: volt=%d, reg=0x%x, mask=0x%x, val=0x%x\n",
 	      __func__, uvolt, info->vsel_reg, mask, val);
 
-	return pmic_clrsetbits(dev->parent, info->vsel_reg, mask, val);
+	return pmic_clrsetbits(pmic, info->vsel_reg, mask, val);
 }
 
 static int ldo_set_value(struct udevice *dev, int uvolt)
@@ -1197,7 +1197,7 @@ static int ldo_set_value(struct udevice *dev, int uvolt)
 	int ldo = dev->driver_data - 1;
 	const struct rk8xx_reg_info *info = get_ldo_reg(dev->parent, ldo, uvolt);
 
-	return _ldo_set_value(dev, info, uvolt);
+	return _ldo_set_value(dev->parent, info, uvolt);
 }
 
 static int nldo_set_value(struct udevice *dev, int uvolt)
@@ -1205,7 +1205,7 @@ static int nldo_set_value(struct udevice *dev, int uvolt)
 	int nldo = dev->driver_data - 1;
 	const struct rk8xx_reg_info *info = get_nldo_reg(dev->parent, nldo, uvolt);
 
-	return _ldo_set_value(dev, info, uvolt);
+	return _ldo_set_value(dev->parent, info, uvolt);
 }
 
 static int pldo_set_value(struct udevice *dev, int uvolt)
@@ -1213,7 +1213,7 @@ static int pldo_set_value(struct udevice *dev, int uvolt)
 	int pldo = dev->driver_data - 1;
 	const struct rk8xx_reg_info *info = get_pldo_reg(dev->parent, pldo, uvolt);
 
-	return _ldo_set_value(dev, info, uvolt);
+	return _ldo_set_value(dev->parent, info, uvolt);
 }
 
 static int _ldo_set_suspend_value(struct udevice *pmic, const struct rk8xx_reg_info *info, int uvolt)

-- 
2.45.1

[PATCH 3/3] regulator: rk8xx: clarify operator precedence

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@cherry.de>

My linter complains that the order isn't clear enough so let's put
parentheses around the ternary condition to make it happy.

Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 drivers/power/regulator/rk8xx.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
index bd5a37e718f..3125835bc07 100644
--- a/drivers/power/regulator/rk8xx.c
+++ b/drivers/power/regulator/rk8xx.c
@@ -520,7 +520,7 @@ static int _buck_get_enable(struct udevice *pmic, int buck)
 	if (ret < 0)
 		return ret;
 
-	return ret & mask ? true : false;
+	return (ret & mask) ? true : false;
 }
 
 static int _buck_set_suspend_enable(struct udevice *pmic, int buck, bool enable)
@@ -585,7 +585,7 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
 		val = pmic_reg_read(pmic, RK816_REG_DCDC_SLP_EN);
 		if (val < 0)
 			return val;
-		ret = val & mask ? 1 : 0;
+		ret = (val & mask) ? 1 : 0;
 		break;
 	case RK806_ID:
 		{
@@ -608,7 +608,7 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
 		val = pmic_reg_read(pmic, REG_SLEEP_SET_OFF1);
 		if (val < 0)
 			return val;
-		ret = val & mask ? 0 : 1;
+		ret = (val & mask) ? 0 : 1;
 		break;
 	case RK809_ID:
 	case RK817_ID:
@@ -620,7 +620,7 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
 		val = pmic_reg_read(pmic, RK817_POWER_SLP_EN(0));
 		if (val < 0)
 			return val;
-		ret = val & mask ? 1 : 0;
+		ret = (val & mask) ? 1 : 0;
 		break;
 	default:
 		ret = -EINVAL;
@@ -723,7 +723,7 @@ static int _ldo_get_enable(struct udevice *pmic, int ldo)
 	if (ret < 0)
 		return ret;
 
-	return ret & mask ? true : false;
+	return (ret & mask) ? true : false;
 }
 
 static int _nldo_get_enable(struct udevice *pmic, int nldo)
@@ -980,7 +980,7 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
 		val = pmic_reg_read(pmic, RK816_REG_LDO_SLP_EN);
 		if (val < 0)
 			return val;
-		ret = val & mask ? 1 : 0;
+		ret = (val & mask) ? 1 : 0;
 		break;
 	case RK808_ID:
 	case RK818_ID:
@@ -988,7 +988,7 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
 		val = pmic_reg_read(pmic, REG_SLEEP_SET_OFF2);
 		if (val < 0)
 			return val;
-		ret = val & mask ? 0 : 1;
+		ret = (val & mask) ? 0 : 1;
 		break;
 	case RK809_ID:
 	case RK817_ID:
@@ -997,13 +997,13 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
 			val = pmic_reg_read(pmic, RK817_POWER_SLP_EN(0));
 			if (val < 0)
 				return val;
-			ret = val & mask ? 1 : 0;
+			ret = (val & mask) ? 1 : 0;
 		} else {
 			mask = 1 << ldo;
 			val = pmic_reg_read(pmic, RK817_POWER_SLP_EN(1));
 			if (val < 0)
 				return val;
-			ret = val & mask ? 1 : 0;
+			ret = (val & mask) ? 1 : 0;
 		}
 		break;
 	}
@@ -1438,7 +1438,7 @@ static int switch_get_enable(struct udevice *dev)
 	if (ret < 0)
 		return ret;
 
-	return ret & mask ? true : false;
+	return (ret & mask) ? true : false;
 }
 
 static int switch_set_suspend_value(struct udevice *dev, int uvolt)
@@ -1493,21 +1493,21 @@ static int switch_get_suspend_enable(struct udevice *dev)
 		val = pmic_reg_read(dev->parent, REG_SLEEP_SET_OFF1);
 		if (val < 0)
 			return val;
-		ret = val & mask ? 0 : 1;
+		ret = (val & mask) ? 0 : 1;
 		break;
 	case RK809_ID:
 		mask = 1 << (sw + 6);
 		val = pmic_reg_read(dev->parent, RK817_POWER_SLP_EN(0));
 		if (val < 0)
 			return val;
-		ret = val & mask ? 1 : 0;
+		ret = (val & mask) ? 1 : 0;
 		break;
 	case RK818_ID:
 		mask = 1 << 6;
 		val = pmic_reg_read(dev->parent, REG_SLEEP_SET_OFF1);
 		if (val < 0)
 			return val;
-		ret = val & mask ? 0 : 1;
+		ret = (val & mask) ? 0 : 1;
 		break;
 	}
 

-- 
2.45.1

Re: [PATCH 3/3] regulator: rk8xx: clarify operator precedence

[Mattijs Korpershoek]

Hi Quentin,

Thank you for the patch.

On mer., juin 05, 2024 at 11:33, Quentin Schulz <foss+uboot@0leil.net> wrote:

> From: Quentin Schulz <quentin.schulz@cherry.de>
>
> My linter complains that the order isn't clear enough so let's put
> parentheses around the ternary condition to make it happy.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/power/regulator/rk8xx.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
> index bd5a37e718f..3125835bc07 100644
> --- a/drivers/power/regulator/rk8xx.c
> +++ b/drivers/power/regulator/rk8xx.c
> @@ -520,7 +520,7 @@ static int _buck_get_enable(struct udevice *pmic, int buck)
>  	if (ret < 0)
>  		return ret;
>  
> -	return ret & mask ? true : false;
> +	return (ret & mask) ? true : false;
>  }
>  
>  static int _buck_set_suspend_enable(struct udevice *pmic, int buck, bool enable)
> @@ -585,7 +585,7 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
>  		val = pmic_reg_read(pmic, RK816_REG_DCDC_SLP_EN);
>  		if (val < 0)
>  			return val;
> -		ret = val & mask ? 1 : 0;
> +		ret = (val & mask) ? 1 : 0;
>  		break;
>  	case RK806_ID:
>  		{
> @@ -608,7 +608,7 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
>  		val = pmic_reg_read(pmic, REG_SLEEP_SET_OFF1);
>  		if (val < 0)
>  			return val;
> -		ret = val & mask ? 0 : 1;
> +		ret = (val & mask) ? 0 : 1;
>  		break;
>  	case RK809_ID:
>  	case RK817_ID:
> @@ -620,7 +620,7 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
>  		val = pmic_reg_read(pmic, RK817_POWER_SLP_EN(0));
>  		if (val < 0)
>  			return val;
> -		ret = val & mask ? 1 : 0;
> +		ret = (val & mask) ? 1 : 0;
>  		break;
>  	default:
>  		ret = -EINVAL;
> @@ -723,7 +723,7 @@ static int _ldo_get_enable(struct udevice *pmic, int ldo)
>  	if (ret < 0)
>  		return ret;
>  
> -	return ret & mask ? true : false;
> +	return (ret & mask) ? true : false;
>  }
>  
>  static int _nldo_get_enable(struct udevice *pmic, int nldo)
> @@ -980,7 +980,7 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
>  		val = pmic_reg_read(pmic, RK816_REG_LDO_SLP_EN);
>  		if (val < 0)
>  			return val;
> -		ret = val & mask ? 1 : 0;
> +		ret = (val & mask) ? 1 : 0;
>  		break;
>  	case RK808_ID:
>  	case RK818_ID:
> @@ -988,7 +988,7 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
>  		val = pmic_reg_read(pmic, REG_SLEEP_SET_OFF2);
>  		if (val < 0)
>  			return val;
> -		ret = val & mask ? 0 : 1;
> +		ret = (val & mask) ? 0 : 1;
>  		break;
>  	case RK809_ID:
>  	case RK817_ID:
> @@ -997,13 +997,13 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
>  			val = pmic_reg_read(pmic, RK817_POWER_SLP_EN(0));
>  			if (val < 0)
>  				return val;
> -			ret = val & mask ? 1 : 0;
> +			ret = (val & mask) ? 1 : 0;
>  		} else {
>  			mask = 1 << ldo;
>  			val = pmic_reg_read(pmic, RK817_POWER_SLP_EN(1));
>  			if (val < 0)
>  				return val;
> -			ret = val & mask ? 1 : 0;
> +			ret = (val & mask) ? 1 : 0;
>  		}
>  		break;
>  	}
> @@ -1438,7 +1438,7 @@ static int switch_get_enable(struct udevice *dev)
>  	if (ret < 0)
>  		return ret;
>  
> -	return ret & mask ? true : false;
> +	return (ret & mask) ? true : false;
>  }
>  
>  static int switch_set_suspend_value(struct udevice *dev, int uvolt)
> @@ -1493,21 +1493,21 @@ static int switch_get_suspend_enable(struct udevice *dev)
>  		val = pmic_reg_read(dev->parent, REG_SLEEP_SET_OFF1);
>  		if (val < 0)
>  			return val;
> -		ret = val & mask ? 0 : 1;
> +		ret = (val & mask) ? 0 : 1;
>  		break;
>  	case RK809_ID:
>  		mask = 1 << (sw + 6);
>  		val = pmic_reg_read(dev->parent, RK817_POWER_SLP_EN(0));
>  		if (val < 0)
>  			return val;
> -		ret = val & mask ? 1 : 0;
> +		ret = (val & mask) ? 1 : 0;
>  		break;
>  	case RK818_ID:
>  		mask = 1 << 6;
>  		val = pmic_reg_read(dev->parent, REG_SLEEP_SET_OFF1);
>  		if (val < 0)
>  			return val;
> -		ret = val & mask ? 0 : 1;
> +		ret = (val & mask) ? 0 : 1;
>  		break;
>  	}
>  
>
> -- 
> 2.45.1

Re: [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks

[Anand Moon]

Hi Quentin,

On Wed, 5 Jun 2024 at 15:03, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> This is for master branch, merge ASAP as it's known to break at least
> Chromebook Jerry.
>
> @Simon, can you please check that this fixes your CB?
>
> The wrong udevice was passed to the functions, making them call the
> pmic callbacks on the parent of the pmic udevice instead of the pmic
> udevice itself.
>
> While at it, ensure consistency by having all internal functions use
> pmic udevice instead of the regulator udevice.
>
> Finally, clarify operator precedence in ternary condition as reported by
> my linter.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>

I see we have not been able to follow configs on a few of the board's for RK3588

CONFIG_CMD_REGULATOR=y
CONFIG_PMIC_RK8XX=y
CONFIG_REGULATOR_RK8XX=y

could you enable the options with imply for all the rockchip boards?

Thanks
-Anand

> ---
> Quentin Schulz (3):
>       regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value
>       regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions
>       regulator: rk8xx: clarify operator precedence
>
>  drivers/power/regulator/rk8xx.c | 54 ++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> ---
> base-commit: c0ea27bccfb7d2d37fd36806ac2a2f7389099420
> change-id: 20240605-pmic-rk8xx-52f2286be334
>
> Best regards,
> --
> Quentin Schulz <quentin.schulz@cherry.de>
>

Re: [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks

[Quentin Schulz]

Hi Anand,

On 6/5/24 3:11 PM, Anand Moon wrote:
> Hi Quentin,
> 
> On Wed, 5 Jun 2024 at 15:03, Quentin Schulz <foss+uboot@0leil.net> wrote:
>>
>> This is for master branch, merge ASAP as it's known to break at least
>> Chromebook Jerry.
>>
>> @Simon, can you please check that this fixes your CB?
>>
>> The wrong udevice was passed to the functions, making them call the
>> pmic callbacks on the parent of the pmic udevice instead of the pmic
>> udevice itself.
>>
>> While at it, ensure consistency by having all internal functions use
>> pmic udevice instead of the regulator udevice.
>>
>> Finally, clarify operator precedence in ternary condition as reported by
>> my linter.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
> I see we have not been able to follow configs on a few of the board's for RK3588
> 
> CONFIG_CMD_REGULATOR=y
> CONFIG_PMIC_RK8XX=y
> CONFIG_REGULATOR_RK8XX=y
> 
> could you enable the options with imply for all the rockchip boards?
> 

This patch series is a bug fix (ok, plus some cosmetic stuff) for master 
before the next release in July, the suggested change is not a bug fix 
so feel free to send a patch for the next branch instead :)

I'm personally not too keen in adding a lot of "imply" but I am no 
authority there, so whatever the maintainer(s) prefer :)

Thanks,
Quentin

Re: [PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value

[Kever Yang]

On 2024/6/5 17:33, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
>
> _ldo_get_suspend_value and _ldo_set_suspend_value get passed the parent
> of the regulator (so the pmic) as first argument, therefore this udevice
> should be used for pmic_* callbacks instead of using the parent of the
> pmic.
>
> To avoid further confusion, let's rename the argument to pmic instead of
> dev, highlighting which kind of device we expect as argument.
>
> Fixes: f047e4ab9762 ("regulator: rk8xx: add indirection level for some ldo callbacks")
> Reported-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   drivers/power/regulator/rk8xx.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
> index 1bd4605d43a..cce3502f89c 100644
> --- a/drivers/power/regulator/rk8xx.c
> +++ b/drivers/power/regulator/rk8xx.c
> @@ -1216,7 +1216,7 @@ static int pldo_set_value(struct udevice *dev, int uvolt)
>   	return _ldo_set_value(dev, info, uvolt);
>   }
>   
> -static int _ldo_set_suspend_value(struct udevice *dev, const struct rk8xx_reg_info *info, int uvolt)
> +static int _ldo_set_suspend_value(struct udevice *pmic, const struct rk8xx_reg_info *info, int uvolt)
>   {
>   	int mask = info->vsel_mask;
>   	int val;
> @@ -1232,7 +1232,7 @@ static int _ldo_set_suspend_value(struct udevice *dev, const struct rk8xx_reg_in
>   	debug("%s: volt=%d, reg=0x%x, mask=0x%x, val=0x%x\n",
>   	      __func__, uvolt, info->vsel_sleep_reg, mask, val);
>   
> -	return pmic_clrsetbits(dev->parent, info->vsel_sleep_reg, mask, val);
> +	return pmic_clrsetbits(pmic, info->vsel_sleep_reg, mask, val);
>   }
>   
>   static int ldo_set_suspend_value(struct udevice *dev, int uvolt)
> @@ -1259,7 +1259,7 @@ static int pldo_set_suspend_value(struct udevice *dev, int uvolt)
>   	return _ldo_set_suspend_value(dev->parent, info, uvolt);
>   }
>   
> -static int _ldo_get_suspend_value(struct udevice *dev, const struct rk8xx_reg_info *info)
> +static int _ldo_get_suspend_value(struct udevice *pmic, const struct rk8xx_reg_info *info)
>   {
>   	int mask = info->vsel_mask;
>   	int val, ret;
> @@ -1267,7 +1267,7 @@ static int _ldo_get_suspend_value(struct udevice *dev, const struct rk8xx_reg_in
>   	if (info->vsel_sleep_reg == NA)
>   		return -ENOSYS;
>   
> -	ret = pmic_reg_read(dev->parent, info->vsel_sleep_reg);
> +	ret = pmic_reg_read(pmic, info->vsel_sleep_reg);
>   	if (ret < 0)
>   		return ret;
>   
>

Re: [PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions

[Kever Yang]

On 2024/6/5 17:33, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
>
> For the sake of consistency, make all internal (starting with _)
> functions expect a pmic udevice instead of a regulator udevice.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   drivers/power/regulator/rk8xx.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
> index cce3502f89c..bd5a37e718f 100644
> --- a/drivers/power/regulator/rk8xx.c
> +++ b/drivers/power/regulator/rk8xx.c
> @@ -1134,14 +1134,14 @@ static int buck_get_enable(struct udevice *dev)
>   	return _buck_get_enable(dev->parent, buck);
>   }
>   
> -static int _ldo_get_value(struct udevice *dev, const struct rk8xx_reg_info *info)
> +static int _ldo_get_value(struct udevice *pmic, const struct rk8xx_reg_info *info)
>   {
>   	int mask = info->vsel_mask;
>   	int ret, val;
>   
>   	if (info->vsel_reg == NA)
>   		return -ENOSYS;
> -	ret = pmic_reg_read(dev->parent, info->vsel_reg);
> +	ret = pmic_reg_read(pmic, info->vsel_reg);
>   	if (ret < 0)
>   		return ret;
>   	val = ret & mask;
> @@ -1154,7 +1154,7 @@ static int ldo_get_value(struct udevice *dev)
>   	int ldo = dev->driver_data - 1;
>   	const struct rk8xx_reg_info *info = get_ldo_reg(dev->parent, ldo, 0);
>   
> -	return _ldo_get_value(dev, info);
> +	return _ldo_get_value(dev->parent, info);
>   }
>   
>   static int nldo_get_value(struct udevice *dev)
> @@ -1162,7 +1162,7 @@ static int nldo_get_value(struct udevice *dev)
>   	int nldo = dev->driver_data - 1;
>   	const struct rk8xx_reg_info *info = get_nldo_reg(dev->parent, nldo, 0);
>   
> -	return _ldo_get_value(dev, info);
> +	return _ldo_get_value(dev->parent, info);
>   }
>   
>   static int pldo_get_value(struct udevice *dev)
> @@ -1170,10 +1170,10 @@ static int pldo_get_value(struct udevice *dev)
>   	int pldo = dev->driver_data - 1;
>   	const struct rk8xx_reg_info *info = get_pldo_reg(dev->parent, pldo, 0);
>   
> -	return _ldo_get_value(dev, info);
> +	return _ldo_get_value(dev->parent, info);
>   }
>   
> -static int _ldo_set_value(struct udevice *dev, const struct rk8xx_reg_info *info, int uvolt)
> +static int _ldo_set_value(struct udevice *pmic, const struct rk8xx_reg_info *info, int uvolt)
>   {
>   	int mask = info->vsel_mask;
>   	int val;
> @@ -1189,7 +1189,7 @@ static int _ldo_set_value(struct udevice *dev, const struct rk8xx_reg_info *info
>   	debug("%s: volt=%d, reg=0x%x, mask=0x%x, val=0x%x\n",
>   	      __func__, uvolt, info->vsel_reg, mask, val);
>   
> -	return pmic_clrsetbits(dev->parent, info->vsel_reg, mask, val);
> +	return pmic_clrsetbits(pmic, info->vsel_reg, mask, val);
>   }
>   
>   static int ldo_set_value(struct udevice *dev, int uvolt)
> @@ -1197,7 +1197,7 @@ static int ldo_set_value(struct udevice *dev, int uvolt)
>   	int ldo = dev->driver_data - 1;
>   	const struct rk8xx_reg_info *info = get_ldo_reg(dev->parent, ldo, uvolt);
>   
> -	return _ldo_set_value(dev, info, uvolt);
> +	return _ldo_set_value(dev->parent, info, uvolt);
>   }
>   
>   static int nldo_set_value(struct udevice *dev, int uvolt)
> @@ -1205,7 +1205,7 @@ static int nldo_set_value(struct udevice *dev, int uvolt)
>   	int nldo = dev->driver_data - 1;
>   	const struct rk8xx_reg_info *info = get_nldo_reg(dev->parent, nldo, uvolt);
>   
> -	return _ldo_set_value(dev, info, uvolt);
> +	return _ldo_set_value(dev->parent, info, uvolt);
>   }
>   
>   static int pldo_set_value(struct udevice *dev, int uvolt)
> @@ -1213,7 +1213,7 @@ static int pldo_set_value(struct udevice *dev, int uvolt)
>   	int pldo = dev->driver_data - 1;
>   	const struct rk8xx_reg_info *info = get_pldo_reg(dev->parent, pldo, uvolt);
>   
> -	return _ldo_set_value(dev, info, uvolt);
> +	return _ldo_set_value(dev->parent, info, uvolt);
>   }
>   
>   static int _ldo_set_suspend_value(struct udevice *pmic, const struct rk8xx_reg_info *info, int uvolt)
>

Re: [PATCH 3/3] regulator: rk8xx: clarify operator precedence

[Kever Yang]

On 2024/6/5 17:33, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
>
> My linter complains that the order isn't clear enough so let's put
> parentheses around the ternary condition to make it happy.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   drivers/power/regulator/rk8xx.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/power/regulator/rk8xx.c b/drivers/power/regulator/rk8xx.c
> index bd5a37e718f..3125835bc07 100644
> --- a/drivers/power/regulator/rk8xx.c
> +++ b/drivers/power/regulator/rk8xx.c
> @@ -520,7 +520,7 @@ static int _buck_get_enable(struct udevice *pmic, int buck)
>   	if (ret < 0)
>   		return ret;
>   
> -	return ret & mask ? true : false;
> +	return (ret & mask) ? true : false;
>   }
>   
>   static int _buck_set_suspend_enable(struct udevice *pmic, int buck, bool enable)
> @@ -585,7 +585,7 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
>   		val = pmic_reg_read(pmic, RK816_REG_DCDC_SLP_EN);
>   		if (val < 0)
>   			return val;
> -		ret = val & mask ? 1 : 0;
> +		ret = (val & mask) ? 1 : 0;
>   		break;
>   	case RK806_ID:
>   		{
> @@ -608,7 +608,7 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
>   		val = pmic_reg_read(pmic, REG_SLEEP_SET_OFF1);
>   		if (val < 0)
>   			return val;
> -		ret = val & mask ? 0 : 1;
> +		ret = (val & mask) ? 0 : 1;
>   		break;
>   	case RK809_ID:
>   	case RK817_ID:
> @@ -620,7 +620,7 @@ static int _buck_get_suspend_enable(struct udevice *pmic, int buck)
>   		val = pmic_reg_read(pmic, RK817_POWER_SLP_EN(0));
>   		if (val < 0)
>   			return val;
> -		ret = val & mask ? 1 : 0;
> +		ret = (val & mask) ? 1 : 0;
>   		break;
>   	default:
>   		ret = -EINVAL;
> @@ -723,7 +723,7 @@ static int _ldo_get_enable(struct udevice *pmic, int ldo)
>   	if (ret < 0)
>   		return ret;
>   
> -	return ret & mask ? true : false;
> +	return (ret & mask) ? true : false;
>   }
>   
>   static int _nldo_get_enable(struct udevice *pmic, int nldo)
> @@ -980,7 +980,7 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
>   		val = pmic_reg_read(pmic, RK816_REG_LDO_SLP_EN);
>   		if (val < 0)
>   			return val;
> -		ret = val & mask ? 1 : 0;
> +		ret = (val & mask) ? 1 : 0;
>   		break;
>   	case RK808_ID:
>   	case RK818_ID:
> @@ -988,7 +988,7 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
>   		val = pmic_reg_read(pmic, REG_SLEEP_SET_OFF2);
>   		if (val < 0)
>   			return val;
> -		ret = val & mask ? 0 : 1;
> +		ret = (val & mask) ? 0 : 1;
>   		break;
>   	case RK809_ID:
>   	case RK817_ID:
> @@ -997,13 +997,13 @@ static int _ldo_get_suspend_enable(struct udevice *pmic, int ldo)
>   			val = pmic_reg_read(pmic, RK817_POWER_SLP_EN(0));
>   			if (val < 0)
>   				return val;
> -			ret = val & mask ? 1 : 0;
> +			ret = (val & mask) ? 1 : 0;
>   		} else {
>   			mask = 1 << ldo;
>   			val = pmic_reg_read(pmic, RK817_POWER_SLP_EN(1));
>   			if (val < 0)
>   				return val;
> -			ret = val & mask ? 1 : 0;
> +			ret = (val & mask) ? 1 : 0;
>   		}
>   		break;
>   	}
> @@ -1438,7 +1438,7 @@ static int switch_get_enable(struct udevice *dev)
>   	if (ret < 0)
>   		return ret;
>   
> -	return ret & mask ? true : false;
> +	return (ret & mask) ? true : false;
>   }
>   
>   static int switch_set_suspend_value(struct udevice *dev, int uvolt)
> @@ -1493,21 +1493,21 @@ static int switch_get_suspend_enable(struct udevice *dev)
>   		val = pmic_reg_read(dev->parent, REG_SLEEP_SET_OFF1);
>   		if (val < 0)
>   			return val;
> -		ret = val & mask ? 0 : 1;
> +		ret = (val & mask) ? 0 : 1;
>   		break;
>   	case RK809_ID:
>   		mask = 1 << (sw + 6);
>   		val = pmic_reg_read(dev->parent, RK817_POWER_SLP_EN(0));
>   		if (val < 0)
>   			return val;
> -		ret = val & mask ? 1 : 0;
> +		ret = (val & mask) ? 1 : 0;
>   		break;
>   	case RK818_ID:
>   		mask = 1 << 6;
>   		val = pmic_reg_read(dev->parent, REG_SLEEP_SET_OFF1);
>   		if (val < 0)
>   			return val;
> -		ret = val & mask ? 0 : 1;
> +		ret = (val & mask) ? 0 : 1;
>   		break;
>   	}
>   
>

Re: [PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value

[Simon Glass]

On Wed, 5 Jun 2024 at 03:33, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@cherry.de>
>
> _ldo_get_suspend_value and _ldo_set_suspend_value get passed the parent
> of the regulator (so the pmic) as first argument, therefore this udevice
> should be used for pmic_* callbacks instead of using the parent of the
> pmic.
>
> To avoid further confusion, let's rename the argument to pmic instead of
> dev, highlighting which kind of device we expect as argument.
>
> Fixes: f047e4ab9762 ("regulator: rk8xx: add indirection level for some ldo callbacks")
> Reported-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  drivers/power/regulator/rk8xx.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>  # chromebook-bob

Re: [PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions

[Simon Glass]

On Wed, 5 Jun 2024 at 03:33, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@cherry.de>
>
> For the sake of consistency, make all internal (starting with _)
> functions expect a pmic udevice instead of a regulator udevice.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  drivers/power/regulator/rk8xx.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>  # chromebook-bob

Re: [PATCH 3/3] regulator: rk8xx: clarify operator precedence

[Simon Glass]

On Wed, 5 Jun 2024 at 03:33, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@cherry.de>
>
> My linter complains that the order isn't clear enough so let's put
> parentheses around the ternary condition to make it happy.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  drivers/power/regulator/rk8xx.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>  # chromebook-bob