[patch 2.6.24-rc1] resource_len() utility function

[patch 2.6.24-rc1] resource_len() utility function

[David Brownell]

Add a new resource_len() function, so drivers can start using this
instead of driver-private code for a common idiom.  The call can be
useful with at least:

 - request_region(), release_region()
 - request_mem_region(), release_mem_region()
 - ioremap()

Candidate drivers include those using platform or PNP busses, and
maybe some others.  PCI already has a similar function.

This patch also updates a representative set of drivers in two
subsystems to use this call (SPI, and USB peripheral/gadget).

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Despite comments that we have "needed" this for at least five years,
this is pure cleanup.  IMO it's appropriate for 2.6.25 and to simmer
with the rest of the MM soup for a while.

 drivers/spi/atmel_spi.c           |    2 +-
 drivers/spi/omap2_mcspi.c         |    7 +++----
 drivers/spi/spi_imx.c             |    5 ++---
 drivers/spi/spi_mpc83xx.c         |    2 +-
 drivers/spi/spi_s3c24xx.c         |    4 ++--
 drivers/spi/spi_txx9.c            |    2 +-
 drivers/spi/xilinx_spi.c          |    5 ++---
 drivers/usb/gadget/at91_udc.c     |   12 +++++-------
 drivers/usb/gadget/fsl_usb2_udc.c |    9 ++++-----
 drivers/usb/gadget/omap_udc.c     |    6 +++---
 include/linux/ioport.h            |    5 +++++
 11 files changed, 29 insertions(+), 30 deletions(-)

--- at91.orig/include/linux/ioport.h	2007-10-19 11:38:39.000000000 -0700
+++ at91/include/linux/ioport.h	2007-10-19 14:26:50.000000000 -0700
@@ -22,6 +22,11 @@ struct resource {
 	struct resource *parent, *sibling, *child;
 };
 
