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

[Laurent Pinchart]

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

:-(

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 */

-- 
Regards,

Laurent Pinchart