| Message ID | 20190326103146.24795-3-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
On 26.03.2019 11:31, 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> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej > --- > 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 > + tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0; > + ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2); > + if (ret < 0) > + goto err_dpcd_write; > + > ret = tc_link_training(tc, DP_TRAINING_PATTERN_1); > if (ret) > goto err;
Hi Tomi, Thank you for the patch. 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. > + tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0; You may want to wrap the line. > + 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. > + if (ret < 0) > + goto err_dpcd_write; > + I can't comment on this as I don't have access to the device documentation :-( > ret = tc_link_training(tc, DP_TRAINING_PATTERN_1); > if (ret) > goto err;
On 20/04/2019 23:30, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > 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 =) >> + 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. >> + 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? >> ret = tc_link_training(tc, DP_TRAINING_PATTERN_1); >> if (ret) >> goto err; > Tomi
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;
On Fri, Apr 26, 2019 at 7:14 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > On 20/04/2019 23:30, Laurent Pinchart wrote: > > Hi Tomi, > > > > Thank you for the patch. > > > > 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. Trying to read two files side by side on a 13" laptop screen might change your mind :-) +1 on wrapping the code around 80 character from me. Thanks, Andrey Smirnov
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 + tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | DP_TRAIN_PRE_EMPH_LEVEL_0; + ret = drm_dp_dpcd_write(aux, DP_TRAINING_LANE0_SET, tmp, 2); + if (ret < 0) + goto err_dpcd_write; + ret = tc_link_training(tc, DP_TRAINING_PATTERN_1); if (ret) goto err;
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(+)