Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Rajaram R]

On Thu, Feb 18, 2016 at 4:17 PM, Heikki Krogerus
 wrote:
> On Thu, Feb 18, 2016 at 04:07:54PM +0530, Rajaram R wrote:
>> On Wed, Feb 17, 2016 at 7:58 PM, Heikki Krogerus
>>  wrote:
>> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
>> >>
>> >> Hi,
>> >>
>> >> Heikki Krogerus  writes:
>> >> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
>> >> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
>> >> >> > Hi,
>> >> >> >
>> >> >> > Oliver Neukum  writes:
>> >> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> >> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> >> >>
>> >> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
>> >> >> > >> > is to be supported, it needs to be defined now.
>> >> >> > >>
>> >> >> > >> When you say API, do you mean the API the class provides to the
>> >> >> > >> drivers? Or did you mean ABI which would be the sysfs in this 
>> >> >> > >> case?
>> >> >> > >
>> >> >> > > The API to user space. That is the point. We cannot break user 
>> >> >> > > space.
>> >> >> > > Once this sysfs API is upstream we are stuck with it.
>> >> >> >
>> >> >> > yeah, in fact I have been wondering if sysfs is the best interface to
>> >> >>
>> >> >> That is the discussion we must have.
>> >> >>
>> >> >> > userspace. I talked with Heikki a few days back about this; I was
>> >> >> > wondering if something like what the NFC folks did with netlink 
>> >> >> > would be
>> >> >> > better here.
>> >> >>
>> >> >> I doubt that, because the main user is likely to be udev scripts.
>> >> >> They can easily deal with sysfs attributes.
>> >> >
>> >> > IMHO for high level interface like this, sysfs is ideal because of the
>> >> > simple fact that you only need a shell to access the files. netlink
>> >> > would make us depend on custom software, no?
>> >> >
>> >> > I'm not against using netlink, but what would be the benefit from it
>> >> > in this case?
>> >>
>> >> With HW we see nowadays, CC stack is hidden on some microcontroller, but
>> >> is it too far-fetched to consider a system where this is not the case ?
>> >
>> > There already are several USB PD stacks out there, like also Greg
>> > pointed out.
>> >
>> >> Specially when we consider things like power delivery which, I know, you
>> >> wanted to keep it out of this interface, however we would have two
>> >> 'stacks' competing for access to the same pins, right ?
>> >
>> > No. This class would be the top layer for the coming stack, where ever
>> > it ends up coming. The class is only the interface to the user space
>> > and nothing else.
>> >
>> > By saying we need to keep USB Type-C separate from USB PD I meant that
>> > the userspace access can not be mixed somewhere in layers of the USB
>> > PD/CC stack like it has been in the USB PD stacks I've seen so far.
>> > They assume that we always use the software USB PD stack with USB
>> > Type-C, which as we can see is not true when the stack is implemented
>> > in EC or firmware or some complex USB PD controller or what ever.
>> > However, the operations the userspace needs to do are exactly the same
>> > in both cases.
>> >
>> > - data role swapping
>> > - power role swapping (depends on USB PD)
>> > - Alternate Modes (depends on USB PD)
>> >
>> > And we really should not forget that we actually also have USB Type-C
>> > PHYs that can't do any USB PD communication over the CC pin, so USB PD
>> > is simply not always going to be available. But the data role swapping
>> > and also accessories are still available with them, as the do not need
>> > USB PD.
>> >
>> > This was the whole point with the class. It allows the different ways
>> > of dealing with Type-C ports to be exposed to userspace in the same
>> > way.
>> >
>> >> IIRC mode and role negotiation goes via CC pins using the power delivery
>> >> protocol. If I misunderstand anything, let me know.
>> >
>> > The data role swap with USB Type-C connectors is in no way tied to USB
>> > Power Delivery. The USB Type-C spec defines that when USB PD is
>>
>> Its not data role swap i guess its dual role, A Data role swap is tied
>> with USB PD,
>>
>> > available, DR_Swap USB PD function is used to swap the role, otherwise
>> > emulated disconnect will do the trick.
>>
>> I doubt a USB host with no device capability implement DRP ?? Also
>> emulated trick(??) is not spec requirement rt ?
>>
>> >
>> > Data role swapping is a must thing to have with USB Type-C connectors
>>
>> I guess you are referring to Dual role (DRP) and not data role (DRD).
>
> There is no term "DRD" in USB Type-C spec. A quote from Type-C spec

Yes, not in Spec 1.1 but a new term to differentiate data and power
roles . All I wanted to bring in some difference between data role
swap and DRP

> ch. 2.3.3:
>
> "Two methods are defined to allow a USB Type-C DRP to functionally
> swap 

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Rajaram R]

On Thu, Feb 18, 2016 at 4:17 PM, Heikki Krogerus
 wrote:
> On Thu, Feb 18, 2016 at 04:07:54PM +0530, Rajaram R wrote:
>> On Wed, Feb 17, 2016 at 7:58 PM, Heikki Krogerus
>>  wrote:
>> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
>> >>
>> >> Hi,
>> >>
>> >> Heikki Krogerus  writes:
>> >> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
>> >> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
>> >> >> > Hi,
>> >> >> >
>> >> >> > Oliver Neukum  writes:
>> >> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> >> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> >> >>
>> >> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
>> >> >> > >> > is to be supported, it needs to be defined now.
>> >> >> > >>
>> >> >> > >> When you say API, do you mean the API the class provides to the
>> >> >> > >> drivers? Or did you mean ABI which would be the sysfs in this 
>> >> >> > >> case?
>> >> >> > >
>> >> >> > > The API to user space. That is the point. We cannot break user 
>> >> >> > > space.
>> >> >> > > Once this sysfs API is upstream we are stuck with it.
>> >> >> >
>> >> >> > yeah, in fact I have been wondering if sysfs is the best interface to
>> >> >>
>> >> >> That is the discussion we must have.
>> >> >>
>> >> >> > userspace. I talked with Heikki a few days back about this; I was
>> >> >> > wondering if something like what the NFC folks did with netlink 
>> >> >> > would be
>> >> >> > better here.
>> >> >>
>> >> >> I doubt that, because the main user is likely to be udev scripts.
>> >> >> They can easily deal with sysfs attributes.
>> >> >
>> >> > IMHO for high level interface like this, sysfs is ideal because of the
>> >> > simple fact that you only need a shell to access the files. netlink
>> >> > would make us depend on custom software, no?
>> >> >
>> >> > I'm not against using netlink, but what would be the benefit from it
>> >> > in this case?
>> >>
>> >> With HW we see nowadays, CC stack is hidden on some microcontroller, but
>> >> is it too far-fetched to consider a system where this is not the case ?
>> >
>> > There already are several USB PD stacks out there, like also Greg
>> > pointed out.
>> >
>> >> Specially when we consider things like power delivery which, I know, you
>> >> wanted to keep it out of this interface, however we would have two
>> >> 'stacks' competing for access to the same pins, right ?
>> >
>> > No. This class would be the top layer for the coming stack, where ever
>> > it ends up coming. The class is only the interface to the user space
>> > and nothing else.
>> >
>> > By saying we need to keep USB Type-C separate from USB PD I meant that
>> > the userspace access can not be mixed somewhere in layers of the USB
>> > PD/CC stack like it has been in the USB PD stacks I've seen so far.
>> > They assume that we always use the software USB PD stack with USB
>> > Type-C, which as we can see is not true when the stack is implemented
>> > in EC or firmware or some complex USB PD controller or what ever.
>> > However, the operations the userspace needs to do are exactly the same
>> > in both cases.
>> >
>> > - data role swapping
>> > - power role swapping (depends on USB PD)
>> > - Alternate Modes (depends on USB PD)
>> >
>> > And we really should not forget that we actually also have USB Type-C
>> > PHYs that can't do any USB PD communication over the CC pin, so USB PD
>> > is simply not always going to be available. But the data role swapping
>> > and also accessories are still available with them, as the do not need
>> > USB PD.
>> >
>> > This was the whole point with the class. It allows the different ways
>> > of dealing with Type-C ports to be exposed to userspace in the same
>> > way.
>> >
>> >> IIRC mode and role negotiation goes via CC pins using the power delivery
>> >> protocol. If I misunderstand anything, let me know.
>> >
>> > The data role swap with USB Type-C connectors is in no way tied to USB
>> > Power Delivery. The USB Type-C spec defines that when USB PD is
>>
>> Its not data role swap i guess its dual role, A Data role swap is tied
>> with USB PD,
>>
>> > available, DR_Swap USB PD function is used to swap the role, otherwise
>> > emulated disconnect will do the trick.
>>
>> I doubt a USB host with no device capability implement DRP ?? Also
>> emulated trick(??) is not spec requirement rt ?
>>
>> >
>> > Data role swapping is a must thing to have with USB Type-C connectors
>>
>> I guess you are referring to Dual role (DRP) and not data role (DRD).
>
> There is no term "DRD" in USB Type-C spec. A quote from Type-C spec

Yes, not in Spec 1.1 but a new term to differentiate data and power
roles . All I wanted to bring in some difference between data role
swap and DRP

> ch. 2.3.3:
>
> "Two methods are defined to allow a USB Type-C DRP to functionally
> swap data roles, one managed using USB PD DR_Swap and the other
> emulating a disconnect/reconnect sequence (see Figure 

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Thu, Feb 18, 2016 at 04:07:54PM +0530, Rajaram R wrote:
> On Wed, Feb 17, 2016 at 7:58 PM, Heikki Krogerus
>  wrote:
> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> >>
> >> Hi,
> >>
> >> Heikki Krogerus  writes:
> >> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
> >> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> >> >> > Hi,
> >> >> >
> >> >> > Oliver Neukum  writes:
> >> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> >> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> >> >>
> >> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
> >> >> > >> > is to be supported, it needs to be defined now.
> >> >> > >>
> >> >> > >> When you say API, do you mean the API the class provides to the
> >> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
> >> >> > >
> >> >> > > The API to user space. That is the point. We cannot break user 
> >> >> > > space.
> >> >> > > Once this sysfs API is upstream we are stuck with it.
> >> >> >
> >> >> > yeah, in fact I have been wondering if sysfs is the best interface to
> >> >>
> >> >> That is the discussion we must have.
> >> >>
> >> >> > userspace. I talked with Heikki a few days back about this; I was
> >> >> > wondering if something like what the NFC folks did with netlink would 
> >> >> > be
> >> >> > better here.
> >> >>
> >> >> I doubt that, because the main user is likely to be udev scripts.
> >> >> They can easily deal with sysfs attributes.
> >> >
> >> > IMHO for high level interface like this, sysfs is ideal because of the
> >> > simple fact that you only need a shell to access the files. netlink
> >> > would make us depend on custom software, no?
> >> >
> >> > I'm not against using netlink, but what would be the benefit from it
> >> > in this case?
> >>
> >> With HW we see nowadays, CC stack is hidden on some microcontroller, but
> >> is it too far-fetched to consider a system where this is not the case ?
> >
> > There already are several USB PD stacks out there, like also Greg
> > pointed out.
> >
> >> Specially when we consider things like power delivery which, I know, you
> >> wanted to keep it out of this interface, however we would have two
> >> 'stacks' competing for access to the same pins, right ?
> >
> > No. This class would be the top layer for the coming stack, where ever
> > it ends up coming. The class is only the interface to the user space
> > and nothing else.
> >
> > By saying we need to keep USB Type-C separate from USB PD I meant that
> > the userspace access can not be mixed somewhere in layers of the USB
> > PD/CC stack like it has been in the USB PD stacks I've seen so far.
> > They assume that we always use the software USB PD stack with USB
> > Type-C, which as we can see is not true when the stack is implemented
> > in EC or firmware or some complex USB PD controller or what ever.
> > However, the operations the userspace needs to do are exactly the same
> > in both cases.
> >
> > - data role swapping
> > - power role swapping (depends on USB PD)
> > - Alternate Modes (depends on USB PD)
> >
> > And we really should not forget that we actually also have USB Type-C
> > PHYs that can't do any USB PD communication over the CC pin, so USB PD
> > is simply not always going to be available. But the data role swapping
> > and also accessories are still available with them, as the do not need
> > USB PD.
> >
> > This was the whole point with the class. It allows the different ways
> > of dealing with Type-C ports to be exposed to userspace in the same
> > way.
> >
> >> IIRC mode and role negotiation goes via CC pins using the power delivery
> >> protocol. If I misunderstand anything, let me know.
> >
> > The data role swap with USB Type-C connectors is in no way tied to USB
> > Power Delivery. The USB Type-C spec defines that when USB PD is
> 
> Its not data role swap i guess its dual role, A Data role swap is tied
> with USB PD,
> 
> > available, DR_Swap USB PD function is used to swap the role, otherwise
> > emulated disconnect will do the trick.
> 
> I doubt a USB host with no device capability implement DRP ?? Also
> emulated trick(??) is not spec requirement rt ?
> 
> >
> > Data role swapping is a must thing to have with USB Type-C connectors
> 
> I guess you are referring to Dual role (DRP) and not data role (DRD).

There is no term "DRD" in USB Type-C spec. A quote from Type-C spec
ch. 2.3.3:

"Two methods are defined to allow a USB Type-C DRP to functionally
swap data roles, one managed using USB PD DR_Swap and the other
emulating a disconnect/reconnect sequence (see Figure 4-16)"


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Thu, Feb 18, 2016 at 04:07:54PM +0530, Rajaram R wrote:
> On Wed, Feb 17, 2016 at 7:58 PM, Heikki Krogerus
>  wrote:
> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> >>
> >> Hi,
> >>
> >> Heikki Krogerus  writes:
> >> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
> >> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> >> >> > Hi,
> >> >> >
> >> >> > Oliver Neukum  writes:
> >> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> >> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> >> >>
> >> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
> >> >> > >> > is to be supported, it needs to be defined now.
> >> >> > >>
> >> >> > >> When you say API, do you mean the API the class provides to the
> >> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
> >> >> > >
> >> >> > > The API to user space. That is the point. We cannot break user 
> >> >> > > space.
> >> >> > > Once this sysfs API is upstream we are stuck with it.
> >> >> >
> >> >> > yeah, in fact I have been wondering if sysfs is the best interface to
> >> >>
> >> >> That is the discussion we must have.
> >> >>
> >> >> > userspace. I talked with Heikki a few days back about this; I was
> >> >> > wondering if something like what the NFC folks did with netlink would 
> >> >> > be
> >> >> > better here.
> >> >>
> >> >> I doubt that, because the main user is likely to be udev scripts.
> >> >> They can easily deal with sysfs attributes.
> >> >
> >> > IMHO for high level interface like this, sysfs is ideal because of the
> >> > simple fact that you only need a shell to access the files. netlink
> >> > would make us depend on custom software, no?
> >> >
> >> > I'm not against using netlink, but what would be the benefit from it
> >> > in this case?
> >>
> >> With HW we see nowadays, CC stack is hidden on some microcontroller, but
> >> is it too far-fetched to consider a system where this is not the case ?
> >
> > There already are several USB PD stacks out there, like also Greg
> > pointed out.
> >
> >> Specially when we consider things like power delivery which, I know, you
> >> wanted to keep it out of this interface, however we would have two
> >> 'stacks' competing for access to the same pins, right ?
> >
> > No. This class would be the top layer for the coming stack, where ever
> > it ends up coming. The class is only the interface to the user space
> > and nothing else.
> >
> > By saying we need to keep USB Type-C separate from USB PD I meant that
> > the userspace access can not be mixed somewhere in layers of the USB
> > PD/CC stack like it has been in the USB PD stacks I've seen so far.
> > They assume that we always use the software USB PD stack with USB
> > Type-C, which as we can see is not true when the stack is implemented
> > in EC or firmware or some complex USB PD controller or what ever.
> > However, the operations the userspace needs to do are exactly the same
> > in both cases.
> >
> > - data role swapping
> > - power role swapping (depends on USB PD)
> > - Alternate Modes (depends on USB PD)
> >
> > And we really should not forget that we actually also have USB Type-C
> > PHYs that can't do any USB PD communication over the CC pin, so USB PD
> > is simply not always going to be available. But the data role swapping
> > and also accessories are still available with them, as the do not need
> > USB PD.
> >
> > This was the whole point with the class. It allows the different ways
> > of dealing with Type-C ports to be exposed to userspace in the same
> > way.
> >
> >> IIRC mode and role negotiation goes via CC pins using the power delivery
> >> protocol. If I misunderstand anything, let me know.
> >
> > The data role swap with USB Type-C connectors is in no way tied to USB
> > Power Delivery. The USB Type-C spec defines that when USB PD is
> 
> Its not data role swap i guess its dual role, A Data role swap is tied
> with USB PD,
> 
> > available, DR_Swap USB PD function is used to swap the role, otherwise
> > emulated disconnect will do the trick.
> 
> I doubt a USB host with no device capability implement DRP ?? Also
> emulated trick(??) is not spec requirement rt ?
> 
> >
> > Data role swapping is a must thing to have with USB Type-C connectors
> 
> I guess you are referring to Dual role (DRP) and not data role (DRD).

There is no term "DRD" in USB Type-C spec. A quote from Type-C spec
ch. 2.3.3:

"Two methods are defined to allow a USB Type-C DRP to functionally
swap data roles, one managed using USB PD DR_Swap and the other
emulating a disconnect/reconnect sequence (see Figure 4-16)"


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

Hi Peter,

On Thu, Feb 18, 2016 at 05:07:04PM +0800, Peter Chen wrote:
> On Wed, Feb 17, 2016 at 04:28:16PM +0200, Heikki Krogerus wrote:
> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> > > IIRC mode and role negotiation goes via CC pins using the power delivery
> > > protocol. If I misunderstand anything, let me know.
> > 
> > The data role swap with USB Type-C connectors is in no way tied to USB
> > Power Delivery. The USB Type-C spec defines that when USB PD is
> > available, DR_Swap USB PD function is used to swap the role, otherwise
> > emulated disconnect will do the trick.
> 
> I am interested in how you design role swap on the fly without USB PD.
> Do you follow the spec like USB OTG 3.0 RSP (Role Swap Protocol) or
> just echo the /sys to give up current role, and swap to another?

No OTG with USB Type-C. You echo the wanted role to the
/sys/class/type-C/usbcN/data_role.

This operation from userspace is the same regardless was USB PD
supported or not. The actual operations needed for the role swap are
of course platform specific, and the responsibility of the drivers
that register the ports with type-c class.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

Hi Peter,

On Thu, Feb 18, 2016 at 05:07:04PM +0800, Peter Chen wrote:
> On Wed, Feb 17, 2016 at 04:28:16PM +0200, Heikki Krogerus wrote:
> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> > > IIRC mode and role negotiation goes via CC pins using the power delivery
> > > protocol. If I misunderstand anything, let me know.
> > 
> > The data role swap with USB Type-C connectors is in no way tied to USB
> > Power Delivery. The USB Type-C spec defines that when USB PD is
> > available, DR_Swap USB PD function is used to swap the role, otherwise
> > emulated disconnect will do the trick.
> 
> I am interested in how you design role swap on the fly without USB PD.
> Do you follow the spec like USB OTG 3.0 RSP (Role Swap Protocol) or
> just echo the /sys to give up current role, and swap to another?

No OTG with USB Type-C. You echo the wanted role to the
/sys/class/type-C/usbcN/data_role.

This operation from userspace is the same regardless was USB PD
supported or not. The actual operations needed for the role swap are
of course platform specific, and the responsibility of the drivers
that register the ports with type-c class.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-18 at 12:30 +0200, Felipe Balbi wrote:
> Hi,
> 
> Oliver Neukum  writes:
> >> > What exactly are you sure about about?
> >> 
> >> heh, missed a NOT there :-)
> >
> > I am still confused :-)
> > Do you think a sysfs interface is good, bad or good
> > but insufficient?
> 
> I'm not sure it's the best interface. My fear is that as new
> requirements/features come along, the amount of files will continue to
> grow.

