Re: [PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2

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

Hi Sergei,

On Tuesday, 16 January 2018 22:17:31 EET Sergei Shtylyov wrote:
> On 01/16/2018 06:46 PM, Laurent Pinchart wrote:
> >>> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>> 
> >>> According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS
> >>> must be enabled and the bias crcuit enabled after the LVDS I/O pins are
> 
> Oops, this needs fixing (note the typo!). Could you please change this
> passage to:
> 
> and the bias circuit must be enabled after the LVDS I/O pins are
> 
> ?

Sure I'll fix that.

> >>> enabled, not before. Fix the gen2 LVDS startup sequence accordingly.
> >>> 
> >>> While at it, also fix the comment preceding the first LVDCR0 write that
> >>> still talks about hardcoding the LVDS mode 0.
> >> 
> >> Please do this in a separate commit then...
> > 
> > The reason I added it here is that I think we don't need patch 1/2 from
> > this series, and I found a bit overkill to split a comment fix to a
> > separate patch when we have a patch that touches the code around the
> > comment.
> 
> OK, you're the maintainer of this driver, you know better. :-)
> 
> >>> Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support")
> >> 
> >> You forgot to specify the other commit this one fixes -- I mean the
> >> comment fix.
> > 
> > Do we need to for a comment update ? It doesn't affect fix the behaviour
> > of the driver or device, and I'd thus prefer to avoid giving the wrong
> > impression that this patch fixes an bug introduced in a previous commit,
> > otherwise it might end up being backported unnecessarily.
> 
>     OK, no dire need to backport indeed...
> 
> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>> Reviewed-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> [Set the mode and input at the same time as the BEN and LVEN bits]
> >>> Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>    drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++-------
> >>>    1 file changed, 7 insertions(+), 7 deletions(-)
> >>> 
> >>> Hi Sergei,
> >>> 
> >>> For your convenience (and if you agree with bundling mode setup with the
> >>> first write as explained in my review of patch 1/2), here's the updated
> >>> version of patch 2/2 that I have taken in my development branch. If
> >>> you're fine with it I'll keep it, otherwise we can continue the review
> >>> discussion.
> >> 
> >> As I said, I don't know how to interpret the note 3 in either manual.
> 
> Moreover, it seems to me that the notes don't match the start-up procedure
> anymore...

How so ?

> > As explained in my latest reply to patch 1/2, my understanding is that the
> > parameters can be programmed at any time before step 6. The fact that the
> > current code works seems to confirm that interpretation. We could ask
> > Renesas for a confirmation if you want.
> 
> Would be good to ask, indeed.

I'll ask.

-- 
Regards,

Laurent Pinchart