[U-Boot] [PATCH 2/6] dm: gpio: hi6220: Add a hi6220 GPIO driver model driver.

[Marek Vasut <marex@denx.de>]

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 :)