Re: [PATCH v7 08/14] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm

[Stephen Boyd <sboyd@codeaurora.org>]

On 11/15, Ritesh Harjani wrote:
> On 11/15/2016 1:07 AM, Stephen Boyd wrote:
> >On 11/14, Ritesh Harjani wrote:
> >
> >>+}
> >>+
> >>+/* sdhci_msm_set_clock - Called with (host->lock) spinlock held. */
> >>+static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
> >>+{
> >>+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >>+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> >>+	int rc;
> >>+
> >>+	if (!clock) {
> >>+		msm_host->clk_rate = clock;
> >>+		goto out;
> >>+	}
> >>+
> >>+	spin_unlock_irq(&host->lock);
> >>+	if (clock != msm_host->clk_rate) {
> >
> >Why do we need to check here? Can't we call clk_set_rate()
> >Unconditionally?
> Since it may so happen that above layers may call for ->set_clock
> function with same requested clock more than once, hence we cache
> the host->clock here.
> Also, since requested clock (host->clock) can be say 400Mhz but the
> actual pltfm supported clock would be say 384MHz.

clk_set_rate() detects the same rate being set even after it
internally rounds the rate. We're not going to touch the clk
hardware if 400 is requested once but 384 is what's set and then
400 is requested again. Caching the rate here in the driver can
lead to problems too if the driver is out of sync with the clk
hardware state, so it's best to avoid doing anything fancy here
and just let the framework handle duplicates.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project