RE: [PATCH 01/10 v4] usb: gadget: mv_udc: drop ARCH dependency

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

 



Hi Balbi,

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@xxxxxx]
> Sent: 2011年12月14日 1:52
> To: Neil Zhang
> Cc: balbi@xxxxxx; stern@xxxxxxxxxxxxxxxxxxx; gregkh@xxxxxxx; linux-
> usb@xxxxxxxxxxxxxxx; Chao Xie
> Subject: Re: [PATCH 01/10 v4] usb: gadget: mv_udc: drop ARCH dependency
> 
> Hi,
> 
> On Tue, Dec 13, 2011 at 08:13:44PM +0800, Neil Zhang wrote:
> > This patch do the following things:
> > 1. Change the Kconfig information.
> > 2. Rename the driver name.
> > 3. Don't do any type cast to io memory.
> > 4. Add dummy stub for clk framework.
> 
> thanks :-) Will apply tomorrow.
> 
> > Signed-off-by: Neil Zhang <zhangwm@xxxxxxxxxxx>
> > ---
> >  drivers/usb/gadget/Kconfig           |   10 +++++-----
> >  drivers/usb/gadget/Makefile          |    2 +-
> >  drivers/usb/gadget/mv_udc.h          |    2 +-
> >  drivers/usb/gadget/mv_udc_core.c     |   14 +++++++-------
> >  include/linux/platform_data/mv_usb.h |   12 ++++++++++--
> >  5 files changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> > index 23a4473..a591376 100644
> > --- a/drivers/usb/gadget/Kconfig
> > +++ b/drivers/usb/gadget/Kconfig
> > @@ -310,13 +310,13 @@ config USB_S3C_HSUDC
> >
> >  	  This driver has been tested on S3C2416 and S3C2450 processors.
> >
> > -config USB_PXA_U2O
> > -	tristate "PXA9xx Processor USB2.0 controller"
> > -	depends on ARCH_MMP
> > +config USB_MV_UDC
> > +	tristate "Marvell USB2.0 Device Controller"
> >  	select USB_GADGET_DUALSPEED
> >  	help
> > -	  PXA9xx Processor series include a high speed USB2.0 device
> > -	  controller, which support high speed and full speed USB
> peripheral.
> > +	  Marvell Socs (including PXA and MMP series) include a high
> speed
> > +	  USB2.0 OTG controller, which can be configured as high speed or
> > +	  full speed USB peripheral.
> >
> >  config USB_GADGET_DWC3
> >  	tristate "DesignWare USB3.0 (DRD) Controller"
> > diff --git a/drivers/usb/gadget/Makefile
> b/drivers/usb/gadget/Makefile
> > index b54ac61..b7f6eef 100644
> > --- a/drivers/usb/gadget/Makefile
> > +++ b/drivers/usb/gadget/Makefile
> > @@ -27,7 +27,7 @@ obj-$(CONFIG_USB_S3C_HSOTG)	+= s3c-hsotg.o
> >  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
> > +obj-$(CONFIG_USB_MV_UDC)	+= mv_udc.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 daa75c1..de67f3d 100644
> > --- a/drivers/usb/gadget/mv_udc.h
> > +++ b/drivers/usb/gadget/mv_udc.h
> > @@ -180,7 +180,7 @@ struct mv_udc {
> >
> >  	struct mv_cap_regs __iomem	*cap_regs;
> >  	struct mv_op_regs __iomem	*op_regs;
> > -	unsigned int			phy_regs;
> > +	void __iomem			*phy_regs;
> >  	unsigned int			max_eps;
> >  	struct mv_dqh			*ep_dqh;
> >  	size_t				ep_dqh_size;
> > diff --git a/drivers/usb/gadget/mv_udc_core.c
> > b/drivers/usb/gadget/mv_udc_core.c
> > index 8924121..cd422ba 100644
> > --- a/drivers/usb/gadget/mv_udc_core.c
> > +++ b/drivers/usb/gadget/mv_udc_core.c
> > @@ -2132,8 +2132,8 @@ static int __devexit 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;
> 
> do you need to set this to NULL ? Aren't you going to free udc anyway ?

Yes, it seems useless.

> 
> > @@ -2213,8 +2213,8 @@ static int __devinit mv_udc_probe(struct
> platform_device *dev)
> >  		goto err_iounmap_capreg;
> >  	}
> >
> > -	udc->phy_regs = (unsigned int)ioremap(r->start, resource_size(r));
> > -	if (udc->phy_regs == 0) {
> > +	udc->phy_regs = ioremap(r->start, resource_size(r));
> > +	if (udc->phy_regs == NULL) {
> >  		dev_err(&dev->dev, "failed to map phy I/O memory\n");
> >  		retval = -EBUSY;
> >  		goto err_iounmap_capreg;
> > @@ -2391,7 +2391,7 @@ err_disable_clock:
> >  		udc->pdata->phy_deinit(udc->phy_regs);
> >  	udc_clock_disable(udc);
> >  err_iounmap_phyreg:
> > -	iounmap((void *)udc->phy_regs);
> > +	iounmap(udc->phy_regs);
> >  err_iounmap_capreg:
> >  	iounmap(udc->cap_regs);
> >  err_put_clk:
> > @@ -2457,13 +2457,13 @@ static struct platform_driver udc_driver = {
> >  	.shutdown	= mv_udc_shutdown,
> >  	.driver		= {
> >  		.owner	= THIS_MODULE,
> > -		.name	= "pxa-u2o",
> > +		.name	= "mv-udc",
> >  #ifdef CONFIG_PM
> >  		.pm	= &mv_udc_pm_ops,
> >  #endif
> >  	},
> >  };
> > -MODULE_ALIAS("platform:pxa-u2o");
> > +MODULE_ALIAS("platform:mv-udc");
> >
> >  MODULE_DESCRIPTION(DRIVER_DESC);
> >  MODULE_AUTHOR("Chao Xie <chao.xie@xxxxxxxxxxx>"); diff --git
> > a/include/linux/platform_data/mv_usb.h
> > b/include/linux/platform_data/mv_usb.h
> > index e9d9149..f3fbb5d 100644
> > --- a/include/linux/platform_data/mv_usb.h
> > +++ b/include/linux/platform_data/mv_usb.h
> > @@ -42,9 +42,17 @@ struct mv_usb_platform_data {
> >  	/* only valid for HCD. OTG or Host only*/
> >  	unsigned int		mode;
> >
> > -	int     (*phy_init)(unsigned int regbase);
> > -	void    (*phy_deinit)(unsigned int regbase);
> > +	int     (*phy_init)(void __iomem *regbase);
> > +	void    (*phy_deinit)(void __iomem *regbase);
> 
> not related to $SUBJECT, but it really seems like these should be part
> of a PHY driver under drivers/usb/otg.

It's these two functions using 'unsigned int' as their parameter will need our driver do type cast, so it need changed.
We need our UDC and EHCI driver can work in stand-alone mode, so it will be used by UDC, OTG, and EHCI driver all.
And we have implement a reference count to avoid being called more than one time.

> 
> >  	int	(*set_vbus)(unsigned int vbus);
> >  };
> >
> > +#ifndef CONFIG_HAVE_CLK
> > +/* Dummy stub for clk framework */
> > +#define clk_get(dev, id)	NULL
> > +#define clk_put(clock)		do {} while (0)
> > +#define clk_enable(clock)	do {} while (0)
> > +#define clk_disable(clock)	do {} while (0)
> > +#endif
> 
> hopefully the generic clock work will help you drop this in the near
> future. What about moving your platform to pm_runtime, though ?
> 
Yes, pm_runtime is a good choice, but we haven't do it yet.

> --
> Balbi

Best Regards,
Neil Zhang
?韬{.n?????%??檩??w?{.n???{炳???骅w*jg????????G??⒏⒎?:+v????????????"??????


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

  Powered by Linux