RE: [PATCH 10/19] mach-ep93xx: break out GPIO driver specifics
From: H Hartley Sweeten
Date: Thu Aug 11 2011 - 13:16:36 EST
On Thursday, August 11, 2011 5:29 AM, Linus Walleij wrote:
> On Wed, Aug 10, 2011 at 7:23 PM, H Hartley Sweeten wrote:
>> On Wednesday, August 10, 2011 5:18 AM, Linus Walleij wrote:
>> I'm a bit confused by the intentions of this patch. Please see below.
> The intentions are in path 0, in this case specifically to
> let gpio.h be only about generic GPIO and gpiolib and
> gpio-<foo>.h be platform and driver specifics.
OK. I overlooked that part. Thanks.
>>> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
>>> index c60f081..94c78bc 100644
>>> --- a/arch/arm/mach-ep93xx/core.c
>>> +++ b/arch/arm/mach-ep93xx/core.c
>>> @@ -38,6 +38,7 @@
>>> #include <mach/fb.h>
>>> #include <mach/ep93xx_keypad.h>
>>> #include <mach/ep93xx_spi.h>
>>> +#include <mach/gpio-ep93xx.h>
>> Why is this additional include needed? With your change to the ep93xx
>> <mach/gpio.h> below, this header is already included by <linux/gpio.h>.
> Yes, but it is included in <mach/gpio.h> because the quick
> generic gpio macros in there use them, not because it
> wants to expose symbols from <mach/gpio-ep93xx.h> to
> the entire system, that is just a side effect due to
> low encapsulation.
>> Since this file actually uses the gpiolib calls, the include of
>> <linux/gpio.h> is needed and appropriate.
> Yes that one *also*.
> Because you do both generic and driver-specific
Hmmm.. Ok, I guess I see your point.
> The intention is not to minimize the number of #include<>
> statements (if we want that we could always rely on
> implicit includes from other files, which is an abomonation),
> the intention is separation of concerns.
Of course. Any header that is "needed" by the source _should_ be included.
Relying on them being included by other headers is just asking for trouble.
>> Question... Shouldn't the gpio_to_irq and irq_to_gpio defines also be moved
> These are currently part of the generic GPIO and gpiolib
> since they are defined in <linux/gpio.h> :-/
I see your point.
> We will get rid of them eventually but I cannot refactor
> the entire universe at once.
>>> -#define EP93XX_GPIO_LINE_EGPIO7 EP93XX_GPIO_LINE_A(7)
>>> +/* new generic GPIO API - see Documentation/gpio.txt */
>> Russell's patch removed this comment. I don't see any reason to
>> put it back.
> My bad, fixing it up.
BTW, it looks like Russell's patches hit linux-next this morning.
>>> -/* GPIO port B. */
>>> -#define EP93XX_GPIO_LINE_B(x) ((x) + 8)
>>> -#define EP93XX_GPIO_LINE_EGPIO8 EP93XX_GPIO_LINE_B(0)
>> -#define EP93XX_GPIO_LINE_EGPIO9 EP93XX_GPIO_LINE_B(1)
>>> -#define EP93XX_GPIO_LINE_EGPIO10 EP93XX_GPIO_LINE_B(2)
>>> -#define EP93XX_GPIO_LINE_EGPIO11 EP93XX_GPIO_LINE_B(3)
>>> -#define EP93XX_GPIO_LINE_EGPIO12 EP93XX_GPIO_LINE_B(4)
>>> -#define EP93XX_GPIO_LINE_EGPIO13 EP93XX_GPIO_LINE_B(5)
>>> -#define EP93XX_GPIO_LINE_EGPIO14 EP93XX_GPIO_LINE_B(6)
>>> -#define EP93XX_GPIO_LINE_EGPIO15 EP93XX_GPIO_LINE_B(7)
>>> +#include <asm-generic/gpio.h>
>> I believe Russell's patch moves this include to arm's <asm/gpio.h>.
>> Having it included here is a bit redundant.
> Fixing it. My error.
>>> +#include "gpio-ep93xx.h"
>> Why this form? Isn't <mach/gpio-ep93xx.h> the preferred form?
> This is to make the inclusion very local. The only reason it is
> included at all is to get EP93XX_GPIO_LINE_MAX_IRQ,
> nothing else.
Hmm.. It is needed for gpio_to_irq()...
The only other user of that define is drivers/gpio/gpio-ep93xx.c.
To follow the intentions of this patch it would be nice to remove this
include from <mach/gpio.h> and add <mach/gpio-ep93xx.h> to the gpio driver.
But, any user of gpio_to_irq() whould also have to include <mach/gpio-ep93xx.h>
The other solution is to hook up the gpiolib to_irq callback in
gpio-ep93xx and do:
#define gpio_to_irq __gpio_to_irq
Maybe this is a better option?
>>> -/* maximum value for irq capable line identifiers */
>>> -#define EP93XX_GPIO_LINE_MAX_IRQ EP93XX_GPIO_LINE_F(7)
>>> +#define gpio_get_value __gpio_get_value
>>> +#define gpio_set_value __gpio_set_value
>>> +#define gpio_cansleep __gpio_cansleep
> You didn't comment on this but it's yet another bug due to
> bad rebasing. Fixing it. Thse should not be reintroduced.
I noticed this also but figured you would see it when you rebased
on top of Russell's patches.
>>> diff --git a/arch/arm/mach-ep93xx/simone.c b/arch/arm/mach-ep93xx/simone.c
>>> index 8392e95..1a472ff 100644
>>> --- a/arch/arm/mach-ep93xx/simone.c
>>> +++ b/arch/arm/mach-ep93xx/simone.c
>>> @@ -18,12 +18,12 @@
>>> #include <linux/kernel.h>
>>> #include <linux/init.h>
>>> #include <linux/platform_device.h>
>>> -#include <linux/gpio.h>
>>> #include <linux/i2c.h>
>>> #include <linux/i2c-gpio.h>
>>> #include <mach/hardware.h>
>>> #include <mach/fb.h>
>>> +#include <mach/gpio-ep93xx.h>
>> Here I can kind of agree on the removal of <linux/gpio.h> and adding <mach/gpio-ep93xx.h>.
>> This file does not use any of the gpiolib calls. It just needs to pick up the defines
>> for the two gpio pins used for the I2C bus.
> Yes that's the idea.
OK. I get it know.
>> Still, it seems a bit awkward....
> In what way?
It seemed awkward only in the fact that including <linux/gpio.h> was implying that this
file was asking for gpio support for the ep93xx. Including the mach specific file instead
just reads a bit strange. I'll get over it.
>> Same comment here. It's technically correct but it's seems awkward.
> Define awkward.
> I know you can get the same *implicitly* from <linux/gpio.h>
> but that is the awkward problem we're trying to get rid of.
> Eventually <linux/gpio.h> will *not* bring in *any* platform-
> or driver-specific crap. Not today, but as soon as we get
> somewhere with the single image concept.
> Similarly, platforms with GPIO not using generic GPIO
> or gpiolib at all have simply had their headers renamed
> Atleast that's my idea, I guess someone may smack my
Thanks for the clarifications. Please rebase on top of Russell's patches and
I'll take a look at them again.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/