From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ritesh Harjani Subject: Re: [PATCH v3 3/9] mmc: sdhci-msm: add pltfm_data support to get clk-rates from DT Date: Fri, 19 Aug 2016 19:06:00 +0530 Message-ID: <415fc66f-54d0-b836-359d-c95a01854e92@codeaurora.org> References: <1471581384-18961-1-git-send-email-riteshh@codeaurora.org> <1471581384-18961-4-git-send-email-riteshh@codeaurora.org> <4de1d4bc-3d59-7852-47d6-e5a1b602b205@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:53289 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754250AbcHSNgJ (ORCPT ); Fri, 19 Aug 2016 09:36:09 -0400 In-Reply-To: <4de1d4bc-3d59-7852-47d6-e5a1b602b205@intel.com> Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Adrian Hunter Cc: ulf.hansson@linaro.org, bjorn.andersson@linaro.org, shawn.lin@rock-chips.com, jh80.chung@samsung.com, linux-mmc@vger.kernel.org, linux-arm-msm@vger.kernel.org, georgi.djakov@linaro.org, alex.lemberg@sandisk.com, mateusz.nowak@intel.com, Yuliy.Izrailov@sandisk.com, asutoshd@codeaurora.org, kdorfman@codeaurora.org, david.griego@linaro.org, stummala@codeaurora.org, venkatg@codeaurora.org Hi Adrian, Thanks for the review. Will address the comments below. Regards Ritesh On 8/19/2016 6:33 PM, Adrian Hunter wrote: > On 19/08/16 07:36, Ritesh Harjani wrote: >> This adds support for sdhc-msm controllers to get supported >> clk-rates from DT. sdhci-msm would need it's own set_clock >> ops to be implemented. For this, supported clk-rates needs >> to be populated in sdhci_msm_pltfm_data. >> >> Signed-off-by: Ritesh Harjani >> --- >> .../devicetree/bindings/mmc/sdhci-msm.txt | 1 + >> drivers/mmc/host/sdhci-msm.c | 71 ++++++++++++++++++++++ >> 2 files changed, 72 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt >> index 485483a..6a83b38 100644 >> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt >> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt >> @@ -17,6 +17,7 @@ Required properties: >> "iface" - Main peripheral bus clock (PCLK/HCLK - AHB Bus clock) (required) >> "core" - SDC MMC clock (MCLK) (required) >> "bus" - SDCC bus voter clock (optional) >> +- clk-rates: Array of supported GCC clock frequencies for sdhc, Units - Hz. >> >> Example: >> >> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >> index 85ddaae..2bf141b 100644 >> --- a/drivers/mmc/host/sdhci-msm.c >> +++ b/drivers/mmc/host/sdhci-msm.c >> @@ -74,6 +74,11 @@ >> #define CMUX_SHIFT_PHASE_SHIFT 24 >> #define CMUX_SHIFT_PHASE_MASK (7 << CMUX_SHIFT_PHASE_SHIFT) >> >> +struct sdhci_msm_pltfm_data { >> + u32 *clk_table; >> + size_t clk_table_sz; > > From of_property_count_elems_of_size(), clk_table_sz is an 'int' and is used > as an 'int' in later patches. So it can be an 'int' everywhere. Thanks for noticing, will change it to int. > >> +}; >> + >> struct sdhci_msm_host { >> struct platform_device *pdev; >> void __iomem *core_mem; /* MSM SDCC mapped address */ >> @@ -83,6 +88,7 @@ struct sdhci_msm_host { >> struct clk *bus_clk; /* SDHC bus voter clock */ >> struct mmc_host *mmc; >> bool use_14lpp_dll_reset; >> + struct sdhci_msm_pltfm_data *pdata; >> }; >> >> /* Platform specific tuning */ >> @@ -582,6 +588,67 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = { >> .ops = &sdhci_msm_ops, >> }; >> >> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name, >> + u32 **table, size_t *size) > > Change 'size_t *size' to be 'int *size' Sure. > >> +{ >> + struct device_node *np = dev->of_node; >> + int count = 0; >> + u32 *arr = NULL; >> + int ret = 0; > > Initialization of count and arr is not needed. Alright. > >> + >> + count = of_property_count_elems_of_size(np, prop_name, sizeof(u32)); >> + if (count < 0) { >> + dev_err(dev, "%s: Invalid dt property, err(%d)\n", >> + prop_name, count); >> + ret = count; >> + goto out; > > Just return count Ok. > >> + } >> + >> + arr = devm_kzalloc(dev, count * sizeof(*arr), GFP_KERNEL); > > Better to use devm_kcalloc here Ok. > >> + if (!arr) { >> + ret = -ENOMEM; >> + goto out; > > Just return -ENOMEM Ok. > >> + } >> + >> + ret = of_property_read_u32_array(np, prop_name, arr, count); >> + if (ret) { >> + dev_err(dev, "%s Invalid dt array property, err(%d)\n", >> + prop_name, ret); >> + goto out; > > Just return ret Ok. > >> + } >> + *table = arr; >> + *size = count; >> +out: >> + return ret; > > Then the label 'out:' is not needed, nor is the initialization of 'ret' and > here just return 0. Ok. > >> +} >> + >> +static struct sdhci_msm_pltfm_data *sdhci_msm_populate_pdata(struct device *dev, >> + struct sdhci_msm_host *msm_host) >> +{ >> + struct sdhci_msm_pltfm_data *pdata = NULL; >> + size_t table_sz = 0; > > table_sz is an 'int' elsewhere. To keep it all the same type make it an > 'int' here too. Ok. > >> + u32 *table = NULL; >> + >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + goto out; > > Here and below just return NULL and then the label out: is not needed. Alright. > >> + >> + if (sdhci_msm_dt_get_array(dev, "clk-rates", &table, &table_sz)) { >> + dev_err(dev, "failed in DT parsing for supported clk-rates\n"); >> + goto out; >> + } >> + if (!table || !table_sz) { >> + dev_err(dev, "Invalid clock table\n"); >> + goto out; >> + } >> + pdata->clk_table = table; >> + pdata->clk_table_sz = table_sz; >> + >> + return pdata; >> +out: >> + return NULL; >> +} >> + >> static int sdhci_msm_probe(struct platform_device *pdev) >> { >> struct sdhci_host *host; >> @@ -608,6 +675,10 @@ static int sdhci_msm_probe(struct platform_device *pdev) >> >> sdhci_get_of_property(pdev); >> >> + msm_host->pdata = sdhci_msm_populate_pdata(&pdev->dev, msm_host); >> + if (!msm_host->pdata) >> + goto pltfm_free; >> + >> /* Setup SDCC bus voter clock. */ >> msm_host->bus_clk = devm_clk_get(&pdev->dev, "bus"); >> if (!IS_ERR(msm_host->bus_clk)) { >> >