[PATCH 1/2] mfd: max8925: request resource region

[PATCH 1/2] mfd: max8925: request resource region

[Haojian Zhuang]

If resource region isn't requested, component devices will fail to request
their resources.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/mfd/max8925-core.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/max8925-core.c b/drivers/mfd/max8925-core.c
index ca881ef..37aadcd 100644
--- a/drivers/mfd/max8925-core.c
+++ b/drivers/mfd/max8925-core.c
@@ -578,6 +578,10 @@ int __devinit max8925_device_init(struct max8925_chip *chip,
 {
 	int ret;
 
+	if (!request_region(0, 0xff, "max8925")) {
+		dev_err(chip->dev, "Failed to register resource region\n");
+		return -EBUSY;
+	}
 	max8925_irq_init(chip, chip->i2c->irq, pdata);
 
 	if (pdata && (pdata->power || pdata->touch)) {
-- 
1.7.5.4

[PATCH 2/2] ARM: mmp: add io head file

[Haojian Zhuang]

We need IO_SPACE_LIMIT definition. Otherwise, we can't register max8925
PMIC.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 arch/arm/Kconfig                    |    1 +
 arch/arm/mach-mmp/include/mach/io.h |   13 +++++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-mmp/include/mach/io.h

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 36586dba..d503539 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -636,6 +636,7 @@ config ARCH_MMP
 	select PLAT_PXA
 	select SPARSE_IRQ
 	select GENERIC_ALLOCATOR
+	select NEED_MACH_IO_H
 	help
 	  Support for Marvell's PXA168/PXA910(MMP) and MMP2 processor line.
 
diff --git a/arch/arm/mach-mmp/include/mach/io.h b/arch/arm/mach-mmp/include/mach/io.h
new file mode 100644
index 0000000..ad25061
--- /dev/null
+++ b/arch/arm/mach-mmp/include/mach/io.h
@@ -0,0 +1,13 @@
+/*
+ * arch/arm/mach-mmp/include/mach/io.h
+ *
+ */
+#ifndef __ARCH_MMP_IO_H
+#define __ARCH_MMP_IO_H
+
+#define IO_SPACE_LIMIT 0xffffffff
+
+#define __io(a)		((a) & IO_SPACE_LIMIT)
+
+#endif
+
-- 
1.7.5.4

[PATCH 2/2] ARM: mmp: add io head file

[Arnd Bergmann]

On Monday 07 May 2012, Haojian Zhuang wrote:
> +#ifndef __ARCH_MMP_IO_H
> +#define __ARCH_MMP_IO_H
> +
> +#define IO_SPACE_LIMIT 0xffffffff
> +
> +#define __io(a)                ((a) & IO_SPACE_LIMIT)
> +
> +#endif

NAK

If you don't have ISA or PCI devices, don't define these.

More importantly, never define them to an incorrect value. This
definition will cause an oops by NULL pointer dereferences for any
access to ISA devices such as writing to /dev/port, or loading a
module that probes the ISA ports.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Russell King - ARM Linux]

On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote:
> If resource region isn't requested, component devices will fail to request
> their resources.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>

This looks wrong.

This driver looks broken.  IO regions are for PC IO stuff, not for I2C
buses.  There is no resource reservation system in the kernel for
registers on I2C devices.

[PATCH 1/2] mfd: max8925: request resource region

[Haojian Zhuang]

On Mon, May 7, 2012 at 3:58 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote:
>> If resource region isn't requested, component devices will fail to request
>> their resources.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
>
> This looks wrong.
>
> This driver looks broken. ?IO regions are for PC IO stuff, not for I2C
> buses. ?There is no resource reservation system in the kernel for
> registers on I2C devices.

There's some components in PMIC devices. I used IO resources to
identify these components,
and these IO resources are register offset in PMIC. But if I choose
MEM resources, it may
conflict with real memory resources. So I choose IO resources. Do you
have any other implementation
instead?

Thanks
Haojian

[PATCH 1/2] mfd: max8925: request resource region

[Russell King - ARM Linux]

On Mon, May 07, 2012 at 04:18:40PM +0800, Haojian Zhuang wrote:
> On Mon, May 7, 2012 at 3:58 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote:
> >> If resource region isn't requested, component devices will fail to request
> >> their resources.
> >>
> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> >
> > This looks wrong.
> >
> > This driver looks broken. ?IO regions are for PC IO stuff, not for I2C
> > buses. ?There is no resource reservation system in the kernel for
> > registers on I2C devices.
> 
> There's some components in PMIC devices. I used IO resources to
> identify these components,
> and these IO resources are register offset in PMIC. But if I choose
> MEM resources, it may
> conflict with real memory resources. So I choose IO resources. Do you
> have any other implementation
> instead?

Which is wrong as I've said above.  This needs discussing and fixing;
this driver should never have been merged with this abuse in.

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 08:58:00AM +0100, Russell King - ARM Linux wrote:
> On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote:
> > If resource region isn't requested, component devices will fail to request
> > their resources.

> > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>

> This looks wrong.

> This driver looks broken.  IO regions are for PC IO stuff, not for I2C
> buses.  There is no resource reservation system in the kernel for
> registers on I2C devices.

They've commonly been used for this, it's a fairly sane way for an MFD
to communicate with its subdevices.  The fix in this series isn't good,
though - providing a parent resource with a suitable range for all the
device resources should do the job much more sensibly.  This isn't great
but the infrastructure seems to do the right thing with it for now and
it only requires a bit of reinterpretation of what IORESOURCE_IO means.

[PATCH 1/2] mfd: max8925: request resource region

[Russell King - ARM Linux]

On Mon, May 07, 2012 at 10:01:28AM +0100, Mark Brown wrote:
> On Mon, May 07, 2012 at 08:58:00AM +0100, Russell King - ARM Linux wrote:
> > On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote:
> > > If resource region isn't requested, component devices will fail to request
> > > their resources.
> 
> > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> 
> > This looks wrong.
> 
> > This driver looks broken.  IO regions are for PC IO stuff, not for I2C
> > buses.  There is no resource reservation system in the kernel for
> > registers on I2C devices.
> 
> They've commonly been used for this, it's a fairly sane way for an MFD
> to communicate with its subdevices.  The fix in this series isn't good,
> though - providing a parent resource with a suitable range for all the
> device resources should do the job much more sensibly.  This isn't great
> but the infrastructure seems to do the right thing with it for now and
> it only requires a bit of reinterpretation of what IORESOURCE_IO means.

It's an abuse.  And it won't work unless PCMCIA, PCI or ISA is enabled.
This abuse must stop, and it must stop right now.  And I really don't
care about "it's commonly been used for XYZ" because it's a totally fucked
idea.

What if you have two devices both claiming IO regions at 0?  Hint: it
fails.

It's buggered beyond belief and it needs to die right now, no questions
about it.  And anyone who supports this idea needs to be...

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 10:08:58AM +0100, Russell King - ARM Linux wrote:
> On Mon, May 07, 2012 at 10:01:28AM +0100, Mark Brown wrote:

> > They've commonly been used for this, it's a fairly sane way for an MFD
> > to communicate with its subdevices.  The fix in this series isn't good,
> > though - providing a parent resource with a suitable range for all the
> > device resources should do the job much more sensibly.  This isn't great
> > but the infrastructure seems to do the right thing with it for now and
> > it only requires a bit of reinterpretation of what IORESOURCE_IO means.

> It's an abuse.  And it won't work unless PCMCIA, PCI or ISA is enabled.

What causes it to fail if these are disabled?  I've got a bunch of
systems here which don't appear to have any of those enabled as far as I
can tell (though I don't know exactly what I'm looking for with ISA) but
seem to be using this quite happily for some time now.

> This abuse must stop, and it must stop right now.  And I really don't
> care about "it's commonly been used for XYZ" because it's a totally fucked
> idea.

I'd agree we should do something a bit nicer (though just having a
separate resource tree for the chip registers like I suggested does seem
like a reasonable first approach there).

> What if you have two devices both claiming IO regions at 0?  Hint: it
> fails.

Don't think I've got any examples with regions beginning at 0 but other
regions seem to not run into any problems with overlap.  Note that all
these drivers do with the regions is use them to look up the base
register.

> It's buggered beyond belief and it needs to die right now, no questions
> about it.  And anyone who supports this idea needs to be...

We have a bunch of drivers which have been in production for some time
now.  Simply saying the above without offering a constructive
alternative doesn't really help move anything forward here.  We could
add a new resource type but it's not clear to me that there's any need
to do so as it should be possible to arrange to avoid conflicts between
the different address ranges, and obviously there's no little space in
the bitmask used for resource types.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/dfc545ad/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Monday 07 May 2012, Mark Brown wrote:
> > What if you have two devices both claiming IO regions at 0?  Hint: it
> > fails.
> 
> Don't think I've got any examples with regions beginning at 0 but other
> regions seem to not run into any problems with overlap.  Note that all
> these drivers do with the regions is use them to look up the base
> register.

ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI
port number for relatively large values, but either ISA or PCMCIA or
an arbitrary MFD for relatively small values" is absolutely crazy.

FWIW, there are resources you define in include/linux/mfd/wm831x/core.h
that have values between 0x4000 and 0x8000, which is exactly the
range occupied by PCI devices on my thinkpad. It's only a matter of
time until someone puts conflicting devices into one machine.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Haojian Zhuang]

On Mon, May 7, 2012 at 6:14 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 07 May 2012, Mark Brown wrote:
>> > What if you have two devices both claiming IO regions at 0? ?Hint: it
>> > fails.
>>
>> Don't think I've got any examples with regions beginning at 0 but other
>> regions seem to not run into any problems with overlap. ?Note that all
>> these drivers do with the regions is use them to look up the base
>> register.
>
> ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI
> port number for relatively large values, but either ISA or PCMCIA or
> an arbitrary MFD for relatively small values" is absolutely crazy.
>
> FWIW, there are resources you define in include/linux/mfd/wm831x/core.h
> that have values between 0x4000 and 0x8000, which is exactly the
> range occupied by PCI devices on my thinkpad. It's only a matter of
> time until someone puts conflicting devices into one machine.
>
> ? ? ? ?Arnd

Even we append a new IORESOURCE type. We still meet similar issue. So
it means that we can't use any IORESOURCE for this kind of i2c usage. Is
it right?

Regards
Haojian

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Monday 07 May 2012, Haojian Zhuang wrote:
> Even we append a new IORESOURCE type. We still meet similar issue. So
> it means that we can't use any IORESOURCE for this kind of i2c usage. Is
> it right?

Yes, basically the concept of IORESOURCE is incompatible here, unless you
want to add a new IORESOURCE type for each one of these drivers, or you
find a way to put all these devices into one global register space at
unique addresses, which doesn't sound any better.

Can you explain why you need this kind of arbitration to start with?
Can't you just ensure that each client of the max8925 only sees a fixed
set of nonconflicting registers, and provide a higher-level abstractions
for the registers that are indeed shared between clients?

Wouldn't you always know which drivers potentially coexist here?

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 11:21:53AM +0000, Arnd Bergmann wrote:

