From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757123Ab0IHA0E (ORCPT ); Tue, 7 Sep 2010 20:26:04 -0400 Received: from cantor2.suse.de ([195.135.220.15]:53701 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757473Ab0IHAZ6 (ORCPT ); Tue, 7 Sep 2010 20:25:58 -0400 Date: Tue, 7 Sep 2010 17:22:29 -0700 From: Greg KH To: Masayuki Ohtake Cc: Randy Dunlap , Peter Korsgaard , Nicolas Ferre , Michal Nazarewicz , Maurus Cuelenaere , linux-usb , Laurent Pinchart , Fabien Chouteau , david Brownell , Christoph Egger , LKML , MeeGo , "Wang, Qi" , "Wang, Yong Y" , Andrew , Intel OTC , "Foster, Margie" , Arjan , Toshiharu Okada , Takahiro Shimizu , Tomoya Morinaga Subject: Re: [PATCH] USB device driver of Topcliff PCH Message-ID: <20100908002229.GC15785@suse.de> References: <4C85EE6F.6000706@dsn.okisemi.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4C85EE6F.6000706@dsn.okisemi.com> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 07, 2010 at 04:49:03PM +0900, Masayuki Ohtake wrote: > This patch adds the USB device driver of Topcliff PCH. > > Topcliff PCH is the platform controller hub that is going to be used in > Intel's upcoming general embedded platform. All IO peripherals in > Topcliff PCH are actually devices sitting on AMBA bus. > Topcliff PCH has USB device I/F. Using this I/F, it is able to access system > devices connected to USB device. > > Signed-off-by: Masayuki Ohtake > --- > drivers/usb/gadget/Kconfig | 24 + > drivers/usb/gadget/Makefile | 2 +- > drivers/usb/gadget/gadget_chips.h | 7 + > drivers/usb/gadget/pch_udc.c | 3153 +++++++++++++++++++++++++++++++++++++ > drivers/usb/gadget/pch_udc.h | 495 ++++++ > 5 files changed, 3680 insertions(+), 1 deletions(-) > create mode 100755 drivers/usb/gadget/pch_udc.c > create mode 100755 drivers/usb/gadget/pch_udc.h Heh, really? executable .c and .h files? I think you need to look at your development system :) > > diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig > index 591ae9f..3bc839d 100644 > --- a/drivers/usb/gadget/Kconfig > +++ b/drivers/usb/gadget/Kconfig > @@ -220,6 +220,30 @@ config USB_OTG > > Select this only if your OMAP board has a Mini-AB connector. > > +config USB_GADGET_PCH > + boolean "PCH USB Dev" Please spell this out a lot more as to what it is. > + depends on PCI > + select USB_GADGET_DUALSPEED > + help > + This is a USB device driver for Topcliff PCH. > + Topcliff PCH is the platform controller hub that is used in Intel's > + general embedded platform. > + Topcliff PCH has USB device interface. > + Using this interface, it is able to access system devices connected > + to USB device. > + This driver enables USB device function. > + USB device is a USB peripheral controller which > + supports both full and high speed USB 2.0 data transfers. > + This driver supports for control transfer, and bulk transfer modes. > + This driver dose not support interrupt transfer, and isochronous > + transfer modes. > + > +config PCH_USBDEV > + tristate > + depends on USB_GADGET_PCH > + default USB_GADGET > + select USB_GADGET_SELECTED > + > config USB_GADGET_PXA25X > boolean "PXA 25x or IXP 4xx" > depends on (ARCH_PXA && PXA25x) || ARCH_IXP4XX > diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile > index 9bcde11..9473430 100644 > --- a/drivers/usb/gadget/Makefile > +++ b/drivers/usb/gadget/Makefile > @@ -63,4 +63,4 @@ obj-$(CONFIG_USB_G_HID) += g_hid.o > obj-$(CONFIG_USB_G_MULTI) += g_multi.o > obj-$(CONFIG_USB_G_NOKIA) += g_nokia.o > obj-$(CONFIG_USB_G_WEBCAM) += g_webcam.o > - > +obj-$(CONFIG_PCH_USBDEV) += pch_udc.o > diff --git a/drivers/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h > index e511fec..f67dd29 100644 > --- a/drivers/usb/gadget/gadget_chips.h > +++ b/drivers/usb/gadget/gadget_chips.h > @@ -142,6 +142,11 @@ > #define gadget_is_s3c_hsotg(g) 0 > #endif > > +#ifdef CONFIG_USB_GADGET_PCH > +#define gadget_is_pch(g) (!strcmp("pch_udc", (g)->name)) > +#else > +#define gadget_is_pch(g) 0 > +#endif > > /** > * usb_gadget_controller_number - support bcdDevice id convention > @@ -200,6 +205,8 @@ static inline int usb_gadget_controller_number(struct usb_gadget *gadget) > return 0x25; > else if (gadget_is_s3c_hsotg(gadget)) > return 0x26; > + else if (gadget_is_pch(gadget)) > + return 0x27; > return -ENOENT; > } > > diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c > new file mode 100755 > index 0000000..2065c11 > --- /dev/null > +++ b/drivers/usb/gadget/pch_udc.c > @@ -0,0 +1,3153 @@ > +/* > + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; version 2 of the License. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307, USA. > + */ > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +/* gadget stack */ > +#include > +#include > +#include "pch_udc.h" Why do you need a .h file? > +/* macro to set a specified bit(mask) at the specified address */ > +#define PCH_UDC_BIT_SET(reg, bitmask) \ > + iowrite32(((ioread32((reg)) | (bitmask))), (reg)) > +/* macro to clear a specified bit(mask) at the specified address */ > +#define PCH_UDC_BIT_CLR(reg, bitMask) \ > + iowrite32((ioread32((reg)) & (~(bitMask))), (reg)) Why not just use the iowrite32 functions directly? Or surely there's another built-in kernel macro for this somewhere, right? > +#define MAX_LOOP 200 > +#define PCH_UDC_PCI_BAR 1 > +#define PCI_VENDOR_ID_INTEL 0x8086 No need to define this, it's in pci_ids.h already. > +#define PCI_DEVICE_ID_INTEL_PCH1_UDC 0x8808 I think this is there too. > + > +const char ep0_string[] = "ep0in"; > +static DEFINE_SPINLOCK(udc_stall_spinlock); /* stall spin lock */ > +static u32 pch_udc_base; > +static union pch_udc_setup_data setup_data; /* received setup data */ > +static unsigned long ep0out_buf[64]; > +static dma_addr_t dma_addr; > +struct pch_udc_dev *pch_udc; /* pointer to device object */ > +int speed_fs; Why are 2 of these not static variables? Have you run this driver through the sparse tool? It will catch these errors. Please do so. thanks, greg k-h