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

[Vikram Narayanan <vikram186@gmail.com>]

On 7/31/2012 9:22 PM, Stephen Warren wrote:
> 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.
>
>>
>> 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...

Yes.

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

Naming isn't really a problem for me. If you want it to be SHIFT, I'd go 
with it.