lkml.org 
[lkml]   [2008]   [Dec]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] USB: Driver for Freescale QUICC Engine USB Host Controller
    On Wed, 24 Dec 2008 00:03:22 +0300
    Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

    > This patch adds support for the FHCI USB controller, as found
    > in the Freescale MPC836x and MPC832x processors. It can support
    > Full or Low speed modes.
    >
    > Quite a lot the hardware is doing by itself (SOF generation, CRC
    > generation and checking), though scheduling and retransmission is on
    > software's shoulders.
    >
    > This controller does not integrate the root hub, so this driver also
    > fakes one-port hub. External hub is required to support more than
    > one device.
    >

    <quick scan>

    Nice-looking driver. But the namespace pollution is tremendous!

    >
    > ...
    >
    > +static int fhci_mem_init(struct fhci_hcd *fhci)
    > +{
    > + int i;
    > + int error = 0;
    > +
    > + fhci->hc_list = kzalloc(sizeof(*fhci->hc_list), GFP_KERNEL);
    > + if (!fhci->hc_list)
    > + return -ENOMEM;
    > +
    > + INIT_LIST_HEAD(&fhci->hc_list->ctrl_list);
    > + INIT_LIST_HEAD(&fhci->hc_list->bulk_list);
    > + INIT_LIST_HEAD(&fhci->hc_list->iso_list);
    > + INIT_LIST_HEAD(&fhci->hc_list->intr_list);
    > + INIT_LIST_HEAD(&fhci->hc_list->done_list);
    > +
    > + fhci->vroot_hub = kzalloc(sizeof(*fhci->vroot_hub), GFP_KERNEL);
    > + if (!fhci->vroot_hub)
    > + return -ENOMEM;

    Did we leak fhci->hc_list?

    > + INIT_LIST_HEAD(&fhci->empty_eds);
    > + INIT_LIST_HEAD(&fhci->empty_tds);
    > +
    > + /* initialize work queue to handle done list */
    > + fhci_tasklet.data = (unsigned long)fhci;
    > + fhci->process_done_task = &fhci_tasklet;
    > +
    > + for (i = 0; i < MAX_TDS; i++) {
    > + struct td *td = kmalloc(sizeof(*td), GFP_KERNEL);
    > +
    > + if (!td) {
    > + error = 1;
    > + break;
    > + }
    > + recycle_empty_td(fhci, td);
    > + }
    > + for (i = 0; i < MAX_EDS; i++) {
    > + struct ed *ed = kmalloc(sizeof(*ed), GFP_KERNEL);
    > +
    > + if (!ed) {
    > + error = 1;
    > + break;
    > + }
    > + recycle_empty_ed(fhci, ed);
    > + }
    > +
    > + if (error) {
    > + fhci_mem_free(fhci);
    > + return -ENOMEM;
    > + }
    > +
    > + fhci->active_urbs = 0;
    > +
    > + return error;
    > +}
    > +
    >
    > ...
    >
    > +void config_transceiver(struct fhci_hcd *fhci, enum fhci_port_status status)

    poorly-chosen global identifier.

    > +{
    > + fhci_dbg(fhci, "-> %s: %d\n", __func__, status);
    > +
    > + switch (status) {
    > + case FHCI_PORT_POWER_OFF:
    > + fhci_gpio_set_value(fhci, GPIO_POWER, false);
    > + break;
    > + case FHCI_PORT_DISABLED:
    > + case FHCI_PORT_WAITING:
    > + fhci_gpio_set_value(fhci, GPIO_POWER, true);
    > + break;
    > + case FHCI_PORT_LOW:
    > + fhci_gpio_set_value(fhci, GPIO_SPEED, false);
    > + break;
    > + case FHCI_PORT_FULL:
    > + fhci_gpio_set_value(fhci, GPIO_SPEED, true);
    > + break;
    > + default:
    > + WARN_ON(1);
    > + break;
    > + }
    > +
    > + fhci_dbg(fhci, "<- %s: %d\n", __func__, status);
    > +}
    > +
    > +/* disable the USB port by clearing the EN bit in the USBMOD register */
    > +void usb_port_disable(struct fhci_hcd *fhci)

    another

    > +{
    > + struct fhci_usb *usb = (struct fhci_usb *)fhci->usb_lld;
    > + enum fhci_port_status port_status;
    > +
    > + fhci_dbg(fhci, "-> %s\n", __func__);
    > +
    > + fhci_stop_sof_timer(fhci);
    > +
    > + flush_all_transmissions(usb);
    > +
    > + fhci_usb_disable_interrupt((struct fhci_usb *)fhci->usb_lld);
    > + port_status = usb->port_status;
    > + usb->port_status = FHCI_PORT_DISABLED;
    > +
    > + /* Enable IDLE since we want to know if something comes along */
    > + usb->saved_msk |= USB_E_IDLE_MASK;
    > + out_be16(&usb->fhci->regs->usb_mask, usb->saved_msk);
    > +
    > + /* check if during the disconnection process attached new device */
    > + if (port_status == FHCI_PORT_WAITING)
    > + device_connected_interrupt(fhci);
    > + usb->vroot_hub->port.wPortStatus &= ~USB_PORT_STAT_ENABLE;
    > + usb->vroot_hub->port.wPortChange |= USB_PORT_STAT_C_ENABLE;
    > + fhci_usb_enable_interrupt((struct fhci_usb *)fhci->usb_lld);
    > +
    > + fhci_dbg(fhci, "<- %s\n", __func__);
    > +}
    > +
    > +/* enable the USB port by setting the EN bit in the USBMOD register */
    > +void usb_port_enable(void *lld)

    another. Please review all non-static symbols which this driver adds.

    > +{
    > + struct fhci_usb *usb = (struct fhci_usb *)lld;
    > + struct fhci_hcd *fhci = usb->fhci;
    > +
    > + fhci_dbg(fhci, "-> %s\n", __func__);
    > +
    > + config_transceiver(fhci, usb->port_status);
    > +
    > + if ((usb->port_status != FHCI_PORT_FULL) &&
    > + (usb->port_status != FHCI_PORT_LOW))
    > + fhci_start_sof_timer(fhci);
    > +
    > + usb->vroot_hub->port.wPortStatus |= USB_PORT_STAT_ENABLE;
    > + usb->vroot_hub->port.wPortChange |= USB_PORT_STAT_C_ENABLE;
    > +
    > + fhci_dbg(fhci, "<- %s\n", __func__);
    > +}
    > +
    >
    > ...
    >
    > +static struct td *get_empty_td(struct fhci_hcd *fhci)
    > +{
    > + struct td *td;
    > +
    > + if (!list_empty(&fhci->empty_tds)) {
    > + td = list_entry(fhci->empty_tds.next, struct td, node);
    > + list_del(fhci->empty_tds.next);
    > + } else {
    > + td = kmalloc(sizeof(*td), GFP_ATOMIC);
    > + if (!td)
    > + fhci_err(fhci, "No memory to allocate to TD\n");
    > + else
    > + init_td(td);
    > + }
    > +
    > + return td;
    > +}
    > +
    > +void recycle_empty_td(struct fhci_hcd *fhci, struct td *td)
    > +{
    > + init_td(td);
    > + list_add(&td->node, &fhci->empty_tds);
    > +}
    > +
    > +struct ed *get_empty_ed(struct fhci_hcd *fhci)
    > +{
    > + struct ed *ed;
    > +
    > + if (!list_empty(&fhci->empty_eds)) {
    > + ed = list_entry(fhci->empty_eds.next, struct ed, node);
    > + list_del(fhci->empty_eds.next);
    > + } else {
    > + ed = kmalloc(sizeof(*ed), GFP_ATOMIC);
    > + if (!ed)
    > + fhci_err(fhci, "No memory to allocate to ED\n");
    > + else
    > + init_ed(ed);
    > + }
    > +
    > + return ed;
    > +}

    The GFP_ATOMICs here are regrettable. Are these functions ever called
    from a context in which a more reliable allocation mode can be used?
    If so, the caller should pass in the gfp_t.

    >
    > ...
    >
    > +u32 create_endpoint(struct fhci_usb *usb, enum fhci_mem_alloc data_mem,
    > + u32 ring_len)

    This is a poorly chosen global identifier. fhci_create_endpoint()
    would be better.




    \
     
     \ /
      Last update: 2008-12-23 22:45    [W:0.036 / U:29.896 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site