Yuvaraj,
On Thu, Jun 26, 2014 at 4:21 AM, Yuvaraj Kumar <yuvaraj.cd@xxxxxxxxx> wrote:
> Doug
>
> On Tue, Jun 24, 2014 at 11:30 PM, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>> Yuvaraj,
>>
>> On Mon, Jun 23, 2014 at 3:45 AM, Yuvaraj Kumar C D <yuvaraj.cd@xxxxxxxxx> 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@xxxxxxxxxxx>
>>> ---
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