> Can you explain why you need this kind of arbitration to start with?
> Can't you just ensure that each client of the max8925 only sees a fixed
> set of nonconflicting registers, and provide a higher-level abstractions
> for the registers that are indeed shared between clients?

This is nothing to do with arbitration or sharing.  It's for the case
where you have a set of IP blocks on the chip (and possibly over a
series of different chips) all with the same register map within the IP
block - you need a way to tell the function driver for the IP block
where it is in the chip register map.  A similar thing happens (without
issue) for the interrupts within the chip.

You'd never expect any collisions to need arbitrating, it's purely about
telling the function driver where to find the IP without having to open
code this.  Anything which is actually shared would be handled in the
MFD core for the device normally, or with some other API like genirq.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/c9432b9c/attachment-0001.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Samuel Ortiz]

Hi Arnd,

On Mon, May 07, 2012 at 01:19:48PM +0000, Arnd Bergmann wrote:
> On Monday 07 May 2012, Mark Brown wrote:
> > On Mon, May 07, 2012 at 11:21:53AM +0000, Arnd Bergmann wrote:
> > 
> > > Can you explain why you need this kind of arbitration to start with?
> > > Can't you just ensure that each client of the max8925 only sees a fixed
> > > set of nonconflicting registers, and provide a higher-level abstractions
> > > for the registers that are indeed shared between clients?
> > 
> > This is nothing to do with arbitration or sharing.  It's for the case
> > where you have a set of IP blocks on the chip (and possibly over a
> > series of different chips) all with the same register map within the IP
> > block - you need a way to tell the function driver for the IP block
> > where it is in the chip register map.  A similar thing happens (without
> > issue) for the interrupts within the chip.
> > 
> > You'd never expect any collisions to need arbitrating, it's purely about
> > telling the function driver where to find the IP without having to open
> > code this.  Anything which is actually shared would be handled in the
> > MFD core for the device normally, or with some other API like genirq.
> 
> The patch that we are discussing adds a call to 'request_region' --
> that is the arbitration interface and that is what is causing the
> conflicts.
> 
> Using a 'struct resource' of type IORESOURCE_IO to pass information
> about non-PIO registers to child devices is inconsistent, ugly and
> confusing, but I agree it doesn't actually result in bugs. The problems
> start when you use those resources in combination with request_region,
> request_resource and ioport_map.
I agree with the latter and decided to not push this patch forward.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 01:19:48PM +0000, Arnd Bergmann wrote:
> On Monday 07 May 2012, Mark Brown wrote:

> > You'd never expect any collisions to need arbitrating, it's purely about
> > telling the function driver where to find the IP without having to open
> > code this.  Anything which is actually shared would be handled in the
> > MFD core for the device normally, or with some other API like genirq.

> The patch that we are discussing adds a call to 'request_region' --
> that is the arbitration interface and that is what is causing the
> conflicts.

Oh dear - that's clearly broken, I'd not actually read the original
patch just the thread that followed.

> Using a 'struct resource' of type IORESOURCE_IO to pass information
> about non-PIO registers to child devices is inconsistent, ugly and
> confusing, but I agree it doesn't actually result in bugs. The problems

I actually don't find it at all confusing, but I think that's mostly
because at a high level I look at _IO as being about a register space
and this is just a different register space.

> start when you use those resources in combination with request_region,
> request_resource and ioport_map.

Right, yes - that's clearly never going to work.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/840207e1/attachment-0001.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Monday 07 May 2012, Mark Brown wrote:
> > Using a 'struct resource' of type IORESOURCE_IO to pass information
> > about non-PIO registers to child devices is inconsistent, ugly and
> > confusing, but I agree it doesn't actually result in bugs. The problems
> 
> I actually don't find it at all confusing, but I think that's mostly
> because at a high level I look at _IO as being about a register space
> and this is just a different register space.

At least it managed to confuse Haojian ;-)

The part that confuses me most about the abuse of IORESOURCE_IO is that
these have no parent resource and are not registered or listed in
/proc/ioports.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 03:15:08PM +0000, Arnd Bergmann wrote:

> The part that confuses me most about the abuse of IORESOURCE_IO is that
> these have no parent resource and are not registered or listed in
> /proc/ioports.

Well, if you're going to get confused about that sort of stuff there's
also the fact that we've got platform devices which aren't the sort of
simple memory mapped things that the platform bus is supposed to be for
since any other bus for this sort of stuff would just be a straight
copy of the platform bus.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/081ec910/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 10:14:03AM +0000, Arnd Bergmann wrote:
> On Monday 07 May 2012, Mark Brown wrote:

> > Don't think I've got any examples with regions beginning at 0 but other
> > regions seem to not run into any problems with overlap.  Note that all
> > these drivers do with the regions is use them to look up the base
> > register.

> ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI
> port number for relatively large values, but either ISA or PCMCIA or
> an arbitrary MFD for relatively small values" is absolutely crazy.

So what you're saying is that it's nothing to do with zero and just
about plain conflicts - there's nothing magic about zero (which is what
Russell seemed to be suggesting)?  For whatever reason in the MFD usage
the conflicts don't seem to be triggering - I suspect it's because the
struct resource is simply inspected by the driver rather than doing
whatever it is that PCI/ISA devices do with them, though I've not
checked recently.

> FWIW, there are resources you define in include/linux/mfd/wm831x/core.h
> that have values between 0x4000 and 0x8000, which is exactly the
> range occupied by PCI devices on my thinkpad. It's only a matter of
> time until someone puts conflicting devices into one machine.

Yes, I agree it will be a problem if they're part of the same resource
tree and used via the same API but if we allow multiple trees of I/O
resources then does that avoid the issue?  It seems like it ought to be
something we can do, if not we'll have to define a new resource type and
like I say it's a pretty full bitmask at the minute...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/72995196/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 01:14:51PM +0000, Arnd Bergmann wrote:

