From: Benjamin Bara <[email protected]>
The mon_disable_reg_set_{higher,lower} properties disable all dt-enabled
monitors when the value of the regulator is changed to a higher or lower
one.
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/regulator/core.c | 41 +++++++++++++++++++++++++++++++++++++----
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b37dcafff407..74b9c12d38e9 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3643,6 +3643,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
int min_uV, int max_uV,
unsigned *selector)
{
+ const struct regulator_desc *desc = rdev->desc;
struct pre_voltage_change_data data;
int ret;
@@ -3654,7 +3655,18 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
if (ret & NOTIFY_STOP_MASK)
return -EINVAL;
- ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
+ if (min_uV > data.old_uV || max_uV > data.old_uV) {
+ ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
+ if (ret)
+ return ret;
+ }
+ if (min_uV < data.old_uV || max_uV < data.old_uV) {
+ ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
+ if (ret)
+ return ret;
+ }
+
+ ret = desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
if (ret >= 0)
return ret;
@@ -3667,6 +3679,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
int uV, unsigned selector)
{
+ const struct regulator_desc *desc = rdev->desc;
struct pre_voltage_change_data data;
int ret;
@@ -3678,7 +3691,18 @@ static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
if (ret & NOTIFY_STOP_MASK)
return -EINVAL;
- ret = rdev->desc->ops->set_voltage_sel(rdev, selector);
+ if (uV > data.old_uV) {
+ ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
+ if (ret)
+ return ret;
+ }
+ if (uV < data.old_uV) {
+ ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
+ if (ret)
+ return ret;
+ }
+
+ ret = desc->ops->set_voltage_sel(rdev, selector);
if (ret >= 0)
return ret;
@@ -3780,7 +3804,8 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
int best_val = 0;
unsigned int selector;
int old_selector = -1;
- const struct regulator_ops *ops = rdev->desc->ops;
+ const struct regulator_desc *desc = rdev->desc;
+ const struct regulator_ops *ops = desc->ops;
int old_uV = regulator_get_voltage_rdev(rdev);
trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
@@ -3819,7 +3844,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
selector = ret;
if (old_selector == selector)
ret = 0;
- else if (rdev->desc->vsel_step)
+ else if (desc->vsel_step)
ret = _regulator_set_voltage_sel_step(
rdev, best_val, selector);
else
@@ -3874,6 +3899,14 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
out:
trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
+ /* if setting voltage failed, ignore monitoring error. */
+ if (ret)
+ monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
+ desc->mon_disable_reg_set_lower);
+ else
+ ret = monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
+ desc->mon_disable_reg_set_lower);
+
return ret;
}
--
2.34.1
On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> The mon_disable_reg_set_{higher,lower} properties disable all dt-enabled
> monitors when the value of the regulator is changed to a higher or lower
> one.
>
> Signed-off-by: Benjamin Bara <[email protected]>
> ---
> drivers/regulator/core.c | 41 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index b37dcafff407..74b9c12d38e9 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3643,6 +3643,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
> int min_uV, int max_uV,
> unsigned *selector)
> {
> + const struct regulator_desc *desc = rdev->desc;
> struct pre_voltage_change_data data;
> int ret;
>
> @@ -3654,7 +3655,18 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
> if (ret & NOTIFY_STOP_MASK)
> return -EINVAL;
>
> - ret = rdev->desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
> + if (min_uV > data.old_uV || max_uV > data.old_uV) {
> + ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
> + if (ret)
> + return ret;
Here, as per comments to previous patches, the logic would be more
obvious for me if this was:
if (desc->mon_disable_reg_set_higher &&
(min_uV > data.old_uV || max_uV > data.old_uV)) {
ret = monitors_disable(...)
> + }
> + if (min_uV < data.old_uV || max_uV < data.old_uV) {
> + ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
> + if (ret)
> + return ret;
> + }
I guess you guess what I think of the above by now :)
> +
> + ret = desc->ops->set_voltage(rdev, min_uV, max_uV, selector);
> if (ret >= 0)
> return ret;
>
> @@ -3667,6 +3679,7 @@ static int _regulator_call_set_voltage(struct regulator_dev *rdev,
> static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
> int uV, unsigned selector)
> {
> + const struct regulator_desc *desc = rdev->desc;
> struct pre_voltage_change_data data;
> int ret;
>
> @@ -3678,7 +3691,18 @@ static int _regulator_call_set_voltage_sel(struct regulator_dev *rdev,
> if (ret & NOTIFY_STOP_MASK)
> return -EINVAL;
>
> - ret = rdev->desc->ops->set_voltage_sel(rdev, selector);
> + if (uV > data.old_uV) {
> + ret = monitors_disable(rdev, desc->mon_disable_reg_set_higher);
> + if (ret)
> + return ret;
> + }
> + if (uV < data.old_uV) {
> + ret = monitors_disable(rdev, desc->mon_disable_reg_set_lower);
> + if (ret)
> + return ret;
> + }
Here I would also pull the check from monitors_disable() to these
callers just to explicitly show the logic.
> +
> + ret = desc->ops->set_voltage_sel(rdev, selector);
> if (ret >= 0)
> return ret;
>
> @@ -3780,7 +3804,8 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
> int best_val = 0;
> unsigned int selector;
> int old_selector = -1;
> - const struct regulator_ops *ops = rdev->desc->ops;
> + const struct regulator_desc *desc = rdev->desc;
> + const struct regulator_ops *ops = desc->ops;
> int old_uV = regulator_get_voltage_rdev(rdev);
>
> trace_regulator_set_voltage(rdev_get_name(rdev), min_uV, max_uV);
> @@ -3819,7 +3844,7 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
> selector = ret;
> if (old_selector == selector)
> ret = 0;
> - else if (rdev->desc->vsel_step)
> + else if (desc->vsel_step)
> ret = _regulator_set_voltage_sel_step(
> rdev, best_val, selector);
> else
> @@ -3874,6 +3899,14 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
> out:
> trace_regulator_set_voltage_complete(rdev_get_name(rdev), best_val);
>
> + /* if setting voltage failed, ignore monitoring error. */
> + if (ret)
> + monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
> + desc->mon_disable_reg_set_lower);
> + else
> + ret = monitors_reenable(rdev, desc->mon_disable_reg_set_higher |
> + desc->mon_disable_reg_set_lower);
Here as well.
> +
> return ret;
> }
Well, pulling the check from monitors_*() to callers will increase line
count quite a bit. Still, my personal take on this is that the logic is
easier to follow that way. I, however, am fine also with the way it is
done in these patches if you think the line count matters more.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~