Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1330485 > unrolled thread
| Started by | Heikki Krogerus <heikki.krogerus@linux.intel.com> |
|---|---|
| First post | 2016-02-09 18:10 +0100 |
| Last post | 2016-02-18 12:20 +0100 |
| Articles | 20 on this page of 52 — 10 participants |
Back to article view | Back to linux.kernel
[PATCH 0/3] usb: USB Type-C Class and driver for UCSI Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-09 18:10 +0100
[PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-09 18:10 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Greg KH <gregkh@linuxfoundation.org> - 2016-02-09 19:30 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-10 11:40 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Greg KH <gregkh@linuxfoundation.org> - 2016-02-10 18:30 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-11 15:10 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-15 16:40 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-16 10:30 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-16 14:50 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-17 09:00 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-17 10:10 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Felipe Balbi <balbif@gmail.com> - 2016-02-17 11:40 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-17 11:40 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-17 12:20 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Felipe Balbi <balbi@kernel.org> - 2016-02-17 14:40 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-17 15:30 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Peter Chen <hzpeterchen@gmail.com> - 2016-02-18 10:20 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-18 11:50 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Rajaram R <rajaram.officemail@gmail.com> - 2016-02-18 11:40 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-18 11:50 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Rajaram R <rajaram.officemail@gmail.com> - 2016-02-18 12:10 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Felipe Balbi <balbi@kernel.org> - 2016-02-17 14:40 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-17 15:00 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Felipe Balbi <balbif@gmail.com> - 2016-02-18 08:10 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-18 11:30 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Felipe Balbi <balbif@gmail.com> - 2016-02-18 11:40 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-18 11:50 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Peter Chen <hzpeterchen@gmail.com> - 2016-02-18 10:40 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-18 10:50 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-10 12:30 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-10 13:10 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-02-10 13:00 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-10 14:30 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-02-10 15:10 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Bjørn Mork <bjorn@mork.no> - 2016-02-10 16:20 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-02-11 09:30 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Bjørn Mork <bjorn@mork.no> - 2016-02-11 10:10 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-10 15:20 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-02-10 15:30 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.de> - 2016-02-10 16:20 +0100
Fwd: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-02-11 09:20 +0100
Re: Fwd: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-11 15:20 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Oliver Neukum <oneukum@suse.com> - 2016-02-10 14:10 +0100
Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-11 15:10 +0100
[PATCH 3/3] usb: type-c: UCSI ACPI driver Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-09 18:10 +0100
Re: [PATCH 3/3] usb: type-c: UCSI ACPI driver Greg KH <gregkh@linuxfoundation.org> - 2016-02-09 19:30 +0100
Re: [PATCH 3/3] usb: type-c: UCSI ACPI driver Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-10 11:30 +0100
Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI Oliver Neukum <oneukum@suse.com> - 2016-02-17 20:00 +0100
Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-18 10:30 +0100
Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI Rajaram R <rajaram.officemail@gmail.com> - 2016-02-17 20:40 +0100
Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-18 12:10 +0100
Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI Oliver Neukum <oneukum@suse.com> - 2016-02-18 12:20 +0100
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Rajaram R <rajaram.officemail@gmail.com> |
|---|---|
| Date | 2016-02-18 12:10 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r3uD7-4Bz-9@gated-at.bofh.it> |
| In reply to | #1337261 |
On Thu, Feb 18, 2016 at 4:17 PM, Heikki Krogerus <heikki.krogerus@linux.intel.com> 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 >> <heikki.krogerus@linux.intel.com> wrote: >> > On Wed, Feb 17, 2016 at 03:36:46PM +0200, Felipe Balbi wrote: >> >> >> >> Hi, >> >> >> >> Heikki Krogerus <heikki.krogerus@linux.intel.com> 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 <oneukum@suse.com> 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 4-16)" Ok I get it. Here in a user perspective its a connect and disconnect and things are random which user may not prefer. > > > Thanks, > > -- > heikki
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-02-17 14:40 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r3auL-73z-11@gated-at.bofh.it> |
| In reply to | #1336233 |
[Multipart message — attachments visible in raw view] — view raw
Hi, Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Oliver Neukum <oneukum@suse.com> |
|---|---|
| Date | 2016-02-17 15:00 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r3aO6-7aS-27@gated-at.bofh.it> |
| In reply to | #1336420 |
On Wed, 2016-02-17 at 15:34 +0200, Felipe Balbi wrote: > Hi, > > Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbif@gmail.com> |
|---|---|
| Date | 2016-02-18 08:10 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r3qSR-1Yu-5@gated-at.bofh.it> |
| In reply to | #1336451 |
[Multipart message — attachments visible in raw view] — view raw
Hi, Oliver Neukum <oneukum@suse.com> writes: >> Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Oliver Neukum <oneukum@suse.com> |
|---|---|
| Date | 2016-02-18 11:30 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r3u0s-44o-75@gated-at.bofh.it> |
| In reply to | #1337094 |
On Thu, 2016-02-18 at 09:08 +0200, Felipe Balbi wrote: > Oliver Neukum <oneukum@suse.com> writes: > >> Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Felipe Balbi <balbif@gmail.com> |
|---|---|
| Date | 2016-02-18 11:40 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r3ua6-49t-19@gated-at.bofh.it> |
| In reply to | #1337243 |
[Multipart message — attachments visible in raw view] — view raw
Hi, Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Oliver Neukum <oneukum@suse.com> |
|---|---|
| Date | 2016-02-18 11:50 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r3ujM-4dh-11@gated-at.bofh.it> |
| In reply to | #1337251 |
On Thu, 2016-02-18 at 12:30 +0200, Felipe Balbi wrote: > Hi, > > Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-02-18 10:40 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r3te3-3vZ-19@gated-at.bofh.it> |
| In reply to | #1331047 |
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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best Regards, Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Oliver Neukum <oneukum@suse.com> |
|---|---|
| Date | 2016-02-18 10:50 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r3tnI-3zl-13@gated-at.bofh.it> |
| In reply to | #1337199 |
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
[toc] | [prev] | [next] | [standalone]
| From | Oliver Neukum <oneukum@suse.com> |
|---|---|
| Date | 2016-02-10 12:30 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0B86-1xR-7@gated-at.bofh.it> |
| In reply to | #1330489 |
> +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(&ucsi->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(&ucsi->complete);
> +
> + /* Something has really gone wrong */
> + if (ucsi->status == UCSI_ERROR) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + memcpy(&error, 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 *)&ucsi->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, &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 = &con->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, &con->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)) {
> + ret = PTR_ERR(con->port);
> + goto err;
> + }
> +
> + con->num = i + 1;
> + con->ucsi = ucsi;
> + INIT_WORK(&con->work, ucsi_connector_change);
You init the work later. Does this depend on the firmware
generating no events before they are enabled? Depending on firmware
is a bad idea.
> +
> + /* Check if the connector is connected */
> + if (WARN_ON(ucsi_get_constat(con, &constat) != 0))
> + continue;
> +
> + if (constat.connected)
> + ucsi_connect(con, &constat);
> + }
> +
> + /* Enable all notifications */
> + ctrl->cmd = UCSI_SET_NOTIFICATION_ENABLE;
> + ctrl->data = UCSI_ENABLE_NTFY_ALL;
> + ret = ucsi_run_cmd(ucsi, NULL, 0);
> + if (ret)
> + goto err;
> +
> + return 0;
> +err:
> + if (i > 0)
> + for (; i >= 0; i--, con--)
> + typec_unregister_port(con->port);
> +
> + kfree(ucsi->connector);
> + return ret;
> +}
> +EXPORT_SYMBOL(ucsi_init);
> +
[toc] | [prev] | [next] | [standalone]
| From | Heikki Krogerus <heikki.krogerus@linux.intel.com> |
|---|---|
| Date | 2016-02-10 13:10 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0BKO-21z-15@gated-at.bofh.it> |
| In reply to | #1331076 |
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(&ucsi->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(&ucsi->complete);
> > +
> > + /* Something has really gone wrong */
> > + if (ucsi->status == UCSI_ERROR) {
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + memcpy(&error, 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 *)&ucsi->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, &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 = &con->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, &con->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)) {
> > + ret = PTR_ERR(con->port);
> > + goto err;
> > + }
> > +
> > + con->num = i + 1;
> > + con->ucsi = ucsi;
> > + INIT_WORK(&con->work, ucsi_connector_change);
>
> You init the work later. Does this depend on the firmware
> generating no events before they are enabled? Depending on firmware
> is a bad idea.
If there is a real risk of getting connector change events before we
have enabled them, I think the only thing we can do is add checks to
ucsi_interrupt() to make sure that we don't try to handle events from
connectors that are not yet initialized.
Thanks,
--
heikki
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2016-02-10 13:00 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0BB8-1J1-19@gated-at.bofh.it> |
| In reply to | #1330489 |
On Tue, Feb 9, 2016 at 7:01 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> 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 *)&ucsi->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(&ucsi->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);
> +}
> +EXPORT_SYMBOL_GPL(ucsi_unregister_ppm);
> +#include <linux/types.h>
> +
> +/* -------------------------------------------------------------------------- */
> +
> +struct ucsi_data {
> + __u16 version;
> + __u16 RESERVED;
Small letters?
> + __u32 cci;
> + __u64 control;
> + __u32 message_in[4];
> + __u32 message_out[4];
> +} __packed;
> +/* Bits for SET_NOTIFICATION_ENABLE command */
> +#define UCSI_ENABLE_NTFY_CMD_COMPLETE BIT(0)
> +#define UCSI_ENABLE_NTFY_EXT_PWR_SRC_CHANGE BIT(1)
> +#define UCSI_ENABLE_NTFY_PWR_OPMODE_CHANGE BIT(2)
> +#define UCSI_ENABLE_NTFY_CAP_CHANGE BIT(5)
> +#define UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE BIT(6)
> +#define UCSI_ENABLE_NTFY_PD_RESET_COMPLETE BIT(7)
> +#define UCSI_ENABLE_NTFY_CAM_CHANGE BIT(8)
> +#define UCSI_ENABLE_NTFY_BAT_STATUS_CHANGE BIT(9)
> +#define UCSI_ENABLE_NTFY_PARTNER_CHANGE BIT(11)
> +#define UCSI_ENABLE_NTFY_PWR_DIR_CHANGE BIT(12)
> +#define UCSI_ENABLE_NTFY_CONNECTOR_CHANGE BIT(14)
> +#define UCSI_ENABLE_NTFY_ERROR BIT(15)
> +#define UCSI_ENABLE_NTFY_ALL 0xdbf3
Hmm... I would add the decode part of this, i.e. what kind of
notifications are excluded (and maybe why), e.g. BIT(2).
> + __u8 num_alt_modes;
> + __u8 RESERVED;
Small letters?
> + __u16 bc_version;
> + __u16 pd_version;
> + __u16 typec_version;
> +} __packed;
> + __u8 mode_support:1;
> + __u8 RESERVED_2:2;
> + __u8 latency:4;
> + __u8 RESERVED_4:4;
> +} __packed;
Ditto.
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Oliver Neukum <oneukum@suse.com> |
|---|---|
| Date | 2016-02-10 14:30 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0D0f-2IY-17@gated-at.bofh.it> |
| In reply to | #1331099 |
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
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2016-02-10 15:10 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0DCV-3cV-3@gated-at.bofh.it> |
| In reply to | #1331162 |
On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Bjørn Mork <bjorn@mork.no> |
|---|---|
| Date | 2016-02-10 16:20 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0EIH-3QV-31@gated-at.bofh.it> |
| In reply to | #1331184 |
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2016-02-11 09:30 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0UNr-6c7-1@gated-at.bofh.it> |
| In reply to | #1331268 |
On Wed, Feb 10, 2016 at 5:11 PM, Bjørn Mork <bjorn@mork.no> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> writes: >> On Wed, Feb 10, 2016 at 3:21 PM, Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Bjørn Mork <bjorn@mork.no> |
|---|---|
| Date | 2016-02-11 10:10 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0Vqa-6Fe-9@gated-at.bofh.it> |
| In reply to | #1331727 |
Andy Shevchenko <andy.shevchenko@gmail.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Oliver Neukum <oneukum@suse.com> |
|---|---|
| Date | 2016-02-10 15:20 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0DMC-3gn-9@gated-at.bofh.it> |
| In reply to | #1331099 |
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
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2016-02-10 15:30 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0DWi-3jE-15@gated-at.bofh.it> |
| In reply to | #1331194 |
On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
| From | Oliver Neukum <oneukum@suse.de> |
|---|---|
| Date | 2016-02-10 16:20 +0100 |
| Subject | Re: [PATCH 2/3] usb: type-c: USB Type-C Connector System Software Interface |
| Message-ID | <r0EIH-3QV-27@gated-at.bofh.it> |
| In reply to | #1331205 |
On Wed, 2016-02-10 at 16:24 +0200, Andy Shevchenko wrote:
> On Wed, Feb 10, 2016 at 4:15 PM, Oliver Neukum <oneukum@suse.com> 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
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web