Hi,
On Sat, Feb 09, 2013 at 07:37:51PM -0800, Paul Zimmerman wrote:
> +static struct dwc2_core_params dwc2_module_params = {
> + .otg_cap = 0, /* HNP/SRP capable */
> + .dma_enable = 1,
> + .dma_desc_enable = 1,
> + .speed = 0, /* High Speed */
> + .host_support_fs_ls_low_power = 0,
> + .host_ls_low_power_phy_clk = 0, /* 48 MHz */
> + .enable_dynamic_fifo = 1,
> + .host_rx_fifo_size = 1024, /* 1K DWORDs */
> + .host_nperio_tx_fifo_size = 256, /* 256 DWORDs */
> + .host_perio_tx_fifo_size = 1024, /* 1K DWORDs */
> + .max_transfer_size = 65535,
> + .max_packet_count = 511,
> + .host_channels = 10,
> + .phy_type = 1, /* UTMI */
> + .phy_utmi_width = 16, /* 16 bits */
> + .phy_ulpi_ddr = 0, /* Single */
> + .phy_ulpi_ext_vbus = 0,
> + .i2c_enable = 0,
> + .ulpi_fs_ls = 0,
> + .ts_dline = 0,
> + .en_multiple_tx_fifo = 1,
> + .lpm_enable = 0,
> + .ic_usb_cap = 0,
> + .reload_ctl = 0,
> + .ahb_single = 0,
> + .otg_ver = 0, /* 1.3 */
> +};
wow!! Don't you have some of the GHWPARAMS registers as you do on dwc3 ?
> +static irqreturn_t dwc2_common_irq(int irq, void *dev)
> +{
> + int retval = dwc2_handle_common_intr(dev);
> +
> + return IRQ_RETVAL(retval);
> +}
IMO this shouldn't be here, dwc2 itself should request the IRQ.
> +static int dwc2_driver_probe(struct pci_dev *dev,
> + const struct pci_device_id *id)
> +{
> + struct dwc2_device *otg_dev;
> + struct resource *res;
> + int retval;
> +
> + dev_dbg(&dev->dev, "%s(%p)\n", __func__, dev);
> +
> + if (!id) {
id will never be NULL when probe() is called.
> + dev_err(&dev->dev, "Invalid pci_device_id %p", id);
> + return -EINVAL;
> + }
> +
> + if (!dev || pci_enable_device(dev) < 0) {
dev will never be NULL when your probe() is called.
Also, I think your pci_enable_device() could be done a little later
> + dev_err(&dev->dev, "Invalid pci_device %p", dev);
> + return -ENODEV;
> + }
> + dev_dbg(&dev->dev, "start=0x%08x\n",
> + (unsigned)pci_resource_start(dev, 0));
> +
> + otg_dev = devm_kzalloc(&dev->dev, sizeof(*otg_dev), GFP_KERNEL);
> + if (!otg_dev) {
> + dev_err(&dev->dev, "kzalloc of otg_dev failed\n");
> + return -ENOMEM;
leaves pci device enabled.
> + }
> +
> + /* Map the DWC_otg Core memory into virtual address space */
> + dev->current_state = PCI_D0;
> + dev->dev.power.power_state = PMSG_ON;
> +
> + if (!dev->irq) {
> + dev_err(&dev->dev,
> + "Found HC with no IRQ. Check BIOS/PCI %s setup!",
> + pci_name(dev));
> + return -ENODEV;
leaves pci device enabled.
> + }
> +
> + otg_dev->rsrc_start = pci_resource_start(dev, 0);
> + otg_dev->rsrc_len = pci_resource_len(dev, 0);
> + dev_dbg(&dev->dev, "PCI resource: start=%08x, len=%08x\n",
> + (unsigned)otg_dev->rsrc_start, (unsigned)otg_dev->rsrc_len);
> +
> + res = devm_request_mem_region(&dev->dev, otg_dev->rsrc_start,
> + otg_dev->rsrc_len, dev_name(&dev->dev));
> + if (!res) {
> + dev_dbg(&dev->dev, "error requesting register space\n");
> + return -EBUSY;
> + }
> +
> + otg_dev->base = devm_ioremap_nocache(&dev->dev, res->start,
> + resource_size(res));
request and ioremap can be done at one go with devm_ioremap_resource().
Can also drop the error messages as that will print one for you.
> + if ((dwc2_get_gsnpsid(otg_dev->base) & 0xfffff000) != 0x4f542000 &&
> + (dwc2_get_gsnpsid(otg_dev->base) & 0xfffff000) != 0x4f543000) {
> + dev_err(&dev->dev, "Bad value for SNPSID: 0x%08x\n",
> + dwc2_get_gsnpsid(otg_dev->base));
> + retval = -ENODEV;
> + goto fail;
> + }
look, this is one reason to make the PCI glue as tiny as possible...
Any piece of code that touches dwc2's registers, will be duplicated in
every single glue layer. When you have to maintain that, it will be a
big PITA.
> + retval = devm_request_irq(&dev->dev, dev->irq, dwc2_common_irq,
> + IRQF_SHARED | IRQF_DISABLED | IRQ_LEVEL,
IRQF_DISABLED is a nop, should be removed.
> +/*
> + * dwc2_driver_init() - Called when the DWC_otg driver is installed (e.g. with
> + * the modprobe command). It registers the dwc2_driver structure with the
> + * appropriate bus driver. This will cause the dwc2_driver_probe function to
> + * be called. In addition, the bus driver will automatically expose attributes
> + * defined for the device and driver in the special sysfs file system.
> + */
> +static int __init dwc2_driver_init(void)
> +{
> + int retval;
> +
> + pr_debug("%s loaded\n", dwc2_driver_name);
> +
> + retval = pci_register_driver(&dwc2_pci_driver);
> + if (retval)
> + pr_err("pci_register_driver() returned %d\n", retval);
> +
> + return retval;
> +}
> +module_init(dwc2_driver_init);
> +
> +/*
> + * dwc2_driver_exit() - Called when the driver is removed (e.g. with the
> + * rmmod command). The driver unregisters itself with its bus driver.
> + */
> +static void __exit dwc2_driver_exit(void)
> +{
> + pci_unregister_driver(&dwc2_pci_driver);
> +
> + pr_debug("%s removed\n", dwc2_driver_name);
> +}
> +module_exit(dwc2_driver_exit);
these two should be removed and you should just use:
module_pci_driver(dwc2_pci_driver);
--
balbi
Attachment:
signature.asc
Description: Digital signature
