[PATCH] mfd: twl-core: One function call less in add_numbered_child() after error detection

2015-12-29 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Tue, 29 Dec 2015 19:29:08 +0100

The platform_device_put() function was called in one case by the
add_numbered_child() function during error handling even if the passed
variable "pdev" contained a null pointer.

Implementation details could be improved by the adjustment of jump targets
according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/mfd/twl-core.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 831696e..0d9350c 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -625,7 +625,7 @@ add_numbered_child(unsigned mod_no, const char *name, int 
num,
if (!pdev) {
dev_dbg(>client->dev, "can't alloc dev\n");
status = -ENOMEM;
-   goto err;
+   goto report_failure;
}
 
pdev->dev.parent = >client->dev;
@@ -634,7 +634,7 @@ add_numbered_child(unsigned mod_no, const char *name, int 
num,
status = platform_device_add_data(pdev, pdata, pdata_len);
if (status < 0) {
dev_dbg(>dev, "can't add platform_data\n");
-   goto err;
+   goto put_device;
}
}
 
@@ -647,21 +647,20 @@ add_numbered_child(unsigned mod_no, const char *name, int 
num,
status = platform_device_add_resources(pdev, r, irq1 ? 2 : 1);
if (status < 0) {
dev_dbg(>dev, "can't add irqs\n");
-   goto err;
+   goto put_device;
}
}
 
status = platform_device_add(pdev);
-   if (status == 0)
+   if (!status) {
device_init_wakeup(>dev, can_wakeup);
-
-err:
-   if (status < 0) {
-   platform_device_put(pdev);
-   dev_err(>client->dev, "can't add %s dev\n", name);
-   return ERR_PTR(status);
+   return >dev;
}
-   return >dev;
+put_device:
+   platform_device_put(pdev);
+report_failure:
+   dev_err(>client->dev, "can't add %s dev\n", name);
+   return ERR_PTR(status);
 }
 
 static inline struct device *add_child(unsigned mod_no, const char *name,
-- 
2.6.3

--
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 v3 2/3] wlcore/wl12xx: spi: add device tree support

2015-12-29 Thread Rob Herring 
On Mon, Dec 28, 2015 at 02:02:18PM +0200, Uri Mashiach wrote:
> Add DT support for the wl1271 SPI WiFi.
> 
> Add documentation file for the wl1271 SPI WiFi.
> 
> Signed-off-by: Uri Mashiach 
> Acked-by: Igor Grinberg 
> ---
> v1 -> v2: update interrupt documentation.
>   replace interrupts and interrupt-parent with interrupts-extended.

Why? interrupts-extended is really just for cases with 2 interrupt 
parents.

> IRQ parameters retrieved from spi_device instead of DT.
> remove redundant #ifdef CONFIG_OF
> v2 -> v3: Add MODULE_DEVICE_TABLE.
>   Remove #ifdef CONFIG_OF.
> Make the Kconfig symbol to depend on OF.
> Replace irqd_get_trigger_type() with irq_get_trigger_type().
> 
>  .../bindings/net/wireless/ti,wlcore,spi.txt| 34 

In any case:

Acked-by: Rob Herring 

>  drivers/net/wireless/ti/wlcore/Kconfig |  2 +-
>  drivers/net/wireless/ti/wlcore/spi.c   | 47 
> --
>  3 files changed, 78 insertions(+), 5 deletions(-)
>  create mode 100644 
> Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt
--
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/3] ti-st: use device handles and add device tree binding

2015-12-29 Thread Rob Herring 
On Wed, Dec 23, 2015 at 11:38:29AM +, Reizer, Eyal wrote:
> - Add support for getting the platform data which includes the uart
>   used and gpio pin used for enable from device tree.
> 
> - Fix the implementation for using device handle for the uart and
>   gpiod for the enable pin, instead of device name (as string) used
>   for the uart and pio number which are both bad practice.
> 
> Signed-off-by: Eyal Reizer 
> ---
>  Documentation/devicetree/bindings/misc/ti-st.txt |   42 ++
>  arch/arm/mach-omap2/pdata-quirks.c   |   16 ++-
>  drivers/misc/ti-st/st_kim.c  |  159 
> --
>  drivers/misc/ti-st/st_ll.c   |   16 ++-
>  include/linux/ti_wilink_st.h |   13 +-

I'd suggest you look at commit c0bd1b9e58959c5 (Revert "ti-st: add 
device tree support") first.

>  5 files changed, 190 insertions(+), 56 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/misc/ti-st.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/ti-st.txt 
> b/Documentation/devicetree/bindings/misc/ti-st.txt
> new file mode 100644
> index 000..4490da6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/ti-st.txt
> @@ -0,0 +1,42 @@
> +TI Wilink 6/7/8 (wl12xx/wl18xx) Shared transport driver

