Re: [PATCH v4 2/6] apple-gmux: Add switch_ddc support

From: Daniel Vetter <daniel@ffwll.ch>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Darren Hart <dvhart@infradead.org>
Subject: Re: [PATCH v4 2/6] apple-gmux: Add switch_ddc support
Date: Tue, 13 Oct 2015 09:32:05 +0200	[thread overview]
Message-ID: <20151013073205.GE26718@phenom.ffwll.local> (raw)
In-Reply-To: <CADnq5_P6OAkHCUzksdBeSzxZxrgD6XS+HeAvDXr=01oDDhM1TA@mail.gmail.com>

On Mon, Oct 12, 2015 at 05:10:20PM -0400, Alex Deucher wrote:
> On Mon, Oct 12, 2015 at 5:07 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> > On Fri, Aug 14, 2015 at 12:18 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >> Originally by Seth Forshee <seth.forshee@canonical.com>, 2012-10-04:
> >>     The gmux allows muxing the DDC independently from the display, so
> >>     support this functionality. This will allow reading the EDID for the
> >>     inactive GPU, fixing issues with machines that either don't have a
> >>     VBT or have invalid mode data in the VBT.
> >>
> >> Modified by Lukas Wunner <lukas@wunner.de>, 2015-10-07:
> >>     Advertise ->switch_ddc handler callback only on the pre-retina
> >>     Macbook Pro. The retina uses eDP and its mux is incapable of
> >>     switching the AUX channel separately from the main link.
> >>     It's an NXP CBTL06142 (alternate parts: TI HD3SS212,
> >>     Pericom PPI3VDP12412).
> >>
> >>     Sources:
> >>     http://www.electronicproducts.com/-whatsinside_text-145.aspx
> >>     http://slideshare.net/jjwu6266/apple-2012-wwdc-apple-macbook-pro-with-retina-display
> >>     http://www.techrepublic.com/blog/cracking-open/teardown-shows-retina-macbook-pro-is-nearly-impossible-to-upgrade-difficult-to-work-on/
> >>
> >>     Datasheets:
> >>     http://www.nxp.com/documents/data_sheet/CBTL06141.pdf
> >>     http://www.ti.com/lit/ds/symlink/hd3ss212.pdf
> >>     https://www.pericom.com/assets/Datasheets/PI3VDP12412.pdf
> >>
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> >> Tested-by: Lukas Wunner <lukas@wunner.de>
> >>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> >>
> >> Cc: Seth Forshee <seth.forshee@canonical.com>
> >> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> >> ---
> >>  drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
> >>  1 file changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> >> index 0dec3f5..78997b7 100644
> >> --- a/drivers/platform/x86/apple-gmux.c
> >> +++ b/drivers/platform/x86/apple-gmux.c
> >> @@ -276,11 +276,9 @@ static const struct backlight_ops gmux_bl_ops = {
> >>  static int gmux_switchto(enum vga_switcheroo_client_id id)
> >>  {
> >>         if (id == VGA_SWITCHEROO_IGD) {
> >> -               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
> >>                 gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2);
> >>                 gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 2);
> >>         } else {
> >> -               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
> >>                 gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 3);
> >>                 gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
> >>         }
> >
> > Won't this hunk break manual switching until the later patches land?
> > Seems like you might want to break this out as a separate patch later
> > in the series.
> 
> Also. I'm not sure how the gmux works, but this might break ddc on
> external displays that are muxed.

Yeah I think the old desing was less surprising. And the
s/ddc_lock/hw_lock/ change would the make sense again.

Also when resending please also keep a per-patch changelog, not only in
the cover letter. Otherwise you have to jump back&forth all the time
between patches and cover letter. And the kerneldoc still seems to be
missing in this resend, so looks incomplete (or I'm missing something).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel