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
