[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [cdi-devel] [PATCH v2 6/9] cdi/usb: Add headers

On 18.04.2015 00:35, Kevin Wolf wrote:
On Thu, Apr 16, 2015 at 12:13:55AM +0200, Max Reitz wrote:
+ Add headers to be used by USB host controller drivers, the USB bus
   driver, and USB device drivers.
   These headers are still lacking functions for periodic transactions
   (interrupt/isochronous), hotplugging, suspend/resume, etc. pp..

Signed-off-by: Max Reitz <max@xxxxxxxxxx>
Lots of documentation texts here. FWIW, I'm okay with leaving the
USB-specific headers in English only. It just felt out of place in the
existing bilingual (or even German-only) headers.

+ * This function is provided by the reference USB bus driver, it must only be
+ * called by the CDI library.
+ *
+ * It is called once a host controller driver provides a host controller device
+ * (returned by its init_device() function) which is passed to this function.
+ */
+void cdi_usb_provide_hc(struct cdi_device* dev);
Hm, I'm afraid I don't fully understand this function yet.

If this is part of the USB bus driver, why isn't it in struct

Okay, I looked in the tyndur patches, and it seems this is actually
_not_ provided by the USB bus driver, but part of the CDI USB library
(and used in the HCD driver process, not the USB one).

Oops, right. The comment is still from when I intended to make the bus driver part of the CDI library.

In other words, since you also say it must only be called by the
library, isn't this a purely internal implementation detail that isn't
necessary in the public header files?

Yes, it should be removed from the public interface. It appears to be just left over in this header; in the implementation, it's actually called cdi_osdep_provide_hc() already.

+ * Contains information about a communication pipe to a USB device endpoint used
+ * for USB transactions.
+ */
+struct cdi_usb_hc_ep_info {
+    /// Device ID
+    int dev;
+    /// Endpoint ID
+    int ep;
+    /// Endpoint type
+    cdi_usb_endpoint_type_t ep_type;
+    /// Transfer speed
+    cdi_usb_speed_t speed;
+    /// Maximum packet size for this endpoint
+    int mps;
+    /**
+     * For low and full speed devices operated behind a high speed hub: @tt_addr
+     * is the USB device address of the translating USB hub (the last high speed
+     * hub), @tt_port is the port (0-based index) of that hub where the first
+     * non-high-speed device of this device's tree is plugged into.
+     */
+    int tt_addr, tt_port;
I wonder if all the ints in this struct should be unsigned (and then
consistently so throughout all the header files).

Well... If you're going that far, dev should be uint8_t, ep should be uint4_t (well...), and we can probably just make mps a size_t. tt_addr should be uint8_t again, and tt_port... Well, maybe uint8_t, too.

+ * Describes a single USB transmission.
+ */
+struct cdi_usb_hc_transmission {
+    /// Transaction this transmission is part of
+    cdi_usb_hc_transaction_t ta;
+    /// Type of transfer to be performed
+    cdi_usb_transfer_token_t token;
+    /// Buffer to be used (may be NULL if @size is 0)
+    void* buffer;
+    /// Number of bytes to be transferred (may be 0, and may exceed the MPS)
+    size_t size;
Perhaps this suggests that the right unsigned type for mps is size_t,

+    /**
+     * Toggle bit to be used for the first packet issued in this transmission.
+     * If @size exceeds the MPS and thus multiple packets are transmitted, the
+     * toggle bit will be toggled automatically.
+     */
+    int toggle;
bool? (Honest question; "bit" just sounds like it, but I don't know how
this actually works)

Good question, I asked myself the same. For normal data transfers, this bit can be only 0 or 1. But for isochronous transfers, I (think I) know it can switch between 0, 1, 2, and even something else.

Also, bool wouldn't be the right type even if it can be only 0 or 1. It's a bit that's switched for every transfer (so that lost packets can be detected, a bit like the TCP sequence number; only you don't need a full-fledged sequence number for USB because every (non-isochronous) packet is replied to with an ACK, and you're waiting for that ACK before sending the next packet), so it's not "false"/"true", but really just an integer mod 2.

+    /**
+     * Result of this transmission. Only valid after wait_transaction() has
+     * returned.
+     */
+    cdi_usb_transmission_result_t* result;
I won't say that this patch is right, but it looks plausible at least. ;-)

That's good enough, I suppose. :-) Thanks!

I'll change the types and remove cdi_usb_provide_hc().