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

Re: [cdi-devel] [PATCH v3 1/5] CDI.usb, CDI.usb-hcd



On Tue, Dec 22, 2009 at 04:41:14PM +0100, Kevin Wolf wrote:
> From: Max Reitz <max@xxxxxxxxxx>
> 
> + Added CDI.usb header
> + Added CDI.usb-hcd header
> 
> Signed-off-by: Max Reitz <max@xxxxxxxxxx>
> Signed-off-by: Kevin Wolf <kevin@xxxxxxxxxx>
> ---
>  include/cdi/usb-hcd.h |  178 ++++++++++
>  include/cdi/usb.h     |  868 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 1046 insertions(+), 0 deletions(-)
>  create mode 100644 include/cdi/usb-hcd.h
>  create mode 100644 include/cdi/usb.h
> 
> diff --git a/include/cdi/usb-hcd.h b/include/cdi/usb-hcd.h
> new file mode 100644
> index 0000000..60b0a01
> --- /dev/null
> +++ b/include/cdi/usb-hcd.h
> @@ -0,0 +1,178 @@
> +/*
> + * Copyright (c) 2009 Max Reitz
> + *
> + * This program is free software. It comes without any warranty, to
> + * the extent permitted by applicable law. You can redistribute it
> + * and/or modify it under the terms of the Do What The Fuck You Want
> + * To Public License, Version 2, as published by Sam Hocevar. See
> + * http://sam.zoy.org/projects/COPYING.WTFPL for more details.
> + */
> +
> +/**
> + * \if german
> + * USB-Hostcontrollertreiber
> + * \elseif english
> + * USB host controller drivers
> + * \endif
> + * \defgroup USBHCD
> + */
> +/*\@{*/
> +
> +#ifndef _CDI_USB_HCD_H_
> +#define _CDI_USB_HCD_H_
> +
> +#include "cdi/misc.h"
> +#include "cdi/usb.h"
> +
> +struct cdi_usb_hc {
> +    struct cdi_device dev;
> +
> +    /**
> +     * \if german
> +     * USB-Geschwindigkeit
> +     * \elseif english
> +     * USB speed
> +     * \endif
> +     */
> +    cdi_usb_speed_t speed;
> +};
> +
> +struct cdi_usb_hcd {
> +    struct cdi_driver drv;
> +
> +    /**
> +     * \if german
> +     *
> +     * Listet alle Geraete auf, die an den Rootports haengen und ruft ggf.
> +     * cdi_usb_device_init auf
> +     *
> +     * @param device Der Hostcontroller
> +     *
> +     * \elseif english
> +     *
> +     * Lists all devices hanging at the root ports and calls (if necessary)
> +     * cdi_usb_device_init
> +     *
> +     * @param device The host controller
> +     *
> +     * \endif
> +     */
> +    void (*find_devices)(struct cdi_usb_hc* device);
> +
> +    /**
> +     * \if german
> +     *
> +     * Aktiviert den zum angegebenen Geraet gehoerigen Rootport
> +     *
> +     * @param usb_device Das USB-Geraet (muss an einem Rootport haengen)
> +     *
> +     * \elseif english
> +     *
> +     * Activates the root port corresponding to the device
> +     *
> +     * @param usb_device The USB device (must be located at a root port)
> +     *
> +     * \endif
> +     */
> +    void (*activate_device)(struct cdi_usb_device* usb_device);
> +
> +    /**
> +     * \if german
> +     *
> +     * Verarbeitet USB-Pakete
> +     *
> +     * @param packets Array zu sendender Pakete
> +     * @param num_packets Anzahl der Pakete
> +     *
> +     * @return Gibt den Status an (z. B. USB_NO_ERROR fuer keinen Fehler)
> +     *
> +     * \elseif english
> +     *
> +     * Sends USB packets
> +     *
> +     * @param packets Array of packages to be sent
> +     * @param num_packages Number of packages
> +     *
> +     * \endif
> +     */
> +    cdi_usb_status_t (*send_packets)(struct cdi_usb_packet* packets,
> +        size_t num_packets);
> +
> +    /**
> +     * \if german
> +     *
> +     * Erstellt eine neue Pipe
> +     *
> +     * @param pipe Die Pipe
> +     *
> +     * \elseif english
> +     *
> +     * Creates a new pipe
> +     *
> +     * @param pipe The pipe
> +     *
> +     * \endif
> +     */
> +    void (*add_pipe)(struct cdi_usb_pipe* pipe);
> +};
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif

Any reason not to put this to the top of the file?

