[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160511144718.GC8192@roeck-us.net>
Date: Wed, 11 May 2016 07:47:18 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Greg KH <gregkh@...uxfoundation.org>, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org,
Mathias Nyman <mathias.nyman@...ux.intel.com>,
Felipe Balbi <balbi@...nel.org>
Subject: Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI
Hi,
On Wed, May 11, 2016 at 12:40:11PM +0300, Heikki Krogerus wrote:
> On Tue, May 10, 2016 at 08:14:34PM -0700, Guenter Roeck wrote:
> > Heikki,
> >
> > On 05/06/2016 01:08 AM, Heikki Krogerus wrote:
> > > Hi,
> > >
> > [ ... ]
> > >
> > > I don't have not made any new code for the class driver yet, but I'm
> > > attempting to prepare v2 next week.
> > >
> > Would it make sense to send feedback about v1 now, or should I wait for v2 ?
>
> I don't think I'm able to send v2 today, or even tomorrow, so feel
> free to give the feedback. Just be aware that I've rewritten the
> alternate mode part completely.
>
Alternate mode handling was my major concern, actually.
> I'm creating a separate device for the partner and also the cable
> during connection. I'm also already going to introduce a small bus for
> the AltModes. It's clear that we need to have AltMode specific
> drivers. The generic parts can't take care of all the AltMode specific
> requirements and VDMs. The bus will give us a nice way to bind those
> drivers to the actual AltModes a partner and the cable plugs offer.
>
> So if there are dependencies between the altmodes, for example if the
> cable plugs needs to be in a certain mode in order for the partner to
> be able to function in some specific mode, the responsibility of
> taking care of those will fall primarily to in the AltMode drivers.
> So not userspace.
>
> The AltMode drivers actually are useful also as they can be part of
> the relevant frameworks, for example DP in some graphics framework.
> For example in case of DP, the number of lanes (I guess 2 or 4) should
> be ideally known if I have understood correctly. Knowledge about the
> connection seems to also be needed, and I've so far seen some pretty
> weird solutions for hotplug events with the DP AltMode. With the
> driver we should be able to avoid those.
>
> But in any case, every SVIDs a partner (or plug) offers will have
> their own device registered with the partner (or cable) itself as
> parent in this design. I'm expecting a little bit of conversation
> about this plan, but right now I feel confident about it.
>
> How does this sound to you?
>
Looking forward to it. My major problem so far was that alternate mode
handling is very platform specific, which didn't seem to be well supported
in v1 of your patch. I thought about implementing a hierarchy of drivers
below the type-c class to solve that problem. Looks like you just solved
it for me.
Other than that, my major concern is the lack of synchronization/protection
between the type-c class and the drivers. Setting port parameters (data role,
power role, operational power role, partner alternate modes, partner type)
from registered drivers may need to be synchronzed/protected. For example,
data and power role are set during connection establishment, but can be
overwritten from the typec class code. Right now I am just setting the
respective variables in struct typec_port directly, but that doesn't seem
right.
For partner_type, I don't really know how to map the options to the identity
reported by the partner. The reported product types are unknown / hub /
peripheral / passive cable / active cable / alternate mode adapter.
The available partner types are unknown / USB / Charger / Alternate Mode /
Audio Accessory / Debug Accessory. What am I missing here ?
The rest is just nitpicks.
- alternate_modes_show() and partner_alt_modes_show() discard the last byte
of the generated string and replace it with \0.
- s/Accessroy/Accessory/
- typec_connect() and typec_disconnect() should probably also set
port->connected.
Thanks,
Guenter
Powered by blists - more mailing lists