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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
