Re: [PATCHv14 2/3] usb: USB Type-C connector class

[Guenter Roeck]

On Tue, Jan 10, 2017 at 04:46:12PM +0200, Heikki Krogerus wrote:
> On Tue, Jan 10, 2017 at 05:50:04AM -0800, Guenter Roeck wrote:
> > On 01/10/2017 12:54 AM, Heikki Krogerus wrote:
> > > Hi Guenter,
> > > 
> > > On Mon, Jan 09, 2017 at 08:59:32AM -0800, Guenter Roeck wrote:
> > > > > +/**
> > > > > + * typec_register_partner - Register a USB Type-C Partner
> > > > > + * @port: The USB Type-C Port the partner is connected to
> > > > > + * @desc: Description of the partner
> > > > > + *
> > > > > + * Registers a device for USB Type-C Partner described in @desc.
> > > > > + *
> > > > > + * Returns handle to the partner on success or NULL on failure.
> > > > > + */
> > > > > +struct typec_partner *typec_register_partner(struct typec_port *port,
> > > > > +					     struct typec_partner_desc *desc)
> > > > > +{
> > > > 
> > > > With the changes to hide the actual partner structure, this looks at first
> > > > glance like a minor API change, but it is substantial.
> > > > 
> > > > Reason is that the vdo as required by typec_partner_desc is provided by a VDM
> > > > command reply, which is completely orthogonal to the PD registration process.
> > > > So far I was able to set the vdo later, after registering the connection,
> > > > and after (and if) the vdo was received.
> > > 
> > > If the identity vdo value is updated after the creation of the device,
> > > then the user space needs to be notified separately.
> > > 
> > > > Since the partner may not even respond to the DISCOVER_IDENT message, or not
> > > > support PD at all, this means that I would have to disconnect partner
> > > > registration from the PD protocol itself and tie it to the VDO message
> > > > exchange, with appropriate timeouts to register anyway even if the identity
> > > > was not received after some period of time or if the partner does not support
> > > > PD.
> > > > 
> > > > This in turn means that I'll have to re-implement and possibly re-architect
> > > > a substantial amount of code.
> > > 
> > > We don't need to protect the structures like this, we can change this
> > > back. But how about we introduce driver callback function for updating
> > > the value instead, which would also notify the uses space?
> > > 
> > 
> > That would work.
> 
> OK, cool.
> 
> I guess we might as well then split the VDO into header, cert stat and
> product parts. What do you think?
> 
> If it's OK, then should we change that file to "identity" and dump the
> whole response from Discover Identity command in hex (minus VDM
> Header), separate the parts in the output, or simply provide separate
> attribute files for each part?
> 
Makes me wonder what you expect in the vdo attribute today. Right now
I copy the value from the identity header into it.

Personally I would prefer separate attributes. identity, cert_stat,
product, maybe ?

> Just as a reminder, the user space can't rely on that attribute file.
> We still can't get any of that information from UCSI or for example
> the Thunderbolt controllers, which is annoying, but I guess it does
> not matter.
> 
> An other question:
> I would like to hide the attribute file(s) when the partner does not
> support USB Power Delivery. Is it OK with you guys?
> 
Ok with me.

Thanks,
Guenter