Re: [PATCH v2 1/2] mmc: dw_mmc: exynos: Stop claiming wp-gpio

Doug Anderson
2012-11-28T18:20:10Z

Seungwon,

Thanks for the review. See below for comments. If you'd like me to
respin then please let me know. Otherwise I look forward to your ack.

On Wed, Nov 28, 2012 at 1:29 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> Yes. pin of write protection is common property.
> This change is good. I have some suggestion below.
> Could you check it?
>
> On Friday, November 23, 2012, Doug Anderson wrote:
>> The exynos code claimed wp-gpio with devm_gpio_request() but never did
>> anything with it. That meant that anyone using a write protect GPIO
>> would effectively be write protected all the time.
>>
>> A future change will move the wp-gpio support to the core dw_mmc.c
>> file. Now the exynos-specific code won't claim the GPIO but will
>> just set the DW_MCI_QUIRK_NO_WRITE_PROTECT quirk if write protect
>> won't be used.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>>
>> ---
>> Changes in v2:
>> - Nothing new in this patch
>>
>> drivers/mmc/host/dw_mmc-exynos.c | 12 ++++++------
>> 1 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
>> index 4d50da6..58cc03e 100644
>> --- a/drivers/mmc/host/dw_mmc-exynos.c
>> +++ b/drivers/mmc/host/dw_mmc-exynos.c
>> @@ -175,12 +175,12 @@ static int dw_mci_exynos_setup_bus(struct dw_mci *host,
>> }
>> }
>>
>> - gpio = of_get_named_gpio(slot_np, "wp-gpios", 0);
>> - if (gpio_is_valid(gpio)) {
>> - if (devm_gpio_request(host->dev, gpio, "dw-mci-wp"))
>> - dev_info(host->dev, "gpio [%d] request failed\n",
>> - gpio);
>> - } else {
>> + /*
>> + * If there are no write-protect GPIOs present then we assume no write
>> + * protect. The mci_readl() in dw_mmc.c won't work since it's not
>> + * hooked up on exynos.
>> + */
>> + if (!of_find_property(slot_np, "wp-gpios", NULL)) {
>> dev_info(host->dev, "wp gpio not available");
>> host->pdata->quirks |= DW_MCI_QUIRK_NO_WRITE_PROTECT;
>> }
> All card types need this quirk in case wp-gpio property is empty?
> I think wp-pin is valid for SD card, not eMMC/SDIO.

Right. It is only checked right now by the SD code (mmc/core/sd.c).
It doesn't particularly hurt to set it the quirk in other cases though
and it seems nice not to add special cases. I could imagine someone
extending the MMC code at some point to support write protect (via
GPIO) for eMMC, so there's even a slight justification for avoiding
the special case.

> Of course, I know origin code did it.
> How about removing whole checking routine?
> Instead, new definition for this quirk can be added into 'dw_mci_of_quirks'(dw_mmc.c) and dts file.

On _exynos_ all SD cards need this quirk if there is no wp-gpio
property. However this is not generally true for all users of dw_mmc.
The DesignWare IP Block actually has a write protect input that can
be read with "mci_readl(slot->host, WRTPRT)" but on exynos the
DesignWare write protect line isn't exposed on any physical pins.
That means that the only possible way to do write protect on exynos is
using a GPIO.

The above means that on exynos if the GPIO isn't defined we will
assume no write protect. On other platforms if the GPIO isn't defined
we'll assume that the "mci_readl" will work and we'll use that.

If people would prefer it I can code up an alternate solution that
doesn't touch any exynos code but that would introduce a new device
tree binding. We could accomplish what's needed for exynos using a
property like "broken-internal-wp".

Please let me know if you'd like me to submit a new patch with this
solution or if you like the existing solution.

> Thanks,
> Seungwon Jeon
>> --
>> 1.7.7.3
>