That will happen. The alternative, however is a "typectool" or
"usbpdtool" which would need to be updated for new features.

> >> I guess what I'm trying to say is that CC microcontroller might not be
> >> the one controlling the multiplexer which switches USB pins to another
> >> function. IOW:
> >> 
> >> Change to alternate mode X message
> >>  CC microcontroller interrupts CPU
> >>   read status to get X
> >>change multiplexer
> >> 
> >
> > Yes. But it seems to me that in this case we need a kernel driver
> > without an API to user space. There are necessarily internal users
> 
> that assumes kernel always knows all possible alternate modes. What do
> we about bogus requests (request alternate mode X when X is not
> supported) ?

Do as the spec says: NACK it.

The questions which modes we offer, if we are a slave, still remains.
And I think the API is deficient in that regard. But again that question
is orthogonal of both issue, handling of bogus requests and how the
CC pins are exported.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-18 at 12:30 +0200, Felipe Balbi wrote:
> Hi,
> 
> Oliver Neukum  writes:
> >> > What exactly are you sure about about?
> >> 
> >> heh, missed a NOT there :-)
> >
> > I am still confused :-)
> > Do you think a sysfs interface is good, bad or good
> > but insufficient?
> 
> I'm not sure it's the best interface. My fear is that as new
> requirements/features come along, the amount of files will continue to
> grow.

That will happen. The alternative, however is a "typectool" or
"usbpdtool" which would need to be updated for new features.

> >> I guess what I'm trying to say is that CC microcontroller might not be
> >> the one controlling the multiplexer which switches USB pins to another
> >> function. IOW:
> >> 
> >> Change to alternate mode X message
> >>  CC microcontroller interrupts CPU
> >>   read status to get X
> >>change multiplexer
> >> 
> >
> > Yes. But it seems to me that in this case we need a kernel driver
> > without an API to user space. There are necessarily internal users
> 
> that assumes kernel always knows all possible alternate modes. What do
> we about bogus requests (request alternate mode X when X is not
> supported) ?

Do as the spec says: NACK it.

The questions which modes we offer, if we are a slave, still remains.
And I think the API is deficient in that regard. But again that question
is orthogonal of both issue, handling of bogus requests and how the
CC pins are exported.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Rajaram R]

On Wed, Feb 17, 2016 at 7:58 PM, Heikki Krogerus
 wrote:
> On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Heikki Krogerus  writes:
>> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
>> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
>> >> > Hi,
>> >> >
>> >> > Oliver Neukum  writes:
>> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> >>
>> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
>> >> > >> > is to be supported, it needs to be defined now.
>> >> > >>
>> >> > >> When you say API, do you mean the API the class provides to the
>> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
>> >> > >
>> >> > > The API to user space. That is the point. We cannot break user space.
>> >> > > Once this sysfs API is upstream we are stuck with it.
>> >> >
>> >> > yeah, in fact I have been wondering if sysfs is the best interface to
>> >>
>> >> That is the discussion we must have.
>> >>
>> >> > userspace. I talked with Heikki a few days back about this; I was
>> >> > wondering if something like what the NFC folks did with netlink would be
>> >> > better here.
>> >>
>> >> I doubt that, because the main user is likely to be udev scripts.
>> >> They can easily deal with sysfs attributes.
>> >
>> > IMHO for high level interface like this, sysfs is ideal because of the
>> > simple fact that you only need a shell to access the files. netlink
>> > would make us depend on custom software, no?
>> >
>> > I'm not against using netlink, but what would be the benefit from it
>> > in this case?
>>
>> With HW we see nowadays, CC stack is hidden on some microcontroller, but
>> is it too far-fetched to consider a system where this is not the case ?
>
> There already are several USB PD stacks out there, like also Greg
> pointed out.
>
>> Specially when we consider things like power delivery which, I know, you
>> wanted to keep it out of this interface, however we would have two
>> 'stacks' competing for access to the same pins, right ?
>
> No. This class would be the top layer for the coming stack, where ever
> it ends up coming. The class is only the interface to the user space
> and nothing else.
>
> By saying we need to keep USB Type-C separate from USB PD I meant that
> the userspace access can not be mixed somewhere in layers of the USB
> PD/CC stack like it has been in the USB PD stacks I've seen so far.
> They assume that we always use the software USB PD stack with USB
> Type-C, which as we can see is not true when the stack is implemented
> in EC or firmware or some complex USB PD controller or what ever.
> However, the operations the userspace needs to do are exactly the same
> in both cases.
>
> - data role swapping
> - power role swapping (depends on USB PD)
> - Alternate Modes (depends on USB PD)
>
> And we really should not forget that we actually also have USB Type-C
> PHYs that can't do any USB PD communication over the CC pin, so USB PD
> is simply not always going to be available. But the data role swapping
> and also accessories are still available with them, as the do not need
> USB PD.
>
> This was the whole point with the class. It allows the different ways
> of dealing with Type-C ports to be exposed to userspace in the same
> way.
>
>> IIRC mode and role negotiation goes via CC pins using the power delivery
>> protocol. If I misunderstand anything, let me know.
>
> The data role swap with USB Type-C connectors is in no way tied to USB
> Power Delivery. The USB Type-C spec defines that when USB PD is

Its not data role swap i guess its dual role, A Data role swap is tied
with USB PD,

> available, DR_Swap USB PD function is used to swap the role, otherwise
> emulated disconnect will do the trick.

I doubt a USB host with no device capability implement DRP ?? Also
emulated trick(??) is not spec requirement rt ?

>
> Data role swapping is a must thing to have with USB Type-C connectors

I guess you are referring to Dual role (DRP) and not data role (DRD).

> because of the fact that the role is selected randomly. Regardless was
> USB PD supported or not.
>
>
> Thanks,
>
> --
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Rajaram R]

On Wed, Feb 17, 2016 at 7:58 PM, Heikki Krogerus
 wrote:
> On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Heikki Krogerus  writes:
>> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
>> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
>> >> > Hi,
>> >> >
>> >> > Oliver Neukum  writes:
>> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> >>
>> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
>> >> > >> > is to be supported, it needs to be defined now.
>> >> > >>
>> >> > >> When you say API, do you mean the API the class provides to the
>> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
>> >> > >
>> >> > > The API to user space. That is the point. We cannot break user space.
>> >> > > Once this sysfs API is upstream we are stuck with it.
>> >> >
>> >> > yeah, in fact I have been wondering if sysfs is the best interface to
>> >>
>> >> That is the discussion we must have.
>> >>
>> >> > userspace. I talked with Heikki a few days back about this; I was
>> >> > wondering if something like what the NFC folks did with netlink would be
>> >> > better here.
>> >>
>> >> I doubt that, because the main user is likely to be udev scripts.
>> >> They can easily deal with sysfs attributes.
>> >
>> > IMHO for high level interface like this, sysfs is ideal because of the
>> > simple fact that you only need a shell to access the files. netlink
>> > would make us depend on custom software, no?
>> >
>> > I'm not against using netlink, but what would be the benefit from it
>> > in this case?
>>
>> With HW we see nowadays, CC stack is hidden on some microcontroller, but
>> is it too far-fetched to consider a system where this is not the case ?
>
> There already are several USB PD stacks out there, like also Greg
> pointed out.
>
>> Specially when we consider things like power delivery which, I know, you
>> wanted to keep it out of this interface, however we would have two
>> 'stacks' competing for access to the same pins, right ?
>
> No. This class would be the top layer for the coming stack, where ever
> it ends up coming. The class is only the interface to the user space
> and nothing else.
>
> By saying we need to keep USB Type-C separate from USB PD I meant that
> the userspace access can not be mixed somewhere in layers of the USB
> PD/CC stack like it has been in the USB PD stacks I've seen so far.
> They assume that we always use the software USB PD stack with USB
> Type-C, which as we can see is not true when the stack is implemented
> in EC or firmware or some complex USB PD controller or what ever.
> However, the operations the userspace needs to do are exactly the same
> in both cases.
>
> - data role swapping
> - power role swapping (depends on USB PD)
> - Alternate Modes (depends on USB PD)
>
> And we really should not forget that we actually also have USB Type-C
> PHYs that can't do any USB PD communication over the CC pin, so USB PD
> is simply not always going to be available. But the data role swapping
> and also accessories are still available with them, as the do not need
> USB PD.
>
> This was the whole point with the class. It allows the different ways
> of dealing with Type-C ports to be exposed to userspace in the same
> way.
>
>> IIRC mode and role negotiation goes via CC pins using the power delivery
>> protocol. If I misunderstand anything, let me know.
>
> The data role swap with USB Type-C connectors is in no way tied to USB
> Power Delivery. The USB Type-C spec defines that when USB PD is

Its not data role swap i guess its dual role, A Data role swap is tied
with USB PD,

> available, DR_Swap USB PD function is used to swap the role, otherwise
> emulated disconnect will do the trick.

I doubt a USB host with no device capability implement DRP ?? Also
emulated trick(??) is not spec requirement rt ?

>
> Data role swapping is a must thing to have with USB Type-C connectors

I guess you are referring to Dual role (DRP) and not data role (DRD).

> because of the fact that the role is selected randomly. Regardless was
> USB PD supported or not.
>
>
> Thanks,
>
> --
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Oliver Neukum  writes:
>> > What exactly are you sure about about?
>> 
>> heh, missed a NOT there :-)
>
> I am still confused :-)
> Do you think a sysfs interface is good, bad or good
> but insufficient?

I'm not sure it's the best interface. My fear is that as new
requirements/features come along, the amount of files will continue to
grow.

>> I guess what I'm trying to say is that CC microcontroller might not be
>> the one controlling the multiplexer which switches USB pins to another
>> function. IOW:
>> 
>> Change to alternate mode X message
>>  CC microcontroller interrupts CPU
>>   read status to get X
>>change multiplexer
>> 
>
> Yes. But it seems to me that in this case we need a kernel driver
> without an API to user space. There are necessarily internal users

that assumes kernel always knows all possible alternate modes. What do
we about bogus requests (request alternate mode X when X is not
supported) ?

> of the PD controller. There are also external users. So the CC pins
> should be seen as a bus, which in essence they are (it reminds me
> of ancient ethernet actually), and if you really want full user
> space access, you'd need the quivalent of an sg driver.
>
> The issue is orthogonal to the question how we support UCSI, except
> that UCSI is a user of the CC pins and PD and frankly I don't see the
> firmware and a driver access this sanely simultaneously.  Therefore
> I'd prefer we make an API here which does not depend on UCSI, but can,
> if necessary, work on top of a driver doing full hardware access.

fair enough.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Oliver Neukum  writes:
>> > What exactly are you sure about about?
>> 
>> heh, missed a NOT there :-)
>
> I am still confused :-)
> Do you think a sysfs interface is good, bad or good
> but insufficient?

I'm not sure it's the best interface. My fear is that as new
requirements/features come along, the amount of files will continue to
grow.

>> I guess what I'm trying to say is that CC microcontroller might not be
>> the one controlling the multiplexer which switches USB pins to another
>> function. IOW:
>> 
>> Change to alternate mode X message
>>  CC microcontroller interrupts CPU
>>   read status to get X
>>change multiplexer
>> 
>
> Yes. But it seems to me that in this case we need a kernel driver
> without an API to user space. There are necessarily internal users