Bindings shouldn't be describing drivers...

> +
> +TI’s Wireless Connectivity chips support Bluetooth (BT), WiFi, and GPS
> +technology cores in a single die.
> +
> +Such a multi-core combo chip will be interfaced to the application processor
> +using a single physical port (like UART).
> +
> +Shared Transport (ST) software enables BT and GPS protocols or software
> +components to interact with their respective cores over single physical port.
> +ST uses logical channels, over physical transport, to communicate with
> +individual cores.
> +
> +Logical channels 1, 2, 3, and 4 are used for BT packets, channel 8 for FM,
> +channel 9 for GPS and channels 30, 31, 32, and 33 are used for Chip Power
> +Management (PM).

All this is irrelevant for a binding. 

> +
> +This node provides properties for passing parameters to the ti shared
> +transport driver.
> +
> +Required properties:
> + - compatible: should be the following:
> +* "kim" - ti-st parameters

Who is kim? Certainly not a description of a h/w block.

> +
> +Optional properties:
> + - nshutdown-gpios : specifies attributes for gpio ping used for enabling
> + the bluetooth,gps and FM sub systems
> + - serial-device : the phandle for the phisical uart used for interacting
> + with the wilink device

There have been multiple discussions on serial slave devices recently. 
I'm not going to accept any device binding without a common uart slave 
device binding first.

> + - flow_cntrl : Indicates if uart flow control is used
> + - flow_cntrl : uart baud rate in BPS

Typo here, but these should be part of a common serial slave binding.

Don't use '_' in property names.

> +
> +Example:
> +
> +kim {
> + compatible = "kim";
> + nshutdown-gpios = < 21 GPIO_ACTIVE_HIGH>;
> + serial-device = <>;
> + flow_cntrl = <1>;
> + flow_cntrl = <300>;
> +};
> +
--
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 2/2] dt-bindings: bus: ti-gpmc: Add AAD timings properties

2015-12-29 Thread Rob Herring 
On Mon, Dec 28, 2015 at 02:39:21PM +0100, Neil Armstrong wrote:
> In order to support advanced AAD timings, add these properties to the DT
> GPMC bindings.
> 
> Signed-off-by: Neil Armstrong 
> ---
>  Documentation/devicetree/bindings/bus/ti-gpmc.txt | 5 +
>  1 file changed, 5 insertions(+)

Acked-by: Rob Herring 
--
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-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

[PATCH v2 1/3] extcon: palmas: Add the support for VBUS detection by using GPIO

2015-12-29 Thread Chanwoo Choi 
From: Felipe Balbi 

This patch support for VBUS detection by using GPIO pin.

Signed-off-by: Felipe Balbi 
Signed-off-by: Chanwoo Choi 
---
 drivers/extcon/extcon-palmas.c | 50 ++
 include/linux/mfd/palmas.h |  3 +++
 2 files changed, 53 insertions(+)

diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
index 93c30a885740..885ee95a6a7b 100644
--- a/drivers/extcon/extcon-palmas.c
+++ b/drivers/extcon/extcon-palmas.c
@@ -216,11 +216,23 @@ static int palmas_usb_probe(struct platform_device *pdev)
return PTR_ERR(palmas_usb->id_gpiod);
}
 
+   palmas_usb->vbus_gpiod = devm_gpiod_get_optional(>dev, "vbus",
+   GPIOD_IN);
+   if (IS_ERR(palmas_usb->vbus_gpiod)) {
+   dev_err(>dev, "failed to get vbus gpio\n");
+   return PTR_ERR(palmas_usb->vbus_gpiod);
+   }
+
if (palmas_usb->enable_id_detection && palmas_usb->id_gpiod) {
palmas_usb->enable_id_detection = false;
palmas_usb->enable_gpio_id_detection = true;
}
 
