[v4] mmc: dw_mmc: let device core setup the default pin configuration

Commit Message

Thomas Abraham April 9, 2013, 5:59 a.m. UTC

With device core now able to setup the default pin configuration,
the pin configuration code based on the deprecated Samsung specific
gpio bindings is removed.

Signed-off-by: Thomas Abraham <[e-mail]>
Acked-by: Linus Walleij <[e-mail]>
Reviewed-by: Doug Anderson <[e-mail]>
Tested-by: Doug Anderson <[e-mail]>
Acked-by: Seungwon Jeon <[e-mail]>
---
Changes since v3:
- Rebased to the latest mmc-next branch, resolving the compilation
  error with this patch due to changes in commit f2f942ce
  "mmc: dw_mmc: Check return value of regulator_enable".
  Thanks to Doug for pointing out this issue with the v3 patch.

 drivers/mmc/host/dw_mmc-exynos.c |   38 --------------------------------------
 drivers/mmc/host/dw_mmc.c        |   14 +++-----------
 drivers/mmc/host/dw_mmc.h        |    3 ---
 3 files changed, 3 insertions(+), 52 deletions(-)

Comments

Doug Anderson April 9, 2013, 11:30 p.m. UTC | #1

Thomas,

On Mon, Apr 8, 2013 at 10:59 PM, Thomas Abraham
<[e-mail]> wrote:
> @@ -2002,7 +1994,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>                 if (ret) {
>                         dev_err(host->dev,
>                                 "failed to enable regulator: %d\n", ret);
> -                       goto err_setup_bus;
> +                       return ret;

It seems like you'd need a "mmc_free_host(mmc);" in this case don't
you?  AKA: this should be a goto and not a return.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thomas Abraham April 10, 2013, 12:48 p.m. UTC | #2

On 10 April 2013 05:00, Doug Anderson <[e-mail]> wrote:
> Thomas,
>
> On Mon, Apr 8, 2013 at 10:59 PM, Thomas Abraham
> <[e-mail]> wrote:
>> @@ -2002,7 +1994,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>>                 if (ret) {
>>                         dev_err(host->dev,
>>                                 "failed to enable regulator: %d\n", ret);
>> -                       goto err_setup_bus;
>> +                       return ret;
>
> It seems like you'd need a "mmc_free_host(mmc);" in this case don't
> you?  AKA: this should be a goto and not a return.

Hi Doug,

The call to regulator_enable() is prior to the call to mmc_add_host().
Hence, call to mmc_fre_host is not required in this case. So the above
change should be right.

Thanks,
Thomas.

>
> -Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Doug Anderson April 10, 2013, 1:56 p.m. UTC | #3

Thomas,

On Wed, Apr 10, 2013 at 5:48 AM, Thomas Abraham
<[e-mail]> wrote:
> The call to regulator_enable() is prior to the call to mmc_add_host().
> Hence, call to mmc_fre_host is not required in this case. So the above
> change should be right.

Are you sure that mmc_free_host() is the opposite of mmc_add_host()
and not mmc_alloc_host()?

I'll double-check myself in a few hours when I'm in front of a computer.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Doug Anderson April 10, 2013, 2:55 p.m. UTC | #4

Thomas,

On Wed, Apr 10, 2013 at 6:56 AM, Doug Anderson <[e-mail]> wrote:
> On Wed, Apr 10, 2013 at 5:48 AM, Thomas Abraham
> <[e-mail]> wrote:
>> The call to regulator_enable() is prior to the call to mmc_add_host().
>> Hence, call to mmc_fre_host is not required in this case. So the above
>> change should be right.
>
> Are you sure that mmc_free_host() is the opposite of mmc_add_host()
> and not mmc_alloc_host()?
>
> I'll double-check myself in a few hours when I'm in front of a computer.

OK, I double checked and I'm still convinced that the free is needed
because we've already called mmc_alloc_host().  Current code always
makes sure to free when it returns an error and that seems right to
me.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Seungwon Jeon April 11, 2013, 3:13 a.m. UTC | #5

On Wednesday, April 10, 2013, Doug Anderson wrote:
> Thomas,
> 
> On Wed, Apr 10, 2013 at 6:56 AM, Doug Anderson <[e-mail]> wrote:
> > On Wed, Apr 10, 2013 at 5:48 AM, Thomas Abraham
> > <[e-mail]> wrote:
> >> The call to regulator_enable() is prior to the call to mmc_add_host().
> >> Hence, call to mmc_fre_host is not required in this case. So the above
> >> change should be right.
> >
> > Are you sure that mmc_free_host() is the opposite of mmc_add_host()
> > and not mmc_alloc_host()?
> >
> > I'll double-check myself in a few hours when I'm in front of a computer.
> 
> OK, I double checked and I'm still convinced that the free is needed
> because we've already called mmc_alloc_host().  Current code always
> makes sure to free when it returns an error and that seems right to
> me.
mmc_alloc_host corresponds to mmc_free_host. And mmc_add_host matches
with mmc_remove_host. Let's focus on mmc_alloc_host.
mmc_alloc_host is called prior to regulator_enable.
So, if regulator_enable is failed, call of mmc_free_host makes sense.

Thanks,
Seungwon Jeon
> 
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thomas Abraham April 11, 2013, 12:06 p.m. UTC | #6

On 11 April 2013 08:43, Seungwon Jeon <[e-mail]> wrote:
> On Wednesday, April 10, 2013, Doug Anderson wrote:
>> Thomas,
>>
>> On Wed, Apr 10, 2013 at 6:56 AM, Doug Anderson <[e-mail]> wrote:
>> > On Wed, Apr 10, 2013 at 5:48 AM, Thomas Abraham
>> > <[e-mail]> wrote:
>> >> The call to regulator_enable() is prior to the call to mmc_add_host().
>> >> Hence, call to mmc_fre_host is not required in this case. So the above
>> >> change should be right.
>> >
>> > Are you sure that mmc_free_host() is the opposite of mmc_add_host()
>> > and not mmc_alloc_host()?
>> >
>> > I'll double-check myself in a few hours when I'm in front of a computer.
>>
>> OK, I double checked and I'm still convinced that the free is needed
>> because we've already called mmc_alloc_host().  Current code always
>> makes sure to free when it returns an error and that seems right to
>> me.
> mmc_alloc_host corresponds to mmc_free_host. And mmc_add_host matches
> with mmc_remove_host. Let's focus on mmc_alloc_host.
> mmc_alloc_host is called prior to regulator_enable.
> So, if regulator_enable is failed, call of mmc_free_host makes sense.
>

Hi Doug, Seungwon,

Ok, I got it wrong again. Thanks for letting me know. I will fix this
and send the v5 of this patch.

Thanks,
Thomas.


> Thanks,
> Seungwon Jeon
>>
>> -Doug
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Russell King - ARM Linux April 19, 2013, 10:11 a.m. UTC | #7

On Wed, Apr 10, 2013 at 06:56:48AM -0700, Doug Anderson wrote:
> Thomas,
> 
> On Wed, Apr 10, 2013 at 5:48 AM, Thomas Abraham
> <[e-mail]> wrote:
> > The call to regulator_enable() is prior to the call to mmc_add_host().
> > Hence, call to mmc_fre_host is not required in this case. So the above
> > change should be right.
> 
> Are you sure that mmc_free_host() is the opposite of mmc_add_host()
> and not mmc_alloc_host()?

mmc_free_host() undoes mmc_alloc_host().  mmc_remove_host() undoes
mmc_add_host().

alloc
add
remove
free

is pretty standard terminology, standard ordering.  If add fails, then
free is the right thing to call to clean up after the alloc.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index c7f0976..f013e7e 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -152,43 +152,6 @@  static int dw_mci_exynos_parse_dt(struct dw_mci *host)
 	return 0;
 }
 
-static int dw_mci_exynos_setup_bus(struct dw_mci *host,
-				struct device_node *slot_np, u8 bus_width)
-{
-	int idx, gpio, ret;
-
-	if (!slot_np)
-		return -EINVAL;
-
-	/* cmd + clock + bus-width pins */
-	for (idx = 0; idx < NUM_PINS(bus_width); idx++) {
-		gpio = of_get_gpio(slot_np, idx);
-		if (!gpio_is_valid(gpio)) {
-			dev_err(host->dev, "invalid gpio: %d\n", gpio);
-			return -EINVAL;
-		}
-
-		ret = devm_gpio_request(host->dev, gpio, "dw-mci-bus");
-		if (ret) {
-			dev_err(host->dev, "gpio [%d] request failed\n", gpio);
-			return -EBUSY;
-		}
-	}
-
-	if (host->pdata->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION)
-		return 0;
-
-	gpio = of_get_named_gpio(slot_np, "samsung,cd-pinmux-gpio", 0);
-	if (gpio_is_valid(gpio)) {
-		if (devm_gpio_request(host->dev, gpio, "dw-mci-cd"))
-			dev_err(host->dev, "gpio [%d] request failed\n", gpio);
-	} else {
-		dev_info(host->dev, "cd gpio not available");
-	}
-
-	return 0;
-}
-
 /* Common capabilities of Exynos4/Exynos5 SoC */
 static unsigned long exynos_dwmmc_caps[4] = {
 	MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR |
@@ -205,7 +168,6 @@  static const struct dw_mci_drv_data exynos_drv_data = {
 	.prepare_command	= dw_mci_exynos_prepare_command,
 	.set_ios		= dw_mci_exynos_set_ios,
 	.parse_dt		= dw_mci_exynos_parse_dt,
-	.setup_bus		= dw_mci_exynos_setup_bus,
 };
 
 static const struct of_device_id dw_mci_exynos_match[] = {
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 45d9216..5dcef43 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1952,14 +1952,6 @@  static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 	else
 		bus_width = 1;
 
-	if (drv_data && drv_data->setup_bus) {
-		struct device_node *slot_np;
-		slot_np = dw_mci_of_find_slot_node(host->dev, slot->id);
-		ret = drv_data->setup_bus(host, slot_np, bus_width);
-		if (ret)
-			goto err_setup_bus;
-	}
-
 	switch (bus_width) {
 	case 8:
 		mmc->caps |= MMC_CAP_8_BIT_DATA;
@@ -2002,7 +1994,7 @@  static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 		if (ret) {
 			dev_err(host->dev,
 				"failed to enable regulator: %d\n", ret);
-			goto err_setup_bus;
+			return ret;
 		}
 	}
 
@@ -2015,7 +2007,7 @@  static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 
 	ret = mmc_add_host(mmc);
 	if (ret)
-		goto err_setup_bus;
+		goto err_add_host;
 
 #if defined(CONFIG_DEBUG_FS)
 	dw_mci_init_debugfs(slot);
@@ -2032,7 +2024,7 @@  static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 
 	return 0;
 
-err_setup_bus:
+err_add_host:
 	mmc_free_host(mmc);
 	return -EINVAL;
 }
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 53b8fd9..0b74189 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -190,7 +190,6 @@  extern int dw_mci_resume(struct dw_mci *host);
  * @prepare_command: handle CMD register extensions.
  * @set_ios: handle bus specific extensions.
  * @parse_dt: parse implementation specific device tree properties.
- * @setup_bus: initialize io-interface
  *
  * Provide controller implementation specific extensions. The usage of this
  * data structure is fully optional and usage of each member in this structure
@@ -203,7 +202,5 @@  struct dw_mci_drv_data {
 	void		(*prepare_command)(struct dw_mci *host, u32 *cmdr);
 	void		(*set_ios)(struct dw_mci *host, struct mmc_ios *ios);
 	int		(*parse_dt)(struct dw_mci *host);
-	int		(*setup_bus)(struct dw_mci *host,
-				struct device_node *slot_np, u8 bus_width);
 };
 #endif /* _DW_MMC_H_ */