[PATCH 1/3] mmc: dw_mmc: use mmc_regulator_get_supply to handle regulators

[Doug Anderson]

Jaehoon,

On Mon, Jun 30, 2014 at 5:13 AM, Jaehoon Chung <jh80.chung at samsung.com> wrote:
> On 06/27/2014 01:18 AM, Doug Anderson wrote:
>> Yuvaraj,
>>
>> On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar <yuvaraj.cd at gmail.com> wrote:
>>> Doug
>>>
>>> On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson <dianders at chromium.org> wrote:
>>>> Yuvaraj,
>>>>
>>>> On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd at gmail.com> wrote:
>>>>> This patch makes use of mmc_regulator_get_supply() to handle
>>>>> the vmmc and vqmmc regulators.Also it moves the code handling
>>>>> the these regulators to dw_mci_set_ios().It turned on the vmmc
>>>>> and vqmmc during MMC_POWER_UP and MMC_POWER_ON,and turned off
>>>>> during MMC_POWER_OFF.
>>>>>
>>>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd at samsung.com>
>>>>> ---
>>>>>  drivers/mmc/host/dw_mmc.c |   71 ++++++++++++++++++++++-----------------------
>>>>>  drivers/mmc/host/dw_mmc.h |    2 ++
>>>>>  2 files changed, 36 insertions(+), 37 deletions(-)
>>>>
>>>> Perhaps you could CC me on the whole series for the next version since
>>>> I was involved in privately reviewing previous versions?
>>> It was just accidental missing you in the CC .Surely i will add you in
>>> CC for next versions.
>>>>
>>>> Overall caveat for my review is that I'm nowhere near an SD/MMC expert.
>>>>
>>>>
>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>> index 1ac227c..f5cabce 100644
>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>> @@ -937,6 +937,7 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>>>>         struct dw_mci_slot *slot = mmc_priv(mmc);
>>>>>         const struct dw_mci_drv_data *drv_data = slot->host->drv_data;
>>>>>         u32 regs;
>>>>> +       int ret;
>>>>>
>>>>>         switch (ios->bus_width) {
>>>>>         case MMC_BUS_WIDTH_4:
>>>>> @@ -975,16 +976,41 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>>>>
>>>>>         switch (ios->power_mode) {
>>>>>         case MMC_POWER_UP:
>>>>> +               if ((!IS_ERR(mmc->supply.vmmc)) &&
>>>>> +                               !test_bit(DW_MMC_CARD_POWERED, &slot->flags)) {
>>>>> +                       ret = regulator_enable(mmc->supply.vmmc);
>>>>> +                       if (!ret)
>>>>> +                               set_bit(DW_MMC_CARD_POWERED, &slot->flags);
>>>>> +               }
>>>>
>>>> As per below, I'm not sure why you'd want to turn on vqmmc and vmmc at
>>>> different times.
>>> As you can see people's have different opinion on this.When i had a
>>> look at the other drivers in the subsystem which does in the same flow
>>> as above.However i will change in the next version.
>>
>> Given my self proclaimed lack of SD/MMC knowledge, if others have a
>> good reason for doing them separate then you should do it that way.
>> So far I haven't heard that reason but I certainly could be wrong.
>
> At first time, i had believed nothing problem that it turns on vmmc and vqmmc at different time.
> It could have the potential problem.

OK.  As I said I'm nowhere near an expert on SD/MMC, so if there's
something out there that says that turning vmmc on before vqmmc is the
right thing to do then I guess that's the answer.  I would still be
very curious to get more details on what the potential problem is.
Could you provide a reference to documentation that says vmmc should
be on before vqmmc or describe a situation where it's important?

Thanks!

-Doug