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 <masa-korg@xxxxxxxxxxxxxxx>
> ---
> 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 <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/ioport.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/smp_lock.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/timer.h>
> +#include <linux/list.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioctl.h>
> +#include <linux/fs.h>
> +#include <linux/dmapool.h>
> +#include <linux/moduleparam.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +
> +#include <asm/byteorder.h>
> +#include <asm/system.h>
> +#include <asm/unaligned.h>
> +
> +/* gadget stack */
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +#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
--
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
