LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/5] Add multiple GPIO and external clock to MMC pwrseq_simple
@ 2015-01-28 10:10 Javier Martinez Canillas
  2015-01-28 10:10 ` [PATCH 1/5] mmc: pwrseq: Document that simple sequence support more than one GPIO Javier Martinez Canillas
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2015-01-28 10:10 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Kukjin Kim, Doug Anderson, Olof Johansson, linux-samsung-soc,
	linux-arm-kernel, linux-mmc, devicetree, linux-kernel,
	Javier Martinez Canillas

Hello Ulf,

Many WLAN chips attached to an SDIO interface needs more than one GPIO
for their reset sequence and also need an external clock to be operational.

Since this is very common, this series extend the simple MMC power sequence
to support more than one reset GPIO and also an optional external clock.

The series depend on v4 "mmc: core: Add support for MMC power sequences":

http://comments.gmane.org/gmane.linux.kernel.mmc/30665

Javier Martinez Canillas (5):
  mmc: pwrseq: Document that simple sequence support more than one GPIO
  mmc: pwrseq_simple: Extend to support more pins
  mmc: pwrseq: Document optional clock for the simple power sequence
  mmc: pwrseq_simple: Add optional reference clock support
  ARM: dts: exynos5250-snow: Enable wifi power-on

 .../devicetree/bindings/mmc/mmc-pwrseq-simple.txt  | 10 ++-
 arch/arm/boot/dts/exynos5250-snow.dts              | 25 ++++++-
 drivers/mmc/core/pwrseq_simple.c                   | 85 ++++++++++++++++++----
 3 files changed, 103 insertions(+), 17 deletions(-)

Patch #1 extends the simple MMC power sequence DT binding to support more
than one GPIO and patch #2 adds the actual implementation.

In the same way, patch #3 and #4 extend the simple MMC power sequence DT
binding and pwrseq_simple driver to support an optional external clock.

Finally as an example, patch #5 adds support for the wifi chip in the
Exynos5250 Snow that is attached to a SDIO interface and needs all these.

Best regards,
Javier


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/5] mmc: pwrseq: Document that simple sequence support more than one GPIO
  2015-01-28 10:10 [PATCH 0/5] Add multiple GPIO and external clock to MMC pwrseq_simple Javier Martinez Canillas
@ 2015-01-28 10:10 ` Javier Martinez Canillas
  2015-01-28 16:34   ` Srinivas Kandagatla
  2015-01-28 10:10 ` [PATCH 2/5] mmc: pwrseq_simple: Extend to support more pins Javier Martinez Canillas
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2015-01-28 10:10 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Kukjin Kim, Doug Anderson, Olof Johansson, linux-samsung-soc,
	linux-arm-kernel, linux-mmc, devicetree, linux-kernel,
	Javier Martinez Canillas

Many SDIO/MMC attached WLAN chips need more than one ping for their reset
sequence. Extend the pwrseq_simple binding to support more than one pin.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
index da333d9ed94c..9a77deb164c7 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
@@ -8,9 +8,9 @@ Required properties:
 - compatible : contains "mmc-pwrseq-simple".
 
 Optional properties:
-- reset-gpios : contains a GPIO specifier. The reset GPIO is asserted at
-	initialization and prior we start the power up procedure of the card. It
-	will be de-asserted right after the power has been provided to the card.
+- reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted
+	at initialization and prior we start the power up procedure of the card.
+	Will be de-asserted right after the power has been provided to the card.
 
 Example:
 
-- 
2.1.3


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/5] mmc: pwrseq_simple: Extend to support more pins
  2015-01-28 10:10 [PATCH 0/5] Add multiple GPIO and external clock to MMC pwrseq_simple Javier Martinez Canillas
  2015-01-28 10:10 ` [PATCH 1/5] mmc: pwrseq: Document that simple sequence support more than one GPIO Javier Martinez Canillas
@ 2015-01-28 10:10 ` Javier Martinez Canillas
  2015-01-28 14:01   ` Srinivas Kandagatla
  2015-01-28 10:10 ` [PATCH 3/5] mmc: pwrseq: Document optional clock for the simple power sequence Javier Martinez Canillas
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2015-01-28 10:10 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Kukjin Kim, Doug Anderson, Olof Johansson, linux-samsung-soc,
	linux-arm-kernel, linux-mmc, devicetree, linux-kernel,
	Javier Martinez Canillas

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;
+	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;
 }
