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

[Peter Griffin]

Hi Marek,

Thanks for reviewing.

> 
> > This patch adds support for the GPIO perif found on hi6220
> > SoC.
> > 
> > Signed-off-by: Peter Griffin <peter.griffin at linaro.org>
> > ---
> >  arch/arm/include/asm/arch-armv8/gpio.h | 47 +++++++++++++++++
> >  drivers/gpio/Makefile                  |  2 +
> >  drivers/gpio/hi6220_gpio.c             | 95
> > ++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+)
> >  create mode 100644 arch/arm/include/asm/arch-armv8/gpio.h
> >  create mode 100644 drivers/gpio/hi6220_gpio.c
> > 
> > diff --git a/arch/arm/include/asm/arch-armv8/gpio.h
> > b/arch/arm/include/asm/arch-armv8/gpio.h new file mode 100644
> > index 0000000..162c2d9
> > --- /dev/null
> > +++ b/arch/arm/include/asm/arch-armv8/gpio.h
> > @@ -0,0 +1,47 @@
> > +/*
> > + * Copyright (C) 2015 Linaro
> > + * Peter Griffin <peter.griffin at linaro.org>
> > + *
> > + * SPDX-License-Identifier:	GPL-2.0+
> > + */
> > +
> > +#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.


> 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.

<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.
> 
> > +#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.

regards,

Peter.