diff mbox series

[02/11,v3] mmc: host: tmio: Use GPIO descriptors

Message ID 20181125225217.23201-3-linus.walleij@linaro.org
State Superseded
Headers show
Series Use GPIO descriptors for CD/WP | expand

Commit Message

Linus Walleij Nov. 25, 2018, 10:52 p.m. UTC 
The TMIO MMC driver was passing global GPIO numbers around for
card detect. It turns out only one single board in the kernel
was actually making use of this feature so it is pretty easy
to convert the driver to use only GPIO descriptors.

The lines are flagged as GPIO_ACTIVE_LOW as that is what they
are, but this will be ignored by the MMC slot GPIO code that
uses the *raw accessors and rely on the caps2 flag for any
inversion semantics.

Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v1->v3:
- Kuninori/Laurent: any tests/ACKs appreciated.
---
 arch/sh/boards/mach-ecovec24/setup.c | 26 ++++++++++++++++++++++----
 drivers/mmc/host/tmio_mmc_core.c     | 12 +++++++-----
 include/linux/mfd/tmio.h             |  9 ++-------
 3 files changed, 31 insertions(+), 16 deletions(-)

-- 
2.19.1

Comments

Kuninori Morimoto Nov. 26, 2018, 12:28 a.m. UTC | #1 
Hi Linus

> The TMIO MMC driver was passing global GPIO numbers around for

> card detect. It turns out only one single board in the kernel

> was actually making use of this feature so it is pretty easy

> to convert the driver to use only GPIO descriptors.

> 

> The lines are flagged as GPIO_ACTIVE_LOW as that is what they

> are, but this will be ignored by the MMC slot GPIO code that

> uses the *raw accessors and rely on the caps2 flag for any

> inversion semantics.

> 

> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog v1->v3:

> - Kuninori/Laurent: any tests/ACKs appreciated.


For Ecovec portion, I can't test it anymore,
but for [01/11], [02/11]

	Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Laurent Pinchart Nov. 26, 2018, 8:23 a.m. UTC | #2 
Hi Linus,

Thank you for the patch.

On Monday, 26 November 2018 00:52:08 EET Linus Walleij wrote:
> The TMIO MMC driver was passing global GPIO numbers around for

> card detect. It turns out only one single board in the kernel

> was actually making use of this feature so it is pretty easy

> to convert the driver to use only GPIO descriptors.

> 

> The lines are flagged as GPIO_ACTIVE_LOW as that is what they

> are, but this will be ignored by the MMC slot GPIO code that

> uses the *raw accessors and rely on the caps2 flag for any

> inversion semantics.

> 

> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog v1->v3:

> - Kuninori/Laurent: any tests/ACKs appreciated.

> ---

>  arch/sh/boards/mach-ecovec24/setup.c | 26 ++++++++++++++++++++++----

>  drivers/mmc/host/tmio_mmc_core.c     | 12 +++++++-----

>  include/linux/mfd/tmio.h             |  9 ++-------

>  3 files changed, 31 insertions(+), 16 deletions(-)

> 

> diff --git a/arch/sh/boards/mach-ecovec24/setup.c

> b/arch/sh/boards/mach-ecovec24/setup.c index 3097307b7cb7..af2c28946319

> 100644

> --- a/arch/sh/boards/mach-ecovec24/setup.c

> +++ b/arch/sh/boards/mach-ecovec24/setup.c

> @@ -696,13 +696,20 @@ static struct gpiod_lookup_table

> sdhi0_power_gpiod_table = { },

>  };

> 

> +static struct gpiod_lookup_table sdhi0_gpio_table = {

> +	.dev_id = "sh_mobile_sdhi.0",

> +	.table = {

> +		/* Card detect */

> +		GPIO_LOOKUP("sh7724_pfc", GPIO_PTY7, "cd", GPIO_ACTIVE_LOW),

> +		{ },

> +	},

> +};

> +

>  static struct tmio_mmc_data sdhi0_info = {

>  	.chan_priv_tx	= (void *)SHDMA_SLAVE_SDHI0_TX,

>  	.chan_priv_rx	= (void *)SHDMA_SLAVE_SDHI0_RX,

>  	.capabilities	= MMC_CAP_SDIO_IRQ | MMC_CAP_POWER_OFF_CARD |

>  			  MMC_CAP_NEEDS_POLL,

> -	.flags		= TMIO_MMC_USE_GPIO_CD,

> -	.cd_gpio	= GPIO_PTY7,

>  };

> 

>  static struct resource sdhi0_resources[] = {

> @@ -735,8 +742,15 @@ static struct tmio_mmc_data sdhi1_info = {

>  	.chan_priv_rx	= (void *)SHDMA_SLAVE_SDHI1_RX,

>  	.capabilities	= MMC_CAP_SDIO_IRQ | MMC_CAP_POWER_OFF_CARD |

>  			  MMC_CAP_NEEDS_POLL,

> -	.flags		= TMIO_MMC_USE_GPIO_CD,

> -	.cd_gpio	= GPIO_PTW7,

> +};

> +

> +static struct gpiod_lookup_table sdhi1_gpio_table = {

> +	.dev_id = "sh_mobile_sdhi.1",

> +	.table = {

> +		/* Card detect */

> +		GPIO_LOOKUP("sh7724_pfc", GPIO_PTW7, "cd", GPIO_ACTIVE_LOW),

> +		{ },

> +	},

>  };

> 

>  static struct resource sdhi1_resources[] = {

> @@ -1445,6 +1459,10 @@ static int __init arch_setup(void)

>  	gpiod_add_lookup_table(&cn12_power_gpiod_table);

>  #if defined(CONFIG_MMC_SDHI) || defined(CONFIG_MMC_SDHI_MODULE)

>  	gpiod_add_lookup_table(&sdhi0_power_gpiod_table);

> +	gpiod_add_lookup_table(&sdhi0_gpio_table);

> +#endif

> +#if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)

> +	gpiod_add_lookup_table(&sdhi1_gpio_table);

>  #endif

> 

>  	return platform_add_devices(ecovec_devices,

> diff --git a/drivers/mmc/host/tmio_mmc_core.c

> b/drivers/mmc/host/tmio_mmc_core.c index 8d64f6196f33..487b88dceff4 100644

> --- a/drivers/mmc/host/tmio_mmc_core.c

> +++ b/drivers/mmc/host/tmio_mmc_core.c

> @@ -1167,11 +1167,13 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)

> if (ret < 0)

>  		return ret;

> 

> -	if (pdata->flags & TMIO_MMC_USE_GPIO_CD) {

> -		ret = mmc_gpio_request_cd(mmc, pdata->cd_gpio, 0);

> -		if (ret)

> -			return ret;

> -	}

> +	/*

> +	 * Look for a card detect GPIO, if it fails with anything

> +	 * else than a probe deferral, just live without it.

> +	 */

> +	ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL);

> +	if (ret == -EPROBE_DEFER)

> +		return ret;


Shouldn't you set the override_active_level argument to true in order for the 
raw GPIO accessors to be used, as explained in the commit message ?

Apart from this the patch looks good to me,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


>  	mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;

>  	mmc->caps2 |= pdata->capabilities2;

> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h

> index 1e70060c92ce..e2687a30e5a1 100644

> --- a/include/linux/mfd/tmio.h

> +++ b/include/linux/mfd/tmio.h

> @@ -54,12 +54,8 @@

>   * idle before writing to some registers.

>   */

>  #define TMIO_MMC_HAS_IDLE_WAIT		BIT(4)

> -/*

> - * A GPIO is used for card hotplug detection. We need an extra flag for

> this, - * because 0 is a valid GPIO number too, and requiring users to

> specify - * cd_gpio < 0 to disable GPIO hotplug would break backwards

> compatibility. - */

> -#define TMIO_MMC_USE_GPIO_CD		BIT(5)

> +

> +/* BIT(5) is unused */

> 

>  /*

>   * Some controllers have CMD12 automatically

> @@ -104,7 +100,6 @@ struct tmio_mmc_data {

>  	unsigned long			capabilities2;

>  	unsigned long			flags;

>  	u32				ocr_mask;	/* available voltages */

> -	unsigned int			cd_gpio;

>  	int				alignment_shift;

>  	dma_addr_t			dma_rx_offset;

>  	unsigned int			max_blk_count;


-- 
Regards,

Laurent Pinchart
Linus Walleij Nov. 30, 2018, 3:44 p.m. UTC | #3 
On Mon, Nov 26, 2018 at 9:23 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> > +     ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL);

>

> Shouldn't you set the override_active_level argument to true in order for the

> raw GPIO accessors to be used, as explained in the commit message ?


As with the other patch, my committ message is confusing.

Since I fixed all the descriptors to indicate active high/low,
they can be trusted and we should not override the active level.

Thanks!

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-ecovec24/setup.c
index 3097307b7cb7..af2c28946319 100644
--- a/arch/sh/boards/mach-ecovec24/setup.c
+++ b/arch/sh/boards/mach-ecovec24/setup.c
@@ -696,13 +696,20 @@  static struct gpiod_lookup_table sdhi0_power_gpiod_table = {
 	},
 };
 
+static struct gpiod_lookup_table sdhi0_gpio_table = {
+	.dev_id = "sh_mobile_sdhi.0",
+	.table = {
+		/* Card detect */
+		GPIO_LOOKUP("sh7724_pfc", GPIO_PTY7, "cd", GPIO_ACTIVE_LOW),
+		{ },
+	},
+};
+
 static struct tmio_mmc_data sdhi0_info = {
 	.chan_priv_tx	= (void *)SHDMA_SLAVE_SDHI0_TX,
 	.chan_priv_rx	= (void *)SHDMA_SLAVE_SDHI0_RX,
 	.capabilities	= MMC_CAP_SDIO_IRQ | MMC_CAP_POWER_OFF_CARD |
 			  MMC_CAP_NEEDS_POLL,
-	.flags		= TMIO_MMC_USE_GPIO_CD,
-	.cd_gpio	= GPIO_PTY7,
 };
 
 static struct resource sdhi0_resources[] = {
@@ -735,8 +742,15 @@  static struct tmio_mmc_data sdhi1_info = {
 	.chan_priv_rx	= (void *)SHDMA_SLAVE_SDHI1_RX,
 	.capabilities	= MMC_CAP_SDIO_IRQ | MMC_CAP_POWER_OFF_CARD |
 			  MMC_CAP_NEEDS_POLL,
-	.flags		= TMIO_MMC_USE_GPIO_CD,
-	.cd_gpio	= GPIO_PTW7,
+};
+
+static struct gpiod_lookup_table sdhi1_gpio_table = {
+	.dev_id = "sh_mobile_sdhi.1",
+	.table = {
+		/* Card detect */
+		GPIO_LOOKUP("sh7724_pfc", GPIO_PTW7, "cd", GPIO_ACTIVE_LOW),
+		{ },
+	},
 };
 
 static struct resource sdhi1_resources[] = {
@@ -1445,6 +1459,10 @@  static int __init arch_setup(void)
 	gpiod_add_lookup_table(&cn12_power_gpiod_table);
 #if defined(CONFIG_MMC_SDHI) || defined(CONFIG_MMC_SDHI_MODULE)
 	gpiod_add_lookup_table(&sdhi0_power_gpiod_table);
+	gpiod_add_lookup_table(&sdhi0_gpio_table);
+#endif
+#if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
+	gpiod_add_lookup_table(&sdhi1_gpio_table);
 #endif
 
 	return platform_add_devices(ecovec_devices,
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 8d64f6196f33..487b88dceff4 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1167,11 +1167,13 @@  int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 	if (ret < 0)
 		return ret;
 
-	if (pdata->flags & TMIO_MMC_USE_GPIO_CD) {
-		ret = mmc_gpio_request_cd(mmc, pdata->cd_gpio, 0);
-		if (ret)
-			return ret;
-	}
+	/*
+	 * Look for a card detect GPIO, if it fails with anything
+	 * else than a probe deferral, just live without it.
+	 */
+	ret = mmc_gpiod_request_cd(mmc, "cd", 0, false, 0, NULL);
+	if (ret == -EPROBE_DEFER)
+		return ret;
 
 	mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
 	mmc->caps2 |= pdata->capabilities2;
diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index 1e70060c92ce..e2687a30e5a1 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -54,12 +54,8 @@ 
  * idle before writing to some registers.
  */
 #define TMIO_MMC_HAS_IDLE_WAIT		BIT(4)
-/*
- * A GPIO is used for card hotplug detection. We need an extra flag for this,
- * because 0 is a valid GPIO number too, and requiring users to specify
- * cd_gpio < 0 to disable GPIO hotplug would break backwards compatibility.
- */
-#define TMIO_MMC_USE_GPIO_CD		BIT(5)
+
+/* BIT(5) is unused */
 
 /*
  * Some controllers have CMD12 automatically
@@ -104,7 +100,6 @@  struct tmio_mmc_data {
 	unsigned long			capabilities2;
 	unsigned long			flags;
 	u32				ocr_mask;	/* available voltages */
-	unsigned int			cd_gpio;
 	int				alignment_shift;
 	dma_addr_t			dma_rx_offset;
 	unsigned int			max_blk_count;