Hi Laurent,
On Sat, Sep 21, 2019 at 1:43 AM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> On Sat, Sep 21, 2019 at 02:40:03AM +0300, Laurent Pinchart wrote:
> > On Tue, Sep 17, 2019 at 08:23:53AM +0200, Geert Uytterhoeven wrote:
> > > Commit 5cca30ebe089be23 ("drm/rcar-du: Add LVDS_LANES quirk") states
> > > that LVDS lanes 1 and 3 are inverted on R-Car H2 ES1 only, and that the
> > > problem has been fixed in newer revisions.
> > >
> > > However, the code didn't take into account the actual hardware revision,
> > > thus applying the quirk also on newer hardware revisions, causing green
> > > color reversals.
> >
> > Oops :-S
Quite understandable, as there was no soc_device_match() in 2013...
> > > Fix this by applying the quirk when running on R-Car H2 ES1.x only.
> > >
> > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > > Fixes: c6a27fa41fabb35f ("drm: rcar-du: Convert LVDS encoder code to bridge driver")
> >
> > Shouldn't this be
> >
> > Fixes: 5cca30ebe089be23 ("drm/rcar-du: Add LVDS_LANES quirk")
Yes, that's where the issue was introduced. But see my original comment
about backporting below.
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > > ---
> > > Does anyone know if this was fixed in ES2.0, or in any earlier ES1.x?
> >
> > Or if there's any ES1.x other than ES1.0 ? :-)
> >
> > > While the issue was present before aforementioned commit, I do not think
> > > there is a real need to fix the older code variant, as the new LVDS
> > > encoder was backported to v4.14-ltsi.
> >
> > Probably not, but I think there's still value in pointing to the right
> > erroneous commit. It's a Fixes: tag, not a Backport-up-to: tag :-)
OK.
> > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > @@ -16,6 +16,7 @@
> > > #include <linux/of_graph.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/slab.h>
> > > +#include <linux/sys_soc.h>
> > >
> > > #include <drm/drm_atomic.h>
> > > #include <drm/drm_atomic_helper.h>
> > > @@ -842,8 +843,23 @@ static int rcar_lvds_get_clocks(struct rcar_lvds *lvds)
> > > return 0;
> > > }
> > >
> > > +static const struct rcar_lvds_device_info rcar_lvds_r8a7790es1_info = {
> > > + .gen = 2,
> > > + .quirks = RCAR_LVDS_QUIRK_LANES,
> > > + .pll_setup = rcar_lvds_pll_setup_gen2,
> > > +};
> > > +
> > > +static const struct soc_device_attribute lvds_quirk_matches[] = {
> > > + {
> > > + .soc_id = "r8a7790", .revision = "ES1.*",
> >
> > Do you mind splitting this in two lines ?
Yes I do: it makes it easier to locate fixes for early silicon.
> Actually, it could be argued that having both on the same line is more
> readable. I'll let you decide what you like best.
I'm happy to hear you're reconsidering!
> > With these small issues fixes,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >
> > Please let me know if I should fix while applying or if you want to send
> > a new version.
Feel free to fix (replace Fixes or add a second Fixes tag) while applying.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
