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 <cont...@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: signature.asc
Description: This is a digitally signed message part

Reply via email to