| Message ID | 1365487186-4587-1-git-send-email-thomas.abraham@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thomas, On Mon, Apr 8, 2013 at 10:59 PM, Thomas Abraham <thomas.abraham@linaro.org> 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
On 10 April 2013 05:00, Doug Anderson <dianders@chromium.org> wrote: > Thomas, > > On Mon, Apr 8, 2013 at 10:59 PM, Thomas Abraham > <thomas.abraham@linaro.org> 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
Thomas, On Wed, Apr 10, 2013 at 5:48 AM, Thomas Abraham <thomas.abraham@linaro.org> 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
Thomas, On Wed, Apr 10, 2013 at 6:56 AM, Doug Anderson <dianders@chromium.org> wrote: > On Wed, Apr 10, 2013 at 5:48 AM, Thomas Abraham > <thomas.abraham@linaro.org> 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
On Wednesday, April 10, 2013, Doug Anderson wrote: > Thomas, > > On Wed, Apr 10, 2013 at 6:56 AM, Doug Anderson <dianders@chromium.org> wrote: > > On Wed, Apr 10, 2013 at 5:48 AM, Thomas Abraham > > <thomas.abraham@linaro.org> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11 April 2013 08:43, Seungwon Jeon <tgih.jun@samsung.com> wrote: > On Wednesday, April 10, 2013, Doug Anderson wrote: >> Thomas, >> >> On Wed, Apr 10, 2013 at 6:56 AM, Doug Anderson <dianders@chromium.org> wrote: >> > On Wed, Apr 10, 2013 at 5:48 AM, Thomas Abraham >> > <thomas.abraham@linaro.org> 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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >
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 > <thomas.abraham@linaro.org> 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.
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_ */