Re: [PATCH 12/17] usb: isp1760: Move core code to isp1760-core.c

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

 



Hi Felipe,

Thank you for the quick review.

On Monday 06 October 2014 11:06:27 Felipe Balbi wrote:
> On Mon, Oct 06, 2014 at 06:55:00PM +0300, Laurent Pinchart wrote:
> > Move core device initialization to a central location in order to share
> > it with the device mode implementation.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > 
> >  drivers/usb/host/Makefile       |  2 +-
> >  drivers/usb/host/isp1760-core.c | 90 ++++++++++++++++++++++++++++++++++++
> >  drivers/usb/host/isp1760-core.h | 37 +++++++++++++++++
> >  drivers/usb/host/isp1760-hcd.c  | 73 ++++++++-------------------------
> >  drivers/usb/host/isp1760-hcd.h  |  8 ++--
> >  drivers/usb/host/isp1760-if.c   |  2 +-
> >  6 files changed, 150 insertions(+), 62 deletions(-)
> >  create mode 100644 drivers/usb/host/isp1760-core.c
> >  create mode 100644 drivers/usb/host/isp1760-core.h
> > 
> > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> > index 144c038..ecba12c 100644
> > --- a/drivers/usb/host/Makefile
> > +++ b/drivers/usb/host/Makefile
> > @@ -5,7 +5,7 @@
> > 
> >  # tell define_trace.h where to find the xhci trace header
> >  CFLAGS_xhci-trace.o := -I$(src)
> > 
> > -isp1760-y := isp1760-hcd.o isp1760-if.o
> > +isp1760-y := isp1760-core.o isp1760-hcd.o isp1760-if.o
> > 
> >  fhci-y := fhci-hcd.o fhci-hub.o fhci-q.o
> >  fhci-y += fhci-mem.o fhci-tds.o fhci-sched.o
> > 
> > diff --git a/drivers/usb/host/isp1760-core.c
> > b/drivers/usb/host/isp1760-core.c new file mode 100644
> > index 0000000..28f8290
> > --- /dev/null
> > +++ b/drivers/usb/host/isp1760-core.c
> > @@ -0,0 +1,90 @@
> > +/*
> > + * Driver for the NXP ISP1760 chip
> > + *
> > + * Copyright 2014 Laurent Pinchart
> > + * Copyright 2007 Sebastian Siewior
> > + *
> > + * Contacts:
> > + *	Sebastian Siewior <bigeasy@xxxxxxxxxxxxx>
> > + *	Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/gpio.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +
> > +#include "isp1760-core.h"
> > +#include "isp1760-hcd.h"
> > +
> > +int isp1760_register(struct resource *mem, int irq, unsigned long
> > irqflags, +		     int rst_gpio, struct device *dev, unsigned int
> > devflags)
> > +{
> > +	struct isp1760_device *isp;
> > +	int ret;
> > +
> > +	if (usb_disabled())
> > +		return -ENODEV;
> > +
> > +	/* prevent usb-core allocating DMA pages */
> > +	dev->dma_mask = NULL;
> > +
> > +	isp = kzalloc(sizeof(*isp), GFP_KERNEL);
> > +	if (!isp)
> > +		return -ENOMEM;
> > +
> > +	isp->rst_gpio = rst_gpio;
> > +
> > +	isp->mem_start = mem->start;
> > +	isp->mem_size = resource_size(mem);
> > +
> > +	isp->regs = ioremap(isp->mem_start, isp->mem_size);
> > +	if (!isp->regs) {
> > +		ret = -EIO;
> > +		goto error;
> > +	}
> 
> where did request_mem_region() go ?

It's still in the glue code.

