Hello Stephen,
On 7/15/2012 10:53 PM, Stephen Warren wrote:
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
Linux kernel follows this naming, to be exact, it should've been
gpio-bcm2835.c. Having a thought in mind that one day the namings would
be made consistent with the kernel. That is the reason for this naming,
but isn't a big deal to change it.
+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;
Agree for all the above. Will get reflected in the v3.
Shouldn't that be BCM2835_GPIO_COMMON_SHIFT not BCM2835_GPIO_COMMON_MASK?
If you'd like to have naming consistency FSEL_SHIFT/COMMON_SHIFT, then
it shall be COMMON_SHIFT.
But it doesn't do any shifting like the FSEL_SHIFT, rather it does only
masking of bits. So, it makes more sense for me to name it as MASK and
not SHIFT.
~Vikram
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
