[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100
This patch adds generic GPIO driver framework support for Armada100 series.
To enable GPIO driver define CONFIG_ARMADA100_GPIO and for GPIO commands define CONFIG_CMD_GPIO in your board configuration file.
NOTE: This driver assumes proper configuration of MFP register for the GPIO in use.
These patches depends on patch series that adds support for Marvell Guruplug-Display [1,2].
Signed-off-by: Ajay Bhargav ajay.bhargav@einfochips.com --- arch/arm/include/asm/arch-armada100/gpio.h | 130 +++++++++++++++++++ drivers/gpio/Makefile | 1 + drivers/gpio/armada100_gpio.c | 192 ++++++++++++++++++++++++++++ 3 files changed, 323 insertions(+), 0 deletions(-) create mode 100644 arch/arm/include/asm/arch-armada100/gpio.h create mode 100644 drivers/gpio/armada100_gpio.c
diff --git a/arch/arm/include/asm/arch-armada100/gpio.h b/arch/arm/include/asm/arch-armada100/gpio.h new file mode 100644 index 0000000..9024a2e --- /dev/null +++ b/arch/arm/include/asm/arch-armada100/gpio.h @@ -0,0 +1,130 @@ +/* + * (C) Copyright 2010 + * eInfochips Ltd. <www.einfochips.com> + * Written-by: Ajay Bhargav ajay.bhargav@einfochips.com + * + * (C) Copyright 2010 + * Marvell Semiconductor <www.marvell.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, + * MA 02110-1301 USA + */ + +#ifndef _ASM_ARCH_GPIO_H +#define _ASM_ARCH_GPIO_H + +#ifndef __ASSEMBLY__ +#include <asm/types.h> +#endif + +#if defined(CONFIG_ARMADA100_GPIO) +#include <asm/arch/armada100.h> + +#define GPIO_LABEL_MAX 20 +#define ARMD_MAX_GPIO 128 + +#define GPIO_TO_REG(gp) (gp >> 5) +#define GPIO_TO_BIT(gp) (1 << (gp & 0x1F)) +#define GPIO_VAL(gp, val) ((val >> (gp & 0x1F)) & 0x01) + +#define GPIO_SET 1 +#define GPIO_CLR 0 + +/* + * GPIO register map + * Refer Datasheet Appendix A.36 + */ +struct armdgpio_registers { + u32 gplr0; /* Pin Level Register - 0x0000 */ + u32 gplr1; /* 0x0004 */ + u32 gplr2; /* 0x0008 */ + u32 gpdr0; /* Pin Direction Register - 0x000C */ + u32 gpdr1; /* 0x0010 */ + u32 gpdr2; /* 0x0014 */ + u32 gpsr0; /* Pin Output Set Register - 0x0018 */ + u32 gpsr1; /* 0x001C */ + u32 gpsr2; /* 0x0020 */ + u32 gpcr0; /* Pin Output Clear Register - 0x0024 */ + u32 gpcr1; /* 0x0028 */ + u32 gpcr2; /* 0x002C */ + u32 grer0; /* Rising-Edge Detect Enable Register - 0x0030 */ + u32 grer1; /* 0x0034 */ + u32 grer2; /* 0x0038 */ + u32 gfer0; /* Falling-Edge Detect Enable Register - 0x003C */ + u32 gfer1; /* 0x0040 */ + u32 gfer2; /* 0x0044 */ + u32 gedr0; /* Edge Detect Status Register - 0x0048 */ + u32 gedr1; /* 0x004C */ + u32 gedr2; /* 0x0050 */ + u32 gsdr0; /* Bitwise Set of GPIO Direction Register - 0x0054 */ + u32 gsdr1; /* 0x0058 */ + u32 gsdr2; /* 0x005C */ + u32 gcdr0; /* Bitwise Clear of GPIO Direction Register - 0x0060 */ + u32 gcdr1; /* 0x0064 */ + u32 gcdr2; /* 0x0068 */ + u32 gsrer0; /* Bitwise Set of Rising-Edge Detect Enable + Register - 0x006C */ + u32 gsrer1; /* 0x0070 */ + u32 gsrer2; /* 0x0074 */ + u32 gcrer0; /* Bitwise Clear of Rising-Edge Detect Enable + Register - 0x0078 */ + u32 gcrer1; /* 0x007C */ + u32 gcrer2; /* 0x0080 */ + u32 gsfer0; /* Bitwise Set of Falling-Edge Detect Enable + Register - 0x0084 */ + u32 gsfer1; /* 0x0088 */ + u32 gsfer2; /* 0x008C */ + u32 gcfer0; /* Bitwise Clear of Falling-Edge Detect Enable + Register - 0x0090 */ + u32 gcfer1; /* 0x0094 */ + u32 gcfer2; /* 0x0098 */ + u32 apmask0; /* Bitwise Mask of Edge Detect Register - 0x009C */ + u32 apmask1; /* 0x00A0 */ + u32 apmask2; /* 0x00A4 */ + u8 pad[0x0100 - 0x00A4 - 4]; + u32 gplr3; /* 0x0100 */ + u8 pad1[0x010C - 0x0100 - 4]; + u32 gpdr3; /* 0x010C */ + u8 pad2[0x0118 - 0x010C - 4]; + u32 gpsr3; /* 0x0118 */ + u8 pad3[0x0124 - 0x0118 - 4]; + u32 gpcr3; /* 0x0124 */ + u8 pad4[0x0130 - 0x0124 - 4]; + u32 grer3; /* 0x0130 */ + u8 pad5[0x013C - 0x0130 - 4]; + u32 gfer3; /* 0x013C */ + u8 pad6[0x0148 - 0x013C - 4]; + u32 gedr3; /* 0x0148 */ + u8 pad7[0x0154 - 0x0148 - 4]; + u32 gsdr3; /* 0x0154 */ + u8 pad8[0x0160 - 0x0154 - 4]; + u32 gcdr3; /* 0x0160 */ + u8 pad9[0x016C - 0x0160 - 4]; + u32 gsrer3; /* 0x016C */ + u8 pad10[0x0178 - 0x016C - 4]; + u32 gcrer3; /* 0x0178 */ + u8 pad11[0x0184 - 0x0178 - 4]; + u32 gsfer3; /* 0x0184 */ + u8 pad12[0x0190 - 0x0184 - 4]; + u32 gcfer3; /* 0x0190 */ + u8 pad13[0x019C - 0x0190 - 4]; + u32 apmask3; /* 0x019C */ +}; + +#endif /* CONFIG_ARMADA100_GPIO */ +#endif /* _ASM_ARCH_GPIO_H */ diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 1e3ae11..31b83cd 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk LIB := $(obj)libgpio.o
COBJS-$(CONFIG_AT91_GPIO) += at91_gpio.o +COBJS-$(CONFIG_ARMADA100_GPIO) += armada100_gpio.o COBJS-$(CONFIG_KIRKWOOD_GPIO) += kw_gpio.o COBJS-$(CONFIG_MARVELL_MFP) += mvmfp.o COBJS-$(CONFIG_MXC_GPIO) += mxc_gpio.o diff --git a/drivers/gpio/armada100_gpio.c b/drivers/gpio/armada100_gpio.c new file mode 100644 index 0000000..578fdac --- /dev/null +++ b/drivers/gpio/armada100_gpio.c @@ -0,0 +1,192 @@ +/* + * (C) Copyright 2010 + * eInfochips Ltd. <www.einfochips.com> + * Written-by: Ajay Bhargav ajay.bhargav@einfochips.com + * + * (C) Copyright 2010 + * Marvell Semiconductor <www.marvell.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, + * MA 02110-1301 USA + */ + +#include <common.h> +#include <asm/io.h> +#include <asm/errno.h> +#include <asm/gpio.h> + +int gpio_request(int gp, const char *label) +{ + /* + * Assumes corresponding MFP is configured peoperly + * for use as GPIO + */ + return 0; +} + +void gpio_free(int gp) +{ + /* default direction for GPIO is input */ + gpio_direction_input(gp); +} + +void gpio_toggle_value(int gp) +{ + gpio_set_value(gp, !gpio_get_value(gp)); +} + +int gpio_direction_input(int gp) +{ + struct armdgpio_registers *gpio_regs = + (struct armdgpio_registers *)ARMD1_GPIO_BASE; + + if (gp < ARMD_MAX_GPIO) { + switch (GPIO_TO_REG(gp)) { + case 0: + writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr0); + break; + case 1: + writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr1); + break; + case 2: + writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr2); + break; + case 3: + writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr3); + break; + default: + return -EINVAL; + } + } else { + return -EINVAL; + } + return 0; +} + +int gpio_direction_output(int gp, int value) +{ + struct armdgpio_registers *gpio_regs = + (struct armdgpio_registers *)ARMD1_GPIO_BASE; + + if (gp < ARMD_MAX_GPIO) { + switch (GPIO_TO_REG(gp)) { + case 0: + writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr0); + if (value) + writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr0); + else + writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr0); + break; + case 1: + writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr1); + if (value) + writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1); + else + writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr1); + break; + case 2: + writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr2); + if (value) + writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr2); + else + writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr2); + break; + case 3: + writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr3); + if (value) + writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr3); + else + writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr3); + break; + default: + return -EINVAL; + } + } else { + return -EINVAL; + } + return 0; +} + +int gpio_get_value(int gp) +{ + struct armdgpio_registers *gpio_regs = + (struct armdgpio_registers *)ARMD1_GPIO_BASE; + u32 gp_val; + + if (gp < ARMD_MAX_GPIO) { + switch (GPIO_TO_REG(gp)) { + case 0: + gp_val = readl(&gpio_regs->gplr0); + break; + case 1: + gp_val = readl(&gpio_regs->gplr1); + break; + case 2: + gp_val = readl(&gpio_regs->gplr2); + break; + case 3: + gp_val = readl(&gpio_regs->gplr3); + break; + default: + return -EINVAL; + } + } else { + return -EINVAL; + } + + return GPIO_VAL(gp, gp_val); +} + +void gpio_set_value(int gp, int value) +{ + struct armdgpio_registers *gpio_regs = + (struct armdgpio_registers *)ARMD1_GPIO_BASE; + + if (gp < ARMD_MAX_GPIO) { + switch (GPIO_TO_REG(gp)) { + case 0: + if (value) + writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr0); + else + writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr0); + break; + case 1: + if (value) + writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1); + else + writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr1); + break; + case 2: + if (value) + writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr2); + else + writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr2); + break; + case 3: + if (value) + writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr3); + else + writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr3); + break; + default: + panic("Invalid GPIO pin %u\n", gp); + } + } else { + panic("Invalid GPIO pin %u\n", gp); + } +}
On Mon, Jul 18, 2011 at 05:41, Ajay Bhargav wrote:
--- /dev/null +++ b/arch/arm/include/asm/arch-armada100/gpio.h
+#ifndef __ASSEMBLY__ +#include <asm/types.h> +#endif ... +struct armdgpio_registers {
u32 gplr0; /* Pin Level Register - 0x0000 */...
considering you dont protect actual C code in this file with __ASSEMBLY__, it doesnt make much sense to protect the include
--- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile
COBJS-$(CONFIG_AT91_GPIO) += at91_gpio.o +COBJS-$(CONFIG_ARMADA100_GPIO) += armada100_gpio.o
"R" comes before "T", so your new entry should be above the at91 one
--- /dev/null +++ b/drivers/gpio/armada100_gpio.c
+int gpio_request(int gp, const char *label) +{
- /*
- * Assumes corresponding MFP is configured peoperly
- * for use as GPIO
- */
- return 0;
+}
i think you mean "properly" in the comment, but this assumption is bogus. error checking for valid pins has to be in the gpio_request() so that all the other funcs can assume the pin is valid. not the other way around.
+void gpio_free(int gp) +{
- /* default direction for GPIO is input */
- gpio_direction_input(gp);
+}
usually people dont do this. usually the free step leaves the pin alone. i know in the linux kernel, they assume (and thus require) that behavior. -mike
-----Original Message----- From: Ajay Bhargav [mailto:ajay.bhargav@einfochips.com] Sent: Monday, July 18, 2011 3:12 PM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de; Ajay Bhargav Subject: [PATCH 1/4] gpio: Adds GPIO driver support for Armada100
Correct patch name as gpio: Add GPIO driver for Marvell SoC Armada100
This patch adds generic GPIO driver framework support for Armada100 series.
To enable GPIO driver define CONFIG_ARMADA100_GPIO and for GPIO commands define CONFIG_CMD_GPIO in your board configuration file.
NOTE: This driver assumes proper configuration of MFP register for the GPIO in use.
These patches depends on patch series that adds support for Marvell Guruplug-Display [1,2].
Signed-off-by: Ajay Bhargav ajay.bhargav@einfochips.com
arch/arm/include/asm/arch-armada100/gpio.h | 130 +++++++++++++++++++ drivers/gpio/Makefile | 1 + drivers/gpio/armada100_gpio.c | 192 ++++++++++++++++++++++++++++ 3 files changed, 323 insertions(+), 0 deletions(-) create mode 100644 arch/arm/include/asm/arch-armada100/gpio.h create mode 100644 drivers/gpio/armada100_gpio.c
diff --git a/arch/arm/include/asm/arch-armada100/gpio.h b/arch/arm/include/asm/arch-armada100/gpio.h new file mode 100644 index 0000000..9024a2e --- /dev/null +++ b/arch/arm/include/asm/arch-armada100/gpio.h @@ -0,0 +1,130 @@ +/*
- (C) Copyright 2010
2011???
- eInfochips Ltd. <www.einfochips.com>
- Written-by: Ajay Bhargav ajay.bhargav@einfochips.com
- (C) Copyright 2010
- Marvell Semiconductor <www.marvell.com>
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
- MA 02110-1301 USA
- */
+#ifndef _ASM_ARCH_GPIO_H +#define _ASM_ARCH_GPIO_H
+#ifndef __ASSEMBLY__ +#include <asm/types.h> +#endif
+#if defined(CONFIG_ARMADA100_GPIO) +#include <asm/arch/armada100.h>
+#define GPIO_LABEL_MAX 20 +#define ARMD_MAX_GPIO 128
Pls derive it from MAX_MFP
+#define GPIO_TO_REG(gp) (gp >> 5) +#define GPIO_TO_BIT(gp) (1 << (gp & 0x1F)) +#define GPIO_VAL(gp, val) ((val >> (gp & 0x1F)) & 0x01)
+#define GPIO_SET 1 +#define GPIO_CLR 0
+/*
- GPIO register map
- Refer Datasheet Appendix A.36
- */
+struct armdgpio_registers {
- u32 gplr0; /* Pin Level Register - 0x0000 */
- u32 gplr1; /* 0x0004 */
- u32 gplr2; /* 0x0008 */
- u32 gpdr0; /* Pin Direction Register - 0x000C */
NAK
You should define all GPIO register in a single struct And point them with base address offsets
- u32 gpdr1; /* 0x0010 */
- u32 gpdr2; /* 0x0014 */
- u32 gpsr0; /* Pin Output Set Register - 0x0018 */
- u32 gpsr1; /* 0x001C */
- u32 gpsr2; /* 0x0020 */
- u32 gpcr0; /* Pin Output Clear Register - 0x0024 */
- u32 gpcr1; /* 0x0028 */
- u32 gpcr2; /* 0x002C */
- u32 grer0; /* Rising-Edge Detect Enable Register - 0x0030 */
- u32 grer1; /* 0x0034 */
- u32 grer2; /* 0x0038 */
- u32 gfer0; /* Falling-Edge Detect Enable Register - 0x003C */
- u32 gfer1; /* 0x0040 */
- u32 gfer2; /* 0x0044 */
- u32 gedr0; /* Edge Detect Status Register - 0x0048 */
- u32 gedr1; /* 0x004C */
- u32 gedr2; /* 0x0050 */
- u32 gsdr0; /* Bitwise Set of GPIO Direction Register - 0x0054 */
- u32 gsdr1; /* 0x0058 */
- u32 gsdr2; /* 0x005C */
- u32 gcdr0; /* Bitwise Clear of GPIO Direction Register - 0x0060 */
- u32 gcdr1; /* 0x0064 */
- u32 gcdr2; /* 0x0068 */
- u32 gsrer0; /* Bitwise Set of Rising-Edge Detect Enable
Register - 0x006C */- u32 gsrer1; /* 0x0070 */
- u32 gsrer2; /* 0x0074 */
- u32 gcrer0; /* Bitwise Clear of Rising-Edge Detect Enable
Register - 0x0078 */- u32 gcrer1; /* 0x007C */
- u32 gcrer2; /* 0x0080 */
- u32 gsfer0; /* Bitwise Set of Falling-Edge Detect Enable
Register - 0x0084 */- u32 gsfer1; /* 0x0088 */
- u32 gsfer2; /* 0x008C */
- u32 gcfer0; /* Bitwise Clear of Falling-Edge Detect Enable
Register - 0x0090 */- u32 gcfer1; /* 0x0094 */
- u32 gcfer2; /* 0x0098 */
- u32 apmask0; /* Bitwise Mask of Edge Detect Register - 0x009C */
- u32 apmask1; /* 0x00A0 */
- u32 apmask2; /* 0x00A4 */
- u8 pad[0x0100 - 0x00A4 - 4];
- u32 gplr3; /* 0x0100 */
- u8 pad1[0x010C - 0x0100 - 4];
- u32 gpdr3; /* 0x010C */
- u8 pad2[0x0118 - 0x010C - 4];
- u32 gpsr3; /* 0x0118 */
- u8 pad3[0x0124 - 0x0118 - 4];
- u32 gpcr3; /* 0x0124 */
- u8 pad4[0x0130 - 0x0124 - 4];
- u32 grer3; /* 0x0130 */
- u8 pad5[0x013C - 0x0130 - 4];
- u32 gfer3; /* 0x013C */
- u8 pad6[0x0148 - 0x013C - 4];
- u32 gedr3; /* 0x0148 */
- u8 pad7[0x0154 - 0x0148 - 4];
- u32 gsdr3; /* 0x0154 */
- u8 pad8[0x0160 - 0x0154 - 4];
- u32 gcdr3; /* 0x0160 */
- u8 pad9[0x016C - 0x0160 - 4];
- u32 gsrer3; /* 0x016C */
- u8 pad10[0x0178 - 0x016C - 4];
- u32 gcrer3; /* 0x0178 */
- u8 pad11[0x0184 - 0x0178 - 4];
- u32 gsfer3; /* 0x0184 */
- u8 pad12[0x0190 - 0x0184 - 4];
- u32 gcfer3; /* 0x0190 */
- u8 pad13[0x019C - 0x0190 - 4];
- u32 apmask3; /* 0x019C */
+};
+#endif /* CONFIG_ARMADA100_GPIO */ +#endif /* _ASM_ARCH_GPIO_H */ diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 1e3ae11..31b83cd 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk LIB := $(obj)libgpio.o
COBJS-$(CONFIG_AT91_GPIO) += at91_gpio.o +COBJS-$(CONFIG_ARMADA100_GPIO) += armada100_gpio.o
I suggest you to add mvgpio.c instead of armada100_gpio.c This will be used in future by other Marvell SoCs.
COBJS-$(CONFIG_KIRKWOOD_GPIO) += kw_gpio.o COBJS-$(CONFIG_MARVELL_MFP) += mvmfp.o COBJS-$(CONFIG_MXC_GPIO) += mxc_gpio.o diff --git a/drivers/gpio/armada100_gpio.c b/drivers/gpio/armada100_gpio.c new file mode 100644 index 0000000..578fdac --- /dev/null +++ b/drivers/gpio/armada100_gpio.c @@ -0,0 +1,192 @@ +/*
- (C) Copyright 2010
2011??
- eInfochips Ltd. <www.einfochips.com>
- Written-by: Ajay Bhargav ajay.bhargav@einfochips.com
- (C) Copyright 2010
- Marvell Semiconductor <www.marvell.com>
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
- MA 02110-1301 USA
- */
+#include <common.h> +#include <asm/io.h> +#include <asm/errno.h> +#include <asm/gpio.h>
+int gpio_request(int gp, const char *label) +{
- /*
* Assumes corresponding MFP is configured peoperly* for use as GPIO*/
NAK, you should check here, respective MFP is being configured as GPIO, if not you should return error
- return 0;
+}
+void gpio_free(int gp) +{
- /* default direction for GPIO is input */
- gpio_direction_input(gp);
+}
+void gpio_toggle_value(int gp) +{
- gpio_set_value(gp, !gpio_get_value(gp));
+}
+int gpio_direction_input(int gp) +{
- struct armdgpio_registers *gpio_regs =
(struct armdgpio_registers *)ARMD1_GPIO_BASE;
Consider now this is generic GPIO driver for marvell SOCs, define this macro in gpio.h
- if (gp < ARMD_MAX_GPIO) {
switch (GPIO_TO_REG(gp)) {
Some code comments are welcomed here.
case 0:writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr0);break;case 1:writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr1);break;case 2:writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr2);break;case 3:writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr3);break;default:return -EINVAL;}- } else {
return -EINVAL;- }
- return 0;
+}
+int gpio_direction_output(int gp, int value) +{
- struct armdgpio_registers *gpio_regs =
(struct armdgpio_registers *)ARMD1_GPIO_BASE;- if (gp < ARMD_MAX_GPIO) {
switch (GPIO_TO_REG(gp)) {case 0:writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr0);
Call gpio_set_value(gp, value) here which is defined below doing the same
if (value)writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr0);elsewritel(GPIO_TO_BIT(gp), &gpio_regs->gpcr0);break;case 1:writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr1);if (value)writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1);elsewritel(GPIO_TO_BIT(gp), &gpio_regs->gpcr1);break;case 2:writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr2);if (value)writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr2);elsewritel(GPIO_TO_BIT(gp), &gpio_regs->gpcr2);break;case 3:writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr3);if (value)writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr3);elsewritel(GPIO_TO_BIT(gp), &gpio_regs->gpcr3);break;default:return -EINVAL;}- } else {
return -EINVAL;- }
- return 0;
+}
+int gpio_get_value(int gp) +{
- struct armdgpio_registers *gpio_regs =
(struct armdgpio_registers *)ARMD1_GPIO_BASE;- u32 gp_val;
- if (gp < ARMD_MAX_GPIO) {
switch (GPIO_TO_REG(gp)) {case 0:gp_val = readl(&gpio_regs->gplr0);break;case 1:gp_val = readl(&gpio_regs->gplr1);break;case 2:gp_val = readl(&gpio_regs->gplr2);break;case 3:gp_val = readl(&gpio_regs->gplr3);break;default:return -EINVAL;}- } else {
return -EINVAL;- }
- return GPIO_VAL(gp, gp_val);
+}
+void gpio_set_value(int gp, int value) +{
- struct armdgpio_registers *gpio_regs =
(struct armdgpio_registers *)ARMD1_GPIO_BASE;- if (gp < ARMD_MAX_GPIO) {
switch (GPIO_TO_REG(gp)) {case 0:if (value)writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr0);elsewritel(GPIO_TO_BIT(gp), &gpio_regs->gpcr0);break;case 1:if (value)writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1);elsewritel(GPIO_TO_BIT(gp), &gpio_regs->gpcr1);break;case 2:if (value)writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr2);elsewritel(GPIO_TO_BIT(gp), &gpio_regs->gpcr2);break;case 3:if (value)writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr3);elsewritel(GPIO_TO_BIT(gp), &gpio_regs->gpcr3);break;default:panic("Invalid GPIO pin %u\n", gp);}- } else {
panic("Invalid GPIO pin %u\n", gp);
Can you eliminate one panic line here?
Regards.. Prafulla . .
How about merge this into current mvmfp.c? Just add some function into it, then no need another c file.
Best regards, Lei
On Tue, Jul 19, 2011 at 3:01 AM, Prafulla Wadaskar prafulla@marvell.com wrote:
-----Original Message----- From: Ajay Bhargav [mailto:ajay.bhargav@einfochips.com] Sent: Monday, July 18, 2011 3:12 PM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de; Ajay Bhargav Subject: [PATCH 1/4] gpio: Adds GPIO driver support for Armada100
Correct patch name as gpio: Add GPIO driver for Marvell SoC Armada100
This patch adds generic GPIO driver framework support for Armada100 series.
To enable GPIO driver define CONFIG_ARMADA100_GPIO and for GPIO commands define CONFIG_CMD_GPIO in your board configuration file.
NOTE: This driver assumes proper configuration of MFP register for the GPIO in use.
These patches depends on patch series that adds support for Marvell Guruplug-Display [1,2].
Signed-off-by: Ajay Bhargav ajay.bhargav@einfochips.com
arch/arm/include/asm/arch-armada100/gpio.h | 130 +++++++++++++++++++ drivers/gpio/Makefile | 1 + drivers/gpio/armada100_gpio.c | 192 ++++++++++++++++++++++++++++ 3 files changed, 323 insertions(+), 0 deletions(-) create mode 100644 arch/arm/include/asm/arch-armada100/gpio.h create mode 100644 drivers/gpio/armada100_gpio.c
diff --git a/arch/arm/include/asm/arch-armada100/gpio.h b/arch/arm/include/asm/arch-armada100/gpio.h new file mode 100644 index 0000000..9024a2e --- /dev/null +++ b/arch/arm/include/asm/arch-armada100/gpio.h @@ -0,0 +1,130 @@ +/*
- (C) Copyright 2010
2011???
- eInfochips Ltd. <www.einfochips.com>
- Written-by: Ajay Bhargav ajay.bhargav@einfochips.com
- (C) Copyright 2010
- Marvell Semiconductor <www.marvell.com>
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
- MA 02110-1301 USA
- */
+#ifndef _ASM_ARCH_GPIO_H +#define _ASM_ARCH_GPIO_H
+#ifndef __ASSEMBLY__ +#include <asm/types.h> +#endif
+#if defined(CONFIG_ARMADA100_GPIO) +#include <asm/arch/armada100.h>
+#define GPIO_LABEL_MAX 20 +#define ARMD_MAX_GPIO 128
Pls derive it from MAX_MFP
+#define GPIO_TO_REG(gp) (gp >> 5) +#define GPIO_TO_BIT(gp) (1 << (gp & 0x1F)) +#define GPIO_VAL(gp, val) ((val >> (gp & 0x1F)) & 0x01)
+#define GPIO_SET 1 +#define GPIO_CLR 0
+/*
- GPIO register map
- Refer Datasheet Appendix A.36
- */
+struct armdgpio_registers {
- u32 gplr0; /* Pin Level Register - 0x0000 */
- u32 gplr1; /* 0x0004 */
- u32 gplr2; /* 0x0008 */
- u32 gpdr0; /* Pin Direction Register - 0x000C */
NAK
You should define all GPIO register in a single struct And point them with base address offsets
- u32 gpdr1; /* 0x0010 */
- u32 gpdr2; /* 0x0014 */
- u32 gpsr0; /* Pin Output Set Register - 0x0018 */
- u32 gpsr1; /* 0x001C */
- u32 gpsr2; /* 0x0020 */
- u32 gpcr0; /* Pin Output Clear Register - 0x0024 */
- u32 gpcr1; /* 0x0028 */
- u32 gpcr2; /* 0x002C */
- u32 grer0; /* Rising-Edge Detect Enable Register - 0x0030 */
- u32 grer1; /* 0x0034 */
- u32 grer2; /* 0x0038 */
- u32 gfer0; /* Falling-Edge Detect Enable Register - 0x003C */
- u32 gfer1; /* 0x0040 */
- u32 gfer2; /* 0x0044 */
- u32 gedr0; /* Edge Detect Status Register - 0x0048 */
- u32 gedr1; /* 0x004C */
- u32 gedr2; /* 0x0050 */
- u32 gsdr0; /* Bitwise Set of GPIO Direction Register - 0x0054 */
- u32 gsdr1; /* 0x0058 */
- u32 gsdr2; /* 0x005C */
- u32 gcdr0; /* Bitwise Clear of GPIO Direction Register - 0x0060 */
- u32 gcdr1; /* 0x0064 */
- u32 gcdr2; /* 0x0068 */
- u32 gsrer0; /* Bitwise Set of Rising-Edge Detect Enable
- Register - 0x006C */
- u32 gsrer1; /* 0x0070 */
- u32 gsrer2; /* 0x0074 */
- u32 gcrer0; /* Bitwise Clear of Rising-Edge Detect Enable
- Register - 0x0078 */
- u32 gcrer1; /* 0x007C */
- u32 gcrer2; /* 0x0080 */
- u32 gsfer0; /* Bitwise Set of Falling-Edge Detect Enable
- Register - 0x0084 */
- u32 gsfer1; /* 0x0088 */
- u32 gsfer2; /* 0x008C */
- u32 gcfer0; /* Bitwise Clear of Falling-Edge Detect Enable
- Register - 0x0090 */
- u32 gcfer1; /* 0x0094 */
- u32 gcfer2; /* 0x0098 */
- u32 apmask0; /* Bitwise Mask of Edge Detect Register - 0x009C */
- u32 apmask1; /* 0x00A0 */
- u32 apmask2; /* 0x00A4 */
- u8 pad[0x0100 - 0x00A4 - 4];
- u32 gplr3; /* 0x0100 */
- u8 pad1[0x010C - 0x0100 - 4];
- u32 gpdr3; /* 0x010C */
- u8 pad2[0x0118 - 0x010C - 4];
- u32 gpsr3; /* 0x0118 */
- u8 pad3[0x0124 - 0x0118 - 4];
- u32 gpcr3; /* 0x0124 */
- u8 pad4[0x0130 - 0x0124 - 4];
- u32 grer3; /* 0x0130 */
- u8 pad5[0x013C - 0x0130 - 4];
- u32 gfer3; /* 0x013C */
- u8 pad6[0x0148 - 0x013C - 4];
- u32 gedr3; /* 0x0148 */
- u8 pad7[0x0154 - 0x0148 - 4];
- u32 gsdr3; /* 0x0154 */
- u8 pad8[0x0160 - 0x0154 - 4];
- u32 gcdr3; /* 0x0160 */
- u8 pad9[0x016C - 0x0160 - 4];
- u32 gsrer3; /* 0x016C */
- u8 pad10[0x0178 - 0x016C - 4];
- u32 gcrer3; /* 0x0178 */
- u8 pad11[0x0184 - 0x0178 - 4];
- u32 gsfer3; /* 0x0184 */
- u8 pad12[0x0190 - 0x0184 - 4];
- u32 gcfer3; /* 0x0190 */
- u8 pad13[0x019C - 0x0190 - 4];
- u32 apmask3; /* 0x019C */
+};
+#endif /* CONFIG_ARMADA100_GPIO */ +#endif /* _ASM_ARCH_GPIO_H */ diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 1e3ae11..31b83cd 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk LIB := $(obj)libgpio.o
COBJS-$(CONFIG_AT91_GPIO) += at91_gpio.o +COBJS-$(CONFIG_ARMADA100_GPIO) += armada100_gpio.o
I suggest you to add mvgpio.c instead of armada100_gpio.c This will be used in future by other Marvell SoCs.
COBJS-$(CONFIG_KIRKWOOD_GPIO) += kw_gpio.o COBJS-$(CONFIG_MARVELL_MFP) += mvmfp.o COBJS-$(CONFIG_MXC_GPIO) += mxc_gpio.o diff --git a/drivers/gpio/armada100_gpio.c b/drivers/gpio/armada100_gpio.c new file mode 100644 index 0000000..578fdac --- /dev/null +++ b/drivers/gpio/armada100_gpio.c @@ -0,0 +1,192 @@ +/*
- (C) Copyright 2010
2011??
- eInfochips Ltd. <www.einfochips.com>
- Written-by: Ajay Bhargav ajay.bhargav@einfochips.com
- (C) Copyright 2010
- Marvell Semiconductor <www.marvell.com>
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
- MA 02110-1301 USA
- */
+#include <common.h> +#include <asm/io.h> +#include <asm/errno.h> +#include <asm/gpio.h>
+int gpio_request(int gp, const char *label) +{
- /*
- * Assumes corresponding MFP is configured peoperly
- * for use as GPIO
- */
NAK, you should check here, respective MFP is being configured as GPIO, if not you should return error
- return 0;
+}
+void gpio_free(int gp) +{
- /* default direction for GPIO is input */
- gpio_direction_input(gp);
+}
+void gpio_toggle_value(int gp) +{
- gpio_set_value(gp, !gpio_get_value(gp));
+}
+int gpio_direction_input(int gp) +{
- struct armdgpio_registers *gpio_regs =
- (struct armdgpio_registers *)ARMD1_GPIO_BASE;
Consider now this is generic GPIO driver for marvell SOCs, define this macro in gpio.h
- if (gp < ARMD_MAX_GPIO) {
- switch (GPIO_TO_REG(gp)) {
Some code comments are welcomed here.
- case 0:
- writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr0);
- break;
- case 1:
- writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr1);
- break;
- case 2:
- writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr2);
- break;
- case 3:
- writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr3);
- break;
- default:
- return -EINVAL;
- }
- } else {
- return -EINVAL;
- }
- return 0;
+}
+int gpio_direction_output(int gp, int value) +{
- struct armdgpio_registers *gpio_regs =
- (struct armdgpio_registers *)ARMD1_GPIO_BASE;
- if (gp < ARMD_MAX_GPIO) {
- switch (GPIO_TO_REG(gp)) {
- case 0:
- writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr0);
Call gpio_set_value(gp, value) here which is defined below doing the same
- if (value)
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr0);
- else
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr0);
- break;
- case 1:
- writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr1);
- if (value)
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1);
- else
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr1);
- break;
- case 2:
- writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr2);
- if (value)
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr2);
- else
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr2);
- break;
- case 3:
- writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr3);
- if (value)
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr3);
- else
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr3);
- break;
- default:
- return -EINVAL;
- }
- } else {
- return -EINVAL;
- }
- return 0;
+}
+int gpio_get_value(int gp) +{
- struct armdgpio_registers *gpio_regs =
- (struct armdgpio_registers *)ARMD1_GPIO_BASE;
- u32 gp_val;
- if (gp < ARMD_MAX_GPIO) {
- switch (GPIO_TO_REG(gp)) {
- case 0:
- gp_val = readl(&gpio_regs->gplr0);
- break;
- case 1:
- gp_val = readl(&gpio_regs->gplr1);
- break;
- case 2:
- gp_val = readl(&gpio_regs->gplr2);
- break;
- case 3:
- gp_val = readl(&gpio_regs->gplr3);
- break;
- default:
- return -EINVAL;
- }
- } else {
- return -EINVAL;
- }
- return GPIO_VAL(gp, gp_val);
+}
+void gpio_set_value(int gp, int value) +{
- struct armdgpio_registers *gpio_regs =
- (struct armdgpio_registers *)ARMD1_GPIO_BASE;
- if (gp < ARMD_MAX_GPIO) {
- switch (GPIO_TO_REG(gp)) {
- case 0:
- if (value)
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr0);
- else
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr0);
- break;
- case 1:
- if (value)
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1);
- else
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr1);
- break;
- case 2:
- if (value)
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr2);
- else
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr2);
- break;
- case 3:
- if (value)
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr3);
- else
- writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr3);
- break;
- default:
- panic("Invalid GPIO pin %u\n", gp);
- }
- } else {
- panic("Invalid GPIO pin %u\n", gp);
Can you eliminate one panic line here?
Regards.. Prafulla . . _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
How about merge this into current mvmfp.c? Just add some function into it, then no need another c file.
Best regards, Lei
Hi Lei,
According to current ongoing development there is generic GPIO framework being introduced. Its good if we keep gpio separate though they are connected to MFP too, It makes more sense if they are kept in different file. lets see what Prafulla has to say about this.
Regards, Ajay Bhargav
Hi Ajay,
On Tue, Jul 19, 2011 at 12:01 PM, Ajay Bhargav ajay.bhargav@einfochips.com wrote:
How about merge this into current mvmfp.c? Just add some function into it, then no need another c file.
Best regards, Lei
Hi Lei,
According to current ongoing development there is generic GPIO framework being introduced. Its good if we keep gpio separate though they are connected to MFP too, It makes more sense if they are kept in different file. lets see what Prafulla has to say about this.
Ok. BTW, I also have some comments towards your patch. You define a huge structure in arch/arm/include/asm/arch-armada100/gpio.h, which don't looks so good to me.
In our code base, that file only keeps as short as:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
#define BANK_OFF(n) (((n) < 3) ? (n) << 2 : 0x100 + (((n) - 3) << 2)) #define GPIO_REG(x) (*((volatile u32 *)(CONFIG_GPIO_REGBASE + (x))))
#define NR_BUILTIN_GPIO (128)
#define GPIO_bit(gpio) (1 << ((gpio) & 0x1f))
#define GPLR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x00) #define GPDR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x0c) #define GPSR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x18) #define GPCR(x) GPIO_REG(BANK_OFF((x) >> 5) + 0x24)
int gpio_get_value(unsigned gpio); void gpio_set_value(unsigned gpio, int value); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
Macro here save a lot space, and keep the code clean.
Best regards, Lei
----- "Lei Wen" adrian.wenl@gmail.com wrote:
Hi Ajay,
On Tue, Jul 19, 2011 at 12:01 PM, Ajay Bhargav ajay.bhargav@einfochips.com wrote:
How about merge this into current mvmfp.c? Just add some function into it, then no need another c file.
Best regards, Lei
Hi Lei,
According to current ongoing development there is generic GPIO
framework being introduced. Its good if we keep gpio separate though they are connected to MFP too, It makes more sense if they are kept in different file. lets see what Prafulla has to say about this.
Ok. BTW, I also have some comments towards your patch. You define a huge structure in arch/arm/include/asm/arch-armada100/gpio.h, which don't looks so good to me.
Comments are welcome. I know its huge.. I just followed what is suggested by Wolfgang. He told not to use BASE+OFFSET thing. I am bit confused here whom to follow :)
Macro here save a lot space, and keep the code clean.
Best regards, Lei
Prafulla is suggesting something.. Let me ask him how he want this.
Regards, Ajay Bhargav
-----Original Message----- From: Ajay Bhargav [mailto:ajay.bhargav@einfochips.com] Sent: Tuesday, July 19, 2011 9:44 AM To: Lei Wen Cc: u-boot@lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik; Prafulla Wadaskar Subject: Re: [U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100
----- "Lei Wen" adrian.wenl@gmail.com wrote:
Hi Ajay,
On Tue, Jul 19, 2011 at 12:01 PM, Ajay Bhargav ajay.bhargav@einfochips.com wrote:
How about merge this into current mvmfp.c? Just add some function into it, then no need another c file.
According to current ongoing development there is generic GPIO
framework being introduced. Its good if we keep gpio separate though they are connected to MFP too, It makes more sense if they are kept in different file. lets see what Prafulla has to say about this.
I think mvgpio.c will address gpio driver, it can be used by Kirkwood, Orion arch too where as MFP is totally different on this core than armada.
To me it makes more sense to call mvgpio.c as gpio driver and mvmfp.c as Multi Function Pin driver.
Ok. BTW, I also have some comments towards your patch. You define a huge structure in arch/arm/include/asm/arch-armada100/gpio.h, which don't looks so good to me.
Comments are welcome. I know its huge.. I just followed what is suggested by Wolfgang. He told not to use BASE+OFFSET thing. I am bit confused here whom to follow :)
You have to follow all :-), more reviewers more better code output. BASE+OFFSET strictly not recommended.
Macro here save a lot space, and keep the code clean.
Best regards, Lei
Prafulla is suggesting something.. Let me ask him how he want this.
I think lei and me are suggesting similar things, macros should be used precisely, the code should be small and smarter.
Regards.. Prafulla . .
Dear Lei Wen,
A: Full quoting.
Q: What is the second most annoying thing in e-mail?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
In message CALZhoSTVOiLQ2Br9A1bDEbE=9XjSHUs=402cFfqP_qQ2S0bZDw@mail.gmail.com you wrote:
How about merge this into current mvmfp.c? Just add some function into it, then no need another c file.
...
-----Original Message-----
500 lines full quote deleted.
It makes ZERO sense to quote 500 lines of text that you are not really referring to.
Please STOP top posting / full quoting. If you need help, please read http://www.netmeister.org/news/learn2quote.html
Thanks.
Wolfgang Denk
Hi Wolfgang
On Tue, Jul 19, 2011 at 1:27 PM, Wolfgang Denk wd@denx.de wrote:
Dear Lei Wen,
A: Full quoting.
Q: What is the second most annoying thing in e-mail?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
Understand. Sorry for this...
Best regards, Lei
----- "Prafulla Wadaskar" prafulla@marvell.com wrote:
You should define all GPIO register in a single struct And point them with base address offsets
I suggest you to add mvgpio.c instead of armada100_gpio.c This will be used in future by other Marvell SoCs.
GPIO registers might vary for other Marvell SoCs? How to deal with that?
+int gpio_request(int gp, const char *label) +{
- /*
* Assumes corresponding MFP is configured peoperly* for use as GPIO*/NAK, you should check here, respective MFP is being configured as GPIO, if not you should return error
I checked datasheet and for most GPIOs, AF1 is given as GPIO but for few its not, so adding a glue logic to check for specific GPIOs wont be a good idea. Thats the reason i thought its good to keep MFP out of this. Please give suggestions.
Consider now this is generic GPIO driver for marvell SOCs, define this macro in gpio.h
ok
- if (gp < ARMD_MAX_GPIO) {
switch (GPIO_TO_REG(gp)) {Some code comments are welcomed here.
case 0:writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr0);break;case 1:writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr1);break;case 2:writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr2);break;case 3:writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr3);break;default:return -EINVAL;}- } else {
return -EINVAL;- }
- return 0;
+}
+int gpio_direction_output(int gp, int value) +{
- struct armdgpio_registers *gpio_regs =
(struct armdgpio_registers *)ARMD1_GPIO_BASE;- if (gp < ARMD_MAX_GPIO) {
switch (GPIO_TO_REG(gp)) {case 0:writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr0);Call gpio_set_value(gp, value) here which is defined below doing the same
thanks for suggestion.
panic("Invalid GPIO pin %u\n", gp);}- } else {
panic("Invalid GPIO pin %u\n", gp);Can you eliminate one panic line here?
Regards.. Prafulla . .
will do that... there is no return value from this function so i thought it wont be good to continue from this point incase designed direction is not set.
Regards, Ajay Bhargav
-----Original Message----- From: Ajay Bhargav [mailto:ajay.bhargav@einfochips.com] Sent: Tuesday, July 19, 2011 9:53 AM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik Subject: Re: [PATCH 1/4] gpio: Adds GPIO driver support for Armada100
----- "Prafulla Wadaskar" prafulla@marvell.com wrote:
You should define all GPIO register in a single struct And point them with base address offsets
I suggest you to add mvgpio.c instead of armada100_gpio.c This will be used in future by other Marvell SoCs.
GPIO registers might vary for other Marvell SoCs? How to deal with that?
That can be thought of while adding support for othe SoCs. Preferably define register struct in asm/arch/gpio.h
Regards.. Prafulla . .
Hi Ajay,
On Tue, Jul 19, 2011 at 12:23 PM, Ajay Bhargav ajay.bhargav@einfochips.com wrote:
----- "Prafulla Wadaskar" prafulla@marvell.com wrote:
You should define all GPIO register in a single struct And point them with base address offsets
I suggest you to add mvgpio.c instead of armada100_gpio.c This will be used in future by other Marvell SoCs.
GPIO registers might vary for other Marvell SoCs? How to deal with that?
I could tell you that for the pxa series branch, the gpio register arrangement not change for years, and still keep it valid for the next generation product.
So mvgpio.c could be used by pxa168(aspen), pxa910(td), pxa610(mmp2), pxa620(mmp3).
Best regards, Lei
participants (5)
-
Ajay Bhargav -
Lei Wen -
Mike Frysinger -
Prafulla Wadaskar -
Wolfgang Denk