Re: [U-Boot] [PATCH 2/6] dm: gpio: hi6220: Add a hi6220 GPIO driver model driver.
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_ */
[...]
On Tue, May 12, 2015 at 08:36:30PM +0200, Marek Vasut wrote:
On Tuesday, May 12, 2015 at 03:38:28 PM, Peter Griffin wrote:
[snip]
+#define BIT(x) (1 << (x))
This macro should be placed into common header files.
We'll fix that once Jagan's series comes in which gives everyone BIT().
Hi Marek,
Thanks for reviewing.
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.
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.
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 :)
participants (3)
-
Marek Vasut -
Peter Griffin -
Tom Rini