On Wednesday, May 13, 2015 at 02:00:13 PM, Peter Griffin wrote:
Hi Marek,
Thanks for reviewing.
Hi!
+#ifndef _HI6220_GPIO_H_ +#define _HI6220_GPIO_H_
+#define HI6220_GPIO0_BASE (void *)0xf8011000
You should drop the explicit cast, that's nasty.
If I drop the cast I get lots of
include/asm/arch/gpio.h:11:32: warning: initialization makes pointer from integer without a cast
compiler warnings.
But this is gpio.h header for your architecture, so maybe fix it there too?
Also, why don't you define this as a HI6220_GPIO_BASE(bank) instead? That'd trim down the number of macros and from what I see, it should be rather easy to do ...
#define HI6220_GPIO_BASE(bank)
(((bank < 4) ? 0xf8012000 : (0xf7020000 - 0x4000)) + (0x1000 * bank))
Yes good idea, I will do it like you suggest in V2. Currently I have
#define HI6220_GPIO_BASE(bank) (void *)(((bank < 4) ? 0xf8011000 : \ 0xf7020000 - 0x4000) + (0x1000 * bank))
To avoid the warnings mentioned above.
Yup, or maybe even make it an inline function so you get proper typechecking?
<snip>
+#define HI6220_GPIO17_BASE (void *)0xf702d000 +#define HI6220_GPIO18_BASE (void *)0xf702e000 +#define HI6220_GPIO19_BASE (void *)0xf702f000
But are these even used in the driver anywhere ?
They are currently used in the hikey.c board file which defines the hikey_gpio_platdata structure.
Although thinking about it some more this should be moved from the hikey board file to the driver as it is SoC specific rather than board specific.
This is specific to the CPU, so this should be in arch-<something> .
+#define BIT(x) (1 << (x))
This macro should be placed into common header files.
+#define HI6220_GPIO_PER_BANK 8 +#define HI6220_GPIO_DIR 0x400
+struct gpio_bank {
- u8 *base; /* address of registers in physical memory */
Should be void __iomem *, no ?
Will fix in v2.
Thanks! Also, please wait a bit for other reviews before sending V2 :)