+   if (palmas_usb->enable_vbus_detection && palmas_usb->vbus_gpiod) {
+   palmas_usb->enable_vbus_detection = false;
+   palmas_usb->enable_gpio_vbus_detection = true;
+   }
+
if (palmas_usb->enable_gpio_id_detection) {
u32 debounce;
 
@@ -311,6 +323,40 @@ static int palmas_usb_probe(struct platform_device *pdev)
palmas_usb->vbus_irq, status);
return status;
}
+   } else if (palmas_usb->enable_gpio_vbus_detection) {
+   /* remux GPIO_1 as VBUSDET */
+   status = palmas_update_bits(palmas,
+   PALMAS_PU_PD_OD_BASE,
+   PALMAS_PRIMARY_SECONDARY_PAD1,
+   PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_MASK,
+   (1 << PALMAS_PRIMARY_SECONDARY_PAD1_GPIO_1_SHIFT));
+   if (status < 0) {
+   dev_err(>dev, "can't remux GPIO1\n");
+   return status;
+   }
+
+   palmas_usb->vbus_otg_irq = regmap_irq_get_virq(palmas->irq_data,
+  PALMAS_VBUS_OTG_IRQ);
+   palmas_usb->gpio_vbus_irq = 
gpiod_to_irq(palmas_usb->vbus_gpiod);
+   if (palmas_usb->gpio_vbus_irq < 0) {
+   dev_err(>dev, "failed to get vbus irq\n");
+   return palmas_usb->gpio_vbus_irq;
+   }
+   status = devm_request_threaded_irq(>dev,
+   palmas_usb->gpio_vbus_irq,
+   NULL,
+   palmas_vbus_irq_handler,
+   IRQF_TRIGGER_FALLING |
+   IRQF_TRIGGER_RISING |
+   IRQF_ONESHOT |
+   IRQF_EARLY_RESUME,
+   "palmas_usb_vbus",
+   palmas_usb);
+   if (status < 0) {
+   dev_err(>dev,
+   "failed to request handler for vbus irq\n");
+   return status;
+   }
}
 
palmas_enable_irq(palmas_usb);
@@ -337,6 +383,8 @@ static int palmas_usb_suspend(struct device *dev)
if (device_may_wakeup(dev)) {
if (palmas_usb->enable_vbus_detection)
enable_irq_wake(palmas_usb->vbus_irq);
+   if (palmas_usb->enable_gpio_vbus_detection)
+   enable_irq_wake(palmas_usb->gpio_vbus_irq);
if (palmas_usb->enable_id_detection)
enable_irq_wake(palmas_usb->id_irq);
if (palmas_usb->enable_gpio_id_detection)
@@ -352,6 +400,8 @@ static int palmas_usb_resume(struct device *dev)
if (device_may_wakeup(dev)) {
if (palmas_usb->enable_vbus_detection)
disable_irq_wake(palmas_usb->vbus_irq);
+   if (palmas_usb->enable_gpio_vbus_detection)
+   disable_irq_wake(palmas_usb->gpio_vbus_irq);
if (palmas_usb->enable_id_detection)
disable_irq_wake(palmas_usb->id_irq);
if (palmas_usb->enable_gpio_id_detection)
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index 13e1d96935ed..7b2526f7bc9b 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -553,7 +553,9 @@ struct palmas_usb {
int vbus_irq;
 
int gpio_id_irq;
+   int gpio_vbus_irq;
struct gpio_desc *id_gpiod;
+

[PATCH v2 2/3] arm: boot: dts: beaglex15: Remove ID GPIO

2015-12-29 Thread Chanwoo Choi 
From: Felipe Balbi 

According to latest schematics [1], this board
leaves ID pin floating. It's not connected to
anything at all.

So let's remove it.

[1] 
https://github.com/beagleboard/beagleboard-x15/blob/master/BeagleBoard-X15_RevA2.pdf

Signed-off-by: Felipe Balbi 
Signed-off-by: Chanwoo Choi 
---
 arch/arm/boot/dts/am57xx-beagle-x15.dts | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/boot/dts/am57xx-beagle-x15.dts 
b/arch/arm/boot/dts/am57xx-beagle-x15.dts
index d9ba6b879fc1..be4990608c77 100644
--- a/arch/arm/boot/dts/am57xx-beagle-x15.dts
+++ b/arch/arm/boot/dts/am57xx-beagle-x15.dts
@@ -560,8 +560,6 @@
extcon_usb2: tps659038_usb {
compatible = "ti,palmas-usb-vid";
ti,enable-vbus-detection;
-   ti,enable-id-detection;
-   id-gpios = < 24 GPIO_ACTIVE_HIGH>;
};
 
};
-- 
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

Re: [PATCH 2/3] drivers: net: cpsw: fix error messages when using phy-handle DT property

2015-12-29 Thread Rob Herring 
On Tue, Dec 22, 2015 at 07:36:33PM -0500, David Rivshin (Allworx) wrote:
> From: David Rivshin 
> 
> The phy-handle, phy_id, and fixed-link properties are mutually exclusive,
> and only one need be specified. However if phy-handle was specified, an
> error message would complain about the lack of phy_id or fixed-link.
> 
> Also, if phy-handle was specified and the subsequent of_phy_connect()
> failed, the error message still referenced slaved->data->phy_id, which
> would be empty. Instead, use the name of the device_node as a useful
> identifier.
> 
> Fixes: 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
> Signed-off-by: David Rivshin 
> ---
> This would require some adjustments to backport to 4.3-stable due to
> other changes in this area. Let me know if you want a version of this
> against v4.3.3.
> 
>  Documentation/devicetree/bindings/net/cpsw.txt |  4 ++--

For the binding:

Acked-by: Rob Herring 
--
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 v2 0/3] arm: beaglex15: fix USB Gadget

2015-12-29 Thread Chanwoo Choi 
This patchset make the extcon-palmas.c more generic style and then send v2
patchset. I add the Felipe comment of v1 patchset as following:
But, I have not test on real board because I don't have it.
Just, I modify this patchset based on v1 patchset[1] from Felipe Balbi.

- Felipe comment of this patchset:
"Hi,

with the following patches I can get USB Gadget working
with my beagle x15 with today's Linus' tree.

regards".

[1] https://lkml.org/lkml/2015/11/12/508

Changes from v1:
(https://lkml.org/lkml/2015/11/12/508)
- Refactoring the extcon-palmas.c driver with more generic style to support
  the VBUS gpio pin.
- Use the 'vbus-gpio' property instead of 'interrupts-extended']

Felipe Balbi (3):
  extcon: palmas: Add the support for VBUS detection by using GPIO
  arm: boot: dts: beaglex15: Remove ID GPIO
  arm: boot: beaglex15: pass correct interrupt

 arch/arm/boot/dts/am57xx-beagle-x15.dts |  3 +-
 drivers/extcon/extcon-palmas.c  | 50 +
 include/linux/mfd/palmas.h  |  3 ++
 3 files changed, 54 insertions(+), 2 deletions(-)

-- 
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

[PATCH v2 3/3] arm: boot: beaglex15: pass correct interrupt

2015-12-29 Thread Chanwoo Choi 
From: Felipe Balbi 

According to latest schematics [1], GPIO_1/VBUSDET
on TPS659038 is tied to AM57x GPIO4_21. We can use
that as a VBUS interrupt, instead of relying on
PMIC's VBUS interrupts which don't seem to be firing
on x15 at all.

A follow up patch will add support for using this
GPIO-based interrupt mechanism for notifying about
VBUS.

[1] 
https://github.com/beagleboard/beagleboard-x15/blob/master/BeagleBoard-X15_RevA2.pdf

Signed-off-by: Felipe Balbi 
[cw00.choi: Use the 'vbus-gpio' property instead of 'interrupts-extended']
Signed-off-by: Chanwoo Choi 
---
 arch/arm/boot/dts/am57xx-beagle-x15.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/am57xx-beagle-x15.dts 
b/arch/arm/boot/dts/am57xx-beagle-x15.dts
index be4990608c77..3887ba89d2a5 100644
--- a/arch/arm/boot/dts/am57xx-beagle-x15.dts
+++ b/arch/arm/boot/dts/am57xx-beagle-x15.dts
@@ -560,6 +560,7 @@
extcon_usb2: tps659038_usb {
compatible = "ti,palmas-usb-vid";
ti,enable-vbus-detection;
+   vbus-gpio = < 21 GPIO_ACTIVE_HIGH>;
};
 
};
-- 
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

