| Message ID | 1422439819-29854-3-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Javier, You are in a lead of 3 hrs from me.. Surprisingly I send very much same patch just few Mins ago :-) May be we can merge goods in both :-) 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@collabora.co.uk> > --- > 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.. If you make this struct gpio_desc *reset_gpios[0]; You can aviod an extra kmalloc and free .. > + 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); } > } > > 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? Probably you should do of_gpio_named_count before allocating memory. > + 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]) > + 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. Thanks, srini -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@collabora.co.uk> >> --- >> 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. >> + 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. > Thanks, > srini > Again, thanks a lot and best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + 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); } 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) { + 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]); + 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; }
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@collabora.co.uk> --- drivers/mmc/core/pwrseq_simple.c | 54 ++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 13 deletions(-)