* [PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value
2024-06-05 9:33 [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks Quentin Schulz
@ 2024-06-05 9:33 ` Quentin Schulz
2024-06-06 6:45 ` Kever Yang
2024-06-06 15:04 ` Simon Glass
2024-06-05 9:33 ` [PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions Quentin Schulz
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Quentin Schulz @ 2024-06-05 9:33 UTC (permalink / raw)
To: Jaehoon Chung, Tom Rini, Kever Yang; +Cc: Simon Glass, u-boot, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value
2024-06-05 9:33 ` [PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value Quentin Schulz
@ 2024-06-06 6:45 ` Kever Yang
2024-06-06 15:04 ` Simon Glass
1 sibling, 0 replies; 13+ messages in thread
From: Kever Yang @ 2024-06-06 6:45 UTC (permalink / raw)
To: Quentin Schulz, Jaehoon Chung, Tom Rini
Cc: Simon Glass, u-boot, Quentin Schulz
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;
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value
2024-06-05 9:33 ` [PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value Quentin Schulz
2024-06-06 6:45 ` Kever Yang
@ 2024-06-06 15:04 ` Simon Glass
1 sibling, 0 replies; 13+ messages in thread
From: Simon Glass @ 2024-06-06 15:04 UTC (permalink / raw)
To: Quentin Schulz
Cc: Jaehoon Chung, Tom Rini, Kever Yang, u-boot, Quentin Schulz
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions
2024-06-05 9:33 [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks Quentin Schulz
2024-06-05 9:33 ` [PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value Quentin Schulz
@ 2024-06-05 9:33 ` Quentin Schulz
2024-06-06 6:45 ` Kever Yang
2024-06-06 15:04 ` Simon Glass
2024-06-05 9:33 ` [PATCH 3/3] regulator: rk8xx: clarify operator precedence Quentin Schulz
2024-06-05 13:11 ` [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks Anand Moon
3 siblings, 2 replies; 13+ messages in thread
From: Quentin Schulz @ 2024-06-05 9:33 UTC (permalink / raw)
To: Jaehoon Chung, Tom Rini, Kever Yang; +Cc: Simon Glass, u-boot, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions
2024-06-05 9:33 ` [PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions Quentin Schulz
@ 2024-06-06 6:45 ` Kever Yang
2024-06-06 15:04 ` Simon Glass
1 sibling, 0 replies; 13+ messages in thread
From: Kever Yang @ 2024-06-06 6:45 UTC (permalink / raw)
To: Quentin Schulz, Jaehoon Chung, Tom Rini
Cc: Simon Glass, u-boot, Quentin Schulz
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)
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions
2024-06-05 9:33 ` [PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions Quentin Schulz
2024-06-06 6:45 ` Kever Yang
@ 2024-06-06 15:04 ` Simon Glass
1 sibling, 0 replies; 13+ messages in thread
From: Simon Glass @ 2024-06-06 15:04 UTC (permalink / raw)
To: Quentin Schulz
Cc: Jaehoon Chung, Tom Rini, Kever Yang, u-boot, Quentin Schulz
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] regulator: rk8xx: clarify operator precedence
2024-06-05 9:33 [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks Quentin Schulz
2024-06-05 9:33 ` [PATCH 1/3] regulator: rk8xx: fix incorrect device used for _ldo_[sg]et_suspend_value Quentin Schulz
2024-06-05 9:33 ` [PATCH 2/3] regulator: rk8xx: pass pmic udevice instead of regulator to all internal functions Quentin Schulz
@ 2024-06-05 9:33 ` Quentin Schulz
2024-06-05 11:20 ` Mattijs Korpershoek
` (2 more replies)
2024-06-05 13:11 ` [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks Anand Moon
3 siblings, 3 replies; 13+ messages in thread
From: Quentin Schulz @ 2024-06-05 9:33 UTC (permalink / raw)
To: Jaehoon Chung, Tom Rini, Kever Yang; +Cc: Simon Glass, u-boot, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] regulator: rk8xx: clarify operator precedence
2024-06-05 9:33 ` [PATCH 3/3] regulator: rk8xx: clarify operator precedence Quentin Schulz
@ 2024-06-05 11:20 ` Mattijs Korpershoek
2024-06-06 6:46 ` Kever Yang
2024-06-06 15:04 ` Simon Glass
2 siblings, 0 replies; 13+ messages in thread
From: Mattijs Korpershoek @ 2024-06-05 11:20 UTC (permalink / raw)
To: Quentin Schulz, Jaehoon Chung, Tom Rini, Kever Yang
Cc: Simon Glass, u-boot, Quentin Schulz
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] regulator: rk8xx: clarify operator precedence
2024-06-05 9:33 ` [PATCH 3/3] regulator: rk8xx: clarify operator precedence Quentin Schulz
2024-06-05 11:20 ` Mattijs Korpershoek
@ 2024-06-06 6:46 ` Kever Yang
2024-06-06 15:04 ` Simon Glass
2 siblings, 0 replies; 13+ messages in thread
From: Kever Yang @ 2024-06-06 6:46 UTC (permalink / raw)
To: Quentin Schulz, Jaehoon Chung, Tom Rini
Cc: Simon Glass, u-boot, Quentin Schulz
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;
> }
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] regulator: rk8xx: clarify operator precedence
2024-06-05 9:33 ` [PATCH 3/3] regulator: rk8xx: clarify operator precedence Quentin Schulz
2024-06-05 11:20 ` Mattijs Korpershoek
2024-06-06 6:46 ` Kever Yang
@ 2024-06-06 15:04 ` Simon Glass
2 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2024-06-06 15:04 UTC (permalink / raw)
To: Quentin Schulz
Cc: Jaehoon Chung, Tom Rini, Kever Yang, u-boot, Quentin Schulz
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks
2024-06-05 9:33 [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks Quentin Schulz
` (2 preceding siblings ...)
2024-06-05 9:33 ` [PATCH 3/3] regulator: rk8xx: clarify operator precedence Quentin Schulz
@ 2024-06-05 13:11 ` Anand Moon
2024-06-05 16:00 ` Quentin Schulz
3 siblings, 1 reply; 13+ messages in thread
From: Anand Moon @ 2024-06-05 13:11 UTC (permalink / raw)
To: Quentin Schulz
Cc: Jaehoon Chung, Tom Rini, Kever Yang, Simon Glass, u-boot,
Quentin Schulz
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>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks
2024-06-05 13:11 ` [PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks Anand Moon
@ 2024-06-05 16:00 ` Quentin Schulz
0 siblings, 0 replies; 13+ messages in thread
From: Quentin Schulz @ 2024-06-05 16:00 UTC (permalink / raw)
To: Anand Moon, Quentin Schulz
Cc: Jaehoon Chung, Tom Rini, Kever Yang, Simon Glass, u-boot
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
^ permalink raw reply [flat|nested] 13+ messages in thread