-- 
2.1.3


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/5] mmc: pwrseq: Document optional clock for the simple power sequence
  2015-01-28 10:10 [PATCH 0/5] Add multiple GPIO and external clock to MMC pwrseq_simple Javier Martinez Canillas
  2015-01-28 10:10 ` [PATCH 1/5] mmc: pwrseq: Document that simple sequence support more than one GPIO Javier Martinez Canillas
  2015-01-28 10:10 ` [PATCH 2/5] mmc: pwrseq_simple: Extend to support more pins Javier Martinez Canillas
@ 2015-01-28 10:10 ` Javier Martinez Canillas
  2015-01-28 10:10 ` [PATCH 4/5] mmc: pwrseq_simple: Add optional reference clock support Javier Martinez Canillas
  2015-01-28 10:10 ` [PATCH 5/5] ARM: dts: exynos5250-snow: Enable wifi power-on Javier Martinez Canillas
  4 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2015-01-28 10:10 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Kukjin Kim, Doug Anderson, Olof Johansson, linux-samsung-soc,
	linux-arm-kernel, linux-mmc, devicetree, linux-kernel,
	Javier Martinez Canillas

Some WLAN chips attached to a SDIO interface, need an external clock
to be operational. Since this is very common, extend the simple MMC
power sequence DT binding to support an optional clock.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
index 9a77deb164c7..6b5c10d1144c 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
@@ -11,6 +11,10 @@ Optional properties:
 - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted
 	at initialization and prior we start the power up procedure of the card.
 	Will be de-asserted right after the power has been provided to the card.
+- clocks : Must contain an entry for the entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names : Must include the following entry:
+  "ext_clock" (External clock provided to the card).
 
 Example:
 
-- 
2.1.3


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 4/5] mmc: pwrseq_simple: Add optional reference clock support
  2015-01-28 10:10 [PATCH 0/5] Add multiple GPIO and external clock to MMC pwrseq_simple Javier Martinez Canillas
                   ` (2 preceding siblings ...)
  2015-01-28 10:10 ` [PATCH 3/5] mmc: pwrseq: Document optional clock for the simple power sequence Javier Martinez Canillas
@ 2015-01-28 10:10 ` Javier Martinez Canillas
  2015-01-28 10:10 ` [PATCH 5/5] ARM: dts: exynos5250-snow: Enable wifi power-on Javier Martinez Canillas
  4 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2015-01-28 10:10 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Kukjin Kim, Doug Anderson, Olof Johansson, linux-samsung-soc,
	linux-arm-kernel, linux-mmc, devicetree, linux-kernel,
	Javier Martinez Canillas

Some WLAN chips attached to a SDIO interface, need a reference clock.

Since this is very common, extend the prseq_simple driver to support
an optional clock that is enabled prior the card power up procedure.

Note, the external clock is optional. Thus an error is not returned
if the clock is not found.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/mmc/core/pwrseq_simple.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 9e51fe1051c5..5ef6db7323f3 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -7,6 +7,7 @@
  *
  *  Simple MMC power sequence management
  */
+#include <linux/clk.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/device.h>
@@ -16,12 +17,14 @@
 
 #include <linux/mmc/host.h>
 
+#include "core.h"
 #include "pwrseq.h"
 
 struct mmc_pwrseq_simple {
 	struct mmc_pwrseq pwrseq;
 	struct gpio_desc **reset_gpio;
 	int nr_gpios;
+	struct clk *ext_clk;
 };
 
 static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
@@ -30,6 +33,9 @@ static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
 					struct mmc_pwrseq_simple, pwrseq);
 	int i;
 
+	if (pwrseq->ext_clk)
+		clk_prepare_enable(pwrseq->ext_clk);
+
 	for (i = 0; i < pwrseq->nr_gpios; i++)
 		if (!IS_ERR(pwrseq->reset_gpio[i]))
 			gpiod_set_value_cansleep(pwrseq->reset_gpio[i], 1);
@@ -46,12 +52,29 @@ static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
 			gpiod_set_value_cansleep(pwrseq->reset_gpio[i], 0);
 }
 
+static void mmc_pwrseq_simple_power_off(struct mmc_host *host)
+{
+	struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
+					struct mmc_pwrseq_simple, pwrseq);
+	int i;
+
+	for (i = 0; i < pwrseq->nr_gpios; i++)
+		if (!IS_ERR(pwrseq->reset_gpio[i]))
+			gpiod_set_value_cansleep(pwrseq->reset_gpio[i], 1);
+
+	if (pwrseq->ext_clk)
+		clk_disable_unprepare(pwrseq->ext_clk);
+}
+
 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 (pwrseq->ext_clk)
+		clk_put(pwrseq->ext_clk);
+
 	if (pwrseq->nr_gpios > 0) {
 		for (i = 0; i < pwrseq->nr_gpios; i++)
 			if (!IS_ERR(pwrseq->reset_gpio[i]))
@@ -66,7 +89,7 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
 static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
 	.pre_power_on = mmc_pwrseq_simple_pre_power_on,
 	.post_power_on = mmc_pwrseq_simple_post_power_on,
-	.power_off = mmc_pwrseq_simple_pre_power_on,
+	.power_off = mmc_pwrseq_simple_power_off,
 	.free = mmc_pwrseq_simple_free,
 };
 
@@ -97,6 +120,14 @@ int mmc_pwrseq_simple_alloc(struct mmc_host *host, struct device *dev)
 		}
 	}
 
+	pwrseq->ext_clk = clk_get(dev, "ext_clock");
+	if (IS_ERR(pwrseq->ext_clk) &&
+	    PTR_ERR(pwrseq->ext_clk) != -ENOENT &&
+	    PTR_ERR(pwrseq->ext_clk) != -ENOSYS) {
+		ret = PTR_ERR(pwrseq->ext_clk);
+		goto free;
+	}
+
 	pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
 	host->pwrseq = &pwrseq->pwrseq;
 
-- 
2.1.3


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 5/5] ARM: dts: exynos5250-snow: Enable wifi power-on
  2015-01-28 10:10 [PATCH 0/5] Add multiple GPIO and external clock to MMC pwrseq_simple Javier Martinez Canillas
                   ` (3 preceding siblings ...)
  2015-01-28 10:10 ` [PATCH 4/5] mmc: pwrseq_simple: Add optional reference clock support Javier Martinez Canillas
@ 2015-01-28 10:10 ` Javier Martinez Canillas
  2015-01-28 14:03   ` Arend van Spriel
  4 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2015-01-28 10:10 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Kukjin Kim, Doug Anderson, Olof Johansson, linux-samsung-soc,
	linux-arm-kernel, linux-mmc, devicetree, linux-kernel,
	Javier Martinez Canillas

The Snow board has a MMC/SDIO wifi chip that is always powered but it
needs a power sequence involving a reset (active low) and an enable
(active high) pins. Both pins are marked as active low since the MMC
simple power sequence driver asserts the pins prior to the card power
up procedure and de-asserts the pins after the card has been powered.

So the reset line will be left de-asserted and the enable pin will be
left asserted.

The chip also needs an external 32kHz reference clock to be operational
that is by the MAX77686 PMIC clock.

Add a simple MMC power sequence provider for the wifi MMC/SDIO slot.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/exynos5250-snow.dts | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
index b9aeec430527..0f7971ba8238 100644
--- a/arch/arm/boot/dts/exynos5250-snow.dts
+++ b/arch/arm/boot/dts/exynos5250-snow.dts
@@ -229,6 +229,13 @@
 		power-supply = <&fet6>;
 		backlight = <&backlight>;
 	};
+
+	mmc3_pwrseq: mmc3_pwrseq {
+		reset-gpios = <&gpx0 2 GPIO_ACTIVE_LOW>, /* WIFI_RSTn */
+			      <&gpx0 1 GPIO_ACTIVE_LOW>; /* WIFI_EN */
+		clocks = <&max77686 MAX77686_CLK_PMIC>;
+		clock-names = "ext_clock";
+	};
 };
 
 &dp {
@@ -531,17 +538,33 @@
 	status = "okay";
 	num-slots = <1>;
 	broken-cd;
+	cap-sdio-irq;
 	card-detect-delay = <200>;
 	samsung,dw-mshc-ciu-div = <3>;
 	samsung,dw-mshc-sdr-timing = <2 3>;
 	samsung,dw-mshc-ddr-timing = <1 2>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&sd3_clk &sd3_cmd &sd3_bus4>;
+	pinctrl-0 = <&sd3_clk &sd3_cmd &sd3_bus4 &wifi_en &wifi_rst>;
 	bus-width = <4>;
 	cap-sd-highspeed;
+	mmc-pwrseq = <&mmc3_pwrseq>;
 };
 
 &pinctrl_0 {
+	wifi_en: wifi-en {
+		samsung,pins = "gpx0-1";
+		samsung,pin-function = <1>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+
+	wifi_rst: wifi-rst {
+		samsung,pins = "gpx0-2";
+		samsung,pin-function = <1>;
+		samsung,pin-pud = <0>;
+		samsung,pin-drv = <0>;
+	};
+
 	power_key_irq: power-key-irq {
 		samsung,pins = "gpx1-3";
 		samsung,pin-function = <0xf>;
-- 
2.1.3


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] mmc: pwrseq_simple: Extend to support more pins
  2015-01-28 10:10 ` [PATCH 2/5] mmc: pwrseq_simple: Extend to support more pins Javier Martinez Canillas
@ 2015-01-28 14:01   ` Srinivas Kandagatla
  2015-01-28 16:13     ` Javier Martinez Canillas
  0 siblings, 1 reply; 13+ messages in thread
From: Srinivas Kandagatla @ 2015-01-28 14:01 UTC (permalink / raw)
  To: Javier Martinez Canillas, Ulf Hansson
  Cc: devicetree, linux-samsung-soc, linux-mmc, Doug Anderson,
	linux-kernel, Kukjin Kim, Olof Johansson, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/5] ARM: dts: exynos5250-snow: Enable wifi power-on
  2015-01-28 10:10 ` [PATCH 5/5] ARM: dts: exynos5250-snow: Enable wifi power-on Javier Martinez Canillas
@ 2015-01-28 14:03   ` Arend van Spriel
  2015-01-28 15:57     ` Javier Martinez Canillas
  0 siblings, 1 reply; 13+ messages in thread
From: Arend van Spriel @ 2015-01-28 14:03 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Ulf Hansson, devicetree, linux-samsung-soc, linux-mmc,
	Doug Anderson, linux-kernel, Kukjin Kim, Olof Johansson,
	linux-arm-kernel

On 01/28/15 11:10, Javier Martinez Canillas wrote:
> The Snow board has a MMC/SDIO wifi chip that is always powered but it
> needs a power sequence involving a reset (active low) and an enable
> (active high) pins. Both pins are marked as active low since the MMC
> simple power sequence driver asserts the pins prior to the card power
> up procedure and de-asserts the pins after the card has been powered.
>
> So the reset line will be left de-asserted and the enable pin will be
> left asserted.
>
> The chip also needs an external 32kHz reference clock to be operational
> that is by the MAX77686 PMIC clock.
>
> Add a simple MMC power sequence provider for the wifi MMC/SDIO slot.
>
> Signed-off-by: Javier Martinez Canillas<javier.martinez@collabora.co.uk>
> ---
>   arch/arm/boot/dts/exynos5250-snow.dts | 25 ++++++++++++++++++++++++-
>   1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
> index b9aeec430527..0f7971ba8238 100644
> --- a/arch/arm/boot/dts/exynos5250-snow.dts
> +++ b/arch/arm/boot/dts/exynos5250-snow.dts
> @@ -229,6 +229,13 @@
>   		power-supply =<&fet6>;
>   		backlight =<&backlight>;
>   	};
> +
> +	mmc3_pwrseq: mmc3_pwrseq {
> +		reset-gpios =<&gpx0 2 GPIO_ACTIVE_LOW>, /* WIFI_RSTn */
> +			<&gpx0 1 GPIO_ACTIVE_LOW>; /* WIFI_EN */
> +		clocks =<&max77686 MAX77686_CLK_PMIC>;
> +		clock-names = "ext_clock";
> +	};
>   };
>
>   &dp {
> @@ -531,17 +538,33 @@
>   	status = "okay";
>   	num-slots =<1>;
>   	broken-cd;
> +	cap-sdio-irq;

This seems like an unrelated change, right?

Regards,
Arend

>   	card-detect-delay =<200>;
>   	samsung,dw-mshc-ciu-div =<3>;
>   	samsung,dw-mshc-sdr-timing =<2 3>;
>   	samsung,dw-mshc-ddr-timing =<1 2>;
>   	pinctrl-names = "default";
> -	pinctrl-0 =<&sd3_clk&sd3_cmd&sd3_bus4>;
> +	pinctrl-0 =<&sd3_clk&sd3_cmd&sd3_bus4&wifi_en&wifi_rst>;
>   	bus-width =<4>;
>   	cap-sd-highspeed;
> +	mmc-pwrseq =<&mmc3_pwrseq>;
>   };
>
>   &pinctrl_0 {
> +	wifi_en: wifi-en {
> +		samsung,pins = "gpx0-1";
> +		samsung,pin-function =<1>;
> +		samsung,pin-pud =<0>;
> +		samsung,pin-drv =<0>;
> +	};
> +
> +	wifi_rst: wifi-rst {
> +		samsung,pins = "gpx0-2";
> +		samsung,pin-function =<1>;
> +		samsung,pin-pud =<0>;
> +		samsung,pin-drv =<0>;
> +	};
> +
>   	power_key_irq: power-key-irq {
>   		samsung,pins = "gpx1-3";
>   		samsung,pin-function =<0xf>;


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/5] ARM: dts: exynos5250-snow: Enable wifi power-on
  2015-01-28 14:03   ` Arend van Spriel