that assumes kernel always knows all possible alternate modes. What do
we about bogus requests (request alternate mode X when X is not
supported) ?

> of the PD controller. There are also external users. So the CC pins
> should be seen as a bus, which in essence they are (it reminds me
> of ancient ethernet actually), and if you really want full user
> space access, you'd need the quivalent of an sg driver.
>
> The issue is orthogonal to the question how we support UCSI, except
> that UCSI is a user of the CC pins and PD and frankly I don't see the
> firmware and a driver access this sanely simultaneously.  Therefore
> I'd prefer we make an API here which does not depend on UCSI, but can,
> if necessary, work on top of a driver doing full hardware access.

fair enough.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-18 at 09:08 +0200, Felipe Balbi wrote:

> Oliver Neukum  writes:
> >> Oliver Neukum  writes:

Hi,

> > What exactly are you sure about about?
> 
> heh, missed a NOT there :-)

I am still confused :-)
Do you think a sysfs interface is good, bad or good
but insufficient?

> >> that, eventually, we will need an actual stack exposed for the CC pin.
> >
> > The raw CC pin? What about the timing requirement?
> 
> well, not the _raw_ CC pin, but a situation where the microcontroller
> handling CC pin is "dumb", in the sense that it provides an interface
> for us to request/start arbitrary transactions. That will, in turn,
> shift the actual bits on the CC pin. Or something along these lines.

Well, how else would we send vendor specific messages?

> An example would be the alternate mode thing. CC microcontroller doesn't
> have to know what are the available alternate modes, but it needs to be
> able to tell processor what request is coming from the other end.

Indeed.

> I guess what I'm trying to say is that CC microcontroller might not be
> the one controlling the multiplexer which switches USB pins to another
> function. IOW:
> 
> Change to alternate mode X message
>  CC microcontroller interrupts CPU
>   read status to get X
>change multiplexer
> 

Yes. But it seems to me that in this case we need a kernel driver
without an API to user space. There are necessarily internal users
of the PD controller. There are also external users. So the CC pins
should be seen as a bus, which in essence they are (it reminds me
of ancient ethernet actually), and if you really want full user
space access, you'd need the quivalent of an sg driver.

The issue is orthogonal to the question how we support UCSI,
except that UCSI is a user of the CC pins and PD and frankly
I don't see the firmware and a driver access this sanely simultaneously.
Therefore I'd prefer we make an API here which does not depend on UCSI,
but can, if necessary, work on top of a driver doing full hardware
access.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-18 at 09:08 +0200, Felipe Balbi wrote:

> Oliver Neukum  writes:
> >> Oliver Neukum  writes:

Hi,

> > What exactly are you sure about about?
> 
> heh, missed a NOT there :-)

I am still confused :-)
Do you think a sysfs interface is good, bad or good
but insufficient?

> >> that, eventually, we will need an actual stack exposed for the CC pin.
> >
> > The raw CC pin? What about the timing requirement?
> 
> well, not the _raw_ CC pin, but a situation where the microcontroller
> handling CC pin is "dumb", in the sense that it provides an interface
> for us to request/start arbitrary transactions. That will, in turn,
> shift the actual bits on the CC pin. Or something along these lines.

Well, how else would we send vendor specific messages?

> An example would be the alternate mode thing. CC microcontroller doesn't
> have to know what are the available alternate modes, but it needs to be
> able to tell processor what request is coming from the other end.

Indeed.

> I guess what I'm trying to say is that CC microcontroller might not be
> the one controlling the multiplexer which switches USB pins to another
> function. IOW:
> 
> Change to alternate mode X message
>  CC microcontroller interrupts CPU
>   read status to get X
>change multiplexer
> 

Yes. But it seems to me that in this case we need a kernel driver
without an API to user space. There are necessarily internal users
of the PD controller. There are also external users. So the CC pins
should be seen as a bus, which in essence they are (it reminds me
of ancient ethernet actually), and if you really want full user
space access, you'd need the quivalent of an sg driver.

The issue is orthogonal to the question how we support UCSI,
except that UCSI is a user of the CC pins and PD and frankly
I don't see the firmware and a driver access this sanely simultaneously.
Therefore I'd prefer we make an API here which does not depend on UCSI,
but can, if necessary, work on top of a driver doing full hardware
access.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-18 at 17:29 +0800, Peter Chen wrote:

> Does this UCSI spec has some similar things with USB Type-C Port
> Controller Interface Spec at usb.org? If not, how to co-work
> together in future?

USB Type-C Port Controller Interface Spec:

What can a type C connector do?

UCSI spec:

How can the OS use ACPI to tell a type C connector what to do.

Obviously UCSI depends on the type C spec, but they cover different
things.

HTH
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-18 at 17:29 +0800, Peter Chen wrote:

> Does this UCSI spec has some similar things with USB Type-C Port
> Controller Interface Spec at usb.org? If not, how to co-work
> together in future?

USB Type-C Port Controller Interface Spec:

What can a type C connector do?

UCSI spec:

How can the OS use ACPI to tell a type C connector what to do.

Obviously UCSI depends on the type C spec, but they cover different
things.

HTH
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Peter Chen]

On Wed, Feb 10, 2016 at 12:30:42PM +0200, Heikki Krogerus wrote:
> On Tue, Feb 09, 2016 at 10:21:55AM -0800, Greg KH wrote:
> > On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> > > USB Type-C Connector System Software Interface (UCSI) is a
> > > specification that defines registers and data structures
> > > used to interface with the USB Type-C connectors on a system.
> > > 
> > > The specification is public and available at:
> > > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> > > 
> > 
> > What does this driver / code actually do?  Why is it needed?  What
> > interface to the rest of the kernel / userspace does it provide?
> 
> I will fix this describe these things in the commit message. I'll just
> but some UCSI background in case somebody is interested. So UCSI is in
> practice a standard for USB Type-C controllers..
> 

Does this UCSI spec has some similar things with USB Type-C Port
Controller Interface Spec at usb.org? If not, how to co-work
together in future?

Peter

> UCSI is the control interface for USB Type-C connectors (regardless
> was USB PD supported or not) in MS Windows, so most likely all new HW
> platforms designed to work also with Windows that are equipped with
> USB Type-C will have UCSI device for controlling the USB Type-C ports.
> In some cases the hardware for Type-C will be just a PHY like fusb30x
> on these platforms (it's cheaper then USB PD or complete USB Type-C
> controller), but in those cases the PHY is probable attached to an EC
> or is completely controlled by system FW like BIOS together with any
> USB PD communication in cases where USB PD is supported, and is in any
> case not visible to the OS. Instead UCSI device is exposed to the OS
> to give it means to apply its policies to the USB Type-C port.
> 
> > Why would we care about this?
> 
> I'll try to explain why it's important to export the control of USB
> Type-C ports to the user space in my answer to your comments to the
> first patch of this series, the one introducing the class.
> 
> But surely everybody agrees that decision about the policies regarding
> USB Type-C ports, like which data role to use, do we charge or are we
> letting the other end charge, etc., belongs to the user? If you plug
> your phone to your desktop, I would imagine that you want to see the
> phone primarily as the USB device and the desktop as host, and to
> achieve the device role, you don't want to be forced to unplug/replug
> your phone from the desktop until you achieve device role, right?
> 
> > You need to describe this a lot better than you did...
> 
> Sure thing. I'm sorry about the poor description. I send these out too
> hastily.
> 
> 
> Thanks,
> 
> -- 
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Peter Chen]

On Wed, Feb 10, 2016 at 12:30:42PM +0200, Heikki Krogerus wrote:
> On Tue, Feb 09, 2016 at 10:21:55AM -0800, Greg KH wrote:
> > On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> > > USB Type-C Connector System Software Interface (UCSI) is a
> > > specification that defines registers and data structures
> > > used to interface with the USB Type-C connectors on a system.
> > > 
> > > The specification is public and available at:
> > > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> > > 
> > 
> > What does this driver / code actually do?  Why is it needed?  What
> > interface to the rest of the kernel / userspace does it provide?
> 
> I will fix this describe these things in the commit message. I'll just
> but some UCSI background in case somebody is interested. So UCSI is in
> practice a standard for USB Type-C controllers..
> 

Does this UCSI spec has some similar things with USB Type-C Port
Controller Interface Spec at usb.org? If not, how to co-work
together in future?

Peter

> UCSI is the control interface for USB Type-C connectors (regardless
> was USB PD supported or not) in MS Windows, so most likely all new HW
> platforms designed to work also with Windows that are equipped with
> USB Type-C will have UCSI device for controlling the USB Type-C ports.
> In some cases the hardware for Type-C will be just a PHY like fusb30x
> on these platforms (it's cheaper then USB PD or complete USB Type-C
> controller), but in those cases the PHY is probable attached to an EC
> or is completely controlled by system FW like BIOS together with any
> USB PD communication in cases where USB PD is supported, and is in any
> case not visible to the OS. Instead UCSI device is exposed to the OS
> to give it means to apply its policies to the USB Type-C port.
> 
> > Why would we care about this?
> 
> I'll try to explain why it's important to export the control of USB
> Type-C ports to the user space in my answer to your comments to the
> first patch of this series, the one introducing the class.
> 
> But surely everybody agrees that decision about the policies regarding
> USB Type-C ports, like which data role to use, do we charge or are we
> letting the other end charge, etc., belongs to the user? If you plug
> your phone to your desktop, I would imagine that you want to see the
> phone primarily as the USB device and the desktop as host, and to
> achieve the device role, you don't want to be forced to unplug/replug
> your phone from the desktop until you achieve device role, right?
> 
> > You need to describe this a lot better than you did...
> 
> Sure thing. I'm sorry about the poor description. I send these out too
> hastily.
> 
> 
> Thanks,
> 
> -- 
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Peter Chen]

On Wed, Feb 17, 2016 at 04:28:16PM +0200, Heikki Krogerus wrote:
> On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> 
> > IIRC mode and role negotiation goes via CC pins using the power delivery
> > protocol. If I misunderstand anything, let me know.
> 
> The data role swap with USB Type-C connectors is in no way tied to USB
> Power Delivery. The USB Type-C spec defines that when USB PD is
> available, DR_Swap USB PD function is used to swap the role, otherwise
> emulated disconnect will do the trick.
> 

I am interested in how you design role swap on the fly without USB PD.
Do you follow the spec like USB OTG 3.0 RSP (Role Swap Protocol) or
just echo the /sys to give up current role, and swap to another?

-- 

Best Regards,
Peter Chen

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Peter Chen]

On Wed, Feb 17, 2016 at 04:28:16PM +0200, Heikki Krogerus wrote:
> On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> 
> > IIRC mode and role negotiation goes via CC pins using the power delivery
> > protocol. If I misunderstand anything, let me know.
> 
> The data role swap with USB Type-C connectors is in no way tied to USB
> Power Delivery. The USB Type-C spec defines that when USB PD is
> available, DR_Swap USB PD function is used to swap the role, otherwise
> emulated disconnect will do the trick.
> 

I am interested in how you design role swap on the fly without USB PD.
Do you follow the spec like USB OTG 3.0 RSP (Role Swap Protocol) or
just echo the /sys to give up current role, and swap to another?

-- 

Best Regards,
Peter Chen

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Oliver Neukum  writes:
>> Oliver Neukum  writes:
>> >> > The API to user space. That is the point. We cannot break user space.
>> >> > Once this sysfs API is upstream we are stuck with it.
>> >> 
>> >> yeah, in fact I have been wondering if sysfs is the best interface to
>> >
>> > That is the discussion we must have.
>> >
>> >> userspace. I talked with Heikki a few days back about this; I was
>> >> wondering if something like what the NFC folks did with netlink would be
>> >> better here.
>> >
>> > I doubt that, because the main user is likely to be udev scripts.
>> 
>> I'm entirely sure about this. I think it's not too far-fetched to think
>
> What exactly are you sure about about?

heh, missed a NOT there :-)

>> that, eventually, we will need an actual stack exposed for the CC pin.
>
> The raw CC pin? What about the timing requirement?

well, not the _raw_ CC pin, but a situation where the microcontroller
handling CC pin is "dumb", in the sense that it provides an interface
for us to request/start arbitrary transactions. That will, in turn,
shift the actual bits on the CC pin. Or something along these lines.

An example would be the alternate mode thing. CC microcontroller doesn't
have to know what are the available alternate modes, but it needs to be
able to tell processor what request is coming from the other end.

I guess what I'm trying to say is that CC microcontroller might not be
the one controlling the multiplexer which switches USB pins to another
function. IOW:

Change to alternate mode X message
 CC microcontroller interrupts CPU
  read status to get X
   change multiplexer

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Oliver Neukum  writes:
>> Oliver Neukum  writes:
>> >> > The API to user space. That is the point. We cannot break user space.
>> >> > Once this sysfs API is upstream we are stuck with it.
>> >> 
>> >> yeah, in fact I have been wondering if sysfs is the best interface to
>> >
>> > That is the discussion we must have.
>> >
>> >> userspace. I talked with Heikki a few days back about this; I was
>> >> wondering if something like what the NFC folks did with netlink would be
>> >> better here.
>> >
>> > I doubt that, because the main user is likely to be udev scripts.
>> 
>> I'm entirely sure about this. I think it's not too far-fetched to think
>
> What exactly are you sure about about?

heh, missed a NOT there :-)

>> that, eventually, we will need an actual stack exposed for the CC pin.
>
> The raw CC pin? What about the timing requirement?

well, not the _raw_ CC pin, but a situation where the microcontroller
handling CC pin is "dumb", in the sense that it provides an interface
for us to request/start arbitrary transactions. That will, in turn,
shift the actual bits on the CC pin. Or something along these lines.

An example would be the alternate mode thing. CC microcontroller doesn't
have to know what are the available alternate modes, but it needs to be
able to tell processor what request is coming from the other end.

I guess what I'm trying to say is that CC microcontroller might not be
the one controlling the multiplexer which switches USB pins to another
function. IOW:

Change to alternate mode X message
 CC microcontroller interrupts CPU
  read status to get X
   change multiplexer

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Heikki Krogerus  writes:
> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> >> > Hi,
> >> > 
> >> > Oliver Neukum  writes:
> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> >> 
> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
> >> > >> > is to be supported, it needs to be defined now.
> >> > >> 
> >> > >> When you say API, do you mean the API the class provides to the
> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
> >> > >
> >> > > The API to user space. That is the point. We cannot break user space.
> >> > > Once this sysfs API is upstream we are stuck with it.
> >> > 
> >> > yeah, in fact I have been wondering if sysfs is the best interface to
> >> 
> >> That is the discussion we must have.
> >> 
> >> > userspace. I talked with Heikki a few days back about this; I was
> >> > wondering if something like what the NFC folks did with netlink would be
> >> > better here.
> >> 
> >> I doubt that, because the main user is likely to be udev scripts.
> >> They can easily deal with sysfs attributes.
> >
> > IMHO for high level interface like this, sysfs is ideal because of the
> > simple fact that you only need a shell to access the files. netlink
> > would make us depend on custom software, no?
> >
> > I'm not against using netlink, but what would be the benefit from it
> > in this case?
> 
> With HW we see nowadays, CC stack is hidden on some microcontroller, but
> is it too far-fetched to consider a system where this is not the case ?

There already are several USB PD stacks out there, like also Greg
pointed out.

> Specially when we consider things like power delivery which, I know, you
> wanted to keep it out of this interface, however we would have two
> 'stacks' competing for access to the same pins, right ?

No. This class would be the top layer for the coming stack, where ever
it ends up coming. The class is only the interface to the user space
and nothing else.