> There are two distinct problems here:

> * conflicts in request_resource() between stuff that is in fundamentally
>   different.
> * defining the __io() address space to have a zero offset, which causes
>   NULL pointer dereferences in legacy ISA drivers.

Right, though in the MFD case neither of these things should be relevant
as the resources should never actually be used in this way.

> My feeling is that the resource model is just hasn't moved along
> with the times (predates and doesn't really map to the device model) and
> is used in a consistent way (request_resource, allocate_resource and
> request_region operate on the same types, but in different ways).
> It's not clear to me whether it makes sense to continue this path
> for new kinds of resources.

This is true.  On the other hand there's some infrastructure around
resources which is pretty helpful, though now I look at it most of this
is actually specific to platform devices rather than being a platform
device wrapper for a generic thing.  Things like platform_get_resource()
are pretty good to use, and in the context of platform devices the whole
resource conflict thing is normally pretty much irrelevant.

> My understanding is also that the uses in MFD (e.g. max8925 and wm831x)
> are only interested in the aspect of passing information to child devices
> rather than arbitrating between conflicting accesses. If that's the case,
> a separate mechanism that doesn't use a global numbering scheme might
> be more appropriate.

Given what I'm saying about platform devices above perhaps we should be
factoring some of the platform device stuff up to struct device level.
Another option would be to work on separating the management of the
number spaces and the interfaces for getting the numbers back out to
make it easier to add more number spaces.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/5b4129d5/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Monday 07 May 2012, Mark Brown wrote:
> On Mon, May 07, 2012 at 01:14:51PM +0000, Arnd Bergmann wrote:
> 
> > There are two distinct problems here:
> 
> > * conflicts in request_resource() between stuff that is in fundamentally
> >   different.
> > * defining the __io() address space to have a zero offset, which causes
> >   NULL pointer dereferences in legacy ISA drivers.
> 
> Right, though in the MFD case neither of these things should be relevant
> as the resources should never actually be used in this way.

Well, particularly patch 2 of this series introduces those problems,
but yes, it should be a separate issue.

> > My feeling is that the resource model is just hasn't moved along
> > with the times (predates and doesn't really map to the device model) and
> > is used in a consistent way (request_resource, allocate_resource and
> > request_region operate on the same types, but in different ways).
> > It's not clear to me whether it makes sense to continue this path
> > for new kinds of resources.
> 
> This is true.  On the other hand there's some infrastructure around
> resources which is pretty helpful, though now I look at it most of this
> is actually specific to platform devices rather than being a platform
> device wrapper for a generic thing.  Things like platform_get_resource()
> are pretty good to use, and in the context of platform devices the whole
> resource conflict thing is normally pretty much irrelevant.

Right.

> > My understanding is also that the uses in MFD (e.g. max8925 and wm831x)
> > are only interested in the aspect of passing information to child devices
> > rather than arbitrating between conflicting accesses. If that's the case,
> > a separate mechanism that doesn't use a global numbering scheme might
> > be more appropriate.
> 
> Given what I'm saying about platform devices above perhaps we should be
> factoring some of the platform device stuff up to struct device level.
> Another option would be to work on separating the management of the
> number spaces and the interfaces for getting the numbers back out to
> make it easier to add more number spaces.

Isn't that what devres is for? We should be able to just attach arbitrary
data to a device with this, e.g. a struct regmap to use for doing I/O
that a driver can use. Maybe we should add some wrappers around that
to make it more obvious to use.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 03:09:54PM +0000, Arnd Bergmann wrote:
> On Monday 07 May 2012, Mark Brown wrote:

> > Given what I'm saying about platform devices above perhaps we should be
> > factoring some of the platform device stuff up to struct device level.
> > Another option would be to work on separating the management of the
> > number spaces and the interfaces for getting the numbers back out to
> > make it easier to add more number spaces.

> Isn't that what devres is for? We should be able to just attach arbitrary
> data to a device with this, e.g. a struct regmap to use for doing I/O
> that a driver can use. Maybe we should add some wrappers around that
> to make it more obvious to use.

Not as far as I understand it, it's more about making error handling and
unregistration code less error prone by automating the freeing of
things when a device goes away or probe() fails.  The managed versions
wrap the unmanaged allocators but don't generally otherwise change the
interface except by adding a struct device.  Unless I'm missing
something, of course.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/6dcfa6bf/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Monday 07 May 2012, Mark Brown wrote:
> On Mon, May 07, 2012 at 03:09:54PM +0000, Arnd Bergmann wrote:
> > On Monday 07 May 2012, Mark Brown wrote:
> 
> > > Given what I'm saying about platform devices above perhaps we should be
> > > factoring some of the platform device stuff up to struct device level.
> > > Another option would be to work on separating the management of the
> > > number spaces and the interfaces for getting the numbers back out to
> > > make it easier to add more number spaces.
> 
> > Isn't that what devres is for? We should be able to just attach arbitrary
> > data to a device with this, e.g. a struct regmap to use for doing I/O
> > that a driver can use. Maybe we should add some wrappers around that
> > to make it more obvious to use.
> 
> Not as far as I understand it, it's more about making error handling and
> unregistration code less error prone by automating the freeing of
> things when a device goes away or probe() fails.  The managed versions
> wrap the unmanaged allocators but don't generally otherwise change the
> interface except by adding a struct device.  Unless I'm missing
> something, of course.

Yes, it also does that, but have a look at how drivers/pci/pci.c uses
devres to attach auxiliary attributes to a device. We can do the same
thing for other devices to attach any data.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 07:26:08PM +0000, Arnd Bergmann wrote:
> On Monday 07 May 2012, Mark Brown wrote:
> > On Mon, May 07, 2012 at 03:09:54PM +0000, Arnd Bergmann wrote:
> > > On Monday 07 May 2012, Mark Brown wrote:

> > > > Given what I'm saying about platform devices above perhaps we should be
> > > > factoring some of the platform device stuff up to struct device level.

> > > Isn't that what devres is for? We should be able to just attach arbitrary
> > > data to a device with this, e.g. a struct regmap to use for doing I/O
> > > that a driver can use. Maybe we should add some wrappers around that
> > > to make it more obvious to use.

> > Not as far as I understand it, it's more about making error handling and
> > unregistration code less error prone by automating the freeing of
> > things when a device goes away or probe() fails.  The managed versions

> Yes, it also does that, but have a look at how drivers/pci/pci.c uses
> devres to attach auxiliary attributes to a device. We can do the same
> thing for other devices to attach any data.

Right, but that's essentially just the bus doing the equivalent of
devm_kzalloc() to allocate some private data which is a separate thing.
It doesn't really address the problem of passing information from
registration code to the device through the bus which is what the
resource API is currently doing for these devices.

We probably should be making a bit more use of devres for this than we
are (using it to bounce through a regmap isn't a bad idea, I might just
do that though a lot of the time you need the parent internal data
struct anyway) but it's a separate thing.

Something like pulling the type out of the flags in struct resource to
give us a bit more space (or replace the bits with a pointer, though
that's even more invasive) would address this more...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/564ed995/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 07:26:08PM +0000, Arnd Bergmann wrote:

> Yes, it also does that, but have a look at how drivers/pci/pci.c uses
> devres to attach auxiliary attributes to a device. We can do the same
> thing for other devices to attach any data.

Actually, thinking about this further devres is definitely not what we
want here: when the device is unbound it'll free all the resources it
has for the device which is definitely not what we want for things like
this.  We need the data to persist for as long as the device exists.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120508/cc6b4ac2/attachment-0001.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Tuesday 08 May 2012, Mark Brown wrote:
>   On Mon, May 07, 2012 at 07:26:08PM +0000, Arnd Bergmann wrote:
> 
> > Yes, it also does that, but have a look at how drivers/pci/pci.c uses
> > devres to attach auxiliary attributes to a device. We can do the same
> > thing for other devices to attach any data.
> 
> Actually, thinking about this further devres is definitely not what we
> want here: when the device is unbound it'll free all the resources it
> has for the device which is definitely not what we want for things like
> this.  We need the data to persist for as long as the device exists.

Yes, good point.

How about a IORESOURCE_REGMAP type then with the following semantics:
Each struct regmap gets an embedded resource that gets a unique
range in the IORESOURCE_REGMAP space using allocate_resource,
and each device using that can have its own sub-resources registered
to that.

Then we add a helper function that pulls out the regmap from the resource
using something like container_of(res->parent, struct regmap, resource)
and calculates the register number by subtracting the parent->start from
res->start.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Tue, May 08, 2012 at 02:44:30PM +0000, Arnd Bergmann wrote:

> How about a IORESOURCE_REGMAP type then with the following semantics:

The trick is allocating a bit for that flag, though I was thinking
earlier on we might be able to get away with carving it out of the bus
specific bits since I think anything using this wouldn't normally be
using resources for anything else except interrupts.  It does mean that
the resource type doesn't do the right thing though.

> Each struct regmap gets an embedded resource that gets a unique
> range in the IORESOURCE_REGMAP space using allocate_resource,
> and each device using that can have its own sub-resources registered
> to that.

> Then we add a helper function that pulls out the regmap from the resource
> using something like container_of(res->parent, struct regmap, resource)
> and calculates the register number by subtracting the parent->start from
> res->start.

That feels complicated with the subtraction there, but modulo that this
is roughly what's happening now.  We would I think want this to work for
non-regmap users too, it's not yet clear that every device with
registers is going to fit into regmap (though it's looking more likely
as things go on).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120508/d4e2a8c4/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Tuesday 08 May 2012, Mark Brown wrote:
> On Tue, May 08, 2012 at 02:44:30PM +0000, Arnd Bergmann wrote:
> 
> > How about a IORESOURCE_REGMAP type then with the following semantics:
> 
> The trick is allocating a bit for that flag, though I was thinking
> earlier on we might be able to get away with carving it out of the bus
> specific bits since I think anything using this wouldn't normally be
> using resources for anything else except interrupts.  It does mean that
> the resource type doesn't do the right thing though.

Right, or we could try to kill IORESOURCE_BUS, which is hardly used
anywhere and the users either get it wrong (bfin net2272), use it
only in one file (acpi, broadcom-pci) or only for printing the
contents (pnp).

> > Each struct regmap gets an embedded resource that gets a unique
> > range in the IORESOURCE_REGMAP space using allocate_resource,
> > and each device using that can have its own sub-resources registered
> > to that.
> 
> > Then we add a helper function that pulls out the regmap from the resource
> > using something like container_of(res->parent, struct regmap, resource)
> > and calculates the register number by subtracting the parent->start from
> > res->start.
> 
> That feels complicated with the subtraction there, but modulo that this
> is roughly what's happening now.  We would I think want this to work for
> non-regmap users too, it's not yet clear that every device with
> registers is going to fit into regmap (though it's looking more likely
> as things go on).

I think the subtraction in some form is needed if you want to register
the resources, to guarantee that they are non-conflicting. Registering
them has the advantage that we can print them in a similar way to what
we do for /proc/{ioports,iomem}.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Wed, May 09, 2012 at 12:43:13PM +0000, Arnd Bergmann wrote:
> On Tuesday 08 May 2012, Mark Brown wrote:

> > The trick is allocating a bit for that flag, though I was thinking
> > earlier on we might be able to get away with carving it out of the bus
> > specific bits since I think anything using this wouldn't normally be
> > using resources for anything else except interrupts.  It does mean that
> > the resource type doesn't do the right thing though.

> Right, or we could try to kill IORESOURCE_BUS, which is hardly used
> anywhere and the users either get it wrong (bfin net2272), use it
> only in one file (acpi, broadcom-pci) or only for printing the
> contents (pnp).

Yeah, I looked at that - PCMCIA seems to be the major sticking point and
the usage there appeared totally sane, though it's possible that if I
stare at it hard enough I might convince myself that it only needs seven
bits and we can steal one.

> > That feels complicated with the subtraction there, but modulo that this
> > is roughly what's happening now.  We would I think want this to work for
> > non-regmap users too, it's not yet clear that every device with
> > registers is going to fit into regmap (though it's looking more likely
> > as things go on).

> I think the subtraction in some form is needed if you want to register
> the resources, to guarantee that they are non-conflicting. Registering
> them has the advantage that we can print them in a similar way to what
> we do for /proc/{ioports,iomem}.

It does result in the really odd situation that the number that gets
stored in the resource is the register number plus an offset.  If
anything I'd expect that the resources would be defined so you had to
add the parent base address with the child resource being the offset
within the parent resource.  Though to be honest the parent resource is
almost always going to be numbered from zero so it's not going to make
much difference most of the time.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/759ff3cc/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Wednesday 09 May 2012, Mark Brown wrote:
> It does result in the really odd situation that the number that gets
> stored in the resource is the register number plus an offset.  If
> anything I'd expect that the resources would be defined so you had to
> add the parent base address with the child resource being the offset
> within the parent resource.  Though to be honest the parent resource is
> almost always going to be numbered from zero so it's not going to make
> much difference most of the time.

But that's not how any of the other resources work: AFAICT, each resource
type makes up an address space with unique ranges assigned to each 
sibling and sub-ranges inside of that for its children.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Wed, May 09, 2012 at 02:19:50PM +0000, Arnd Bergmann wrote:
> On Wednesday 09 May 2012, Mark Brown wrote:

> > It does result in the really odd situation that the number that gets
> > stored in the resource is the register number plus an offset.  If
> > anything I'd expect that the resources would be defined so you had to
> > add the parent base address with the child resource being the offset
> > within the parent resource.  Though to be honest the parent resource is
> > almost always going to be numbered from zero so it's not going to make
> > much difference most of the time.

> But that's not how any of the other resources work: AFAICT, each resource
> type makes up an address space with unique ranges assigned to each 
> sibling and sub-ranges inside of that for its children.

That's what I'd expect, but you're saying that the values in the child
resources are being stored as base+actual values (where base is some
random number) rather than either actual values or as offsets within the
base range either of which would seem less surprising.  Presumably you'd
also need to walk up the tree to subtract all the bases if there was
more than one resource.

Perhaps I'm just totally failing to understand what you're saying?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/70a9fd82/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Wednesday 09 May 2012, Mark Brown wrote:
> That's what I'd expect, but you're saying that the values in the child
> resources are being stored as base+actual values (where base is some
> random number) rather than either actual values or as offsets within the
> base range either of which would seem less surprising.  Presumably you'd
> also need to walk up the tree to subtract all the bases if there was
> more than one resource.
> 
> Perhaps I'm just totally failing to understand what you're saying?

I mean that each device providing resources of the new type would need
to register its resources to the same root resource with non-conclicting
addresses. Having resources hanging in the air without a parent pointer
is not good, although we seem to be doing that for a lot of platform
devices these days, where we just pass the struct resource from platform
code to a driver without ever registering it at each end.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Wed, May 09, 2012 at 03:03:22PM +0000, Arnd Bergmann wrote:

> I mean that each device providing resources of the new type would need
> to register its resources to the same root resource with non-conclicting

Oh, right - I see what you mean.  I was thinking of creating a
per-chip parent for each tree rather than trying to make them a unified
tree, though I guess it's useful for diagnostics.  Multi-level resources
might be slightly fun with this though I'm not sure how realistic they
are.

> addresses. Having resources hanging in the air without a parent pointer
> is not good, although we seem to be doing that for a lot of platform
> devices these days, where we just pass the struct resource from platform
> code to a driver without ever registering it at each end.

They should all be being registered by the platform core when the
platform device is registered I think?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/c99a5363/attachment-0001.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Wednesday 09 May 2012, Mark Brown wrote:
>   Show Details
>   On Wed, May 09, 2012 at 03:03:22PM +0000, Arnd Bergmann wrote:
> 
> > I mean that each device providing resources of the new type would need
> > to register its resources to the same root resource with non-conclicting
> 
> Oh, right - I see what you mean.  I was thinking of creating a
> per-chip parent for each tree rather than trying to make them a unified
> tree, though I guess it's useful for diagnostics.  Multi-level resources
> might be slightly fun with this though I'm not sure how realistic they
> are.

Well, as Russell just pointed out, that's not actually a requirement,
so we probably don't want to do that after all.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Russell King - ARM Linux]

On Wed, May 09, 2012 at 03:13:16PM +0100, Mark Brown wrote:
> On Wed, May 09, 2012 at 12:43:13PM +0000, Arnd Bergmann wrote:
> > On Tuesday 08 May 2012, Mark Brown wrote:
> 
> > > The trick is allocating a bit for that flag, though I was thinking
> > > earlier on we might be able to get away with carving it out of the bus
> > > specific bits since I think anything using this wouldn't normally be
> > > using resources for anything else except interrupts.  It does mean that
> > > the resource type doesn't do the right thing though.
> 
> > Right, or we could try to kill IORESOURCE_BUS, which is hardly used
> > anywhere and the users either get it wrong (bfin net2272), use it
> > only in one file (acpi, broadcom-pci) or only for printing the
> > contents (pnp).
> 
> Yeah, I looked at that - PCMCIA seems to be the major sticking point and
> the usage there appeared totally sane, though it's possible that if I
> stare at it hard enough I might convince myself that it only needs seven
> bits and we can steal one.

Err what?  PCMCIA doesn't use IORESOURCE_BUS.  (Don't get it confused with
IORESOURCE_BUSY which is totally different).

IORESOURCE_BUS seems to be used for:

net2272: specifying the register index shift value
acpi: to get the start/end PCI bus number values
pnp: for ACPI bus numbers
x86: PCI bus numbering

So it looks like net2272 is abusing this resource type for its own
purposes...  The others look like proper use of this resource type.

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Wed, May 09, 2012 at 05:18:39PM +0100, Russell King - ARM Linux wrote:
> On Wed, May 09, 2012 at 03:13:16PM +0100, Mark Brown wrote:

> > Yeah, I looked at that - PCMCIA seems to be the major sticking point and
> > the usage there appeared totally sane, though it's possible that if I
> > stare at it hard enough I might convince myself that it only needs seven
> > bits and we can steal one.

> Err what?  PCMCIA doesn't use IORESOURCE_BUS.  (Don't get it confused with
> IORESOURCE_BUSY which is totally different).

I've actually got it confused with IORESOURCE_BITS (because it's there
for bus-specific usage, as opposed to bus numbers) which it does use -
sorry.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/40e330c3/attachment.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Russell King - ARM Linux]

