Re: [PATCH 4/6] regulator: lp872x: Add enable GPIO pin support

[Paul Kocialkowski <contact@paulk.fr>]

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

Le lundi 28 décembre 2015 à 09:34 +0900, Milo Kim a écrit :
> Hi Paul,
> 
> Thanks for the patches. Please see my comments below.

Thanks for the review Milo, I have just submitted v2 with those
suggestions integrated.

> 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 <contact@paulk.fr>
> > ---
> >   .../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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]