diff mbox

[11/11] OMAPDSS: DPI: use VPLL2 regulator if VDDS_DSI is not found

Message ID 5287AB20.9070407@collabora.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Javier Martinez Canillas Nov. 16, 2013, 5:28 p.m. UTC
Hi Sebastian,

On 11/16/2013 05:02 PM, Sebastian Reichel wrote:
> On Sat, Nov 16, 2013 at 07:45:07AM -0800, Tony Lindgren wrote:
>> Well let's see what Tomi prefers.
>> 
>> > > &vaux1 {
>> > > 	/* Needed for ads7846 */
>> > > 	regulator-name = "vcc";
>> > > };
>> > > 
>> > > &vpll2 {
>> > > 	/* Needed for DSS */
>> > > 	regulator-name = "vdds_dsi";
>> > > };
>> 
>> In the long we'll use regulator phandles anyways in the DSS related
>> nodes, so from that point of view fixing dpi.c makes sense.
> 
> The hack, as being sent by Javier, will result in the wrong
> regulator being used on the Nokia N900, which is using vaux1.
> It must depend on the machine.
>

I see, so this hack is not generic enough to be usable for all OMAP3 boards so I
think is better to just add the hack on the DTS.

I was more found about adding a hack on dpi.c since the DTB is an stable ABI
while the dpi.c implementation is not. But I guess is not that bad to change the
DTS later once DSS DT bindings are added to mainline.

> At the same time the N900 device tree file uses V28 as
> regulator-name for vaux1 (which is the same one as legacy boot
> used). Nothing depends on this, but I don't think it's a
> good idea to use this property. Apparently it does not work
> if multiple drivers need the same regulator under different
> names.
> 
> -- Sebastian
> 

So, what do you think about the following patch instead?

From 99f1c60f6db6cac07ea2c0398a9cbb3525dfdd72 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Date: Sat, 16 Nov 2013 17:59:08 +0100
Subject: [PATCH 1/1] ARM: dts: omap3-igep0020: name twl4030 VPLL2 regulator as
 vdds_dsi

On Device Tree boot the VDDS_DSI regulator is not linked to
the DPI device so omapfb driver probing fails with:

[    3.186035] OMAPFB: omapfb_probe
[    3.190704] omapdss DPI error: can't get VDDS_DSI regulator
[    3.196594] omapfb omapfb: failed to connect default display
[    3.202667] omapfb omapfb: failed to init overlay connections
[    3.208892] OMAPFB: free_resources
[    3.212493] OMAPFB: free all fbmem
[    3.216735] omapfb omapfb: failed to setup omapfb

As a workaround name the VPLL2 regulator from twl4030 as "vdds_dsi"
so getting the VDDS_DSI regulator will succeed on dpi_init_regulator().

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/omap3-igep0020.dts | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Sebastian Reichel Nov. 17, 2013, 12:26 a.m. UTC | #1
Hi Javier,

On Sat, Nov 16, 2013 at 06:28:00PM +0100, Javier Martinez Canillas wrote:
> > The hack, as being sent by Javier, will result in the wrong
> > regulator being used on the Nokia N900, which is using vaux1.
> > It must depend on the machine.
> 
> I see, so this hack is not generic enough to be usable for all OMAP3 boards so I
> think is better to just add the hack on the DTS.
>
> I was more found about adding a hack on dpi.c since the DTB is an stable ABI
> while the dpi.c implementation is not.

I agree with this. My suggestion would be to get the vdds_dsi regulator name
from platform data. This way each board can setup its own quirk until omapdss
supports DT.

> But I guess is not that bad to change the DTS later once DSS DT
> bindings are added to mainline.

As far as I know DT bindings are supposed to be as stable as
possible.

> > At the same time the N900 device tree file uses V28 as
> > regulator-name for vaux1 (which is the same one as legacy boot
> > used). Nothing depends on this, but I don't think it's a
> > good idea to use this property. Apparently it does not work
> > if multiple drivers need the same regulator under different
> > names.
> > 
> > -- Sebastian
> > 
> 
> So, what do you think about the following patch instead?
> 
> [PATCH]

For Nokia N900 vaux1 is also used for si4713 radio chip, lis3lv02d
accelerometer and battery cover sensor, so I don't like this hack.

If vppl2 is only used for omapdss.dsi on IGEP boards it might be
sensible to label it, though? So it might be ok there.

-- Sebastian
Javier Martinez Canillas Nov. 17, 2013, 1:06 a.m. UTC | #2
Hi Sebastian,

On 11/17/2013 01:26 AM, Sebastian Reichel wrote:
> Hi Javier,
> 
> On Sat, Nov 16, 2013 at 06:28:00PM +0100, Javier Martinez Canillas wrote:
>> > The hack, as being sent by Javier, will result in the wrong
>> > regulator being used on the Nokia N900, which is using vaux1.
>> > It must depend on the machine.
>> 
>> I see, so this hack is not generic enough to be usable for all OMAP3 boards so I
>> think is better to just add the hack on the DTS.
>>
>> I was more found about adding a hack on dpi.c since the DTB is an stable ABI
>> while the dpi.c implementation is not.
> 
> I agree with this. My suggestion would be to get the vdds_dsi regulator name
> from platform data. This way each board can setup its own quirk until omapdss
> supports DT.
> 
>> But I guess is not that bad to change the DTS later once DSS DT
>> bindings are added to mainline.
> 
> As far as I know DT bindings are supposed to be as stable as
> possible.
> 

Indeed, although as far as I understood on the latest ARM kernel summit it was
decided that DT bindings (and DTS) can be changed in incompatible ways if it can
be reasonably argued that nobody will be affected by the change.

IMHO this is the case for IGEP boards since the vendor (ISEE) is still shipping
the boards using an old 2.6.37 forked kernel that uses board files.

So I think is reasonable to remove this DT hack later once we have omapdss + CDF
bindings in mainline since current users that choose to use the mainline kernel
instead of the vendor one have to be able to replace the stock kernel +
bootloader that comes with the boards anyways.

>> > At the same time the N900 device tree file uses V28 as
>> > regulator-name for vaux1 (which is the same one as legacy boot
>> > used). Nothing depends on this, but I don't think it's a
>> > good idea to use this property. Apparently it does not work
>> > if multiple drivers need the same regulator under different
>> > names.
>> > 
>> > -- Sebastian
>> > 
>> 
>> So, what do you think about the following patch instead?
>> 
>> [PATCH]
> 
> For Nokia N900 vaux1 is also used for si4713 radio chip, lis3lv02d
> accelerometer and battery cover sensor, so I don't like this hack.
>
> If vppl2 is only used for omapdss.dsi on IGEP boards it might be
> sensible to label it, though? So it might be ok there.
>

Yes, you can't use this hack for the N900 since other devices already use the
vaux1 regulator named as "V28" so you can't just rename it to "vdds_dsi".

But for IGEP boards vpll2 is only used for omapdss DPI so we don't have that issue.

Let's see what Tomi says that is the best way to handle this to have DVI working
with DT until the propers bindings land in mainline.

> -- Sebastian
> 

Thanks a lot for your feedback and best regards,
Javier
diff mbox

Patch

diff --git a/arch/arm/boot/dts/omap3-igep0020.dts
b/arch/arm/boot/dts/omap3-igep0020.dts
index b9a9e17..1c7e74d 100644
--- a/arch/arm/boot/dts/omap3-igep0020.dts
+++ b/arch/arm/boot/dts/omap3-igep0020.dts
@@ -215,3 +215,8 @@ 
 &usbhsehci {
 	phys = <&hsusb1_phy>;
 };
+
+&vpll2 {
+        /* Needed for DSS */
+        regulator-name = "vdds_dsi";
+};