RE: [PATCH 01/17 v2] USB: mv_udc: refine the driver structure.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@xxxxxx]
> Sent: Thursday, September 29, 2011 7:15 PM
> To: Neil Zhang
> Cc: Chao Xie; gregkh@xxxxxxx; balbi@xxxxxx; bigeasy@xxxxxxxxxxxxx;
> linux-usb@xxxxxxxxxxxxxxx; Haojian Zhuang
> Subject: Re: [PATCH 01/17 v2] USB: mv_udc: refine the driver structure.
>
> Hi,
>
> (add -M -C flags to git format-patch)
>
> On Thu, Sep 22, 2011 at 03:03:48PM +0800, Neil Zhang wrote:
> > This patch do the following things:
> >
> > 1. Adjust the head files, move the common MACRO and type definition
> to
> >    include/linux/usb/mv_usb.h.
> > 2. Add mv_usb_platform_data structure, make the driver fits all the
> >    marvell platform using the same ChipIdea usb ip.
> > 3. Some SOC may has mutiple clock sources, so let me define it in
> >    mv_usb_platform_data and give two helper function to deal with the
> >    clock named udc_clock_enable/udc_clock_disable.
>
> ideally, drivers would not need to care about clocks. Don't you have
> pm_runtime support hiding clock operations from drivers ??
>
We haven't pm_runtime support at this time.

> > 4. Different SOC will have some difference in PHY initialization,
> >    so we will remove file mv_udc_phy.c and add two funtions in
> >    mv_usb_platform_data, let the platform relative driver to realize
> it.
>
> Ideally, you would provide your transceiver driver, rather than hacking
> it around.
>
Just like Sebastian said, people are trying to come up with a PHY layer.
We will try to switch over to this once it is merged.
But at this moment, it will not be so graceful to provide the transceiver
drivers for all Marvell platforms.

> btw, this is too big for my taste. Care to split it further ?
>

> > Signed-off-by: Neil Zhang <zhangwm@xxxxxxxxxxx>
> > ---
> >  arch/arm/plat-pxa/include/plat/usb.h |   44 +++++++
> >  drivers/usb/gadget/Makefile          |    2 +-
> >  drivers/usb/gadget/mv_udc.h          |  201 ++++++------------------
> --------
> >  drivers/usb/gadget/mv_udc_core.c     |  184 ++++++++++++++++++++----
> -----
> >  drivers/usb/gadget/mv_udc_phy.c      |  214 ------------------------
> ----------
> >  include/linux/usb/mv_usb.h           |  193
> ++++++++++++++++++++++++++++++
> >  6 files changed, 406 insertions(+), 432 deletions(-)  create mode
> > 100644 arch/arm/plat-pxa/include/plat/usb.h
> >  delete mode 100644 drivers/usb/gadget/mv_udc_phy.c  create mode
> > 100644 include/linux/usb/mv_usb.h
> >
> > diff --git a/arch/arm/plat-pxa/include/plat/usb.h
> > b/arch/arm/plat-pxa/include/plat/usb.h
> > new file mode 100644
> > index 0000000..2fa82a5
> > --- /dev/null
> > +++ b/arch/arm/plat-pxa/include/plat/usb.h
> > @@ -0,0 +1,44 @@
> > +/*
> > + * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
> > + *
> > + * 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;  either version 2 of the  License, or
> > +(at your
> > + * option) any later version.
> > + */
> > +
> > +#ifndef __MV_PLATFORM_USB_H
> > +#define __MV_PLATFORM_USB_H
> > +
> > +#include <linux/usb/mv_usb.h>
> > +
> > +enum pxa_ehci_type {
> > +   EHCI_UNDEFINED = 0,
> > +   PXA_U2OEHCI,    /* pxa 168, 9xx */
> > +   PXA_SPH,        /* pxa 168, 9xx SPH */
> > +   MMP3_HSIC,      /* mmp3 hsic */
> > +   MMP3_FSIC,      /* mmp3 fsic */
> > +};
> > +
> > +struct mv_usb_addon_irq {
> > +   unsigned int    irq;
> > +   int             (*poll)(void);
> > +};
> > +
> > +struct mv_usb_platform_data {
> > +   unsigned int            clknum;
> > +   char                    **clkname;
> > +   struct mv_usb_addon_irq *id;    /* Only valid for OTG. ID pin
> change*/
> > +   struct mv_usb_addon_irq *vbus;  /* valid for OTG/UDC. VBUS
> change*/
> > +
> > +   /* only valid for HCD. OTG or Host only*/
> > +   unsigned int            mode;
> > +
> > +   int     (*phy_init)(void __iomem *regbase);
> > +   void    (*phy_deinit)(void __iomem *regbase);
> > +   int     (*set_vbus)(unsigned int vbus);
> > +   int     (*private_init)(struct mv_op_regs *opregs,
> > +                           unsigned int phyregs);
> > +};
> > +
> > +#endif
>
> I would lilke it better if you move this header to
> include/linux/platform_data. We want to be able to build drivers on all
> architectures even if they will never be supported there. That's to
> catch compile errors easily via linux-next.
>
> > diff --git a/drivers/usb/gadget/Makefile
> b/drivers/usb/gadget/Makefile
> > index 9ba725a..c36da63 100644
> > --- a/drivers/usb/gadget/Makefile
> > +++ b/drivers/usb/gadget/Makefile
> > @@ -28,7 +28,7 @@ obj-$(CONFIG_USB_S3C_HSUDC)       += s3c-hsudc.o
> >  obj-$(CONFIG_USB_LANGWELL) += langwell_udc.o
> >  obj-$(CONFIG_USB_EG20T)            += pch_udc.o
> >  obj-$(CONFIG_USB_PXA_U2O)  += mv_udc.o
> > -mv_udc-y                   := mv_udc_core.o mv_udc_phy.o
> > +mv_udc-y                   := mv_udc_core.o
> >  obj-$(CONFIG_USB_CI13XXX_MSM)      += ci13xxx_msm.o
> >  obj-$(CONFIG_USB_FUSB300)  += fusb300_udc.o
> >
> > diff --git a/drivers/usb/gadget/mv_udc.h
> b/drivers/usb/gadget/mv_udc.h
> > index 65f1f7c..7ea85f2 100644
> > --- a/drivers/usb/gadget/mv_udc.h
> > +++ b/drivers/usb/gadget/mv_udc.h
> > @@ -1,9 +1,15 @@
> > +/*
> > + * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
> > + *
> > + * 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;  either version 2 of the  License, or
> > +(at your
> > + * option) any later version.
> > + */
> >
> >  #ifndef __MV_UDC_H
> >  #define __MV_UDC_H
> >
> > -#define VUSBHS_MAX_PORTS   8
> > -
> >  #define DQH_ALIGNMENT              2048
> >  #define DTD_ALIGNMENT              64
> >  #define DMA_BOUNDARY               4096
> > @@ -21,158 +27,17 @@
> >  #define WAIT_FOR_OUT_STATUS        3
> >  #define DATA_STATE_RECV            4
> >
> > -#define CAPLENGTH_MASK             (0xff)
> > -#define DCCPARAMS_DEN_MASK (0x1f)
> > -
> > -#define HCSPARAMS_PPC              (0x10)
> > -
> > -/* Frame Index Register Bit Masks */
> > -#define USB_FRINDEX_MASKS  0x3fff
> > -
> > -/* Command Register Bit Masks */
> > -#define USBCMD_RUN_STOP                            (0x00000001)
> > -#define USBCMD_CTRL_RESET                  (0x00000002)
> > -#define USBCMD_SETUP_TRIPWIRE_SET          (0x00002000)
> > -#define USBCMD_SETUP_TRIPWIRE_CLEAR
>       (~USBCMD_SETUP_TRIPWIRE_SET)
> > -
> > -#define USBCMD_ATDTW_TRIPWIRE_SET          (0x00004000)
> > -#define USBCMD_ATDTW_TRIPWIRE_CLEAR
>       (~USBCMD_ATDTW_TRIPWIRE_SET)
> > -
> > -/* bit 15,3,2 are for frame list size */
> > -#define USBCMD_FRAME_SIZE_1024                     (0x00000000) /* 000 */
> > -#define USBCMD_FRAME_SIZE_512                      (0x00000004) /* 001 */
> > -#define USBCMD_FRAME_SIZE_256                      (0x00000008) /* 010 */
> > -#define USBCMD_FRAME_SIZE_128                      (0x0000000C) /* 011 */
> > -#define USBCMD_FRAME_SIZE_64                       (0x00008000) /* 100 */
> > -#define USBCMD_FRAME_SIZE_32                       (0x00008004) /* 101 */
> > -#define USBCMD_FRAME_SIZE_16                       (0x00008008) /* 110 */
> > -#define USBCMD_FRAME_SIZE_8                        (0x0000800C) /* 111 */
> > -
> > -#define EPCTRL_TX_ALL_MASK                 (0xFFFF0000)
> > -#define EPCTRL_RX_ALL_MASK                 (0x0000FFFF)
> > -
> > -#define EPCTRL_TX_DATA_TOGGLE_RST          (0x00400000)
> > -#define EPCTRL_TX_EP_STALL                 (0x00010000)
> > -#define EPCTRL_RX_EP_STALL                 (0x00000001)
> > -#define EPCTRL_RX_DATA_TOGGLE_RST          (0x00000040)
> > -#define EPCTRL_RX_ENABLE                   (0x00000080)
> > -#define EPCTRL_TX_ENABLE                   (0x00800000)
> > -#define EPCTRL_CONTROL                             (0x00000000)
> > -#define EPCTRL_ISOCHRONOUS                 (0x00040000)
> > -#define EPCTRL_BULK                                (0x00080000)
> > -#define EPCTRL_INT                         (0x000C0000)
> > -#define EPCTRL_TX_TYPE                             (0x000C0000)
> > -#define EPCTRL_RX_TYPE                             (0x0000000C)
> > -#define EPCTRL_DATA_TOGGLE_INHIBIT         (0x00000020)
> > -#define EPCTRL_TX_EP_TYPE_SHIFT                    (18)
> > -#define EPCTRL_RX_EP_TYPE_SHIFT                    (2)
> > -
> > -#define EPCOMPLETE_MAX_ENDPOINTS           (16)
> > -
> > -/* endpoint list address bit masks */
> > -#define USB_EP_LIST_ADDRESS_MASK              0xfffff800
> > -
> > -#define PORTSCX_W1C_BITS                   0x2a
> > -#define PORTSCX_PORT_RESET                 0x00000100
> > -#define PORTSCX_PORT_POWER                 0x00001000
> > -#define PORTSCX_FORCE_FULL_SPEED_CONNECT   0x01000000
> > -#define PORTSCX_PAR_XCVR_SELECT                    0xC0000000
> > -#define PORTSCX_PORT_FORCE_RESUME          0x00000040
> > -#define PORTSCX_PORT_SUSPEND                       0x00000080
> > -#define PORTSCX_PORT_SPEED_FULL                    0x00000000
> > -#define PORTSCX_PORT_SPEED_LOW                     0x04000000
> > -#define PORTSCX_PORT_SPEED_HIGH                    0x08000000
> > -#define PORTSCX_PORT_SPEED_MASK                    0x0C000000
> > -
> > -/* USB MODE Register Bit Masks */
> > -#define USBMODE_CTRL_MODE_IDLE                     0x00000000
> > -#define USBMODE_CTRL_MODE_DEVICE           0x00000002
> > -#define USBMODE_CTRL_MODE_HOST                     0x00000003
> > -#define USBMODE_CTRL_MODE_RSV                      0x00000001
> > -#define USBMODE_SETUP_LOCK_OFF                     0x00000008
> > -#define USBMODE_STREAM_DISABLE                     0x00000010
> > -
> > -/* USB STS Register Bit Masks */
> > -#define USBSTS_INT                 0x00000001
> > -#define USBSTS_ERR                 0x00000002
> > -#define USBSTS_PORT_CHANGE         0x00000004
> > -#define USBSTS_FRM_LST_ROLL                0x00000008
> > -#define USBSTS_SYS_ERR                     0x00000010
> > -#define USBSTS_IAA                 0x00000020
> > -#define USBSTS_RESET                       0x00000040
> > -#define USBSTS_SOF                 0x00000080
> > -#define USBSTS_SUSPEND                     0x00000100
> > -#define USBSTS_HC_HALTED           0x00001000
> > -#define USBSTS_RCL                 0x00002000
> > -#define USBSTS_PERIODIC_SCHEDULE   0x00004000
> > -#define USBSTS_ASYNC_SCHEDULE              0x00008000
> > -
> > -
> > -/* Interrupt Enable Register Bit Masks */
> > -#define USBINTR_INT_EN                          (0x00000001)
> > -#define USBINTR_ERR_INT_EN                      (0x00000002)
> > -#define USBINTR_PORT_CHANGE_DETECT_EN           (0x00000004)
> > -
> > -#define USBINTR_ASYNC_ADV_AAE                   (0x00000020)
> > -#define USBINTR_ASYNC_ADV_AAE_ENABLE            (0x00000020)
> > -#define USBINTR_ASYNC_ADV_AAE_DISABLE           (0xFFFFFFDF)
> > -
> > -#define USBINTR_RESET_EN                        (0x00000040)
> > -#define USBINTR_SOF_UFRAME_EN                   (0x00000080)
> > -#define USBINTR_DEVICE_SUSPEND                  (0x00000100)
> > -
> > -#define USB_DEVICE_ADDRESS_MASK                    (0xfe000000)
> > -#define USB_DEVICE_ADDRESS_BIT_SHIFT               (25)
> > -
> > -struct mv_cap_regs {
> > -   u32     caplength_hciversion;
> > -   u32     hcsparams;      /* HC structural parameters */
> > -   u32     hccparams;      /* HC Capability Parameters*/
> > -   u32     reserved[5];
> > -   u32     dciversion;     /* DC version number and reserved 16 bits */
> > -   u32     dccparams;      /* DC Capability Parameters */
> > -};
> > -
> > -struct mv_op_regs {
> > -   u32     usbcmd;         /* Command register */
> > -   u32     usbsts;         /* Status register */
> > -   u32     usbintr;        /* Interrupt enable */
> > -   u32     frindex;        /* Frame index */
> > -   u32     reserved1[1];
> > -   u32     deviceaddr;     /* Device Address */
> > -   u32     eplistaddr;     /* Endpoint List Address */
> > -   u32     ttctrl;         /* HOST TT status and control */
> > -   u32     burstsize;      /* Programmable Burst Size */
> > -   u32     txfilltuning;   /* Host Transmit Pre-Buffer Packet Tuning
> */
> > -   u32     reserved[4];
> > -   u32     epnak;          /* Endpoint NAK */
> > -   u32     epnaken;        /* Endpoint NAK Enable */
> > -   u32     configflag;     /* Configured Flag register */
> > -   u32     portsc[VUSBHS_MAX_PORTS]; /* Port Status/Control x, x =
> 1..8 */
> > -   u32     otgsc;
> > -   u32     usbmode;        /* USB Host/Device mode */
> > -   u32     epsetupstat;    /* Endpoint Setup Status */
> > -   u32     epprime;        /* Endpoint Initialize */
> > -   u32     epflush;        /* Endpoint De-initialize */
> > -   u32     epstatus;       /* Endpoint Status */
> > -   u32     epcomplete;     /* Endpoint Interrupt On Complete */
> > -   u32     epctrlx[16];    /* Endpoint Control, where x = 0.. 15 */
> > -   u32     mcr;            /* Mux Control */
> > -   u32     isr;            /* Interrupt Status */
> > -   u32     ier;            /* Interrupt Enable */
> > -};
>
> this doesn't seem to be part of this patch. Why do you need to move
> this code ?
>
> >  struct mv_udc {
> >     struct usb_gadget               gadget;
> >     struct usb_gadget_driver        *driver;
> >     spinlock_t                      lock;
> > -   struct completion               *done;
> > +   struct completion               *done;  /* make sure release is done
> */
>
> adding this comment isn't part of this patch.
>
> >     struct platform_device          *dev;
> >     int                             irq;
> >
> >     struct mv_cap_regs __iomem      *cap_regs;
> >     struct mv_op_regs __iomem       *op_regs;
> > -   unsigned int                    phy_regs;
> > +   void __iomem                    *phy_regs;
>
> not part of this patch.
>
> >     unsigned int                    max_eps;
> >     struct mv_dqh                   *ep_dqh;
> >     size_t                          ep_dqh_size;
> > @@ -201,7 +66,12 @@ struct mv_udc {
> >                             remote_wakeup:1,
> >                             softconnected:1,
> >                             force_fs:1;
> > -   struct clk              *clk;
> > +
> > +   struct mv_usb_platform_data     *pdata;
> > +
> > +   /* some SOC has mutiple clock sources for USB*/
> > +   unsigned int    clknum;
> > +   struct clk      *clk[0];
> >  };
> >
> >  /* endpoint data structure */
> > @@ -245,17 +115,18 @@ struct mv_req {
> >  #define EP_MAX_LENGTH_TRANSFER                     0x4000
> >
> >  struct mv_dqh {
> > -   /* Bits 16..26 Bit 15 is Interrupt On Setup */
> > +   /* Mult(31-30) , Zlt(29) , Max Pkt len(26-16)  and IOS(15) */
> >     u32     max_packet_length;
> >     u32     curr_dtd_ptr;           /* Current dTD Pointer */
> >     u32     next_dtd_ptr;           /* Next dTD Pointer */
> > -   /* Total bytes (16..30), IOC (15), INT (8), STS (0-7) */
> > +
> > +   /* Total bytes (30-16), IOC (15),  MultO(11-10), STS (7-0)  */
> >     u32     size_ioc_int_sts;
> > -   u32     buff_ptr0;              /* Buffer pointer Page 0 (12-31) */
> > -   u32     buff_ptr1;              /* Buffer pointer Page 1 (12-31) */
> > -   u32     buff_ptr2;              /* Buffer pointer Page 2 (12-31) */
> > -   u32     buff_ptr3;              /* Buffer pointer Page 3 (12-31) */
> > -   u32     buff_ptr4;              /* Buffer pointer Page 4 (12-31) */
> > +   u32     buff_ptr0;              /* Buffer pointer Page 0 (31-12) */
> > +   u32     buff_ptr1;              /* Buffer pointer Page 1 (31-12) */
> > +   u32     buff_ptr2;              /* Buffer pointer Page 2 (31-12) */
> > +   u32     buff_ptr3;              /* Buffer pointer Page 3 (31-12) */
> > +   u32     buff_ptr4;              /* Buffer pointer Page 4 (31-12) */
>
> updating those comments isn't part of this patch.
>
> >     u32     reserved1;
> >     /* 8 bytes of setup data that follows the Setup PID */
> >     u8      setup_buffer[8];
> > @@ -264,19 +135,25 @@ struct mv_dqh {
> >
> >
> >  #define DTD_NEXT_TERMINATE         (0x00000001)
> > -#define DTD_IOC                            (0x00008000)
> > -#define DTD_STATUS_ACTIVE          (0x00000080)
> > -#define DTD_STATUS_HALTED          (0x00000040)
> > -#define DTD_STATUS_DATA_BUFF_ERR   (0x00000020)
> > +#define DTD_STATUS_MASK                    (0X000000FF)
> >  #define DTD_STATUS_TRANSACTION_ERR (0x00000008)
> > -#define DTD_RESERVED_FIELDS                (0x00007F00)
> > -#define DTD_ERROR_MASK                     (0x68)
> > -#define DTD_ADDR_MASK                      (0xFFFFFFE0)
> > -#define DTD_PACKET_SIZE                    0x7FFF0000
> > +#define DTD_STATUS_DATA_BUFF_ERR   (0x00000020)
> > +#define DTD_STATUS_HALTED          (0x00000040)
> > +#define DTD_STATUS_ACTIVE          (0x00000080)
> > +#define DTD_ERROR_MASK                     (DTD_STATUS_HALTED \
> > +                                   | DTD_STATUS_DATA_BUFF_ERR \
> > +                                   | DTD_STATUS_TRANSACTION_ERR)
> > +#define DTD_RESERVED_FIELDS                (0x80007F00)
> > +#define DTD_IOC                            (0x00008000)
> > +#define DTD_PACKET_SIZE                    (0x7FFF0000)
> >  #define DTD_LENGTH_BIT_POS         (16)
> >
> > +#define REQ_UNCOMPLETE                     (1)
> > +
> >  struct mv_dtd {
> > +   /* Next TD pointer(31-5), T(0) set indicate invalid */
> >     u32     dtd_next;
> > +   /* Total bytes (30-16), IOC (15), MultO(11-10), STS (7-0)  */
> >     u32     size_ioc_sts;
> >     u32     buff_ptr0;              /* Buffer pointer Page 0 */
> >     u32     buff_ptr1;              /* Buffer pointer Page 1 */
> > @@ -289,6 +166,4 @@ struct mv_dtd {
> >     struct mv_dtd *next_dtd_virt;
> >  };
> >
> > -extern int mv_udc_phy_init(unsigned int base);
> > -
> >  #endif
> > diff --git a/drivers/usb/gadget/mv_udc_core.c
> > b/drivers/usb/gadget/mv_udc_core.c
> > index ce1ac2b..2153da1 100644
> > --- a/drivers/usb/gadget/mv_udc_core.c
> > +++ b/drivers/usb/gadget/mv_udc_core.c
> > @@ -1,3 +1,14 @@
> > +/*
> > + * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
> > + * Author: Chao Xie <chao.xie@xxxxxxxxxxx>
> > + *    Neil Zhang <zhangwm@xxxxxxxxxxx>
> > + *
> > + * 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;  either version 2 of the  License, or
> > +(at your
> > + * option) any later version.
> > + */
>
> adding this header isn't part of this patch.
>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> >  #include <linux/dma-mapping.h>
> > @@ -22,9 +33,12 @@
> >  #include <linux/irq.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/clk.h>
> > +#include <linux/usb/mv_usb.h>
> >  #include <asm/system.h>
> >  #include <asm/unaligned.h>
> >
> > +#include <plat/usb.h>
>
> drivers shouldn't use <arch>, <asm> or <plat> headers. Or at least,
> should avoid as much as possible.
>
> > +
> >  #include "mv_udc.h"
> >
> >  #define DRIVER_DESC                "Marvell PXA USB Device Controller
> driver"
> > @@ -48,6 +62,8 @@
> >  static const char driver_name[] = "mv_udc";  static const char
> > driver_desc[] = DRIVER_DESC;
> >
> > +static DECLARE_COMPLETION(release_done);
> > +
> >  /* controller device global variable */
> >  static struct mv_udc       *the_controller;
> >  int mv_usb_otgsc;
> > @@ -987,6 +1003,22 @@ static struct usb_ep_ops mv_ep_ops = {
> >     .fifo_flush     = mv_ep_fifo_flush,     /* flush fifo */
> >  };
> >
> > +static void udc_clock_enable(struct mv_udc *udc) {
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < udc->clknum; i++)
> > +           clk_enable(udc->clk[i]);
> > +}
> > +
> > +static void udc_clock_disable(struct mv_udc *udc) {
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < udc->clknum; i++)
> > +           clk_disable(udc->clk[i]);
> > +}
> > +
> >  static void udc_stop(struct mv_udc *udc)  {
> >     u32 tmp;
> > @@ -1877,18 +1909,15 @@ static void gadget_release(struct device
> *_dev)
> >     struct mv_udc *udc = the_controller;
> >
> >     complete(udc->done);
> > -   kfree(udc);
> >  }
> >
> > -static int mv_udc_remove(struct platform_device *dev)
> > +static __devexit int mv_udc_remove(struct platform_device *dev)
>
> not part of this patch.
>
> >  {
> >     struct mv_udc *udc = the_controller;
> > -   DECLARE_COMPLETION(done);
> > +   int clk_i;
> >
> >     usb_del_gadget_udc(&udc->gadget);
> >
> > -   udc->done = &done;
> > -
> >     /* free memory allocated in probe */
> >     if (udc->dtd_pool)
> >             dma_pool_destroy(udc->dtd_pool);
> > @@ -1907,83 +1936,101 @@ static int mv_udc_remove(struct
> platform_device *dev)
> >     udc->cap_regs = NULL;
> >
> >     if (udc->phy_regs)
> > -           iounmap((void *)udc->phy_regs);
> > -   udc->phy_regs = 0;
> > +           iounmap(udc->phy_regs);
> > +   udc->phy_regs = NULL;
> >
> >     if (udc->status_req) {
> >             kfree(udc->status_req->req.buf);
> >             kfree(udc->status_req);
> >     }
> >
> > +   for (clk_i = 0; clk_i <= udc->clknum; clk_i++)
> > +           clk_put(udc->clk[clk_i]);
> > +
> >     device_unregister(&udc->gadget.dev);
> >
> >     /* free dev, wait for the release() finished */
> > -   wait_for_completion(&done);
> > -
> > +   wait_for_completion(udc->done);
>
> should you have a timeout here ?
>
> wait_for_completion_timeout() ??
>
> >     the_controller = NULL;
> > +   kfree(udc);
> >
> >     return 0;
> >  }
> >
> > -int mv_udc_probe(struct platform_device *dev)
> > +static int __devinit mv_udc_probe(struct platform_device *dev)
> >  {
> > -   struct mv_udc *udc;
> > +   struct mv_usb_platform_data *pdata = dev->dev.platform_data;
> > +   struct mv_udc *udc = NULL;
> >     int retval = 0;
> > +   int clk_i = 0;
> >     struct resource *r;
> >     size_t size;
> >
> > -   udc = kzalloc(sizeof *udc, GFP_KERNEL);
> > +   if (pdata == NULL) {
> > +           dev_err(&dev->dev, "missing platform_data\n");
> > +           return -ENODEV;
> > +   }
> > +
> > +   size = sizeof(*udc) + sizeof(struct clk *) * pdata->clknum;
> > +   udc = kzalloc(size, GFP_KERNEL);
> >     if (udc == NULL) {
> >             dev_err(&dev->dev, "failed to allocate memory for udc\n");
> > -           retval = -ENOMEM;
> > -           goto error;
> > +           return -ENOMEM;
> >     }
> >
> > +   the_controller = udc;
> > +   udc->done = &release_done;
> >     spin_lock_init(&udc->lock);
> >
> >     udc->dev = dev;
> > -
> > -   udc->clk = clk_get(&dev->dev, "U2OCLK");
> > -   if (IS_ERR(udc->clk)) {
> > -           retval = PTR_ERR(udc->clk);
> > -           goto error;
> > +   udc->pdata = dev->dev.platform_data;
> > +
> > +   udc->clknum = pdata->clknum;
> > +   for (clk_i = 0; clk_i < udc->clknum; clk_i++) {
> > +           udc->clk[clk_i] = clk_get(&dev->dev, pdata->clkname[clk_i]);
> > +           if (IS_ERR(udc->clk[clk_i])) {
> > +                   retval = PTR_ERR(udc->clk[clk_i]);
> > +                   goto err_put_clk;
> > +           }
> >     }
> >
> > -   r = platform_get_resource_byname(udc->dev, IORESOURCE_MEM, "u2o");
> > +   r = platform_get_resource_byname(udc->dev, IORESOURCE_MEM,
> > +"capregs");
> >     if (r == NULL) {
> >             dev_err(&dev->dev, "no I/O memory resource defined\n");
> >             retval = -ENODEV;
> > -           goto error;
> > +           goto err_put_clk;
> >     }
> >
> >     udc->cap_regs = (struct mv_cap_regs __iomem *)
> > -           ioremap(r->start, resource_size(r));
> > +                           ioremap(r->start, resource_size(r));
>
> what's this change ?? Seems like you're just making the indentation
> even uglier.
>
> (please break this patch)
>

It seems to be a bit too big, I will try to split it further.
Thanks.

> --
> balbi


Best Regards,
Neil Zhang
--
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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux