Hi Marek, Thanks for reviewing.
> > > This patch adds support for the GPIO perif found on hi6220 > > SoC. > > > > Signed-off-by: Peter Griffin <peter.grif...@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.grif...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot