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

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



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
cdi_usb_driver?

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).

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?

> +/**
> + * 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).

> +
> +/**
> + * 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,
too.

> +    /**
> +     * 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)

> +    /**
> +     * 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. ;-)

Kevin