Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230419-dynamic-vmon-v4-0-4d3734e62ada@skidata.com>
 <20230419-dynamic-vmon-v4-9-4d3734e62ada@skidata.com> <7097865e-d31b-099a-8b53-9e7264c64cd2@gmail.com>
In-Reply-To: <7097865e-d31b-099a-8b53-9e7264c64cd2@gmail.com>
From:   Benjamin Bara <bbara93@gmail.com>
Date:   Mon, 3 Jul 2023 20:50:11 +0200
Message-ID: <CAJpcXm4oqNcmU5__w4apNwa32aNEpAEjOb=4bZbwTUhYMCjueQ@mail.gmail.com>
Subject: Re: [PATCH RFC v4 09/13] regulator: implement mon_disable_reg_disabled
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        support.opensource@diasemi.com,
        DLG-Adam.Ward.opensource@dm.renesas.com,
        Martin Fuzzey <martin.fuzzey@flowbird.group>,
        linux-kernel@vger.kernel.org,
        Benjamin Bara <benjamin.bara@skidata.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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! ~~
>