linux-kernel - Re: [PATCH 2/2] mmc: dw_mmc: Handle wp-gpios from device tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAD=FV=W9injFAP+Qx9Ge9zK8DxwuWBCPHc4w4Jyn3PKuX_3vkg@mail.gmail.com>
Date:	Wed, 21 Nov 2012 20:43:57 -0800
From:	Doug Anderson <dianders@...omium.org>
To:	Jaehoon Chung <jh80.chung@...sung.com>
Cc:	linux-samsung-soc@...r.kernel.org,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Olof Johansson <olof@...om.net>, Arnd Bergmann <arnd@...db.de>,
	Will Newton <will.newton@...tec.com>,
	Chris Ball <cjb@...top.org>,
	Seungwon Jeon <tgih.jun@...sung.com>,
	linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: 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 <jh80.chung@...sung.com> 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 <dianders@...omium.org>
>> ---
>>  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 <linux/regulator/consumer.h>
>>  #include <linux/workqueue.h>
>>  #include <linux/of.h>
>> +#include <linux/of_gpio.h>
>>
>>  #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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/