On Fri, Sep 28, 2012 at 06:28:31PM +0800, yongd wrote:
> In the current code logic, sdhci_add_host() will enable the polling
> method (set MMC_CAP_NEEDS_POLL) for a removable card (MMC_CAP_
> NONREMOVABLE is not set) whose host's internal card detection method
> is disabled for some reason (SDHCI_QUIRK_BROKEN_CARD_DETECTION is set).
>
> However, this is improper since we can have some other card detection
> methods besides host internal card detection and polling method. For
> example, if the card detection type is ESDHC_CD_GPIO (external gpio pin
> for CD), we will keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set. This is right.
> But, just as above said, sdhci_add_host() will also enable polling for such
> a card. This is redundant.
>
At least for sdhci-esdhc-imx, SDHCI_QUIRK_BROKEN_CARD_DETECTION will
be set only when neither ESDHC_CD_GPIO nor ESDHC_CD_CONTROLLER works.
Shawn
> On the other hand, for the card with ESDHC_CD_NONE detection type(no CD, neither
> controller nor gpio, so use polling), we currently rely on sdhci_add_host() to
> enable polling for us.
>
> Here proposed a solution for such an embarrassing case. 1st, this patch will
> de-couple polling enabling with sdhci_add_host() by doing this in host driver
> itself, just as some other vendors. 2nd, one more patch will remove such improper
> MMC_CAP_NEEDS_POLL enabling in sdhci_add_host().
>
> Change-Id: Ia7525009d8fd188e3f0169f225e4a76ff9e94b47
> Signed-off-by: yongd <[email protected]>
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index e23f813..f70079c 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -569,6 +569,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
> break;
>
> case ESDHC_CD_NONE:
> + host->mmc->caps = MMC_CAP_NEEDS_POLL;
> break;
> }
>
> --
> 1.7.9.5
>
Shawn,
Thanks for your comment. And sorry for my so late due to illness:-)
SDHCI_QUIRK_BROKEN_CARD_DETECTION is used for notifying we don't use the host internal card detection method so that we don't need enable/disable those relevant interrupt bits of host(sdhci_set_card_detection in sdhci.c).
And as I double-checked the latest kernel code, actually sdhci-esdhc-imx sets this flag by default, and then will clear it only when the detection type is ESDHC_CD_CONTROLLER. So this aligns with my understanding.
What I want to do is that 1st we shall set MMC_CAP_NEEDS_POLL by our host driver itself and 2nd remove the improper logic in sdhci_add_host() . How do u think? Thanks.
________________________________________
From: Shawn Guo [[email protected]]
Sent: Sunday, October 07, 2012 22:06
To: Yong Ding
Cc: Chris Ball; Anton Vorontsov; Marek Szyprowski; Wolfram Sang; Daniel Drake; Sascha Hauer; Wilson Callan; Ben Dooks; Zhangfei Gao; Kevin Liu; Jialing Fu; [email protected]; [email protected]
Subject: Re: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself
On Fri, Sep 28, 2012 at 06:28:31PM +0800, yongd wrote:
> In the current code logic, sdhci_add_host() will enable the polling
> method (set MMC_CAP_NEEDS_POLL) for a removable card (MMC_CAP_
> NONREMOVABLE is not set) whose host's internal card detection method
> is disabled for some reason (SDHCI_QUIRK_BROKEN_CARD_DETECTION is set).
>
> However, this is improper since we can have some other card detection
> methods besides host internal card detection and polling method. For
> example, if the card detection type is ESDHC_CD_GPIO (external gpio pin
> for CD), we will keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set. This is right.
> But, just as above said, sdhci_add_host() will also enable polling for such
> a card. This is redundant.
>
At least for sdhci-esdhc-imx, SDHCI_QUIRK_BROKEN_CARD_DETECTION will
be set only when neither ESDHC_CD_GPIO nor ESDHC_CD_CONTROLLER works.
Shawn
> On the other hand, for the card with ESDHC_CD_NONE detection type(no CD, neither
> controller nor gpio, so use polling), we currently rely on sdhci_add_host() to
> enable polling for us.
>
> Here proposed a solution for such an embarrassing case. 1st, this patch will
> de-couple polling enabling with sdhci_add_host() by doing this in host driver
> itself, just as some other vendors. 2nd, one more patch will remove such improper
> MMC_CAP_NEEDS_POLL enabling in sdhci_add_host().
>
> Change-Id: Ia7525009d8fd188e3f0169f225e4a76ff9e94b47
> Signed-off-by: yongd <[email protected]>
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index e23f813..f70079c 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -569,6 +569,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
> break;
>
> case ESDHC_CD_NONE:
> + host->mmc->caps = MMC_CAP_NEEDS_POLL;
> break;
> }
>
> --
> 1.7.9.5
>
On Tue, Oct 16, 2012 at 09:01:40PM -0700, Yong Ding wrote:
> Shawn,
> Thanks for your comment. And sorry for my so late due to illness:-)
> SDHCI_QUIRK_BROKEN_CARD_DETECTION is used for notifying we don't use the host internal card detection method so that we don't need enable/disable those relevant interrupt bits of host(sdhci_set_card_detection in sdhci.c).
> And as I double-checked the latest kernel code, actually sdhci-esdhc-imx sets this flag by default, and then will clear it only when the detection type is ESDHC_CD_CONTROLLER. So this aligns with my understanding.
What I was saying is the bit will be cleared when the detection type is
ESDHC_CD_CONTROLLE or ESDHC_CD_GPIO. You may have missed the fact that
there is no "break" in case ESDHC_CD_GPIO but a "fall through" comment.
switch (boarddata->cd_type) {
case ESDHC_CD_GPIO:
err = gpio_request_one(boarddata->cd_gpio, GPIOF_IN, "ESDHC_CD");
if (err) {
dev_err(mmc_dev(host->mmc),
"no card-detect pin available!\n");
goto no_card_detect_pin;
}
err = request_irq(gpio_to_irq(boarddata->cd_gpio), cd_irq,
IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
mmc_hostname(host->mmc), host);
if (err) {
dev_err(mmc_dev(host->mmc), "request irq error\n");
goto no_card_detect_irq;
}
/* fall through */
case ESDHC_CD_CONTROLLER:
/* we have a working card_detect back */
host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
break;
case ESDHC_CD_PERMANENT:
host->mmc->caps = MMC_CAP_NONREMOVABLE;
break;
case ESDHC_CD_NONE:
break;
}
Shawn
> What I want to do is that 1st we shall set MMC_CAP_NEEDS_POLL by our host driver itself and 2nd remove the improper logic in sdhci_add_host() . How do u think? Thanks.
>
Shawn,
Thanks. Oh, sorry I really have missed the fact u mentioned. U are right in the current code, the bit will also be cleared for ESDHC_CD_GPIO.
But I think this is improper since for GPIO detection type, we don't use the host controller internal card detection(ESDHC_CD_CONTROLLER), but with SDHCI_QUIRK_BROKEN_CARD_DETECTION cleared, we'll still enable/disable relevant INT bits (in sdhci_set_card_detection in sdhci.c). This is my biggest concern. And I think the SDHCI_QUIRK_BROKEN_CARD_DETECTION shall be purely used to notify whether the host controller detection method is used or not. So even for the ESDHC_CD_GPIO type, we should still set this flag. How do u think?
-----Original Message-----
From: Shawn Guo [mailto:[email protected]]
Sent: 2012??10??18?? 13:51
To: Yong Ding
Cc: Chris Ball; Anton Vorontsov; Marek Szyprowski; Wolfram Sang; Daniel Drake; Sascha Hauer; Wilson Callan; Ben Dooks; Zhangfei Gao; Kevin Liu; Jialing Fu; [email protected]; [email protected]
Subject: Re: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself
On Tue, Oct 16, 2012 at 09:01:40PM -0700, Yong Ding wrote:
> Shawn,
> Thanks for your comment. And sorry for my so late due to illness:-)
> SDHCI_QUIRK_BROKEN_CARD_DETECTION is used for notifying we don't use the host internal card detection method so that we don't need enable/disable those relevant interrupt bits of host(sdhci_set_card_detection in sdhci.c).
> And as I double-checked the latest kernel code, actually sdhci-esdhc-imx sets this flag by default, and then will clear it only when the detection type is ESDHC_CD_CONTROLLER. So this aligns with my understanding.
What I was saying is the bit will be cleared when the detection type is
ESDHC_CD_CONTROLLE or ESDHC_CD_GPIO. You may have missed the fact that
there is no "break" in case ESDHC_CD_GPIO but a "fall through" comment.
switch (boarddata->cd_type) {
case ESDHC_CD_GPIO:
err = gpio_request_one(boarddata->cd_gpio, GPIOF_IN, "ESDHC_CD");
if (err) {
dev_err(mmc_dev(host->mmc),
"no card-detect pin available!\n");
goto no_card_detect_pin;
}
err = request_irq(gpio_to_irq(boarddata->cd_gpio), cd_irq,
IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
mmc_hostname(host->mmc), host);
if (err) {
dev_err(mmc_dev(host->mmc), "request irq error\n");
goto no_card_detect_irq;
}
/* fall through */
case ESDHC_CD_CONTROLLER:
/* we have a working card_detect back */
host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
break;
case ESDHC_CD_PERMANENT:
host->mmc->caps = MMC_CAP_NONREMOVABLE;
break;
case ESDHC_CD_NONE:
break;
}
Shawn
> What I want to do is that 1st we shall set MMC_CAP_NEEDS_POLL by our host driver itself and 2nd remove the improper logic in sdhci_add_host() . How do u think? Thanks.
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Can you fix your mailer to use bottom posting rather than top posting,
and have texts wrap around column 70? Otherwise, you message stands
a good chance to be ignored by people.
On Wed, Oct 17, 2012 at 11:27:17PM -0700, Yong Ding wrote:
> Shawn,
> Thanks. Oh, sorry I really have missed the fact u mentioned. U are right in the current code, the bit will also be cleared for ESDHC_CD_GPIO.
> But I think this is improper since for GPIO detection type, we don't use the host controller internal card detection(ESDHC_CD_CONTROLLER), but with SDHCI_QUIRK_BROKEN_CARD_DETECTION cleared, we'll still enable/disable relevant INT bits (in sdhci_set_card_detection in sdhci.c). This is my biggest concern. And I think the SDHCI_QUIRK_BROKEN_CARD_DETECTION shall be purely used to notify whether the host controller detection method is used or not. So even for the ESDHC_CD_GPIO type, we should still set this flag. How do u think?
>
I'm fine with that. Just remind you a fact you seem missed in case
you need to change sdhci-esdhc-imx.c to adapt the changes you are going
to make on sdhci.c.
Shawn
Shawn,
Thanks, I will update the patch for sdhci-esdhc-imx.c and also fix my mail ASAP:-)
-----Original Message-----
From: Shawn Guo [mailto:[email protected]]
Sent: 2012??10??18?? 14:38
To: Yong Ding
Cc: Chris Ball; Anton Vorontsov; Marek Szyprowski; Wolfram Sang; Daniel Drake; Sascha Hauer; Wilson Callan; Ben Dooks; Zhangfei Gao; Kevin Liu; Jialing Fu; [email protected]; [email protected]
Subject: Re: [PATCH 1/3] mmc: esdhc: enable polling to detect card by itself
Can you fix your mailer to use bottom posting rather than top posting,
and have texts wrap around column 70? Otherwise, you message stands
a good chance to be ignored by people.
On Wed, Oct 17, 2012 at 11:27:17PM -0700, Yong Ding wrote:
> Shawn,
> Thanks. Oh, sorry I really have missed the fact u mentioned. U are right in the current code, the bit will also be cleared for ESDHC_CD_GPIO.
> But I think this is improper since for GPIO detection type, we don't use the host controller internal card detection(ESDHC_CD_CONTROLLER), but with SDHCI_QUIRK_BROKEN_CARD_DETECTION cleared, we'll still enable/disable relevant INT bits (in sdhci_set_card_detection in sdhci.c). This is my biggest concern. And I think the SDHCI_QUIRK_BROKEN_CARD_DETECTION shall be purely used to notify whether the host controller detection method is used or not. So even for the ESDHC_CD_GPIO type, we should still set this flag. How do u think?
>
I'm fine with that. Just remind you a fact you seem missed in case
you need to change sdhci-esdhc-imx.c to adapt the changes you are going
to make on sdhci.c.
Shawn
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?