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

2015-12-29 Thread Milo Kim 

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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-28 Thread Milo Kim 

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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-27 Thread Milo Kim 

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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] regulator: lp872x: Add missing of_match in regulators descriptions

2015-12-27 Thread Milo Kim 


On 23/12/15 19:58, Paul Kocialkowski wrote:

In order to select the regulators via of_find_regulator_by_node (and thus use
them in devicetree), defining of_match for each regulator is required.

Signed-off-by: Paul Kocialkowski <cont...@paulk.fr>


Acked-by: Milo Kim <milo@ti.com>


---
  drivers/regulator/lp872x.c | 15 +++
  1 file changed, 15 insertions(+)

diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index e5af072..9412353 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -520,6 +520,7 @@ static struct regulator_ops lp8725_buck_ops = {
  static struct regulator_desc lp8720_regulator_desc[] = {
{
.name = "ldo1",
+   .of_match = of_match_ptr("ldo1"),
.id = LP8720_ID_LDO1,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -533,6 +534,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo2",
+   .of_match = of_match_ptr("ldo2"),
.id = LP8720_ID_LDO2,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -546,6 +548,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo3",
+   .of_match = of_match_ptr("ldo3"),
.id = LP8720_ID_LDO3,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -559,6 +562,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo4",
+   .of_match = of_match_ptr("ldo4"),
.id = LP8720_ID_LDO4,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp8720_ldo4_vtbl),
@@ -572,6 +576,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "ldo5",
+   .of_match = of_match_ptr("ldo5"),
.id = LP8720_ID_LDO5,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -585,6 +590,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
},
{
.name = "buck",
+   .of_match = of_match_ptr("buck"),
.id = LP8720_ID_BUCK,
.ops = _buck_ops,
.n_voltages = ARRAY_SIZE(lp8720_buck_vtbl),
@@ -599,6 +605,7 @@ static struct regulator_desc lp8720_regulator_desc[] = {
  static struct regulator_desc lp8725_regulator_desc[] = {
{
.name = "ldo1",
+   .of_match = of_match_ptr("ldo1"),
.id = LP8725_ID_LDO1,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -612,6 +619,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo2",
+   .of_match = of_match_ptr("ldo2"),
.id = LP8725_ID_LDO2,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -625,6 +633,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo3",
+   .of_match = of_match_ptr("ldo3"),
.id = LP8725_ID_LDO3,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -638,6 +647,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo4",
+   .of_match = of_match_ptr("ldo4"),
.id = LP8725_ID_LDO4,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -651,6 +661,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "ldo5",
+   .of_match = of_match_ptr("ldo5"),
.id = LP8725_ID_LDO5,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp872x_ldo_vtbl),
@@ -664,6 +675,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "lilo1",
+   .of_match = of_match_ptr("lilo1"),
.id = LP8725_ID_LILO1,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
@@ -677,6 +689,7 @@ static struct regulator_desc lp8725_regulator_desc[] = {
},
{
.name = "lilo2",
+   .of_match = of_match_ptr("lilo2"),
.id = LP8725_ID_LILO2,
.ops = _ldo_ops,
.n_voltages = ARRAY_SIZE(lp8725_lilo_vtbl),
@@ -690,6 +703,7 @@ static struct regulator_desc lp8

Re: [PATCH 2/6] regulator: lp872x: Get rid of duplicate reference to DVS GPIO

2015-12-27 Thread Milo Kim 

On 23/12/15 19:58, Paul Kocialkowski wrote:

The lp872x structure holds a reference to the DVS GPIO, but it is never actually
used anywhere, since a first reference exists from the lp872x_dvs structure.

Signed-off-by: Paul Kocialkowski <cont...@paulk.fr>


Acked-by: Milo Kim <milo@ti.com>


---
  drivers/regulator/lp872x.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
index 9412353..19d7584 100644
--- a/drivers/regulator/lp872x.c
+++ b/drivers/regulator/lp872x.c
@@ -108,7 +108,6 @@ struct lp872x {
struct lp872x_platform_data *pdata;
int num_regulators;
enum lp872x_dvs_state dvs_pin;
-   int dvs_gpio;
  };

  /* LP8720/LP8725 shared voltage table for LDOs */
@@ -752,7 +751,6 @@ static int lp872x_init_dvs(struct lp872x *lp)
}

lp->dvs_pin = pinstate;
-   lp->dvs_gpio = gpio;

return 0;



--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-27 Thread Milo Kim 

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", _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-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] irqchip: omap-intc: Remove duplicate setup for IRQ chip type handler

2015-12-21 Thread Milo Kim 
Some OMAP interrupt controllers use generic level detection, so
handle_level_irq() is used as the chip type handler.
Allocated IRQ chip type handler doesn't need to set it again because
irq_alloc_domain_generic_chips() has already registered it.

Tested with BeagleBoneBlack Rev C.

Cc: Tony Lindgren <t...@atomide.com>
Cc: Thomas Gleixner <t...@linutronix.de>
Cc: Jason Cooper <ja...@lakedaemon.net>
Cc: Marc Zyngier <marc.zyng...@arm.com>
Cc: linux-omap@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Milo Kim <milo@ti.com>
---
 drivers/irqchip/irq-omap-intc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/irqchip/irq-omap-intc.c b/drivers/irqchip/irq-omap-intc.c
index 8587d0f..ed25175 100644
--- a/drivers/irqchip/irq-omap-intc.c
+++ b/drivers/irqchip/irq-omap-intc.c
@@ -207,7 +207,6 @@ static int __init omap_alloc_gc_of(struct irq_domain *d, 
void __iomem *base)
ct = gc->chip_types;
 
ct->type = IRQ_TYPE_LEVEL_MASK;
-   ct->handler = handle_level_irq;
 
ct->chip.irq_ack = omap_mask_ack_irq;
ct->chip.irq_mask = irq_gc_mask_disable_reg;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html