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