Re: [PATCH 2/2] mmc: dw_mmc: Handle wp-gpios from device tree
Jaehoon,
Thanks for the review. See below for comments. I'll plan on a new
patch either Monday or Tuesday when I have a chance to spin and
re-test.
On Wed, Nov 21, 2012 at 5:55 PM, Jaehoon Chung wrote:
> On 11/22/2012 07:03 AM, Doug Anderson wrote:
>> On some SoCs (like exynos5250) you need to use an external GPIO for
>> write protect. Add support for wp-gpios to the core dw_mmc driver
>> since it could be useful across multiple SoCs.
>>
>> With this change I am able to make use of the write protect for the
>> external SD slot on exynos5250-snow.
>>
>> Signed-off-by: Doug Anderson
>> ---
>> drivers/mmc/host/dw_mmc.c | 35 +++
>> 1 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 5b41348..9c79870 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -34,6 +34,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "dw_mmc.h"
>>
>> @@ -74,6 +75,7 @@ struct idmac_desc {
>> * struct dw_mci_slot - MMC slot state
>> * @mmc: The mmc_host representing this slot.
>> * @host: The MMC controller this slot is using.
>> + * @wp_gpio: If gpio_is_valid() we'll use this to read write protect.
>> * @ctype: Card type for this slot.
>> * @mrq: mmc_request currently being processed or waiting to be
>> * processed, or NULL when the slot is idle.
>> @@ -88,6 +90,8 @@ struct dw_mci_slot {
>> struct mmc_host *mmc;
>> struct dw_mci *host;
>>
>> + int wp_gpio;
>> +
>> u32 ctype;
>>
>> struct mmc_request *mrq;
>> @@ -832,6 +836,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
>> read_only = 0;
>> else if (brd->get_ro)
>> read_only = brd->get_ro(slot->id);
>> + else if (gpio_is_valid(slot->wp_gpio))
>> + read_only = gpio_get_value(slot->wp_gpio);
>> else
>> read_only =
>> mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1 :
>> 0;
>> @@ -1802,6 +1808,29 @@ static u32 dw_mci_of_get_bus_wd(struct device *dev,
>> u8 slot)
>> " as 1\n");
>> return bus_wd;
>> }
>> +
>> +/* find the write protect gpio for a given slot; or -1 if none specified */
>> +static u32 dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
>> +{
>> + struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
>> + int gpio;
>> +
>> + if (!np)
>> + return -1;
> I think good that use the error number instead of -1. Also the below code.
In this case it's not really returning an error code which is why I
chose -1. It's returning a gpio number or anything that is a sentinel
value indicating that the GPIO is not valid.
...but you're right, an error code would work. I'll replace with
"-EINVAL" in my next patch.
>
>> +
>> + gpio = of_get_named_gpio(np, "wp-gpios", 0);
>> +
>> + /* Having a missing entry is valid; return silently */
>> + if (!gpio_is_valid(gpio))
>> + return -1;
>> +
>> + if (devm_gpio_request(dev, gpio, "dw-mci-wp")) {
>> + dev_warn(dev, "gpio [%d] request failed\n", gpio);
>> + return -1;
>> + }
>> +
>> + return gpio;
> gpio is int type, but return type is u32?
Good catch. Will fix.
>
> Best Regards,
> Jaehoon Chung
>> +}
>> #else /* CONFIG_OF */
>> static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
>> {
>> @@ -1811,6 +1840,10 @@ static struct device_node
>> *dw_mci_of_find_slot_node(struct device *dev, u8 slot)
>> {
>> return NULL;
>> }
>> +static u32 dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
>> +{
>> + return -1;
>> +}
>> #endif /* CONFIG_OF */
>>
>> static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
>> @@ -1923,6 +1956,8 @@ static int dw_mci_init_slot(struct dw_mci *host,
>> unsigned int id)
>> else
>> clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>>
>> + slot->wp_gpio = dw_mci_of_get_wp_gpio(host->dev, slot->id);
>> +
>> mmc_add_host(mmc);
>>
>> #if defined(CONFIG_DEBUG_FS)
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] mmc: dw_mmc: Handle wp-gpios from device tree
On Wed, Nov 21, 2012 at 5:42 PM, Seungwon Jeon wrote:
> Hi,
>
> wp-gpios has been implemented in dw_mmc-exynos.c
> It can be reused for EXYNOS platform? We need to modify some though.
Yup, I've seen that. Patch 1/2 ("mmc: dw_mmc: exynos: Stop claiming
wp-gpio") addressed that. For some reason I can't find that on
LKML.org yet. Strange. :-/ I'll forward it on to you shortly.
In any case: I found that the exynos code didn't actually work. It
claimed the GPIO but didn't ever look at it.
I have the beginnings of the code to implement this properly in the
exynos code but I stopped working on it when I decided that other SoCs
could also benefit from the code and it fit better in the general
dw_mmc driver.
If you disagree and would like me to cleanup the version of this patch
that's just in the exynos driver and post that, I will. Just let me
know.
Thanks for the review!
-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] mmc: dw_mmc: Handle wp-gpios from device tree
On 11/22/2012 07:03 AM, Doug Anderson wrote:
> On some SoCs (like exynos5250) you need to use an external GPIO for
> write protect. Add support for wp-gpios to the core dw_mmc driver
> since it could be useful across multiple SoCs.
>
> With this change I am able to make use of the write protect for the
> external SD slot on exynos5250-snow.
>
> Signed-off-by: Doug Anderson
> ---
> drivers/mmc/host/dw_mmc.c | 35 +++
> 1 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 5b41348..9c79870 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -34,6 +34,7 @@
> #include
> #include
> #include
> +#include
>
> #include "dw_mmc.h"
>
> @@ -74,6 +75,7 @@ struct idmac_desc {
> * struct dw_mci_slot - MMC slot state
> * @mmc: The mmc_host representing this slot.
> * @host: The MMC controller this slot is using.
> + * @wp_gpio: If gpio_is_valid() we'll use this to read write protect.
> * @ctype: Card type for this slot.
> * @mrq: mmc_request currently being processed or waiting to be
> * processed, or NULL when the slot is idle.
> @@ -88,6 +90,8 @@ struct dw_mci_slot {
> struct mmc_host *mmc;
> struct dw_mci *host;
>
> + int wp_gpio;
> +
> u32 ctype;
>
> struct mmc_request *mrq;
> @@ -832,6 +836,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
> read_only = 0;
> else if (brd->get_ro)
> read_only = brd->get_ro(slot->id);
> + else if (gpio_is_valid(slot->wp_gpio))
> + read_only = gpio_get_value(slot->wp_gpio);
> else
> read_only =
> mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1 : 0;
> @@ -1802,6 +1808,29 @@ static u32 dw_mci_of_get_bus_wd(struct device *dev, u8
> slot)
> " as 1\n");
> return bus_wd;
> }
> +
> +/* find the write protect gpio for a given slot; or -1 if none specified */
> +static u32 dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
> +{
> + struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
> + int gpio;
> +
> + if (!np)
> + return -1;
I think good that use the error number instead of -1. Also the below code.
> +
> + gpio = of_get_named_gpio(np, "wp-gpios", 0);
> +
> + /* Having a missing entry is valid; return silently */
> + if (!gpio_is_valid(gpio))
> + return -1;
> +
> + if (devm_gpio_request(dev, gpio, "dw-mci-wp")) {
> + dev_warn(dev, "gpio [%d] request failed\n", gpio);
> + return -1;
> + }
> +
> + return gpio;
gpio is int type, but return type is u32?
Best Regards,
Jaehoon Chung
> +}
> #else /* CONFIG_OF */
> static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
> {
> @@ -1811,6 +1840,10 @@ static struct device_node
> *dw_mci_of_find_slot_node(struct device *dev, u8 slot)
> {
> return NULL;
> }
> +static u32 dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
> +{
> + return -1;
> +}
> #endif /* CONFIG_OF */
>
> static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> @@ -1923,6 +1956,8 @@ static int dw_mci_init_slot(struct dw_mci *host,
> unsigned int id)
> else
> clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>
> + slot->wp_gpio = dw_mci_of_get_wp_gpio(host->dev, slot->id);
> +
> mmc_add_host(mmc);
>
> #if defined(CONFIG_DEBUG_FS)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] mmc: dw_mmc: Handle wp-gpios from device tree
On Thursday, November 22, 2012, Doug Anderson wrote:
> On Wed, Nov 21, 2012 at 5:42 PM, Seungwon Jeon wrote:
> > Hi,
> >
> > wp-gpios has been implemented in dw_mmc-exynos.c
> > It can be reused for EXYNOS platform? We need to modify some though.
>
> Yup, I've seen that. Patch 1/2 ("mmc: dw_mmc: exynos: Stop claiming
> wp-gpio") addressed that. For some reason I can't find that on
> LKML.org yet. Strange. :-/ I'll forward it on to you shortly.
>
> In any case: I found that the exynos code didn't actually work. It
> claimed the GPIO but didn't ever look at it.
>
> I have the beginnings of the code to implement this properly in the
> exynos code but I stopped working on it when I decided that other SoCs
> could also benefit from the code and it fit better in the general
> dw_mmc driver.
>
> If you disagree and would like me to cleanup the version of this patch
> that's just in the exynos driver and post that, I will. Just let me
> know.
Yes, origin code of dw_mmc-exynos didn't work fine. We need to modify it.
Anyway, some problem in mailing? I didn't get 1/2 of patch.
In addition, it's not found in any mail-archive. After resolved, I can review.
Thanks,
Seungwon Jeon
>
>
> Thanks for the review!
>
> -Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] mmc: dw_mmc: Handle wp-gpios from device tree
Hi,
wp-gpios has been implemented in dw_mmc-exynos.c
It can be reused for EXYNOS platform? We need to modify some though.
Thanks,
Seungwon Jeon
On Thursday, November 22, 2012, Doug Anderson wrote:
> On some SoCs (like exynos5250) you need to use an external GPIO for
> write protect. Add support for wp-gpios to the core dw_mmc driver
> since it could be useful across multiple SoCs.
>
> With this change I am able to make use of the write protect for the
> external SD slot on exynos5250-snow.
>
> Signed-off-by: Doug Anderson
> ---
> drivers/mmc/host/dw_mmc.c | 35 +++
> 1 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 5b41348..9c79870 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -34,6 +34,7 @@
> #include
> #include
> #include
> +#include
>
> #include "dw_mmc.h"
>
> @@ -74,6 +75,7 @@ struct idmac_desc {
> * struct dw_mci_slot - MMC slot state
> * @mmc: The mmc_host representing this slot.
> * @host: The MMC controller this slot is using.
> + * @wp_gpio: If gpio_is_valid() we'll use this to read write protect.
> * @ctype: Card type for this slot.
> * @mrq: mmc_request currently being processed or waiting to be
> * processed, or NULL when the slot is idle.
> @@ -88,6 +90,8 @@ struct dw_mci_slot {
> struct mmc_host *mmc;
> struct dw_mci *host;
>
> + int wp_gpio;
> +
> u32 ctype;
>
> struct mmc_request *mrq;
> @@ -832,6 +836,8 @@ static int dw_mci_get_ro(struct mmc_host *mmc)
> read_only = 0;
> else if (brd->get_ro)
> read_only = brd->get_ro(slot->id);
> + else if (gpio_is_valid(slot->wp_gpio))
> + read_only = gpio_get_value(slot->wp_gpio);
> else
> read_only =
> mci_readl(slot->host, WRTPRT) & (1 << slot->id) ? 1 : 0;
> @@ -1802,6 +1808,29 @@ static u32 dw_mci_of_get_bus_wd(struct device *dev, u8
> slot)
> " as 1\n");
> return bus_wd;
> }
> +
> +/* find the write protect gpio for a given slot; or -1 if none specified */
> +static u32 dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
> +{
> + struct device_node *np = dw_mci_of_find_slot_node(dev, slot);
> + int gpio;
> +
> + if (!np)
> + return -1;
> +
> + gpio = of_get_named_gpio(np, "wp-gpios", 0);
> +
> + /* Having a missing entry is valid; return silently */
> + if (!gpio_is_valid(gpio))
> + return -1;
> +
> + if (devm_gpio_request(dev, gpio, "dw-mci-wp")) {
> + dev_warn(dev, "gpio [%d] request failed\n", gpio);
> + return -1;
> + }
> +
> + return gpio;
> +}
> #else /* CONFIG_OF */
> static u32 dw_mci_of_get_bus_wd(struct device *dev, u8 slot)
> {
> @@ -1811,6 +1840,10 @@ static struct device_node
> *dw_mci_of_find_slot_node(struct device *dev, u8 slot)
> {
> return NULL;
> }
> +static u32 dw_mci_of_get_wp_gpio(struct device *dev, u8 slot)
> +{
> + return -1;
> +}
> #endif /* CONFIG_OF */
>
> static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> @@ -1923,6 +1956,8 @@ static int dw_mci_init_slot(struct dw_mci *host,
> unsigned int id)
> else
> clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
>
> + slot->wp_gpio = dw_mci_of_get_wp_gpio(host->dev, slot->id);
> +
> mmc_add_host(mmc);
>
> #if defined(CONFIG_DEBUG_FS)
> --
> 1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
