[PATCHv2,08/22] drm/bridge: tc358767: split stream enable/disable

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

Comments

Andrzej Hajda April 15, 2019, 8:26 a.m. | #1
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
Laurent Pinchart April 20, 2019, 9:29 p.m. | #2
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);
>  }
Tomi Valkeinen May 3, 2019, 9:20 a.m. | #3
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
Laurent Pinchart May 3, 2019, 12:55 p.m. | #4
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 */
Tomi Valkeinen May 3, 2019, 1:08 p.m. | #5
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

Patch

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);
 }