| Message ID | 1588080785-6812-10-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Tue, 28 Apr 2020 at 15:39, Rajendra Nayak <rnayak@codeaurora.org> wrote: > > Even though specifying OPP's in device tree is optional, ignoring all errors > reported by dev_pm_opp_of_add_table() means we can't distinguish between a > missing OPP table and a wrong/buggy OPP table. While missing OPP table > (dev_pm_opp_of_add_table() returns a -ENODEV in such case) can be ignored, > a wrong/buggy OPP table in device tree should make the driver error out. > > while we fix that, lets also fix the variable names for opp/opp_table to > avoid confusion and name them opp_table/has_opp_table instead. > > Suggested-by: Matthias Kaehlcke <matthias@chromium.org> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Pradeep P V K <ppvk@codeaurora.org> > Cc: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> > Cc: linux-mmc@vger.kernel.org Is this a standalone patch that I queue up via my mmc tree? Kind regards Uffe > --- > drivers/mmc/host/sdhci-msm.c | 27 ++++++++++++++++----------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index 8a055dd..97758fa 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -244,8 +244,8 @@ struct sdhci_msm_host { > struct clk_bulk_data bulk_clks[4]; /* core, iface, cal, sleep clocks */ > unsigned long clk_rate; > struct mmc_host *mmc; > - struct opp_table *opp; > - bool opp_table; > + struct opp_table *opp_table; > + bool has_opp_table; > bool use_14lpp_dll_reset; > bool tuning_done; > bool calibration_done; > @@ -1967,15 +1967,20 @@ static int sdhci_msm_probe(struct platform_device *pdev) > } > msm_host->bulk_clks[0].clk = clk; > > - msm_host->opp = dev_pm_opp_set_clkname(&pdev->dev, "core"); > - if (IS_ERR(msm_host->opp)) { > - ret = PTR_ERR(msm_host->opp); > + msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); > + if (IS_ERR(msm_host->opp_table)) { > + ret = PTR_ERR(msm_host->opp_table); > goto bus_clk_disable; > } > > /* OPP table is optional */ > - if (!dev_pm_opp_of_add_table(&pdev->dev)) > - msm_host->opp_table = true; > + ret = dev_pm_opp_of_add_table(&pdev->dev); > + if (!ret) { > + msm_host->has_opp_table = true; > + } else if (ret != -ENODEV) { > + dev_err(&pdev->dev, "Invalid OPP table in Device tree\n"); > + goto opp_cleanup; > + } > > /* Vote for maximum clock rate for maximum performance */ > ret = dev_pm_opp_set_rate(&pdev->dev, INT_MAX); > @@ -2133,9 +2138,9 @@ static int sdhci_msm_probe(struct platform_device *pdev) > clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks), > msm_host->bulk_clks); > opp_cleanup: > - if (msm_host->opp_table) > + if (msm_host->has_opp_table) > dev_pm_opp_of_remove_table(&pdev->dev); > - dev_pm_opp_put_clkname(msm_host->opp); > + dev_pm_opp_put_clkname(msm_host->opp_table); > bus_clk_disable: > if (!IS_ERR(msm_host->bus_clk)) > clk_disable_unprepare(msm_host->bus_clk); > @@ -2154,9 +2159,9 @@ static int sdhci_msm_remove(struct platform_device *pdev) > > sdhci_remove_host(host, dead); > > - if (msm_host->opp_table) > + if (msm_host->has_opp_table) > dev_pm_opp_of_remove_table(&pdev->dev); > - dev_pm_opp_put_clkname(msm_host->opp); > + dev_pm_opp_put_clkname(msm_host->opp_table); > pm_runtime_get_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > pm_runtime_put_noidle(&pdev->dev); > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation
On 4/28/2020 11:59 PM, Ulf Hansson wrote: > On Tue, 28 Apr 2020 at 15:39, Rajendra Nayak <rnayak@codeaurora.org> wrote: >> >> Even though specifying OPP's in device tree is optional, ignoring all errors >> reported by dev_pm_opp_of_add_table() means we can't distinguish between a >> missing OPP table and a wrong/buggy OPP table. While missing OPP table >> (dev_pm_opp_of_add_table() returns a -ENODEV in such case) can be ignored, >> a wrong/buggy OPP table in device tree should make the driver error out. >> >> while we fix that, lets also fix the variable names for opp/opp_table to >> avoid confusion and name them opp_table/has_opp_table instead. >> >> Suggested-by: Matthias Kaehlcke <matthias@chromium.org> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> Cc: Pradeep P V K <ppvk@codeaurora.org> >> Cc: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> >> Cc: linux-mmc@vger.kernel.org > > Is this a standalone patch that I queue up via my mmc tree? Hi Ulf, yes, its a standalone patch which applies on top of the one you already have in your tree. No other dependencies. > > Kind regards > Uffe > >> --- >> drivers/mmc/host/sdhci-msm.c | 27 ++++++++++++++++----------- >> 1 file changed, 16 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >> index 8a055dd..97758fa 100644 >> --- a/drivers/mmc/host/sdhci-msm.c >> +++ b/drivers/mmc/host/sdhci-msm.c >> @@ -244,8 +244,8 @@ struct sdhci_msm_host { >> struct clk_bulk_data bulk_clks[4]; /* core, iface, cal, sleep clocks */ >> unsigned long clk_rate; >> struct mmc_host *mmc; >> - struct opp_table *opp; >> - bool opp_table; >> + struct opp_table *opp_table; >> + bool has_opp_table; >> bool use_14lpp_dll_reset; >> bool tuning_done; >> bool calibration_done; >> @@ -1967,15 +1967,20 @@ static int sdhci_msm_probe(struct platform_device *pdev) >> } >> msm_host->bulk_clks[0].clk = clk; >> >> - msm_host->opp = dev_pm_opp_set_clkname(&pdev->dev, "core"); >> - if (IS_ERR(msm_host->opp)) { >> - ret = PTR_ERR(msm_host->opp); >> + msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); >> + if (IS_ERR(msm_host->opp_table)) { >> + ret = PTR_ERR(msm_host->opp_table); >> goto bus_clk_disable; >> } >> >> /* OPP table is optional */ >> - if (!dev_pm_opp_of_add_table(&pdev->dev)) >> - msm_host->opp_table = true; >> + ret = dev_pm_opp_of_add_table(&pdev->dev); >> + if (!ret) { >> + msm_host->has_opp_table = true; >> + } else if (ret != -ENODEV) { >> + dev_err(&pdev->dev, "Invalid OPP table in Device tree\n"); >> + goto opp_cleanup; >> + } >> >> /* Vote for maximum clock rate for maximum performance */ >> ret = dev_pm_opp_set_rate(&pdev->dev, INT_MAX); >> @@ -2133,9 +2138,9 @@ static int sdhci_msm_probe(struct platform_device *pdev) >> clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks), >> msm_host->bulk_clks); >> opp_cleanup: >> - if (msm_host->opp_table) >> + if (msm_host->has_opp_table) >> dev_pm_opp_of_remove_table(&pdev->dev); >> - dev_pm_opp_put_clkname(msm_host->opp); >> + dev_pm_opp_put_clkname(msm_host->opp_table); >> bus_clk_disable: >> if (!IS_ERR(msm_host->bus_clk)) >> clk_disable_unprepare(msm_host->bus_clk); >> @@ -2154,9 +2159,9 @@ static int sdhci_msm_remove(struct platform_device *pdev) >> >> sdhci_remove_host(host, dead); >> >> - if (msm_host->opp_table) >> + if (msm_host->has_opp_table) >> dev_pm_opp_of_remove_table(&pdev->dev); >> - dev_pm_opp_put_clkname(msm_host->opp); >> + dev_pm_opp_put_clkname(msm_host->opp_table); >> pm_runtime_get_sync(&pdev->dev); >> pm_runtime_disable(&pdev->dev); >> pm_runtime_put_noidle(&pdev->dev); >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation
On Wed, 29 Apr 2020 at 16:09, Rajendra Nayak <rnayak@codeaurora.org> wrote: > > > On 4/28/2020 11:59 PM, Ulf Hansson wrote: > > On Tue, 28 Apr 2020 at 15:39, Rajendra Nayak <rnayak@codeaurora.org> wrote: > >> > >> Even though specifying OPP's in device tree is optional, ignoring all errors > >> reported by dev_pm_opp_of_add_table() means we can't distinguish between a > >> missing OPP table and a wrong/buggy OPP table. While missing OPP table > >> (dev_pm_opp_of_add_table() returns a -ENODEV in such case) can be ignored, > >> a wrong/buggy OPP table in device tree should make the driver error out. > >> > >> while we fix that, lets also fix the variable names for opp/opp_table to > >> avoid confusion and name them opp_table/has_opp_table instead. > >> > >> Suggested-by: Matthias Kaehlcke <matthias@chromium.org> > >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > >> Cc: Ulf Hansson <ulf.hansson@linaro.org> > >> Cc: Pradeep P V K <ppvk@codeaurora.org> > >> Cc: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> > >> Cc: linux-mmc@vger.kernel.org > > > > Is this a standalone patch that I queue up via my mmc tree? > > Hi Ulf, yes, its a standalone patch which applies on top of the one > you already have in your tree. No other dependencies. Thanks for confirming! Perhaps next time you could add this information as part of a description to the patch (where we usually add patch version information). Anyway, applied for next! [...] Kind regards Uffe
On 5/5/2020 5:03 PM, Ulf Hansson wrote: > On Wed, 29 Apr 2020 at 16:09, Rajendra Nayak <rnayak@codeaurora.org> wrote: >> >> >> On 4/28/2020 11:59 PM, Ulf Hansson wrote: >>> On Tue, 28 Apr 2020 at 15:39, Rajendra Nayak <rnayak@codeaurora.org> wrote: >>>> >>>> Even though specifying OPP's in device tree is optional, ignoring all errors >>>> reported by dev_pm_opp_of_add_table() means we can't distinguish between a >>>> missing OPP table and a wrong/buggy OPP table. While missing OPP table >>>> (dev_pm_opp_of_add_table() returns a -ENODEV in such case) can be ignored, >>>> a wrong/buggy OPP table in device tree should make the driver error out. >>>> >>>> while we fix that, lets also fix the variable names for opp/opp_table to >>>> avoid confusion and name them opp_table/has_opp_table instead. >>>> >>>> Suggested-by: Matthias Kaehlcke <matthias@chromium.org> >>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >>>> Cc: Ulf Hansson <ulf.hansson@linaro.org> >>>> Cc: Pradeep P V K <ppvk@codeaurora.org> >>>> Cc: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> >>>> Cc: linux-mmc@vger.kernel.org >>> >>> Is this a standalone patch that I queue up via my mmc tree? >> >> Hi Ulf, yes, its a standalone patch which applies on top of the one >> you already have in your tree. No other dependencies. > > Thanks for confirming! Perhaps next time you could add this > information as part of a description to the patch (where we usually > add patch version information). > > Anyway, applied for next! Thanks Ulf, I should have sent this out as a standalone patch instead of including it with the reset of the series, which caused the confusion. Sorry about that :/
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 8a055dd..97758fa 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -244,8 +244,8 @@ struct sdhci_msm_host { struct clk_bulk_data bulk_clks[4]; /* core, iface, cal, sleep clocks */ unsigned long clk_rate; struct mmc_host *mmc; - struct opp_table *opp; - bool opp_table; + struct opp_table *opp_table; + bool has_opp_table; bool use_14lpp_dll_reset; bool tuning_done; bool calibration_done; @@ -1967,15 +1967,20 @@ static int sdhci_msm_probe(struct platform_device *pdev) } msm_host->bulk_clks[0].clk = clk; - msm_host->opp = dev_pm_opp_set_clkname(&pdev->dev, "core"); - if (IS_ERR(msm_host->opp)) { - ret = PTR_ERR(msm_host->opp); + msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); + if (IS_ERR(msm_host->opp_table)) { + ret = PTR_ERR(msm_host->opp_table); goto bus_clk_disable; } /* OPP table is optional */ - if (!dev_pm_opp_of_add_table(&pdev->dev)) - msm_host->opp_table = true; + ret = dev_pm_opp_of_add_table(&pdev->dev); + if (!ret) { + msm_host->has_opp_table = true; + } else if (ret != -ENODEV) { + dev_err(&pdev->dev, "Invalid OPP table in Device tree\n"); + goto opp_cleanup; + } /* Vote for maximum clock rate for maximum performance */ ret = dev_pm_opp_set_rate(&pdev->dev, INT_MAX); @@ -2133,9 +2138,9 @@ static int sdhci_msm_probe(struct platform_device *pdev) clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks), msm_host->bulk_clks); opp_cleanup: - if (msm_host->opp_table) + if (msm_host->has_opp_table) dev_pm_opp_of_remove_table(&pdev->dev); - dev_pm_opp_put_clkname(msm_host->opp); + dev_pm_opp_put_clkname(msm_host->opp_table); bus_clk_disable: if (!IS_ERR(msm_host->bus_clk)) clk_disable_unprepare(msm_host->bus_clk); @@ -2154,9 +2159,9 @@ static int sdhci_msm_remove(struct platform_device *pdev) sdhci_remove_host(host, dead); - if (msm_host->opp_table) + if (msm_host->has_opp_table) dev_pm_opp_of_remove_table(&pdev->dev); - dev_pm_opp_put_clkname(msm_host->opp); + dev_pm_opp_put_clkname(msm_host->opp_table); pm_runtime_get_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); pm_runtime_put_noidle(&pdev->dev);
Even though specifying OPP's in device tree is optional, ignoring all errors reported by dev_pm_opp_of_add_table() means we can't distinguish between a missing OPP table and a wrong/buggy OPP table. While missing OPP table (dev_pm_opp_of_add_table() returns a -ENODEV in such case) can be ignored, a wrong/buggy OPP table in device tree should make the driver error out. while we fix that, lets also fix the variable names for opp/opp_table to avoid confusion and name them opp_table/has_opp_table instead. Suggested-by: Matthias Kaehlcke <matthias@chromium.org> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Pradeep P V K <ppvk@codeaurora.org> Cc: Veerabhadrarao Badiganti <vbadigan@codeaurora.org> Cc: linux-mmc@vger.kernel.org --- drivers/mmc/host/sdhci-msm.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)