By saying we need to keep USB Type-C separate from USB PD I meant that
the userspace access can not be mixed somewhere in layers of the USB
PD/CC stack like it has been in the USB PD stacks I've seen so far.
They assume that we always use the software USB PD stack with USB
Type-C, which as we can see is not true when the stack is implemented
in EC or firmware or some complex USB PD controller or what ever.
However, the operations the userspace needs to do are exactly the same
in both cases.

- data role swapping
- power role swapping (depends on USB PD)
- Alternate Modes (depends on USB PD)

And we really should not forget that we actually also have USB Type-C
PHYs that can't do any USB PD communication over the CC pin, so USB PD
is simply not always going to be available. But the data role swapping
and also accessories are still available with them, as the do not need
USB PD.

This was the whole point with the class. It allows the different ways
of dealing with Type-C ports to be exposed to userspace in the same
way.

> IIRC mode and role negotiation goes via CC pins using the power delivery
> protocol. If I misunderstand anything, let me know.

The data role swap with USB Type-C connectors is in no way tied to USB
Power Delivery. The USB Type-C spec defines that when USB PD is
available, DR_Swap USB PD function is used to swap the role, otherwise
emulated disconnect will do the trick.

Data role swapping is a must thing to have with USB Type-C connectors
because of the fact that the role is selected randomly. Regardless was
USB PD supported or not.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Heikki Krogerus  writes:
> > On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
> >> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> >> > Hi,
> >> > 
> >> > Oliver Neukum  writes:
> >> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> >> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> >> 
> >> > >> > Yes, but we need an API. We can't keep adding to it. So if that
> >> > >> > is to be supported, it needs to be defined now.
> >> > >> 
> >> > >> When you say API, do you mean the API the class provides to the
> >> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
> >> > >
> >> > > The API to user space. That is the point. We cannot break user space.
> >> > > Once this sysfs API is upstream we are stuck with it.
> >> > 
> >> > yeah, in fact I have been wondering if sysfs is the best interface to
> >> 
> >> That is the discussion we must have.
> >> 
> >> > userspace. I talked with Heikki a few days back about this; I was
> >> > wondering if something like what the NFC folks did with netlink would be
> >> > better here.
> >> 
> >> I doubt that, because the main user is likely to be udev scripts.
> >> They can easily deal with sysfs attributes.
> >
> > IMHO for high level interface like this, sysfs is ideal because of the
> > simple fact that you only need a shell to access the files. netlink
> > would make us depend on custom software, no?
> >
> > I'm not against using netlink, but what would be the benefit from it
> > in this case?
> 
> With HW we see nowadays, CC stack is hidden on some microcontroller, but
> is it too far-fetched to consider a system where this is not the case ?

There already are several USB PD stacks out there, like also Greg
pointed out.

> Specially when we consider things like power delivery which, I know, you
> wanted to keep it out of this interface, however we would have two
> 'stacks' competing for access to the same pins, right ?

No. This class would be the top layer for the coming stack, where ever
it ends up coming. The class is only the interface to the user space
and nothing else.

By saying we need to keep USB Type-C separate from USB PD I meant that
the userspace access can not be mixed somewhere in layers of the USB
PD/CC stack like it has been in the USB PD stacks I've seen so far.
They assume that we always use the software USB PD stack with USB
Type-C, which as we can see is not true when the stack is implemented
in EC or firmware or some complex USB PD controller or what ever.
However, the operations the userspace needs to do are exactly the same
in both cases.

- data role swapping
- power role swapping (depends on USB PD)
- Alternate Modes (depends on USB PD)

And we really should not forget that we actually also have USB Type-C
PHYs that can't do any USB PD communication over the CC pin, so USB PD
is simply not always going to be available. But the data role swapping
and also accessories are still available with them, as the do not need
USB PD.

This was the whole point with the class. It allows the different ways
of dealing with Type-C ports to be exposed to userspace in the same
way.

> IIRC mode and role negotiation goes via CC pins using the power delivery
> protocol. If I misunderstand anything, let me know.

The data role swap with USB Type-C connectors is in no way tied to USB
Power Delivery. The USB Type-C spec defines that when USB PD is
available, DR_Swap USB PD function is used to swap the role, otherwise
emulated disconnect will do the trick.

Data role swapping is a must thing to have with USB Type-C connectors
because of the fact that the role is selected randomly. Regardless was
USB PD supported or not.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-17 at 15:34 +0200, Felipe Balbi wrote:
> Hi,
> 
> Oliver Neukum  writes:
> >> > The API to user space. That is the point. We cannot break user space.
> >> > Once this sysfs API is upstream we are stuck with it.
> >> 
> >> yeah, in fact I have been wondering if sysfs is the best interface to
> >
> > That is the discussion we must have.
> >
> >> userspace. I talked with Heikki a few days back about this; I was
> >> wondering if something like what the NFC folks did with netlink would be
> >> better here.
> >
> > I doubt that, because the main user is likely to be udev scripts.
> 
> I'm entirely sure about this. I think it's not too far-fetched to think

What exactly are you sure about about?

> that, eventually, we will need an actual stack exposed for the CC pin.

The raw CC pin? What about the timing requirement?

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-17 at 15:34 +0200, Felipe Balbi wrote:
> Hi,
> 
> Oliver Neukum  writes:
> >> > The API to user space. That is the point. We cannot break user space.
> >> > Once this sysfs API is upstream we are stuck with it.
> >> 
> >> yeah, in fact I have been wondering if sysfs is the best interface to
> >
> > That is the discussion we must have.
> >
> >> userspace. I talked with Heikki a few days back about this; I was
> >> wondering if something like what the NFC folks did with netlink would be
> >> better here.
> >
> > I doubt that, because the main user is likely to be udev scripts.
> 
> I'm entirely sure about this. I think it's not too far-fetched to think

What exactly are you sure about about?

> that, eventually, we will need an actual stack exposed for the CC pin.

The raw CC pin? What about the timing requirement?

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Heikki Krogerus  writes:
> On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
>> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
>> > Hi,
>> > 
>> > Oliver Neukum  writes:
>> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> 
>> > >> > Yes, but we need an API. We can't keep adding to it. So if that
>> > >> > is to be supported, it needs to be defined now.
>> > >> 
>> > >> When you say API, do you mean the API the class provides to the
>> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
>> > >
>> > > The API to user space. That is the point. We cannot break user space.
>> > > Once this sysfs API is upstream we are stuck with it.
>> > 
>> > yeah, in fact I have been wondering if sysfs is the best interface to
>> 
>> That is the discussion we must have.
>> 
>> > userspace. I talked with Heikki a few days back about this; I was
>> > wondering if something like what the NFC folks did with netlink would be
>> > better here.
>> 
>> I doubt that, because the main user is likely to be udev scripts.
>> They can easily deal with sysfs attributes.
>
> IMHO for high level interface like this, sysfs is ideal because of the
> simple fact that you only need a shell to access the files. netlink
> would make us depend on custom software, no?
>
> I'm not against using netlink, but what would be the benefit from it
> in this case?

With HW we see nowadays, CC stack is hidden on some microcontroller, but
is it too far-fetched to consider a system where this is not the case ?

Specially when we consider things like power delivery which, I know, you
wanted to keep it out of this interface, however we would have two
'stacks' competing for access to the same pins, right ?

IIRC mode and role negotiation goes via CC pins using the power delivery
protocol. If I misunderstand anything, let me know.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Heikki Krogerus  writes:
> On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
>> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
>> > Hi,
>> > 
>> > Oliver Neukum  writes:
>> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> 
>> > >> > Yes, but we need an API. We can't keep adding to it. So if that
>> > >> > is to be supported, it needs to be defined now.
>> > >> 
>> > >> When you say API, do you mean the API the class provides to the
>> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
>> > >
>> > > The API to user space. That is the point. We cannot break user space.
>> > > Once this sysfs API is upstream we are stuck with it.
>> > 
>> > yeah, in fact I have been wondering if sysfs is the best interface to
>> 
>> That is the discussion we must have.
>> 
>> > userspace. I talked with Heikki a few days back about this; I was
>> > wondering if something like what the NFC folks did with netlink would be
>> > better here.
>> 
>> I doubt that, because the main user is likely to be udev scripts.
>> They can easily deal with sysfs attributes.
>
> IMHO for high level interface like this, sysfs is ideal because of the
> simple fact that you only need a shell to access the files. netlink
> would make us depend on custom software, no?
>
> I'm not against using netlink, but what would be the benefit from it
> in this case?

With HW we see nowadays, CC stack is hidden on some microcontroller, but
is it too far-fetched to consider a system where this is not the case ?

Specially when we consider things like power delivery which, I know, you
wanted to keep it out of this interface, however we would have two
'stacks' competing for access to the same pins, right ?

IIRC mode and role negotiation goes via CC pins using the power delivery
protocol. If I misunderstand anything, let me know.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Oliver Neukum  writes:
>> > The API to user space. That is the point. We cannot break user space.
>> > Once this sysfs API is upstream we are stuck with it.
>> 
>> yeah, in fact I have been wondering if sysfs is the best interface to
>
> That is the discussion we must have.
>
>> userspace. I talked with Heikki a few days back about this; I was
>> wondering if something like what the NFC folks did with netlink would be
>> better here.
>
> I doubt that, because the main user is likely to be udev scripts.

I'm entirely sure about this. I think it's not too far-fetched to think
that, eventually, we will need an actual stack exposed for the CC pin.

> They can easily deal with sysfs attributes.

right

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Oliver Neukum  writes:
>> > The API to user space. That is the point. We cannot break user space.
>> > Once this sysfs API is upstream we are stuck with it.
>> 
>> yeah, in fact I have been wondering if sysfs is the best interface to
>
> That is the discussion we must have.
>
>> userspace. I talked with Heikki a few days back about this; I was
>> wondering if something like what the NFC folks did with netlink would be
>> better here.
>
> I doubt that, because the main user is likely to be udev scripts.

I'm entirely sure about this. I think it's not too far-fetched to think
that, eventually, we will need an actual stack exposed for the CC pin.

> They can easily deal with sysfs attributes.

right

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> > Hi,
> > 
> > Oliver Neukum  writes:
> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> 
> > >> > Yes, but we need an API. We can't keep adding to it. So if that
> > >> > is to be supported, it needs to be defined now.
> > >> 
> > >> When you say API, do you mean the API the class provides to the
> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
> > >
> > > The API to user space. That is the point. We cannot break user space.
> > > Once this sysfs API is upstream we are stuck with it.
> > 
> > yeah, in fact I have been wondering if sysfs is the best interface to
> 
> That is the discussion we must have.
> 
> > userspace. I talked with Heikki a few days back about this; I was
> > wondering if something like what the NFC folks did with netlink would be
> > better here.
> 
> I doubt that, because the main user is likely to be udev scripts.
> They can easily deal with sysfs attributes.

IMHO for high level interface like this, sysfs is ideal because of the
simple fact that you only need a shell to access the files. netlink
would make us depend on custom software, no?

I'm not against using netlink, but what would be the benefit from it
in this case?


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 17, 2016 at 11:36:52AM +0100, Oliver Neukum wrote:
> On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> > Hi,
> > 
> > Oliver Neukum  writes:
> > > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> > >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> 
> > >> > Yes, but we need an API. We can't keep adding to it. So if that
> > >> > is to be supported, it needs to be defined now.
> > >> 
> > >> When you say API, do you mean the API the class provides to the
> > >> drivers? Or did you mean ABI which would be the sysfs in this case?
> > >
> > > The API to user space. That is the point. We cannot break user space.
> > > Once this sysfs API is upstream we are stuck with it.
> > 
> > yeah, in fact I have been wondering if sysfs is the best interface to
> 
> That is the discussion we must have.
> 
> > userspace. I talked with Heikki a few days back about this; I was
> > wondering if something like what the NFC folks did with netlink would be
> > better here.
> 
> I doubt that, because the main user is likely to be udev scripts.
> They can easily deal with sysfs attributes.

IMHO for high level interface like this, sysfs is ideal because of the
simple fact that you only need a shell to access the files. netlink
would make us depend on custom software, no?

I'm not against using netlink, but what would be the benefit from it
in this case?


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> Hi,
> 
> Oliver Neukum  writes:
> > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:

> >> > Yes, but we need an API. We can't keep adding to it. So if that
> >> > is to be supported, it needs to be defined now.
> >> 
> >> When you say API, do you mean the API the class provides to the
> >> drivers? Or did you mean ABI which would be the sysfs in this case?
> >
> > The API to user space. That is the point. We cannot break user space.
> > Once this sysfs API is upstream we are stuck with it.
> 
> yeah, in fact I have been wondering if sysfs is the best interface to

That is the discussion we must have.

> userspace. I talked with Heikki a few days back about this; I was
> wondering if something like what the NFC folks did with netlink would be
> better here.

I doubt that, because the main user is likely to be udev scripts.
They can easily deal with sysfs attributes.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-17 at 12:29 +0200, Felipe Balbi wrote:
> Hi,
> 
> Oliver Neukum  writes:
> > On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> >> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:

> >> > Yes, but we need an API. We can't keep adding to it. So if that
> >> > is to be supported, it needs to be defined now.
> >> 
> >> When you say API, do you mean the API the class provides to the
> >> drivers? Or did you mean ABI which would be the sysfs in this case?
> >
> > The API to user space. That is the point. We cannot break user space.
> > Once this sysfs API is upstream we are stuck with it.
> 
> yeah, in fact I have been wondering if sysfs is the best interface to

That is the discussion we must have.

> userspace. I talked with Heikki a few days back about this; I was
> wondering if something like what the NFC folks did with netlink would be
> better here.

I doubt that, because the main user is likely to be udev scripts.
They can easily deal with sysfs attributes.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Oliver Neukum  writes:
> On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> > On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
>> > > > That question has not been answered. It would be awkward for the OS
>> > > > to find itself in the slave role, which it is ill equipped for. So
>> > > > the data role should be switched before the new device is announced
>> > > > to user space. How is that handled?
>> > > 
>> > > In the class driver, once we add support for preselecting the role,
>> > > when the connection happens we compare the initial role to the
>> > > preselected one and execute swap if it differs. Only after that we
>> > > notify userspace.
>> > 
>> > Yes, but we need an API. We can't keep adding to it. So if that
>> > is to be supported, it needs to be defined now.
>> 
>> When you say API, do you mean the API the class provides to the
>> drivers? Or did you mean ABI which would be the sysfs in this case?
>
> The API to user space. That is the point. We cannot break user space.
> Once this sysfs API is upstream we are stuck with it.

yeah, in fact I have been wondering if sysfs is the best interface to
userspace. I talked with Heikki a few days back about this; I was
wondering if something like what the NFC folks did with netlink would be
better here.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Felipe Balbi]

Hi,

Oliver Neukum  writes:
> On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
>> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
>> > On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
>> > > > That question has not been answered. It would be awkward for the OS
>> > > > to find itself in the slave role, which it is ill equipped for. So
>> > > > the data role should be switched before the new device is announced
>> > > > to user space. How is that handled?
>> > > 
>> > > In the class driver, once we add support for preselecting the role,
>> > > when the connection happens we compare the initial role to the
>> > > preselected one and execute swap if it differs. Only after that we
>> > > notify userspace.
>> > 
>> > Yes, but we need an API. We can't keep adding to it. So if that
>> > is to be supported, it needs to be defined now.
>> 
>> When you say API, do you mean the API the class provides to the
>> drivers? Or did you mean ABI which would be the sysfs in this case?
>
> The API to user space. That is the point. We cannot break user space.
> Once this sysfs API is upstream we are stuck with it.