Routable IRQs

2015-12-29 Thread Felipe Balbi 

Hi Thomas & Jason,

I'm dealing with an interesting situation which I'm wondering if Linux
already support for.

Basically, in some TI SoCs we have what's referred to as Programmable
Real-Time Unit SubSystem (PRUSS). That's essentially a really simple,
low latency, single cycle architecture which is pretty darn good for bit
banging peripherals (ETH, SPI, I2C, UART, etc). It's very predicatable
because every instruction takes 5ns and interrupts don't cause
exceptions, they just get registered.

Anyway, the interesting part is that PRUSS has 64 events (on current
incarnations at least) and PRUSS has 10 physical IRQ lines to the ARM
land. Each of these 64 events can be routed to any of these 10 IRQ
lines. This might not be very useful on UP (AM335x & AM437x) other than
the fact that soft-IP drivers running on Linux would need to guarantee
they are the ones who should handle the IRQ. However, on SMP (AM57xx) we
could have real tangible benefits by means of IRQ affinity, etc.

So, the question is, what is there in IRQ subsystem today for routable
IRQ support ?

If a Diagram helps here's a simple one. Note that I'm not showing
details on the PRUSS side, but that side can also map events pretty much
any way it wants.

 .. ..
 |  HOST CPU  | |   PRUSS|
 || ||
 || ||
 |   irq0 |<-.--|evt0|
 ||  |  ||
 |   irq1 |  |  .---|evt1|
 ||  |  |   ||
 |   irq2 |  '--|evt2|
 || |   ||
 |   irq3 | |   ||
 || |   ||
 |   irq4 | |   | .  |
 || |   ||
 |   irq5 | |   | .  |
 || |   ||
 |   irq6 | |   | .  |
 || |   ||
 |   irq7 |<'   ||
 || ||
 |   irq8 | ||
 || ||
 |   irq9 |<|evtN|
 '' ''

