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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
