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 17:03:58 +0100
Message-ID: <20190617160358.GC5316@sirena.org.uk> (raw)
In-Reply-To: <CAOCk7NrwYezbVyLKOZdxgGRVemKtBmHKP+fSO0a2p3bCPNdW3w@mail.gmail.com>

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

On Mon, Jun 17, 2019 at 09:17:21AM -0600, Jeffrey Hugo wrote:
> On Mon, Jun 17, 2019 at 9:05 AM Mark Brown <broonie@kernel.org> wrote:

> > > +static int spmi_regulator_ftsmps426_set_voltage(struct regulator_dev *rdev,
> > > +                                           unsigned selector)
> > > +{

> > > +     mV = spmi_regulator_common_list_voltage(rdev, selector) / 1000;

> > This could just be a set_voltage_sel(), no need for it to be a
> > set_voltage() operation....

> 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?

> We already have code in the driver to convert a selector to the
> voltage.  Why duplicate
> that inline in spmi_regulator_ftsmps426_set_voltage?

Either work with selectors or work with voltages, don't mix and match
the two.

> > > +     switch (mode) {
> > > +     case REGULATOR_MODE_NORMAL:
> > > +             val = SPMI_FTSMPS426_MODE_HPM_MASK;
> > > +             break;
> > > +     case REGULATOR_MODE_FAST:
> > > +             val = SPMI_FTSMPS426_MODE_AUTO_MASK;
> > > +             break;
> > > +     default:
> > > +             val = SPMI_FTSMPS426_MODE_LPM_MASK;
> > > +             break;
> > > +     }

> > This should validate, it shouldn't just translate invalid values into
> > valid ones.

> Validate what?  The other defines are REGULATOR_MODE_IDLE
> and REGULATOR_MODE_STANDBY which correspond to the LPM
> mode.  Or are you suggesting that regulator framework is going to pass
> REGULATOR_MODE_INVALID to this operation?

You should be validating that the argument passed in is one that the
driver understands, your assumption will break if we add any new modes
and in any case there should be a 1:1 mapping between hardware and API
modes so you shouldn't be translating two different API modes into the
same hardware mode.

[-- 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 [this message]
2019-06-17 17:07           ` Jeffrey Hugo
2019-06-17 18:37             ` Mark Brown
     [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=20190617160358.GC5316@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 linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


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