Re: [PATCH 0/5] mfd: replace IORESOURCE_IO by IORESOURCE_MEM

Arnd Bergmann
2012-08-07T14:28:31Z

On Tuesday 07 August 2012, Mark Brown wrote:
> On Tue, Aug 07, 2012 at 01:11:57PM +0100, Russell King wrote:
>
> > The only open questions on this are:
> > 1. Is there another driver you are concerned about.
>
> As I said elsewhere 88pm* needs this as a stable bugfix and wm831x
> should be converted over too.

I've looked through the remaining MFD drivers and found one more,
max8925-core, which is using IORESOURCE_IO for something that is
not ISA/PCI IO-space.

> > 2. Choosing a better name.
>
> I'm not sure we need one, _REG seems perfectly fine here unless we want
> to go with Arnd's suggestion of _OTHER. Do you have any concerns with
> the use of reg?

BenH actually suggested _OTHER. I think either one is fine.

> > But I won't put question marks at the end of those because you never seem
> > to answer questions. You will just produce yet more justifications why
> > this approach is not one you're willing to accept. So I really don't know
> > why I wasted my time to produce this patch.
>
> As I mentioned in my mail a few moments ago I had not replied to your
> mails asking these question about which drivers are affected because
> those mails have been arriving so quickly since the first one I saw with
> the question in that there has not been a chance to do so.
>
> > index 589e0e7..bfee885 100644
> > --- a/include/linux/ioport.h
> > +++ b/include/linux/ioport.h
> > @@ -31,6 +31,7 @@ struct resource {
> > #define IORESOURCE_TYPE_BITS 0x00001f00 /* Resource type */
> > #define IORESOURCE_IO 0x00000100
> > #define IORESOURCE_MEM 0x00000200
> > +#define IORESOURCE_REG 0x00000300 /* Register offsets */
> > #define IORESOURCE_IRQ 0x00000400
> > #define IORESOURCE_DMA 0x00000800
> > #define IORESOURCE_BUS 0x00001000
>
> As I've said before I'm fine with the driver changes. I do feel that it
> would be better to also renumber all the existing resource types while
> we're at it in order to make it clear that these are just plain numbers,
> that's the reason nobody wrote this patch previously. This will avoid
> any future confusion.

This gets into a lot more tricky territory: We have a bunch of drivers
doing their own bitmask operations on these, like drivers/video/offb.c
testing

if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
return NULL;

or drivers/scsi/gdth.c doing

if (!(base0 & IORESOURCE_MEM) ||
!(base2 & IORESOURCE_MEM) ||
!(base1 & IORESOURCE_IO))
return -ENODEV;

Now I've looked at the three drivers with the immediate problem of
IORESOURCE_IO abuse (max8925, wm831x, 88pm860x) and none of them are
doing such bitmask operations, so I'm reasonably sure we are fine
for those drivers. I also agree that renumbering the resources in a
way that makes it impossible to use bitmasks is a good idea, but
that would actually be pretty invasive because then we have to rewrite
all the functions that currently do it.

Arnd