On Wed, May 09, 2012 at 12:43:13PM +0000, Arnd Bergmann wrote:
> I think the subtraction in some form is needed if you want to register
> the resources, to guarantee that they are non-conflicting. Registering
> them has the advantage that we can print them in a similar way to what
> we do for /proc/{ioports,iomem}.

You don't need to - you only need them non-conflicting if they're part of
the same tree.

It seems to me that having all device register resources being part of
the same tree is the wrong approach, it's certainly the wrong model for
this kind of thing.

Instead, we should be having per-device resources trees.  So, for instance,
an I2C device with two functions, function 1 say owns register indices
0-0x3f and function 2 owns 0x40-0x7f.  We should have:

struct i2c_blah_driver_info {
	struct resource reg_res;
};

	priv->reg_res.start = 0;
	priv->reg_res.end = 0x7f;
	priv->reg_res.flags = 0;

	subdev1->resource[0].start = 0;
	subdev1->resource[0].end = 0x3f;
	subdev1->resource[0].flags = 0;

	ret = request_resource(&priv->reg_res, &subdev1->resource[0]);
	/* check ret */

	subdev2->resource[0].start = 0x40;
	subdev2->resource[0].end = 0x7f;
	subdev2->resource[0].flags = 0;

	ret = request_resource(&priv->reg_res, &subdev2->resource[0]);
	/* check ret */

