| Message ID | 20190326103146.24795-9-tomi.valkeinen@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/bridge: tc358767: DP support | expand |
On 26.03.2019 11:31, Tomi Valkeinen wrote: > It is nicer to have enable/disable functions instead of set(bool enable) > style function. > > Split tc_main_link_stream into tc_stream_enable and tc_stream_disable. So my comment regarding previous patch was already addresed :) > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> 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:32PM +0200, Tomi Valkeinen wrote: > It is nicer to have enable/disable functions instead of set(bool enable) > style function. When the two functions have nothing in common, yes. > Split tc_main_link_stream into tc_stream_enable and tc_stream_disable. Should you keep the tc_main_link_ prefix ? I suppose it is implied in a way, as the stream is carried over the main link. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/bridge/tc358767.c | 81 +++++++++++++++++-------------- > 1 file changed, 45 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 86b272422281..bfc673bd5986 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1013,47 +1013,56 @@ static int tc_main_link_setup(struct tc_data *tc) > return ret; > } > > -static int tc_main_link_stream(struct tc_data *tc, int state) > +static int tc_stream_enable(struct tc_data *tc) > { > int ret; > u32 value; > > - dev_dbg(tc->dev, "stream: %d\n", state); > + dev_dbg(tc->dev, "stream enable\n"); Maybe "enable video stream\n" (and similarly for the tc_stream_disable() function) ? > > - if (state) { > - ret = tc_set_video_mode(tc, tc->mode); > - if (ret) > - goto err; > + ret = tc_set_video_mode(tc, tc->mode); > + if (ret) > + goto err; Let's return ret directly and remove the err label. With these issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > - /* Set M/N */ > - ret = tc_stream_clock_calc(tc); > - 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; > - tc_write(DP0CTL, value); > - /* > - * VID_EN assertion should be delayed by at least N * LSCLK > - * cycles from the time VID_MN_GEN is enabled in order to > - * generate stable values for VID_M. LSCLK is 270 MHz or > - * 162 MHz, VID_N is set to 32768 in tc_stream_clock_calc(), > - * so a delay of at least 203 us should suffice. > - */ > - usleep_range(500, 1000); > - value |= VID_EN; > - tc_write(DP0CTL, value); > - /* Set input interface */ > - value = DP0_AUDSRC_NO_INPUT; > - if (tc_test_pattern) > - value |= DP0_VIDSRC_COLOR_BAR; > - else > - value |= DP0_VIDSRC_DPI_RX; > - tc_write(SYSCTRL, value); > - } else { > - tc_write(DP0CTL, 0); > - } > + value = VID_MN_GEN | DP_EN; > + if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) > + value |= EF_EN; > + tc_write(DP0CTL, value); > + /* > + * VID_EN assertion should be delayed by at least N * LSCLK > + * cycles from the time VID_MN_GEN is enabled in order to > + * generate stable values for VID_M. LSCLK is 270 MHz or > + * 162 MHz, VID_N is set to 32768 in tc_stream_clock_calc(), > + * so a delay of at least 203 us should suffice. > + */ > + usleep_range(500, 1000); > + value |= VID_EN; > + tc_write(DP0CTL, value); > + /* Set input interface */ > + value = DP0_AUDSRC_NO_INPUT; > + if (tc_test_pattern) > + value |= DP0_VIDSRC_COLOR_BAR; > + else > + value |= DP0_VIDSRC_DPI_RX; > + tc_write(SYSCTRL, value); > + > + return 0; > +err: > + return ret; > +} > + > +static int tc_stream_disable(struct tc_data *tc) > +{ > + int ret; > + > + dev_dbg(tc->dev, "stream disable\n"); > + > + tc_write(DP0CTL, 0); > > return 0; > err: > @@ -1078,7 +1087,7 @@ static void tc_bridge_enable(struct drm_bridge *bridge) > return; > } > > - ret = tc_main_link_stream(tc, 1); > + ret = tc_stream_enable(tc); > if (ret < 0) { > dev_err(tc->dev, "main link stream start error: %d\n", ret); > return; > @@ -1094,7 +1103,7 @@ static void tc_bridge_disable(struct drm_bridge *bridge) > > drm_panel_disable(tc->panel); > > - ret = tc_main_link_stream(tc, 0); > + ret = tc_stream_disable(tc); > if (ret < 0) > dev_err(tc->dev, "main link stream stop error: %d\n", ret); > }
On 21/04/2019 00:29, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tue, Mar 26, 2019 at 12:31:32PM +0200, Tomi Valkeinen wrote: >> It is nicer to have enable/disable functions instead of set(bool enable) >> style function. > > When the two functions have nothing in common, yes. > >> Split tc_main_link_stream into tc_stream_enable and tc_stream_disable. > > Should you keep the tc_main_link_ prefix ? I suppose it is implied in a > way, as the stream is carried over the main link. It sounds a bit redundant, only making the functions names longer. >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/gpu/drm/bridge/tc358767.c | 81 +++++++++++++++++-------------- >> 1 file changed, 45 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c >> index 86b272422281..bfc673bd5986 100644 >> --- a/drivers/gpu/drm/bridge/tc358767.c >> +++ b/drivers/gpu/drm/bridge/tc358767.c >> @@ -1013,47 +1013,56 @@ static int tc_main_link_setup(struct tc_data *tc) >> return ret; >> } >> >> -static int tc_main_link_stream(struct tc_data *tc, int state) >> +static int tc_stream_enable(struct tc_data *tc) >> { >> int ret; >> u32 value; >> >> - dev_dbg(tc->dev, "stream: %d\n", state); >> + dev_dbg(tc->dev, "stream enable\n"); > > Maybe "enable video stream\n" (and similarly for the tc_stream_disable() > function) ? Ok. >> >> - if (state) { >> - ret = tc_set_video_mode(tc, tc->mode); >> - if (ret) >> - goto err; >> + ret = tc_set_video_mode(tc, tc->mode); >> + if (ret) >> + goto err; > > Let's return ret directly and remove the err label. Can't remove the err label, because of the tc_write() calls... Tomi
Hi Tomi, On Fri, May 03, 2019 at 12:20:49PM +0300, Tomi Valkeinen wrote: > On 21/04/2019 00:29, Laurent Pinchart wrote: > > On Tue, Mar 26, 2019 at 12:31:32PM +0200, Tomi Valkeinen wrote: > >> It is nicer to have enable/disable functions instead of set(bool enable) > >> style function. > > > > When the two functions have nothing in common, yes. > > > >> Split tc_main_link_stream into tc_stream_enable and tc_stream_disable. > > > > Should you keep the tc_main_link_ prefix ? I suppose it is implied in a > > way, as the stream is carried over the main link. > > It sounds a bit redundant, only making the functions names longer. A bit, but it also makes the code a bit clearer in my opinion. Up to you. > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > >> --- > >> drivers/gpu/drm/bridge/tc358767.c | 81 +++++++++++++++++-------------- > >> 1 file changed, 45 insertions(+), 36 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > >> index 86b272422281..bfc673bd5986 100644 > >> --- a/drivers/gpu/drm/bridge/tc358767.c > >> +++ b/drivers/gpu/drm/bridge/tc358767.c > >> @@ -1013,47 +1013,56 @@ static int tc_main_link_setup(struct tc_data *tc) > >> return ret; > >> } > >> > >> -static int tc_main_link_stream(struct tc_data *tc, int state) > >> +static int tc_stream_enable(struct tc_data *tc) > >> { > >> int ret; > >> u32 value; > >> > >> - dev_dbg(tc->dev, "stream: %d\n", state); > >> + dev_dbg(tc->dev, "stream enable\n"); > > > > Maybe "enable video stream\n" (and similarly for the tc_stream_disable() > > function) ? > > Ok. > > >> > >> - if (state) { > >> - ret = tc_set_video_mode(tc, tc->mode); > >> - if (ret) > >> - goto err; > >> + ret = tc_set_video_mode(tc, tc->mode); > >> + if (ret) > >> + goto err; > > > > Let's return ret directly and remove the err label. > > Can't remove the err label, because of the tc_write() calls... :-( I'd love to see this getting fixed. The best way I've found so far would be int tc_write(struct tc_data *tc, unsigned int reg, unsigned int value, int &err) { unsigned int ret; if (err && *err) return *err; ret = do_the_write_here(..., reg, value); if (err) *err = ret; return ret; } This can be used as int ret = 0; tc_write(tc, REG0, VAL0, &ret); ... tc_write(tc, REGn, VALn, &ret); if (ret) /* Error handling */
On 03/05/2019 15:55, Laurent Pinchart wrote: >>>> >>>> - if (state) { >>>> - ret = tc_set_video_mode(tc, tc->mode); >>>> - if (ret) >>>> - goto err; >>>> + ret = tc_set_video_mode(tc, tc->mode); >>>> + if (ret) >>>> + goto err; >>> >>> Let's return ret directly and remove the err label. >> >> Can't remove the err label, because of the tc_write() calls... > > :-( > > I'd love to see this getting fixed. The best way I've found so far would > be And by fixed you mean cleaned up? Yes, it's a mess. That's why I want to get this series merged asap, so Andrey can rebase his series, and we can proceed with all the cleanups =). His series removes these macros that require the err label. Tomi
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 86b272422281..bfc673bd5986 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1013,47 +1013,56 @@ static int tc_main_link_setup(struct tc_data *tc) return ret; } -static int tc_main_link_stream(struct tc_data *tc, int state) +static int tc_stream_enable(struct tc_data *tc) { int ret; u32 value; - dev_dbg(tc->dev, "stream: %d\n", state); + dev_dbg(tc->dev, "stream enable\n"); - if (state) { - ret = tc_set_video_mode(tc, tc->mode); - if (ret) - goto err; + 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; + /* 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; - tc_write(DP0CTL, value); - /* - * VID_EN assertion should be delayed by at least N * LSCLK - * cycles from the time VID_MN_GEN is enabled in order to - * generate stable values for VID_M. LSCLK is 270 MHz or - * 162 MHz, VID_N is set to 32768 in tc_stream_clock_calc(), - * so a delay of at least 203 us should suffice. - */ - usleep_range(500, 1000); - value |= VID_EN; - tc_write(DP0CTL, value); - /* Set input interface */ - value = DP0_AUDSRC_NO_INPUT; - if (tc_test_pattern) - value |= DP0_VIDSRC_COLOR_BAR; - else - value |= DP0_VIDSRC_DPI_RX; - tc_write(SYSCTRL, value); - } else { - tc_write(DP0CTL, 0); - } + value = VID_MN_GEN | DP_EN; + if (tc->link.base.capabilities & DP_LINK_CAP_ENHANCED_FRAMING) + value |= EF_EN; + tc_write(DP0CTL, value); + /* + * VID_EN assertion should be delayed by at least N * LSCLK + * cycles from the time VID_MN_GEN is enabled in order to + * generate stable values for VID_M. LSCLK is 270 MHz or + * 162 MHz, VID_N is set to 32768 in tc_stream_clock_calc(), + * so a delay of at least 203 us should suffice. + */ + usleep_range(500, 1000); + value |= VID_EN; + tc_write(DP0CTL, value); + /* Set input interface */ + value = DP0_AUDSRC_NO_INPUT; + if (tc_test_pattern) + value |= DP0_VIDSRC_COLOR_BAR; + else + value |= DP0_VIDSRC_DPI_RX; + tc_write(SYSCTRL, value); + + return 0; +err: + return ret; +} + +static int tc_stream_disable(struct tc_data *tc) +{ + int ret; + + dev_dbg(tc->dev, "stream disable\n"); + + tc_write(DP0CTL, 0); return 0; err: @@ -1078,7 +1087,7 @@ static void tc_bridge_enable(struct drm_bridge *bridge) return; } - ret = tc_main_link_stream(tc, 1); + ret = tc_stream_enable(tc); if (ret < 0) { dev_err(tc->dev, "main link stream start error: %d\n", ret); return; @@ -1094,7 +1103,7 @@ static void tc_bridge_disable(struct drm_bridge *bridge) drm_panel_disable(tc->panel); - ret = tc_main_link_stream(tc, 0); + ret = tc_stream_disable(tc); if (ret < 0) dev_err(tc->dev, "main link stream stop error: %d\n", ret); }
It is nicer to have enable/disable functions instead of set(bool enable) style function. Split tc_main_link_stream into tc_stream_enable and tc_stream_disable. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/bridge/tc358767.c | 81 +++++++++++++++++-------------- 1 file changed, 45 insertions(+), 36 deletions(-)