Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI

[Heikki Krogerus]

Hi,

On Wed, May 11, 2016 at 07:47:18AM -0700, Guenter Roeck wrote:
> 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.

I'm actually struggling with this same question. I decided to protect
the whole struct typec_port by not allowing the drivers to touch it,
but I'm still working on it..

> 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 partner types don't map directly to the USB PD Product Types. We
need to describe the partner even when USB PD is not available.
Accessory Modes are for example out side the scope of USB PD.

But I'll try to propose something for those.

> 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.

OK. I'll check these.

So I failed to finish my proposal for v2 this week. On top of the
sync/protection problems, I'm also still trying to tune my AltMode
bus. I'm going to attempt to send it as an RFC on Monday or Tuesday in
any case.


Thanks,

-- 
heikki