> +
> +/**
> + * \if german
> + *
> + * Initialisiert einen HCD
> + *
> + * @param driver Der HCD
> + *
> + * \elseif english
> + *
> + * Initializes an HCD
> + *
> + * @param driver The HCD
> + *
> + * \endif
> + */
> +void cdi_usb_hcd_init(struct cdi_usb_hcd* driver);
> +
> +/**
> + * \if german
> + *
> + * Gibt einen HCD frei
> + *
> + * @param driver Freizugebender HCD
> + *
> + * \elseif english
> + *
> + * Removes an HCD
> + *
> + * @param driver HCD to be removed
> + *
> + * \endif
> + */
> +void cdi_usb_hcd_destroy(struct cdi_usb_hcd* driver);
> +
> +/**
> + * \if german
> + *
> + * Registriert einen HCD
> + *
> + * @param driver Zu registrierender HCD
> + *
> + * \elseif english
> + *
> + * Registers an HCD
> + *
> + * @param driver HCD to be registered
> + *
> + * \endif
> + */
> +void cdi_usb_hcd_register(struct cdi_usb_hcd* driver);
> +
> +#ifdef __cplusplus
> +} //extern "C"
> +#endif
> +
> +#endif
> diff --git a/include/cdi/usb.h b/include/cdi/usb.h
> new file mode 100644
> index 0000000..54ce1a0
> --- /dev/null
> +++ b/include/cdi/usb.h
> @@ -0,0 +1,868 @@
> +/*
> + * Copyright (c) 2009 Max Reitz
> + *
> + * This program is free software. It comes without any warranty, to
> + * the extent permitted by applicable law. You can redistribute it
> + * and/or modify it under the terms of the Do What The Fuck You Want
> + * To Public License, Version 2, as published by Sam Hocevar. See
> + * http://sam.zoy.org/projects/COPYING.WTFPL for more details.
> + */
> +
> +/**
> + * \if german
> + * USB-Bustreiber
> + * \elseif english
> + * USB bus drivers
> + * \endif
> + * \defgroup USB
> + */
> +/*\@{*/
> +
> +#ifndef _CDI_USB_H_
> +#define _CDI_USB_H_
> +
> +#include "stddef.h"
> +#include "stdint.h"

These should have angled brackets.