> > +	isp->hcd.rst_gpio = rst_gpio;
> > +	ret = isp1760_hcd_register(&isp->hcd, isp->regs, mem, irq, irqflags,
> > +				   dev, devflags);
> > +	if (ret < 0)
> > +		goto error;
> > +
> > +	dev_set_drvdata(dev, isp);
> > +
> > +	return 0;
> > +
> > +error:
> > +	if (isp->regs)
> > +		iounmap(isp->regs);
> > +
> > +	kfree(isp);
> > +
> > +	return ret;
> > +}
> > +
> > +void isp1760_unregister(struct device *dev)
> > +{
> > +	struct isp1760_device *isp = dev_get_drvdata(dev);
> > +
> > +	release_mem_region(isp->mem_start, isp->mem_size);
> 
> only after iounmap()

Sure, will fix that. I'll actually fix it in patch 02/17 and fix the rebase 
conflict here.

> > +	isp1760_hcd_unregister(&isp->hcd);
> > +
> > +	iounmap(isp->regs);
> > +
> > +	if (gpio_is_valid(isp->rst_gpio))
> > +		gpio_free(isp->rst_gpio);
> > +
> > +	kfree(isp);
> > +}
> > +
> > +MODULE_DESCRIPTION("Driver for the ISP1760 USB-controller from NXP");
> > +MODULE_AUTHOR("Sebastian Siewior <bigeasy@xxxxxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/usb/host/isp1760-core.h
> > b/drivers/usb/host/isp1760-core.h new file mode 100644
> > index 0000000..8bd997c
> > --- /dev/null
> > +++ b/drivers/usb/host/isp1760-core.h
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Driver for the NXP ISP1760 chip
> > + *
> > + * Copyright 2014 Laurent Pinchart
> > + * Copyright 2007 Sebastian Siewior
> > + *
> > + * Contacts:
> > + *	Sebastian Siewior <bigeasy@xxxxxxxxxxxxx>
> > + *	Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef _ISP1760_CORE_H_
> > +#define _ISP1760_CORE_H_
> > +
> > +#include <linux/ioport.h>
> > +
> > +#include "isp1760-hcd.h"
> > +
> > +struct isp1760_device {
> > +	resource_size_t mem_start;
> > +	resource_size_t mem_size;
> > +
> > +	void __iomem *regs;
> > +	int rst_gpio;
> > +
> > +	struct isp1760_hcd hcd;
> > +};
> > +
> > +int isp1760_register(struct resource *mem, int irq, unsigned long
> > irqflags, +		     int rst_gpio, struct device *dev, unsigned int
> > devflags);
> > +void isp1760_unregister(struct device *dev);
> > +
> > +#endif
> > diff --git a/drivers/usb/host/isp1760-hcd.c
> > b/drivers/usb/host/isp1760-hcd.c index 9c8cd37..f903ad9 100644
> > --- a/drivers/usb/host/isp1760-hcd.c
> > +++ b/drivers/usb/host/isp1760-hcd.c
> > @@ -2237,81 +2237,42 @@ void isp1760_deinit_kmem_cache(void)
> > 
> >  	kmem_cache_destroy(urb_listitem_cachep);
> >  
> >  }
> > 
> > -int isp1760_register(struct resource *mem, int irq, unsigned long
> > irqflags, -		     int rst_gpio, struct device *dev, unsigned int
> > devflags)
> > +int isp1760_hcd_register(struct isp1760_hcd *priv, void __iomem *regs,
> > +			 struct resource *mem, int irq, unsigned long irqflags,
> > +			 struct device *dev, unsigned int devflags)
> > 
> >  {
> > 
> > -	struct usb_hcd *hcd = NULL;
> > -	struct isp1760_hcd *priv;
> > +	struct usb_hcd *hcd;
> > 
> >  	int ret;
> > 
> > -	if (usb_disabled())
> > -		return -ENODEV;
> > -
> > -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > -	if (!priv)
> > -		return -ENOMEM;
> > -
> > -	/* prevent usb-core allocating DMA pages */
> > -	dev->dma_mask = NULL;
> > -
> > 
> >  	hcd = usb_create_hcd(&isp1760_hc_driver, dev, dev_name(dev));
> > 
> > -	if (!hcd) {
> > -		ret = -ENOMEM;
> > -		goto err_put;
> > -	}
> > +	if (!hcd)
> > +		return -ENOMEM;
> > 
> > -	priv->hcd = hcd;
> > 
> >  	*(struct isp1760_hcd **)hcd->hcd_priv = priv;
> > 
> > +	priv->hcd = hcd;
> > 
> >  	priv->devflags = devflags;
> > 
> > -	priv->rst_gpio = rst_gpio;
> > +
> > 
> >  	init_memory(priv);
> > 
> > -	hcd->regs = ioremap(mem->start, resource_size(mem));
> > -	if (!hcd->regs) {
> > -		ret = -EIO;
> > -		goto err_put;
> > -	}
> > 
> >  	hcd->irq = irq;
> > 
> > +	hcd->regs = regs;
> > 
> >  	hcd->rsrc_start = mem->start;
> >  	hcd->rsrc_len = resource_size(mem);
> >  	
> >  	ret = usb_add_hcd(hcd, irq, irqflags);
> > 
> > -	if (ret)
> > -		goto err_unmap;
> > -	device_wakeup_enable(hcd->self.controller);
> > +	if (ret) {
> > +		usb_put_hcd(hcd);
> > +		return ret;
> > +	}
> > 
> > -	dev_set_drvdata(dev, priv);
> > +	device_wakeup_enable(hcd->self.controller);
> > 
> >  	return 0;
> > 
> > -
> > -err_unmap:
> > -	iounmap(hcd->regs);
> > -
> > -err_put:
> > -	usb_put_hcd(hcd);
> > -	kfree(priv);
> > -
> > -	return ret;
> > 
> >  }
> > 
> > -void isp1760_unregister(struct device *dev)
> > +void isp1760_hcd_unregister(struct isp1760_hcd *priv)
> > 
> >  {
> > 
> > -	struct isp1760_hcd *priv = dev_get_drvdata(dev);
> > -	struct usb_hcd *hcd = priv->hcd;
> > -
> > -	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> > -
> > -	usb_remove_hcd(hcd);
> > -	iounmap(hcd->regs);
> > -	usb_put_hcd(hcd);
> > -
> > -	if (gpio_is_valid(priv->rst_gpio))
> > -		gpio_free(priv->rst_gpio);
> > -
> > -	kfree(priv);
> > +	usb_remove_hcd(priv->hcd);
> > +	usb_put_hcd(priv->hcd);
> > 
> >  }
> > 
> > -
> > -MODULE_DESCRIPTION("Driver for the ISP1760 USB-controller from NXP");
> > -MODULE_AUTHOR("Sebastian Siewior <bigeasy@xxxxxxxxxxxxxx>");
> > -MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/usb/host/isp1760-hcd.h
> > b/drivers/usb/host/isp1760-hcd.h index 3ece078..51996a2 100644
> > --- a/drivers/usb/host/isp1760-hcd.h
> > +++ b/drivers/usb/host/isp1760-hcd.h
> > @@ -84,10 +84,10 @@ struct isp1760_hcd {
> > 
> >  	int			rst_gpio;
> >  
> >  };
> > 
> > -/* exports for if */
> > -int isp1760_register(struct resource *mem, int irq, unsigned long
> > irqflags, -		     int rst_gpio, struct device *dev, unsigned int
> > devflags);
> > -void isp1760_unregister(struct device *dev);
> > +int isp1760_hcd_register(struct isp1760_hcd *priv, void __iomem *regs,
> > +			 struct resource *mem, int irq, unsigned long irqflags,
> > +			 struct device *dev, unsigned int devflags);
> > +void isp1760_hcd_unregister(struct isp1760_hcd *priv);
> > 
> >  int isp1760_init_kmem_once(void);
> >  void isp1760_deinit_kmem_cache(void);
> > 
> > diff --git a/drivers/usb/host/isp1760-if.c b/drivers/usb/host/isp1760-if.c
> > index 2880604..16db1be 100644
> > --- a/drivers/usb/host/isp1760-if.c
> > +++ b/drivers/usb/host/isp1760-if.c
> > @@ -16,7 +16,7 @@
> > 
> >  #include <linux/usb/isp1760.h>
> >  #include <linux/usb/hcd.h>
> > 
> > -#include "isp1760-hcd.h"
> > +#include "isp1760-core.h"
> > 
> >  #include "isp1760-regs.h"
> >  
> >  #if defined(CONFIG_OF) && defined(CONFIG_OF_IRQ)

-- 
Regards,

Laurent Pinchart

--
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