All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vikas Manocha <vikas.manocha@st.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v2 08/18] dm: gpio: Add driver for stm32f7 gpio controller
Date: Mon, 10 Apr 2017 09:25:29 -0700	[thread overview]
Message-ID: <a8137c74-1f6d-ac1a-5d9c-be75fbac1768@st.com> (raw)
In-Reply-To: <CAPnjgZ2Z81_CLNxhb9=uCQd6c-1J98pwqfOm_Qx6QuDq7R6oEw@mail.gmail.com>

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

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

  reply	other threads:[~2017-04-10 16:25 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-04 21:45 [U-Boot] [PATCH v2 00/18] stm32f7: add sdram & gpio drivers Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 01/18] stm32f7: use clock driver to enable qspi controller clock Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 02/18] stm32f7: sdram: move sdram driver code to ram drivers area Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 03/18] stm32f7: dm: add driver model support for sdram Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 04/18] ARM: DT: stm32f7: add sdram pin contol node Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 05/18] stm32f7: use driver model for sdram initialization Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 06/18] stm32f7: use clock driver to enable sdram controller clock Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 07/18] stm32f7: sdram: use sdram device tree node to configure sdram controller Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 08/18] dm: gpio: Add driver for stm32f7 gpio controller Vikas Manocha
2017-04-09 19:27   ` Simon Glass
2017-04-10 16:25     ` Vikas Manocha [this message]
2017-04-04 21:45 ` [U-Boot] [PATCH v2 09/18] ARM: DT: stm32f7: add gpio device tree nodes Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 10/18] stm32f7: use stm32f7 gpio driver supporting driver model Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 11/18] stm32f746: to switch on user LED1 & read user button Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 12/18] stm32f7: stm32f746-disco: read memory info from device tree Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 13/18] stm32f7: enable board info read " Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 14/18] stm32f7: sdram: correct sdram configuration as per micron sdram Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 15/18] stm32f7: increase the max no of pin configuration to 70 Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 16/18] stm32f7: move board specific pin muxing to dts Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 17/18] stm32f7: add support for stm32f769 disco board Vikas Manocha
2017-04-04 21:45 ` [U-Boot] [PATCH v2 18/18] stm32f7: remove not needed configuration from board config Vikas Manocha

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a8137c74-1f6d-ac1a-5d9c-be75fbac1768@st.com \
    --to=vikas.manocha@st.com \
    --cc=u-boot@lists.denx.de \
    --subject='Re: [U-Boot] [PATCH v2 08/18] dm: gpio: Add driver for stm32f7 gpio controller' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.