On 07/11/2012 02:37 PM, Vikram Narayanan wrote: > Driver for BCM2835 SoC. This gives the basic functionality of > setting/clearing the output.
> diff --git a/arch/arm/include/asm/arch-bcm2835/gpio.h > b/arch/arm/include/asm/arch-bcm2835/gpio.h > +#define BCM2835_GPIO_BASE 0x7E200000 > +#define BCM2835_NUM_GPIOS 53 For consistency, that might be better as BCM2835_GPIO_COUNT, but not a big deal. > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > COBJS-$(CONFIG_DA8XX_GPIO) += da8xx_gpio.o > COBJS-$(CONFIG_ALTERA_PIO) += altera_pio.o > COBJS-$(CONFIG_MPC83XX_GPIO) += mpc83xx_gpio.o > +COBJS-$(CONFIG_BCM2835_GPIO) += gpio_bcm2835.o It looks like the name bcm2835_gpio.c would be more consistent with existing drivers, but not a big deal. > diff --git a/drivers/gpio/gpio_bcm2835.c b/drivers/gpio/gpio_bcm2835.c > +inline int gpio_is_valid(unsigned gpio) > +{ > + return (gpio > BCM2835_NUM_GPIOS) ? 0 : 1; Presumably gpio==0 is a valid GPIO, so that should be >= not >. It'd be simpler to write it as: return gpio < BCM2835_NUM_GPIOS; > +int gpio_request(unsigned gpio, const char *label) > +{ > + return (gpio_is_valid(gpio)) ? 1 : 0; Why not just return gpio_is_valid_(gpio) directly? > +int gpio_direction_input(unsigned gpio) > + val = readl(®->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]); > + val &= ~(BCM2835_GPIO_FSEL_MASK << BCM2835_GPIO_FSEL_SHIFT(gpio)); Even if BCM2835_GPIO_OUTPUT==0, it seems better to | it in here for documentation purposes, so add: val |= (BCM2835_GPIO_INPUT << BCM2835_GPIO_FSEL_SHIFT(gpio)); Otherwise, there's not much point creating the #define BCM2835_GPIO_INPUT. > +int gpio_direction_output(unsigned gpio, int value) > +{ > + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; > + unsigned val; > + > + val = readl(®->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]); > + val &= ~(BCM2835_GPIO_FSEL_MASK << BCM2835_GPIO_FSEL_SHIFT(gpio)); > + val |= (BCM2835_GPIO_OUTPUT << BCM2835_GPIO_FSEL_SHIFT(gpio)); > + writel(val, reg->gpfsel[BCM2835_GPIO_FSEL_BANK(gpio)]); This (setting the direction) should happen after the following to set the value: > + if (value) > + gpio_set_value(gpio, value); That way, when the GPIO is set to output, the correct value will immediately be driven onto the GPIO, so a glitch may be avoided. > +int gpio_get_value(unsigned gpio) > + return (val >> BCM2835_GPIO_COMMON_MASK(gpio)) & 0x1; Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK? > +int gpio_set_value(unsigned gpio, int value) > +{ > + struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE; > + u32 *output_reg = value ? reg->gpset : reg->gpclr; > + > + writel(1 << BCM2835_GPIO_COMMON_MASK(gpio), > + output_reg[BCM2835_GPIO_COMMON_BANK(gpio)]); Same comment here. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot