Re: [PATCH 2/5] mmc: pwrseq_simple: Extend to support more pins
From: Srinivas Kandagatla
Date: Wed Jan 28 2015 - 15:41:57 EST
On 28/01/15 16:13, Javier Martinez Canillas wrote:
Hello Srinivas,
Thanks a lot for your feedback.
On 01/28/2015 03:01 PM, Srinivas Kandagatla wrote:
Hi Javier,
You are in a lead of 3 hrs from me..
Surprisingly I send very much same patch just few Mins ago :-)
:-)
I didn't find the posted patch you are referring too though, did you cc
linux-mmc?
May be we can merge goods in both :-)
Sure, I want $subject to be generic enough to be useful for other platforms.
On 28/01/15 10:10, Javier Martinez Canillas wrote:
Many WLAN attached to a SDIO/MMC interface, needs more than one pin for
their reset sequence. For example, is very common for chips to have two
pins: one for reset and one for power enable.
This patch adds support for more reset pins to the pwrseq_simple driver
and instead hardcoding a fixed number, it uses the of_gpio_named_count()
since the MMC power sequence is only built when CONFIG_OF is enabled.
Signed-off-by: Javier Martinez Canillas <javier.martinez@xxxxxxxxxxxxxxx>
---
drivers/mmc/core/pwrseq_simple.c | 54 ++++++++++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 13 deletions(-)
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 0958c696137f..9e51fe1051c5 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -11,6 +11,7 @@
#include <linux/slab.h>
#include <linux/device.h>
#include <linux/err.h>
+#include <linux/of_gpio.h>
#include <linux/gpio/consumer.h>
#include <linux/mmc/host.h>
@@ -19,34 +20,44 @@
struct mmc_pwrseq_simple {
struct mmc_pwrseq pwrseq;
- struct gpio_desc *reset_gpio;
+ struct gpio_desc **reset_gpio;
May be renaming it to reset_gpios makes more sense..
Ok
If you make this struct gpio_desc *reset_gpios[0]; You can aviod an
extra kmalloc and free ..
That's a very good idea, thanks.
+ int nr_gpios;
};
static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
{
[...
struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
struct mmc_pwrseq_simple, pwrseq);
+ int i;
- if (!IS_ERR(pwrseq->reset_gpio))
- gpiod_set_value_cansleep(pwrseq->reset_gpio, 1);
+ for (i = 0; i < pwrseq->nr_gpios; i++)
+ if (!IS_ERR(pwrseq->reset_gpio[i]))
+ gpiod_set_value_cansleep(pwrseq->reset_gpio[i], 1);
...]
}
static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
{
[...
struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
struct mmc_pwrseq_simple, pwrseq);
+ int i;
- if (!IS_ERR(pwrseq->reset_gpio))
- gpiod_set_value_cansleep(pwrseq->reset_gpio, 0);
+ for (i = 0; i < pwrseq->nr_gpios; i++)
+ if (!IS_ERR(pwrseq->reset_gpio[i]))
+ gpiod_set_value_cansleep(pwrseq->reset_gpio[i], 0);
...]
Now that we have more code in mmc_pwrseq_simple_post_power_on() and
mmc_pwrseq_simple_pre_power_on(), Just move most of them into a common
function like:
static void __mmc_pwrseq_simple_power_on_off(struct mmc_host *host,
bool on)
{
struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
struct mmc_pwrseq_simple, pwrseq);
int i;
if (!IS_ERR(pwrseq->reset_gpios)) {
for (i = 0; i < pwrseq->ngpios; i++)
gpiod_set_value_cansleep(pwrseq->reset_gpios[i],
on ? : 0);
}
}
static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
{
__mmc_pwrseq_simple_power_on_off(host, true);
}
static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
{
__mmc_pwrseq_simple_power_on_off(host, false);
}
Sure, will do.
}
static void mmc_pwrseq_simple_free(struct mmc_host *host)
{
struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
struct mmc_pwrseq_simple, pwrseq);
+ int i;
- if (!IS_ERR(pwrseq->reset_gpio))
- gpiod_put(pwrseq->reset_gpio);
+ if (pwrseq->nr_gpios > 0) {
+ for (i = 0; i < pwrseq->nr_gpios; i++)
+ if (!IS_ERR(pwrseq->reset_gpio[i]))
+ gpiod_put(pwrseq->reset_gpio[i]);
+ kfree(pwrseq->reset_gpio);
+ }
kfree(pwrseq);
host->pwrseq = NULL;
@@ -63,17 +74,27 @@ int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev)
{
struct mmc_pwrseq_simple *pwrseq;
int ret = 0;
+ int i;
pwrseq = kzalloc(sizeof(struct mmc_pwrseq_simple), GFP_KERNEL);
if (!pwrseq)
return -ENOMEM;
- pwrseq->reset_gpio = gpiod_get_index(dev, "reset", 0, GPIOD_OUT_HIGH);
- if (IS_ERR(pwrseq->reset_gpio) &&
- PTR_ERR(pwrseq->reset_gpio) != -ENOENT &&
- PTR_ERR(pwrseq->reset_gpio) != -ENOSYS) {
- ret = PTR_ERR(pwrseq->reset_gpio);
- goto free;
+ pwrseq->nr_gpios = of_gpio_named_count(dev->of_node, "reset-gpios");
+ if (pwrseq->nr_gpios > 0) {
What happens if there are no gpios? This fuction should return -ENOENT
and should not even try to allocate pwrseq?
Not quite, the DT binding states that the GPIOs are optional so it should
not fail if no GPIOs are defined.
Probably you should do of_gpio_named_count before allocating memory.
I didn't do that because patch #4 "mmc: pwrseq_simple: Add optional reference
clock support" will need the struct mmc_pwrseq_simple even if no GPIOs are
defined.
A SDIO attached chip could require only an external clock or someone could
extend the pwrseq_simple driver to support an external regulator for example.
That makes sense.
+ pwrseq->reset_gpio = kzalloc(sizeof(struct gpio_desc *) *
+ pwrseq->nr_gpios, GFP_KERNEL);
+
+ for (i = 0; i < pwrseq->nr_gpios; i++) {
+ pwrseq->reset_gpio[i] = gpiod_get_index(dev, "reset", i,
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(pwrseq->reset_gpio[i]) &&
+ PTR_ERR(pwrseq->reset_gpio[i]) != -ENOENT &&
+ PTR_ERR(pwrseq->reset_gpio[i]) != -ENOSYS) {
+ ret = PTR_ERR(pwrseq->reset_gpio[i]);
is simple to add:
while(--i)
gpiod_put(pwrseq->reset_gpio[i])
That's true, will change.
+ goto free;
+ }
+ }
}
pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
@@ -81,6 +102,13 @@ int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev)
return 0;
free:
+ if (pwrseq->nr_gpios > 0) {
+ for (i = 0; i < pwrseq->nr_gpios; i++)
+ if (!IS_ERR_OR_NULL(pwrseq->reset_gpio[i]))
+ gpiod_put(pwrseq->reset_gpio[i]);
+ kfree(pwrseq->reset_gpio);
+ }
+
kfree(pwrseq);
return ret;
}
I get a feeling that am just dumping my patch here.. If possible could
you have look at it too.
Of course, do you have a link archive since I can't find it on my inbox.
I did send to linux-mmc@xxxxxxxxxxxxxxx, Anyway i did forward you the patch.
thanks,
--srini
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/