| Message ID | 1450868319-20513-5-git-send-email-contact@paulk.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote: > + 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; > + } This isn't really adding support for the enable GPIO as the changelog suggests, it's requesting but not managing the GPIO. Since there is core support for manging enable GPIOs this seems especially silly, please tell the core about the GPIO and then it will work at runtime too.
Le mercredi 23 décembre 2015 à 11:56 +0000, Mark Brown a écrit : > On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote: > > > + 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; > > + } > > This isn't really adding support for the enable GPIO as the changelog > suggests, it's requesting but not managing the GPIO. Since there is > core support for manging enable GPIOs this seems especially silly, > please tell the core about the GPIO and then it will work at runtime > too. You're right, I had clearly overlooked this, thanks for pointing it out. Will send v2 using GPIO handling from core.
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 <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
Hi Paul, On 23/12/15 20:56, Mark Brown wrote: > On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote: > >> + 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; >> + } > > This isn't really adding support for the enable GPIO as the changelog > suggests, it's requesting but not managing the GPIO. Since there is > core support for manging enable GPIOs this seems especially silly, > please tell the core about the GPIO and then it will work at runtime > too. > With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only can be controlled by external pin when CONFIG pin is grounded. Please see the description at page 5 of the datasheet. http://www.ti.com/lit/ds/symlink/lp8725.pdf Best regards, Milo
Hi Milo, thanks for the review, Le lundi 28 décembre 2015 à 09:56 +0900, Milo Kim a écrit : > Hi Paul, > > On 23/12/15 20:56, Mark Brown wrote: > > On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote: > > > >> + 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; > >> + } > > > > This isn't really adding support for the enable GPIO as the changelog > > suggests, it's requesting but not managing the GPIO. Since there is > > core support for manging enable GPIOs this seems especially silly, > > please tell the core about the GPIO and then it will work at runtime > > too. > > > > With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in > LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only > can be controlled by external pin when CONFIG pin is grounded. > > Please see the description at page 5 of the datasheet. > > http://www.ti.com/lit/ds/symlink/lp8725.pdf After reading the datasheets thoroughly, it seems to me that for the lp8720, the EN pin is used to enable the regulators output, which is a good fit for the core regulator GPIO framework, as there is no reason to keep it on when no regulator is in use. The serial interface is already available when EN=0 and regulators can be configured in that state. The lp8725 seems seems to behave the same when CONFIG=0 (the datasheet clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if EN=0"). On the other hand, it is indeed used as a power-on pin when CONFIG=1. Since my intent here is to cover the lp8720 use case, I suggest that we implement this using the core regulator GPIO framework (I have patches ready for that) and leave out the case where CONFIG=1, which could be dealt with later by providing that piece of information via platform data (and devicetree) and then either use the regulator GPIO framework (when CONFIG=0 and default) or register the GPIO within the driver and keep it on at all times (when CONFIG=1). I could most certainly implement that behaviour, but I'd rather leave it to someone else (or at least the testing) since I don't have any lp8725 to play with. What do you think?
Hi Paul, On 29/12/15 07:49, Paul Kocialkowski wrote: > Hi Milo, thanks for the review, > > Le lundi 28 décembre 2015 à 09:56 +0900, Milo Kim a écrit : >> Hi Paul, >> >> On 23/12/15 20:56, Mark Brown wrote: >>> On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote: >>> >>>> + 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; >>>> + } >>> >>> This isn't really adding support for the enable GPIO as the changelog >>> suggests, it's requesting but not managing the GPIO. Since there is >>> core support for manging enable GPIOs this seems especially silly, >>> please tell the core about the GPIO and then it will work at runtime >>> too. >>> >> >> With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in >> LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only >> can be controlled by external pin when CONFIG pin is grounded. >> >> Please see the description at page 5 of the datasheet. >> >> http://www.ti.com/lit/ds/symlink/lp8725.pdf > > After reading the datasheets thoroughly, it seems to me that for the > lp8720, the EN pin is used to enable the regulators output, which is a > good fit for the core regulator GPIO framework, as there is no reason to > keep it on when no regulator is in use. The serial interface is already > available when EN=0 and regulators can be configured in that state. The > lp8725 seems seems to behave the same when CONFIG=0 (the datasheet > clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if > EN=0"). On the other hand, it is indeed used as a power-on pin when > CONFIG=1. I think it's different use case. LP8720/5 are designed for system PMU, so some regulators are enabled by default after the device is on. EN pin is used for turning on/off the chip. This pin does not control regulator outputs directly. It's separate functional block in the silicon. On the other hand, 'ena_gpio' is used for each regulator control itself. For example, WM8994 has two LDOs which are controlled by external pins. LDOs are enabled/disabled through LDO1ENA and LDO2ENA pins. In this case, 'ena_gpio' is used. http://www.cirrus.com/en/pubs/proDatasheet/WM8994_v4.4.pdf (please refer to page 224 and 225) Best regards, Milo
Hi Milo, Le mardi 29 décembre 2015 à 09:45 +0900, Milo Kim a écrit : > Hi Paul, > > On 29/12/15 07:49, Paul Kocialkowski wrote: > > Hi Milo, thanks for the review, > > > > Le lundi 28 décembre 2015 à 09:56 +0900, Milo Kim a écrit : > >> Hi Paul, > >> > >> On 23/12/15 20:56, Mark Brown wrote: > >>> On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote: > >>> > >>>> + 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; > >>>> + } > >>> > >>> This isn't really adding support for the enable GPIO as the changelog > >>> suggests, it's requesting but not managing the GPIO. Since there is > >>> core support for manging enable GPIOs this seems especially silly, > >>> please tell the core about the GPIO and then it will work at runtime > >>> too. > >>> > >> > >> With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in > >> LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only > >> can be controlled by external pin when CONFIG pin is grounded. > >> > >> Please see the description at page 5 of the datasheet. > >> > >> http://www.ti.com/lit/ds/symlink/lp8725.pdf > > > > After reading the datasheets thoroughly, it seems to me that for the > > lp8720, the EN pin is used to enable the regulators output, which is a > > good fit for the core regulator GPIO framework, as there is no reason to > > keep it on when no regulator is in use. The serial interface is already > > available when EN=0 and regulators can be configured in that state. The > > lp8725 seems seems to behave the same when CONFIG=0 (the datasheet > > clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if > > EN=0"). On the other hand, it is indeed used as a power-on pin when > > CONFIG=1. > > I think it's different use case. LP8720/5 are designed for system PMU, > so some regulators are enabled by default after the device is on. EN pin > is used for turning on/off the chip. This pin does not control regulator > outputs directly. It's separate functional block in the silicon. Well, I really don't understand why the EN pin would turn on/off the chip. All it does it enable the regulators outputs (by entering IDLE mode), the serial block is already available in STANDBY state. If we want some regulators enabled at boot, the best thing to do seems to be to request the GPIO with the GPIOF_INIT_HIGH flag, as done in e.g. the max8952 regulator driver: if (pdata->reg_data->constraints.boot_on) config.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH; > On the other hand, 'ena_gpio' is used for each regulator control itself. > For example, WM8994 has two LDOs which are controlled by external pins. > LDOs are enabled/disabled through LDO1ENA and LDO2ENA pins. In this > case, 'ena_gpio' is used. Of course, but the ena_gpio feature is also a good fit for a global enable pin, as the GPIO can be shared by multiple regulators of the same chip, which is what we have here. In my opinion, using the ena_gpio feature is a good fit, as we don't need to keep the EN pin high when no regulator is used. > http://www.cirrus.com/en/pubs/proDatasheet/WM8994_v4.4.pdf > (please refer to page 224 and 225) Cheers,
On Wed, Dec 23, 2015 at 11:58:37AM +0100, 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. > > 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. Should be "-gpios" instead of "-gpio". Rob
Le mardi 29 décembre 2015 à 14:02 -0600, Rob Herring a écrit : > On Wed, Dec 23, 2015 at 11:58:37AM +0100, 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. > > > > 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. > > Should be "-gpios" instead of "-gpio". Care to comment why? There is only one GPIO that can be used here, since there is only one single EN pin. I thought this matched what is done already with "ti,dvs-gpio". Thanks!
On Tue, Dec 29, 2015 at 3:26 PM, Paul Kocialkowski <contact@paulk.fr> wrote: > Le mardi 29 décembre 2015 à 14:02 -0600, Rob Herring a écrit : >> On Wed, Dec 23, 2015 at 11:58:37AM +0100, 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. >> > >> > 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. >> >> Should be "-gpios" instead of "-gpio". > > Care to comment why? There is only one GPIO that can be used here, since > there is only one single EN pin. I thought this matched what is done > already with "ti,dvs-gpio". To be consistent. We use "clocks" and "interrupts" always whether one or more for example. -gpio is documented as deprecated now. Rob
Hi Paul, On 29/12/15 20:13, Paul Kocialkowski wrote: > Hi Milo, > > Le mardi 29 décembre 2015 à 09:45 +0900, Milo Kim a écrit : >> Hi Paul, >> >> On 29/12/15 07:49, Paul Kocialkowski wrote: >>> Hi Milo, thanks for the review, >>> >>> Le lundi 28 décembre 2015 à 09:56 +0900, Milo Kim a écrit : >>>> Hi Paul, >>>> >>>> On 23/12/15 20:56, Mark Brown wrote: >>>>> On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote: >>>>> >>>>>> + 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; >>>>>> + } >>>>> >>>>> This isn't really adding support for the enable GPIO as the changelog >>>>> suggests, it's requesting but not managing the GPIO. Since there is >>>>> core support for manging enable GPIOs this seems especially silly, >>>>> please tell the core about the GPIO and then it will work at runtime >>>>> too. >>>>> >>>> >>>> With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in >>>> LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only >>>> can be controlled by external pin when CONFIG pin is grounded. >>>> >>>> Please see the description at page 5 of the datasheet. >>>> >>>> http://www.ti.com/lit/ds/symlink/lp8725.pdf >>> >>> After reading the datasheets thoroughly, it seems to me that for the >>> lp8720, the EN pin is used to enable the regulators output, which is a >>> good fit for the core regulator GPIO framework, as there is no reason to >>> keep it on when no regulator is in use. The serial interface is already >>> available when EN=0 and regulators can be configured in that state. The >>> lp8725 seems seems to behave the same when CONFIG=0 (the datasheet >>> clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if >>> EN=0"). On the other hand, it is indeed used as a power-on pin when >>> CONFIG=1. >> >> I think it's different use case. LP8720/5 are designed for system PMU, >> so some regulators are enabled by default after the device is on. EN pin >> is used for turning on/off the chip. This pin does not control regulator >> outputs directly. It's separate functional block in the silicon. > > Well, I really don't understand why the EN pin would turn on/off the > chip. All it does it enable the regulators outputs (by entering IDLE > mode), the serial block is already available in STANDBY state. > > If we want some regulators enabled at boot, the best thing to do seems > to be to request the GPIO with the GPIOF_INIT_HIGH flag, as done in e.g. > the max8952 regulator driver: > > if (pdata->reg_data->constraints.boot_on) > config.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH; According to MAX8952 datasheet, output regulator is enabled/disabled using EN pin, so ena_gpio is used correctly. However, LP8720/5 regulators are enabled/disabled through I2C command. Only few regulators of LP8725 can be on/off by separate external pins. (B2_EN and LDO3_EN) Please note that EN pin in LP8720/5 is not the control pin for enabling/disabling regulators. Best regards, Milo
Hi Milo, Le mercredi 30 décembre 2015 à 09:22 +0900, Milo Kim a écrit : > Hi Paul, > > On 29/12/15 20:13, Paul Kocialkowski wrote: > > Hi Milo, > > > > Le mardi 29 décembre 2015 à 09:45 +0900, Milo Kim a écrit : > >> Hi Paul, > >> > >> On 29/12/15 07:49, Paul Kocialkowski wrote: > >>> Hi Milo, thanks for the review, > >>> > >>> Le lundi 28 décembre 2015 à 09:56 +0900, Milo Kim a écrit : > >>>> Hi Paul, > >>>> > >>>> On 23/12/15 20:56, Mark Brown wrote: > >>>>> On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote: > >>>>> > >>>>>> + 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; > >>>>>> + } > >>>>> > >>>>> This isn't really adding support for the enable GPIO as the changelog > >>>>> suggests, it's requesting but not managing the GPIO. Since there is > >>>>> core support for manging enable GPIOs this seems especially silly, > >>>>> please tell the core about the GPIO and then it will work at runtime > >>>>> too. > >>>>> > >>>> > >>>> With reference to my previous mail, external GPIOs for LDO3 and BUCK2 in > >>>> LP8725 can be specified through regulator_config.ena_gpio. BUCK2 only > >>>> can be controlled by external pin when CONFIG pin is grounded. > >>>> > >>>> Please see the description at page 5 of the datasheet. > >>>> > >>>> http://www.ti.com/lit/ds/symlink/lp8725.pdf > >>> > >>> After reading the datasheets thoroughly, it seems to me that for the > >>> lp8720, the EN pin is used to enable the regulators output, which is a > >>> good fit for the core regulator GPIO framework, as there is no reason to > >>> keep it on when no regulator is in use. The serial interface is already > >>> available when EN=0 and regulators can be configured in that state. The > >>> lp8725 seems seems to behave the same when CONFIG=0 (the datasheet > >>> clearly states: "CONFIG=0: EN=1 turns on outputs or standby mode if > >>> EN=0"). On the other hand, it is indeed used as a power-on pin when > >>> CONFIG=1. > >> > >> I think it's different use case. LP8720/5 are designed for system PMU, > >> so some regulators are enabled by default after the device is on. EN pin > >> is used for turning on/off the chip. This pin does not control regulator > >> outputs directly. It's separate functional block in the silicon. > > > > Well, I really don't understand why the EN pin would turn on/off the > > chip. All it does it enable the regulators outputs (by entering IDLE > > mode), the serial block is already available in STANDBY state. > > > > If we want some regulators enabled at boot, the best thing to do seems > > to be to request the GPIO with the GPIOF_INIT_HIGH flag, as done in e.g. > > the max8952 regulator driver: > > > > if (pdata->reg_data->constraints.boot_on) > > config.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH; > > According to MAX8952 datasheet, output regulator is enabled/disabled > using EN pin, so ena_gpio is used correctly. > However, LP8720/5 regulators are enabled/disabled through I2C command. > Only few regulators of LP8725 can be on/off by separate external pins. > (B2_EN and LDO3_EN) > Please note that EN pin in LP8720/5 is not the control pin for > enabling/disabling regulators. Oh, I should have mentionned that I also suggest making it possible to use *both* an enable GPIO and registers to enable a regulator. You are of course right in saying that the GPIO alone is not enough, and the core regulator framework will currently only do GPIO or registers to enable not both. In my opinion, it would be more elegant to adapt the core regulator framework to first enable the GPIO and then call the regulator enable ops callback instead of handling the GPIO in the driver. However, if that change is not welcome to the core regulator framework, we'll have to deal with the GPIO in the driver (and probably keep it enabled at all times there). Cheers,
On Wed, Dec 30, 2015 at 09:35:21AM +0100, Paul Kocialkowski wrote: > In my opinion, it would be more elegant to adapt the core regulator > framework to first enable the GPIO and then call the regulator enable > ops callback instead of handling the GPIO in the driver. Why would we want to actively manage both things at runtime? It's more work, what do we gain from it?
Le mercredi 30 décembre 2015 à 16:33 +0000, Mark Brown a écrit : > On Wed, Dec 30, 2015 at 09:35:21AM +0100, Paul Kocialkowski wrote: > > > In my opinion, it would be more elegant to adapt the core regulator > > framework to first enable the GPIO and then call the regulator enable > > ops callback instead of handling the GPIO in the driver. > > Why would we want to actively manage both things at runtime? It's more > work, what do we gain from it? Well, I figured that it would be best to disable the EN pin when we're not using any of the regulators, since that allows the chip to enter standby mode (and thus consume less power). Implementing that logic in the driver seems very redundant when we're one step away from doing it with the core regulator framework. It is also likely that in the future, other chips will use and need to handle a global enable pin for the same purpose, while allowing regulator configuration and enable via registers. It also doesn't hurt regulators that only use a GPIO for enable. The diff to do this is very minimal, I already have a patch ready, that I could send if there is a chance for it to get through.
On Wed, Dec 30, 2015 at 07:37:19PM +0100, Paul Kocialkowski wrote: > Le mercredi 30 décembre 2015 à 16:33 +0000, Mark Brown a écrit : > > On Wed, Dec 30, 2015 at 09:35:21AM +0100, Paul Kocialkowski wrote: > > > In my opinion, it would be more elegant to adapt the core regulator > > > framework to first enable the GPIO and then call the regulator enable > > > ops callback instead of handling the GPIO in the driver. > > Why would we want to actively manage both things at runtime? It's more > > work, what do we gain from it? > Well, I figured that it would be best to disable the EN pin when we're > not using any of the regulators, since that allows the chip to enter > standby mode (and thus consume less power). This doesn't sound like it's anything to do with the regulators, that's a chip wide power management function which should be implemented via runtime PM if there's any value in implementing it at all (if the device is a primary PMIC normally this would be handled by the CPU core when it enters low power state without any software). It's not something we should be considering on a per regulator basis since it's at the chip level and on a per regulator basis it's not doing anything useful for the reasons above. > It also doesn't hurt regulators that only use a GPIO for enable. It causes problems for any device with an optional GPIO, it means that we end up mantaining both GPIO and register which as I've said a couple of times now defeats the point of having the GPIO.
Le jeudi 31 décembre 2015 à 21:40 +0000, Mark Brown a écrit : > On Wed, Dec 30, 2015 at 07:37:19PM +0100, Paul Kocialkowski wrote: > > Le mercredi 30 décembre 2015 à 16:33 +0000, Mark Brown a écrit : > > > On Wed, Dec 30, 2015 at 09:35:21AM +0100, Paul Kocialkowski wrote: > > > > > In my opinion, it would be more elegant to adapt the core regulator > > > > framework to first enable the GPIO and then call the regulator enable > > > > ops callback instead of handling the GPIO in the driver. > > > > Why would we want to actively manage both things at runtime? It's more > > > work, what do we gain from it? > > > Well, I figured that it would be best to disable the EN pin when we're > > not using any of the regulators, since that allows the chip to enter > > standby mode (and thus consume less power). > > This doesn't sound like it's anything to do with the regulators, that's > a chip wide power management function which should be implemented via > runtime PM if there's any value in implementing it at all (if the device > is a primary PMIC normally this would be handled by the CPU core when it > enters low power state without any software). It's not something we > should be considering on a per regulator basis since it's at the chip > level and on a per regulator basis it's not doing anything useful for > the reasons above. I understand, thanks for pointing this out. Well, for my use case, there is no use in disabling the chip at any point as it powers the external mmc. Would you agree to have the enable pin handled directly (and by that, I mean enabled once, when requested, as I first suggested in the patchset) in the driver then? > > It also doesn't hurt regulators that only use a GPIO for enable. > > It causes problems for any device with an optional GPIO, it means that > we end up mantaining both GPIO and register which as I've said a couple > of times now defeats the point of having the GPIO. Fair enough.
On Thu, Dec 31, 2015 at 10:59:06PM +0100, Paul Kocialkowski wrote: > I understand, thanks for pointing this out. Well, for my use case, there > is no use in disabling the chip at any point as it powers the external > mmc. Presumably someone might decide not to use the MMC in some case (perhaps only mounting it when explicitly needed in order to save power for example, or the MMC subsystem might figure out a way to power down an idle MMC block device). > Would you agree to have the enable pin handled directly (and by that, I > mean enabled once, when requested, as I first suggested in the patchset) > in the driver then? That's probably fine, or do it via runtime PM (the framework is fairly simple to use, I'll probably go add support in the core for it in the next day or two as this seems like a sensible use case). I can't remember if this device is a MFD or not and I'm just on my way out the door.
Le jeudi 31 décembre 2015 à 22:14 +0000, Mark Brown a écrit : > On Thu, Dec 31, 2015 at 10:59:06PM +0100, Paul Kocialkowski wrote: > > > I understand, thanks for pointing this out. Well, for my use case, there > > is no use in disabling the chip at any point as it powers the external > > mmc. > > Presumably someone might decide not to use the MMC in some case (perhaps > only mounting it when explicitly needed in order to save power for > example, or the MMC subsystem might figure out a way to power down an > idle MMC block device). Makes sense, I'll keep that in mind. > > Would you agree to have the enable pin handled directly (and by that, I > > mean enabled once, when requested, as I first suggested in the patchset) > > in the driver then? > > That's probably fine, or do it via runtime PM (the framework is fairly > simple to use, I'll probably go add support in the core for it in the > next day or two as this seems like a sensible use case). I can't > remember if this device is a MFD or not and I'm just on my way out the > door. Runtime PM seems like a good fit (though I hadn't heard about it before: you can guess I'm fairly new to kernel development), please let me know whether you end up implementing it so I can try to handle the GPIO this way. Thanks!
Le jeudi 31 décembre 2015 à 22:14 +0000, Mark Brown a écrit : > On Thu, Dec 31, 2015 at 10:59:06PM +0100, Paul Kocialkowski wrote: > > > I understand, thanks for pointing this out. Well, for my use case, there > > is no use in disabling the chip at any point as it powers the external > > mmc. > > Presumably someone might decide not to use the MMC in some case (perhaps > only mounting it when explicitly needed in order to save power for > example, or the MMC subsystem might figure out a way to power down an > idle MMC block device). > > > Would you agree to have the enable pin handled directly (and by that, I > > mean enabled once, when requested, as I first suggested in the patchset) > > in the driver then? > > That's probably fine, or do it via runtime PM (the framework is fairly > simple to use, I'll probably go add support in the core for it in the > next day or two as this seems like a sensible use case). I can't > remember if this device is a MFD or not and I'm just on my way out the > door. Is there some git tree I can work with that has regulator runtime PM support at this point? I'll certainly end up handling the GPIO in the driver otherwise.
On Sat, Jan 16, 2016 at 08:32:13AM +0100, Paul Kocialkowski wrote: > Is there some git tree I can work with that has regulator runtime PM > support at this point? I'll certainly end up handling the GPIO in the > driver otherwise. No, the last week was much more busy than I was expecting. I'm hoping for this week (or you could have a go at implementing it, it'd be adding the PM operations in _regulator_enable() or _regulator_do_enable() if a flag was set in the regulator_desc).
Hi, Le lundi 18 janvier 2016 à 16:32 +0000, Mark Brown a écrit : > On Sat, Jan 16, 2016 at 08:32:13AM +0100, Paul Kocialkowski wrote: > > > Is there some git tree I can work with that has regulator runtime PM > > support at this point? I'll certainly end up handling the GPIO in the > > driver otherwise. > > No, the last week was much more busy than I was expecting. I'm hoping > for this week (or you could have a go at implementing it, it'd be adding > the PM operations in _regulator_enable() or _regulator_do_enable() if a > flag was set in the regulator_desc). I decided to go with a function simply requesting the GPIO for v2. Support for runtime PM can be added later. I'll get back to you on this on the thread related to your patches, since it appears to be somewhat broken for now. Thanks!
Hi, Le mardi 29 décembre 2015 à 15:55 -0600, Rob Herring a écrit : > On Tue, Dec 29, 2015 at 3:26 PM, Paul Kocialkowski <contact@paulk.fr> wrote: > > Le mardi 29 décembre 2015 à 14:02 -0600, Rob Herring a écrit : > >> On Wed, Dec 23, 2015 at 11:58:37AM +0100, 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. > >> > > >> > 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. > >> > >> Should be "-gpios" instead of "-gpio". Thanks for the review Rob, I have just submitted v2 with those suggestions integrated. > > Care to comment why? There is only one GPIO that can be used here, since > > there is only one single EN pin. I thought this matched what is done > > already with "ti,dvs-gpio". > > To be consistent. We use "clocks" and "interrupts" always whether one > or more for example. > > -gpio is documented as deprecated now. > > Rob
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
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. 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) +{ + 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; + } + + 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); } 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); + if (of_get_child_count(np) == 0) goto out; diff --git a/include/linux/regulator/lp872x.h b/include/linux/regulator/lp872x.h index 132e05c..44316dd 100644 --- a/include/linux/regulator/lp872x.h +++ b/include/linux/regulator/lp872x.h @@ -79,12 +79,14 @@ struct lp872x_regulator_data { * @update_config : if LP872X_GENERAL_CFG register is updated, set true * @regulator_data : platform regulator id and init data * @dvs : dvs data for buck voltage control + * @enable_gpio : gpio pin number for enable control */ struct lp872x_platform_data { u8 general_config; bool update_config; struct lp872x_regulator_data regulator_data[LP872X_MAX_REGULATORS]; struct lp872x_dvs *dvs; + int enable_gpio; }; #endif
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. 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(-)