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@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")
>
> > 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 :-)
>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
>
> 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
