Hi Sergei,

Thank you for the patch.

On Friday, 12 January 2018 22:12:04 EET Sergei Shtylyov wrote:
> According to the latest revisions of the R-Car gen3 manual, the LVDS mode
> must be set before the LVDS I/O pins are enabled, not after --  fix  the
> gen3 LVDS startup sequence accordingly...
> 
> While  at it,  also fix the comment  preceding the first LVDCR0 write in
> the R-Car gen2 startup code that still talks about hardcoding the LVDS
> mode 0...

How about fixing that in patch 2/2 that touches the Gen2 initialization 
sequence ? I think I'd even go as far as squashing both patches, I don't think 
there's a need to split them.

> Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode selection")
> Signed-off-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>

Is this really needed ? Does it fix a problem you've experienced, or is it 
theoretical only ? The mode shouldn't matter before the LVDS internal logic is 
turned on. Unless there's a real issue I'm not sure we should make the code 
more complex.

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> Index: linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> ===================================================================
> --- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> +++ linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> @@ -60,8 +60,8 @@ static void rcar_du_lvdsenc_start_gen2(s
>       rcar_lvds_write(lvds, LVDPLLCR, pllcr);
> 
>       /*
> -      * Select the input, hardcode mode 0, enable LVDS operation and turn
> -      * bias circuitry on.
> +      * Set the  LVDS mode, select the input, enable LVDS operation,
> +      * and turn bias circuitry on.
>        */
>       lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_BEN | LVDCR0_LVEN;
>       if (rcrtc->index == 2)
> @@ -106,6 +106,9 @@ static void rcar_du_lvdsenc_start_gen3(s
> 
>       rcar_lvds_write(lvds, LVDPLLCR, pllcr);
> 
> +     lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
> +     rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> +
>       /* Turn all the channels on. */
>       rcar_lvds_write(lvds, LVDCR1,
>                       LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
> @@ -115,7 +118,8 @@ static void rcar_du_lvdsenc_start_gen3(s
>        * Turn the PLL on, set it to LVDS normal mode, wait for the startup
>        * delay and turn the output on.
>        */
> -     lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_PLLON;
> +
> +     lvdcr0 = | LVDCR0_PLLON;
>       rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> 
>       lvdcr0 |= LVDCR0_PWD;

-- 
Regards,

Laurent Pinchart

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

Reply via email to