LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: David Collins <collinsd@codeaurora.org>
Cc: Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-arm-msm@vger.kernel.org,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	devicetree@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Stephen Boyd <sboyd@kernel.org>
Subject: Re: [PATCH v4 2/2] regulator: add QCOM RPMh regulator driver
Date: Tue, 29 May 2018 22:32:38 -0700	[thread overview]
Message-ID: <CAD=FV=WF+onhAAWrbmwcHiWyCjuZ=6vvnhodUsia=Ps-da6_4A@mail.gmail.com> (raw)
In-Reply-To: <7489cd65fedb8a31488cf8188885759bcd4820ce.1527040878.git.collinsd@codeaurora.org>

Hi,

On Tue, May 22, 2018 at 7:43 PM, David Collins <collinsd@codeaurora.org> wrote:
> + * @ever_enabled:              Boolean indicating that the regulator has been
> + *                             explicitly enabled at least once.  Voltage
> + *                             requests should be cached when this flag is not
> + *                             set.

Do you really need this extra boolean?  Can't you just check if
"enabled" is still "-EINVAL"?  If it is then you don't pass the
voltage along.

...this would mean that you'd also need to send the voltage vote when
the regulator core tries to disable unused regulators at the end of
bootup, but that should be OK right?  If we never touched a regulator
anywhere at probe time and we're about to vote to disable it, we know
there's nobody requiring it to still be on.  We can vote for the
voltage now without fear of messing up a vote that the BIOS left in
place.

In theory this should also allow you to assert your vote about the
voltage of a regulator that has never been enabled, which (if I
understand correctly) you consider to be a feature.

---

Other than that comment, everything else here looks good to go if Mark
is good with it.  As per the previous threads there are some things
that I don't like a ton, but I feel it is between you and Mark to
decide if you're good with it.  A summary of those issues are:

1. I still believe that when we disable a regulator in Linux we should
tell RPMh that we vote for the lowest voltage.  ...but if Mark is
happy with the way the driver works now I won't push it.

2. As per my comments in the bindings patch, I still believe that
"qcom,drms-mode-max-microamps" belongs in the core.  Again, up to
Mark.

3. As per my comments in the bindings patch, I still believe that
we're just adding lots of noise to the DT most of the time by
specifying both "qcom,regulator-drms-modes" and
"regulator-allowed-modes".  Again, up to Mark.


-Doug

  reply	other threads:[~2018-05-30  5:32 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-23  2:43 [PATCH v4 0/2] " David Collins
2018-05-23  2:43 ` [PATCH v4 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings David Collins
2018-05-23 16:22   ` Rob Herring
2018-05-30  5:23   ` Doug Anderson
2018-05-30 10:37     ` Mark Brown
2018-05-30 14:54       ` Doug Anderson
2018-05-30 15:50         ` Mark Brown
2018-05-30 16:12           ` Doug Anderson
2018-05-30 16:20             ` Mark Brown
2018-05-30 16:24               ` Doug Anderson
2018-05-30 23:39                 ` David Collins
2018-05-31  0:34                   ` Doug Anderson
2018-05-31  1:03                     ` David Collins
2018-05-31 11:48                   ` Mark Brown
2018-06-01 21:41                     ` David Collins
2018-05-23  2:43 ` [PATCH v4 2/2] regulator: add QCOM RPMh regulator driver David Collins
2018-05-30  5:32   ` Doug Anderson [this message]
2018-05-30 23:58     ` David Collins
2018-05-30 16:33 ` [PATCH v4 0/2] " Mark Brown
2018-05-31  0:11   ` David Collins
2018-05-31 10:38     ` Mark Brown

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='CAD=FV=WF+onhAAWrbmwcHiWyCjuZ=6vvnhodUsia=Ps-da6_4A@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=broonie@kernel.org \
    --cc=collinsd@codeaurora.org \
    --cc=devicetree@vger.kernel.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=rnayak@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --subject='Re: [PATCH v4 2/2] regulator: add QCOM RPMh regulator driver' \
    /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

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).