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 :) _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot