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@xxxxxxxx>
> > ---
> > .../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
