[PATCH 0/3] rockchip: rk8xx: fix broken [np]ldo callbacks
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,
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;
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;
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
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)
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)
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
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; }
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;@@ -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;@@ -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;@@ -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;@@ -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;@@ -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;
ret = val & mask ? 1 : 0;
ret = (val & mask) ? 1 : 0;@@ -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;
ret = val & mask ? 1 : 0;
ret = (val & mask) ? 1 : 0;
ret = val & mask ? 0 : 1;
ret = (val & mask) ? 0 : 1;-- 2.45.1
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;@@ -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;@@ -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;@@ -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;@@ -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;@@ -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;
ret = val & mask ? 1 : 0;
ret = (val & mask) ? 1 : 0;@@ -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;
ret = val & mask ? 1 : 0;
ret = (val & mask) ? 1 : 0;
ret = val & mask ? 0 : 1;
ret = (val & mask) ? 0 : 1;
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
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
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
participants (6)
-
Anand Moon -
Kever Yang -
Mattijs Korpershoek -
Quentin Schulz -
Quentin Schulz -
Simon Glass