From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Tue, 31 Jul 2012 09:52:18 -0600
Subject: [U-Boot] [PATCH v2 1/2] bcm: Add GPIO driver
In-Reply-To: <5017FDD4.9080602@gmail.com>
References: <4FFDE3A9.7080804@gmail.com> <4FFDE406.4030901@gmail.com>
	<5002FCA2.1050706@wwwdotorg.org> <5017FDD4.9080602@gmail.com>
Message-ID: <5017FF32.2060308@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/31/2012 09:46 AM, Vikram Narayanan wrote:
> 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

One more comment on the patch subject; it probably should be "gpio:
bcm2835:" not "bcm:" since (a) it's in the GPIO directory and (b) the
GPIO module is specifically for a BCM2835, and probably doesn't apply to
any/all Broadcom devices.

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

Hmmm. It seems better to be internally consistent with U-Boot rather
than keeping (onyl part of) U-Boot consistent with the kernel...

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

The full quote you're replying to was:

>> +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?

... so that macro is being used as a shift not as a mask.