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@xxxxxx>
> >> ---
> >> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