Given this setup, what I want to do, is let soft-IP drivers running on
linux rely on standard *request_*irq() calls and DTS descrition. But I'm
still considering how/if we should describe the routing itself or just
go round-robin (i.o.w. irq0 -> evt0, irq1 -> evt1, ..., irq9 -> evt9,
irq0 -> evt10, ...).

Thoughts ?

-- 
balbi


signature.asc
Description: PGP signature

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

2015-12-29 Thread Paul Kocialkowski 
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 
> > ---
> >  .../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!


signature.asc
Description: This is a digitally signed message part

Re: [PATCH v2 1/3] devicetree/bindings: add reset-gpios and vcc-supply for panel-dpi

2015-12-29 Thread Rob Herring 
On Sun, Dec 20, 2015 at 12:13:20PM +0100, Uwe Kleine-König wrote:
> Some displays have a reset input and/or need a regulator to function
> properly. Allow to specify them for panel-dpi devices.
> 
> Signed-off-by: Uwe Kleine-König 
> ---
>  Documentation/devicetree/bindings/display/panel/panel-dpi.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.txt 
> b/Documentation/devicetree/bindings/display/panel/panel-dpi.txt
> index 216c894d4f99..b52ac52757df 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.txt
> +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.txt
> @@ -7,6 +7,8 @@ Required properties:
>  Optional properties:
>  - label: a symbolic name for the panel
>  - enable-gpios: panel enable gpio
> +- reset-gpios: GPIO to control the RESET pin

The problem with this in a generic binding is what if the panel has 
ordering requirements like enable gpio has to be inactive when reset 
is deasserted?

> +- vcc-supply: phandle of regulator that will be used to enable power to the 
> display

What if there are 2 supplies?

While there are limits to what can be described here, I'm okay with 
allowing these, so:

Acked-by: Rob Herring 

Rob
--
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-29 Thread Rob Herring 
On Tue, Dec 29, 2015 at 3:26 PM, Paul Kocialkowski  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 
>> > ---
>> >  .../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
--
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-29 Thread Rob Herring 
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 
> ---
>  .../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
--
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 3/7] omapfb: fix error return code

2015-12-29 Thread Tomi Valkeinen 

On 26/12/15 17:28, Julia Lawall wrote:
> Return a negative error code on failure.
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // 
> @@
> identifier ret; expression e1,e2;
> @@
> (
> if (\(ret < 0\|ret != 0\))
>  { ... return ret; }
> |
> ret = 0
> )
> ... when != ret = e1
> when != 
> *if(...)
> {
>   ... when != ret = e2
>   when forall
>  return ret;
> }
> // 
> 
> Signed-off-by: Julia Lawall 
> 
> ---
>  drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c |   12 
> +++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c 
> b/drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c
> index 677e254..fc4cfa9 100644
> --- a/drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c
> +++ b/drivers/video/fbdev/omap2/omapfb/displays/encoder-tpd12s015.c
> @@ -241,22 +241,28 @@ static int tpd_probe(struct platform_device *pdev)
>  
>   gpio = devm_gpiod_get_index_optional(>dev, NULL, 0,
>   GPIOD_OUT_LOW);
> - if (IS_ERR(gpio))
> + if (IS_ERR(gpio)) {
> + r = PTR_ERR(gpio);
>   goto err_gpio;
> + }
>  
>   ddata->ct_cp_hpd_gpio = gpio;
>  
>   gpio = devm_gpiod_get_index_optional(>dev, NULL, 1,
>   GPIOD_OUT_LOW);
> - if (IS_ERR(gpio))
> + if (IS_ERR(gpio)) {
> + r = PTR_ERR(gpio);
>   goto err_gpio;
> + }
>  
>   ddata->ls_oe_gpio = gpio;
>  
>   gpio = devm_gpiod_get_index(>dev, NULL, 2,
>   GPIOD_IN);
> - if (IS_ERR(gpio))
> + if (IS_ERR(gpio)) {
> + r = PTR_ERR(gpio);
>   goto err_gpio;
> + }
>  
>   ddata->hpd_gpio = gpio;

Thanks. Looks like recent changes to the driver break the error
handling. I'll just drop those patches from my for-next branch, and let
the author fix the patches.

 Tomi



signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH] thermal: Schedule a backup thermal shutdown workqueue after a known period of time to tackle failed poweroff

2015-12-29 Thread Keerthy 

Hi Nishanth,

On Monday 28 December 2015 11:11 PM, Nishanth Menon wrote:

On 12/20/2015 11:46 PM, Keerthy wrote:

+linux-pm.


Thanks for looping!




In few rare conditions like during boot up the orderly_poweroff
function might not be able to complete fully leaving the device
running at dangerously high temperatures. Hence adding a backup
workqueue to act after a known period of time and poweroff the device.





Suggested-by: Nishanth Menon 
Reported-by: Nishanth Menon 


The specific case I hit was a critical temperature event as soon as
the hwmon device was probed (the driver tmp102 was a kernel module).


Signed-off-by: Keerthy 
---
  drivers/thermal/Kconfig|  9 +
  drivers/thermal/thermal_core.c | 26 ++
  2 files changed, 35 insertions(+)

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 8cc4ac6..25584ee 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -92,6 +92,15 @@ config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR

  endchoice

+config THERMAL_BKUP_SHUTDOWN_DELAY_MS
+int "Thermal shutdown  backup delay in milli-seconds"
+depends on THERMAL
+default "5000"
+---help---
+The number of milliseconds to delay after orderly_poweroff call.


Probably needs a rephrase.


Delay in milliseconds before the backup thermal shutdown is triggered.




+
+Default: 5000 (5 seconds)
+
  config THERMAL_GOV_FAIR_SHARE
bool "Fair-share thermal governor"
help
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index d9e525c..b793857 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -61,6 +61,12 @@ static DEFINE_MUTEX(thermal_governor_lock);

  static struct thermal_governor *def_governor;

+#ifdef CONFIG_THERMAL_BKUP_SHUTDOWN_DELAY_MS
+#define BKUP_SHUTDOWN_DELAY CONFIG_THERMAL_BKUP_SHUTDOWN_DELAY_MS
+#else
+#define BKUP_SHUTDOWN_DELAY 5000
+#endif
+


I am not sure if this #ifdeffery is even needed.


Eduardo, Rui: If this is not the suggested technique, maybe you guys
could suggest how we could handle a case where userspace might be
hungup due to some reason and a case where a critical temperature
event in the middle of device probe was triggered?

