| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8A42379416420646B9BFAC9682273B6D0649D8D0@limkexm3.ad.analog.com>
Date: Thu, 6 Nov 2008 19:29:30 -0000
From: "Hennerich, Michael" <Michael.Hennerich@...log.com>
To: "Sebastian Andrzej Siewior" <bigeasy@...utronix.de>,
"Bryan Wu" <cooloney@...nel.org>
Cc: <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
"Michael Hennerich" <michael.hennerich@...log.com>
Subject: RE: [PATCH] USB/ISP1760: Add support for the generic platfrom device centralized driver model
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@...utronix.de]
>Sent: Thursday, November 06, 2008 6:57 PM
>To: Bryan Wu
>Cc: linux-usb@...r.kernel.org; linux-kernel@...r.kernel.org; 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@...log.com>
>>
>>Signed-off-by: Michael Hennerich <michael.hennerich@...log.com>
>>Signed-off-by: Bryan Wu <cooloney@...nel.org>
>>---
>> 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@...utronix.de>
>> *
>>@@ -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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists