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

[Marek Vasut <marex@denx.de>]

On Tuesday, May 12, 2015 at 03:38:28 PM, Peter Griffin wrote:

Hi!

> This patch adds support for the GPIO perif found on hi6220
> SoC.
> 
> Signed-off-by: Peter Griffin <peter.griffin@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@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.

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

> +#define HI6220_GPIO1_BASE	(void *)0xf8012000
> +#define HI6220_GPIO2_BASE	(void *)0xf8013000
> +#define HI6220_GPIO3_BASE	(void *)0xf8014000
> +#define HI6220_GPIO4_BASE	(void *)0xf7020000
> +#define HI6220_GPIO5_BASE	(void *)0xf7021000
> +#define HI6220_GPIO6_BASE	(void *)0xf7022000
> +#define HI6220_GPIO7_BASE	(void *)0xf7023000
> +#define HI6220_GPIO8_BASE	(void *)0xf7024000
> +#define HI6220_GPIO9_BASE	(void *)0xf7025000
> +#define HI6220_GPIO10_BASE	(void *)0xf7026000
> +#define HI6220_GPIO11_BASE	(void *)0xf7027000
> +#define HI6220_GPIO12_BASE	(void *)0xf7028000
> +#define HI6220_GPIO13_BASE	(void *)0xf7029000
> +#define HI6220_GPIO14_BASE	(void *)0xf702a000
> +#define HI6220_GPIO15_BASE	(void *)0xf702b000
> +#define HI6220_GPIO16_BASE	(void *)0xf702c000
> +#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 ?

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

> +};
> +
> +/* Information about a GPIO bank */
> +struct hikey_gpio_platdata {
> +	int bank_index;
> +	void *base;     /* address of registers in physical memory */
> +};
> +
> +#endif /* _HI6220_GPIO_H_ */

[...]