Re: [PATCH v1 6/9] usb: xhci: Add NVIDIA Tegra XHCI host-controller driver

[Andrew Bresticker]

On Fri, Jun 20, 2014 at 9:58 AM, Julius Werner <jwerner@xxxxxxxxxxxx> wrote:
>> +static const struct hc_driver tegra_xhci_hc_driver = {
>> +       .description =          "tegra-xhci-hcd",
>> +       .product_desc =         "Tegra xHCI Host Controller",
>> +       .hcd_priv_size =        sizeof(struct xhci_hcd *),
>> +
>> +       /*
>> +        * generic hardware linkage
>> +        */
>> +       .irq =                  xhci_irq,
>> +       .flags =                HCD_MEMORY | HCD_USB3 | HCD_SHARED,
>> +
>> +       /*
>> +        * basic lifecycle operations
>> +        */
>> +       .reset =                tegra_xhci_setup,
>> +       .start =                xhci_run,
>> +       .stop =                 xhci_stop,
>> +       .shutdown =             xhci_shutdown,
>> +
>> +       /*
>> +        * managing i/o requests and associated device resources
>> +        */
>> +       .urb_enqueue =          xhci_urb_enqueue,
>> +       .urb_dequeue =          xhci_urb_dequeue,
>> +       .alloc_dev =            xhci_alloc_dev,
>> +       .free_dev =             xhci_free_dev,
>> +       .alloc_streams =        xhci_alloc_streams,
>> +       .free_streams =         xhci_free_streams,
>> +       .add_endpoint =         xhci_add_endpoint,
>> +       .drop_endpoint =        xhci_drop_endpoint,
>> +       .endpoint_reset =       xhci_endpoint_reset,
>> +       .check_bandwidth =      xhci_check_bandwidth,
>> +       .reset_bandwidth =      xhci_reset_bandwidth,
>> +       .address_device =       xhci_address_device,
>> +       .enable_device =        xhci_enable_device,
>> +       .update_hub_device =    xhci_update_hub_device,
>> +       .reset_device =         xhci_discover_or_reset_device,
>> +
>> +       /*
>> +        * scheduling support
>> +        */
>> +       .get_frame_number =     xhci_get_frame,
>> +
>> +       /* Root hub support */
>> +       .hub_control =          xhci_hub_control,
>> +       .hub_status_data =      xhci_hub_status_data,
>> +       .bus_suspend =          xhci_bus_suspend,
>> +       .bus_resume =           xhci_bus_resume,
>> +};
>
> I know I missed the first round of discussion where this was
> suggested, but I don't think it's a good idea to pull the whole
> hc_driver structure out into every platform implementation. It will
> lead to duplication, then to future additions only being applied to
> some of the implementations and everything getting out of sync. This
> is already a problem with the PCI/plat split (e.g. the LPM functions
> were only added to xhci-pci even though they should apply to both).
> Also, if I'm not mistaken this code would fail to compile as a module
> (you are referencing lots of symbols that are internal to the xhci-hcd
> module).

You're right Julius, this won't build as a module without a few EXPORT_SYMBOLs.

> I think at the very least you should add a function
> "xhci_default_driver(struct hc_driver *driver)" to xhci-plat.c (or
> even better to xhci.c and use it for PCI as well) that initializes all
> function pointers to the default (internal) symbols, and can then be
> overridden afterwards.

Currently all XHCI host drivers (PCI, platform, MVEBU) will be built
into the xhci-hcd module.  I could append the Tegra driver to that
module or introduce a xhci_init_driver() like EHCI does as Julius
suggests.  USB folks, do you have a preference?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/