| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ADE657CA350FB648AAC2C43247A983F001F388389983@AUSP01VMBX24.collaborationhost.net> Date: Thu, 11 Aug 2011 12:16:14 -0500 From: H Hartley Sweeten <hartleys@...ionengravers.com> To: Linus Walleij <linus.walleij@...aro.org> CC: Linus Walleij <linus.walleij@...ricsson.com>, Grant Likely <grant.likely@...retlab.ca>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, Lee Jones <lee.jones@...aro.org>, Ryan Mallon <rmallon@...il.com> Subject: RE: [PATCH 10/19] mach-ep93xx: break out GPIO driver specifics 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 > operations. 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 >> here? > > 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> also. 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 > <mach/gpio-foo.h>. > > Atleast that's my idea, I guess someone may smack my > fingers. Thanks for the clarifications. Please rebase on top of Russell's patches and I'll take a look at them again. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists