Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751774AbbL1Aeo (ORCPT ); Sun, 27 Dec 2015 19:34:44 -0500 Received: from comal.ext.ti.com ([198.47.26.152]:54292 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750953AbbL1Aeg (ORCPT ); Sun, 27 Dec 2015 19:34:36 -0500 Subject: Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support To: Paul Kocialkowski References: <1450868319-20513-1-git-send-email-contact@paulk.fr> <1450868319-20513-5-git-send-email-contact@paulk.fr> CC: , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King , =?UTF-8?Q?Beno=c3=aet_Cousson?= , Tony Lindgren , Liam Girdwood , Mark Brown , , , From: Milo Kim Message-ID: <5680839E.4080800@ti.com> Date: Mon, 28 Dec 2015 09:34:38 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1450868319-20513-5-git-send-email-contact@paulk.fr> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3915 Lines: 121 Hi Paul, Thanks for the patches. Please see my comments below. On 23/12/15 19:58, Paul Kocialkowski wrote: > LP872x regulators are made active via the EN pin, which might be hooked to a > GPIO. This adds support for driving the GPIO high when the driver is in use. EN pin is used for enabling HW logic like I2C block. It's not regulator enable pin. Please check the block diagram in the datasheet. All regulators of LP8720 and LP8725 are controlled through I2C registers. Additionally, LP8725 provides external pin control for LDO3 and BUCK2. In this case, you can use 'regulator_config.ena_gpio' when a regulator is registered. > > Signed-off-by: Paul Kocialkowski > --- > .../devicetree/bindings/regulator/lp872x.txt | 1 + > drivers/regulator/lp872x.c | 33 ++++++++++++++++++++-- > include/linux/regulator/lp872x.h | 2 ++ > 3 files changed, 33 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/regulator/lp872x.txt b/Documentation/devicetree/bindings/regulator/lp872x.txt > index 7818318..0559c25 100644 > --- a/Documentation/devicetree/bindings/regulator/lp872x.txt > +++ b/Documentation/devicetree/bindings/regulator/lp872x.txt > @@ -28,6 +28,7 @@ Optional properties: > - ti,dvs-gpio: GPIO specifier for external DVS pin control of LP872x devices. > - ti,dvs-vsel: DVS selector. 0 = SEL_V1, 1 = SEL_V2. > - ti,dvs-state: initial DVS pin state. 0 = DVS_LOW, 1 = DVS_HIGH. > + - ti,enable-gpio: GPIO specifier for EN pin control of LP872x devices. Please use general property, "enable-gpios" instead of "ti,enable-gpio". > > Sub nodes for regulator_init_data > LP8720 has maximum 6 nodes. (child name: ldo1 ~ 5 and buck) > diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c > index 21c49d8..c8855f3 100644 > --- a/drivers/regulator/lp872x.c > +++ b/drivers/regulator/lp872x.c > @@ -726,6 +726,27 @@ static struct regulator_desc lp8725_regulator_desc[] = { > }, > }; > > +static int lp872x_init_enable(struct lp872x *lp) lp872x_enable_hw() would be better. > +{ > + int ret, gpio; > + > + if (!lp->pdata) > + return -EINVAL; > + > + gpio = lp->pdata->enable_gpio; > + if (!gpio_is_valid(gpio)) > + return 0; > + > + /* Always set enable GPIO high. */ > + ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN"); > + if (ret) { > + dev_err(lp->dev, "gpio request err: %d\n", ret); > + return ret; > + } LP8720 device needs max 200usec for startup time. LP8725 also requires enable time about 30ms. Please use usleep_range() after EN pin control. > + > + return 0; > +} > + > static int lp872x_init_dvs(struct lp872x *lp) > { > int ret, gpio; > @@ -763,14 +784,18 @@ static int lp872x_config(struct lp872x *lp) > int ret; > > if (!pdata || !pdata->update_config) > - goto init_dvs; > + goto init_dvs_enable; > > ret = lp872x_write_byte(lp, LP872X_GENERAL_CFG, pdata->general_config); > if (ret) > return ret; > > -init_dvs: > - return lp872x_init_dvs(lp); > +init_dvs_enable: > + ret = lp872x_init_dvs(lp); > + if (ret) > + return ret; > + > + return lp872x_init_enable(lp); > } Logic should be enabled prior to DVS configuration. And please call lp872x_enable_hw() in _probe(). > > static struct regulator_init_data > @@ -875,6 +900,8 @@ static struct lp872x_platform_data > of_property_read_u8(np, "ti,dvs-state", &dvs_state); > pdata->dvs->init_state = dvs_state ? DVS_HIGH : DVS_LOW; > > + pdata->enable_gpio = of_get_named_gpio(np, "ti,enable-gpio", 0); > + Please replace "ti,enable-gpio" with "enable-gpios". Best regards, Milo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/