[PATCH v3 6/7] ARM: pxa: move gpio-pxa.h into include directory

[Haojian Zhuang]

On Wed, Oct 12, 2011 at 4:42 AM, Arnd Bergmann <arnd at arndb.de> wrote:
> On Tuesday 11 October 2011 21:51:28 Linus Walleij wrote:
>> 2011/10/11 Haojian Zhuang <haojian.zhuang at marvell.com>:
>>
>> > Merge the gpio-pxa.h in arch-pxa and arch-mmp. Move the new gpio-pxa.h
>> > into include directory.
>> (...)
>> >  arch/arm/plat-pxa/include/plat/gpio-pxa.h  |   80 ---------------------------
>> >  drivers/gpio/gpio-pxa.c                    |   42 ++++++++++++++-
>> >  include/linux/gpio-pxa.h                   |   81 ++++++++++++++++++++++++++++
>>
>> This seems like a fully valid merge of common pxa GPIO include
>> files. And I guess if both are not using plat-pxa it cannot reside
>> in <plat/gpio-pxa.h>?
>>
>> Grant: shall this be <linux/gpio-pxa.h> or <linux/gpio/pxa.h> or so?
>
> Hmm, more fundamentally, does this even need to be a header file?
>
> Most of the header file looks like it should not be visible in the entire
> kernel, so better move as much as possible into the gpio-pxa.c file.
> I'm not sure how much remains in the end, but that should probably
> determine the location (drivers/gpio/gpio-pxa.h, asm/gpio-pxa.h
> or linux/gpio-pxa.h). In general, everything should be as local
> as possible.
>
> One more thing regarding the contents of the header:
>
>> +/* GPIO Pin Level Registers */
>> +#define PXA_GPLR(x)  (*(volatile u32 *)(pxa_gpio_regs.gplr   \
>> +                     + PXA_BANK_OFF((x >> 5))))
>> +/* GPIO Pin Direction Registers */
>> +#define PXA_GPDR(x)  (*(volatile u32 *)(pxa_gpio_regs.gpdr   \
>> +                     + PXA_BANK_OFF((x >> 5))))
>
> When these get m> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

oved into the driver itself, it's probably a good
> idea to turn these into proper accessors, something like
>
> enum pxa_gpio_reg {
>        GPLR,
>        GPDR,
>        GPSR,
>        GPCR,
>        GRER,
>        GFER,
>        GEDR,
> };
> static void __iomem *pxa_gpio_reg_base;
> static inline u32 pxa_gpio_read(unsigned int bank, enum pxa_gpio_reg reg)
> {
>        unsigned offset = (bank < 3) ? bank << 2 : 0x100 + ((bank - 3) << 2);
>        return readl_relaxed(pxa_gpio_reg_base + offset + 12 * reg);
> }
> static inline void pxa_gpio_write(unsigned int bank, unsigned int reg, u32 val)
> {
>        unsigned offset = (bank < 3) ? bank << 2 : 0x100 + ((bank - 3) << 2);
>        return writel_relaxed(val, pxa_gpio_reg_base + offset + 12 * reg);
> }
>
> That should let you get rid of most of the macros and the incorrect
> 'volatile' access.
>
>        Arnd
>
It sounds good. I'll fix it.

Thanks
Haojian