@ 2015-01-28 15:57     ` Javier Martinez Canillas
  0 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2015-01-28 15:57 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: Ulf Hansson, devicetree, linux-samsung-soc, linux-mmc,
	Doug Anderson, linux-kernel, Kukjin Kim, Olof Johansson,
	linux-arm-kernel

Hello Arend,

Thanks for your feedback.

On 01/28/2015 03:03 PM, Arend van Spriel wrote:
> On 01/28/15 11:10, Javier Martinez Canillas wrote:

>>   &dp {
>> @@ -531,17 +538,33 @@
>>   	status = "okay";
>>   	num-slots =<1>;
>>   	broken-cd;
>> +	cap-sdio-irq;
> 
> This seems like an unrelated change, right?
> 

Well, I found that enabling the SDIO IRQ signalling I had the double
of the transfer rate and I didn't think it justified a separate patch
but you are right and I'll split on v2.

> Regards,
> Arend
> 

Best regards,
Javier

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] mmc: pwrseq_simple: Extend to support more pins
  2015-01-28 14:01   ` Srinivas Kandagatla
@ 2015-01-28 16:13     ` Javier Martinez Canillas
  2015-01-28 16:31       ` Srinivas Kandagatla
  0 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2015-01-28 16:13 UTC (permalink / raw)
  To: Srinivas Kandagatla, Ulf Hansson
  Cc: devicetree, linux-samsung-soc, linux-mmc, Doug Anderson,
	linux-kernel, Kukjin Kim, Olof Johansson, linux-arm-kernel

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] mmc: pwrseq_simple: Extend to support more pins
  2015-01-28 16:13     ` Javier Martinez Canillas
@ 2015-01-28 16:31       ` Srinivas Kandagatla
  0 siblings, 0 replies; 13+ messages in thread
From: Srinivas Kandagatla @ 2015-01-28 16:31 UTC (permalink / raw)
  To: Javier Martinez Canillas, Ulf Hansson
  Cc: devicetree, linux-samsung-soc, linux-mmc, Doug Anderson,
	linux-kernel, Kukjin Kim, Olof Johansson, linux-arm-kernel



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@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.

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@vger.kernel.org, Anyway i did forward you the patch.

thanks,
--srini

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] mmc: pwrseq: Document that simple sequence support more than one GPIO
  2015-01-28 10:10 ` [PATCH 1/5] mmc: pwrseq: Document that simple sequence support more than one GPIO Javier Martinez Canillas
@ 2015-01-28 16:34   ` Srinivas Kandagatla
  2015-01-28 16:35     ` Javier Martinez Canillas
  0 siblings, 1 reply; 13+ messages in thread
From: Srinivas Kandagatla @ 2015-01-28 16:34 UTC (permalink / raw)
  To: Javier Martinez Canillas, Ulf Hansson
  Cc: devicetree, linux-samsung-soc, linux-mmc, Doug Anderson,
	linux-kernel, Kukjin Kim, Olof Johansson, linux-arm-kernel



On 28/01/15 10:10, Javier Martinez Canillas wrote:
> Many SDIO/MMC attached WLAN chips need more than one ping for their reset
> sequence. Extend the pwrseq_simple binding to support more than one pin.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>   Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> index da333d9ed94c..9a77deb164c7 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> @@ -8,9 +8,9 @@ Required properties:
>   - compatible : contains "mmc-pwrseq-simple".
>
>   Optional properties:
> -- reset-gpios : contains a GPIO specifier. The reset GPIO is asserted at
> -	initialization and prior we start the power up procedure of the card. It
> -	will be de-asserted right after the power has been provided to the card.
> +- reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted
> +	at initialization and prior we start the power up procedure of the card.
> +	Will be de-asserted right after the power has been provided to the card.
Makes sense if you add "They" before last sentence.
>
>   Example:
>
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] mmc: pwrseq: Document that simple sequence support more than one GPIO
  2015-01-28 16:34   ` Srinivas Kandagatla