yeah, in fact I have been wondering if sysfs is the best interface to
userspace. I talked with Heikki a few days back about this; I was
wondering if something like what the NFC folks did with netlink would be
better here.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> > On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
> > > > That question has not been answered. It would be awkward for the OS
> > > > to find itself in the slave role, which it is ill equipped for. So
> > > > the data role should be switched before the new device is announced
> > > > to user space. How is that handled?
> > > 
> > > In the class driver, once we add support for preselecting the role,
> > > when the connection happens we compare the initial role to the
> > > preselected one and execute swap if it differs. Only after that we
> > > notify userspace.
> > 
> > Yes, but we need an API. We can't keep adding to it. So if that
> > is to be supported, it needs to be defined now.
> 
> When you say API, do you mean the API the class provides to the
> drivers? Or did you mean ABI which would be the sysfs in this case?

The API to user space. That is the point. We cannot break user space.
Once this sysfs API is upstream we are stuck with it.

> For the sysfs I would image we can manage with the current files,
> current_data_role and current_power_role. If somebody writes to them
> when we are disconnected, we still callback the dr_swap or pr_swap
> hooks, and make a rule that when disconnected, it means we are
> setting the "preferred" roles.
> 
> Would that be OK? Or did I still misunderstood your question?

That would be absolutely OK, but that function needs to be decided on
and documented as all files in sysfs are. And likewise it needs to be
documented that alternate modes behave differently.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-17 at 09:58 +0200, Heikki Krogerus wrote:
> On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> > On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
> > > > That question has not been answered. It would be awkward for the OS
> > > > to find itself in the slave role, which it is ill equipped for. So
> > > > the data role should be switched before the new device is announced
> > > > to user space. How is that handled?
> > > 
> > > In the class driver, once we add support for preselecting the role,
> > > when the connection happens we compare the initial role to the
> > > preselected one and execute swap if it differs. Only after that we
> > > notify userspace.
> > 
> > Yes, but we need an API. We can't keep adding to it. So if that
> > is to be supported, it needs to be defined now.
> 
> When you say API, do you mean the API the class provides to the
> drivers? Or did you mean ABI which would be the sysfs in this case?

The API to user space. That is the point. We cannot break user space.
Once this sysfs API is upstream we are stuck with it.

> For the sysfs I would image we can manage with the current files,
> current_data_role and current_power_role. If somebody writes to them
> when we are disconnected, we still callback the dr_swap or pr_swap
> hooks, and make a rule that when disconnected, it means we are
> setting the "preferred" roles.
> 
> Would that be OK? Or did I still misunderstood your question?

That would be absolutely OK, but that function needs to be decided on
and documented as all files in sysfs are. And likewise it needs to be
documented that alternate modes behave differently.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
> > > That question has not been answered. It would be awkward for the OS
> > > to find itself in the slave role, which it is ill equipped for. So
> > > the data role should be switched before the new device is announced
> > > to user space. How is that handled?
> > 
> > In the class driver, once we add support for preselecting the role,
> > when the connection happens we compare the initial role to the
> > preselected one and execute swap if it differs. Only after that we
> > notify userspace.
> 
> Yes, but we need an API. We can't keep adding to it. So if that
> is to be supported, it needs to be defined now.

When you say API, do you mean the API the class provides to the
drivers? Or did you mean ABI which would be the sysfs in this case?

For the sysfs I would image we can manage with the current files,
current_data_role and current_power_role. If somebody writes to them
when we are disconnected, we still callback the dr_swap or pr_swap
hooks, and make a rule that when disconnected, it means we are
setting the "preferred" roles.

Would that be OK? Or did I still misunderstood your question?


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Tue, Feb 16, 2016 at 02:39:47PM +0100, Oliver Neukum wrote:
> On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
> > > That question has not been answered. It would be awkward for the OS
> > > to find itself in the slave role, which it is ill equipped for. So
> > > the data role should be switched before the new device is announced
> > > to user space. How is that handled?
> > 
> > In the class driver, once we add support for preselecting the role,
> > when the connection happens we compare the initial role to the
> > preselected one and execute swap if it differs. Only after that we
> > notify userspace.
> 
> Yes, but we need an API. We can't keep adding to it. So if that
> is to be supported, it needs to be defined now.

When you say API, do you mean the API the class provides to the
drivers? Or did you mean ABI which would be the sysfs in this case?

For the sysfs I would image we can manage with the current files,
current_data_role and current_power_role. If somebody writes to them
when we are disconnected, we still callback the dr_swap or pr_swap
hooks, and make a rule that when disconnected, it means we are
setting the "preferred" roles.

Would that be OK? Or did I still misunderstood your question?


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
> > That question has not been answered. It would be awkward for the OS
> > to find itself in the slave role, which it is ill equipped for. So
> > the data role should be switched before the new device is announced
> > to user space. How is that handled?
> 
> In the class driver, once we add support for preselecting the role,
> when the connection happens we compare the initial role to the
> preselected one and execute swap if it differs. Only after that we
> notify userspace.

Yes, but we need an API. We can't keep adding to it. So if that
is to be supported, it needs to be defined now.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Tue, 2016-02-16 at 11:22 +0200, Heikki Krogerus wrote:
> > That question has not been answered. It would be awkward for the OS
> > to find itself in the slave role, which it is ill equipped for. So
> > the data role should be switched before the new device is announced
> > to user space. How is that handled?
> 
> In the class driver, once we add support for preselecting the role,
> when the connection happens we compare the initial role to the
> preselected one and execute swap if it differs. Only after that we
> notify userspace.

Yes, but we need an API. We can't keep adding to it. So if that
is to be supported, it needs to be defined now.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

Hi,

On Mon, Feb 15, 2016 at 04:30:18PM +0100, Oliver Neukum wrote:
> On Thu, 2016-02-11 at 15:50 +0200, Heikki Krogerus wrote:
> > Because USB Type-C ports (DRP ones) will select the data role randomly
> > when you connect (to an other DRP port). USB Type-C spec defines that
> > you can "prefer" host mode, but when both ends prefer host mode, it's
> > +-0.
> 
> That question has not been answered. It would be awkward for the OS
> to find itself in the slave role, which it is ill equipped for. So
> the data role should be switched before the new device is announced
> to user space. How is that handled?

In the class driver, once we add support for preselecting the role,
when the connection happens we compare the initial role to the
preselected one and execute swap if it differs. Only after that we
notify userspace.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

Hi,

On Mon, Feb 15, 2016 at 04:30:18PM +0100, Oliver Neukum wrote:
> On Thu, 2016-02-11 at 15:50 +0200, Heikki Krogerus wrote:
> > Because USB Type-C ports (DRP ones) will select the data role randomly
> > when you connect (to an other DRP port). USB Type-C spec defines that
> > you can "prefer" host mode, but when both ends prefer host mode, it's
> > +-0.
> 
> That question has not been answered. It would be awkward for the OS
> to find itself in the slave role, which it is ill equipped for. So
> the data role should be switched before the new device is announced
> to user space. How is that handled?

In the class driver, once we add support for preselecting the role,
when the connection happens we compare the initial role to the
preselected one and execute swap if it differs. Only after that we
notify userspace.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-11 at 15:50 +0200, Heikki Krogerus wrote:
> Because USB Type-C ports (DRP ones) will select the data role randomly
> when you connect (to an other DRP port). USB Type-C spec defines that
> you can "prefer" host mode, but when both ends prefer host mode, it's
> +-0.

That question has not been answered. It would be awkward for the OS
to find itself in the slave role, which it is ill equipped for. So
the data role should be switched before the new device is announced
to user space. How is that handled?

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Thu, 2016-02-11 at 15:50 +0200, Heikki Krogerus wrote:
> Because USB Type-C ports (DRP ones) will select the data role randomly
> when you connect (to an other DRP port). USB Type-C spec defines that
> you can "prefer" host mode, but when both ends prefer host mode, it's
> +-0.

That question has not been answered. It would be awkward for the OS
to find itself in the slave role, which it is ill equipped for. So
the data role should be switched before the new device is announced
to user space. How is that handled?

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 10, 2016 at 02:04:07PM +0100, Oliver Neukum wrote:
> On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> > +#define UCSI_CONSTAT_BC_NOT_CHARGING   0
> > +#define UCSI_CONSTAT_BC_NOMINAL_CHARGING   1
> > +#define UCSI_CONSTAT_BC_SLOW_CHARGING  2
> > +#define UCSI_CONSTAT_BC_TRICLE_CHARGING3
> 
> typo. It is spelled TRICKLE

Thanks! I'll fix it.

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

Hi Greg,

> > But surely everybody agrees that decision about the policies regarding
> > USB Type-C ports, like which data role to use, do we charge or are we
> > letting the other end charge, etc., belongs to the user?
> 
> No, I don't agree.  It's still unknown if userspace can react fast
> though to these types of "policy" changes.  I've heard from some
> manufacturers that the response time needed is something that we can't
> leave to userspace.

There are no restrictions on when role swapping could to be executed
or when an alt mode can be entered after the connection is made and
an initial role and mode are set. The timing constraints these guys
are most likely talking about are related to the USB PD functions that
need to be executed, for example DR_Swap when the data role swap is
requested and so on. But those are a problem for the drivers that
implement the dr_swap, pr_swap and set_alt_mode, or more likely the
PD stack, and indeed happen inside kernel.

This is probable just a misunderstand. I'm not talking about USB PD
Policy, Device Manager, System Policy Manager or anything else USB PD
spec defines. Those things will indeed happen inside kernel.

My little class is just a high level interface that allows userspace
to request kernel to do things which then end up being executed inside
kernel. There really should not be any problem here.

> And along those lines, do you have a working userspace user of this
> interface?  We don't create interfaces without a user, especially given
> that it takes a long time to ensure that a user/kernel api actually is
> correct.  We would need to see that to ensure that this kernel
> implementation is "correct" and working properly.

No users (well, let me get back on this). I want to force peoples hand
with this early because, if we exclude details about the cable, which
I don't see of any interest to the userspace, the functions and
features USB Type-C spec defines are what I'm presenting, and that's
it. Unless newer versions of USB Type-C connectors bring something
different to the table, the interface is solid. We just need to fine
tune it, agree on what are proper names for the files, etc.

There is just one function that USB Type-C spec has defined that I
have left out of the interface. That is VCONN swapping. I left it out
on purpose as it is cable specific, but I'm now thinking about adding
that as well. It's not like you have to use it, so why not.

> > If you plug
> > your phone to your desktop, I would imagine that you want to see the
> > phone primarily as the USB device and the desktop as host, and to
> > achieve the device role, you don't want to be forced to unplug/replug
> > your phone from the desktop until you achieve device role, right?
> 
> Why is unplugging somehow required?

Because USB Type-C ports (DRP ones) will select the data role randomly
when you connect (to an other DRP port). USB Type-C spec defines that
you can "prefer" host mode, but when both ends prefer host mode, it's
+-0.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Bjørn Mork]

Andy Shevchenko  writes:

> To me out: sounds out, like printing error and return code, or
> something like that. Here the case is different.
>
> And since we have two full switches it might be hard to have on one
> screen out code and exact goto. See my point now?

No, sorry.  Not really.

The pattern

   out:
  return whatever;

is so common in the kernel that I cannot see any point starting a
discussion about it:

 bjorn@nemi:/usr/local/src/git/linux$ git grep -A1 ^out:|grep return|wc -l
 3622


And I also fail to see any relation between the explanation you come up
with here and the text in CodingStyle, which was what you initially
referred to.  There is no specific interpreation of the label name "out"
there AFAICS.

I apologize if you think I am being an ass now. I am.  I'm just too fed
up with coding style arguments with no other reason than a vague pointer
to some document.  If you're going to point out coding style problems,
then you could at least make an effort explaining why a style change
improves the code.  My bet is that most of the time you'll find that the
comment is unnecessary...



Bjørn

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 5:11 PM, Bjørn Mork  wrote:
> Andy Shevchenko  writes:
>> On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum  wrote:
>>> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
 > +out:

 CodingStyle suggests to do a better label naming.

[...]

> Exactly what is it about "out" that is unclear to you here?  Could you
> propose a better alternative if you seriously mean that this needs to be
> changed?

Those switches of course an extra stuff, but I would propose
out_connect:

To me out: sounds out, like printing error and return code, or
something like that. Here the case is different.

And since we have two full switches it might be hard to have on one
screen out code and exact goto. See my point now?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 5:11 PM, Bjørn Mork  wrote:
> Andy Shevchenko  writes:
>> On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum  wrote:
>>> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
 > +out:

 CodingStyle suggests to do a better label naming.

[...]

> Exactly what is it about "out" that is unclear to you here?  Could you
> propose a better alternative if you seriously mean that this needs to be
> changed?

Those switches of course an extra stuff, but I would propose
out_connect:

To me out: sounds out, like printing error and return code, or
something like that. Here the case is different.

And since we have two full switches it might be hard to have on one
screen out code and exact goto. See my point now?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Bjørn Mork]

Andy Shevchenko  writes:

> To me out: sounds out, like printing error and return code, or
> something like that. Here the case is different.
>
> And since we have two full switches it might be hard to have on one
> screen out code and exact goto. See my point now?

No, sorry.  Not really.

The pattern

   out:
  return whatever;

is so common in the kernel that I cannot see any point starting a
discussion about it:

 bjorn@nemi:/usr/local/src/git/linux$ git grep -A1 ^out:|grep return|wc -l
 3622


And I also fail to see any relation between the explanation you come up
with here and the text in CodingStyle, which was what you initially
referred to.  There is no specific interpreation of the label name "out"
there AFAICS.

I apologize if you think I am being an ass now. I am.  I'm just too fed
up with coding style arguments with no other reason than a vague pointer
to some document.  If you're going to point out coding style problems,
then you could at least make an effort explaining why a style change
improves the code.  My bet is that most of the time you'll find that the
comment is unnecessary...



Bjørn

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

Hi Greg,

> > But surely everybody agrees that decision about the policies regarding
> > USB Type-C ports, like which data role to use, do we charge or are we
> > letting the other end charge, etc., belongs to the user?
> 
> No, I don't agree.  It's still unknown if userspace can react fast
> though to these types of "policy" changes.  I've heard from some
> manufacturers that the response time needed is something that we can't
> leave to userspace.

There are no restrictions on when role swapping could to be executed
or when an alt mode can be entered after the connection is made and
an initial role and mode are set. The timing constraints these guys
are most likely talking about are related to the USB PD functions that
need to be executed, for example DR_Swap when the data role swap is
requested and so on. But those are a problem for the drivers that
implement the dr_swap, pr_swap and set_alt_mode, or more likely the
PD stack, and indeed happen inside kernel.

This is probable just a misunderstand. I'm not talking about USB PD
Policy, Device Manager, System Policy Manager or anything else USB PD
spec defines. Those things will indeed happen inside kernel.

My little class is just a high level interface that allows userspace
to request kernel to do things which then end up being executed inside
kernel. There really should not be any problem here.

> And along those lines, do you have a working userspace user of this
> interface?  We don't create interfaces without a user, especially given
> that it takes a long time to ensure that a user/kernel api actually is
> correct.  We would need to see that to ensure that this kernel
> implementation is "correct" and working properly.

No users (well, let me get back on this). I want to force peoples hand
with this early because, if we exclude details about the cable, which
I don't see of any interest to the userspace, the functions and
features USB Type-C spec defines are what I'm presenting, and that's
it. Unless newer versions of USB Type-C connectors bring something
different to the table, the interface is solid. We just need to fine
tune it, agree on what are proper names for the files, etc.

