From: Javier Martinez Canillas <javier.martinez@collabora.co.uk> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: Kukjin Kim <kgene@kernel.org>, Doug Anderson <dianders@chromium.org>, Olof Johansson <olof@lixom.net>, Arend van Spriel <arend@broadcom.com>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, linux-samsung-soc <linux-samsung-soc@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, linux-mmc <linux-mmc@vger.kernel.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2 4/6] mmc: pwrseq_simple: Add optional reference clock support Date: Thu, 29 Jan 2015 14:53:10 +0100 [thread overview] Message-ID: <54CA3B46.20204@collabora.co.uk> (raw) In-Reply-To: <CAPDyKFommNu7oscgmhwMpT-5KYN9b6d1xvt513_0qWE=HEGbow@mail.gmail.com> Hello Ulf, Thanks a lot for your feedback. On 01/29/2015 02:05 PM, Ulf Hansson wrote: >> >> struct mmc_pwrseq_simple { >> struct mmc_pwrseq pwrseq; >> + struct clk *ext_clk; > > You need to add a bool, maybe call it clk_enabled; See why below. > Ok >> int nr_gpios; >> struct gpio_desc *reset_gpios[0]; >> }; >> @@ -39,6 +41,9 @@ static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host) >> struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq, >> struct mmc_pwrseq_simple, pwrseq); >> >> + if (!IS_ERR(pwrseq->ext_clk)) >> + clk_prepare_enable(pwrseq->ext_clk); >> + > > There are no guarantee that the ->mmc_pwrseq_simple_pre_power_on() > will be invoked prior ->mmc_pwrseq_simple_power_off(). > Got it, I didn't know that mmc_pwrseq_simple_power_off() could be invoked. without mmc_pwrseq_simple_pre_power_on() not being called before. > That means you need to keep track of if you have gated/ungated the > clock. In other words check pwrseq->clk_enabled. That will prevent > potential clk unbalance issues. Yes, I'll change to check for the boolean in _simple_power_off() and _post_power_on() then. >> @@ -85,6 +104,14 @@ int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev) >> if (!pwrseq) >> return -ENOMEM; >> >> + pwrseq->ext_clk = clk_get(dev, "ext_clock"); >> + if (IS_ERR(pwrseq->ext_clk) && >> + PTR_ERR(pwrseq->ext_clk) != -ENOENT && >> + PTR_ERR(pwrseq->ext_clk) != -ENOSYS) { > > I don't think you can get -ENOSYS. > You are right, I'll remove that. Best regards, Javier
WARNING: multiple messages have this Message-ID (diff)
From: javier.martinez@collabora.co.uk (Javier Martinez Canillas) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 4/6] mmc: pwrseq_simple: Add optional reference clock support Date: Thu, 29 Jan 2015 14:53:10 +0100 [thread overview] Message-ID: <54CA3B46.20204@collabora.co.uk> (raw) In-Reply-To: <CAPDyKFommNu7oscgmhwMpT-5KYN9b6d1xvt513_0qWE=HEGbow@mail.gmail.com> Hello Ulf, Thanks a lot for your feedback. On 01/29/2015 02:05 PM, Ulf Hansson wrote: >> >> struct mmc_pwrseq_simple { >> struct mmc_pwrseq pwrseq; >> + struct clk *ext_clk; > > You need to add a bool, maybe call it clk_enabled; See why below. > Ok >> int nr_gpios; >> struct gpio_desc *reset_gpios[0]; >> }; >> @@ -39,6 +41,9 @@ static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host) >> struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq, >> struct mmc_pwrseq_simple, pwrseq); >> >> + if (!IS_ERR(pwrseq->ext_clk)) >> + clk_prepare_enable(pwrseq->ext_clk); >> + > > There are no guarantee that the ->mmc_pwrseq_simple_pre_power_on() > will be invoked prior ->mmc_pwrseq_simple_power_off(). > Got it, I didn't know that mmc_pwrseq_simple_power_off() could be invoked. without mmc_pwrseq_simple_pre_power_on() not being called before. > That means you need to keep track of if you have gated/ungated the > clock. In other words check pwrseq->clk_enabled. That will prevent > potential clk unbalance issues. Yes, I'll change to check for the boolean in _simple_power_off() and _post_power_on() then. >> @@ -85,6 +104,14 @@ int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev) >> if (!pwrseq) >> return -ENOMEM; >> >> + pwrseq->ext_clk = clk_get(dev, "ext_clock"); >> + if (IS_ERR(pwrseq->ext_clk) && >> + PTR_ERR(pwrseq->ext_clk) != -ENOENT && >> + PTR_ERR(pwrseq->ext_clk) != -ENOSYS) { > > I don't think you can get -ENOSYS. > You are right, I'll remove that. Best regards, Javier
next prev parent reply other threads:[~2015-01-29 13:53 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-01-28 18:15 [PATCH v2 0/6] Add multiple GPIO and external clock to MMC pwrseq_simple Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` [PATCH v2 1/6] mmc: pwrseq: Document that simple sequence support more than one GPIO Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` [PATCH v2 2/6] mmc: pwrseq_simple: Extend to support more pins Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` [PATCH v2 3/6] mmc: pwrseq: Document optional clock for the simple power sequence Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` [PATCH v2 4/6] mmc: pwrseq_simple: Add optional reference clock support Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-29 13:05 ` Ulf Hansson 2015-01-29 13:05 ` Ulf Hansson 2015-01-29 13:05 ` Ulf Hansson 2015-01-29 13:53 ` Javier Martinez Canillas [this message] 2015-01-29 13:53 ` Javier Martinez Canillas 2015-01-29 13:53 ` Javier Martinez Canillas 2015-01-28 18:15 ` [PATCH v2 5/6] ARM: dts: exynos5250-snow: Enable wifi power-on Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` [PATCH v2 6/6] ARM: dts: exynos5250-snow: Add cap-sdio-irq to wifi mmc node Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas 2015-01-28 18:15 ` Javier Martinez Canillas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=54CA3B46.20204@collabora.co.uk \ --to=javier.martinez@collabora.co.uk \ --cc=arend@broadcom.com \ --cc=devicetree@vger.kernel.org \ --cc=dianders@chromium.org \ --cc=kgene@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mmc@vger.kernel.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=olof@lixom.net \ --cc=srinivas.kandagatla@linaro.org \ --cc=ulf.hansson@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.