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

[Doug Anderson]

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.


>>> @@ -225,6 +225,8 @@ struct dw_mci_slot {
>>>         unsigned long           flags;
>>>  #define DW_MMC_CARD_PRESENT    0
>>>  #define DW_MMC_CARD_NEED_INIT  1
>>> +#define DW_MMC_CARD_POWERED    2
>>> +#define DW_MMC_IO_POWERED      3
>>
>> I don't really think you should have two bits here.  From my
>> understanding of SD cards there should be very little reason to have
>> vqmmc and vmmc not powered at the same time.
> I think if i can use mmc_regulator_set_ocr(), we don't need additional
> flag.But for tps65090 mmc_regulator_get_ocr() and
> mmc_regulator_set_ocr() is failing as its a fixed-regulator.

Can you explain more about what's failing?  It sure looks like mmc
core is supposed to handle this given comments below

/*
* If we're using a fixed/static regulator, don't call
* regulator_set_voltage; it would fail.
*/
voltage = regulator_get_voltage(supply);

if (!regulator_can_change_voltage(supply))
  min_uV = max_uV = voltage;


-Doug