>[...]
>> @@ -1607,6 +1608,11 @@ void usb_disconnect(struct usb_device **pdev)
>> */
>> device_del(&udev->dev);
>>
>> + /* Notify OTG disconnect event, only for USB device which is
>> + * directly connected to root hub */
>
> The preferred style of multi-line comments is this:
>
>/*
> * bla
> * bla
> */
>
Thanks for your help on code view.
Ok, will fix it.
>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> index 502a7e6..769a957 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>[...]
>> diff --git a/drivers/usb/host/ehci-intel-mid-pci.c
>b/drivers/usb/host/ehci-intel-mid-pci.c
>> new file mode 100644
>> index 0000000..b05bc8a
>> --- /dev/null
>> +++ b/drivers/usb/host/ehci-intel-mid-pci.c
>> @@ -0,0 +1,188 @@
>[...]
>> +static int ehci_mid_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *id)
>> +{
>[...]
>> + hcd = usb_create_hcd(driver,&pdev->dev, dev_name(&pdev->dev));
>> + if (!hcd) {
>> + retval = -ENOMEM;
>> + goto err1;
>> + }
>> +
>> + hcd->self.otg_port = 1;
>> + ehci = hcd_to_ehci(hcd);
>> +
>> + otg = otg_get_transceiver();
>> + if (otg == NULL) {
>> + printk(KERN_ERR "%s Failed to get otg transceiver\n", __func__);
>> + retval = -EINVAL;
>> + goto err1;
>
> Shouldn't you also call usb_put_hcd() in this case?
Usb_put_hcd is called in ehci_mid_remove function. Please see my explain for usb_put_hcd/otg_put_transceiver below.
>
>> + if (hcd->regs == NULL) {
>> + dev_dbg(&pdev->dev, "error mapping memory\n");
>> + retval = -EFAULT;
>
> Rather -ENOMEM.
I prefer to keep EFAULT here. Ehci-intel-mid-pci gets the regs base address from penwell_otg transceiver driver.
So if it is NULL, it should be a EFAULT to ehci-intel-mid-pci. Do you see if it is fine for this?
>
>> + goto err2;
>> + }
>> + retval = usb_add_hcd(hcd, irq, IRQF_DISABLED | IRQF_SHARED);
>
> IRQF_DISABLED and IRQF_SHARED together?
Yes, it is shared interrupt, but there is only one hardware (otg controller), and transceiver driver/ ehci host driver / udc driver shared the irq line.
At same time, looks like IRQF_DISABLED is DEPRECATED now, I will check if it can be removed directly.
>
>> + otg_put_transceiver(otg);
>
> Er, shouldn't this be called in ehci_mid_remove() instead?
For hcd, ehci-intel-mid-pci will get hcd in probe function, and do usb_put_hcd() in remove function.
But for otg_transceiver, it is a little different, ehci-intel-mid-pci only get the otg_transceiver when it needs to do some otg related action/notification, and after that put otg_transceiver again.
It is no need to hold otg_transceiver in all ehci-intel-mid-pci life cycle.
>> +err2:
>> + usb_put_hcd(hcd);
>> +err1:
>> + dev_err(&pdev->dev, "init %s fail, %d\n", dev_name(&pdev->dev), retval);
>
> dev_err() already prints dev_name(&pdev->dev).
> s/fail/failed/
Yes, you are correct, will fix it.
>
>> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
>> index a1e8d27..455f0aa 100644
>> --- a/drivers/usb/host/ehci-pci.c
>> +++ b/drivers/usb/host/ehci-pci.c
>> @@ -50,6 +50,7 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
>> u8 rev;
>> u32 temp;
>> int retval;
>> + int force_otg_hc_mode = 0;
>>
>> switch (pdev->vendor) {
>> case PCI_VENDOR_ID_TOSHIBA_2:
>> @@ -63,6 +64,17 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
>> #endif
>> }
>> break;
>> + case PCI_VENDOR_ID_INTEL:
>> + if (pdev->device == 0x0811) {
>> + ehci_info(ehci, "Detected Langwell OTG HC\n");
>> + hcd->has_tt = 1;
>> + ehci->has_hostpc = 1;
>> + force_otg_hc_mode = 1;
>> + } else if (pdev->device == 0x0806) {
>> + ehci_info(ehci, "Detected Langwell MPH\n");
>> + hcd->has_tt = 1;
>> + ehci->has_hostpc = 1;
>
> *switch* statement would be more natural here. And it will help avoid code
>duplication.
Hm.. let me try with *switch*. Thanks for your comment.
Hao
--
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
