Re: [PATCH v2 4/6] mmc: pwrseq_simple: Add optional reference clock support

Javier Martinez Canillas
2015-01-29T13:53:10Z

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