There is just one function that USB Type-C spec has defined that I
have left out of the interface. That is VCONN swapping. I left it out
on purpose as it is cable specific, but I'm now thinking about adding
that as well. It's not like you have to use it, so why not.

> > If you plug
> > your phone to your desktop, I would imagine that you want to see the
> > phone primarily as the USB device and the desktop as host, and to
> > achieve the device role, you don't want to be forced to unplug/replug
> > your phone from the desktop until you achieve device role, right?
> 
> Why is unplugging somehow required?

Because USB Type-C ports (DRP ones) will select the data role randomly
when you connect (to an other DRP port). USB Type-C spec defines that
you can "prefer" host mode, but when both ends prefer host mode, it's
+-0.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 10, 2016 at 02:04:07PM +0100, Oliver Neukum wrote:
> On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> > +#define UCSI_CONSTAT_BC_NOT_CHARGING   0
> > +#define UCSI_CONSTAT_BC_NOMINAL_CHARGING   1
> > +#define UCSI_CONSTAT_BC_SLOW_CHARGING  2
> > +#define UCSI_CONSTAT_BC_TRICLE_CHARGING3
> 
> typo. It is spelled TRICKLE

Thanks! I'll fix it.

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Greg KH]

On Wed, Feb 10, 2016 at 12:30:42PM +0200, Heikki Krogerus wrote:
> On Tue, Feb 09, 2016 at 10:21:55AM -0800, Greg KH wrote:
> > On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> > > USB Type-C Connector System Software Interface (UCSI) is a
> > > specification that defines registers and data structures
> > > used to interface with the USB Type-C connectors on a system.
> > > 
> > > The specification is public and available at:
> > > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> > > 
> > 
> > What does this driver / code actually do?  Why is it needed?  What
> > interface to the rest of the kernel / userspace does it provide?
> 
> I will fix this describe these things in the commit message. I'll just
> but some UCSI background in case somebody is interested. So UCSI is in
> practice a standard for USB Type-C controllers..
> 
> UCSI is the control interface for USB Type-C connectors (regardless
> was USB PD supported or not) in MS Windows, so most likely all new HW
> platforms designed to work also with Windows that are equipped with
> USB Type-C will have UCSI device for controlling the USB Type-C ports.

There's many millions of devices with type-C without Windows on them, so
don't count on this being everywhere :)

> In some cases the hardware for Type-C will be just a PHY like fusb30x
> on these platforms (it's cheaper then USB PD or complete USB Type-C
> controller), but in those cases the PHY is probable attached to an EC
> or is completely controlled by system FW like BIOS together with any
> USB PD communication in cases where USB PD is supported, and is in any
> case not visible to the OS. Instead UCSI device is exposed to the OS
> to give it means to apply its policies to the USB Type-C port.
> 
> > Why would we care about this?
> 
> I'll try to explain why it's important to export the control of USB
> Type-C ports to the user space in my answer to your comments to the
> first patch of this series, the one introducing the class.
> 
> But surely everybody agrees that decision about the policies regarding
> USB Type-C ports, like which data role to use, do we charge or are we
> letting the other end charge, etc., belongs to the user?

No, I don't agree.  It's still unknown if userspace can react fast
though to these types of "policy" changes.  I've heard from some
manufacturers that the response time needed is something that we can't
leave to userspace.

And along those lines, do you have a working userspace user of this
interface?  We don't create interfaces without a user, especially given
that it takes a long time to ensure that a user/kernel api actually is
correct.  We would need to see that to ensure that this kernel
implementation is "correct" and working properly.

> If you plug
> your phone to your desktop, I would imagine that you want to see the
> phone primarily as the USB device and the desktop as host, and to
> achieve the device role, you don't want to be forced to unplug/replug
> your phone from the desktop until you achieve device role, right?

Why is unplugging somehow required?

thanks,

greg k-h

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Bjørn Mork]

Andy Shevchenko  writes:
> On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum  wrote:
>> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>>> > +out:
>>>
>>> CodingStyle suggests to do a better label naming.
>>
>> Names coming from specs are what they are. There is
>> no place for coding style here.
>
> Yes, and how is it related to C label names?

It did appear as if you were commenting on the case labels since you
quoted two full switch blocks.  That's how I read your comment as well.

It's now clear that you somehow mean than "out:" is in conflict with
CodingStyle.  It is still very unclear how, and it does not seem like
you intend to make it any clearer since you did not take the opportunity
to explain yourself.

FWIW, I read the CodingStyle recommendation as: use descriptive labels
instead of "foo1", "foo2" etc, where "foo" is typically "err".  I do not
see this as conflicting with the use of "err" or "out" when there is a
single such label in a function.  The meaning of those labels are very
clear IMHO.

Exactly what is it about "out" that is unclear to you here?  Could you
propose a better alternative if you seriously mean that this needs to be
changed?


Bjørn

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-10 at 16:24 +0200, Andy Shevchenko wrote:
> On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum  wrote:
> > On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> >> > +err:
> >> > +   if (i > 0)
> >> > +   for (; i >= 0; i--, con--)
> >> > +   typec_unregister_port(con->port);
> >>
> >> Perhaps
> >>
> >> while (--i >= 0) {
> >>  ...
> >> }
> >
> > While we are at it. No we should not change the semantics
> > of conditionals for the sake of appearance.
> 
> I'm sorry I didn't get you.
> How this more or less standard pattern to clean up stuff on error path
> does with conditional semantics?

You change a postdecrement to a predecrement. The highest
number the loop is executed for is changed.

Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum  wrote:
> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>> > +err:
>> > +   if (i > 0)
>> > +   for (; i >= 0; i--, con--)
>> > +   typec_unregister_port(con->port);
>>
>> Perhaps
>>
>> while (--i >= 0) {
>>  ...
>> }
>
> While we are at it. No we should not change the semantics
> of conditionals for the sake of appearance.

I'm sorry I didn't get you.
How this more or less standard pattern to clean up stuff on error path
does with conditional semantics?

I also noticed that this code might be factored out to helper which
will do same things (only number of loops is different) in both cases.
What did I miss?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> > +err:
> > +   if (i > 0)
> > +   for (; i >= 0; i--, con--)
> > +   typec_unregister_port(con->port);
> 
> Perhaps
> 
> while (--i >= 0) {
>  ...
> }

While we are at it. No we should not change the semantics
of conditionals for the sake of appearance.

Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum  wrote:
> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>> > +out:
>>
>> CodingStyle suggests to do a better label naming.
>
> Names coming from specs are what they are. There is
> no place for coding style here.

Yes, and how is it related to C label names?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> > +out:
> 
> CodingStyle suggests to do a better label naming.

Names coming from specs are what they are. There is
no place for coding style here.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> +#define UCSI_CONSTAT_BC_NOT_CHARGING   0
> +#define UCSI_CONSTAT_BC_NOMINAL_CHARGING   1
> +#define UCSI_CONSTAT_BC_SLOW_CHARGING  2
> +#define UCSI_CONSTAT_BC_TRICLE_CHARGING3

typo. It is spelled TRICKLE

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 10, 2016 at 12:19:53PM +0100, Oliver Neukum wrote:
> 
> > +static int ucsi_run_cmd(struct ucsi *ucsi, void *data, size_t size)
> > +{
> > +   int status;
> > +   int ret;
> > +
> > +   dev_vdbg(ucsi->dev, "%s control 0x%llx\n", __func__,
> > +ucsi->ppm->data->control);
> > +
> > +   ret = ucsi->ppm->cmd(ucsi->ppm);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* REVISIT: We may need to set UCSI_CCI_CMD_COMPLETE flag here */
> > +   wait_for_completion(>complete);
> > +
> > +   status = ucsi->status;
> > +   if (status != UCSI_ERROR && size)
> > +   memcpy(data, ucsi->ppm->data->message_in, size);
> > +
> > +   ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> > +   if (ret)
> > +   goto out;
> > +
> > +   if (status == UCSI_ERROR) {
> > +   u16 error;
> > +
> > +   ucsi->ppm->data->control = UCSI_GET_ERROR_STATUS;
> > +   ret = ucsi->ppm->cmd(ucsi->ppm);
> > +   if (ret)
> > +   goto out;
> > +
> > +   wait_for_completion(>complete);
> > +
> > +   /* Something has really gone wrong */
> > +   if (ucsi->status == UCSI_ERROR) {
> > +   ret = -ENODEV;
> > +   goto out;
> > +   }
> > +
> > +   memcpy(, ucsi->ppm->data->message_in, sizeof(error));
> > +
> > +   ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> > +   if (ret)
> > +   goto out;
> > +
> > +   switch (error) {
> > +   case UCSI_ERROR_INVALID_CON_NUM:
> > +   ret = -ENXIO;
> > +   break;
> > +   case UCSI_ERROR_INCOMPATIBLE_PARTNER:
> > +   case UCSI_ERROR_CC_COMMUNICATION_ERR:
> > +   case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
> > +   ret = -EIO;
> > +   break;
> 
> This looks like you mapped all the interesting error condition
> on a single error code and gave out other error codes to
> conditions of lesser importance.

OK, I'll fix those.

> > +   case UCSI_ERROR_DEAD_BATTERY:
> > +   dev_warn(ucsi->dev, "Dead Battery Condition!\n");
> > +   ret = -EPERM;
> > +   break;
> > +   case UCSI_ERROR_UNREGONIZED_CMD:
> > +   case UCSI_ERROR_INVALID_CMD_ARGUMENT:
> > +   default:
> > +   ret = -EINVAL;
> > +   break;
> > +   }
> > +   }
> > +out:
> > +   ucsi->ppm->data->control = 0;
> > +   return ret;
> > +}
> 
> [..]
> 
> > +/**
> > + * ucsi_init - Initialize an UCSI Interface
> > + * @ucsi: The UCSI Interface
> > + *
> > + * Registers all the USB Type-C ports governed by the PPM of @ucsi and 
> > enables
> > + * all the notifications from the PPM.
> > + */
> > +int ucsi_init(struct ucsi *ucsi)
> > +{
> > +   struct ucsi_control *ctrl = (void *)>ppm->data->control;
> > +   struct ucsi_connector *con;
> > +   int ret;
> > +   int i;
> > +
> > +   /* Enable basic notifications */
> > +   ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> > +   ctrl->data = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> > +   ret = ucsi_run_cmd(ucsi, NULL, 0);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Get PPM capabilities */
> > +   ctrl->cmd = UCSI_GET_CAPABILITY;
> > +   ret = ucsi_run_cmd(ucsi, >cap, sizeof(ucsi->cap));
> > +   if (ret)
> > +   return ret;
> > +
> > +   ucsi->connector = kcalloc(ucsi->cap.num_connectors,
> > + sizeof(struct ucsi_connector), GFP_KERNEL);
> > +   if (!ucsi->connector)
> > +   return -ENOMEM;
> > +
> > +   for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> > +i++, con++) {
> > +   struct typec_capability *cap = >typec_cap;
> > +   struct ucsi_connector_status constat;
> > +
> > +   /* Get connector capability */
> > +   ctrl->cmd = UCSI_GET_CONNECTOR_CAPABILITY;
> > +   ctrl->data = i + 1;
> > +   ret = ucsi_run_cmd(ucsi, >cap, sizeof(con->cap));
> > +   if (ret)
> > +   goto err;
> > +
> > +   /* Register the connector */
> > +
> > +   if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
> > +   cap->type = TYPEC_PORT_DRP;
> > +   else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP)
> > +   cap->type = TYPEC_PORT_DFP;
> > +   else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP)
> > +   cap->type = TYPEC_PORT_UFP;
> > +
> > +   cap->usb_pd = !!(ucsi->cap.attributes &
> > +  UCSI_CAP_ATTR_USB_PD);
> > +   cap->audio_accessory = !!(con->cap.op_mode &
> > + UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY);
> > +   cap->debug_accessory = !!(con->cap.op_mode &
> > + UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY);
> > +
> > +   /* TODO: Alt modes */
> > +
> > +   cap->dr_swap = ucsi_dr_swap;
> > 

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Tue, Feb 9, 2016 at 7:01 PM, Heikki Krogerus
 wrote:
> USB Type-C Connector System Software Interface (UCSI) is a
> specification that defines registers and data structures
> used to interface with the USB Type-C connectors on a system.
>
> The specification is public and available at:
> http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html

+#define to_ucsi_connector(_port_) container_of(_port_->cap,
 \
+  struct ucsi_connector,  \
+  typec_cap)

Perhaps
static inline ...

+
+#define cci_to_connector(_ucsi_, cci) (_ucsi_->connector +\
+   UCSI_CCI_CONNECTOR_CHANGE(cci) - 1)
+

If you start you macro on the next line it will look better.

> +static int
> +ucsi_connect(struct ucsi_connector *con, struct ucsi_connector_status 
> *constat)
> +{
> +   struct typec_port *port = con->port;
> +
> +   port->connected = true;
> +
> +   if (constat->partner_flags & UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE)
> +   port->partner_type = TYPEC_PARTNER_ALTMODE;
> +   else
> +   port->partner_type = TYPEC_PARTNER_USB;
> +
> +   switch (constat->partner_type) {
> +   case UCSI_CONSTAT_PARTNER_TYPE_CABLE_NO_UFP:
> +   /* REVISIT: We don't care about just the cable for now */
> +   return 0;
> +   case UCSI_CONSTAT_PARTNER_TYPE_DFP:
> +   case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> +   port->pwr_role = TYPEC_PWR_SINK;
> +   port->data_role = TYPEC_DEVICE;
> +   break;
> +   case UCSI_CONSTAT_PARTNER_TYPE_UFP:
> +   port->pwr_role = TYPEC_PWR_SOURCE;
> +   port->data_role = TYPEC_HOST;
> +   break;
> +   case UCSI_CONSTAT_PARTNER_TYPE_DEBUG:
> +   port->partner_type = TYPEC_PARTNER_DEBUG;
> +   goto out;
> +   case UCSI_CONSTAT_PARTNER_TYPE_AUDIO:
> +   port->partner_type = TYPEC_PARTNER_AUDIO;
> +   goto out;
> +   }
> +
> +   switch (constat->pwr_op_mode) {
> +   case UCSI_CONSTAT_PWR_OPMODE_NONE:
> +   case UCSI_CONSTAT_PWR_OPMODE_DEFAULT:
> +   port->pwr_opmode = TYPEC_PWR_MODE_USB;
> +   break;
> +   case UCSI_CONSTAT_PWR_OPMODE_BC:
> +   port->partner_type = TYPEC_PARTNER_CHARGER;
> +   port->pwr_opmode = TYPEC_PWR_MODE_BC1_2;
> +   break;
> +   case UCSI_CONSTAT_PWR_OPMODE_PD:
> +   port->pwr_opmode = TYPEC_PWR_MODE_PD;
> +   break;
> +   case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_3:
> +   port->pwr_opmode = TYPEC_PWR_MODE_1_5A;
> +   break;
> +   case UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0:
> +   port->pwr_opmode = TYPEC_PWR_MODE_3_0A;
> +   break;
> +   default:
> +   break;
> +   }
> +out:

CodingStyle suggests to do a better label naming.

> +   return typec_connect(port);
> +}

> +static void ucsi_connector_change(struct work_struct *work)
> +{
> +   struct ucsi_connector *con = container_of(work, struct ucsi_connector,
> + work);

> +}


> +int ucsi_init(struct ucsi *ucsi)
> +{
> +   struct ucsi_control *ctrl = (void *)>ppm->data->control;
> +   struct ucsi_connector *con;
> +   int ret;

> +   int i;

> +   for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +i++, con++) {

> +err:
> +   if (i > 0)
> +   for (; i >= 0; i--, con--)
> +   typec_unregister_port(con->port);

Perhaps

while (--i >= 0) {
 ...
}

But...

> +
> +   kfree(ucsi->connector);
> +   return ret;
> +}

