RE: [PATCH] USB/ISP1760: Add support for the generic platfrom device centralized driver model

[Hennerich, Michael]

Sebastian,

Thanks for your feedback, see my comments below.
We will resubmit a patch soon.

Thanks and best regards,
Michael


>-----Original Message-----
>From: Sebastian Andrzej Siewior [mailto:bigeasy@xxxxxxxxxxxxx]
>Sent: Thursday, November 06, 2008 6:57 PM
>To: Bryan Wu
>Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michael
>Hennerich
>Subject: Re: [PATCH] USB/ISP1760: Add support for the generic platfrom
>device centralized driver model
>
>* Bryan Wu | 2008-11-06 17:25:49 [+0800]:
>
>>From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>>
>>Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>>Signed-off-by: Bryan Wu <cooloney@xxxxxxxxxx>
>>---
>> drivers/usb/host/isp1760-if.c |   98
>+++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 98 insertions(+), 0 deletions(-)
>>
>>diff --git a/drivers/usb/host/isp1760-if.c
b/drivers/usb/host/isp1760-if.c
>>index af849f5..dc16698 100644
>>--- a/drivers/usb/host/isp1760-if.c
>>+++ b/drivers/usb/host/isp1760-if.c
>>@@ -3,6 +3,7 @@
>>  * Currently there is support for
>>  * - OpenFirmware
>>  * - PCI
>>+ * - PDEV (generic platform device centralized driver model)
>>  *
>>  * (c) 2007 Sebastian Siewior <bigeasy@xxxxxxxxxxxxx>
>>  *
>>@@ -23,6 +24,12 @@
>> #include <linux/pci.h>
>> #endif
>>
>>+#if !defined(CONFIG_USB_ISP1760_OF) &&
!defined(CONFIG_USB_ISP1760_PCI)
>>+#define USB_ISP1760_PDEV
>Usually I would prefer to make it conditional on
>CONFIGU_USB_ISP1760_PDEV. But since
>http://marc.info/?l=linux-usb&m=122563596420156&w=2 I would prefer to
>have unconditional.
>Any reason why you only enable it PDEV if you have neiher PCI nor OF?


Originally I had this kconfig option, but later decided to toss it.
Why would you use pdev if you have PCI or OF, was my argument... :-)
I'll add it back.   

>
>>+#include <linux/platform_device.h>
>>+#include <linux/usb/isp1760.h>
>You can't include files which are not mainline

My tree features this file.
It simply misses in this patch.


>
>>+#endif
>>+
>> #ifdef CONFIG_USB_ISP1760_OF
>> static int of_isp1760_probe(struct of_device *dev,
>> 		const struct of_device_id *match)
>>@@ -286,12 +293,100 @@ static struct pci_driver isp1761_pci_driver = {
>> };
>> #endif
>>
>>+#ifdef USB_ISP1760_PDEV
>>+static int __devinit
>>+isp1760_pdev_probe(struct platform_device *pdev)
>>+{
>Please make it
>static int __devinit isp1760_pdev_probe(struct platform_device *pdev)

I see - single line...

>
>>+	struct usb_hcd *hcd;
>>+	struct resource	*addr;
>>+	int irq;
>>+	unsigned int devflags = 0;
>>+	struct isp1760_platform_data *priv = pdev->dev.platform_data;
>>+
>>+	/* basic sanity checks first.  board-specific init logic should
>>+	 * have initialized these two resources and probably board
>>+	 * specific platform_data.  we don't probe for IRQs, and do only
>>+	 * minimal sanity checking.
>>+	 */
>>+
>>+	if (usb_disabled())
>>+		return -ENODEV;
>>+
>>+	if (pdev->num_resources < 2)
>>+		return -ENODEV;
>>+
>>+	addr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>+	irq = platform_get_irq(pdev, 0);
>>+
>>+	if (!addr || irq < 0)
>>+		return -ENODEV;
>>+
>>+	if (priv) {
>>+		if (priv->is_isp1761)
>>+			devflags |= ISP1760_FLAG_ISP1761;
>>+		if (priv->port1_disable)
>>+			devflags |= ISP1760_FLAG_PORT1_DIS;
>>+		if (priv->bus_width_16)
>>+			devflags |= ISP1760_FLAG_BUS_WIDTH_16;
>>+		if (priv->port1_otg)
>>+			devflags |= ISP1760_FLAG_OTG_EN;
>>+		if (priv->analog_oc)
>>+			devflags |= ISP1760_FLAG_ANALOG_OC;
>>+		if (priv->dack_polarity_high)
>>+			devflags |= ISP1760_FLAG_DACK_POL_HIGH;
>>+		if (priv->dreq_polarity_high)
>>+			devflags |= ISP1760_FLAG_DREQ_POL_HIGH;
>>+	}
>>+
>>+	hcd = isp1760_register(addr->start, resource_size(addr), irq,
>>+		IRQF_DISABLED | IRQF_SHARED | IRQF_TRIGGER_FALLING,
>This won't work. The chip itself is configured as level, active low.
You
>have to make sure the chip is configured as edge as well.

Well it somehow worked - I'll fix it.

>
>>+		&pdev->dev, dev_name(&pdev->dev),
>>+		devflags);
>>+
>>+	if (IS_ERR(hcd))
>>+		return PTR_ERR(hcd);
>>+
>>+	return 0;
>>+}
>>+
>>+static int __devexit
>>+isp1760_pdev_remove(struct platform_device *pdev)
>>+{
>>+	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>>+
>>+	platform_set_drvdata(pdev, NULL);
>>+
>>+	usb_remove_hcd(hcd);
>>+	iounmap(hcd->regs);
>>+	usb_put_hcd(hcd);
>>+	return 0;
>>+}
>>+
>>+/* this driver is exported so sl811_cs can depend on it */
>What are you telling me here?

Noting - I'll remove it

>
>>+struct platform_driver isp1760_pdev_driver = {
>>+	.probe =	isp1760_pdev_probe,
>>+	.remove =	__devexit_p(isp1760_pdev_remove),
>>+	.driver = {
>>+		.name =	"isp1760-hcd",
>>+		.owner = THIS_MODULE,
>>+	},
>>+};
>>+#endif /* USB_ISP1760_PDEV */
>>+
>> static int __init isp1760_init(void)
>> {
>> 	int ret = -ENODEV;
>>
>> 	init_kmem_once();
>>
>>+#ifdef USB_ISP1760_PDEV
>>+	ret = platform_driver_register(&isp1760_pdev_driver);
>>+	if (ret) {
>>+		deinit_kmem_cache();
>>+		return ret;
>>+	}
>>+#endif
>>+
>> #ifdef CONFIG_USB_ISP1760_OF
>> 	ret = of_register_platform_driver(&isp1760_of_driver);
>> 	if (ret) {
>>@@ -325,6 +420,9 @@ static void __exit isp1760_exit(void)
>> #ifdef CONFIG_USB_ISP1760_PCI
>> 	pci_unregister_driver(&isp1761_pci_driver);
>> #endif
>>+#ifdef USB_ISP1760_PDEV
>>+	platform_driver_unregister(&isp1760_pdev_driver);
>>+#endif
>> 	deinit_kmem_cache();
>> }
>> module_exit(isp1760_exit);
>>--
>>1.5.6.3
>
>Sebastian
--
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/