[PATCHv2 10/22] drm/bridge: tc358767: add link disable function

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Apr 15, 2019 at 02:39:21PM +0300, Tomi Valkeinen wrote:
> On 15/04/2019 11:36, Andrzej Hajda wrote:
> 
> >> +static int tc_main_link_disable(struct tc_data *tc)
> >> +{
> >> +	int ret;
> >> +
> >> +	dev_dbg(tc->dev, "link disable\n");
> >> +
> >> +	tc_write(DP0_SRCCTRL, 0);
> >> +	tc_write(DP0CTL, 0);
> > 
> > The same register is set in stream_disable, is it correct? Looks
> > suspicious, disabling stream and link should be different things.
> 
> Good point. It's probably not correct. With these patches, the link and
> stream are always enabled and disabled together, so it shouldn't cause
> any problems at the moment.
> 
> During my debugging and development, at some point I had a version where
> I enabled the link right away when we got HPD high, mostly to help my
> debugging. That's why I separated link and stream setup (although I
> think that's a logical design in any case).
> 
> However, I never did try disabling only stream, and keeping the link up,
> so it may well be non-functional. Or, well, it clearly is
> non-functional, as we disable the link (DP0CTL) in tc_stream_disable()...
> 
> So this is mostly about just adding the architecture to have separate
> link/stream handling, but the functionality is not there. I should
> perhaps add a comment to the code to point this out.

This makes me a bit uneasy about the rework, as the
tc_main_link_enable() function doesn't enable the link (it doesn't set
the DP_EN bit in DP0CTL), and stream disabling separately from link
disabling is broken. Is this fixable ?

-- 
Regards,

Laurent Pinchart