[PATCHv2,16/22] drm/bridge: tc358767: remove check for video mode in link enable

Message ID 20190326103146.24795-17-tomi.valkeinen@ti.com
State Superseded
Headers show
Series
  • drm/bridge: tc358767: DP support
Related show

Commit Message

Tomi Valkeinen March 26, 2019, 10:31 a.m.
tc_main_link_enable() checks if videomode has been set, and fails if
there's no videomode. As tc_main_link_enable() no longer depends on the
videomode, we can drop the check.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Andrzej Hajda April 15, 2019, 9:55 a.m. | #1
On 26.03.2019 11:31, Tomi Valkeinen wrote:
> tc_main_link_enable() checks if videomode has been set, and fails if
> there's no videomode. As tc_main_link_enable() no longer depends on the
> videomode, we can drop the check.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
Laurent Pinchart April 20, 2019, 10:14 p.m. | #2
Hi Tomi,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:31:40PM +0200, Tomi Valkeinen wrote:
> tc_main_link_enable() checks if videomode has been set, and fails if
> there's no videomode. As tc_main_link_enable() no longer depends on the
> videomode, we can drop the check.

Shouldn't you move the check to the stream enable function ? Or if it's
not needed there, explain why in the commit message ?

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 1c61f6205e43..ece330c05b9f 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -776,10 +776,6 @@ static int tc_main_link_enable(struct tc_data *tc)
>  	u8 tmp[8];
>  	u32 error;
>  
> -	/* display mode should be set at this point */
> -	if (!tc->mode)
> -		return -EINVAL;
> -
>  	dev_dbg(tc->dev, "link enable\n");
>  
>  	tc_write(DP0CTL, 0);
Tomi Valkeinen May 3, 2019, 8:10 a.m. | #3
On 21/04/2019 01:14, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Mar 26, 2019 at 12:31:40PM +0200, Tomi Valkeinen wrote:
>> tc_main_link_enable() checks if videomode has been set, and fails if
>> there's no videomode. As tc_main_link_enable() no longer depends on the
>> videomode, we can drop the check.
> 
> Shouldn't you move the check to the stream enable function ? Or if it's
> not needed there, explain why in the commit message ?

True. I believe it is not needed. I don't think bridge_enable should be
called at all, if there has not been a mode set before it. If there's no
mode, bridge enable presumably would fail for any bridge...

 Tomi
Laurent Pinchart May 3, 2019, 1 p.m. | #4
Hi Tomi,

On Fri, May 03, 2019 at 11:10:54AM +0300, Tomi Valkeinen wrote:
> On 21/04/2019 01:14, Laurent Pinchart wrote:
> > On Tue, Mar 26, 2019 at 12:31:40PM +0200, Tomi Valkeinen wrote:
> >> tc_main_link_enable() checks if videomode has been set, and fails if
> >> there's no videomode. As tc_main_link_enable() no longer depends on the
> >> videomode, we can drop the check.
> > 
> > Shouldn't you move the check to the stream enable function ? Or if it's
> > not needed there, explain why in the commit message ?
> 
> True. I believe it is not needed. I don't think bridge_enable should be
> called at all, if there has not been a mode set before it. If there's no
> mode, bridge enable presumably would fail for any bridge...

That's my understanding too. A quick check in the core could be useful,
and mentioning this in the commit message too. Ideally the documentation
of the bridge operations should be updated to make this clear :-)

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 1c61f6205e43..ece330c05b9f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -776,10 +776,6 @@  static int tc_main_link_enable(struct tc_data *tc)
 	u8 tmp[8];
 	u32 error;
 
-	/* display mode should be set at this point */
-	if (!tc->mode)
-		return -EINVAL;
-
 	dev_dbg(tc->dev, "link enable\n");
 
 	tc_write(DP0CTL, 0);