From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vikas Manocha Date: Mon, 10 Apr 2017 09:25:29 -0700 Subject: [U-Boot] [PATCH v2 08/18] dm: gpio: Add driver for stm32f7 gpio controller In-Reply-To: References: <1491342324-5820-1-git-send-email-vikas.manocha@st.com> <1491342324-5820-9-git-send-email-vikas.manocha@st.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Thanks Simon, On 04/09/2017 12:27 PM, Simon Glass wrote: > Hi Vikas, > > On 4 April 2017 at 15:45, Vikas Manocha wrote: >> This patch adds gpio driver supporting driver model for stm32f7 gpio. >> >> Signed-off-by: Vikas Manocha >> cc: Christophe KERELLO >> --- >> >> Changes in v2: >> - included files in correct order. >> - moved the pinctrl specific routine from gpio driver to pinctrl >> - used dev_get_addr() instead of fdtdec_get_addr_size_auto_parent() in >> gpio driver. >> - pointed gpio name to bank name in device tree blob rather than copy. >> >> arch/arm/include/asm/arch-stm32f7/gpio.h | 16 ++++ >> drivers/gpio/Kconfig | 9 +++ >> drivers/gpio/Makefile | 1 + >> drivers/gpio/stm32f7_gpio.c | 135 +++++++++++++++++++++++++++++++ >> drivers/pinctrl/pinctrl_stm32.c | 36 ++++++++- >> 5 files changed, 196 insertions(+), 1 deletion(-) >> create mode 100644 drivers/gpio/stm32f7_gpio.c >> > > Reviewed-by: Simon Glass > > But please see below. > >> diff --git a/arch/arm/include/asm/arch-stm32f7/gpio.h b/arch/arm/include/asm/arch-stm32f7/gpio.h >> index 2942cd9..5c0300f 100644 >> --- a/arch/arm/include/asm/arch-stm32f7/gpio.h >> +++ b/arch/arm/include/asm/arch-stm32f7/gpio.h >> @@ -96,6 +96,22 @@ struct stm32_gpio_ctl { >> enum stm32_gpio_af af; >> }; >> >> +struct stm32_gpio_regs { >> + u32 moder; /* GPIO port mode */ >> + u32 otyper; /* GPIO port output type */ >> + u32 ospeedr; /* GPIO port output speed */ >> + u32 pupdr; /* GPIO port pull-up/pull-down */ >> + u32 idr; /* GPIO port input data */ >> + u32 odr; /* GPIO port output data */ >> + u32 bsrr; /* GPIO port bit set/reset */ >> + u32 lckr; /* GPIO port configuration lock */ >> + u32 afr[2]; /* GPIO alternate function */ >> +}; >> + >> +struct stm32_gpio_priv { >> + struct stm32_gpio_regs *regs; >> +}; >> + >> static inline unsigned stm32_gpio_to_port(unsigned gpio) >> { >> return gpio / 16; >> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig >> index 8d9ab52..c8af398 100644 >> --- a/drivers/gpio/Kconfig >> +++ b/drivers/gpio/Kconfig >> @@ -151,6 +151,15 @@ config PIC32_GPIO >> help >> Say yes here to support Microchip PIC32 GPIOs. >> >> +config STM32F7_GPIO >> + bool "ST STM32 GPIO driver" >> + depends on DM_GPIO >> + default y >> + help >> + Device model driver support for STM32 GPIO controller. It should be >> + usable on many stm32 families like stm32f4 & stm32H7. >> + Tested on STM32F7. >> + >> config MVEBU_GPIO >> bool "Marvell MVEBU GPIO driver" >> depends on DM_GPIO && ARCH_MVEBU >> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile >> index 8939226..9c2a9cc 100644 >> --- a/drivers/gpio/Makefile >> +++ b/drivers/gpio/Makefile >> @@ -49,6 +49,7 @@ oby-$(CONFIG_SX151X) += sx151x.o >> obj-$(CONFIG_SUNXI_GPIO) += sunxi_gpio.o >> obj-$(CONFIG_LPC32XX_GPIO) += lpc32xx_gpio.o >> obj-$(CONFIG_STM32_GPIO) += stm32_gpio.o >> +obj-$(CONFIG_STM32F7_GPIO) += stm32f7_gpio.o >> obj-$(CONFIG_GPIO_UNIPHIER) += gpio-uniphier.o >> obj-$(CONFIG_ZYNQ_GPIO) += zynq_gpio.o >> obj-$(CONFIG_VYBRID_GPIO) += vybrid_gpio.o >> diff --git a/drivers/gpio/stm32f7_gpio.c b/drivers/gpio/stm32f7_gpio.c >> new file mode 100644 >> index 0000000..5ab1c5c >> --- /dev/null >> +++ b/drivers/gpio/stm32f7_gpio.c >> @@ -0,0 +1,135 @@ >> +/* >> + * (C) Copyright 2017 >> + * Vikas Manocha, >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include >> +#include > > common.h should always be first ok, I will move it before clk.h in v3. > >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + > > [..] > >> diff --git a/drivers/pinctrl/pinctrl_stm32.c b/drivers/pinctrl/pinctrl_stm32.c >> index aa2c440..e5b6b3b 100644 >> --- a/drivers/pinctrl/pinctrl_stm32.c >> +++ b/drivers/pinctrl/pinctrl_stm32.c >> @@ -1,10 +1,44 @@ >> #include >> -#include >> #include >> #include >> +#include >> +#include >> +#include >> >> DECLARE_GLOBAL_DATA_PTR; >> >> +#define MODE_BITS_MASK 3 >> +#define OSPEED_MASK 3 >> +#define PUPD_MASK 3 >> +#define OTYPE_MSK 1 >> +#define AFR_MASK 0xF >> + >> +int stm32_gpio_config(struct gpio_desc *desc, const struct stm32_gpio_ctl *ctl) > > What is this function for? It does not seem to be called. This function is being called from stm32_pinctrl_set_state_simple() in same pinctrl driver. > > Also we should not be exporting functions from drivers. Instead here a > pinctrl driver should be accessed via the API in pinctrl.h. pinctrl driver is accessed via the the APIs only, the function stm32_gpio_config should be static to avoid this confusion. I will make it static in v3. Cheers, Vikas > > Regards, > Simon > . >