RE: GPIO support for HTC Dream

[H Hartley Sweeten]

On Tuesday, December 15, 2009 12:47 PM, Pavel Machek wrote:
>
> Hi!
>
>>> +int gpio_to_irq(unsigned gpio)
>>> +{
>>> +	return -EINVAL;
>>> +}
>> 
>> This should probably just be an inline function in
>> arch/arm/mach-msm/include/mach/gpio.h
>
> Well, it is not performance critical in any way and it is likely to
> change in future. I'd leave it here.

Comment below..

>>> diff --git a/arch/arm/mach-msm/board-dream.h b/arch/arm/mach-msm/board-dream.h
>>> index 4f345a5..dbd78b9 100644
>>> --- a/arch/arm/mach-msm/board-dream.h
>>> +++ b/arch/arm/mach-msm/board-dream.h
>>> @@ -1,5 +1,58 @@
>>>  
>>> -#define TROUT_CPLD_BASE   0xE8100000
>>> -#define TROUT_CPLD_START  0x98000000
>>> -#define TROUT_CPLD_SIZE   SZ_4K
>>> +#define MSM_SMI_BASE		0x00000000
>>> +#define MSM_SMI_SIZE		0x00800000
...
>>> +#define DREAM_CPLD_BASE   0xE8100000
>>> +#define DREAM_CPLD_START  0x98000000
>>> +#define DREAM_CPLD_SIZE   SZ_4K
>>> +
>>
>> This header might need to be a separate patch.  The only thing in it
>> related to the rest of this is DREAM_CPLD_BASE.
>
> Yep.
>
>>> +#ifndef __ASM_ARCH_MSM_GPIO_H
>>> +#define __ASM_ARCH_MSM_GPIO_H
>>> +
>>> +extern int gpio_to_irq(unsigned gpio);
>> 
>> This should probably be an inline as mentioned above.
>> 
>> For completeness you should probably also add:
>> 
>> static inline int irq_to_gpio(unsigned irq)
>> {
>> 	return -EINVAL;
>> }
>
> I'd say that would be overdoing it.

I only mentioned that one because it is one of the functions listed in
Documentation/gpio.txt.  I'm not sure if that means it's "required".  You
make the call...

>> And, nitpick, move both of them after the gpio_cansleep below.
>
> I'll do the move.
>
>>> +#define DREAM_GPIO_CABLE_IN1		(83)
>>> +#define DREAM_GPIO_CABLE_IN2		(49)
>>> +
>>> +#define DREAM_GPIO_START (128)
>> 
>> Nitpick.  Tab align these three with the ones below.
>
> Ok.
>
>>> +#define DREAM_GPIO_SDMC_CD_N                (DREAM_GPIO_VIRTUAL_BASE + 0)
>>> +#define DREAM_GPIO_END                      (DREAM_GPIO_SDMC_CD_N)
>>> +#define DREAM_GPIO_BANK1_FIRST_INT_SOURCE   (DREAM_GPIO_SDMC_CD_N)
>>> +#define DREAM_GPIO_BANK1_LAST_INT_SOURCE    (DREAM_GPIO_SDMC_CD_N)
>>> +
>>> +#endif
>> 
>> Otherwise, looks good to me.  Just test it to make sure it works :-).
>> 
>> Since I have no way of compiling or testing this...
>> 
>> Reviewed-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>
> I believe inlining that function would be bad change. Can I still use
> reviewed-by tag?

The reason I suggest making it an inline in the header is you might
eventually want to change both gpio_to_irq and irq_to_gpio into macros
so that they can be used for static initializers.  For right now both
are simple return -EINVAL but when gpio irq's are supported you might
find a need to use one or the other in a platform init...

Regardless, yes you can still use my reviesed-by tag.

Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/