Messages in this thread | | | From | Benjamin Bara <> | Date | Mon, 3 Jul 2023 20:50:11 +0200 | Subject | Re: [PATCH RFC v4 09/13] regulator: implement mon_disable_reg_disabled |
| |
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! ~~ >
| |