> +struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm)
> +{
> +   struct ucsi *ucsi;
> +
> +   ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> +   if (!ucsi)
> +   return ERR_PTR(-ENOMEM);
> +
> +   init_completion(>complete);
> +   ucsi->dev = dev;
> +   ucsi->ppm = ppm;
> +
> +   return ucsi;
> +}
> +EXPORT_SYMBOL_GPL(ucsi_register_ppm);
> +
> +/**
> + * ucsi_unregister_ppm - Unregister UCSI PPM Interface
> + * @ucsi: struct ucsi associated with the PPM
> + *
> + * Unregister an UCSI PPM that was created with ucsi_register().
> + */
> +void ucsi_unregister_ppm(struct ucsi *ucsi)
> +{
> +   struct ucsi_connector *con;
> +   int i;
> +
> +   /* Disable all notifications */
> +   ucsi->ppm->data->control = UCSI_SET_NOTIFICATION_ENABLE;
> +   ucsi->ppm->cmd(ucsi->ppm);
> +

> +   for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +i++, con++)
> +   typec_unregister_port(con->port);

...looks a code duplication.

> +
> +   kfree(ucsi->connector);
> +   kfree(ucsi);
> +}
> 

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

> +static int ucsi_run_cmd(struct ucsi *ucsi, void *data, size_t size)
> +{
> + int status;
> + int ret;
> +
> + dev_vdbg(ucsi->dev, "%s control 0x%llx\n", __func__,
> +  ucsi->ppm->data->control);
> +
> + ret = ucsi->ppm->cmd(ucsi->ppm);
> + if (ret)
> + return ret;
> +
> + /* REVISIT: We may need to set UCSI_CCI_CMD_COMPLETE flag here */
> + wait_for_completion(>complete);
> +
> + status = ucsi->status;
> + if (status != UCSI_ERROR && size)
> + memcpy(data, ucsi->ppm->data->message_in, size);
> +
> + ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> + if (ret)
> + goto out;
> +
> + if (status == UCSI_ERROR) {
> + u16 error;
> +
> + ucsi->ppm->data->control = UCSI_GET_ERROR_STATUS;
> + ret = ucsi->ppm->cmd(ucsi->ppm);
> + if (ret)
> + goto out;
> +
> + wait_for_completion(>complete);
> +
> + /* Something has really gone wrong */
> + if (ucsi->status == UCSI_ERROR) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + memcpy(, ucsi->ppm->data->message_in, sizeof(error));
> +
> + ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> + if (ret)
> + goto out;
> +
> + switch (error) {
> + case UCSI_ERROR_INVALID_CON_NUM:
> + ret = -ENXIO;
> + break;
> + case UCSI_ERROR_INCOMPATIBLE_PARTNER:
> + case UCSI_ERROR_CC_COMMUNICATION_ERR:
> + case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
> + ret = -EIO;
> + break;

This looks like you mapped all the interesting error condition
on a single error code and gave out other error codes to
conditions of lesser importance.

> + case UCSI_ERROR_DEAD_BATTERY:
> + dev_warn(ucsi->dev, "Dead Battery Condition!\n");
> + ret = -EPERM;
> + break;
> + case UCSI_ERROR_UNREGONIZED_CMD:
> + case UCSI_ERROR_INVALID_CMD_ARGUMENT:
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + }
> +out:
> + ucsi->ppm->data->control = 0;
> + return ret;
> +}

[..]

> +/**
> + * ucsi_init - Initialize an UCSI Interface
> + * @ucsi: The UCSI Interface
> + *
> + * Registers all the USB Type-C ports governed by the PPM of @ucsi and 
> enables
> + * all the notifications from the PPM.
> + */
> +int ucsi_init(struct ucsi *ucsi)
> +{
> + struct ucsi_control *ctrl = (void *)>ppm->data->control;
> + struct ucsi_connector *con;
> + int ret;
> + int i;
> +
> + /* Enable basic notifications */
> + ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> + ctrl->data = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> + ret = ucsi_run_cmd(ucsi, NULL, 0);
> + if (ret)
> + return ret;
> +
> + /* Get PPM capabilities */
> + ctrl->cmd = UCSI_GET_CAPABILITY;
> + ret = ucsi_run_cmd(ucsi, >cap, sizeof(ucsi->cap));
> + if (ret)
> + return ret;
> +
> + ucsi->connector = kcalloc(ucsi->cap.num_connectors,
> +   sizeof(struct ucsi_connector), GFP_KERNEL);
> + if (!ucsi->connector)
> + return -ENOMEM;
> +
> + for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +  i++, con++) {
> + struct typec_capability *cap = >typec_cap;
> + struct ucsi_connector_status constat;
> +
> + /* Get connector capability */
> + ctrl->cmd = UCSI_GET_CONNECTOR_CAPABILITY;
> + ctrl->data = i + 1;
> + ret = ucsi_run_cmd(ucsi, >cap, sizeof(con->cap));
> + if (ret)
> + goto err;
> +
> + /* Register the connector */
> +
> + if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
> + cap->type = TYPEC_PORT_DRP;
> + else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP)
> + cap->type = TYPEC_PORT_DFP;
> + else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP)
> + cap->type = TYPEC_PORT_UFP;
> +
> + cap->usb_pd = !!(ucsi->cap.attributes &
> +UCSI_CAP_ATTR_USB_PD);
> + cap->audio_accessory = !!(con->cap.op_mode &
> +   UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY);
> + cap->debug_accessory = !!(con->cap.op_mode &
> +   UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY);
> +
> + /* TODO: Alt modes */
> +
> + cap->dr_swap = ucsi_dr_swap;
> + cap->pr_swap = ucsi_pr_swap;
> +
> + con->port = typec_register_port(ucsi->dev, cap);
> + if (IS_ERR(con->port)) {
> +  

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Tue, Feb 09, 2016 at 10:21:55AM -0800, Greg KH wrote:
> On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> > USB Type-C Connector System Software Interface (UCSI) is a
> > specification that defines registers and data structures
> > used to interface with the USB Type-C connectors on a system.
> > 
> > The specification is public and available at:
> > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> > 
> 
> What does this driver / code actually do?  Why is it needed?  What
> interface to the rest of the kernel / userspace does it provide?

I will fix this describe these things in the commit message. I'll just
but some UCSI background in case somebody is interested. So UCSI is in
practice a standard for USB Type-C controllers..

UCSI is the control interface for USB Type-C connectors (regardless
was USB PD supported or not) in MS Windows, so most likely all new HW
platforms designed to work also with Windows that are equipped with
USB Type-C will have UCSI device for controlling the USB Type-C ports.
In some cases the hardware for Type-C will be just a PHY like fusb30x
on these platforms (it's cheaper then USB PD or complete USB Type-C
controller), but in those cases the PHY is probable attached to an EC
or is completely controlled by system FW like BIOS together with any
USB PD communication in cases where USB PD is supported, and is in any
case not visible to the OS. Instead UCSI device is exposed to the OS
to give it means to apply its policies to the USB Type-C port.

> Why would we care about this?

I'll try to explain why it's important to export the control of USB
Type-C ports to the user space in my answer to your comments to the
first patch of this series, the one introducing the class.

But surely everybody agrees that decision about the policies regarding
USB Type-C ports, like which data role to use, do we charge or are we
letting the other end charge, etc., belongs to the user? If you plug
your phone to your desktop, I would imagine that you want to see the
phone primarily as the USB device and the desktop as host, and to
achieve the device role, you don't want to be forced to unplug/replug
your phone from the desktop until you achieve device role, right?

> You need to describe this a lot better than you did...

Sure thing. I'm sorry about the poor description. I send these out too
hastily.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> > +err:
> > +   if (i > 0)
> > +   for (; i >= 0; i--, con--)
> > +   typec_unregister_port(con->port);
> 
> Perhaps
> 
> while (--i >= 0) {
>  ...
> }

While we are at it. No we should not change the semantics
of conditionals for the sake of appearance.

Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum  wrote:
> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>> > +err:
>> > +   if (i > 0)
>> > +   for (; i >= 0; i--, con--)
>> > +   typec_unregister_port(con->port);
>>
>> Perhaps
>>
>> while (--i >= 0) {
>>  ...
>> }
>
> While we are at it. No we should not change the semantics
> of conditionals for the sake of appearance.

I'm sorry I didn't get you.
How this more or less standard pattern to clean up stuff on error path
does with conditional semantics?

I also noticed that this code might be factored out to helper which
will do same things (only number of loops is different) in both cases.
What did I miss?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum  wrote:
> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>> > +out:
>>
>> CodingStyle suggests to do a better label naming.
>
> Names coming from specs are what they are. There is
> no place for coding style here.

Yes, and how is it related to C label names?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-10 at 16:24 +0200, Andy Shevchenko wrote:
> On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum  wrote:
> > On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> >> > +err:
> >> > +   if (i > 0)
> >> > +   for (; i >= 0; i--, con--)
> >> > +   typec_unregister_port(con->port);
> >>
> >> Perhaps
> >>
> >> while (--i >= 0) {
> >>  ...
> >> }
> >
> > While we are at it. No we should not change the semantics
> > of conditionals for the sake of appearance.
> 
> I'm sorry I didn't get you.
> How this more or less standard pattern to clean up stuff on error path
> does with conditional semantics?

You change a postdecrement to a predecrement. The highest
number the loop is executed for is changed.

Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Bjørn Mork]

Andy Shevchenko  writes:
> On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum  wrote:
>> On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
>>> > +out:
>>>
>>> CodingStyle suggests to do a better label naming.
>>
>> Names coming from specs are what they are. There is
>> no place for coding style here.
>
> Yes, and how is it related to C label names?

It did appear as if you were commenting on the case labels since you
quoted two full switch blocks.  That's how I read your comment as well.

It's now clear that you somehow mean than "out:" is in conflict with
CodingStyle.  It is still very unclear how, and it does not seem like
you intend to make it any clearer since you did not take the opportunity
to explain yourself.

FWIW, I read the CodingStyle recommendation as: use descriptive labels
instead of "foo1", "foo2" etc, where "foo" is typically "err".  I do not
see this as conflicting with the use of "err" or "out" when there is a
single such label in a function.  The meaning of those labels are very
clear IMHO.

Exactly what is it about "out" that is unclear to you here?  Could you
propose a better alternative if you seriously mean that this needs to be
changed?


Bjørn

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Greg KH]

On Wed, Feb 10, 2016 at 12:30:42PM +0200, Heikki Krogerus wrote:
> On Tue, Feb 09, 2016 at 10:21:55AM -0800, Greg KH wrote:
> > On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> > > USB Type-C Connector System Software Interface (UCSI) is a
> > > specification that defines registers and data structures
> > > used to interface with the USB Type-C connectors on a system.
> > > 
> > > The specification is public and available at:
> > > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> > > 
> > 
> > What does this driver / code actually do?  Why is it needed?  What
> > interface to the rest of the kernel / userspace does it provide?
> 
> I will fix this describe these things in the commit message. I'll just
> but some UCSI background in case somebody is interested. So UCSI is in
> practice a standard for USB Type-C controllers..
> 
> UCSI is the control interface for USB Type-C connectors (regardless
> was USB PD supported or not) in MS Windows, so most likely all new HW
> platforms designed to work also with Windows that are equipped with
> USB Type-C will have UCSI device for controlling the USB Type-C ports.

There's many millions of devices with type-C without Windows on them, so
don't count on this being everywhere :)

> In some cases the hardware for Type-C will be just a PHY like fusb30x
> on these platforms (it's cheaper then USB PD or complete USB Type-C
> controller), but in those cases the PHY is probable attached to an EC
> or is completely controlled by system FW like BIOS together with any
> USB PD communication in cases where USB PD is supported, and is in any
> case not visible to the OS. Instead UCSI device is exposed to the OS
> to give it means to apply its policies to the USB Type-C port.
> 
> > Why would we care about this?
> 
> I'll try to explain why it's important to export the control of USB
> Type-C ports to the user space in my answer to your comments to the
> first patch of this series, the one introducing the class.
> 
> But surely everybody agrees that decision about the policies regarding
> USB Type-C ports, like which data role to use, do we charge or are we
> letting the other end charge, etc., belongs to the user?

No, I don't agree.  It's still unknown if userspace can react fast
though to these types of "policy" changes.  I've heard from some
manufacturers that the response time needed is something that we can't
leave to userspace.

And along those lines, do you have a working userspace user of this
interface?  We don't create interfaces without a user, especially given
that it takes a long time to ensure that a user/kernel api actually is
correct.  We would need to see that to ensure that this kernel
implementation is "correct" and working properly.

> If you plug
> your phone to your desktop, I would imagine that you want to see the
> phone primarily as the USB device and the desktop as host, and to
> achieve the device role, you don't want to be forced to unplug/replug
> your phone from the desktop until you achieve device role, right?

Why is unplugging somehow required?

thanks,

greg k-h

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

