[U-Boot] [PATCH 1/2] bcm: Add GPIO driver for BCM2835 SoC

[Stephen Warren]

On 06/24/2012 11:21 AM, Vikram Narayanan wrote:

First off, it's great to see some patches for the chip. Thanks. Sorry
for being so nit-picky below; it's a tendency of mine...

It'd be nice to include a patch description here, so that something
shows up besides just the patch subject.

> 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)

There's no need for brackets around simple numbers.

> +#define BCM2835_MAX_GPIOS	53

I think that should be named either BCM2835_NUM_GPIOS or
BCM2835_MAX_GPIO; you can have a number of GPIOs or a maximum GPIO
number, but not a maximum GPIOs.

> +#define BCM2835_GPIO_FSEL_CLR_MASK	(0x7)

The mask applies equally to set or clear; I'd rename this to just
BCM2835_GPIO_FSEL_MASK.

> +#define BCM2835_GPIO_OUTPUT			(0x1)

I'd like to see a matching BCM2835_GPIO_INPUT function select value too;
even if the value is actually 0 so it doesn't matter, it's still good to
document it.

It would also be useful to define all the values for the function
selector, in order to extend this driver to provide pinmuxing in the
future, e.g.

#define BCM2835_GPIO_FSEL_ALT0 4
etc.

> +#define GPIO_TO_BANK(n)	(n / 10)
> +#define GPIO_TO_PIN(n)	(n % 10)

There are two kinds of bank relevant to the current driver, and perhaps
more in the HW. The macros should also be prefixed with the module name
to avoid conflicts. PIN isn't really a good name, since the GPIOs
themselves /are/ pins; how about SHIFT? I'd like to see these renamed
BCM2835_GPIO_FSEL_BANK/SHIFT, and also to add
BCM2835_GPIO_OTHER_BANK/SHIFT macros.

(or perhaps GENERAL or 1BIT instead of OTHER?)

> +struct bcm_gpio_regs {
> +	u32 gpfsel[6];
> +	u32 reserved1;

> +	u32 gpset0;
> +	u32 gpset1;

You could replace those two with gpset[2], and then use
BCM2835_GPIO_OTHER_BANK() to index into the array. That will simplify
the driver code a bit. The same applies to gpclr* and gplev* below.

> +	u32 reserved2;
> +	u32 gpclr0;
> +	u32 gpclr1;
> +	u32 reserved3;
> +	u32 gplev0;
> +	u32 gplev1;

There are quite a few more registers defined in the datasheet after
this. Even if the driver doesn't use them yet, it'd still be good to add
them here.

> +};

> diff --git a/drivers/gpio/rpi_gpio.c b/drivers/gpio/rpi_gpio.c

I think this should be named bcm2835_gpio.c, since it's a driver for the
SoC in general, rather than a particular board that uses the SoC.

> +struct bcm_gpio {
> +	u32 bank;
> +	u32 pin;
> +};
...
> +static int get_bank_pin(unsigned gpio, struct bcm_gpio *pio)
> +{
> +	int bank = GPIO_TO_BANK(gpio);
> +	int pin = GPIO_TO_PIN(gpio);
> +
> +	if (!gpio_is_valid(gpio))
> +		return -1;
> +
> +	pin &= 0x09;

GPIO_TO_PIN already performs any required masking. Also, this line
doesn't work, because you want mod 9, not bitwise-and with 9.

> +	pio->pin = pin;
> +	pio->bank = bank;
> +	return 0;
> +}

I'm not really sure that structure or function are useful; you can just
use macros BCM2835_GPIO_*_BANK/SHIFT directly in the code.

> +int gpio_request(unsigned gpio, const char *label)
> +{
> +	return (gpio_is_valid(gpio)) ? 1 : 0;
> +}
> +
> +int gpio_free(unsigned gpio)
> +{
> +	return 0;
> +}

Hmmm. Don't you want to do something like save the label away so you
know who requested the pin for what, and mark it requested so you can't
request it twice? IIRC, there's some "gpio info" command that will list
out all the GPIO owners, at least for some pre-existing GPIO drivers.

> +int gpio_direction_input(unsigned gpio)
> +{
> +	struct bcm_gpio pio;
> +	struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
> +	unsigned val = 0;

There's no need to initialize val; it's unconditionally written to below
before it's used.

> +
> +	if (get_bank_pin(gpio, &pio))
> +		return -1;

I would drop that function call and the pio variable, and just use the
macros directly where needed.

> +	val = readl(&reg->gpfsel[pio.bank]);
> +	val &= ~(BCM2835_GPIO_FSEL_CLR_MASK << (pio.pin * 3));

That might be clearer as:

val &= ~(BCM2835_GPIO_FSEL_CLR_MASK << BCM2835_GPIO_FSEL_SHIFT(pin));
val |= ~(BCM2835_GPIO_OUTPUT << BCM2835_GPIO_FSEL_SHIFT(pin));

Similar for gpio_direction_output() below.

> +	writel(val, reg->gpfsel[pio.bank]);

> +int gpio_get_value(unsigned gpio)
> +{
> +	struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
> +	unsigned val = 0;
> +
> +	if (!gpio_is_valid(gpio))
> +		return -1;
> +
> +	val = (gpio < 32) ? readl(&reg->gplev0) : readl(&reg->gplev1);

How about:

val = readl(&reg->gplev[BCM2835_GPIO_OTHER_BANK(gpio)]);

> +	gpio &= 0x1f;

That's not needed, since we extract only the bits we care about below
anyway.

> +	return (val & (1 << gpio)) >> gpio;

That would be simpler as:

return (val >> BCM2835_GPIO_OTHER_SHIFT(gpio)) & 1;

> +int gpio_set_value(unsigned gpio, int value)
> +{
> +	struct bcm_gpio_regs *reg = (struct bcm_gpio_regs *)BCM2835_GPIO_BASE;
> +
> +	if (!gpio_is_valid(gpio))
> +		return -1;
> +
> +	if (gpio < 32) {
> +		if (value)
> +			writel((1 << gpio), reg->gpset0);
> +		else
> +			writel((1 << gpio), reg->gpclr0);
> +	} else {
> +		gpio &= 0x1f;
> +		if (value)
> +			writel((1 << pin), reg->gpset1);
> +		else
> +			writel((1 << pin), reg->gpclr1);
> +	}
> +	return 0;
> +}

How about:

u32 *reg = value ? reg->gpset : reg->gpclr;
writel(1 << BCM2835_GPIO_VALUE_SHIFT(gpio),
      reg[BCM2835_GPIO_VALUE_BANK(gpio)]);

BTW, I don't think this patch compiles; gpio_set_value() references
variable "pin", which doesn't exist.