Re: [PATCH 0/3] drm/omap: fix am4 evm lcd

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Adam,

On Mon, Dec 02, 2019 at 07:24:09AM -0600, Adam Ford wrote:
> On Mon, Dec 2, 2019 at 7:13 AM Laurent Pinchart wrote:
> > On Thu, Nov 14, 2019 at 11:39:47AM +0200, Tomi Valkeinen wrote:
> > > Hi Tony, Thierry, Laurent,
> > >
> > > After the recent change of moving from omapdrm specific panel-dpi driver
> > > to the DRM simple panel, AM4 EVM/ePOS's panel is not working quite
> > > right. This series has fixes for it, but I'm not sure if these are the
> > > right ways to fix the issues, so comments welcome.
> > >
> > > 1) Panel driver is not probed. With omapdrm's panel-dpi, the match
> > > happened with "panel-dpi" compatible string. Now with panel-simple, the
> > > match should happen with the panel model compatible string, which is
> > > "osddisplays,osd057T0559-34ts" in the DT file. However, no such
> > > compatible exists in panel-simple.
> >
> > I've also noticed that we have a few other omap-based platforms that
> > got broken, for the same reason :-( We're missing driver support for
> > innolux,at070tn83, samsung,lte430wq-f0c and startek,startek-kd050c, and
> > we have a few nodes that use panel-dpi without any more precise
> > compatible string.
> >
> > > Interestingly, the actual panel at least on my EVMs and ePOSes is not
> > > osd057T0559-34ts, but osd070t1718-19ts. Also, I was unable to find any
> > > information about osd057T0559-34ts. I don't know the history with this,
> > > so it is possible that the early versions of the boards did have
> > > osd057T0559-34ts, but was later changed to osd070t1718-19ts.
> > >
> > > As osd070t1718-19ts is supported by panel-simple, changing the
> > > compatible string to osd070t1718-19ts in the DT file solves this one.
> > >
> > > 2) Timings in DT file cause a kernel warning. Omapdrm's panel-dpi used
> > > video timings from the DT file, so they are present in all the DT files.
> > > panel-simple uses timings from a table in the panel-simple driver, but
> > > gives a kernel warning if the DT file contains timings.
> > >
> > > This can be solved by removing the timings from the DT file.
> > >
> > > 3) Sync drive edge is not right. This one might have been present also
> > > with panel-dpi, I didn't verify. The problem is that the panel-simple
> > > data for osddisplays_osd070t1718_19ts defines bus_flags for DE polarity
> > > and pixdata edge, but not for sync edge.
> > >
> > > The datasheet for the panel does not give any hint on what the edge
> > > should be.  Omapdrm defaults to driving syncs on falling edge, which
> > > caused the image to be shifted one pixel to the right.
> > >
> > > Adding DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE bus_flag solves the problem. AM5
> > > EVM also has the same panel with the same behavior.
> > >
> > > ====
> > >
> > > The reason I'm not sure if the 1) and 2) fixes are correct is that
> > > they're breaking DT compatibility. Should we instead make changes to
> > > panel-simple to keep the same DT files working?
> >
> > That's tempting, as breaking DT is quite bad, but that would require
> > adding a match on panel-dpi, and parsing timings in the panel-simple
> > driver. Thierry has always been opposed to that as far as I can tell,
> > and even if I don't share his point of view, I don't want to move in
> > this direction without a consensus.
> 
> What about a generic driver separate from the simple panel driver that
> does just the panel-dpi parsing?

A separate driver wouldn't make too much of a difference. Thierry's
point was that drivers should hardcode the timings, as the compatible
string gives enough information and encoding timings in DT would
duplicate that information. I have a different point of view, and there
are pros and cons for both options, so we haven't been able to reach an
agreement. That's why I haven't pushed too hard for timings parsing from
DT, regardless of whether it would be in panel-simple or in a separate
driver.

> If we exported the necessary functions from simple-panel, we could
> call them from the panel-dpi parser and not have to re-invent the
> functions to enable power, gpio or back light and/or fix them each
> time they get updated.
> 
> > Your series is fine in my opinion, as even if we decide to handle
> > backward compatibility in this case, changing the DT files in mainline
> > is still the right way to go (if only to avoid giving wrong examples).
> >
> > > This would mean adding a new entry for the osd057T0559-34ts panel, but
> > > as we don't have datasheet for it, I think we could just append the
> > > compatible string to osd070t1718-19t's data.
> > >
> > > It would also mean doing some change to the panel-simple code that gives
> > > the warning about timings in DT data. This might make sense, as I think
> > > we have other DT files with video timings too.
> > >
> > > For 3), I think the patch is fine, but I'm not sure if the display
> > > controller driver should be able to deduce the sync drive edge from the
> > > pixdata drive edge. Are they usually the same? I have no idea...
> > >
> > > Tomi Valkeinen (3):
> > >   ARM: dts: am437x-gp/epos-evm: fix panel compatible
> > >   ARM: dts: am437x-gp/epos-evm: drop unused panel timings
> > >   drm/panel: simple: fix osd070t1718_19ts sync drive edge
> > >
> > >  arch/arm/boot/dts/am437x-gp-evm.dts  | 18 +-----------------
> > >  arch/arm/boot/dts/am43x-epos-evm.dts | 18 +-----------------
> > >  drivers/gpu/drm/panel/panel-simple.c |  3 ++-
> > >  3 files changed, 4 insertions(+), 35 deletions(-)

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel