[U-Boot] [PATCH v2 08/18] dm: gpio: Add driver for stm32f7 gpio controller

From: Simon Glass <sjg@chromium.org>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v2 08/18] dm: gpio: Add driver for stm32f7 gpio controller
Date: Sun, 9 Apr 2017 13:27:26 -0600	[thread overview]
Message-ID: <CAPnjgZ2Z81_CLNxhb9=uCQd6c-1J98pwqfOm_Qx6QuDq7R6oEw@mail.gmail.com> (raw)
In-Reply-To: <1491342324-5820-9-git-send-email-vikas.manocha@st.com>

Hi Vikas,

On 4 April 2017 at 15:45, Vikas Manocha <vikas.manocha@st.com> wrote:
> This patch adds gpio driver supporting driver model for stm32f7 gpio.
>
> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> cc: Christophe KERELLO <christophe.kerello@st.com>
> ---
>
> 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 <sjg@chromium.org>

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, <vikas.manocha@st.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <clk.h>
> +#include <common.h>

common.h should always be first

> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <asm/arch/gpio.h>
> +#include <asm/arch/stm32.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +

[..]

> 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 <common.h>
> -#include <asm/arch/gpio.h>
>  #include <dm.h>
>  #include <dm/pinctrl.h>
> +#include <asm/arch/gpio.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
>
>  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.

Also we should not be exporting functions from drivers. Instead here a
pinctrl driver should be accessed via the API in pinctrl.h.

Regards,
Simon