Obviously, we'd like to take into consideration userspace latencies as
well- but that is very specific to fs being run.. not really a simple
problem, IMHO..


  static struct thermal_governor *__find_governor(const char *name)
  {
struct thermal_governor *pos;
@@ -423,6 +429,18 @@ static void handle_non_critical_trips(struct 
thermal_zone_device *tz,
   def_governor->throttle(tz, trip);
  }

+static void bkup_shutdown_func(struct work_struct *unused);
+static DECLARE_DELAYED_WORK(bkup_shutdown_work, bkup_shutdown_func);
+
+static void bkup_shutdown_func(struct work_struct *work)
+{
+   pr_warn("Orderly_poweroff has failed! Attempting kernel_power_off\n");
+   kernel_power_off();
+
+   pr_warn("kernel_power_off has failed! Attempting emergency_restart\n");
+   emergency_restart();


I think documentation is necessary that we are hoping for bootloader
to be able to detect and halt as needed -> risk here obviously is an
infinite reboot loop :( .


Agreed.





+}
+
  static void handle_critical_trips(struct thermal_zone_device *tz,
int trip, enum thermal_trip_type trip_type)
  {
@@ -443,6 +461,14 @@ static void handle_critical_trips(struct 
thermal_zone_device *tz,
dev_emerg(>device,
  "critical temperature reached(%d C),shutting down\n",
  tz->temperature / 1000);
+   /**


needs to be kernel doc style?


+* This is a backup option to shutdown the
+* system in case orderly_poweroff
+* fails

Maybe adjust to 80char?


Okay.




+*/
+   schedule_delayed_work(_shutdown_work,
+ msecs_to_jiffies(BKUP_SHUTDOWN_DELAY));
+
orderly_poweroff(true);
}
  }






I will wait for Eduardo/Rui's inputs before posting the next version.

Best Regards,
Keerthy
--
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: OMAP display kconfig options changing

2015-12-29 Thread Javier Martinez Canillas 
Hello Tomi,

On Tue, Dec 29, 2015 at 7:23 AM, Tomi Valkeinen  wrote:
> Hi Javier, Tony,
>
> On 22/12/15 23:18, Javier Martinez Canillas wrote:
>
>>> But if the userspace is using any omapfb specific apps, then yes, update
>>> is necessary.
>>>
>>
>> Yes but these people should have a migration plan anyways since omapfb
>> (and fbdev in general) is going away right?
>
> Well, I would guess most people don't know/care about that.
>

Fair enough.

>>> Ok. I think it's then best that I just update the defconfig to enable
>>> omapfb as modules, as it is currently.
>>>
>>
>> In that case I think you should squash the defconfig changes with
>> commit ("70ba4e05531f omapfb/displays: change CONFIG_DISPLAY_* to
>> CONFIG_FB_OMAP2_*") to maintain bisectability.
>
> Here's the diff to change the defconfig to enable the same items as before:
>
> diff --git a/arch/arm/configs/omap2plus_defconfig
> b/arch/arm/configs/omap2plus_defconfig
> index c5e1943e5427..b9581f1f0536 100644
> --- a/arch/arm/configs/omap2plus_defconfig
> +++ b/arch/arm/configs/omap2plus_defconfig
> @@ -290,24 +290,23 @@ CONFIG_FB=y
>  CONFIG_FIRMWARE_EDID=y
>  CONFIG_FB_MODE_HELPERS=y
>  CONFIG_FB_TILEBLITTING=y
> -CONFIG_OMAP2_DSS=m
> -CONFIG_OMAP5_DSS_HDMI=y
> -CONFIG_OMAP2_DSS_SDI=y
> -CONFIG_OMAP2_DSS_DSI=y
> +CONFIG_FB_OMAP5_DSS_HDMI=y
> +CONFIG_FB_OMAP2_DSS_SDI=y
> +CONFIG_FB_OMAP2_DSS_DSI=y
>  CONFIG_FB_OMAP2=m
> -CONFIG_DISPLAY_ENCODER_TFP410=m
> -CONFIG_DISPLAY_ENCODER_TPD12S015=m
> -CONFIG_DISPLAY_CONNECTOR_DVI=m
> -CONFIG_DISPLAY_CONNECTOR_HDMI=m
> -CONFIG_DISPLAY_CONNECTOR_ANALOG_TV=m
> -CONFIG_DISPLAY_PANEL_DPI=m
> -CONFIG_DISPLAY_PANEL_DSI_CM=m
> -CONFIG_DISPLAY_PANEL_SONY_ACX565AKM=m
> -CONFIG_DISPLAY_PANEL_LGPHILIPS_LB035Q02=m
> -CONFIG_DISPLAY_PANEL_SHARP_LS037V7DW01=m
> -CONFIG_DISPLAY_PANEL_TPO_TD028TTEC1=m
> -CONFIG_DISPLAY_PANEL_TPO_TD043MTEA1=m
> -CONFIG_DISPLAY_PANEL_NEC_NL8048HL11=m
> +CONFIG_FB_OMAP2_ENCODER_TFP410=m
> +CONFIG_FB_OMAP2_ENCODER_TPD12S015=m
> +CONFIG_FB_OMAP2_CONNECTOR_DVI=m
> +CONFIG_FB_OMAP2_CONNECTOR_HDMI=m
> +CONFIG_FB_OMAP2_CONNECTOR_ANALOG_TV=m
> +CONFIG_FB_OMAP2_PANEL_DPI=m
> +CONFIG_FB_OMAP2_PANEL_DSI_CM=m
> +CONFIG_FB_OMAP2_PANEL_SONY_ACX565AKM=m
> +CONFIG_FB_OMAP2_PANEL_LGPHILIPS_LB035Q02=m
> +CONFIG_FB_OMAP2_PANEL_SHARP_LS037V7DW01=m
> +CONFIG_FB_OMAP2_PANEL_TPO_TD028TTEC1=m
> +CONFIG_FB_OMAP2_PANEL_TPO_TD043MTEA1=m
> +CONFIG_FB_OMAP2_PANEL_NEC_NL8048HL11=m
>  CONFIG_BACKLIGHT_LCD_SUPPORT=y
>  CONFIG_LCD_CLASS_DEVICE=y
>  CONFIG_LCD_PLATFORM=y
>

Cool, that's what I changed to test omapfb as well. I can't test your
diff right now but looks correct to me.

Best regards,
Javier
--
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: OMAP display kconfig options changing

2015-12-29 Thread Tomi Valkeinen 
Hi Javier, Tony,

On 22/12/15 23:18, Javier Martinez Canillas wrote:

>> But if the userspace is using any omapfb specific apps, then yes, update
>> is necessary.
>>
> 
> Yes but these people should have a migration plan anyways since omapfb
> (and fbdev in general) is going away right?

Well, I would guess most people don't know/care about that.

>> Ok. I think it's then best that I just update the defconfig to enable
>> omapfb as modules, as it is currently.
>>
> 
> In that case I think you should squash the defconfig changes with
> commit ("70ba4e05531f omapfb/displays: change CONFIG_DISPLAY_* to
> CONFIG_FB_OMAP2_*") to maintain bisectability.

Here's the diff to change the defconfig to enable the same items as before:

diff --git a/arch/arm/configs/omap2plus_defconfig
b/arch/arm/configs/omap2plus_defconfig
index c5e1943e5427..b9581f1f0536 100644
--- a/arch/arm/configs/omap2plus_defconfig
+++ b/arch/arm/configs/omap2plus_defconfig
@@ -290,24 +290,23 @@ CONFIG_FB=y
 CONFIG_FIRMWARE_EDID=y
 CONFIG_FB_MODE_HELPERS=y
 CONFIG_FB_TILEBLITTING=y
-CONFIG_OMAP2_DSS=m
-CONFIG_OMAP5_DSS_HDMI=y
-CONFIG_OMAP2_DSS_SDI=y
-CONFIG_OMAP2_DSS_DSI=y
+CONFIG_FB_OMAP5_DSS_HDMI=y
+CONFIG_FB_OMAP2_DSS_SDI=y
+CONFIG_FB_OMAP2_DSS_DSI=y
 CONFIG_FB_OMAP2=m
-CONFIG_DISPLAY_ENCODER_TFP410=m
-CONFIG_DISPLAY_ENCODER_TPD12S015=m
-CONFIG_DISPLAY_CONNECTOR_DVI=m
-CONFIG_DISPLAY_CONNECTOR_HDMI=m
-CONFIG_DISPLAY_CONNECTOR_ANALOG_TV=m
-CONFIG_DISPLAY_PANEL_DPI=m
-CONFIG_DISPLAY_PANEL_DSI_CM=m
-CONFIG_DISPLAY_PANEL_SONY_ACX565AKM=m
-CONFIG_DISPLAY_PANEL_LGPHILIPS_LB035Q02=m
-CONFIG_DISPLAY_PANEL_SHARP_LS037V7DW01=m
-CONFIG_DISPLAY_PANEL_TPO_TD028TTEC1=m
-CONFIG_DISPLAY_PANEL_TPO_TD043MTEA1=m
-CONFIG_DISPLAY_PANEL_NEC_NL8048HL11=m
+CONFIG_FB_OMAP2_ENCODER_TFP410=m
+CONFIG_FB_OMAP2_ENCODER_TPD12S015=m
+CONFIG_FB_OMAP2_CONNECTOR_DVI=m
+CONFIG_FB_OMAP2_CONNECTOR_HDMI=m
+CONFIG_FB_OMAP2_CONNECTOR_ANALOG_TV=m
+CONFIG_FB_OMAP2_PANEL_DPI=m
+CONFIG_FB_OMAP2_PANEL_DSI_CM=m
+CONFIG_FB_OMAP2_PANEL_SONY_ACX565AKM=m
+CONFIG_FB_OMAP2_PANEL_LGPHILIPS_LB035Q02=m
+CONFIG_FB_OMAP2_PANEL_SHARP_LS037V7DW01=m
+CONFIG_FB_OMAP2_PANEL_TPO_TD028TTEC1=m
+CONFIG_FB_OMAP2_PANEL_TPO_TD043MTEA1=m
+CONFIG_FB_OMAP2_PANEL_NEC_NL8048HL11=m
 CONFIG_BACKLIGHT_LCD_SUPPORT=y
 CONFIG_LCD_CLASS_DEVICE=y
 CONFIG_LCD_PLATFORM=y

Tony, the diff above looks like something that's safe to merge via fbdev
tree. The context contains only display related configs. Is it ok if I
squash the above change to my series to keep bisectability?

> Alternatively, the current panel and encoders Kconfig symbols could
> remain for omapfb since that's the current ones used in
> omap2plus_defconfig where omapfb is the default and have new Kconfig
> symbols for omapdrm (i.e: CONFIG_DRM_OMAP_ENCODER_TFP410).

I want to change the config symbols, as the current ones are too generic
(e.g. CONFIG_DISPLAY_PANEL_DPI doesn't mention OMAP anywhere). I think
this is a good time to change them for omapfb. But this is probably a
good time to change them for omapdrm also, so I think I'll add that change.

 Tomi



signature.asc
Description: OpenPGP digital signature

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

2015-12-29 Thread Paul Kocialkowski 
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,

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: https://www.replicant.us/
Blog: https://blog.replicant.us/
Wiki/tracker/forums: https://redmine.replicant.us/



signature.asc
Description: This is a digitally signed message part