> +
> +typedef enum {
> +    /// Low-Speed (1,5 Mbit/s)
> +    CDI_USB_LOW_SPEED   = 0x01,
> +    /// Full-Speed (12 Mbit/s)
> +    CDI_USB_FULL_SPEED  = 0x02,
> +    /// High-Speed (480 Mbit/s)
> +    CDI_USB_HIGH_SPEED  = 0x04,
> +    /// SuperSpeed (5 Gbit/s)
> +    CDI_USB_SUPER_SPEED = 0x08,
> +} cdi_usb_speed_t;
> +
> +typedef enum {
> +    /**
> +     * \if german
> +     * SETUP-Paket
> +     * \elseif english
> +     * SETUP packet
> +     * \endif
> +     */
> +    CDI_USB_PACKET_SETUP = 0x2D,
> +
> +    /**
> +     * \if german
> +     * Daten vom Geraet zum Host
> +     * \elseif english
> +     * Data from the device to the host
> +     * \endif
> +     */
> +    CDI_USB_PACKET_IN    = 0x69,
> +
> +    /**
> +     * \if german
> +     * Daten vom Host zum Geraet
> +     * \elseif english
> +     * Data from the host to the device
> +     * \endif
> +     */
> +    CDI_USB_PACKET_OUT   = 0xE1,
> +} cdi_usb_packet_type_t;
> +
> +typedef enum {
> +    /**
> +     * \if german
> +     * Kein Fehler
> +     * \elseif english
> +     * No error
> +     * \endif
> +     */
> +    CDI_USB_NO_ERROR      = 0x0000,
> +
> +    /// Stalled
> +    CDI_USB_STALLED       = 0x0001,
> +
> +    /**
> +     * \if german
> +     * Bufferfehler (Overflow, underrun, ...)
> +     * \elseif english
> +     * Buffer error (overflow, underrun, ...)
> +     * \endif
> +     */
> +    CDI_USB_BUFFER_ERROR  = 0x0002,
> +
> +    /**
> +     * \if german
> +     * Babble (Geraet hat zu viele Daten gesendet)
> +     * \elseif english
> +     * Babble (device has sent more data than expected)
> +     * \endif
> +     */
> +    CDI_USB_BABBLE        = 0x0004,
> +
> +    /**
> +     * \if german
> +     * NAK (Daten sind erneut zu senden)
> +     * \elseif english
> +     * NAK (data has to be sent again)
> +     * \endif
> +     */
> +    CDI_USB_NAK           = 0x0008,
> +
> +    /**
> +     * \if german
> +     * CRC-Fehler
> +     * \elseif english
> +     * CRC error
> +     * \endif
> +     */
> +    CDI_USB_CRC           = 0x0010,
> +
> +    /// Timeout
> +    CDI_USB_TIMEOUT       = 0x0020,
> +
> +    /**
> +     * \if german
> +     * Bitstuff/-destuff-Fehler
> +     * \elseif english
> +     * bitstuff/-destuff error
> +     * \endif
> +     */
> +    CDI_USB_BITSTUFF      = 0x0040,
> +
> +    /**
> +     * \if german
> +     * Reservieren von Speicher fehlgeschlagen, ...
> +     * \elseif english
> +     * Allocating of memory failed, ...
> +     * \endif
> +     */
> +    CDI_USB_TRIVIAL_ERROR = 0x0080,
> +} cdi_usb_status_t;
> +
> +struct cdi_usb_hc;
> +struct cdi_usb_pipe;
> +struct cdi_usb_interface;
> +struct cdi_usb_driver;
> +
> +struct cdi_usb_device {
> +    /**
> +     * \if german
> +     * Der USB-Bustreiber
> +     * \elseif english
> +     * The USB bus driver
> +     * \endif
> +     */
> +    struct cdi_usb_driver* drv;
> +
> +    /**
> +     * \if german
> +     * Der Hostcontroller, an dem das Geraet im Endeffekt haengt
> +     * \elseif english
> +     * The host controller where the device is eventually located
> +     * \endif
> +     */
> +    struct cdi_usb_hc* hc;

Such pointers have a very fundamental problem: The driver for the host
controller might run in a different process.

Maybe someone has a better suggestion, but the only options I can think
of are:

* Use a different struct here like struct cdi_usb_hc_link; its
  definition would be OS specific (assuming the USB driver doesn't need
  access to the fields but just some kind of ID for its HC)
* Use an IDC endpoint here. Obviously depends on someone implementing
  the IDC thing. Maybe we could go with the current pointer, put a FIXME
  there and replace it by an IDC endpoint later.

I think this definitely needs some discussion.

> +    /**
> +     * \if german
> +     * Haengt das Geraet an einem Hub, dann ist dieses Feld ein Pointer auf
> +     * das entsprechende USB-Geraet, haengt es an einem Rootport, dann ist
> +     * dieses Feld NULL.
> +     * \elseif english
> +     * If the device is located at a hub, then this field contains a pointer to
> +     * the corresponding USB device, if it's located at a root port, then this
> +     * field is set to NULL.
> +     * \endif
> +     */
> +    struct cdi_usb_device* hub;
> +
> +    /**
> +     * \if german
> +     * Geschwindigkeit(-en) dieses Geraets
> +     * \elseif english
> +     * Speed(-s) supported by this device
> +     * \endif
> +     */
> +    cdi_usb_speed_t speed;
> +
> +    /// ID
> +    int id;
> +
> +    /**
> +     * \if german
> +     * Entweder der Rootport oder der Port des Hubs, an dem das Geraet haengt
> +     * \elseif english
> +     * Either the root port or the port hub's port where the device is located
> +     * \endif
> +     */
> +    int port;
> +
> +    /**
> +     * \if german
> +     * Hier darf der Geraetetreiber seine Daten unterbringen
> +     * \elseif english
> +     * The device driver may store its data here
> +     * \endif
> +     */
> +    void* driver_data;

This is really ugly, but I can see why you put it there. Hope it'll go
away with the planned device creation patches. They should include some
notion of a bus (be it USB, PCI or whatever) that each device is on.

> +    /**
> +     * \if german
> +     * Felder, die so auch bei PCI existieren: Vendor-, Device-, Class-,
> +     * Subclass- und Protocol-ID
> +     * \elseif english
> +     * Fields existing at PCI, too: vendor, device, class, subclass and
> +     * protocol ID.
> +     * \endif
> +     */
> +    int vendor_id;
> +    int device_id;
> +    int class_id;
> +    int subclass_id;
> +    int protocol_id;

I guess these have a fixed size in fact and not only int?

> +
> +    /**
> +     * \if german
> +     *
> +     * Setzt das USB-Geraet zurueck (kann sowohl vom HCD als auch vom Hub
> +     * driver ausgefuehrt werden)
> +     *
> +     * @param usb_device Das Geraet
> +     *
> +     * \elseif english
> +     *
> +     * Resets the USB device (may be executed either by the HCD or the hub
> +     * driver)
> +     *
> +     * @param usb_device The device
> +     *
> +     * \endif
> +     */
> +    void (*reset)(struct cdi_usb_device* usb_device);
> +
> +    /**
> +     * \if german
> +     * Ziemlich lowlevelig. Gehoert das hier wirklich rein?
> +     * \elseif english
> +     * Very low level. Does it really fit in here?
> +     * \endif
> +     */

Probably doesn't fit here without a matching declaration. ;-)

> +    /**
> +     * \if german
> +     * Im einzelnen sind das: Pipe fuer Endpoint 0
> +     * \elseif english
> +     * In particular these things are: Pipe for endpoint 0
> +     * \endif
> +     */
> +    struct cdi_usb_pipe* ep0;
> +
> +    /// Device descriptor
> +    void* device_desc;
> +
> +    /// Configuration descriptor
> +    void* config_desc;
> +
> +    /// Interface descriptor
> +    struct cdi_usb_interface* interface_desc;
> +
> +    /// Class descriptor
> +    void* class_desc;
> +};