This means that we end up with lots of per-device resource trees rooted
at the top-level-device driver, each effectively with their own separate
name spaces.

If we care about marking resources busy to avoid drivers conflicting, I
would also suggest that we augment the resource interfaces so that we can
have mark_resource_busy(dev, res, drivername) which takes the 'bus'
resource and adds the IORESOURCE_BUSY resource below it.  I'd also suggest
at this point that we don't have a corresponding release function for this
as I believe we should be using devm stuff internally for this now.

Do we care about making them appear in procfs?  I suspect at this stage
that would be wrong.  If we care at all, they should probably be per-
top-level-device sysfs files - and we should have a function which sysfs
can use along with the root resource to create that output.

[PATCH 1/2] mfd: max8925: request resource region

[Arnd Bergmann]

On Wednesday 09 May 2012, Russell King - ARM Linux wrote:
> Instead, we should be having per-device resources trees.  So, for instance,
> an I2C device with two functions, function 1 say owns register indices
> 0-0x3f and function 2 owns 0x40-0x7f.  We should have:
> 
> struct i2c_blah_driver_info {
>         struct resource reg_res;
> };
> 
>         priv->reg_res.start = 0;
>         priv->reg_res.end = 0x7f;
>         priv->reg_res.flags = 0;
> 
>         subdev1->resource[0].start = 0;
>         subdev1->resource[0].end = 0x3f;
>         subdev1->resource[0].flags = 0;
> 
>         ret = request_resource(&priv->reg_res, &subdev1->resource[0]);
>         /* check ret */
> 
>         subdev2->resource[0].start = 0x40;
>         subdev2->resource[0].end = 0x7f;
>         subdev2->resource[0].flags = 0;
> 
>         ret = request_resource(&priv->reg_res, &subdev2->resource[0]);
>         /* check ret */
> 
> This means that we end up with lots of per-device resource trees rooted
> at the top-level-device driver, each effectively with their own separate
> name spaces.

