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

[Guenter Roeck]

Hi,

On Fri, May 13, 2016 at 05:23:21PM +0300, Heikki Krogerus wrote:
> 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..
> 
Sounds good to me, as long as you provide APIs to change the values.

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

> > 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.
> 
No worries. It turns out the PD code in existing type-c devices isn't
exactly stable or predictable, so I end up spending a lot of time
trying to stabilize my state machine anyway.

Thanks,
Guenter