From: Ulf Hansson <ulf.hansson@stericsson.com>
To: Saugata Das <saugata.das@linaro.org>
Cc: Subhash Jadavani <subhashj@codeaurora.org>,
Girish K S <girish.shivananjappa@linaro.org>,
"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
"patches@linaro.org" <patches@linaro.org>
Subject: Re: [PATCH v3 1/2] MMC-4.5 Power OFF Notify rework
Date: Mon, 14 May 2012 15:26:20 +0200 [thread overview]
Message-ID: <4FB107FC.8010508@stericsson.com> (raw)
In-Reply-To: <CAKLKtzf6VyfR7x3TbJLLRN8CNtiiDVHh-W3DB4R-512XBc8M_w@mail.gmail.com>
On 05/14/2012 02:49 PM, Saugata Das wrote:
> On 14 May 2012 16:45, Ulf Hansson<ulf.hansson@stericsson.com> wrote:
>> Hi Girish,
>>
>> Please some comments below.
>>
>>
>> On 05/11/2012 01:44 PM, Subhash Jadavani wrote:
>>>
>>> Hi Girish,
>>>
>>> On 5/7/2012 7:11 PM, Girish K S wrote:
>>>>
>>>> This is a rework of the existing POWER OFF NOTIFY patch. The current
>>>> problem
>>>> with the patch comes from the ambiguity on the usage of POWER OFF NOTIFY
>>>> together with SLEEP and misunderstanding on the usage of MMC_POWER_OFF
>>>> power_mode from mmc_set_ios in different host controller drivers.
>>>>
>>>> This new patch works around this problem by adding a new host CAP,
>>>> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND, which when set sends a
>>>> POWER OFF NOTIFY from mmc_suspend instead of SLEEP. It is expected that
>>>> host
>>>> controller drivers will set this CAP, if they switch off both Vcc and
>>>> Vccq
>>>> from MMC_POWER_OFF condition within mmc_set_ios. However, note that there
>>>> is no harm in sending MMC_POWER_NOTIFY even if Vccq is not switched off.
>>>>
>>>> This patch also sends POWER OFF NOTIFY from power management routines
>>>> (e.g.
>>>> mmc_power_save_host, mmc_pm_notify/PM_SUSPEND_PREPARE, mmc_stop_host),
>>>> which
>>>> does reinitialization of the eMMC on the return path of the power
>>>> management
>>>> routines (e.g. mmc_power_restore_host, mmc_pm_notify/PM_POST_RESTORE,
>>>> mmc_start_host).
>>>>
>>>> This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent
>>>> from
>>>> the suspend sequence. If it is sent from shutdown sequence then it is set
>>>> to
>>>> POWER_OFF_LONG.
>>>>
>>>> Previuos implementation of PowerOff Notify as a core function is replaced
>>>> as
>>>> a device's bus operation.
>>>>
>>>> Signed-off-by: Saugata Das<saugata.das@linaro.org>
>>>> Signed-off-by: Girish K S<girish.shivananjappa@linaro.org>
>>>>
>>>> changes in v3:
>>>> This version addresses the review comments given by Subhash and Ulf
>>>> changes in v2:
>>>> This version addresses the changes suggested by Ulf
>>>> ---
>>>> drivers/mmc/core/core.c | 98
>>>> +++++++++++++++-------------------------------
>>>> drivers/mmc/core/core.h | 1 +
>>>> drivers/mmc/core/mmc.c | 73 ++++++++++++++++++++++++++++------
>>>> include/linux/mmc/card.h | 8 +++-
>>>> include/linux/mmc/host.h | 9 ++--
>>>> 5 files changed, 105 insertions(+), 84 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index ba821fe..3db3b32 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -1100,48 +1100,6 @@ void mmc_set_driver_type(struct mmc_host *host,
>>>> unsigned int drv_type)
>>>> mmc_host_clk_release(host);
>>>> }
>>>>
>>>> -static void mmc_poweroff_notify(struct mmc_host *host)
>>>> -{
>>>> - struct mmc_card *card;
>>>> - unsigned int timeout;
>>>> - unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
>>>> - int err = 0;
>>>> -
>>>> - card = host->card;
>>>> - mmc_claim_host(host);
>>>> -
>>>> - /*
>>>> - * Send power notify command only if card
>>>> - * is mmc and notify state is powered ON
>>>> - */
>>>> - if (card&& mmc_card_mmc(card)&&
>>>> - (card->poweroff_notify_state == MMC_POWERED_ON)) {
>>>> -
>>>> - if (host->power_notify_type == MMC_HOST_PW_NOTIFY_SHORT) {
>>>> - notify_type = EXT_CSD_POWER_OFF_SHORT;
>>>> - timeout = card->ext_csd.generic_cmd6_time;
>>>> - card->poweroff_notify_state = MMC_POWEROFF_SHORT;
>>>> - } else {
>>>> - notify_type = EXT_CSD_POWER_OFF_LONG;
>>>> - timeout = card->ext_csd.power_off_longtime;
>>>> - card->poweroff_notify_state = MMC_POWEROFF_LONG;
>>>> - }
>>>> -
>>>> - err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>> - EXT_CSD_POWER_OFF_NOTIFICATION,
>>>> - notify_type, timeout);
>>>> -
>>>> - if (err&& err != -EBADMSG)
>>>> - pr_err("Device failed to respond within %d poweroff
>>>> "
>>>> - "time. Forcefully powering down the
>>>> device\n",
>>>> - timeout);
>>>> -
>>>> - /* Set the card state to no notification after the poweroff
>>>> */
>>>> - card->poweroff_notify_state = MMC_NO_POWER_NOTIFICATION;
>>>> - }
>>>> - mmc_release_host(host);
>>>> -}
>>>> -
>>>> /*
>>>> * Apply power to the MMC stack. This is a two-stage process.
>>>> * First, we enable power to the card without the clock running.
>>>> @@ -1198,30 +1156,12 @@ static void mmc_power_up(struct mmc_host *host)
>>>>
>>>> void mmc_power_off(struct mmc_host *host)
>>>> {
>>>> - int err = 0;
>>>> mmc_host_clk_hold(host);
>>>>
>>>> host->ios.clock = 0;
>>>> host->ios.vdd = 0;
>>>>
>>>> /*
>>>> - * For eMMC 4.5 device send AWAKE command before
>>>> - * POWER_OFF_NOTIFY command, because in sleep state
>>>> - * eMMC 4.5 devices respond to only RESET and AWAKE cmd
>>>> - */
>>>> - if (host->card&& mmc_card_is_sleep(host->card)&&
>>>> - host->bus_ops->resume) {
>>>> - err = host->bus_ops->resume(host);
>>>> -
>>>> - if (!err)
>>>> - mmc_poweroff_notify(host);
>>>> - else
>>>> - pr_warning("%s: error %d during resume "
>>>> - "(continue with poweroff sequence)\n",
>>>> - mmc_hostname(host), err);
>>>> - }
>>>> -
>>>> - /*
>>>> * Reset ocr mask to be the highest possible voltage supported for
>>>> * this mmc host. This value will be used at next power up.
>>>> */
>>>> @@ -2081,9 +2021,16 @@ void mmc_stop_host(struct mmc_host *host)
>>>>
>>>> /* clear pm flags now and let card drivers set them as needed */
>>>> host->pm_flags = 0;
>>>> -
>>>> mmc_bus_get(host);
>>>> if (host->bus_ops&& !host->bus_dead) {
>>>> +
>>>> + if (host->bus_ops->poweroff_notify) {
>>
>>
>> Instead of checking EOPNOTSUP you can do:
>> if (mmc_card_can_poweroff_notify ..)
>>
>
> If I understood your suggestion well, you want to remove
> mmc_card_can_poweroff_notify check from inside mmc_poweroff_notify and
> move it outside here before calling poweroff_notify. It can be done.
>
OK, great!
>>
>>
>>>> + int err = host->bus_ops->poweroff_notify(host);
>>>> + if (err&& err != -EOPNOTSUPP)
>>>> + pr_info("%s: error [%d] in poweroff
>>>> notify\n",
>>>> + mmc_hostname(host), err);
>>>> + }
>>>> +
>>>> /* Calling bus_ops->remove() with a claimed host can
>>>> deadlock */
>>>> if (host->bus_ops->remove)
>>>> host->bus_ops->remove(host);
>>>> @@ -2093,6 +2040,7 @@ void mmc_stop_host(struct mmc_host *host)
>>>> mmc_power_off(host);
>>>> mmc_release_host(host);
>>>> mmc_bus_put(host);
>>>> +
>>
>>
>> white space
>>
>
> Ok
>
>>
>>>> return;
>>>> }
>>>> mmc_bus_put(host);
>>>> @@ -2119,6 +2067,13 @@ int mmc_power_save_host(struct mmc_host *host)
>>>>
>>>> if (host->bus_ops->power_save)
>>>> ret = host->bus_ops->power_save(host);
>>>> + host->poweroff_notify_type = MMC_HOST_PW_OFF_NOTIFY_SHORT;
>>>
>>> Before sending this short power off notification, shouldn't we make sure
>>> that host has defined MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND cap?
>>
>>
>> Instead of checking EOPNOTSUP you can do:
>> if (mmc_card_can_poweroff_notify ..)
>
> Ok (Same way as understood above)
>
>>
>> Moreover MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND shall be checked during
>> mmc_init_card, when setting poweroff_notify_state to MMC_POWERED_ON, that is
>> not done today.
>>
>
> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND is only linked when putting
> eMMC to SLEEP state. We need to handle scenario, where the platform
> goes to power OFF before putting eMMC to SLEEP (e.g. mmc_stop_host).
Yes, of course you are right! So no changes shall be done to
"mmc_init_card" then.
> So, MMC_POWERED_ON is linked to only MMC_CAP2_POWEROFF_NOTIFY and
> right initial value of poweroff_notify_type as set from host drivers
> (i.e. poweroff_notify_type should not be MMC_HOST_PW_NOTIFY_NONE).
>
In that context, that sound perfectly fine!
>
>> Thereafter you can only do this when checking if poweroff_notify shall be
>> done:
>> "if (mmc_card_can_poweroff_notify ..)"
>>
>
> Ok
>
>>
>>>> + if (host->bus_ops->poweroff_notify) {
>>>> + int err = host->bus_ops->poweroff_notify(host);
>>>> + if (err&& err != -EOPNOTSUPP)
>>>> + pr_info("%s: error [%d] in poweroff notify\n",
>>>> + mmc_hostname(host), err);
>>>> + }
>>>>
>>>> mmc_bus_put(host);
>>>>
>>>> @@ -2142,7 +2097,6 @@ int mmc_power_restore_host(struct mmc_host *host)
>>>> mmc_bus_put(host);
>>>> return -EINVAL;
>>>> }
>>>> -
more white space
>>>> mmc_power_up(host);
>>>> ret = host->bus_ops->power_restore(host);
>>>>
>>>> @@ -2161,8 +2115,11 @@ int mmc_card_awake(struct mmc_host *host)
>>>>
>>>> mmc_bus_get(host);
>>>>
>>>> - if (host->bus_ops&& !host->bus_dead&& host->bus_ops->awake)
>>>> + if (host->bus_ops&& !host->bus_dead&& host->bus_ops->awake) {
>>>> err = host->bus_ops->awake(host);
>>>> + if (!err)
>>>> + mmc_card_clr_sleep(host->card);
>>>> + }
>>>>
>>>> mmc_bus_put(host);
>>>>
>>>> @@ -2179,8 +2136,11 @@ int mmc_card_sleep(struct mmc_host *host)
>>>>
>>>> mmc_bus_get(host);
>>>>
>>>> - if (host->bus_ops&& !host->bus_dead&& host->bus_ops->sleep)
>>>> + if (host->bus_ops&& !host->bus_dead&& host->bus_ops->sleep) {
>>>> err = host->bus_ops->sleep(host);
>>>> + if (!err)
>>>> + mmc_card_set_sleep(host->card);
>>>> + }
>>>>
>>>> mmc_bus_put(host);
>>>>
>>>> @@ -2373,12 +2333,18 @@ int mmc_pm_notify(struct notifier_block
>>>> *notify_block,
>>>>
>>>> spin_lock_irqsave(&host->lock, flags);
>>>> host->rescan_disable = 1;
>>>> - host->power_notify_type = MMC_HOST_PW_NOTIFY_SHORT;
>>>> + host->poweroff_notify_type = MMC_HOST_PW_OFF_NOTIFY_SHORT;
>>>
>>> Before sending this short power off notification, shouldn't we make sure
>>> that host has defined MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND cap?
>>
>> Suggest to remove the "poweroff_notify_type" variable in the host. Use an
>> in-parameter to the poweroff_notify bus_ops function instead.
>>
>
> This is coming from the original version of the patch. Need to check
> the reason for this and fix it, if needed.
Realize that, but it would make scene to fix this in patch as well. If
feasible of course.
>
>>
>>>> spin_unlock_irqrestore(&host->lock, flags);
>>>> cancel_delayed_work_sync(&host->detect);
>>>>
>>>> if (!host->bus_ops || host->bus_ops->suspend)
>>>> break;
>>
>>
>> You can do this instead:
>> "if (mmc_card_can_poweroff_notify ..)"
>>
>
> Ok
>
>>>> + if (host->bus_ops->poweroff_notify) {
>>>> + int err = host->bus_ops->poweroff_notify(host);
>>>> + if (err&& err != -EOPNOTSUPP)
>>>> + pr_info("%s: error [%d] in poweroff
>>>> notify\n",
>>>> + mmc_hostname(host), err);
>>>> + }
>>>>
>>>> /* Calling bus_ops->remove() with a claimed host can
>>>> deadlock */
>>>> if (host->bus_ops->remove)
>>>> @@ -2397,7 +2363,7 @@ int mmc_pm_notify(struct notifier_block
>>>> *notify_block,
>>>>
>>>> spin_lock_irqsave(&host->lock, flags);
>>>> host->rescan_disable = 0;
>>>> - host->power_notify_type = MMC_HOST_PW_NOTIFY_LONG;
>>>> + host->poweroff_notify_type = MMC_HOST_PW_OFF_NOTIFY_LONG;
>>
>>
>> Suggest to remove the "poweroff_notify_type" variable in the host. Use an
>> in-parameter to the poweroff_notify bus_ops function instead.
>>
>
> Same as above. Need to check the reason of having this in the original patch.
>
>>
>>>> spin_unlock_irqrestore(&host->lock, flags);
>>>> mmc_detect_change(host, 0);
>>>>
>>>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>>>> index 3bdafbc..351cbbe 100644
>>>> --- a/drivers/mmc/core/core.h
>>>> +++ b/drivers/mmc/core/core.h
>>>> @@ -25,6 +25,7 @@ struct mmc_bus_ops {
>>>> int (*power_save)(struct mmc_host *);
>>>> int (*power_restore)(struct mmc_host *);
>>>> int (*alive)(struct mmc_host *);
>>>> + int (*poweroff_notify)(struct mmc_host *);
>>>> };
>>>>
>>>> void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops
>>>> *ops);
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>> index 54df5ad..a86e5f8 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -1282,6 +1282,47 @@ err:
>>>> return err;
>>>> }
>>>>
>>>> +static int mmc_poweroff_notify(struct mmc_host *host)
>>
>>
>> Suggest to add a "poweroff_notify_type" as in-parameter instead of using the
>> host struct for storing this information.
>>
>
> Same as above. Need to check the reason of having this in the original patch.
>
>>
>>>> +{
>>>> + struct mmc_card *card;
>>>> + unsigned int timeout;
>>>> + unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
>>>> + int err = -EOPNOTSUPP;
>>>> +
>>>> + card = host->card;
>>>> +
>>>> + /*
>>>> + * Send power notify command only if card
>>>> + * is mmc and notify state is powered ON
>>>> + */
>>>> + if (mmc_card_can_poweroff_notify(host->card)) {
>>
>>
>> Suggest to move this check outside of this function. Please see how this is
>> done for mmc_card_can_sleep and mmc_card_sleep.
>>
>
> Ok
>
>>
>>>> + if (host->poweroff_notify_type ==
>>>> + MMC_HOST_PW_OFF_NOTIFY_SHORT) {
>>>> + notify_type = EXT_CSD_POWER_OFF_SHORT;
>>>> + timeout = card->ext_csd.generic_cmd6_time;
>>>> + } else {
>>>
>>> poweroff_notify_type can take 3 different values.
>>> #define MMC_HOST_PW_OFF_NOTIFY_NONE 0
>>> #define MMC_HOST_PW_OFF_NOTIFY_SHORT 1
>>> #define MMC_HOST_PW_OFF_NOTIFY_LONG 2
>>>
>>> So shouldn't we have explicit check for NOTIFY_LONG?
>>> else if (notify_type == LONG) { ... }
>>> else {error printing?}
>>>
>>>> + notify_type = EXT_CSD_POWER_OFF_LONG;
>>>> + timeout = card->ext_csd.power_off_longtime;
>>>> + }
>>>> +
>>>> + mmc_claim_host(host);
>>
>>
>> Suggest to handle claim and release outside this function. Similar how
>> mmc_card_sleep is implemented.
>>
>
> Ok
>
>>
>>>> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>>> + EXT_CSD_POWER_OFF_NOTIFICATION,
>>>> + notify_type, timeout);
>>>> + mmc_release_host(host);
>>>> +
>>>> + if (err&& err != -EBADMSG)
>>>> + pr_err("%s: Device failed to respond within %d "
>>>> + "poweroff time. Forcefully powering down "
>>>> + "the device\n", mmc_hostname(host),
>>>> timeout);
>>>> + else
>>>> + card->poweroff_notify_state =
>>>> + MMC_NO_POWER_NOTIFICATION;
>>>
>>> Just wondering, if we should really to set the notify_state to
>>> NO_POWER_NOTIFICATION incase if err = -EBADMSG ?
>>>>
>>>> + }
>>>> +
>>>> + return err;
>>>> +}
>>>> +
>>>> /*
>>>> * Host is being removed. Free up the current card.
>>>> */
>>>> @@ -1341,15 +1382,21 @@ static int mmc_suspend(struct mmc_host *host)
>>>> BUG_ON(!host);
>>>> BUG_ON(!host->card);
>>>>
>>>> - mmc_claim_host(host);
>>
>>
>> Suggest to handle claim and release as before and not inside the
>> mmc_poweroff_notify function. Similar how mmc_card_sleep is implemented.
>>
>
> Ok
>
>>
>>>> - if (mmc_card_can_sleep(host)) {
>>>> - err = mmc_card_sleep(host);
>>>> - if (!err)
>>>> - mmc_card_set_sleep(host->card);
>>>> - } else if (!mmc_host_is_spi(host))
>>>> - mmc_deselect_cards(host);
>>>> - host->card->state&= ~(MMC_STATE_HIGHSPEED |
>>>> MMC_STATE_HIGHSPEED_200);
>>>> - mmc_release_host(host);
>>>> + if (mmc_card_can_poweroff_notify(host->card)&&
>>>> + (host->caps2& MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
>>
>> Sorry for not proposing this earlier;
>> By checking the MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND could be done only
>> from mmc_init_card function, it is enough to check
>> "mmc_card_can_poweroff_notify".
>>
>
> MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND is only linked for the special
> feature of powering off eMMC during platform suspend (instead of
> putting eMMC to SLEEP). It has no relation to the power OFF notify
> feature which is done when platform is shutting down (e.g.
> mmc_stop_host). So, mmc_init_card just checks
> MMC_CAP2_POWEROFF_NOTIFY.
Agree, you are right. MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND needs to be
explicitly checked here, as done.
>
>
>>
>>>> + err = mmc_poweroff_notify(host);
>>>> + } else {
>>>> + mmc_claim_host(host);
>>>> + if (mmc_card_can_sleep(host))
>>>> + err = mmc_card_sleep(host);
>>>> + else if (!mmc_host_is_spi(host))
>>>> + mmc_deselect_cards(host);
>>>> + mmc_release_host(host);
>>>> + }
>>>> +
>>>> + if (!err)
>>>> + host->card->state&=
>>>> + ~(MMC_STATE_HIGHSPEED | MMC_STATE_HIGHSPEED_200);
>>>>
>>>> return err;
>>>> }
>>>> @@ -1368,11 +1415,11 @@ static int mmc_resume(struct mmc_host *host)
>>>> BUG_ON(!host->card);
>>>>
>>>> mmc_claim_host(host);
>>>> - if (mmc_card_is_sleep(host->card)) {
>>>> + if (mmc_card_is_sleep(host->card))
>>>> err = mmc_card_awake(host);
>>>> - mmc_card_clr_sleep(host->card);
>>>> - } else
>>>> + else
>>>> err = mmc_init_card(host, host->ocr, host->card);
>>>> +
>>>> mmc_release_host(host);
>>>>
>>>> return err;
>>>> @@ -1430,6 +1477,7 @@ static const struct mmc_bus_ops mmc_ops = {
>>>> .resume = NULL,
>>>> .power_restore = mmc_power_restore,
>>>> .alive = mmc_alive,
>>>> + .poweroff_notify = mmc_poweroff_notify,
>>>> };
>>>>
>>>> static const struct mmc_bus_ops mmc_ops_unsafe = {
>>>> @@ -1441,6 +1489,7 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>>>> .resume = mmc_resume,
>>>> .power_restore = mmc_power_restore,
>>>> .alive = mmc_alive,
>>>> + .poweroff_notify = mmc_poweroff_notify,
>>>> };
>>>>
>>>> static void mmc_attach_bus_ops(struct mmc_host *host)
>>>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>>>> index 629b823..a6cdc43 100644
>>>> --- a/include/linux/mmc/card.h
>>>> +++ b/include/linux/mmc/card.h
>>>> @@ -238,8 +238,6 @@ struct mmc_card {
>>>> unsigned int poweroff_notify_state; /* eMMC4.5 notify feature
>>>> */
>>>> #define MMC_NO_POWER_NOTIFICATION 0
>>>> #define MMC_POWERED_ON 1
>>>> -#define MMC_POWEROFF_SHORT 2
>>>> -#define MMC_POWEROFF_LONG 3
>>>>
>>>> unsigned int erase_size; /* erase size in sectors */
>>>> unsigned int erase_shift; /* if erase unit is power 2
>>>> */
>>>> @@ -465,6 +463,12 @@ static inline int mmc_card_long_read_time(const
>>>> struct mmc_card *c)
>>>> return c->quirks& MMC_QUIRK_LONG_READ_TIME;
>>>> }
>>>>
>>>> +static inline int mmc_card_can_poweroff_notify(const struct mmc_card *c)
>>>> +{
>>>> + return c&& mmc_card_mmc(c)&&
>>>> + (c->poweroff_notify_state ==
>>>> MMC_POWERED_ON);
>>>> +}
>>>> +
>>>
>>> I guess you may want to move this function in drivers/mmc/core/core.c
>>> along with other mmc_can_* function in same file. Look for
>>> mmc_can_sanitize() or mmc_can_discard() functions as example.
>>>
>>>> #define mmc_card_name(c) ((c)->cid.prod_name)
>>>> #define mmc_card_id(c) (dev_name(&(c)->dev))
>>>>
>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>> index 0707d22..aad894e 100644
>>>> --- a/include/linux/mmc/host.h
>>>> +++ b/include/linux/mmc/host.h
>>>> @@ -238,12 +238,13 @@ struct mmc_host {
>>>> #define MMC_CAP2_BROKEN_VOLTAGE (1<< 7) /* Use the broken
>>>> voltage */
>>>> #define MMC_CAP2_DETECT_ON_ERR (1<< 8) /* On I/O err
>>>> check card removal */
>>>> #define MMC_CAP2_HC_ERASE_SZ (1<< 9) /* High-capacity
>>>> erase size */
>>>> +#define MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND (1<< 10)
>>>>
>>>> mmc_pm_flag_t pm_caps; /* supported pm features */
>>>> - unsigned int power_notify_type;
>>>> -#define MMC_HOST_PW_NOTIFY_NONE 0
>>>> -#define MMC_HOST_PW_NOTIFY_SHORT 1
>>>> -#define MMC_HOST_PW_NOTIFY_LONG 2
>>
>>
>> Suggest to remove this entirely from the mmc host struct.
>>
>
> As stated above, we need to check the reason of having this in the
> original patch. Then, we will either remove this and add an argument
> to mmc_poweroff_notify, or keep it as it is with some justification.
>
>
>>
>>>> + unsigned int poweroff_notify_type;
>>>
>>> Can you add additional tab to make poweroff_notify_state aligned to
>>> other members in this same structure?
>>>>
>>>> +#define MMC_HOST_PW_OFF_NOTIFY_NONE 0
>>>> +#define MMC_HOST_PW_OFF_NOTIFY_SHORT 1
>>>> +#define MMC_HOST_PW_OFF_NOTIFY_LONG 2
>>>>
>>>> #ifdef CONFIG_MMC_CLKGATE
>>>> int clk_requests; /* internal reference
>>>> counter */
>>>
>>>
>>
>> Kind regards
>> Ulf Hansson
Thanks!
Ulf Hansson
prev parent reply other threads:[~2012-05-14 13:26 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-05-07 13:41 [PATCH v3 1/2] MMC-4.5 Power OFF Notify rework Girish K S
2012-05-07 13:41 ` [PATCH v3 2/2] mmc4.5: host: poweroff notify rework Girish K S
2012-05-14 7:20 ` Ulf Hansson
2012-05-14 8:56 ` Girish K S
2012-05-14 9:00 ` Girish K S
2012-05-11 11:44 ` [PATCH v3 1/2] MMC-4.5 Power OFF Notify rework Subhash Jadavani
2012-05-14 6:32 ` Saugata Das
2012-05-14 8:49 ` Ulf Hansson
2012-05-14 9:48 ` Saugata Das
2012-05-14 11:15 ` Ulf Hansson
2012-05-14 12:49 ` Saugata Das
2012-05-14 13:26 ` Ulf Hansson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4FB107FC.8010508@stericsson.com \
--to=ulf.hansson@stericsson.com \
--cc=girish.shivananjappa@linaro.org \
--cc=linux-mmc@vger.kernel.org \
--cc=patches@linaro.org \
--cc=saugata.das@linaro.org \
--cc=subhashj@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox