linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Nishanth Menon <nm@ti.com>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Viresh Kumar <vireshk@kernel.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>,
	linux-pm@vger.kernel.org,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Rafael Wysocki <rjw@rjwysocki.net>,
	mka@chromium.org, sibis@codeaurora.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] opp: Reused enabled flag and remove regulator_enabled
Date: Sat, 15 Aug 2020 00:55:25 -0700	[thread overview]
Message-ID: <159747812515.33733.6233341429546003955@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <359b588928b7e58b009f786b17ddc088c6a7d18b.1597292833.git.viresh.kumar@linaro.org>

Quoting Viresh Kumar (2020-08-12 21:29:00)
> The common "enabled" flag can be used here instead of
> "regulator_enabled" now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Why not put this before the other patch? And mention that it will be
reused in another place soon? Then the previous patch won't look like
we're adding a variable to the struct when it is only used inside a
single function. Or at least squash it with the previous patch.

> ---
>  drivers/opp/core.c | 13 +++----------
>  drivers/opp/opp.h  |  2 --
>  2 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e8882e7fd8a5..5f5da257f58a 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
>          * Enable the regulator after setting its voltages, otherwise it breaks
>          * some boot-enabled regulators.
>          */
> -       if (unlikely(!opp_table->regulator_enabled)) {
> +       if (unlikely(!opp_table->enabled)) {
>                 ret = regulator_enable(reg);
>                 if (ret < 0)
>                         dev_warn(dev, "Failed to enable regulator: %d", ret);
> -               else
> -                       opp_table->regulator_enabled = true;

A quick glance makes this look unsafe now because we're only checking
'enabled' and not actually setting it when this function is called. I
have to go back to the previous patch to understand where enabled is now
set to confirm that it is OK. If it was all one patch all the context
would be here.

  reply	other threads:[~2020-08-15 22:11 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-10  7:06 [PATCH] opp: Fix dev_pm_opp_set_rate() to not return early Rajendra Nayak
2020-08-10 10:41 ` Sibi Sankar
2020-08-11 17:12 ` Matthias Kaehlcke
2020-08-12  4:30   ` Rajendra Nayak
2020-08-11 21:09 ` Stephen Boyd
2020-08-13  4:29   ` Viresh Kumar
2020-08-16 12:30     ` Rajendra Nayak
2020-08-13  4:28 ` [PATCH V2 1/4] opp: Enable resources again if they were disabled earlier Viresh Kumar
2020-08-13  4:28   ` [PATCH 2/4] opp: Track device's resources configuration status Viresh Kumar
2020-08-15  8:03     ` Stephen Boyd
2020-08-17  5:05       ` Viresh Kumar
2020-08-13  4:29   ` [PATCH 3/4] opp: Reused enabled flag and remove regulator_enabled Viresh Kumar
2020-08-15  7:55     ` Stephen Boyd [this message]
2020-08-20  7:19       ` Viresh Kumar
2020-08-18  7:10     ` Rajendra Nayak
2020-08-13  4:29   ` [PATCH 4/4] opp: Split out _opp_set_rate_zero() Viresh Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=159747812515.33733.6233341429546003955@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=nm@ti.com \
    --cc=rjw@rjwysocki.net \
    --cc=sibis@codeaurora.org \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --cc=vireshk@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).