From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2012-02-23 20:19:15
On Thu, 2012-02-23 at 11:29 +0000, Russell King - ARM Linux wrote:
What's this stuff doing in generic drivers?
Well, I suppose that's because the xilinx stuff used to be ppc
only ? :-)
See drivers/gpio/gpio-xilinx.c:
static int xgpio_get(struct gpio_chip *gc, unsigned int gpio)
{
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
}
include/linux/of_gpio.h:
struct of_mm_gpio_chip {
struct gpio_chip gc;
void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
void __iomem *regs;
};
Why am I being asked to add in_be32() etc to ARMs io.h ? Why do we need
yet another set of IO accessors? Is there something wrong with
ioread*()/ioread*be() etc?
Nope, nothing wrong with them, the driver should be fixed. in_be* is
historical ppc stuff.
My guess is this stems from a lack of proper review
That or history. Our readX/writeX used to be more PCI specific (have
infrastructure to work around PCI bridge bugs) which some drivers
avoided using the in_/out_ variants, in some case it's just pure
history, etc... Some of these things are ancient.
Cheers,
Ben.
From: Russell King - ARM Linux <hidden> Date: 2012-02-23 20:25:53
On Fri, Feb 24, 2012 at 07:18:59AM +1100, Benjamin Herrenschmidt wrote:
On Thu, 2012-02-23 at 11:29 +0000, Russell King - ARM Linux wrote:
quoted
What's this stuff doing in generic drivers?
Well, I suppose that's because the xilinx stuff used to be ppc
only ? :-)
Note that's just the first one grep turned up. I've no idea which specific
drivers the ST SPEAr people are wanting these accessors for (they didn't
include that information in their submission adding them to ARM.)
quoted
See drivers/gpio/gpio-xilinx.c:
static int xgpio_get(struct gpio_chip *gc, unsigned int gpio)
{
struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
}
include/linux/of_gpio.h:
struct of_mm_gpio_chip {
struct gpio_chip gc;
void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
void __iomem *regs;
};
Why am I being asked to add in_be32() etc to ARMs io.h ? Why do we need
yet another set of IO accessors? Is there something wrong with
ioread*()/ioread*be() etc?
Nope, nothing wrong with them, the driver should be fixed. in_be* is
historical ppc stuff.
quoted
My guess is this stems from a lack of proper review
That or history. Our readX/writeX used to be more PCI specific (have
infrastructure to work around PCI bridge bugs) which some drivers
avoided using the in_/out_ variants, in some case it's just pure
history, etc... Some of these things are ancient.
So, if I tell the SPEAr people that any driver they come across using
these old in_XX should be converted to use ioread*() you'll be happy
for that to happen?
}
include/linux/of_gpio.h:
struct of_mm_gpio_chip {
=A0 =A0 =A0 =A0 struct gpio_chip gc;
=A0 =A0 =A0 =A0 void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
=A0 =A0 =A0 =A0 void __iomem *regs;
};
Why am I being asked to add in_be32() etc to ARMs io.h ? =A0Why do we =
need
quoted
quoted
yet another set of IO accessors? =A0Is there something wrong with
ioread*()/ioread*be() etc?
Nope, nothing wrong with them, the driver should be fixed. in_be* is
historical ppc stuff.
quoted
My guess is this stems from a lack of proper review
That or history. Our readX/writeX used to be more PCI specific (have
infrastructure to work around PCI bridge bugs) which some drivers
avoided using the in_/out_ variants, in some case it's just pure
history, etc... Some of these things are ancient.
So, if I tell the SPEAr people that any driver they come across using
these old in_XX should be converted to use ioread*() you'll be happy
for that to happen?
yes
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.