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

[Guenter Roeck <linux@roeck-us.net>]

On 05/25/2016 04:51 AM, Heikki Krogerus wrote:
> 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@linux.intel.com>
>>
>> [ ... ]
>>
>>> +
>>> +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.
>

Alternate mode discovery is an orthogonal process to the connection
state machine, and may take a while to complete. Are you saying
that the call to typec_connect() should be delayed until after
alternate mode discovery completes or times out ?

So far I call typec_connect() in SRC.Ready and SNK.Ready, and
typec_register_altmodes() after mode discovery is complete.
It is also orthogonal, meaning it is only called if and when alternate
mode discovery completes, and the alternate mode discovery state machine
is separate to the port state machine.

No problem for me to change that, just making sure that the registration
delay is understood and accepted.

Thanks,
Guenter

>>> +
>>> +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,
>