On 06/27/2014 01:18 AM, Doug Anderson wrote:
> 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.
At first time, i had believed nothing problem that it turns on vmmc and vqmmc at different time.
It could have the potential problem.
Best Regards,
Jaehoon Chung
>
>
>>>> @@ -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-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
