Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Cc: lgirdwood@gmail.com, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	MSM <linux-arm-msm@vger.kernel.org>,
	lkml <linux-kernel@vger.kernel.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v4 4/7] regulator: qcom_spmi: Add support for PM8005
Date: Mon, 17 Jun 2019 19:37:57 +0100
Message-ID: <20190617183757.GH5316@sirena.org.uk> (raw)
In-Reply-To: <CAOCk7Nrnd7yJQ=0pO64iT+RfmsKfJW0x0RhrmSLkO_brFqZ2+Q@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3551 bytes --]

On Mon, Jun 17, 2019 at 11:07:18AM -0600, Jeffrey Hugo wrote:
> On Mon, Jun 17, 2019 at 10:04 AM Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Jun 17, 2019 at 09:17:21AM -0600, Jeffrey Hugo wrote:

Something really weird is going on with the word wrapping in your mail,
it looks like you're writing lines longer than 80 characters (120?) and
they're getting a newline added in the middle of the line to wrap them
without reflowing the paragraph.

> > > This is a set_voltage_sel() in spmi_ftsmps426_ops.  Is the issue because this
> > > function is "spmi_regulator_ftsmps426_set_voltage" and not
> > > "spmi_regulator_ftsmps426_set_voltage_sel"?

> > Well, that's certainly confusing naming and there's some confusion in
> > the code about what a selector is - it's supposed to be a raw register
> > value so if you're having to convert it into a voltage something is
> > going wrong.  Just implement a set_voltage() operation?

> No.

> Is what a selector is documented anywhere?  I just looked again and I
> haven't found
> documentation explaining that a selector is the raw register value.

Well, it doesn't *have* to be the raw register value, more accurately
it's some token which is useful for passing to and from the hardware; 
The documentation such as it
is is in the documentation of the list_voltage() operation (which is
what defines the selector values for a given driver).  

> Now I understand why this driver has the hardware to software selector
> translation.
> The selector being the raw register value seems to be a very limited
> assumption that
> I don't see working for more than very basic implementations.

Your idea of very basic implementations is how the overwhelming majority
of hardware is implemented.  Regulators with register maps will tend to
just have a bitfield where you set the voltage with each valid value in
that bitfield mapped to a single voltage, the exceptions tend to use
direct voltage values instead (and not support enumeration at all).

Looking at the driver I think it's got what the helpers are terming
pickable linear ranges (naming is hard) and could use those helpers.

> We've already figured out a virtualized selector mapping, I don't want
> to reimplement
> the complicated math to correctly map a requested voltage range to
> what the regulator
> can provide, and possibly get it wrong, or at the very least have two duplicate
> implementations.

I don't know what a "virtualized selector mapping" is...  We do have an
extensive range of helper implementations which cover the majority of
cases, are you sure that none of these cover what the hardware is doing?

> The naming is consistent with the rest of the driver, and the name
> seems long enough
> already.  Lets just keep this.

I appreciate that the existing code is a bit of a mess but as you can
see it's making it difficult to maintain so I'd rather push towards
making it cleaner, it looks like we're getting more and more devices
added to the driver so it's actually an issue.  

> Again, it would be nice if the documentation for regulator_ops
> indicated that a driver
> should only implement the voltage operations or the selector
> operations, not mix and
> match if that is your expectation.

Please add whatever documentation you're looking for here.  I genuinely
don't know where people would look for that and TBH I'm having a hard
time figuring out why you'd implement the operations asymmetrically.
It's not an issue that ever comes up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 21:24 [PATCH v4 0/7] PM8005 and PMS405 regulator support Jeffrey Hugo
2019-06-13 21:25 ` [PATCH v4 1/7] regulator: qcom_spmi: enable linear range info Jeffrey Hugo
2019-06-13 21:25   ` [PATCH v4 2/7] regulator: qcom_spmi: Refactor get_mode/set_mode Jeffrey Hugo
2019-06-13 21:32     ` Bjorn Andersson
2019-06-17 15:24     ` Applied "regulator: qcom_spmi: Refactor get_mode/set_mode" to the regulator tree Mark Brown
2019-06-17 15:24   ` Applied "regulator: qcom_spmi: enable linear range info" " Mark Brown
2019-06-13 21:25 ` [PATCH v4 3/7] dt-bindings: qcom_spmi: Document PM8005 regulators Jeffrey Hugo
2019-06-13 21:25   ` [PATCH v4 4/7] regulator: qcom_spmi: Add support for PM8005 Jeffrey Hugo
2019-06-17 15:05     ` Mark Brown
2019-06-17 15:17       ` Jeffrey Hugo
2019-06-17 16:03         ` Mark Brown
2019-06-17 17:07           ` Jeffrey Hugo
2019-06-17 18:37             ` Mark Brown [this message]
     [not found]               ` <CAOCk7NpbZwAreGpVCvF2yFBDJKbAxBZ23oncfF_SyEwoiC2+PQ@mail.gmail.com>
     [not found]                 ` <20190617192413.GI5316@sirena.org.uk>
2019-06-17 19:41                   ` Jeffrey Hugo
2019-06-13 21:26 ` [PATCH v4 5/7] arm64: dts: msm8998-mtp: Add pm8005_s1 regulator Jeffrey Hugo
2019-06-13 21:27 ` [PATCH v4 6/7] dt-bindings: qcom_spmi: Document pms405 support Jeffrey Hugo
2019-06-13 21:27   ` [PATCH v4 7/7] regulator: qcom_spmi: add PMS405 SPMI regulator Jeffrey Hugo
2019-06-13 21:37     ` Bjorn Andersson
2019-06-13 21:37   ` [PATCH v4 6/7] dt-bindings: qcom_spmi: Document pms405 support Bjorn Andersson
2019-06-17 14:58 ` [PATCH v4 0/7] PM8005 and PMS405 regulator support Mark Brown
2019-06-17 15:04   ` Jeffrey Hugo
2019-06-17 15:12     ` Mark Brown

Reply instructions:

You may reply publically 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=20190617183757.GH5316@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jeffrey.l.hugo@gmail.com \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@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

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git