Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754205AbdCTK3D (ORCPT ); Mon, 20 Mar 2017 06:29:03 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:46231 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753019AbdCTK2G (ORCPT ); Mon, 20 Mar 2017 06:28:06 -0400 Message-ID: <1490005679.2917.32.camel@pengutronix.de> Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling From: Philipp Zabel To: Andrzej Hajda Cc: linux-mmc@vger.kernel.org, Jaehoon Chung , Ulf Hansson , linux-kernel@vger.kernel.org, "Szyprowski, Marek" , Bartlomiej Zolnierkiewicz , linux-samsung-soc Date: Mon, 20 Mar 2017 11:27:59 +0100 In-Reply-To: References: <20170315113139.17989-1-p.zabel@pengutronix.de> <1490003602.2917.16.camel@pengutronix.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3806 Lines: 98 Hi Andrzej, On Mon, 2017-03-20 at 11:03 +0100, Andrzej Hajda wrote: > Hi Philipp, > > On 20.03.2017 10:53, Philipp Zabel wrote: > > On Mon, 2017-03-20 at 10:22 +0100, Andrzej Hajda wrote: > >> Hi Philipp, > >> > >> Todays next branch does not work with exynos5433-tm2 board. > >> I guess this patch causes regression. On MMC without reset controller I > >> get errors: > >> [ 4.938222] dwmmc_exynos 15540000.mshc: platform data not available > >> [ 4.943268] dwmmc_exynos: probe of 15540000.mshc failed with error -22 I was thrown off by this. Should maybe dw_mci_probe return the error value reported by dw_mci_parse_dt instead of always returning -EINVAL? > >> [ 4.950184] dwmmc_exynos 15560000.mshc: platform data not available > >> [ 4.955962] dwmmc_exynos: probe of 15560000.mshc failed with error -22 > >> > >> Commenting out reset controller get and error checks 'fixes' the issue. > >> > >> On 15.03.2017 12:31, Philipp Zabel wrote: > >>> As of commit bb475230b8e5 ("reset: make optional functions really > >>> optional"), the reset framework API calls use NULL pointers to describe > >>> optional, non-present reset controls. > >>> > >>> This allows to return errors from devm_reset_control_get_optional and to > >>> call reset_control_(de)assert unconditionally. > >>> > >>> Signed-off-by: Philipp Zabel > >>> --- > >>> drivers/mmc/host/dw_mmc.c | 14 +++++--------- > >>> 1 file changed, 5 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > >>> index a9ac0b4573131..3d62b0a1f81cb 100644 > >>> --- a/drivers/mmc/host/dw_mmc.c > >>> +++ b/drivers/mmc/host/dw_mmc.c > >>> @@ -2968,10 +2968,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host) > >>> > >>> /* find reset controller when exist */ > >>> pdata->rstc = devm_reset_control_get_optional(dev, "reset"); > >>> - if (IS_ERR(pdata->rstc)) { > >>> - if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER) > >>> - return ERR_PTR(-EPROBE_DEFER); > >>> - } > >>> + if (IS_ERR(pdata->rstc)) > >>> + return ERR_CAST(pdata->rstc); > >> > >> With three lines above commented out it works. > > So devm_reset_control_get_optional returns -EINVAL. Why? > > > > The mshc@15560000 node is compatible to "samsung,exynos7-dw-mshc-smu", > > so that's dw_mmc-exynos.c calling dw_mci_pltfm_register, which then > > calls dw_mci_probe, passing the original platform device as > > host->dev = &pdev->dev, and I expect __of_reset_control_get being called > > with a valid DT node (dev->of_node). > > Since id is set to "reset", of_property_match_string(node, > > "reset-names", id) should then be called and return -EINVAL because the > > "reset-names" property does not exist. Then __of_reset_control_get > > should return NULL because optional == true. > > Some of this obviously doesn't happen, where am I wrong? > > > When RESET_CONTROLLER is not enabled dummy stubs return -ENOSUPP error [1]. > > [1]: http://lxr.free-electrons.com/source/include/linux/reset.h#L77 Thanks, I suppose that issue should be fixed by: ----------8<---------- diff --git a/include/linux/reset.h b/include/linux/reset.h index 86b4ed75359e8..c905ff1c21ec6 100644 --- a/include/linux/reset.h +++ b/include/linux/reset.h @@ -74,14 +74,14 @@ static inline struct reset_control *__of_reset_control_get( const char *id, int index, bool shared, bool optional) { - return ERR_PTR(-ENOTSUPP); + return optional ? NULL : ERR_PTR(-ENOTSUPP); } static inline struct reset_control *__devm_reset_control_get( struct device *dev, const char *id, int index, bool shared, bool optional) { - return ERR_PTR(-ENOTSUPP); + return optional ? NULL : ERR_PTR(-ENOTSUPP); } #endif /* CONFIG_RESET_CONTROLLER */ ---------->8---------- regards Philipp