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

[Vikas Manocha]

Thanks Simon,

On 04/09/2017 12:27 PM, Simon Glass wrote:
> Hi Vikas,
> 
> On 4 April 2017 at 15:45, Vikas Manocha <vikas.manocha at st.com> wrote:
>> This patch adds gpio driver supporting driver model for stm32f7 gpio.
>>
>> Signed-off-by: Vikas Manocha <vikas.manocha at st.com>
>> cc: Christophe KERELLO <christophe.kerello at 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 at 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 at st.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <clk.h>
>> +#include <common.h>
> 
> common.h should always be first

ok, I will move it before clk.h in v3.

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

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