[PATCH] drm: rcar_lvds: Fix color mismatches on R-Car H2 ES2.0 and later

[Laurent Pinchart]

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
> 
> > Fix this by applying the quirk when running on R-Car H2 ES1.x only.
> > 
> > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh at renesas.com>
> > 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")
> 
> > Signed-off-by: Geert Uytterhoeven <geert+renesas at glider.be>
> > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh at renesas.com>
> > ---
> > 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 :-)
> 
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 28 +++++++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 3fc7e6899cab5843..50c11a7f0467f746 100644
> > --- 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 ?

Actually, it could be argued that having both on the same line is more
readable. I'll let you decide what you like best.

> With these small issues fixes,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Please let me know if I should fix while applying or if you want to send
> a new version.
> 
> > +		.data = &rcar_lvds_r8a7790es1_info,
> > +	},
> > +	{ /* sentinel */ }
> > +};
> > +
> >  static int rcar_lvds_probe(struct platform_device *pdev)
> >  {
> > +	const struct soc_device_attribute *attr;
> >  	struct rcar_lvds *lvds;
> >  	struct resource *mem;
> >  	int ret;
> > @@ -857,6 +873,10 @@ static int rcar_lvds_probe(struct platform_device *pdev)
> >  	lvds->dev = &pdev->dev;
> >  	lvds->info = of_device_get_match_data(&pdev->dev);
> >  
> > +	attr = soc_device_match(lvds_quirk_matches);
> > +	if (attr)
> > +		lvds->info = attr->data;
> > +
> >  	ret = rcar_lvds_parse_dt(lvds);
> >  	if (ret < 0)
> >  		return ret;
> > @@ -893,12 +913,6 @@ static const struct rcar_lvds_device_info rcar_lvds_gen2_info = {
> >  	.pll_setup = rcar_lvds_pll_setup_gen2,
> >  };
> >  
> > -static const struct rcar_lvds_device_info rcar_lvds_r8a7790_info = {
> > -	.gen = 2,
> > -	.quirks = RCAR_LVDS_QUIRK_LANES,
> > -	.pll_setup = rcar_lvds_pll_setup_gen2,
> > -};
> > -
> >  static const struct rcar_lvds_device_info rcar_lvds_gen3_info = {
> >  	.gen = 3,
> >  	.quirks = RCAR_LVDS_QUIRK_PWD,
> > @@ -930,7 +944,7 @@ static const struct of_device_id rcar_lvds_of_table[] = {
> >  	{ .compatible = "renesas,r8a7744-lvds", .data = &rcar_lvds_gen2_info },
> >  	{ .compatible = "renesas,r8a774a1-lvds", .data = &rcar_lvds_gen3_info },
> >  	{ .compatible = "renesas,r8a774c0-lvds", .data = &rcar_lvds_r8a77990_info },
> > -	{ .compatible = "renesas,r8a7790-lvds", .data = &rcar_lvds_r8a7790_info },
> > +	{ .compatible = "renesas,r8a7790-lvds", .data = &rcar_lvds_gen2_info },
> >  	{ .compatible = "renesas,r8a7791-lvds", .data = &rcar_lvds_gen2_info },
> >  	{ .compatible = "renesas,r8a7793-lvds", .data = &rcar_lvds_gen2_info },
> >  	{ .compatible = "renesas,r8a7795-lvds", .data = &rcar_lvds_gen3_info },
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,

Laurent Pinchart