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
