Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Tue, May 24, 2016 at 12:28:26PM -0700, Guenter Roeck wrote:
> On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote:
> > The purpose of this class is to provide unified interface for user
> > space to get the status and basic information about USB Type-C
> > Connectors in the system, control data role swapping, and when USB PD
> > is available, also power role swapping and Alternate Modes.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> 
> [ ... ]
> 
> > +
> > +static void typec_remove_partner(struct typec_port *port)
> > +{
> > +	sysfs_remove_link(&port->dev.kobj, "partner");
> > +	typec_unregister_altmodes(port->partner->alt_modes);
> 
> This only unregisters alternate modes registered through typec_add_partner(),
> but not alternate modes registered separately. Or is the calling code expected
> to set port->partner->alt_modes when calling typec_register_altmodes()
> directly ?

The altmodes for the partner are not meant to be registered
separately. With the partners and also cable plugs the class is in
control of registering and unregistering of the altmode devices after
typec_connect() is called.

The idea was that only the ports will register the alternate modes
they support separately, but I think we have to change that too. So I
don't think we'll export the typec_un/register_altmodes() at all.

We will have to prevent any drivers from being bound to the port
alternate mode devices when we add the alternate mode bus, and I had
some idea where by making the port drivers themselves in charge of
registering the port alternate modes, we could prevent it easily. But
it's probable easier to just handle those in the class driver as well.

> > +
> > +void typec_unregister_altmodes(struct typec_altmode *alt_modes)
> > +{
> > +	struct typec_altmode *alt;
> > +
> This will crash if alt_modes is NULL, which will happen if 
> partner->alt_modes is NULL at connection time. Semantically
> this is different to typec_register_altmodes(), which does
> have a NULL check.

Yes, need to fix that.


Thanks Guenter,

-- 
heikki