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
