Re: [PATCH 6/6] ARM: dts: omap4-droid4: Add LCD

[Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4491 bytes --]

Hi,

On Mon, Mar 20, 2017 at 09:52:20AM +0200, Tomi Valkeinen wrote:
> On 20/03/17 00:55, Sebastian Reichel wrote:
> > On Sun, Mar 19, 2017 at 09:10:30AM -0700, Tony Lindgren wrote:
> >> * Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [170318 18:31]:
> >>> On Sat, Mar 04, 2017 at 09:43:59PM -0800, Tony Lindgren wrote:
> >>>> The LCD panel on droid 4 is a command mode LCD. The binding follows
> >>>> the standard omapdrm binding and the changes needed for omapdrm command
> >>>> mode panels are posted separately.
> >>>>
> >>>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>>> Cc: Marcel Partap <mpartap-hi6Y0CQ0nG0@public.gmane.org>
> >>>> Cc: Michael Scott <michael.scott-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>> Cc: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>>> Cc: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
> >>>> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> >>>
> >>> Tested-By: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>>
> >>> With a non-modular kernel lcd is not working with omapdrm if HDMI
> >>> is enabled. After dropping HDMI in droid4's dts file everything
> >>> worked as expected. I assume both work properly with a modular
> >>> kernel?
> >>
> >> Yes with loadable modules both work just fine. If things do not
> >> work properly as built-in, chances are there's some unhandled
> >> dependency that needs -EPROBE_DEFER somewhere for a regulator
> >> or a clock.
> > 
> > I think that would also result in problems with disabled HDMI.
> > I guess the problem is, that omapdrm is initialized too early.
> > AFAIK omapdrm is not hotplug-capable.
> 
> It shouldn't matter when omapdrm is initialized. omapdrm should wait
> until it has all the displays.

I tried to trace this. I assume, that your "should wait until it has
all the displays" refers to the code in "omap_connect_dssdevs()".
Unfortunately that code does not work correctly:

droid4# dmesg | grep-interesting
[    1.222137] omapdss_dss 58000000.dss: 58000000.dss supply vdda_video not found, using dummy regulator
[    1.236663] omapdss_dispc 58001000.dispc: OMAP DISPC rev 4.0
[    1.236877] omapdss_dss 58000000.dss: bound 58001000.dispc (ops dispc_component_ops)
[    1.245208] omapdss_dsi 58004000.encoder: OMAP DSI rev 3.0
[    1.246246] omapdss_dss 58000000.dss: bound 58004000.encoder (ops dsi_component_ops)
[    1.255462] omapdss_dss 58000000.dss: bound 58006000.encoder (ops hdmi4_component_ops)
[    1.264923] panel-dsi-cm 58004000.encoder:display: probe

This probe failed with -EPROBE_DEFER due to regulator not yet
available.

[    3.294586] connector-hdmi connector: connected display: hdmi

I added this to omap_connect_dssdevs(). DSI is not even in the for loop,
otherwise there would have been another print.

[    3.299560] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[    3.299560] [drm] No driver support for vblank timestamp query.
[    3.313507] [drm] Cannot find any crtc or sizes - going 1024x768
[    3.342376] [drm] Enabling DMM ywrap scrolling
[    3.386871] omapdrm omapdrm.0: fb0: omapdrm frame buffer device
[    3.397460] [drm] Initialized omapdrm 1.0.0 20110917 for omapdrm.0 on minor 0
[    3.406219] panel-dsi-cm 58004000.encoder:display: probe

Here the display is probed again (this time successfully, but
omapdrm doesn't care anymore).

Note, that the EPROBE_DEFER handling in omap_connect_dssdevs() is
correct, since the connect function can also return EPROBE_DEFER.
This is because the omapdss SoC modules request their regulators
in their connect handler.

So currently all code in omapdrm/displays returning EPROBE_DEFER
in their probe function are potentially ignored by omapdrm. It
does work however, if that results in no display being initialized
at all.

The simple fix would be to move the regulator probing from
display's probe function to display's connect function (multiple
omapdrm display drivers are affected). IMHO this is ugly and
drives us further away from a common driver scheme, so I suggest
to fix omap_connect_dssdevs() instead. It should be enough to
check, that all devices have been probed either successfully or
with a fatal error at the start of the function. Also the "no
devices found" could become -ENODEV instead of -EPROBE_DEFER.
I think it may have hid this problem for others.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]