@ 2015-01-28 16:35     ` Javier Martinez Canillas
  0 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2015-01-28 16:35 UTC (permalink / raw)
  To: Srinivas Kandagatla, Ulf Hansson
  Cc: devicetree, linux-samsung-soc, linux-mmc, Doug Anderson,
	linux-kernel, Kukjin Kim, Olof Johansson, linux-arm-kernel

Srinivas,

On 01/28/2015 05:34 PM, Srinivas Kandagatla wrote:
>> -- reset-gpios : contains a GPIO specifier. The reset GPIO is asserted at
>> -	initialization and prior we start the power up procedure of the card. It
>> -	will be de-asserted right after the power has been provided to the card.
>> +- reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted
>> +	at initialization and prior we start the power up procedure of the card.
>> +	Will be de-asserted right after the power has been provided to the card.
> Makes sense if you add "They" before last sentence.

Thanks, I'll change it.

Best regards,
Javier


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-01-29  3:14 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-28 10:10 [PATCH 0/5] Add multiple GPIO and external clock to MMC pwrseq_simple Javier Martinez Canillas
2015-01-28 10:10 ` [PATCH 1/5] mmc: pwrseq: Document that simple sequence support more than one GPIO Javier Martinez Canillas
2015-01-28 16:34   ` Srinivas Kandagatla
2015-01-28 16:35     ` Javier Martinez Canillas
2015-01-28 10:10 ` [PATCH 2/5] mmc: pwrseq_simple: Extend to support more pins Javier Martinez Canillas
2015-01-28 14:01   ` Srinivas Kandagatla
2015-01-28 16:13     ` Javier Martinez Canillas
2015-01-28 16:31       ` Srinivas Kandagatla
2015-01-28 10:10 ` [PATCH 3/5] mmc: pwrseq: Document optional clock for the simple power sequence Javier Martinez Canillas
2015-01-28 10:10 ` [PATCH 4/5] mmc: pwrseq_simple: Add optional reference clock support Javier Martinez Canillas
2015-01-28 10:10 ` [PATCH 5/5] ARM: dts: exynos5250-snow: Enable wifi power-on Javier Martinez Canillas
2015-01-28 14:03   ` Arend van Spriel
2015-01-28 15:57     ` Javier Martinez Canillas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).