Messages in this thread | | | Date | Tue, 13 May 2014 10:39:14 +0100 | From | Srinivas Kandagatla <> | Subject | Re: [PATCH v1 09/11] mmc: mmci: Add clock support for Qualcomm. |
| |
Thanks Linus W,
On 13/05/14 09:28, Linus Walleij wrote: >> code is conditioned on hw designer. >> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > (...) >> + if (host->hw_designer == AMBA_VENDOR_QCOM) { >> + host->cclk = host->mclk; >> + } else if (desired >= host->mclk) { > > Again refrain from hard-checking the vendor everywhere. > > struct variant_data { > bool qcom_cclk_is_mclk; > (...) > Got it.. Will fix it in next version. > As per example from st_clkdiv... > > Then > > if (host->vendor->qcom_cclk_is_mclk) { > (...) > } > >> + if (ios->clock != host->mclk && >> + host->hw_designer == AMBA_VENDOR_QCOM) { >> + /* Qcom MCLKCLK register does not define bypass bits */ >> + int rc = clk_set_rate(host->clk, ios->clock); >> + if (rc < 0) { >> + dev_err(mmc_dev(host->mmc), >> + "Error setting clock rate (%d)\n", rc); >> + } else { >> + host->mclk = clk_get_rate(host->clk); >> + host->cclk = host->mclk; >> + } >> + } > > For this I would define a vendor data like: > > struct variant_data { > bool explicit_mclk_control; > (...) This looks good. > > Or something. It explains what is actually going on. > >> if (plat->f_max) >> - mmc->f_max = min(host->mclk, plat->f_max); >> + mmc->f_max = (host->hw_designer == AMBA_VENDOR_QCOM) ? >> + plat->f_max : min(host->mclk, plat->f_max); > > So rewrite like that: > > if (host->vendor->explicit_mclk_control) > mmc->f_max = plat->f_max; > else > mmc->f_max = min(host->mclk, plat->f_max); >
This looks much clean
> Yours, > Linus Walleij >
| |