From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Sun, 15 Jul 2012 11:23:46 -0600
Subject: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver
In-Reply-To: <4FFDE406.4030901@gmail.com>
References: <4FFDE3A9.7080804@gmail.com> <4FFDE406.4030901@gmail.com>
Message-ID: <5002FCA2.1050706@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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(&reg->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(&reg->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.