Re: [PATCH] USB device driver of Topcliff PCH

[Greg KH <gregkh@suse.de>]

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@dsn.okisemi.com>
> ---
>  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