Ah, I had not realized that this is possible. In this case, it certainly
makes sense to have a separate space for each device that provides a
register range.

> Do we care about making them appear in procfs?  I suspect at this stage
> that would be wrong.  If we care at all, they should probably be per-
> top-level-device sysfs files - and we should have a function which sysfs
> can use along with the root resource to create that output.

If the resources have separate roots, I see no way how to make them
all appear in the same procfs file. Having them represented in sysfs
the way you describe sounds useful but not important.

	Arnd

[PATCH 1/2] mfd: max8925: request resource region

[Russell King - ARM Linux]

On Wed, May 09, 2012 at 04:26:27PM +0000, Arnd Bergmann wrote:
> On Wednesday 09 May 2012, Russell King - ARM Linux wrote:
> > Do we care about making them appear in procfs?  I suspect at this stage
> > that would be wrong.  If we care at all, they should probably be per-
> > top-level-device sysfs files - and we should have a function which sysfs
> > can use along with the root resource to create that output.
> 
> If the resources have separate roots, I see no way how to make them
> all appear in the same procfs file. Having them represented in sysfs
> the way you describe sounds useful but not important.

I'd agree with that.  I included that statement as food for thought if
someone _did_ want this.

[PATCH 1/2] mfd: max8925: request resource region

[Russell King - ARM Linux]

