| Message ID | 20190326103146.24795-8-tomi.valkeinen@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
On 26.03.2019 11:31, Tomi Valkeinen wrote: > The driver currently sets the video stream registers in > tc_main_link_setup. One should be able to establish the DP link without > any video stream, so a more logical place is to configure the stream in > the tc_main_link_stream. So move them there. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/tc358767.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index f5c232a9064e..86b272422281 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1003,15 +1003,6 @@ static int tc_main_link_setup(struct tc_data *tc) > return -EAGAIN; > } > > - ret = tc_set_video_mode(tc, tc->mode); > - if (ret) > - goto err; > - > - /* Set M/N */ > - ret = tc_stream_clock_calc(tc); > - if (ret) > - goto err; > - > return 0; > err_dpcd_read: > dev_err(tc->dev, "Failed to read DPCD: %d\n", ret); > @@ -1030,6 +1021,15 @@ static int tc_main_link_stream(struct tc_data *tc, int state) > dev_dbg(tc->dev, "stream: %d\n", state); > > if (state) { Maybe would be better to simplify the conditional: if (!state) { tc_write(..., 0); return 0; } ... To avoid indentation of increasing chunk of code. Anyway: Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej
Hi Tomi, Thank you for the patch. On Tue, Mar 26, 2019 at 12:31:31PM +0200, Tomi Valkeinen wrote: > The driver currently sets the video stream registers in > tc_main_link_setup. One should be able to establish the DP link without > any video stream, so a more logical place is to configure the stream in > the tc_main_link_stream. So move them there. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/tc358767.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index f5c232a9064e..86b272422281 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1003,15 +1003,6 @@ static int tc_main_link_setup(struct tc_data *tc) > return -EAGAIN; > } > > - ret = tc_set_video_mode(tc, tc->mode); > - if (ret) > - goto err; > - > - /* Set M/N */ > - ret = tc_stream_clock_calc(tc); > - if (ret) > - goto err; > - > return 0; > err_dpcd_read: > dev_err(tc->dev, "Failed to read DPCD: %d\n", ret); > @@ -1030,6 +1021,15 @@ static int tc_main_link_stream(struct tc_data *tc, int state) > dev_dbg(tc->dev, "stream: %d\n", state); > > if (state) { > + ret = tc_set_video_mode(tc, tc->mode); > + if (ret) > + goto err; > + > + /* Set M/N */ > + ret = tc_stream_clock_calc(tc); > + if (ret) > + goto err; > + Assuming this change will have a purpose further down in the series, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> On a side note you may want to remove the err label in tc_stream_clock_calc(), or even inline the write to DP0_VIDMNGEN1 here directly. > value = VID_MN_GEN | DP_EN; > if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) > value |= EF_EN;
On 21/04/2019 00:25, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tue, Mar 26, 2019 at 12:31:31PM +0200, Tomi Valkeinen wrote: >> The driver currently sets the video stream registers in >> tc_main_link_setup. One should be able to establish the DP link without >> any video stream, so a more logical place is to configure the stream in >> the tc_main_link_stream. So move them there. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/gpu/drm/bridge/tc358767.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c >> index f5c232a9064e..86b272422281 100644 >> --- a/drivers/gpu/drm/bridge/tc358767.c >> +++ b/drivers/gpu/drm/bridge/tc358767.c >> @@ -1003,15 +1003,6 @@ static int tc_main_link_setup(struct tc_data *tc) >> return -EAGAIN; >> } >> >> - ret = tc_set_video_mode(tc, tc->mode); >> - if (ret) >> - goto err; >> - >> - /* Set M/N */ >> - ret = tc_stream_clock_calc(tc); >> - if (ret) >> - goto err; >> - >> return 0; >> err_dpcd_read: >> dev_err(tc->dev, "Failed to read DPCD: %d\n", ret); >> @@ -1030,6 +1021,15 @@ static int tc_main_link_stream(struct tc_data *tc, int state) >> dev_dbg(tc->dev, "stream: %d\n", state); >> >> if (state) { >> + ret = tc_set_video_mode(tc, tc->mode); >> + if (ret) >> + goto err; >> + >> + /* Set M/N */ >> + ret = tc_stream_clock_calc(tc); >> + if (ret) >> + goto err; >> + > > Assuming this change will have a purpose further down in the series, The purpose is mainly cleanup. The series doesn't go to the point where the link and the stream could be enabled/disabled independently, but it tries to separate those functionalities for clarity. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > On a side note you may want to remove the err label in > tc_stream_clock_calc(), or even inline the write to DP0_VIDMNGEN1 here > directly. err label is needed by the tc_write macro. Yes, ugly. There's another series that cleans those up. Tomi
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index f5c232a9064e..86b272422281 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1003,15 +1003,6 @@ static int tc_main_link_setup(struct tc_data *tc) return -EAGAIN; } - ret = tc_set_video_mode(tc, tc->mode); - if (ret) - goto err; - - /* Set M/N */ - ret = tc_stream_clock_calc(tc); - if (ret) - goto err; - return 0; err_dpcd_read: dev_err(tc->dev, "Failed to read DPCD: %d\n", ret); @@ -1030,6 +1021,15 @@ static int tc_main_link_stream(struct tc_data *tc, int state) dev_dbg(tc->dev, "stream: %d\n", state); if (state) { + ret = tc_set_video_mode(tc, tc->mode); + if (ret) + goto err; + + /* Set M/N */ + ret = tc_stream_clock_calc(tc); + if (ret) + goto err; + value = VID_MN_GEN | DP_EN; if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) value |= EF_EN;
The driver currently sets the video stream registers in tc_main_link_setup. One should be able to establish the DP link without any video stream, so a more logical place is to configure the stream in the tc_main_link_stream. So move them there. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/bridge/tc358767.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)