> +static int ucsi_run_cmd(struct ucsi *ucsi, void *data, size_t size)
> +{
> + int status;
> + int ret;
> +
> + dev_vdbg(ucsi->dev, "%s control 0x%llx\n", __func__,
> +  ucsi->ppm->data->control);
> +
> + ret = ucsi->ppm->cmd(ucsi->ppm);
> + if (ret)
> + return ret;
> +
> + /* REVISIT: We may need to set UCSI_CCI_CMD_COMPLETE flag here */
> + wait_for_completion(>complete);
> +
> + status = ucsi->status;
> + if (status != UCSI_ERROR && size)
> + memcpy(data, ucsi->ppm->data->message_in, size);
> +
> + ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> + if (ret)
> + goto out;
> +
> + if (status == UCSI_ERROR) {
> + u16 error;
> +
> + ucsi->ppm->data->control = UCSI_GET_ERROR_STATUS;
> + ret = ucsi->ppm->cmd(ucsi->ppm);
> + if (ret)
> + goto out;
> +
> + wait_for_completion(>complete);
> +
> + /* Something has really gone wrong */
> + if (ucsi->status == UCSI_ERROR) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + memcpy(, ucsi->ppm->data->message_in, sizeof(error));
> +
> + ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> + if (ret)
> + goto out;
> +
> + switch (error) {
> + case UCSI_ERROR_INVALID_CON_NUM:
> + ret = -ENXIO;
> + break;
> + case UCSI_ERROR_INCOMPATIBLE_PARTNER:
> + case UCSI_ERROR_CC_COMMUNICATION_ERR:
> + case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
> + ret = -EIO;
> + break;

This looks like you mapped all the interesting error condition
on a single error code and gave out other error codes to
conditions of lesser importance.

> + case UCSI_ERROR_DEAD_BATTERY:
> + dev_warn(ucsi->dev, "Dead Battery Condition!\n");
> + ret = -EPERM;
> + break;
> + case UCSI_ERROR_UNREGONIZED_CMD:
> + case UCSI_ERROR_INVALID_CMD_ARGUMENT:
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + }
> +out:
> + ucsi->ppm->data->control = 0;
> + return ret;
> +}

[..]

> +/**
> + * ucsi_init - Initialize an UCSI Interface
> + * @ucsi: The UCSI Interface
> + *
> + * Registers all the USB Type-C ports governed by the PPM of @ucsi and 
> enables
> + * all the notifications from the PPM.
> + */
> +int ucsi_init(struct ucsi *ucsi)
> +{
> + struct ucsi_control *ctrl = (void *)>ppm->data->control;
> + struct ucsi_connector *con;
> + int ret;
> + int i;
> +
> + /* Enable basic notifications */
> + ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> + ctrl->data = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> + ret = ucsi_run_cmd(ucsi, NULL, 0);
> + if (ret)
> + return ret;
> +
> + /* Get PPM capabilities */
> + ctrl->cmd = UCSI_GET_CAPABILITY;
> + ret = ucsi_run_cmd(ucsi, >cap, sizeof(ucsi->cap));
> + if (ret)
> + return ret;
> +
> + ucsi->connector = kcalloc(ucsi->cap.num_connectors,
> +   sizeof(struct ucsi_connector), GFP_KERNEL);
> + if (!ucsi->connector)
> + return -ENOMEM;
> +
> + for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +  i++, con++) {
> + struct typec_capability *cap = >typec_cap;
> + struct ucsi_connector_status constat;
> +
> + /* Get connector capability */
> + ctrl->cmd = UCSI_GET_CONNECTOR_CAPABILITY;
> + ctrl->data = i + 1;
> + ret = ucsi_run_cmd(ucsi, >cap, sizeof(con->cap));
> + if (ret)
> + goto err;
> +
> + /* Register the connector */
> +
> + if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
> + cap->type = TYPEC_PORT_DRP;
> + else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP)
> + cap->type = TYPEC_PORT_DFP;
> + else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP)
> + cap->type = TYPEC_PORT_UFP;
> +
> + cap->usb_pd = !!(ucsi->cap.attributes &
> +UCSI_CAP_ATTR_USB_PD);
> + cap->audio_accessory = !!(con->cap.op_mode &
> +   UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY);
> + cap->debug_accessory = !!(con->cap.op_mode &
> +   UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY);
> +
> + /* TODO: Alt modes */
> +
> + cap->dr_swap = ucsi_dr_swap;
> + cap->pr_swap = ucsi_pr_swap;
> +
> + con->port = typec_register_port(ucsi->dev, cap);
> + if (IS_ERR(con->port)) {
> +  

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Tue, Feb 09, 2016 at 10:21:55AM -0800, Greg KH wrote:
> On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> > USB Type-C Connector System Software Interface (UCSI) is a
> > specification that defines registers and data structures
> > used to interface with the USB Type-C connectors on a system.
> > 
> > The specification is public and available at:
> > http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> > 
> 
> What does this driver / code actually do?  Why is it needed?  What
> interface to the rest of the kernel / userspace does it provide?

I will fix this describe these things in the commit message. I'll just
but some UCSI background in case somebody is interested. So UCSI is in
practice a standard for USB Type-C controllers..

UCSI is the control interface for USB Type-C connectors (regardless
was USB PD supported or not) in MS Windows, so most likely all new HW
platforms designed to work also with Windows that are equipped with
USB Type-C will have UCSI device for controlling the USB Type-C ports.
In some cases the hardware for Type-C will be just a PHY like fusb30x
on these platforms (it's cheaper then USB PD or complete USB Type-C
controller), but in those cases the PHY is probable attached to an EC
or is completely controlled by system FW like BIOS together with any
USB PD communication in cases where USB PD is supported, and is in any
case not visible to the OS. Instead UCSI device is exposed to the OS
to give it means to apply its policies to the USB Type-C port.

> Why would we care about this?

I'll try to explain why it's important to export the control of USB
Type-C ports to the user space in my answer to your comments to the
first patch of this series, the one introducing the class.

But surely everybody agrees that decision about the policies regarding
USB Type-C ports, like which data role to use, do we charge or are we
letting the other end charge, etc., belongs to the user? If you plug
your phone to your desktop, I would imagine that you want to see the
phone primarily as the USB device and the desktop as host, and to
achieve the device role, you don't want to be forced to unplug/replug
your phone from the desktop until you achieve device role, right?

> You need to describe this a lot better than you did...

Sure thing. I'm sorry about the poor description. I send these out too
hastily.


Thanks,

-- 
heikki

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Andy Shevchenko]

On Tue, Feb 9, 2016 at 7:01 PM, Heikki Krogerus
 wrote:
> USB Type-C Connector System Software Interface (UCSI) is a
> specification that defines registers and data structures
> used to interface with the USB Type-C connectors on a system.
>
> The specification is public and available at:
> http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html

+#define to_ucsi_connector(_port_) container_of(_port_->cap,
 \
+  struct ucsi_connector,  \
+  typec_cap)

Perhaps
static inline ...

+
+#define cci_to_connector(_ucsi_, cci) (_ucsi_->connector +\
+   UCSI_CCI_CONNECTOR_CHANGE(cci) - 1)
+

If you start you macro on the next line it will look better.

> +static int
> +ucsi_connect(struct ucsi_connector *con, struct ucsi_connector_status 
> *constat)
> +{
> +   struct typec_port *port = con->port;
> +
> +   port->connected = true;
> +
> +   if (constat->partner_flags & UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE)
> +   port->partner_type = TYPEC_PARTNER_ALTMODE;
> +   else
> +   port->partner_type = TYPEC_PARTNER_USB;
> +
> +   switch (constat->partner_type) {
> +   case UCSI_CONSTAT_PARTNER_TYPE_CABLE_NO_UFP:
> +   /* REVISIT: We don't care about just the cable for now */
> +   return 0;
> +   case UCSI_CONSTAT_PARTNER_TYPE_DFP:
> +   case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> +   port->pwr_role = TYPEC_PWR_SINK;
> +   port->data_role = TYPEC_DEVICE;
> +   break;
> +   case UCSI_CONSTAT_PARTNER_TYPE_UFP:
> +   port->pwr_role = TYPEC_PWR_SOURCE;
> +   port->data_role = TYPEC_HOST;
> +   break;
> +   case UCSI_CONSTAT_PARTNER_TYPE_DEBUG:
> +   port->partner_type = TYPEC_PARTNER_DEBUG;
> +   goto out;
> +   case UCSI_CONSTAT_PARTNER_TYPE_AUDIO:
> +   port->partner_type = TYPEC_PARTNER_AUDIO;
> +   goto out;
> +   }
> +
> +   switch (constat->pwr_op_mode) {
> +   case UCSI_CONSTAT_PWR_OPMODE_NONE:
> +   case UCSI_CONSTAT_PWR_OPMODE_DEFAULT:
> +   port->pwr_opmode = TYPEC_PWR_MODE_USB;
> +   break;
> +   case UCSI_CONSTAT_PWR_OPMODE_BC:
> +   port->partner_type = TYPEC_PARTNER_CHARGER;
> +   port->pwr_opmode = TYPEC_PWR_MODE_BC1_2;
> +   break;
> +   case UCSI_CONSTAT_PWR_OPMODE_PD:
> +   port->pwr_opmode = TYPEC_PWR_MODE_PD;
> +   break;
> +   case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_3:
> +   port->pwr_opmode = TYPEC_PWR_MODE_1_5A;
> +   break;
> +   case UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0:
> +   port->pwr_opmode = TYPEC_PWR_MODE_3_0A;
> +   break;
> +   default:
> +   break;
> +   }
> +out:

CodingStyle suggests to do a better label naming.

> +   return typec_connect(port);
> +}

> +static void ucsi_connector_change(struct work_struct *work)
> +{
> +   struct ucsi_connector *con = container_of(work, struct ucsi_connector,
> + work);

> +}


> +int ucsi_init(struct ucsi *ucsi)
> +{
> +   struct ucsi_control *ctrl = (void *)>ppm->data->control;
> +   struct ucsi_connector *con;
> +   int ret;

> +   int i;

> +   for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +i++, con++) {

> +err:
> +   if (i > 0)
> +   for (; i >= 0; i--, con--)
> +   typec_unregister_port(con->port);

Perhaps

while (--i >= 0) {
 ...
}

But...

> +
> +   kfree(ucsi->connector);
> +   return ret;
> +}

> +struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm)
> +{
> +   struct ucsi *ucsi;
> +
> +   ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> +   if (!ucsi)
> +   return ERR_PTR(-ENOMEM);
> +
> +   init_completion(>complete);
> +   ucsi->dev = dev;
> +   ucsi->ppm = ppm;
> +
> +   return ucsi;
> +}
> +EXPORT_SYMBOL_GPL(ucsi_register_ppm);
> +
> +/**
> + * ucsi_unregister_ppm - Unregister UCSI PPM Interface
> + * @ucsi: struct ucsi associated with the PPM
> + *
> + * Unregister an UCSI PPM that was created with ucsi_register().
> + */
> +void ucsi_unregister_ppm(struct ucsi *ucsi)
> +{
> +   struct ucsi_connector *con;
> +   int i;
> +
> +   /* Disable all notifications */
> +   ucsi->ppm->data->control = UCSI_SET_NOTIFICATION_ENABLE;
> +   ucsi->ppm->cmd(ucsi->ppm);
> +

> +   for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> +i++, con++)
> +   typec_unregister_port(con->port);

...looks a code duplication.

> +
> +   kfree(ucsi->connector);
> +   kfree(ucsi);
> +}
> 

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Heikki Krogerus]

On Wed, Feb 10, 2016 at 12:19:53PM +0100, Oliver Neukum wrote:
> 
> > +static int ucsi_run_cmd(struct ucsi *ucsi, void *data, size_t size)
> > +{
> > +   int status;
> > +   int ret;
> > +
> > +   dev_vdbg(ucsi->dev, "%s control 0x%llx\n", __func__,
> > +ucsi->ppm->data->control);
> > +
> > +   ret = ucsi->ppm->cmd(ucsi->ppm);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* REVISIT: We may need to set UCSI_CCI_CMD_COMPLETE flag here */
> > +   wait_for_completion(>complete);
> > +
> > +   status = ucsi->status;
> > +   if (status != UCSI_ERROR && size)
> > +   memcpy(data, ucsi->ppm->data->message_in, size);
> > +
> > +   ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> > +   if (ret)
> > +   goto out;
> > +
> > +   if (status == UCSI_ERROR) {
> > +   u16 error;
> > +
> > +   ucsi->ppm->data->control = UCSI_GET_ERROR_STATUS;
> > +   ret = ucsi->ppm->cmd(ucsi->ppm);
> > +   if (ret)
> > +   goto out;
> > +
> > +   wait_for_completion(>complete);
> > +
> > +   /* Something has really gone wrong */
> > +   if (ucsi->status == UCSI_ERROR) {
> > +   ret = -ENODEV;
> > +   goto out;
> > +   }
> > +
> > +   memcpy(, ucsi->ppm->data->message_in, sizeof(error));
> > +
> > +   ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> > +   if (ret)
> > +   goto out;
> > +
> > +   switch (error) {
> > +   case UCSI_ERROR_INVALID_CON_NUM:
> > +   ret = -ENXIO;
> > +   break;
> > +   case UCSI_ERROR_INCOMPATIBLE_PARTNER:
> > +   case UCSI_ERROR_CC_COMMUNICATION_ERR:
> > +   case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
> > +   ret = -EIO;
> > +   break;
> 
> This looks like you mapped all the interesting error condition
> on a single error code and gave out other error codes to
> conditions of lesser importance.

OK, I'll fix those.

> > +   case UCSI_ERROR_DEAD_BATTERY:
> > +   dev_warn(ucsi->dev, "Dead Battery Condition!\n");
> > +   ret = -EPERM;
> > +   break;
> > +   case UCSI_ERROR_UNREGONIZED_CMD:
> > +   case UCSI_ERROR_INVALID_CMD_ARGUMENT:
> > +   default:
> > +   ret = -EINVAL;
> > +   break;
> > +   }
> > +   }
> > +out:
> > +   ucsi->ppm->data->control = 0;
> > +   return ret;
> > +}
> 
> [..]
> 
> > +/**
> > + * ucsi_init - Initialize an UCSI Interface
> > + * @ucsi: The UCSI Interface
> > + *
> > + * Registers all the USB Type-C ports governed by the PPM of @ucsi and 
> > enables
> > + * all the notifications from the PPM.
> > + */
> > +int ucsi_init(struct ucsi *ucsi)
> > +{
> > +   struct ucsi_control *ctrl = (void *)>ppm->data->control;
> > +   struct ucsi_connector *con;
> > +   int ret;
> > +   int i;
> > +
> > +   /* Enable basic notifications */
> > +   ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> > +   ctrl->data = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
> > +   ret = ucsi_run_cmd(ucsi, NULL, 0);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Get PPM capabilities */
> > +   ctrl->cmd = UCSI_GET_CAPABILITY;
> > +   ret = ucsi_run_cmd(ucsi, >cap, sizeof(ucsi->cap));
> > +   if (ret)
> > +   return ret;
> > +
> > +   ucsi->connector = kcalloc(ucsi->cap.num_connectors,
> > + sizeof(struct ucsi_connector), GFP_KERNEL);
> > +   if (!ucsi->connector)
> > +   return -ENOMEM;
> > +
> > +   for (i = 0, con = ucsi->connector; i < ucsi->cap.num_connectors;
> > +i++, con++) {
> > +   struct typec_capability *cap = >typec_cap;
> > +   struct ucsi_connector_status constat;
> > +
> > +   /* Get connector capability */
> > +   ctrl->cmd = UCSI_GET_CONNECTOR_CAPABILITY;
> > +   ctrl->data = i + 1;
> > +   ret = ucsi_run_cmd(ucsi, >cap, sizeof(con->cap));
> > +   if (ret)
> > +   goto err;
> > +
> > +   /* Register the connector */
> > +
> > +   if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
> > +   cap->type = TYPEC_PORT_DRP;
> > +   else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP)
> > +   cap->type = TYPEC_PORT_DFP;
> > +   else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP)
> > +   cap->type = TYPEC_PORT_UFP;
> > +
> > +   cap->usb_pd = !!(ucsi->cap.attributes &
> > +  UCSI_CAP_ATTR_USB_PD);
> > +   cap->audio_accessory = !!(con->cap.op_mode &
> > + UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY);
> > +   cap->debug_accessory = !!(con->cap.op_mode &
> > + UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY);
> > +
> > +   /* TODO: Alt modes */
> > +
> > +   cap->dr_swap = ucsi_dr_swap;
> > 

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> +#define UCSI_CONSTAT_BC_NOT_CHARGING   0
> +#define UCSI_CONSTAT_BC_NOMINAL_CHARGING   1
> +#define UCSI_CONSTAT_BC_SLOW_CHARGING  2
> +#define UCSI_CONSTAT_BC_TRICLE_CHARGING3

typo. It is spelled TRICKLE

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Oliver Neukum]

On Wed, 2016-02-10 at 13:56 +0200, Andy Shevchenko wrote:
> > +out:
> 
> CodingStyle suggests to do a better label naming.

Names coming from specs are what they are. There is
no place for coding style here.

Regards
Oliver

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Greg KH]

On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> USB Type-C Connector System Software Interface (UCSI) is a
> specification that defines registers and data structures
> used to interface with the USB Type-C connectors on a system.
> 
> The specification is public and available at:
> http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> 

What does this driver / code actually do?  Why is it needed?  What
interface to the rest of the kernel / userspace does it provide?

Why would we care about this?


You need to describe this a lot better than you did...

thanks,

greg k-h

Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface

[Greg KH]

On Tue, Feb 09, 2016 at 07:01:22PM +0200, Heikki Krogerus wrote:
> USB Type-C Connector System Software Interface (UCSI) is a
> specification that defines registers and data structures
> used to interface with the USB Type-C connectors on a system.
> 
> The specification is public and available at:
> http://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-ucsi-spec.html
> 

What does this driver / code actually do?  Why is it needed?  What
interface to the rest of the kernel / userspace does it provide?

Why would we care about this?


You need to describe this a lot better than you did...

thanks,

greg k-h