On Mon, May 07, 2012 at 11:37:29AM +0100, Mark Brown wrote:
> On Mon, May 07, 2012 at 10:14:03AM +0000, Arnd Bergmann wrote:
> > On Monday 07 May 2012, Mark Brown wrote:
> 
> > > Don't think I've got any examples with regions beginning at 0 but other
> > > regions seem to not run into any problems with overlap.  Note that all
> > > these drivers do with the regions is use them to look up the base
> > > register.
> 
> > ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI
> > port number for relatively large values, but either ISA or PCMCIA or
> > an arbitrary MFD for relatively small values" is absolutely crazy.
> 
> So what you're saying is that it's nothing to do with zero and just
> about plain conflicts - there's nothing magic about zero (which is what
> Russell seemed to be suggesting)?

You've totally missed my point if that's what you think I was saying.

The resource system as it stands handles two types of resources:

1. MMIO resources, of type IORESOURCE_MEM, registered against the
   iomem_resource root resource
2. PCI/ISA IO resources, of type IORESOURCE_IO, registered against the
   ioport_resource root resource

The two root resources are hard coded into the BUSY-marking request/release
APIs.

If you don't have PCMCIA/ISA/PCI (and their derivatives) then the ioport
resource ends up being zero sized, and therefore child resources fail to
register against it (we want this behaviour to prevent ISA drivers working
on platforms without ISA.)

The problem comes if you start abusing IORESOURCE_IO - which has _always_
meant "this is a PCI/ISA IO resource", and then you have someone adding
calls to request_region() etc.  That will make stuff look at the
ioport_resource root, and it won't work.

If you _do_ have PCMCIA/ISA/PCI, then request_region() may well succeed.
However, if you have two drivers playing this game, with overlapping
start/end, _even_ if they're totally unrelated devices which would never
clash, the APIs will fail just because of the _numeric_ overlap.  The
resource management subsystem really doesn't have any real knowledge of
anything beyond "MMIO" and "PCI/ISA IO" resources.

So, using IORESOURCE_IO for device-internal register spaces is an abuse
of what this resource type was meant for, and leads to people getting
confused about how to deal with this resource type (that's proven by
Haojian's patch.)

We really don't have a resource handling subsystem that sanely deals with
device private register spaces.

You can bodge around it by using request_resource() with specific parents,
which themselves are disconnected from the above two root resources, but
at the end of the day you're bodging around to try and get the resource
stuff to do something it was never designed to.

In any case, if the resource tree is disconnected from the main two parent
resources, then the resource probably shouldn't have either IORESOURCE_IO
nor IORESOURCE_MEM set in it.  It's neither of those two resource types.

[PATCH 1/2] mfd: max8925: request resource region

[Mark Brown]

On Mon, May 07, 2012 at 07:57:35PM +0100, Russell King - ARM Linux wrote:
> On Mon, May 07, 2012 at 11:37:29AM +0100, Mark Brown wrote:

> > So what you're saying is that it's nothing to do with zero and just
> > about plain conflicts - there's nothing magic about zero (which is what
> > Russell seemed to be suggesting)?

> You've totally missed my point if that's what you think I was saying.

I don't think I missed anything.  I understood you were saying you think
this is a terrible idea; I had thought you were saying that it was an
even worse idea for a value of zero for some reason (ie, that that broke
things further).

> The resource system as it stands handles two types of resources:

> 1. MMIO resources, of type IORESOURCE_MEM, registered against the
>    iomem_resource root resource
> 2. PCI/ISA IO resources, of type IORESOURCE_IO, registered against the
>    ioport_resource root resource

> The two root resources are hard coded into the BUSY-marking request/release
> APIs.

There's also IRQ, DMA and BUS resource types defined, though not handled
by kernel/resource.c.

> The problem comes if you start abusing IORESOURCE_IO - which has _always_
> meant "this is a PCI/ISA IO resource", and then you have someone adding
> calls to request_region() etc.  That will make stuff look at the
> ioport_resource root, and it won't work.

Yes, the request_region() usage is totally broken - I hadn't realised
that the original patch was doing that.  I had thought it was just
peering at the start of the address range and using that as the base
address for register I/O which is not wonderful but not actively broken
on the client side.

> In any case, if the resource tree is disconnected from the main two parent
> resources, then the resource probably shouldn't have either IORESOURCE_IO
> nor IORESOURCE_MEM set in it.  It's neither of those two resource types.

There is something of a shortage of other options as things stand...  I
guess using _BUS or something would at least eliminate the possibility
of confusion with the management code would be a quick, short term
improvement.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/bb26f1ae/attachment-0001.sig>

[PATCH 1/2] mfd: max8925: request resource region

[Samuel Ortiz]

Hi Haojian,

On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote:
> If resource region isn't requested, component devices will fail to request
> their resources.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> ---
>  drivers/mfd/max8925-core.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
Applied to my for-next branch, thanks.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/