From: Ludovic Desroches <ludovic.desroches@atmel.com>
To: Wenyou Yang <wenyou.yang@atmel.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
Pawel Moll <pawel.moll@arm.com>,
Mark Rutland <mark.rutland@arm.com>,
Ian Campbell <ijc+devicetree@hellion.org.uk>,
Kumar Gala <galak@codeaurora.org>, <devicetree@vger.kernel.org>,
Nicolas Ferre <nicolas.ferre@atmel.com>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 1/2] regulator: act8865: support output voltage by VSET2[] bits
Date: Wed, 30 Sep 2015 15:21:07 +0200 [thread overview]
Message-ID: <20150930132107.GH31199@odux.rfo.atmel.com> (raw)
In-Reply-To: <1443597950-27849-2-git-send-email-wenyou.yang@atmel.com>
Hi Wenyou,
I see nothing wrong in it.
On Wed, Sep 30, 2015 at 03:25:49PM +0800, Wenyou Yang wrote:
> For the step-down DC/DC regulators, the output voltage is
> selectable by setting VSEL pin that when VSEL is low, output
> voltage is programmed by VSET1[] bits, and when VSEL is high,
> output voltage is programmed by VSET2[] bits.
>
> The DT property "active-semi,vsel-high" is used to specify
> the VSEL pin at high on the board.
>
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
Minor comment below, it's only my point of view.
Anyway, Reviewed-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
>
> Changes in v2:
> - fix missed argument of macro "ACT88xx_REG" for act8865_alt_regulators
> structure variable.
> - set variable "voltage_select" initial value to avoid compile warning.
>
> drivers/regulator/act8865-regulator.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
> index 896db16..f8d4cd3 100644
> --- a/drivers/regulator/act8865-regulator.c
> +++ b/drivers/regulator/act8865-regulator.c
> @@ -261,6 +261,16 @@ static const struct regulator_desc act8865_regulators[] = {
> ACT88xx_REG("LDO_REG4", ACT8865, LDO4, VSET, "inl67"),
> };
>
> +static const struct regulator_desc act8865_alt_regulators[] = {
> + ACT88xx_REG("DCDC_REG1", ACT8865, DCDC1, VSET2, "vp1"),
> + ACT88xx_REG("DCDC_REG2", ACT8865, DCDC2, VSET2, "vp2"),
> + ACT88xx_REG("DCDC_REG3", ACT8865, DCDC3, VSET2, "vp3"),
> + ACT88xx_REG("LDO_REG1", ACT8865, LDO1, VSET, "inl45"),
> + ACT88xx_REG("LDO_REG2", ACT8865, LDO2, VSET, "inl45"),
> + ACT88xx_REG("LDO_REG3", ACT8865, LDO3, VSET, "inl67"),
> + ACT88xx_REG("LDO_REG4", ACT8865, LDO4, VSET, "inl67"),
> +};
> +
> #ifdef CONFIG_OF
> static const struct of_device_id act8865_dt_ids[] = {
> { .compatible = "active-semi,act8600", .data = (void *)ACT8600 },
> @@ -413,6 +423,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
> struct act8865 *act8865;
> unsigned long type;
> int off_reg, off_mask;
> + int voltage_select = 0;
I think adding this variable is useless...
>
> pdata = dev_get_platdata(dev);
>
> @@ -424,6 +435,10 @@ static int act8865_pmic_probe(struct i2c_client *client,
> return -ENODEV;
>
> type = (unsigned long) id->data;
> +
> + voltage_select = !!of_get_property(dev->of_node,
> + "active-semi,vsel-high",
> + NULL);
> } else {
> type = i2c_id->driver_data;
> }
> @@ -442,8 +457,13 @@ static int act8865_pmic_probe(struct i2c_client *client,
> off_mask = ACT8846_OFF_SYSMASK;
> break;
> case ACT8865:
> - regulators = act8865_regulators;
> - num_regulators = ARRAY_SIZE(act8865_regulators);
> + if (voltage_select) {
I would directly do
if (of_get_property(dev->of_node, "active-semi,vsel-high", NULL) {
> + regulators = act8865_alt_regulators;
> + num_regulators = ARRAY_SIZE(act8865_alt_regulators);
> + } else {
> + regulators = act8865_regulators;
> + num_regulators = ARRAY_SIZE(act8865_regulators);
> + }
> off_reg = ACT8865_SYS_CTRL;
> off_mask = ACT8865_MSTROFF;
> break;
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2015-09-30 13:22 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-30 7:25 [PATCH v2 0/2] Add support to make output voltage determined " Wenyou Yang
2015-09-30 7:25 ` [PATCH v2 1/2] regulator: act8865: support output voltage " Wenyou Yang
2015-09-30 13:21 ` Ludovic Desroches [this message]
2015-09-30 7:25 ` [PATCH v2 2/2] regulator: act8865: add DT binding for property "active-semi,vsel-high" Wenyou Yang
2015-09-30 13:21 ` [PATCH v2 2/2] regulator: act8865: add DT binding for property "active-semi, vsel-high" Ludovic Desroches
2015-10-02 16:55 ` [PATCH v2 0/2] Add support to make output voltage determined by VSET2[] bits 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=20150930132107.GH31199@odux.rfo.atmel.com \
--to=ludovic.desroches@atmel.com \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=galak@codeaurora.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=nicolas.ferre@atmel.com \
--cc=pawel.moll@arm.com \
--cc=robh+dt@kernel.org \
--cc=wenyou.yang@atmel.com \
/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
all inboxes | Powered by JetHome®