+static inline resource_size_t __pure resource_len(const struct resource *r)
+{
+	return r->end + 1 - r->start;
+}
+
 struct resource_list {
 	struct resource_list *next;
 	struct resource *res;
--- at91.orig/drivers/usb/gadget/at91_udc.c	2007-10-13 15:16:13.000000000 -0700
+++ at91/drivers/usb/gadget/at91_udc.c	2007-10-19 14:26:33.000000000 -0700
@@ -1656,9 +1656,7 @@ static int __init at91udc_probe(struct p
 	if (!res)
 		return -ENXIO;
 
-	if (!request_mem_region(res->start,
-			res->end - res->start + 1,
-			driver_name)) {
+	if (!request_mem_region(res->start, resource_len(res), driver_name)) {
 		DBG("someone's using UDC memory\n");
 		return -EBUSY;
 	}
@@ -1670,9 +1668,9 @@ static int __init at91udc_probe(struct p
 	udc->pdev = pdev;
 	udc->enabled = 0;
 
-	udc->udp_baseaddr = ioremap(res->start, res->end - res->start + 1);
+	udc->udp_baseaddr = ioremap(res->start, resource_len(res));
 	if (!udc->udp_baseaddr) {
-		release_mem_region(res->start, res->end - res->start + 1);
+		release_mem_region(res->start, resource_len(res));
 		return -ENOMEM;
 	}
 
@@ -1735,7 +1733,7 @@ static int __init at91udc_probe(struct p
 fail1:
 	device_unregister(&udc->gadget.dev);
 fail0:
-	release_mem_region(res->start, res->end - res->start + 1);
+	release_mem_region(res->start, resource_len(res));
 	DBG("%s probe failed, %d\n", driver_name, retval);
 	return retval;
 }
@@ -1761,7 +1759,7 @@ static int __exit at91udc_remove(struct 
 
 	iounmap(udc->udp_baseaddr);
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(res->start, res->end - res->start + 1);
+	release_mem_region(res->start, resource_len(res));
 
 	clk_put(udc->iclk);
 	clk_put(udc->fclk);
--- at91.orig/drivers/usb/gadget/fsl_usb2_udc.c	2007-10-13 15:16:13.000000000 -0700
+++ at91/drivers/usb/gadget/fsl_usb2_udc.c	2007-10-19 11:43:12.000000000 -0700
@@ -2268,14 +2268,13 @@ static int __init fsl_udc_probe(struct p
 		return -ENXIO;
 	}
 
-	if (!request_mem_region(res->start, res->end - res->start + 1,
-				driver_name)) {
+	if (!request_mem_region(res->start, resource_len(res), driver_name)) {
 		ERR("request mem region for %s failed \n", pdev->name);
 		kfree(udc_controller);
 		return -EBUSY;
 	}
 
-	dr_regs = ioremap(res->start, res->end - res->start + 1);
+	dr_regs = ioremap(res->start, resource_len(res));
 	if (!dr_regs) {
 		ret = -ENOMEM;
 		goto err1;
@@ -2376,7 +2375,7 @@ err3:
 err2:
 	iounmap(dr_regs);
 err1:
-	release_mem_region(res->start, res->end - res->start + 1);
+	release_mem_region(res->start, resource_len(res));
 	kfree(udc_controller);
 	return ret;
 }
@@ -2405,7 +2404,7 @@ static int __exit fsl_udc_remove(struct 
 	dma_pool_destroy(udc_controller->td_pool);
 	free_irq(udc_controller->irq, udc_controller);
 	iounmap(dr_regs);
-	release_mem_region(res->start, res->end - res->start + 1);
+	release_mem_region(res->start, resource_len(res));
 
 	device_unregister(&udc_controller->gadget.dev);
 	/* free udc --wait for the release() finished */
--- at91.orig/drivers/usb/gadget/omap_udc.c	2007-10-13 15:16:13.000000000 -0700
+++ at91/drivers/usb/gadget/omap_udc.c	2007-10-19 11:43:12.000000000 -0700
@@ -2758,7 +2758,7 @@ static int __init omap_udc_probe(struct 
 
 	/* NOTE:  "knows" the order of the resources! */
 	if (!request_mem_region(pdev->resource[0].start,
-			pdev->resource[0].end - pdev->resource[0].start + 1,
+			resource_len(&pdev->resource[0]),
 			driver_name)) {
 		DBG("request_mem_region failed\n");
 		return -EBUSY;
@@ -2962,7 +2962,7 @@ cleanup0:
 	}
 
 	release_mem_region(pdev->resource[0].start,
-			pdev->resource[0].end - pdev->resource[0].start + 1);
+			resource_len(&pdev->resource[0]));
 
 	return status;
 }
@@ -3001,7 +3001,7 @@ static int __exit omap_udc_remove(struct
 	}
 
 	release_mem_region(pdev->resource[0].start,
-			pdev->resource[0].end - pdev->resource[0].start + 1);
+			resource_len(&pdev->resource[0]));
 
 	device_unregister(&udc->gadget.dev);
 	wait_for_completion(&done);
--- at91.orig/drivers/spi/atmel_spi.c	2007-10-19 11:38:28.000000000 -0700
+++ at91/drivers/spi/atmel_spi.c	2007-10-19 11:43:12.000000000 -0700
@@ -657,7 +657,7 @@ static int __init atmel_spi_probe(struct
 	spin_lock_init(&as->lock);
 	INIT_LIST_HEAD(&as->queue);
 	as->pdev = pdev;
-	as->regs = ioremap(regs->start, (regs->end - regs->start) + 1);
+	as->regs = ioremap(regs->start, resource_len(regs));
 	if (!as->regs)
 		goto out_free_buffer;
 	as->irq = irq;
--- at91.orig/drivers/spi/omap2_mcspi.c	2007-10-19 11:38:28.000000000 -0700
+++ at91/drivers/spi/omap2_mcspi.c	2007-10-19 11:43:12.000000000 -0700
@@ -964,8 +964,7 @@ static int __init omap2_mcspi_probe(stru
 		status = -ENODEV;
 		goto err1;
 	}
-	if (!request_mem_region(r->start, (r->end - r->start) + 1,
-			pdev->dev.bus_id)) {
+	if (!request_mem_region(r->start, resource_len(r), pdev->dev.bus_id)) {
 		status = -EBUSY;
 		goto err1;
 	}
@@ -1020,7 +1019,7 @@ err3:
 err2:
 	clk_put(mcspi->ick);
 err1a:
-	release_mem_region(r->start, (r->end - r->start) + 1);
+	release_mem_region(r->start, resource_len(r));
 err1:
 	spi_master_put(master);
 	return status;
@@ -1041,7 +1040,7 @@ static int __exit omap2_mcspi_remove(str
 	clk_put(mcspi->ick);
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(r->start, (r->end - r->start) + 1);
+	release_mem_region(r->start, resource_len(r));
 
 	spi_unregister_master(master);
 	kfree(dma_channels);
--- at91.orig/drivers/spi/spi_imx.c	2007-10-19 11:38:28.000000000 -0700
+++ at91/drivers/spi/spi_imx.c	2007-10-19 11:43:12.000000000 -0700
@@ -1487,15 +1487,14 @@ static int __init spi_imx_probe(struct p
 		status = -ENODEV;
 		goto err_no_iores;
 	}
-	drv_data->ioarea = request_mem_region(res->start,
-						res->end - res->start + 1,
+	drv_data->ioarea = request_mem_region(res->start, resource_len(res),
 						pdev->name);
 	if (drv_data->ioarea == NULL) {
 		dev_err(&pdev->dev, "probe - cannot reserve region\n");
 		status = -ENXIO;
 		goto err_no_iores;
 	}
-	drv_data->regs = ioremap(res->start, res->end - res->start + 1);
+	drv_data->regs = ioremap(res->start, resource_len(res));
 	if (drv_data->regs == NULL) {
 		dev_err(&pdev->dev, "probe - cannot map IO\n");
 		status = -ENXIO;
--- at91.orig/drivers/spi/spi_mpc83xx.c	2007-10-19 11:38:28.000000000 -0700
+++ at91/drivers/spi/spi_mpc83xx.c	2007-10-19 11:43:12.000000000 -0700
@@ -452,7 +452,7 @@ static int __init mpc83xx_spi_probe(stru
 	mpc83xx_spi->bitbang.master->setup = mpc83xx_spi_setup;
 	init_completion(&mpc83xx_spi->done);
 
-	mpc83xx_spi->base = ioremap(r->start, r->end - r->start + 1);
+	mpc83xx_spi->base = ioremap(r->start, resource_len(r));
 	if (mpc83xx_spi->base == NULL) {
 		ret = -ENOMEM;
 		goto put_master;
--- at91.orig/drivers/spi/spi_s3c24xx.c	2007-10-19 11:38:28.000000000 -0700
+++ at91/drivers/spi/spi_s3c24xx.c	2007-10-19 11:43:12.000000000 -0700
@@ -284,7 +284,7 @@ static int __init s3c24xx_spi_probe(stru
 		goto err_no_iores;
 	}
 
-	hw->ioarea = request_mem_region(res->start, (res->end - res->start)+1,
+	hw->ioarea = request_mem_region(res->start, resource_len(res),
 					pdev->name);
 
 	if (hw->ioarea == NULL) {
@@ -293,7 +293,7 @@ static int __init s3c24xx_spi_probe(stru
 		goto err_no_iores;
 	}
 
-	hw->regs = ioremap(res->start, (res->end - res->start)+1);
+	hw->regs = ioremap(res->start, resource_len(res));
 	if (hw->regs == NULL) {
 		dev_err(&pdev->dev, "Cannot map IO\n");
 		err = -ENXIO;
--- at91.orig/drivers/spi/spi_txx9.c	2007-10-19 11:38:28.000000000 -0700
+++ at91/drivers/spi/spi_txx9.c	2007-10-19 11:43:12.000000000 -0700
@@ -382,7 +382,7 @@ static int __init txx9spi_probe(struct p
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	if (!res)
 		goto exit;
-	c->membase = ioremap(res->start, res->end - res->start + 1);
+	c->membase = ioremap(res->start, resource_len(res));
 	if (!c->membase)
 		goto exit;
 
--- at91.orig/drivers/spi/xilinx_spi.c	2007-10-13 14:35:41.000000000 -0700
+++ at91/drivers/spi/xilinx_spi.c	2007-10-19 11:43:12.000000000 -0700
@@ -341,13 +341,12 @@ static int __init xilinx_spi_probe(struc
 	xspi->bitbang.master->setup = xilinx_spi_setup;
 	init_completion(&xspi->done);
 
-	if (!request_mem_region(r->start,
-			r->end - r->start + 1, XILINX_SPI_NAME)) {
+	if (!request_mem_region(r->start, resource_len(r), XILINX_SPI_NAME)) {
 		ret = -ENXIO;
 		goto put_master;
 	}
 
-	xspi->regs = ioremap(r->start, r->end - r->start + 1);
+	xspi->regs = ioremap(r->start, resource_len(r));
 	if (xspi->regs == NULL) {
 		ret = -ENOMEM;
 		goto put_master;

Re: [patch 2.6.24-rc1] resource_len() utility function

[Alan Cox]

On Wed, 24 Oct 2007 18:20:52 -0700
David Brownell <david-b@pacbell.net> wrote:

> Add a new resource_len() function, so drivers can start using this
> instead of driver-private code for a common idiom.  The call can be
> useful with at least:
> 
>  - request_region(), release_region()
>  - request_mem_region(), release_mem_region()
>  - ioremap()
> 
> Candidate drivers include those using platform or PNP busses, and
> maybe some others.  PCI already has a similar function.
> 
> This patch also updates a representative set of drivers in two
> subsystems to use this call (SPI, and USB peripheral/gadget).

PCI also increasingly is using functions that allow the user to choose to
map a resource as a resource (eg pci_iomap). So is it better to have
functions request_mem_resource(res)  free_mem_resource(res) and similar
instead or as well ?

Re: [patch 2.6.24-rc1] resource_len() utility function

[Jeff Garzik]

Alan Cox wrote:
> On Wed, 24 Oct 2007 18:20:52 -0700
> David Brownell <david-b@pacbell.net> wrote:
> 
>> Add a new resource_len() function, so drivers can start using this
>> instead of driver-private code for a common idiom.  The call can be
>> useful with at least:
>>
>>  - request_region(), release_region()
>>  - request_mem_region(), release_mem_region()
>>  - ioremap()
>>
>> Candidate drivers include those using platform or PNP busses, and
>> maybe some others.  PCI already has a similar function.
>>
>> This patch also updates a representative set of drivers in two
>> subsystems to use this call (SPI, and USB peripheral/gadget).
> 
> PCI also increasingly is using functions that allow the user to choose to
> map a resource as a resource (eg pci_iomap). So is it better to have
> functions request_mem_resource(res)  free_mem_resource(res) and similar
> instead or as well ?


With regards to resource reservation...  IMO we should mimic struct 
pci_dev and add struct resource[] to struct device.

Just like we have pci_request_regions(), we should also be able to 
easily to a dev_request_regions().  the implementation should be very 
close to pci_request_region() and pci_release_region().

Then a dev_iomap() analogue to pci_iomap() should be pretty 
straightforward to create.

	Jeff

Re: [patch 2.6.24-rc1] resource_len() utility function

[David Brownell]

On Wednesday 24 October 2007, Jeff Garzik wrote:
> With regards to resource reservation...  IMO we should mimic struct 
> pci_dev and add struct resource[] to struct device.

One minor difficulty:  PCI has a limit on the number of BARs,
but other busses don't.  It'd be better as a "struct resource *".


> Just like we have pci_request_regions(), we should also be able to 
> easily to a dev_request_regions().  the implementation should be very 
> close to pci_request_region() and pci_release_region().

Wouldn't it be nicer to have PCI use those dev_*() calls?
Ideally pci_request_region() wraps dev_request_region()...


> Then a dev_iomap() analogue to pci_iomap() should be pretty 
> straightforward to create.

Another minor nit:  addressing the various resource types.
The platform bus code has multiple lookup schemes. 

Calls like resource_iomap() might be more flexible, so that
lookup schemes can stay flexible.

- Dave

Re: [patch 2.6.24-rc1] resource_len() utility function

[Jeff Garzik]

David Brownell wrote:
> On Wednesday 24 October 2007, Jeff Garzik wrote:
>> With regards to resource reservation...  IMO we should mimic struct 
>> pci_dev and add struct resource[] to struct device.
> 
> One minor difficulty:  PCI has a limit on the number of BARs,
> but other busses don't.  It'd be better as a "struct resource *".
> 
> 
>> Just like we have pci_request_regions(), we should also be able to 
>> easily to a dev_request_regions().  the implementation should be very 
>> close to pci_request_region() and pci_release_region().
> 
> Wouldn't it be nicer to have PCI use those dev_*() calls?
> Ideally pci_request_region() wraps dev_request_region()...

Absolutely...


>> Then a dev_iomap() analogue to pci_iomap() should be pretty 
>> straightforward to create.
> 
> Another minor nit:  addressing the various resource types.
> The platform bus code has multiple lookup schemes. 
> 
> Calls like resource_iomap() might be more flexible, so that
> lookup schemes can stay flexible.

I figured that, in the absence of a true, defined BAR concept, the 
struct device version would simply index into the discussed array of 
struct resource.  That means any ordering or layout of resources should 
work, presuming (the usual case) that both driver and platform agree on 
the resource layout.

	Jeff

Re: [patch 2.6.24-rc1] resource_len() utility function

[David Brownell]

On Wednesday 24 October 2007, Jeff Garzik wrote:
> >> Then a dev_iomap() analogue to pci_iomap() should be pretty 
> >> straightforward to create.
> > 
> > Another minor nit:  addressing the various resource types.
> > The platform bus code has multiple lookup schemes. 
> > 
> > Calls like resource_iomap() might be more flexible, so that
> > lookup schemes can stay flexible.
> 
> I figured that, in the absence of a true, defined BAR concept, the 
> struct device version would simply index into the discussed array of 
> struct resource.  That means any ordering or layout of resources should 
> work, presuming (the usual case) that both driver and platform agree on 
> the resource layout.

So you'd suggest having search utilities (as with platform_bus)
returning resource indices not resources?

Thing is, BARs are usually well defined, but when folk glue
resources together they use whatever order is convenient on
that particular platform.  And different platforms can have
different numbers and types of resources, etc.

- Dave

Re: [patch 2.6.24-rc1] resource_len() utility function

[Alan Cox]

> So you'd suggest having search utilities (as with platform_bus)
> returning resource indices not resources?

That seems a bad idea to me

> Thing is, BARs are usually well defined, but when folk glue
> resources together they use whatever order is convenient on
> that particular platform.  And different platforms can have
> different numbers and types of resources, etc.

Far better I think that pci_ functions that take BAR values end up as
wrappers of the form

	pci_iomap(pdev, bar)
		return dev_iomap_resource(&pdev->resource[bar]);

Re: [patch 2.6.24-rc1] resource_len() utility function

[David Brownell]

On Thursday 25 October 2007, Alan Cox wrote:
> > So you'd suggest having search utilities (as with platform_bus)
> > returning resource indices not resources?
> 
> That seems a bad idea to me

I'm assuming you mean they should continue to work like they
do today:  return the resource.  Your pseudocode below shows
an iomap utility taking a resource.


> > Thing is, BARs are usually well defined, but when folk glue
> > resources together they use whatever order is convenient on
> > that particular platform.  And different platforms can have
> > different numbers and types of resources, etc.
> 
> Far better I think that pci_ functions that take BAR values end up as
> wrappers of the form
> 
> 	pci_iomap(pdev, bar)
> 		return dev_iomap_resource(&pdev->resource[bar]);

Sure, for PCI ... where the meaning of BARs is a fixed part of the
hardware spec, and it's not uncommon to skip a few.

But as I noted, that notion doesn't apply cleanly outside PCI;
indexes aren't necessarily portable between systems.  So any
such interface should discourage their use.


One issue with a dev_iomap() is that only memory resources
really need mapping ... but *all* of them need claiming.
(Modulo the detail that the iomap morphs i/o addresses too.)

The $SUBJECT function is a (minor) simplification for both
the mapping and claiming steps.

I think I'd rather see a dev_resource_claim() which combines
the request_{,mem_}region() semantics with mapping ... that
way drivers could save code, not just unify the two types
of register addressing.

- Dave

Re: [patch 2.6.24-rc1] resource_len() utility function

[David Brownell]

On Wednesday 24 October 2007, Alan Cox wrote:
> On Wed, 24 Oct 2007 18:20:52 -0700
> David Brownell <david-b@pacbell.net> wrote:
> 
> > Add a new resource_len() function, so drivers can start using this
> > instead of driver-private code for a common idiom.  The call can be
> > useful with at least:
> > 
> >  - request_region(), release_region()
> >  - request_mem_region(), release_mem_region()
> >  - ioremap()
> > 
> > Candidate drivers include those using platform or PNP busses, and
> > maybe some others.  PCI already has a similar function.
> > 
> > This patch also updates a representative set of drivers in two
> > subsystems to use this call (SPI, and USB peripheral/gadget).
> 
> PCI also increasingly is using functions that allow the user to choose to
> map a resource as a resource (eg pci_iomap). So is it better to have
> functions request_mem_resource(res)  free_mem_resource(res) and similar
> instead or as well ?

This was intended to be a minor band-aid.  ;)

We already have request_resource(), which does something
different than the request_*region() calls.  I think calls
with those names would complicate an already-too-strange
interface, adding oddball siblings to request_resource().

I'd hope that when those resource calls were defined they
made sense ... but to me, they don't do so today.  Consider
that the *typical* caller is given a "struct resource", and
then to claim the specified address space it must convert
it into a "start + length" representation before getting
back a *NEW* "struct resource" ... with identical contents,
other than the value of one all-but-undocumented flag bit.
Then, if it's I/O space the address is usable already; but
for memory space, it still needs an ioremap()...

Oh, and PCI has its own resource structs ("BAR") that don't
look or act the same as other resources.


So while I like the notion of starting to abolish that
conversion step, this wasn't an attempt to fix all the
bizarre behaviors of the resource API.

I could imagine a call taking a resource and returning
a "void __iomem *" to use for IO, which implicitly claims
the region (in either memory or i/o space) and does any
ioremap needed for memory space.  With a sibling call to
undo all that.  If that's the answer, someone else should
develop the patch and update drivers...

- Dave