[PATCHv2,07/22] drm/bridge: tc358767: move video stream setup to tc_main_link_stream

Message ID 20190326103146.24795-8-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.
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(-)

Comments

Andrzej Hajda April 15, 2019, 7:48 a.m. | #1
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
Laurent Pinchart April 20, 2019, 9:25 p.m. | #2
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;
Tomi Valkeinen May 3, 2019, 9:12 a.m. | #3
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

Patch

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;