Re: [PATCH] RFC: fmc: Try to convert to GPIO descriptors

[Federico Vaga <federico.vaga@xxxxxxx>]

Hello Linus,

I take this occasion to renovate my proposal

https://lkml.org/lkml/2018/10/29/534


On Tuesday, June 4, 2019 1:06:04 AM CEST Linus Walleij wrote:
> This tries to convert the FMC subsystem to use GPIO descriptors.
> I say try because several pieces of this puzzle seems to not
> be in the mainline kernel.
>
> For details of this change, see drivers/gpio/TODO.
> 
> We assume the FMC device is created somewhere and the GPIOs can
> be added in this place using gpio descriptor tables from
> <linux/gpio/machine.h> as in other conversions, but the place
> where the FMC device is created does not seem to be in the
> mainline Linux kernel. We assume the index ordering can be
> made to match the current GPIO index order in FMC which
> isn't especially specific.
> 
> We get rid of some GPIO compatibility defines for kernel 3.0
> in the process, it is long overdue.
> 
> It might be that gpio numbers in this subsystem has nothing
> to do with gpio numbers in the GPIO subsystem, and it is just
> including <linux/gpio.h> and reusing some defines for no
> good reason. Then this should be changed to some approach
> decoupling FMC from the GPIO subsystem.
> 
> Cc: Federico Vaga <federico.vaga@xxxxxxx>
> Cc: Pat Riehecky <riehecky@xxxxxxxx>
> Cc: Alessandro Rubini <rubini@xxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/fmc/fmc-core.c    |  1 +
>  drivers/fmc/fmc-trivial.c | 21 ++++++++++++++++-----
>  include/linux/fmc.h       | 15 ++++-----------
>  3 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/fmc/fmc-core.c b/drivers/fmc/fmc-core.c
> index bbcb505d1522..f08b6f81f442 100644
> --- a/drivers/fmc/fmc-core.c
> +++ b/drivers/fmc/fmc-core.c
> @@ -14,6 +14,7 @@
>  #include <linux/device.h>
>  #include <linux/fmc.h>
>  #include <linux/fmc-sdb.h>
> +#include <linux/gpio/consumer.h>
> 
>  #include "fmc-private.h"
> 
> diff --git a/drivers/fmc/fmc-trivial.c b/drivers/fmc/fmc-trivial.c
> index 8defdee3e3a3..bab02d17f02c 100644
> --- a/drivers/fmc/fmc-trivial.c
> +++ b/drivers/fmc/fmc-trivial.c
> @@ -15,7 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/fmc.h>
> 
>  static struct fmc_driver t_drv; /* initialized later */
> @@ -31,12 +31,12 @@ static irqreturn_t t_handler(int irq, void *dev_id)
> 
>  static struct fmc_gpio t_gpio[] = {
>  	{
> -		.gpio = FMC_GPIO_IRQ(0),
> -		.mode = GPIOF_DIR_IN,
> +		.gpio_index = FMC_GPIO_IRQ(0),
> +		.flags = GPIOD_IN,
>  		.irqmode = IRQF_TRIGGER_RISING,
>  	}, {
> -		.gpio = FMC_GPIO_IRQ(1),
> -		.mode = GPIOF_DIR_IN,
> +		.gpio_index = FMC_GPIO_IRQ(1),
> +		.flags = GPIOD_IN,
>  		.irqmode = IRQF_TRIGGER_RISING,
>  	}
>  };
> @@ -45,6 +45,7 @@ static int t_probe(struct fmc_device *fmc)
>  {
>  	int ret;
>  	int index = 0;
> +	int i;
> 
>  	index = fmc_validate(fmc, &t_drv);
>  	if (index < 0)
> @@ -53,6 +54,16 @@ static int t_probe(struct fmc_device *fmc)
>  	ret = fmc_irq_request(fmc, t_handler, "fmc-trivial", IRQF_SHARED);
>  	if (ret < 0)
>  		return ret;
> +	/*
> +	 * All GPIOs are associated with the FMC device using machine
> +	 * descriptor tables or similar. Pick the indices we want.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(t_gpio); i++) {
> +		t_gpio[i].gpiod = devm_gpiod_get_index(&fmc->dev,
> +						       NULL,
> +						       
t_gpio[i].gpio_index,
> +						       
t_gpio[i].flags);
> +	}
>  	/* ignore error code of call below, we really don't care */
>  	fmc_gpio_config(fmc, t_gpio, ARRAY_SIZE(t_gpio));
> 
> diff --git a/include/linux/fmc.h b/include/linux/fmc.h
> index 3dc8a1b2db7b..452688aa97db 100644
> --- a/include/linux/fmc.h
> +++ b/include/linux/fmc.h
> @@ -15,6 +15,7 @@
>  #include <linux/list.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/gpio/consumer.h>
> 
>  struct fmc_device;
>  struct fmc_driver;
> @@ -99,9 +100,9 @@ struct fmc_driver {
>   */
>  struct fmc_gpio {
>  	char *carrier_name; /* name or NULL for virtual pins */
> -	int gpio;
> -	int _gpio;	/* internal use by the carrier */
> -	int mode;	/* GPIOF_DIR_OUT etc, from <linux/gpio.h> */
> +	int gpio_index;
> +	struct gpio_desc *gpiod;
> +	enum gpiod_flags flags; /* see <linux/gpio/consumer.h> */
>  	int irqmode;	/* IRQF_TRIGGER_LOW and so on */
>  };
> 
> @@ -115,14 +116,6 @@ struct fmc_gpio {
>  #define FMC_GPIO_USER(x)	((x) + 0x1400)	/*  256 of them */
>  /* We may add SCL and SDA, or other roles if the need arises */
> 
> -/* GPIOF_DIR_IN etc are missing before 3.0. copy from <linux/gpio.h> */
> -#ifndef GPIOF_DIR_IN
> -#  define GPIOF_DIR_OUT   (0 << 0)
> -#  define GPIOF_DIR_IN    (1 << 0)
> -#  define GPIOF_INIT_LOW  (0 << 1)
> -#  define GPIOF_INIT_HIGH (1 << 1)
> -#endif
> -
>  /*
>   * The operations are offered by each carrier and should make driver
>   * design completely independent of the carrier. Named GPIO pins may be