On Tue, 15 May 2012, Felipe Balbi wrote:
> > > Unrelated to $SUBJECT, but all those quirks could be passed through
> > > driver_data from pci ID table. That will remove the need for the switch
> > > statement. My 2 cents
> >
> > Many of them could, it's true. I don't know that it would be much of
> > an improvement, though. The code would still have to test for most of
> > the quirk flags because they require runtime checks (the one added by
> > this patch is an exception).
> >
> > Also, which do you think would occupy less memory space: an array of
> > pci_device_id structures or a switch statement jump table?
>
> The array gets freed after init with proper section annotations. I also
Then what happens if you unbind ehci-hcd from a controller and rebind
it later?
> don't understand why you say you still require runtime checks. The only
Let's look through the code. The first NVIDIA handler does this:
if (pci_set_consistent_dma_mask(pdev,
DMA_BIT_MASK(31)) < 0)
ehci_warn(ehci, "can't enable NVidia "
"workaround for >2GB RAM\n");
One of the clauses in the Intel and TDI handlers does this:
hcd->has_tt = 1;
tdi_reset(ehci);
The AMD handler does this:
if (usb_amd_find_chipset_info())
ehci->amd_pll_fix = 1;
I could go on. The point is that all of these involve more than just
copying flag bits.
> problem I see is that the way the *HCI PCI drivers were written, we
> do all quirk checks too late and don't have access to the pci_device_id
> pointer anymore. Still, all your switch jump table could be converted
> into something like:
>
> ehci->no_selective_suspend = data->no_selective_suspend;
> ehci->has_fsl_port_bug = data->has_fsl_port_bug;
> ehci->big_endian_desc = data->big_endian_desc;
> ehci->big_endian_capbase = data->big_endian_capbase;
> ehci->has_amcc_usb23 = data->has_amcc_usb23;
> ehci->need_io_watchdog = data->need_io_watchdog;
> ehci->broken_periodic = data->broken_periodic;
> ehci->amd_pll_fix = data->amd_pll_fix;
> ehci->fs_i_thresh = data->fs_i_thresh;
> ehci->use_dummy_qh = data->use_dummy_qh;
> ehci->has_synopsys_hc_bug = data->has_synopsys_hc_bug;
> ehci->frame_index_bug = data->frame_index_bug;
>
> #ifdef CONFIG_USB_EHCI_BIG_ENDIAN_MMIO
> ehci->big_endian_mmio = data->big_endian_mmio;
> #endif
>
> Because all those field were properly initialized on pci_device_id. It
> would look something like:
>
> static const struct
> ehci_pci_driver_data ehci_intel_27cc_driver_data __devinitconst = {
> .fs_i_thresh = true,
> .broken_periodic = true,
> };
>
> static DEFINE_PCI_DEVICE_TABLE(pci_ids) = {
> PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x27cc)
> .driver_data = (kernel_ulong_t) &ehci_intel_27cc_driver_data,
> };
>
> or something similar. The only thing I saw was the VIA VT6212 which uses
> a different sleep time, but that register write can be done no matter if
> it's VIA or not... just write the 10usec to the register on all cases.
> It won't (shouldn't) do any harm.
See all the examples listed above. Your scheme won't handle them.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
