Re: [PATCHv2 02/22] drm/bridge: tc358767: reset voltage-swing & pre-emphasis

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

Hi Tomi,

On Fri, Apr 26, 2019 at 05:14:17PM +0300, Tomi Valkeinen wrote:
> On 20/04/2019 23:30, Laurent Pinchart wrote:
> > On Tue, Mar 26, 2019 at 12:31:26PM +0200, Tomi Valkeinen wrote:
> >> We need to reset DPCD voltage-swing & pre-emphasis before starting the
> >> link training, as otherwise tc358767 will use the previous values as
> >> minimums.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/bridge/tc358767.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index 7031c4f52c57..11a50f7bb4be 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -956,6 +956,12 @@ static int tc_main_link_setup(struct tc_data *tc)
> >>  	if (ret < 0)
> >>  		goto err_dpcd_write;
> >>  
> >> +	// Reset voltage-swing & pre-emphasis
> > 
> > The driver uses C-style comments, I think it would be best to stick to
> > them to avoid a style mismatch.
> 
> Oops. Yep. I often use c++ comments when hacking/developing as they're
> often easier to use. Sometimes I miss converting them to c comments...
> 
> >> +	tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0;
> > 
> > You may want to wrap the line.
> 
> Well, I personally don't think wrapping at 80 is a good idea. Something
> like 120 is more sensible and makes the code more readable.
> 
> I can wrap it if you insist =)

I don't mind going over 80 when it makes the code more readable, but
when it's easy to wrap, 80 is a nice value, and matches most of the
kernel code :-)

> >> +	ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2);
> > 
> > What branch does this series apply to ? DP_TRAINING_LANE0_SET isn't
> > defined in Linus' or Dave's master branches.
> 
> It's there. At least v5.0 has it.

My bad, I thought it was a driver-specific macro.

> >> +	if (ret < 0)
> >> +		goto err_dpcd_write;
> >> +
> > 
> > I can't comment on this as I don't have access to the device
> > documentation :-(
> 
> Hmm, comment on what?

On the overall change. But now that I've realized this isn't specific to
the tc358767, your change seems fine to me.

> >>  	ret = tc_link_training(tc, DP_TRAINING_PATTERN_1);
> >>  	if (ret)
> >>  		goto err;

-- 
Regards,

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