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

[Tomi Valkeinen]

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 at 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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki