Messages in this thread | | | Date | Thu, 11 Jun 2009 17:28:50 -0400 | From | Andres Salomon <> | Subject | Re: [PATCH 1/2] cs5535-gpio: add AMD CS5535/CS5536 GPIO driver support |
| |
On Thu, 11 Jun 2009 22:11:58 +0200 Tobias Müller <Tobias_Mueller@twam.info> wrote:
> >> #define DRV_NAME "cs5535-gpio" > >> #define GPIO_BAR 1 > >> +#define GPIO_DEFAULT_MASK 0x0B003C66 > > > > Where does this mask of available GPIOs originate from? > > I had a comment in my original patch: > > /** > * Some GPIO pins > * 31-29,23 : reserved (always mask out) > * 28 : Power Button > * 26 : PME# > * 22-16 : LPC > * 14,15 : SMBus > * 9,8 : UART1 > * 7 : PCI INTB > * 3,4 : UART2/DDC > * 2 : IDE_IRQ0 > * 1 : AC_BEEP > * 0 : PCI INTA > * > * If a mask was not specified, be conservative and only allow: > * 1,2,5,6,10-13,24,25,27 > */ > > I'll add this in my patch to clear it out. >
But why are you being conservative in the first place? If something's using GPIOs, unless they're unmapped, you should allow it to use them without requiring a boot arg.
For example, OLPC uses GPIO 7 for its DCON IRQ. With the masking scheme, OLPC will need to set that mask from the default. I don't see the point of having the mask at all if other drivers in the kernel are going to be requesting GPIOs (presumably they know what they're doing).
> >> + /* disable output aux 1 & 2 on this pin */ > >> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1); > >> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2); > >> + > >> + /* disable input aux 1 on this pin */ > >> + __cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1); > >> + > >> + /* disable output */ > >> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE); > >> + > >> + /* enable input */ > >> + __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE); > > > > I don't think this is the right place for all of this. Your earlier > > email mentioned disabling OUT_AUX{1,2} for outputs, and IN_AUX for > > inputs. I'm fine with doing that here, but I don't see why you're > > also disabling output and enabling input by default. > > I mentioned this in an ealier mail too. When I request the GPIO from > userspace the direction file always contains "in", so I thought > this is the standard direction after resetting as I should be in a > defined state after requesting. But I didn't found anything > about this in GPIO lib documentation, so I would be fine to change > this if there is any common default behavoir.
To be honest, I'd have to play around with it a bit before I knew whether it actually breaks anything or not. I'm not sure if it would break anything on OLPC, and I don't have any other geode machines that do anything interesting w/ GPIOs.
Maybe David can clear up whether this behavior is correct from the userspace GPIO usage standpoint..
> > >> - .ngpio = 28, > >> + .ngpio = 32, > > > > Since GPIOs 29-31 aren't externally available, and 28 is > > unavailable anyways, shouldn't we just set ngpio to 28? > I thought that 32 is in consistency with the datasheet as it always > talks about 32 GPIO pins.
Fair enough. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |