[U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver

[Vikram Narayanan <vikram186@gmail.com>]

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(&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;
>

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