From: Benjamin Bara <benjamin.bara@skidata.com>
The mon_disable_reg_disabled property disables all dt-enabled monitors
before a regulator is disabled. If an error occurs while disabling the
regulator, the monitors are enabled again.
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
drivers/regulator/core.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 873e53633698..b37dcafff407 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2965,7 +2965,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
trace_regulator_enable_complete(rdev_get_name(rdev));
- return 0;
+ return monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
}
/**
@@ -3124,8 +3124,13 @@ EXPORT_SYMBOL_GPL(regulator_enable);
static int _regulator_do_disable(struct regulator_dev *rdev)
{
+ const struct regulator_desc *desc = rdev->desc;
int ret;
+ ret = monitors_disable(rdev, desc->mon_disable_reg_disabled);
+ if (ret)
+ return ret;
+
trace_regulator_disable(rdev_get_name(rdev));
if (rdev->ena_pin) {
@@ -3136,13 +3141,13 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
rdev->ena_gpio_state = 0;
}
- } else if (rdev->desc->ops->disable) {
- ret = rdev->desc->ops->disable(rdev);
+ } else if (desc->ops->disable) {
+ ret = desc->ops->disable(rdev);
if (ret != 0)
return ret;
}
- if (rdev->desc->off_on_delay)
+ if (desc->off_on_delay)
rdev->last_off = ktime_get_boottime();
trace_regulator_disable_complete(rdev_get_name(rdev));
@@ -3180,6 +3185,7 @@ static int _regulator_disable(struct regulator *regulator)
_notifier_call_chain(rdev,
REGULATOR_EVENT_ABORT_DISABLE,
NULL);
+ monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
return ret;
}
_notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
@@ -3246,6 +3252,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
rdev_err(rdev, "failed to force disable: %pe\n", ERR_PTR(ret));
_notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
REGULATOR_EVENT_ABORT_DISABLE, NULL);
+ monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
return ret;
}
@@ -6422,8 +6429,10 @@ static int regulator_late_cleanup(struct device *dev, void *data)
*/
rdev_info(rdev, "disabling\n");
ret = _regulator_do_disable(rdev);
- if (ret != 0)
+ if (ret != 0) {
rdev_err(rdev, "couldn't disable: %pe\n", ERR_PTR(ret));
+ monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
+ }
} else {
/* The intention is that in future we will
* assume that full constraints are provided
--
2.34.1Hi deeee Ho Benjamin,
I hope your train back to home was not delayed too much ;)
On 6/20/23 23:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> The mon_disable_reg_disabled
The name of this always makes me to scratch my head a bit. (or, maybe it
is just the sunburns at my bald).
Do you think making it:
mon_disable_at_reg_disable or mon_disable_when_reg_disabled would be too
long?
> property disables all dt-enabled monitors
> before a regulator is disabled. If an error occurs while disabling the
> regulator, the monitors are enabled again.
>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> drivers/regulator/core.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 873e53633698..b37dcafff407 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2965,7 +2965,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
>
> trace_regulator_enable_complete(rdev_get_name(rdev));
>
> - return 0;
> + return monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
As I wrote in my comment to previous patch, I might find the logic a bit
more clear if the condition check was done here. Eg:
if (rdev->desc->mon_disable_when_reg_disabled)
return monitors_reenable(...);
return 0;
> }
>
> /**
> @@ -3124,8 +3124,13 @@ EXPORT_SYMBOL_GPL(regulator_enable);
>
> static int _regulator_do_disable(struct regulator_dev *rdev)
> {
> + const struct regulator_desc *desc = rdev->desc;
> int ret;
>
> + ret = monitors_disable(rdev, desc->mon_disable_reg_disabled);
> + if (ret)
> + return ret;
Similarly, for me the logic would be easier to follow if this was:
if (desc->mon_disable_when_reg_disabled)
monitors_disable(...);
> +
> trace_regulator_disable(rdev_get_name(rdev));
>
> if (rdev->ena_pin) {
> @@ -3136,13 +3141,13 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
> rdev->ena_gpio_state = 0;
> }
>
> - } else if (rdev->desc->ops->disable) {
> - ret = rdev->desc->ops->disable(rdev);
> + } else if (desc->ops->disable) {
> + ret = desc->ops->disable(rdev);
> if (ret != 0)
> return ret;
> }
>
> - if (rdev->desc->off_on_delay)
> + if (desc->off_on_delay)
> rdev->last_off = ktime_get_boottime();
>
> trace_regulator_disable_complete(rdev_get_name(rdev));
> @@ -3180,6 +3185,7 @@ static int _regulator_disable(struct regulator *regulator)
> _notifier_call_chain(rdev,
> REGULATOR_EVENT_ABORT_DISABLE,
> NULL);
> + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
same here,
> return ret;
> }
> _notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
> @@ -3246,6 +3252,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
> rdev_err(rdev, "failed to force disable: %pe\n", ERR_PTR(ret));
> _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
> REGULATOR_EVENT_ABORT_DISABLE, NULL);
> + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
here...
> return ret;
> }
>
> @@ -6422,8 +6429,10 @@ static int regulator_late_cleanup(struct device *dev, void *data)
> */
> rdev_info(rdev, "disabling\n");
> ret = _regulator_do_disable(rdev);
> - if (ret != 0)
> + if (ret != 0) {
> rdev_err(rdev, "couldn't disable: %pe\n", ERR_PTR(ret));
> + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
... and here.
> + }
> } else {
> /* The intention is that in future we will
> * assume that full constraints are provided
>
These were just very minor things. Mostly looks good for me.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Hi Matti!
On Mon, 3 Jul 2023 at 12:31, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> I hope your train back to home was not delayed too much ;)
Yes, much better this time :)
> On 6/20/23 23:03, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > The mon_disable_reg_disabled
>
> The name of this always makes me to scratch my head a bit. (or, maybe it
> is just the sunburns at my bald).
>
> Do you think making it:
> mon_disable_at_reg_disable or mon_disable_when_reg_disabled would be too
> long?
mons_to_disable_while_reg_off maybe fits better, or mons_off_while_reg_off.
Still not satisfied though, I will think about it - maybe something
better comes to
my mind.
> > property disables all dt-enabled monitors
> > before a regulator is disabled. If an error occurs while disabling the
> > regulator, the monitors are enabled again.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > drivers/regulator/core.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 873e53633698..b37dcafff407 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -2965,7 +2965,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
> >
> > trace_regulator_enable_complete(rdev_get_name(rdev));
> >
> > - return 0;
> > + return monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> As I wrote in my comment to previous patch, I might find the logic a bit
> more clear if the condition check was done here. Eg:
>
> if (rdev->desc->mon_disable_when_reg_disabled)
> return monitors_reenable(...);
>
> return 0;
Yes, thanks. I applied this to all the mentioned occasions.
> > }
> >
> > /**
> > @@ -3124,8 +3124,13 @@ EXPORT_SYMBOL_GPL(regulator_enable);
> >
> > static int _regulator_do_disable(struct regulator_dev *rdev)
> > {
> > + const struct regulator_desc *desc = rdev->desc;
> > int ret;
> >
> > + ret = monitors_disable(rdev, desc->mon_disable_reg_disabled);
> > + if (ret)
> > + return ret;
>
> Similarly, for me the logic would be easier to follow if this was:
>
> if (desc->mon_disable_when_reg_disabled)
> monitors_disable(...);
>
> > +
> > trace_regulator_disable(rdev_get_name(rdev));
> >
> > if (rdev->ena_pin) {
> > @@ -3136,13 +3141,13 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
> > rdev->ena_gpio_state = 0;
> > }
> >
> > - } else if (rdev->desc->ops->disable) {
> > - ret = rdev->desc->ops->disable(rdev);
> > + } else if (desc->ops->disable) {
> > + ret = desc->ops->disable(rdev);
> > if (ret != 0)
> > return ret;
> > }
> >
> > - if (rdev->desc->off_on_delay)
> > + if (desc->off_on_delay)
> > rdev->last_off = ktime_get_boottime();
> >
> > trace_regulator_disable_complete(rdev_get_name(rdev));
> > @@ -3180,6 +3185,7 @@ static int _regulator_disable(struct regulator *regulator)
> > _notifier_call_chain(rdev,
> > REGULATOR_EVENT_ABORT_DISABLE,
> > NULL);
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> same here,
>
> > return ret;
> > }
> > _notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
> > @@ -3246,6 +3252,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
> > rdev_err(rdev, "failed to force disable: %pe\n", ERR_PTR(ret));
> > _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
> > REGULATOR_EVENT_ABORT_DISABLE, NULL);
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> here...
>
> > return ret;
> > }
> >
> > @@ -6422,8 +6429,10 @@ static int regulator_late_cleanup(struct device *dev, void *data)
> > */
> > rdev_info(rdev, "disabling\n");
> > ret = _regulator_do_disable(rdev);
> > - if (ret != 0)
> > + if (ret != 0) {
> > rdev_err(rdev, "couldn't disable: %pe\n", ERR_PTR(ret));
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> ... and here.
> > + }
> > } else {
> > /* The intention is that in future we will
> > * assume that full constraints are provided
> >
>
> These were just very minor things. Mostly looks good for me.
Thanks!
Benjamin
> Yours,
> -- Matti
>
> --
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
>
> ~~ When things go utterly wrong vim users can always type :help! ~~
>
On Mon, 3 Jul 2023 at 12:31, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> Hi deeee Ho Benjamin,
>
> I hope your train back to home was not delayed too much ;)
>
> On 6/20/23 23:03, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > The mon_disable_reg_disabled
>
> The name of this always makes me to scratch my head a bit. (or, maybe it
> is just the sunburns at my bald).
>
> Do you think making it:
> mon_disable_at_reg_disable or mon_disable_when_reg_disabled would be too
> long?
>
> > property disables all dt-enabled monitors
> > before a regulator is disabled. If an error occurs while disabling the
> > regulator, the monitors are enabled again.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > drivers/regulator/core.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 873e53633698..b37dcafff407 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -2965,7 +2965,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
> >
> > trace_regulator_enable_complete(rdev_get_name(rdev));
> >
> > - return 0;
> > + return monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> As I wrote in my comment to previous patch, I might find the logic a bit
> more clear if the condition check was done here. Eg:
>
> if (rdev->desc->mon_disable_when_reg_disabled)
> return monitors_reenable(...);
>
> return 0;
>
> > }
> >
> > /**
> > @@ -3124,8 +3124,13 @@ EXPORT_SYMBOL_GPL(regulator_enable);
> >
> > static int _regulator_do_disable(struct regulator_dev *rdev)
> > {
> > + const struct regulator_desc *desc = rdev->desc;
> > int ret;
> >
> > + ret = monitors_disable(rdev, desc->mon_disable_reg_disabled);
> > + if (ret)
> > + return ret;
>
> Similarly, for me the logic would be easier to follow if this was:
>
> if (desc->mon_disable_when_reg_disabled)
> monitors_disable(...);
>
> > +
> > trace_regulator_disable(rdev_get_name(rdev));
> >
> > if (rdev->ena_pin) {
> > @@ -3136,13 +3141,13 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
> > rdev->ena_gpio_state = 0;
> > }
> >
> > - } else if (rdev->desc->ops->disable) {
> > - ret = rdev->desc->ops->disable(rdev);
> > + } else if (desc->ops->disable) {
> > + ret = desc->ops->disable(rdev);
> > if (ret != 0)
> > return ret;
> > }
> >
> > - if (rdev->desc->off_on_delay)
> > + if (desc->off_on_delay)
> > rdev->last_off = ktime_get_boottime();
> >
> > trace_regulator_disable_complete(rdev_get_name(rdev));
> > @@ -3180,6 +3185,7 @@ static int _regulator_disable(struct regulator *regulator)
> > _notifier_call_chain(rdev,
> > REGULATOR_EVENT_ABORT_DISABLE,
> > NULL);
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> same here,
>
> > return ret;
> > }
> > _notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
> > @@ -3246,6 +3252,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
> > rdev_err(rdev, "failed to force disable: %pe\n", ERR_PTR(ret));
> > _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
> > REGULATOR_EVENT_ABORT_DISABLE, NULL);
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> here...
>
> > return ret;
> > }
> >
> > @@ -6422,8 +6429,10 @@ static int regulator_late_cleanup(struct device *dev, void *data)
> > */
> > rdev_info(rdev, "disabling\n");
> > ret = _regulator_do_disable(rdev);
> > - if (ret != 0)
> > + if (ret != 0) {
> > rdev_err(rdev, "couldn't disable: %pe\n", ERR_PTR(ret));
> > + monitors_reenable(rdev, rdev->desc->mon_disable_reg_disabled);
>
> ... and here.
> > + }
> > } else {
> > /* The intention is that in future we will
> > * assume that full constraints are provided
> >
>
> These were just very minor things. Mostly looks good for me.
>
>
> Yours,
> -- Matti
>
> --
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
>
> ~~ When things go utterly wrong vim users can always type :help! ~~
>
© 2016 - 2025 Red Hat, Inc.