* [RFC PATCH 0/3] mmc: pwrseq: convert to proper driver
@ 2016-01-19 10:57 ` Srinivas Kandagatla
0 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 10:57 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc
Cc: linux-kernel, linux-arm-kernel, Srinivas Kandagatla
Hi Ulf,
This patchset aims at converting the pwrseq devices to proper in built drivers,
The issue is that on Qualcomm based platforms, most of the gpios require
a pinctrl setup. Existing pwrseq code does not do any kind of pinctrl setup.
So this makes the pwrseq unusable on those platfroms.
There are two possible solutions to this isssue.
1. Convert the pwrseq to proper drivers so that they can reuse
common driver setup like pinctrl from drivers/base/pinctrl.c
2. copy pinctrl_bind_pins() code and modify to fit pwrseq.
IMO, solution 1 works much better and would make pwrseq to reuse
all the driver setup done in the common code.
Thanks,
srini
Srinivas Kandagatla (3):
mmc: pwrseq_simple: add to_pwrseq_simple() macro
mmc: pwrseq_emmc: add to_pwrseq_emmc() macro
mmc: pwrseq: convert to proper platform device
drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
drivers/mmc/core/pwrseq.h | 13 ++++--
drivers/mmc/core/pwrseq_emmc.c | 73 ++++++++++++++++++-----------
drivers/mmc/core/pwrseq_simple.c | 85 ++++++++++++++++++++--------------
4 files changed, 155 insertions(+), 115 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH 0/3] mmc: pwrseq: convert to proper driver
@ 2016-01-19 10:57 ` Srinivas Kandagatla
0 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 10:57 UTC (permalink / raw)
To: linux-arm-kernel
Hi Ulf,
This patchset aims at converting the pwrseq devices to proper in built drivers,
The issue is that on Qualcomm based platforms, most of the gpios require
a pinctrl setup. Existing pwrseq code does not do any kind of pinctrl setup.
So this makes the pwrseq unusable on those platfroms.
There are two possible solutions to this isssue.
1. Convert the pwrseq to proper drivers so that they can reuse
common driver setup like pinctrl from drivers/base/pinctrl.c
2. copy pinctrl_bind_pins() code and modify to fit pwrseq.
IMO, solution 1 works much better and would make pwrseq to reuse
all the driver setup done in the common code.
Thanks,
srini
Srinivas Kandagatla (3):
mmc: pwrseq_simple: add to_pwrseq_simple() macro
mmc: pwrseq_emmc: add to_pwrseq_emmc() macro
mmc: pwrseq: convert to proper platform device
drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
drivers/mmc/core/pwrseq.h | 13 ++++--
drivers/mmc/core/pwrseq_emmc.c | 73 ++++++++++++++++++-----------
drivers/mmc/core/pwrseq_simple.c | 85 ++++++++++++++++++++--------------
4 files changed, 155 insertions(+), 115 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH 1/3] mmc: pwrseq_simple: add to_pwrseq_simple() macro
2016-01-19 10:57 ` Srinivas Kandagatla
@ 2016-01-19 10:58 ` Srinivas Kandagatla
-1 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 10:58 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc
Cc: linux-kernel, linux-arm-kernel, Srinivas Kandagatla
This patch adds to_pwrseq_simple() macro to make the code more readable.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/mmc/core/pwrseq_simple.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index d10538b..c091f98 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -26,6 +26,8 @@ struct mmc_pwrseq_simple {
struct gpio_descs *reset_gpios;
};
+#define to_pwrseq_simple(p) container_of(p, struct mmc_pwrseq_simple, pwrseq)
+
static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
int value)
{
@@ -42,8 +44,7 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
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);
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
if (!IS_ERR(pwrseq->ext_clk) && !pwrseq->clk_enabled) {
clk_prepare_enable(pwrseq->ext_clk);
@@ -55,16 +56,14 @@ static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
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);
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
mmc_pwrseq_simple_set_gpios_value(pwrseq, 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);
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
@@ -76,8 +75,7 @@ static void mmc_pwrseq_simple_power_off(struct mmc_host *host)
static void mmc_pwrseq_simple_free(struct mmc_host *host)
{
- struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
- struct mmc_pwrseq_simple, pwrseq);
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
gpiod_put_array(pwrseq->reset_gpios);
--
1.9.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 1/3] mmc: pwrseq_simple: add to_pwrseq_simple() macro
@ 2016-01-19 10:58 ` Srinivas Kandagatla
0 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 10:58 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds to_pwrseq_simple() macro to make the code more readable.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/mmc/core/pwrseq_simple.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index d10538b..c091f98 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -26,6 +26,8 @@ struct mmc_pwrseq_simple {
struct gpio_descs *reset_gpios;
};
+#define to_pwrseq_simple(p) container_of(p, struct mmc_pwrseq_simple, pwrseq)
+
static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
int value)
{
@@ -42,8 +44,7 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
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);
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
if (!IS_ERR(pwrseq->ext_clk) && !pwrseq->clk_enabled) {
clk_prepare_enable(pwrseq->ext_clk);
@@ -55,16 +56,14 @@ static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
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);
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
mmc_pwrseq_simple_set_gpios_value(pwrseq, 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);
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
@@ -76,8 +75,7 @@ static void mmc_pwrseq_simple_power_off(struct mmc_host *host)
static void mmc_pwrseq_simple_free(struct mmc_host *host)
{
- struct mmc_pwrseq_simple *pwrseq = container_of(host->pwrseq,
- struct mmc_pwrseq_simple, pwrseq);
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
gpiod_put_array(pwrseq->reset_gpios);
--
1.9.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 2/3] mmc: pwrseq_emmc: add to_pwrseq_emmc() macro
2016-01-19 10:57 ` Srinivas Kandagatla
@ 2016-01-19 10:59 ` Srinivas Kandagatla
-1 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 10:59 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc
Cc: linux-kernel, linux-arm-kernel, Srinivas Kandagatla
This patch adds to_pwrseq_emmc() macro to make the code more readable.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/mmc/core/pwrseq_emmc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
index ad4f94e..7978fb7 100644
--- a/drivers/mmc/core/pwrseq_emmc.c
+++ b/drivers/mmc/core/pwrseq_emmc.c
@@ -25,6 +25,8 @@ struct mmc_pwrseq_emmc {
struct gpio_desc *reset_gpio;
};
+#define to_pwrseq_emmc(p) container_of(p, struct mmc_pwrseq_emmc, pwrseq)
+
static void __mmc_pwrseq_emmc_reset(struct mmc_pwrseq_emmc *pwrseq)
{
gpiod_set_value(pwrseq->reset_gpio, 1);
@@ -35,16 +37,14 @@ static void __mmc_pwrseq_emmc_reset(struct mmc_pwrseq_emmc *pwrseq)
static void mmc_pwrseq_emmc_reset(struct mmc_host *host)
{
- struct mmc_pwrseq_emmc *pwrseq = container_of(host->pwrseq,
- struct mmc_pwrseq_emmc, pwrseq);
+ struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
__mmc_pwrseq_emmc_reset(pwrseq);
}
static void mmc_pwrseq_emmc_free(struct mmc_host *host)
{
- struct mmc_pwrseq_emmc *pwrseq = container_of(host->pwrseq,
- struct mmc_pwrseq_emmc, pwrseq);
+ struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
unregister_restart_handler(&pwrseq->reset_nb);
gpiod_put(pwrseq->reset_gpio);
--
1.9.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 2/3] mmc: pwrseq_emmc: add to_pwrseq_emmc() macro
@ 2016-01-19 10:59 ` Srinivas Kandagatla
0 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 10:59 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds to_pwrseq_emmc() macro to make the code more readable.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/mmc/core/pwrseq_emmc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
index ad4f94e..7978fb7 100644
--- a/drivers/mmc/core/pwrseq_emmc.c
+++ b/drivers/mmc/core/pwrseq_emmc.c
@@ -25,6 +25,8 @@ struct mmc_pwrseq_emmc {
struct gpio_desc *reset_gpio;
};
+#define to_pwrseq_emmc(p) container_of(p, struct mmc_pwrseq_emmc, pwrseq)
+
static void __mmc_pwrseq_emmc_reset(struct mmc_pwrseq_emmc *pwrseq)
{
gpiod_set_value(pwrseq->reset_gpio, 1);
@@ -35,16 +37,14 @@ static void __mmc_pwrseq_emmc_reset(struct mmc_pwrseq_emmc *pwrseq)
static void mmc_pwrseq_emmc_reset(struct mmc_host *host)
{
- struct mmc_pwrseq_emmc *pwrseq = container_of(host->pwrseq,
- struct mmc_pwrseq_emmc, pwrseq);
+ struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
__mmc_pwrseq_emmc_reset(pwrseq);
}
static void mmc_pwrseq_emmc_free(struct mmc_host *host)
{
- struct mmc_pwrseq_emmc *pwrseq = container_of(host->pwrseq,
- struct mmc_pwrseq_emmc, pwrseq);
+ struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
unregister_restart_handler(&pwrseq->reset_nb);
gpiod_put(pwrseq->reset_gpio);
--
1.9.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device
2016-01-19 10:57 ` Srinivas Kandagatla
@ 2016-01-19 10:59 ` Srinivas Kandagatla
-1 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 10:59 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc
Cc: linux-kernel, linux-arm-kernel, Srinivas Kandagatla
simple-pwrseq and emmc-pwrseq drivers rely on platform_device
structure from of_find_device_by_node(), this works mostly. But, as there
is no driver associated with this devices, cases like default/init pinctrl
setup would never be performed by pwrseq. This becomes problem when the
gpios used in pwrseq require pinctrl setup.
Currently most of the common pinctrl setup is done in
drivers/base/pinctrl.c by pinctrl_bind_pins().
There are two ways to slove this issue on either convert pwrseq drivers
to a proper platform drivers or copy the exact code from
pcintrl_bind_pins(). I prefer converting pwrseq to proper drivers so that
other cases like setting up clks/parents from dt would also be possible.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
drivers/mmc/core/pwrseq.h | 13 ++++--
drivers/mmc/core/pwrseq_emmc.c | 65 ++++++++++++++++----------
drivers/mmc/core/pwrseq_simple.c | 71 ++++++++++++++++++----------
4 files changed, 145 insertions(+), 103 deletions(-)
diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
index 4c1d175..669608e 100644
--- a/drivers/mmc/core/pwrseq.c
+++ b/drivers/mmc/core/pwrseq.c
@@ -8,50 +8,30 @@
* MMC power sequence management
*/
#include <linux/kernel.h>
-#include <linux/platform_device.h>
#include <linux/err.h>
#include <linux/of.h>
-#include <linux/of_platform.h>
#include <linux/mmc/host.h>
#include "pwrseq.h"
-struct mmc_pwrseq_match {
- const char *compatible;
- struct mmc_pwrseq *(*alloc)(struct mmc_host *host, struct device *dev);
-};
-
-static struct mmc_pwrseq_match pwrseq_match[] = {
- {
- .compatible = "mmc-pwrseq-simple",
- .alloc = mmc_pwrseq_simple_alloc,
- }, {
- .compatible = "mmc-pwrseq-emmc",
- .alloc = mmc_pwrseq_emmc_alloc,
- },
-};
-
-static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
+static DEFINE_MUTEX(pwrseq_list_mutex);
+static LIST_HEAD(pwrseq_list);
+
+static struct mmc_pwrseq *of_find_mmc_pwrseq(struct device_node *np)
{
- struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
- int i;
+ struct mmc_pwrseq *p;
- for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
- if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
- match = &pwrseq_match[i];
- break;
- }
- }
+ list_for_each_entry(p, &pwrseq_list, list)
+ if (p->dev->of_node == np)
+ return p;
- return match;
+ return NULL;
}
int mmc_pwrseq_alloc(struct mmc_host *host)
{
- struct platform_device *pdev;
struct device_node *np;
- struct mmc_pwrseq_match *match;
struct mmc_pwrseq *pwrseq;
int ret = 0;
@@ -59,25 +39,18 @@ int mmc_pwrseq_alloc(struct mmc_host *host)
if (!np)
return 0;
- pdev = of_find_device_by_node(np);
- if (!pdev) {
- ret = -ENODEV;
- goto err;
- }
+ pwrseq = of_find_mmc_pwrseq(np);
- match = mmc_pwrseq_find(np);
- if (IS_ERR(match)) {
- ret = PTR_ERR(match);
- goto err;
- }
+ if (pwrseq && pwrseq->ops && pwrseq->ops->alloc) {
+ host->pwrseq = pwrseq;
+ ret = pwrseq->ops->alloc(host);
+ if (IS_ERR_VALUE(ret)) {
+ host->pwrseq = NULL;
+ goto err;
+ }
- pwrseq = match->alloc(host, &pdev->dev);
- if (IS_ERR(pwrseq)) {
- ret = PTR_ERR(pwrseq);
- goto err;
}
- host->pwrseq = pwrseq;
dev_info(host->parent, "allocated mmc-pwrseq\n");
err:
@@ -89,7 +62,7 @@ void mmc_pwrseq_pre_power_on(struct mmc_host *host)
{
struct mmc_pwrseq *pwrseq = host->pwrseq;
- if (pwrseq && pwrseq->ops && pwrseq->ops->pre_power_on)
+ if (pwrseq && pwrseq->ops->pre_power_on)
pwrseq->ops->pre_power_on(host);
}
@@ -97,7 +70,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host)
{
struct mmc_pwrseq *pwrseq = host->pwrseq;
- if (pwrseq && pwrseq->ops && pwrseq->ops->post_power_on)
+ if (pwrseq && pwrseq->ops->post_power_on)
pwrseq->ops->post_power_on(host);
}
@@ -105,7 +78,7 @@ void mmc_pwrseq_power_off(struct mmc_host *host)
{
struct mmc_pwrseq *pwrseq = host->pwrseq;
- if (pwrseq && pwrseq->ops && pwrseq->ops->power_off)
+ if (pwrseq && pwrseq->ops->power_off)
pwrseq->ops->power_off(host);
}
@@ -113,8 +86,34 @@ void mmc_pwrseq_free(struct mmc_host *host)
{
struct mmc_pwrseq *pwrseq = host->pwrseq;
- if (pwrseq && pwrseq->ops && pwrseq->ops->free)
- pwrseq->ops->free(host);
+ if (pwrseq) {
+ if (pwrseq->ops->free)
+ pwrseq->ops->free(host);
+
+ host->pwrseq = NULL;
+ }
- host->pwrseq = NULL;
}
+
+int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
+{
+ if (!pwrseq || !pwrseq->ops || !pwrseq->dev)
+ return -EINVAL;
+
+ mutex_lock(&pwrseq_list_mutex);
+ list_add(&pwrseq->list, &pwrseq_list);
+ mutex_unlock(&pwrseq_list_mutex);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mmc_pwrseq_register);
+
+void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
+{
+ if (pwrseq) {
+ mutex_lock(&pwrseq_list_mutex);
+ list_del(&pwrseq->list);
+ mutex_unlock(&pwrseq_list_mutex);
+ }
+}
+EXPORT_SYMBOL_GPL(mmc_pwrseq_unregister);
diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
index 096da48..08a7d2d 100644
--- a/drivers/mmc/core/pwrseq.h
+++ b/drivers/mmc/core/pwrseq.h
@@ -8,7 +8,10 @@
#ifndef _MMC_CORE_PWRSEQ_H
#define _MMC_CORE_PWRSEQ_H
+#include <linux/mmc/host.h>
+
struct mmc_pwrseq_ops {
+ int (*alloc)(struct mmc_host *host);
void (*pre_power_on)(struct mmc_host *host);
void (*post_power_on)(struct mmc_host *host);
void (*power_off)(struct mmc_host *host);
@@ -17,21 +20,21 @@ struct mmc_pwrseq_ops {
struct mmc_pwrseq {
struct mmc_pwrseq_ops *ops;
+ struct device *dev;
+ struct list_head list;
};
#ifdef CONFIG_OF
+int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
+void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
+
int mmc_pwrseq_alloc(struct mmc_host *host);
void mmc_pwrseq_pre_power_on(struct mmc_host *host);
void mmc_pwrseq_post_power_on(struct mmc_host *host);
void mmc_pwrseq_power_off(struct mmc_host *host);
void mmc_pwrseq_free(struct mmc_host *host);
-struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
- struct device *dev);
-struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
- struct device *dev);
-
#else
static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
index 7978fb7..75f7423 100644
--- a/drivers/mmc/core/pwrseq_emmc.c
+++ b/drivers/mmc/core/pwrseq_emmc.c
@@ -9,6 +9,7 @@
*/
#include <linux/delay.h>
#include <linux/kernel.h>
+#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/device.h>
#include <linux/err.h>
@@ -48,14 +49,8 @@ static void mmc_pwrseq_emmc_free(struct mmc_host *host)
unregister_restart_handler(&pwrseq->reset_nb);
gpiod_put(pwrseq->reset_gpio);
- kfree(pwrseq);
}
-static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
- .post_power_on = mmc_pwrseq_emmc_reset,
- .free = mmc_pwrseq_emmc_free,
-};
-
static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
unsigned long mode, void *cmd)
{
@@ -66,21 +61,14 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
return NOTIFY_DONE;
}
-struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
- struct device *dev)
+static int mmc_pwrseq_emmc_alloc(struct mmc_host *host)
{
- struct mmc_pwrseq_emmc *pwrseq;
- int ret = 0;
-
- pwrseq = kzalloc(sizeof(struct mmc_pwrseq_emmc), GFP_KERNEL);
- if (!pwrseq)
- return ERR_PTR(-ENOMEM);
+ struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
- pwrseq->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
- if (IS_ERR(pwrseq->reset_gpio)) {
- ret = PTR_ERR(pwrseq->reset_gpio);
- goto free;
- }
+ pwrseq->reset_gpio = gpiod_get(host->pwrseq->dev,
+ "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(pwrseq->reset_gpio))
+ return PTR_ERR(pwrseq->reset_gpio);
/*
* register reset handler to ensure emmc reset also from
@@ -91,10 +79,41 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
pwrseq->reset_nb.priority = 255;
register_restart_handler(&pwrseq->reset_nb);
+ return 0;
+}
+
+static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
+ .alloc = mmc_pwrseq_emmc_alloc,
+ .post_power_on = mmc_pwrseq_emmc_reset,
+ .free = mmc_pwrseq_emmc_free,
+};
+
+static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
+{
+ struct mmc_pwrseq_emmc *pwrseq;
+ struct device *dev = &pdev->dev;
+
+ pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
+ if (!pwrseq)
+ return -ENOMEM;
+
pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops;
+ pwrseq->pwrseq.dev = dev;
- return &pwrseq->pwrseq;
-free:
- kfree(pwrseq);
- return ERR_PTR(ret);
+ return mmc_pwrseq_register(&pwrseq->pwrseq);
}
+
+static const struct of_device_id mmc_pwrseq_emmc_of_match[] = {
+ { .compatible = "mmc-pwrseq-emmc",},
+ {/* sentinel */},
+};
+
+static struct platform_driver mmc_pwrseq_emmc_driver = {
+ .probe = mmc_pwrseq_emmc_probe,
+ .driver = {
+ .name = "pwrseq_emmc",
+ .of_match_table = mmc_pwrseq_emmc_of_match,
+ },
+};
+
+builtin_platform_driver(mmc_pwrseq_emmc_driver);
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index c091f98..b04cc2d 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -9,6 +9,7 @@
*/
#include <linux/clk.h>
#include <linux/kernel.h>
+#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/device.h>
#include <linux/err.h>
@@ -82,46 +83,66 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
if (!IS_ERR(pwrseq->ext_clk))
clk_put(pwrseq->ext_clk);
- kfree(pwrseq);
}
-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_power_off,
- .free = mmc_pwrseq_simple_free,
-};
-
-struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
- struct device *dev)
+int mmc_pwrseq_simple_alloc(struct mmc_host *host)
{
- struct mmc_pwrseq_simple *pwrseq;
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
+ struct device *dev = host->pwrseq->dev;
int ret = 0;
- pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL);
- if (!pwrseq)
- return ERR_PTR(-ENOMEM);
-
pwrseq->ext_clk = clk_get(dev, "ext_clock");
if (IS_ERR(pwrseq->ext_clk) &&
PTR_ERR(pwrseq->ext_clk) != -ENOENT) {
- ret = PTR_ERR(pwrseq->ext_clk);
- goto free;
+ return PTR_ERR(pwrseq->ext_clk);
+
}
pwrseq->reset_gpios = gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(pwrseq->reset_gpios)) {
ret = PTR_ERR(pwrseq->reset_gpios);
- goto clk_put;
+ clk_put(pwrseq->ext_clk);
+ return ret;
}
+ return 0;
+}
+
+static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
+ .alloc = mmc_pwrseq_simple_alloc,
+ .pre_power_on = mmc_pwrseq_simple_pre_power_on,
+ .post_power_on = mmc_pwrseq_simple_post_power_on,
+ .power_off = mmc_pwrseq_simple_power_off,
+ .free = mmc_pwrseq_simple_free,
+};
+
+static const struct of_device_id mmc_pwrseq_simple_of_match[] = {
+ { .compatible = "mmc-pwrseq-simple",},
+ {/* sentinel */},
+};
+
+static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
+{
+ struct mmc_pwrseq_simple *pwrseq;
+ struct device *dev = &pdev->dev;
+
+ pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
+ if (!pwrseq)
+ return ERR_PTR(-ENOMEM);
+
+ pwrseq->pwrseq.dev = dev;
pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
- return &pwrseq->pwrseq;
-clk_put:
- if (!IS_ERR(pwrseq->ext_clk))
- clk_put(pwrseq->ext_clk);
-free:
- kfree(pwrseq);
- return ERR_PTR(ret);
+ return mmc_pwrseq_register(&pwrseq->pwrseq);
}
+
+
+static struct platform_driver mmc_pwrseq_simple_driver = {
+ .probe = mmc_pwrseq_simple_probe,
+ .driver = {
+ .name = "pwrseq_simple",
+ .of_match_table = mmc_pwrseq_simple_of_match,
+ },
+};
+
+builtin_platform_driver(mmc_pwrseq_simple_driver);
--
1.9.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device
@ 2016-01-19 10:59 ` Srinivas Kandagatla
0 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 10:59 UTC (permalink / raw)
To: linux-arm-kernel
simple-pwrseq and emmc-pwrseq drivers rely on platform_device
structure from of_find_device_by_node(), this works mostly. But, as there
is no driver associated with this devices, cases like default/init pinctrl
setup would never be performed by pwrseq. This becomes problem when the
gpios used in pwrseq require pinctrl setup.
Currently most of the common pinctrl setup is done in
drivers/base/pinctrl.c by pinctrl_bind_pins().
There are two ways to slove this issue on either convert pwrseq drivers
to a proper platform drivers or copy the exact code from
pcintrl_bind_pins(). I prefer converting pwrseq to proper drivers so that
other cases like setting up clks/parents from dt would also be possible.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
drivers/mmc/core/pwrseq.h | 13 ++++--
drivers/mmc/core/pwrseq_emmc.c | 65 ++++++++++++++++----------
drivers/mmc/core/pwrseq_simple.c | 71 ++++++++++++++++++----------
4 files changed, 145 insertions(+), 103 deletions(-)
diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
index 4c1d175..669608e 100644
--- a/drivers/mmc/core/pwrseq.c
+++ b/drivers/mmc/core/pwrseq.c
@@ -8,50 +8,30 @@
* MMC power sequence management
*/
#include <linux/kernel.h>
-#include <linux/platform_device.h>
#include <linux/err.h>
#include <linux/of.h>
-#include <linux/of_platform.h>
#include <linux/mmc/host.h>
#include "pwrseq.h"
-struct mmc_pwrseq_match {
- const char *compatible;
- struct mmc_pwrseq *(*alloc)(struct mmc_host *host, struct device *dev);
-};
-
-static struct mmc_pwrseq_match pwrseq_match[] = {
- {
- .compatible = "mmc-pwrseq-simple",
- .alloc = mmc_pwrseq_simple_alloc,
- }, {
- .compatible = "mmc-pwrseq-emmc",
- .alloc = mmc_pwrseq_emmc_alloc,
- },
-};
-
-static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
+static DEFINE_MUTEX(pwrseq_list_mutex);
+static LIST_HEAD(pwrseq_list);
+
+static struct mmc_pwrseq *of_find_mmc_pwrseq(struct device_node *np)
{
- struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
- int i;
+ struct mmc_pwrseq *p;
- for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
- if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
- match = &pwrseq_match[i];
- break;
- }
- }
+ list_for_each_entry(p, &pwrseq_list, list)
+ if (p->dev->of_node == np)
+ return p;
- return match;
+ return NULL;
}
int mmc_pwrseq_alloc(struct mmc_host *host)
{
- struct platform_device *pdev;
struct device_node *np;
- struct mmc_pwrseq_match *match;
struct mmc_pwrseq *pwrseq;
int ret = 0;
@@ -59,25 +39,18 @@ int mmc_pwrseq_alloc(struct mmc_host *host)
if (!np)
return 0;
- pdev = of_find_device_by_node(np);
- if (!pdev) {
- ret = -ENODEV;
- goto err;
- }
+ pwrseq = of_find_mmc_pwrseq(np);
- match = mmc_pwrseq_find(np);
- if (IS_ERR(match)) {
- ret = PTR_ERR(match);
- goto err;
- }
+ if (pwrseq && pwrseq->ops && pwrseq->ops->alloc) {
+ host->pwrseq = pwrseq;
+ ret = pwrseq->ops->alloc(host);
+ if (IS_ERR_VALUE(ret)) {
+ host->pwrseq = NULL;
+ goto err;
+ }
- pwrseq = match->alloc(host, &pdev->dev);
- if (IS_ERR(pwrseq)) {
- ret = PTR_ERR(pwrseq);
- goto err;
}
- host->pwrseq = pwrseq;
dev_info(host->parent, "allocated mmc-pwrseq\n");
err:
@@ -89,7 +62,7 @@ void mmc_pwrseq_pre_power_on(struct mmc_host *host)
{
struct mmc_pwrseq *pwrseq = host->pwrseq;
- if (pwrseq && pwrseq->ops && pwrseq->ops->pre_power_on)
+ if (pwrseq && pwrseq->ops->pre_power_on)
pwrseq->ops->pre_power_on(host);
}
@@ -97,7 +70,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host)
{
struct mmc_pwrseq *pwrseq = host->pwrseq;
- if (pwrseq && pwrseq->ops && pwrseq->ops->post_power_on)
+ if (pwrseq && pwrseq->ops->post_power_on)
pwrseq->ops->post_power_on(host);
}
@@ -105,7 +78,7 @@ void mmc_pwrseq_power_off(struct mmc_host *host)
{
struct mmc_pwrseq *pwrseq = host->pwrseq;
- if (pwrseq && pwrseq->ops && pwrseq->ops->power_off)
+ if (pwrseq && pwrseq->ops->power_off)
pwrseq->ops->power_off(host);
}
@@ -113,8 +86,34 @@ void mmc_pwrseq_free(struct mmc_host *host)
{
struct mmc_pwrseq *pwrseq = host->pwrseq;
- if (pwrseq && pwrseq->ops && pwrseq->ops->free)
- pwrseq->ops->free(host);
+ if (pwrseq) {
+ if (pwrseq->ops->free)
+ pwrseq->ops->free(host);
+
+ host->pwrseq = NULL;
+ }
- host->pwrseq = NULL;
}
+
+int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
+{
+ if (!pwrseq || !pwrseq->ops || !pwrseq->dev)
+ return -EINVAL;
+
+ mutex_lock(&pwrseq_list_mutex);
+ list_add(&pwrseq->list, &pwrseq_list);
+ mutex_unlock(&pwrseq_list_mutex);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mmc_pwrseq_register);
+
+void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
+{
+ if (pwrseq) {
+ mutex_lock(&pwrseq_list_mutex);
+ list_del(&pwrseq->list);
+ mutex_unlock(&pwrseq_list_mutex);
+ }
+}
+EXPORT_SYMBOL_GPL(mmc_pwrseq_unregister);
diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
index 096da48..08a7d2d 100644
--- a/drivers/mmc/core/pwrseq.h
+++ b/drivers/mmc/core/pwrseq.h
@@ -8,7 +8,10 @@
#ifndef _MMC_CORE_PWRSEQ_H
#define _MMC_CORE_PWRSEQ_H
+#include <linux/mmc/host.h>
+
struct mmc_pwrseq_ops {
+ int (*alloc)(struct mmc_host *host);
void (*pre_power_on)(struct mmc_host *host);
void (*post_power_on)(struct mmc_host *host);
void (*power_off)(struct mmc_host *host);
@@ -17,21 +20,21 @@ struct mmc_pwrseq_ops {
struct mmc_pwrseq {
struct mmc_pwrseq_ops *ops;
+ struct device *dev;
+ struct list_head list;
};
#ifdef CONFIG_OF
+int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
+void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
+
int mmc_pwrseq_alloc(struct mmc_host *host);
void mmc_pwrseq_pre_power_on(struct mmc_host *host);
void mmc_pwrseq_post_power_on(struct mmc_host *host);
void mmc_pwrseq_power_off(struct mmc_host *host);
void mmc_pwrseq_free(struct mmc_host *host);
-struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
- struct device *dev);
-struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
- struct device *dev);
-
#else
static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
index 7978fb7..75f7423 100644
--- a/drivers/mmc/core/pwrseq_emmc.c
+++ b/drivers/mmc/core/pwrseq_emmc.c
@@ -9,6 +9,7 @@
*/
#include <linux/delay.h>
#include <linux/kernel.h>
+#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/device.h>
#include <linux/err.h>
@@ -48,14 +49,8 @@ static void mmc_pwrseq_emmc_free(struct mmc_host *host)
unregister_restart_handler(&pwrseq->reset_nb);
gpiod_put(pwrseq->reset_gpio);
- kfree(pwrseq);
}
-static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
- .post_power_on = mmc_pwrseq_emmc_reset,
- .free = mmc_pwrseq_emmc_free,
-};
-
static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
unsigned long mode, void *cmd)
{
@@ -66,21 +61,14 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
return NOTIFY_DONE;
}
-struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
- struct device *dev)
+static int mmc_pwrseq_emmc_alloc(struct mmc_host *host)
{
- struct mmc_pwrseq_emmc *pwrseq;
- int ret = 0;
-
- pwrseq = kzalloc(sizeof(struct mmc_pwrseq_emmc), GFP_KERNEL);
- if (!pwrseq)
- return ERR_PTR(-ENOMEM);
+ struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
- pwrseq->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
- if (IS_ERR(pwrseq->reset_gpio)) {
- ret = PTR_ERR(pwrseq->reset_gpio);
- goto free;
- }
+ pwrseq->reset_gpio = gpiod_get(host->pwrseq->dev,
+ "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(pwrseq->reset_gpio))
+ return PTR_ERR(pwrseq->reset_gpio);
/*
* register reset handler to ensure emmc reset also from
@@ -91,10 +79,41 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
pwrseq->reset_nb.priority = 255;
register_restart_handler(&pwrseq->reset_nb);
+ return 0;
+}
+
+static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
+ .alloc = mmc_pwrseq_emmc_alloc,
+ .post_power_on = mmc_pwrseq_emmc_reset,
+ .free = mmc_pwrseq_emmc_free,
+};
+
+static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
+{
+ struct mmc_pwrseq_emmc *pwrseq;
+ struct device *dev = &pdev->dev;
+
+ pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
+ if (!pwrseq)
+ return -ENOMEM;
+
pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops;
+ pwrseq->pwrseq.dev = dev;
- return &pwrseq->pwrseq;
-free:
- kfree(pwrseq);
- return ERR_PTR(ret);
+ return mmc_pwrseq_register(&pwrseq->pwrseq);
}
+
+static const struct of_device_id mmc_pwrseq_emmc_of_match[] = {
+ { .compatible = "mmc-pwrseq-emmc",},
+ {/* sentinel */},
+};
+
+static struct platform_driver mmc_pwrseq_emmc_driver = {
+ .probe = mmc_pwrseq_emmc_probe,
+ .driver = {
+ .name = "pwrseq_emmc",
+ .of_match_table = mmc_pwrseq_emmc_of_match,
+ },
+};
+
+builtin_platform_driver(mmc_pwrseq_emmc_driver);
diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index c091f98..b04cc2d 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -9,6 +9,7 @@
*/
#include <linux/clk.h>
#include <linux/kernel.h>
+#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/device.h>
#include <linux/err.h>
@@ -82,46 +83,66 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
if (!IS_ERR(pwrseq->ext_clk))
clk_put(pwrseq->ext_clk);
- kfree(pwrseq);
}
-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_power_off,
- .free = mmc_pwrseq_simple_free,
-};
-
-struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
- struct device *dev)
+int mmc_pwrseq_simple_alloc(struct mmc_host *host)
{
- struct mmc_pwrseq_simple *pwrseq;
+ struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
+ struct device *dev = host->pwrseq->dev;
int ret = 0;
- pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL);
- if (!pwrseq)
- return ERR_PTR(-ENOMEM);
-
pwrseq->ext_clk = clk_get(dev, "ext_clock");
if (IS_ERR(pwrseq->ext_clk) &&
PTR_ERR(pwrseq->ext_clk) != -ENOENT) {
- ret = PTR_ERR(pwrseq->ext_clk);
- goto free;
+ return PTR_ERR(pwrseq->ext_clk);
+
}
pwrseq->reset_gpios = gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(pwrseq->reset_gpios)) {
ret = PTR_ERR(pwrseq->reset_gpios);
- goto clk_put;
+ clk_put(pwrseq->ext_clk);
+ return ret;
}
+ return 0;
+}
+
+static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
+ .alloc = mmc_pwrseq_simple_alloc,
+ .pre_power_on = mmc_pwrseq_simple_pre_power_on,
+ .post_power_on = mmc_pwrseq_simple_post_power_on,
+ .power_off = mmc_pwrseq_simple_power_off,
+ .free = mmc_pwrseq_simple_free,
+};
+
+static const struct of_device_id mmc_pwrseq_simple_of_match[] = {
+ { .compatible = "mmc-pwrseq-simple",},
+ {/* sentinel */},
+};
+
+static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
+{
+ struct mmc_pwrseq_simple *pwrseq;
+ struct device *dev = &pdev->dev;
+
+ pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
+ if (!pwrseq)
+ return ERR_PTR(-ENOMEM);
+
+ pwrseq->pwrseq.dev = dev;
pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
- return &pwrseq->pwrseq;
-clk_put:
- if (!IS_ERR(pwrseq->ext_clk))
- clk_put(pwrseq->ext_clk);
-free:
- kfree(pwrseq);
- return ERR_PTR(ret);
+ return mmc_pwrseq_register(&pwrseq->pwrseq);
}
+
+
+static struct platform_driver mmc_pwrseq_simple_driver = {
+ .probe = mmc_pwrseq_simple_probe,
+ .driver = {
+ .name = "pwrseq_simple",
+ .of_match_table = mmc_pwrseq_simple_of_match,
+ },
+};
+
+builtin_platform_driver(mmc_pwrseq_simple_driver);
--
1.9.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device
2016-01-19 10:59 ` Srinivas Kandagatla
(?)
@ 2016-01-19 13:01 ` Ulf Hansson
-1 siblings, 0 replies; 14+ messages in thread
From: Ulf Hansson @ 2016-01-19 13:01 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: linux-mmc, linux-kernel, linux-arm-kernel
On 19 January 2016 at 11:59, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> simple-pwrseq and emmc-pwrseq drivers rely on platform_device
> structure from of_find_device_by_node(), this works mostly. But, as there
> is no driver associated with this devices, cases like default/init pinctrl
> setup would never be performed by pwrseq. This becomes problem when the
> gpios used in pwrseq require pinctrl setup.
>
> Currently most of the common pinctrl setup is done in
> drivers/base/pinctrl.c by pinctrl_bind_pins().
>
> There are two ways to slove this issue on either convert pwrseq drivers
> to a proper platform drivers or copy the exact code from
> pcintrl_bind_pins(). I prefer converting pwrseq to proper drivers so that
> other cases like setting up clks/parents from dt would also be possible.
Overall I like the approach, thanks for posting this!
I have browsed through the patch, it looks okay, besides one issue...
There is a possibility that the pwrseq drivers haven't been probed
before mmc_pwrseq_alloc() gets called. This leads to that the
pwrseq_list may be empty, which means that we would silently fail to
find a corresponding pwrseq type even if the device node would say
there should be one.
Perhaps you should do a pre-validation of the device node in
mmc_pwrseq_alloc(), to see if there is a pwrseq handle. If so, but no
corresponding pwrseq method registerd (because the driver hasn’t been
probed yet), you could return -EPROBE_DEFER. Or perhaps there are
other alternatives to solved this issue!?
Kind regards
Uffe
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
> drivers/mmc/core/pwrseq.h | 13 ++++--
> drivers/mmc/core/pwrseq_emmc.c | 65 ++++++++++++++++----------
> drivers/mmc/core/pwrseq_simple.c | 71 ++++++++++++++++++----------
> 4 files changed, 145 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
> index 4c1d175..669608e 100644
> --- a/drivers/mmc/core/pwrseq.c
> +++ b/drivers/mmc/core/pwrseq.c
> @@ -8,50 +8,30 @@
> * MMC power sequence management
> */
> #include <linux/kernel.h>
> -#include <linux/platform_device.h>
> #include <linux/err.h>
> #include <linux/of.h>
> -#include <linux/of_platform.h>
>
> #include <linux/mmc/host.h>
>
> #include "pwrseq.h"
>
> -struct mmc_pwrseq_match {
> - const char *compatible;
> - struct mmc_pwrseq *(*alloc)(struct mmc_host *host, struct device *dev);
> -};
> -
> -static struct mmc_pwrseq_match pwrseq_match[] = {
> - {
> - .compatible = "mmc-pwrseq-simple",
> - .alloc = mmc_pwrseq_simple_alloc,
> - }, {
> - .compatible = "mmc-pwrseq-emmc",
> - .alloc = mmc_pwrseq_emmc_alloc,
> - },
> -};
> -
> -static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
> +static DEFINE_MUTEX(pwrseq_list_mutex);
> +static LIST_HEAD(pwrseq_list);
> +
> +static struct mmc_pwrseq *of_find_mmc_pwrseq(struct device_node *np)
> {
> - struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
> - int i;
> + struct mmc_pwrseq *p;
>
> - for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
> - if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
> - match = &pwrseq_match[i];
> - break;
> - }
> - }
> + list_for_each_entry(p, &pwrseq_list, list)
> + if (p->dev->of_node == np)
> + return p;
>
> - return match;
> + return NULL;
> }
>
> int mmc_pwrseq_alloc(struct mmc_host *host)
> {
> - struct platform_device *pdev;
> struct device_node *np;
> - struct mmc_pwrseq_match *match;
> struct mmc_pwrseq *pwrseq;
> int ret = 0;
>
> @@ -59,25 +39,18 @@ int mmc_pwrseq_alloc(struct mmc_host *host)
> if (!np)
> return 0;
>
> - pdev = of_find_device_by_node(np);
> - if (!pdev) {
> - ret = -ENODEV;
> - goto err;
> - }
> + pwrseq = of_find_mmc_pwrseq(np);
>
> - match = mmc_pwrseq_find(np);
> - if (IS_ERR(match)) {
> - ret = PTR_ERR(match);
> - goto err;
> - }
> + if (pwrseq && pwrseq->ops && pwrseq->ops->alloc) {
> + host->pwrseq = pwrseq;
> + ret = pwrseq->ops->alloc(host);
> + if (IS_ERR_VALUE(ret)) {
> + host->pwrseq = NULL;
> + goto err;
> + }
>
> - pwrseq = match->alloc(host, &pdev->dev);
> - if (IS_ERR(pwrseq)) {
> - ret = PTR_ERR(pwrseq);
> - goto err;
> }
>
> - host->pwrseq = pwrseq;
> dev_info(host->parent, "allocated mmc-pwrseq\n");
>
> err:
> @@ -89,7 +62,7 @@ void mmc_pwrseq_pre_power_on(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->pre_power_on)
> + if (pwrseq && pwrseq->ops->pre_power_on)
> pwrseq->ops->pre_power_on(host);
> }
>
> @@ -97,7 +70,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->post_power_on)
> + if (pwrseq && pwrseq->ops->post_power_on)
> pwrseq->ops->post_power_on(host);
> }
>
> @@ -105,7 +78,7 @@ void mmc_pwrseq_power_off(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->power_off)
> + if (pwrseq && pwrseq->ops->power_off)
> pwrseq->ops->power_off(host);
> }
>
> @@ -113,8 +86,34 @@ void mmc_pwrseq_free(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->free)
> - pwrseq->ops->free(host);
> + if (pwrseq) {
> + if (pwrseq->ops->free)
> + pwrseq->ops->free(host);
> +
> + host->pwrseq = NULL;
> + }
>
> - host->pwrseq = NULL;
> }
> +
> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
> +{
> + if (!pwrseq || !pwrseq->ops || !pwrseq->dev)
> + return -EINVAL;
> +
> + mutex_lock(&pwrseq_list_mutex);
> + list_add(&pwrseq->list, &pwrseq_list);
> + mutex_unlock(&pwrseq_list_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mmc_pwrseq_register);
> +
> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
> +{
> + if (pwrseq) {
> + mutex_lock(&pwrseq_list_mutex);
> + list_del(&pwrseq->list);
> + mutex_unlock(&pwrseq_list_mutex);
> + }
> +}
> +EXPORT_SYMBOL_GPL(mmc_pwrseq_unregister);
> diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
> index 096da48..08a7d2d 100644
> --- a/drivers/mmc/core/pwrseq.h
> +++ b/drivers/mmc/core/pwrseq.h
> @@ -8,7 +8,10 @@
> #ifndef _MMC_CORE_PWRSEQ_H
> #define _MMC_CORE_PWRSEQ_H
>
> +#include <linux/mmc/host.h>
> +
> struct mmc_pwrseq_ops {
> + int (*alloc)(struct mmc_host *host);
> void (*pre_power_on)(struct mmc_host *host);
> void (*post_power_on)(struct mmc_host *host);
> void (*power_off)(struct mmc_host *host);
> @@ -17,21 +20,21 @@ struct mmc_pwrseq_ops {
>
> struct mmc_pwrseq {
> struct mmc_pwrseq_ops *ops;
> + struct device *dev;
> + struct list_head list;
> };
>
> #ifdef CONFIG_OF
>
> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
> +
> int mmc_pwrseq_alloc(struct mmc_host *host);
> void mmc_pwrseq_pre_power_on(struct mmc_host *host);
> void mmc_pwrseq_post_power_on(struct mmc_host *host);
> void mmc_pwrseq_power_off(struct mmc_host *host);
> void mmc_pwrseq_free(struct mmc_host *host);
>
> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
> - struct device *dev);
> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
> - struct device *dev);
> -
> #else
>
> static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
> index 7978fb7..75f7423 100644
> --- a/drivers/mmc/core/pwrseq_emmc.c
> +++ b/drivers/mmc/core/pwrseq_emmc.c
> @@ -9,6 +9,7 @@
> */
> #include <linux/delay.h>
> #include <linux/kernel.h>
> +#include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -48,14 +49,8 @@ static void mmc_pwrseq_emmc_free(struct mmc_host *host)
>
> unregister_restart_handler(&pwrseq->reset_nb);
> gpiod_put(pwrseq->reset_gpio);
> - kfree(pwrseq);
> }
>
> -static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
> - .post_power_on = mmc_pwrseq_emmc_reset,
> - .free = mmc_pwrseq_emmc_free,
> -};
> -
> static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
> unsigned long mode, void *cmd)
> {
> @@ -66,21 +61,14 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
> return NOTIFY_DONE;
> }
>
> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
> - struct device *dev)
> +static int mmc_pwrseq_emmc_alloc(struct mmc_host *host)
> {
> - struct mmc_pwrseq_emmc *pwrseq;
> - int ret = 0;
> -
> - pwrseq = kzalloc(sizeof(struct mmc_pwrseq_emmc), GFP_KERNEL);
> - if (!pwrseq)
> - return ERR_PTR(-ENOMEM);
> + struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
>
> - pwrseq->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> - if (IS_ERR(pwrseq->reset_gpio)) {
> - ret = PTR_ERR(pwrseq->reset_gpio);
> - goto free;
> - }
> + pwrseq->reset_gpio = gpiod_get(host->pwrseq->dev,
> + "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(pwrseq->reset_gpio))
> + return PTR_ERR(pwrseq->reset_gpio);
>
> /*
> * register reset handler to ensure emmc reset also from
> @@ -91,10 +79,41 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
> pwrseq->reset_nb.priority = 255;
> register_restart_handler(&pwrseq->reset_nb);
>
> + return 0;
> +}
> +
> +static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
> + .alloc = mmc_pwrseq_emmc_alloc,
> + .post_power_on = mmc_pwrseq_emmc_reset,
> + .free = mmc_pwrseq_emmc_free,
> +};
> +
> +static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
> +{
> + struct mmc_pwrseq_emmc *pwrseq;
> + struct device *dev = &pdev->dev;
> +
> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
> + if (!pwrseq)
> + return -ENOMEM;
> +
> pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops;
> + pwrseq->pwrseq.dev = dev;
>
> - return &pwrseq->pwrseq;
> -free:
> - kfree(pwrseq);
> - return ERR_PTR(ret);
> + return mmc_pwrseq_register(&pwrseq->pwrseq);
> }
> +
> +static const struct of_device_id mmc_pwrseq_emmc_of_match[] = {
> + { .compatible = "mmc-pwrseq-emmc",},
> + {/* sentinel */},
> +};
> +
> +static struct platform_driver mmc_pwrseq_emmc_driver = {
> + .probe = mmc_pwrseq_emmc_probe,
> + .driver = {
> + .name = "pwrseq_emmc",
> + .of_match_table = mmc_pwrseq_emmc_of_match,
> + },
> +};
> +
> +builtin_platform_driver(mmc_pwrseq_emmc_driver);
> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
> index c091f98..b04cc2d 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -9,6 +9,7 @@
> */
> #include <linux/clk.h>
> #include <linux/kernel.h>
> +#include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -82,46 +83,66 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
> if (!IS_ERR(pwrseq->ext_clk))
> clk_put(pwrseq->ext_clk);
>
> - kfree(pwrseq);
> }
>
> -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_power_off,
> - .free = mmc_pwrseq_simple_free,
> -};
> -
> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
> - struct device *dev)
> +int mmc_pwrseq_simple_alloc(struct mmc_host *host)
> {
> - struct mmc_pwrseq_simple *pwrseq;
> + struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
> + struct device *dev = host->pwrseq->dev;
> int ret = 0;
>
> - pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL);
> - if (!pwrseq)
> - return ERR_PTR(-ENOMEM);
> -
> pwrseq->ext_clk = clk_get(dev, "ext_clock");
> if (IS_ERR(pwrseq->ext_clk) &&
> PTR_ERR(pwrseq->ext_clk) != -ENOENT) {
> - ret = PTR_ERR(pwrseq->ext_clk);
> - goto free;
> + return PTR_ERR(pwrseq->ext_clk);
> +
> }
>
> pwrseq->reset_gpios = gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
> if (IS_ERR(pwrseq->reset_gpios)) {
> ret = PTR_ERR(pwrseq->reset_gpios);
> - goto clk_put;
> + clk_put(pwrseq->ext_clk);
> + return ret;
> }
>
> + return 0;
> +}
> +
> +static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
> + .alloc = mmc_pwrseq_simple_alloc,
> + .pre_power_on = mmc_pwrseq_simple_pre_power_on,
> + .post_power_on = mmc_pwrseq_simple_post_power_on,
> + .power_off = mmc_pwrseq_simple_power_off,
> + .free = mmc_pwrseq_simple_free,
> +};
> +
> +static const struct of_device_id mmc_pwrseq_simple_of_match[] = {
> + { .compatible = "mmc-pwrseq-simple",},
> + {/* sentinel */},
> +};
> +
> +static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
> +{
> + struct mmc_pwrseq_simple *pwrseq;
> + struct device *dev = &pdev->dev;
> +
> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
> + if (!pwrseq)
> + return ERR_PTR(-ENOMEM);
> +
> + pwrseq->pwrseq.dev = dev;
> pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
>
> - return &pwrseq->pwrseq;
> -clk_put:
> - if (!IS_ERR(pwrseq->ext_clk))
> - clk_put(pwrseq->ext_clk);
> -free:
> - kfree(pwrseq);
> - return ERR_PTR(ret);
> + return mmc_pwrseq_register(&pwrseq->pwrseq);
> }
> +
> +
> +static struct platform_driver mmc_pwrseq_simple_driver = {
> + .probe = mmc_pwrseq_simple_probe,
> + .driver = {
> + .name = "pwrseq_simple",
> + .of_match_table = mmc_pwrseq_simple_of_match,
> + },
> +};
> +
> +builtin_platform_driver(mmc_pwrseq_simple_driver);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device
@ 2016-01-19 13:01 ` Ulf Hansson
0 siblings, 0 replies; 14+ messages in thread
From: Ulf Hansson @ 2016-01-19 13:01 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: linux-mmc, linux-kernel, linux-arm-kernel
On 19 January 2016 at 11:59, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> simple-pwrseq and emmc-pwrseq drivers rely on platform_device
> structure from of_find_device_by_node(), this works mostly. But, as there
> is no driver associated with this devices, cases like default/init pinctrl
> setup would never be performed by pwrseq. This becomes problem when the
> gpios used in pwrseq require pinctrl setup.
>
> Currently most of the common pinctrl setup is done in
> drivers/base/pinctrl.c by pinctrl_bind_pins().
>
> There are two ways to slove this issue on either convert pwrseq drivers
> to a proper platform drivers or copy the exact code from
> pcintrl_bind_pins(). I prefer converting pwrseq to proper drivers so that
> other cases like setting up clks/parents from dt would also be possible.
Overall I like the approach, thanks for posting this!
I have browsed through the patch, it looks okay, besides one issue...
There is a possibility that the pwrseq drivers haven't been probed
before mmc_pwrseq_alloc() gets called. This leads to that the
pwrseq_list may be empty, which means that we would silently fail to
find a corresponding pwrseq type even if the device node would say
there should be one.
Perhaps you should do a pre-validation of the device node in
mmc_pwrseq_alloc(), to see if there is a pwrseq handle. If so, but no
corresponding pwrseq method registerd (because the driver hasn’t been
probed yet), you could return -EPROBE_DEFER. Or perhaps there are
other alternatives to solved this issue!?
Kind regards
Uffe
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
> drivers/mmc/core/pwrseq.h | 13 ++++--
> drivers/mmc/core/pwrseq_emmc.c | 65 ++++++++++++++++----------
> drivers/mmc/core/pwrseq_simple.c | 71 ++++++++++++++++++----------
> 4 files changed, 145 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
> index 4c1d175..669608e 100644
> --- a/drivers/mmc/core/pwrseq.c
> +++ b/drivers/mmc/core/pwrseq.c
> @@ -8,50 +8,30 @@
> * MMC power sequence management
> */
> #include <linux/kernel.h>
> -#include <linux/platform_device.h>
> #include <linux/err.h>
> #include <linux/of.h>
> -#include <linux/of_platform.h>
>
> #include <linux/mmc/host.h>
>
> #include "pwrseq.h"
>
> -struct mmc_pwrseq_match {
> - const char *compatible;
> - struct mmc_pwrseq *(*alloc)(struct mmc_host *host, struct device *dev);
> -};
> -
> -static struct mmc_pwrseq_match pwrseq_match[] = {
> - {
> - .compatible = "mmc-pwrseq-simple",
> - .alloc = mmc_pwrseq_simple_alloc,
> - }, {
> - .compatible = "mmc-pwrseq-emmc",
> - .alloc = mmc_pwrseq_emmc_alloc,
> - },
> -};
> -
> -static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
> +static DEFINE_MUTEX(pwrseq_list_mutex);
> +static LIST_HEAD(pwrseq_list);
> +
> +static struct mmc_pwrseq *of_find_mmc_pwrseq(struct device_node *np)
> {
> - struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
> - int i;
> + struct mmc_pwrseq *p;
>
> - for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
> - if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
> - match = &pwrseq_match[i];
> - break;
> - }
> - }
> + list_for_each_entry(p, &pwrseq_list, list)
> + if (p->dev->of_node == np)
> + return p;
>
> - return match;
> + return NULL;
> }
>
> int mmc_pwrseq_alloc(struct mmc_host *host)
> {
> - struct platform_device *pdev;
> struct device_node *np;
> - struct mmc_pwrseq_match *match;
> struct mmc_pwrseq *pwrseq;
> int ret = 0;
>
> @@ -59,25 +39,18 @@ int mmc_pwrseq_alloc(struct mmc_host *host)
> if (!np)
> return 0;
>
> - pdev = of_find_device_by_node(np);
> - if (!pdev) {
> - ret = -ENODEV;
> - goto err;
> - }
> + pwrseq = of_find_mmc_pwrseq(np);
>
> - match = mmc_pwrseq_find(np);
> - if (IS_ERR(match)) {
> - ret = PTR_ERR(match);
> - goto err;
> - }
> + if (pwrseq && pwrseq->ops && pwrseq->ops->alloc) {
> + host->pwrseq = pwrseq;
> + ret = pwrseq->ops->alloc(host);
> + if (IS_ERR_VALUE(ret)) {
> + host->pwrseq = NULL;
> + goto err;
> + }
>
> - pwrseq = match->alloc(host, &pdev->dev);
> - if (IS_ERR(pwrseq)) {
> - ret = PTR_ERR(pwrseq);
> - goto err;
> }
>
> - host->pwrseq = pwrseq;
> dev_info(host->parent, "allocated mmc-pwrseq\n");
>
> err:
> @@ -89,7 +62,7 @@ void mmc_pwrseq_pre_power_on(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->pre_power_on)
> + if (pwrseq && pwrseq->ops->pre_power_on)
> pwrseq->ops->pre_power_on(host);
> }
>
> @@ -97,7 +70,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->post_power_on)
> + if (pwrseq && pwrseq->ops->post_power_on)
> pwrseq->ops->post_power_on(host);
> }
>
> @@ -105,7 +78,7 @@ void mmc_pwrseq_power_off(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->power_off)
> + if (pwrseq && pwrseq->ops->power_off)
> pwrseq->ops->power_off(host);
> }
>
> @@ -113,8 +86,34 @@ void mmc_pwrseq_free(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->free)
> - pwrseq->ops->free(host);
> + if (pwrseq) {
> + if (pwrseq->ops->free)
> + pwrseq->ops->free(host);
> +
> + host->pwrseq = NULL;
> + }
>
> - host->pwrseq = NULL;
> }
> +
> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
> +{
> + if (!pwrseq || !pwrseq->ops || !pwrseq->dev)
> + return -EINVAL;
> +
> + mutex_lock(&pwrseq_list_mutex);
> + list_add(&pwrseq->list, &pwrseq_list);
> + mutex_unlock(&pwrseq_list_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mmc_pwrseq_register);
> +
> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
> +{
> + if (pwrseq) {
> + mutex_lock(&pwrseq_list_mutex);
> + list_del(&pwrseq->list);
> + mutex_unlock(&pwrseq_list_mutex);
> + }
> +}
> +EXPORT_SYMBOL_GPL(mmc_pwrseq_unregister);
> diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
> index 096da48..08a7d2d 100644
> --- a/drivers/mmc/core/pwrseq.h
> +++ b/drivers/mmc/core/pwrseq.h
> @@ -8,7 +8,10 @@
> #ifndef _MMC_CORE_PWRSEQ_H
> #define _MMC_CORE_PWRSEQ_H
>
> +#include <linux/mmc/host.h>
> +
> struct mmc_pwrseq_ops {
> + int (*alloc)(struct mmc_host *host);
> void (*pre_power_on)(struct mmc_host *host);
> void (*post_power_on)(struct mmc_host *host);
> void (*power_off)(struct mmc_host *host);
> @@ -17,21 +20,21 @@ struct mmc_pwrseq_ops {
>
> struct mmc_pwrseq {
> struct mmc_pwrseq_ops *ops;
> + struct device *dev;
> + struct list_head list;
> };
>
> #ifdef CONFIG_OF
>
> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
> +
> int mmc_pwrseq_alloc(struct mmc_host *host);
> void mmc_pwrseq_pre_power_on(struct mmc_host *host);
> void mmc_pwrseq_post_power_on(struct mmc_host *host);
> void mmc_pwrseq_power_off(struct mmc_host *host);
> void mmc_pwrseq_free(struct mmc_host *host);
>
> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
> - struct device *dev);
> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
> - struct device *dev);
> -
> #else
>
> static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
> index 7978fb7..75f7423 100644
> --- a/drivers/mmc/core/pwrseq_emmc.c
> +++ b/drivers/mmc/core/pwrseq_emmc.c
> @@ -9,6 +9,7 @@
> */
> #include <linux/delay.h>
> #include <linux/kernel.h>
> +#include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -48,14 +49,8 @@ static void mmc_pwrseq_emmc_free(struct mmc_host *host)
>
> unregister_restart_handler(&pwrseq->reset_nb);
> gpiod_put(pwrseq->reset_gpio);
> - kfree(pwrseq);
> }
>
> -static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
> - .post_power_on = mmc_pwrseq_emmc_reset,
> - .free = mmc_pwrseq_emmc_free,
> -};
> -
> static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
> unsigned long mode, void *cmd)
> {
> @@ -66,21 +61,14 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
> return NOTIFY_DONE;
> }
>
> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
> - struct device *dev)
> +static int mmc_pwrseq_emmc_alloc(struct mmc_host *host)
> {
> - struct mmc_pwrseq_emmc *pwrseq;
> - int ret = 0;
> -
> - pwrseq = kzalloc(sizeof(struct mmc_pwrseq_emmc), GFP_KERNEL);
> - if (!pwrseq)
> - return ERR_PTR(-ENOMEM);
> + struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
>
> - pwrseq->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> - if (IS_ERR(pwrseq->reset_gpio)) {
> - ret = PTR_ERR(pwrseq->reset_gpio);
> - goto free;
> - }
> + pwrseq->reset_gpio = gpiod_get(host->pwrseq->dev,
> + "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(pwrseq->reset_gpio))
> + return PTR_ERR(pwrseq->reset_gpio);
>
> /*
> * register reset handler to ensure emmc reset also from
> @@ -91,10 +79,41 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
> pwrseq->reset_nb.priority = 255;
> register_restart_handler(&pwrseq->reset_nb);
>
> + return 0;
> +}
> +
> +static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
> + .alloc = mmc_pwrseq_emmc_alloc,
> + .post_power_on = mmc_pwrseq_emmc_reset,
> + .free = mmc_pwrseq_emmc_free,
> +};
> +
> +static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
> +{
> + struct mmc_pwrseq_emmc *pwrseq;
> + struct device *dev = &pdev->dev;
> +
> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
> + if (!pwrseq)
> + return -ENOMEM;
> +
> pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops;
> + pwrseq->pwrseq.dev = dev;
>
> - return &pwrseq->pwrseq;
> -free:
> - kfree(pwrseq);
> - return ERR_PTR(ret);
> + return mmc_pwrseq_register(&pwrseq->pwrseq);
> }
> +
> +static const struct of_device_id mmc_pwrseq_emmc_of_match[] = {
> + { .compatible = "mmc-pwrseq-emmc",},
> + {/* sentinel */},
> +};
> +
> +static struct platform_driver mmc_pwrseq_emmc_driver = {
> + .probe = mmc_pwrseq_emmc_probe,
> + .driver = {
> + .name = "pwrseq_emmc",
> + .of_match_table = mmc_pwrseq_emmc_of_match,
> + },
> +};
> +
> +builtin_platform_driver(mmc_pwrseq_emmc_driver);
> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
> index c091f98..b04cc2d 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -9,6 +9,7 @@
> */
> #include <linux/clk.h>
> #include <linux/kernel.h>
> +#include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -82,46 +83,66 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
> if (!IS_ERR(pwrseq->ext_clk))
> clk_put(pwrseq->ext_clk);
>
> - kfree(pwrseq);
> }
>
> -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_power_off,
> - .free = mmc_pwrseq_simple_free,
> -};
> -
> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
> - struct device *dev)
> +int mmc_pwrseq_simple_alloc(struct mmc_host *host)
> {
> - struct mmc_pwrseq_simple *pwrseq;
> + struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
> + struct device *dev = host->pwrseq->dev;
> int ret = 0;
>
> - pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL);
> - if (!pwrseq)
> - return ERR_PTR(-ENOMEM);
> -
> pwrseq->ext_clk = clk_get(dev, "ext_clock");
> if (IS_ERR(pwrseq->ext_clk) &&
> PTR_ERR(pwrseq->ext_clk) != -ENOENT) {
> - ret = PTR_ERR(pwrseq->ext_clk);
> - goto free;
> + return PTR_ERR(pwrseq->ext_clk);
> +
> }
>
> pwrseq->reset_gpios = gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
> if (IS_ERR(pwrseq->reset_gpios)) {
> ret = PTR_ERR(pwrseq->reset_gpios);
> - goto clk_put;
> + clk_put(pwrseq->ext_clk);
> + return ret;
> }
>
> + return 0;
> +}
> +
> +static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
> + .alloc = mmc_pwrseq_simple_alloc,
> + .pre_power_on = mmc_pwrseq_simple_pre_power_on,
> + .post_power_on = mmc_pwrseq_simple_post_power_on,
> + .power_off = mmc_pwrseq_simple_power_off,
> + .free = mmc_pwrseq_simple_free,
> +};
> +
> +static const struct of_device_id mmc_pwrseq_simple_of_match[] = {
> + { .compatible = "mmc-pwrseq-simple",},
> + {/* sentinel */},
> +};
> +
> +static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
> +{
> + struct mmc_pwrseq_simple *pwrseq;
> + struct device *dev = &pdev->dev;
> +
> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
> + if (!pwrseq)
> + return ERR_PTR(-ENOMEM);
> +
> + pwrseq->pwrseq.dev = dev;
> pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
>
> - return &pwrseq->pwrseq;
> -clk_put:
> - if (!IS_ERR(pwrseq->ext_clk))
> - clk_put(pwrseq->ext_clk);
> -free:
> - kfree(pwrseq);
> - return ERR_PTR(ret);
> + return mmc_pwrseq_register(&pwrseq->pwrseq);
> }
> +
> +
> +static struct platform_driver mmc_pwrseq_simple_driver = {
> + .probe = mmc_pwrseq_simple_probe,
> + .driver = {
> + .name = "pwrseq_simple",
> + .of_match_table = mmc_pwrseq_simple_of_match,
> + },
> +};
> +
> +builtin_platform_driver(mmc_pwrseq_simple_driver);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device
@ 2016-01-19 13:01 ` Ulf Hansson
0 siblings, 0 replies; 14+ messages in thread
From: Ulf Hansson @ 2016-01-19 13:01 UTC (permalink / raw)
To: linux-arm-kernel
On 19 January 2016 at 11:59, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> simple-pwrseq and emmc-pwrseq drivers rely on platform_device
> structure from of_find_device_by_node(), this works mostly. But, as there
> is no driver associated with this devices, cases like default/init pinctrl
> setup would never be performed by pwrseq. This becomes problem when the
> gpios used in pwrseq require pinctrl setup.
>
> Currently most of the common pinctrl setup is done in
> drivers/base/pinctrl.c by pinctrl_bind_pins().
>
> There are two ways to slove this issue on either convert pwrseq drivers
> to a proper platform drivers or copy the exact code from
> pcintrl_bind_pins(). I prefer converting pwrseq to proper drivers so that
> other cases like setting up clks/parents from dt would also be possible.
Overall I like the approach, thanks for posting this!
I have browsed through the patch, it looks okay, besides one issue...
There is a possibility that the pwrseq drivers haven't been probed
before mmc_pwrseq_alloc() gets called. This leads to that the
pwrseq_list may be empty, which means that we would silently fail to
find a corresponding pwrseq type even if the device node would say
there should be one.
Perhaps you should do a pre-validation of the device node in
mmc_pwrseq_alloc(), to see if there is a pwrseq handle. If so, but no
corresponding pwrseq method registerd (because the driver hasn?t been
probed yet), you could return -EPROBE_DEFER. Or perhaps there are
other alternatives to solved this issue!?
Kind regards
Uffe
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
> drivers/mmc/core/pwrseq.h | 13 ++++--
> drivers/mmc/core/pwrseq_emmc.c | 65 ++++++++++++++++----------
> drivers/mmc/core/pwrseq_simple.c | 71 ++++++++++++++++++----------
> 4 files changed, 145 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
> index 4c1d175..669608e 100644
> --- a/drivers/mmc/core/pwrseq.c
> +++ b/drivers/mmc/core/pwrseq.c
> @@ -8,50 +8,30 @@
> * MMC power sequence management
> */
> #include <linux/kernel.h>
> -#include <linux/platform_device.h>
> #include <linux/err.h>
> #include <linux/of.h>
> -#include <linux/of_platform.h>
>
> #include <linux/mmc/host.h>
>
> #include "pwrseq.h"
>
> -struct mmc_pwrseq_match {
> - const char *compatible;
> - struct mmc_pwrseq *(*alloc)(struct mmc_host *host, struct device *dev);
> -};
> -
> -static struct mmc_pwrseq_match pwrseq_match[] = {
> - {
> - .compatible = "mmc-pwrseq-simple",
> - .alloc = mmc_pwrseq_simple_alloc,
> - }, {
> - .compatible = "mmc-pwrseq-emmc",
> - .alloc = mmc_pwrseq_emmc_alloc,
> - },
> -};
> -
> -static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
> +static DEFINE_MUTEX(pwrseq_list_mutex);
> +static LIST_HEAD(pwrseq_list);
> +
> +static struct mmc_pwrseq *of_find_mmc_pwrseq(struct device_node *np)
> {
> - struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
> - int i;
> + struct mmc_pwrseq *p;
>
> - for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
> - if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
> - match = &pwrseq_match[i];
> - break;
> - }
> - }
> + list_for_each_entry(p, &pwrseq_list, list)
> + if (p->dev->of_node == np)
> + return p;
>
> - return match;
> + return NULL;
> }
>
> int mmc_pwrseq_alloc(struct mmc_host *host)
> {
> - struct platform_device *pdev;
> struct device_node *np;
> - struct mmc_pwrseq_match *match;
> struct mmc_pwrseq *pwrseq;
> int ret = 0;
>
> @@ -59,25 +39,18 @@ int mmc_pwrseq_alloc(struct mmc_host *host)
> if (!np)
> return 0;
>
> - pdev = of_find_device_by_node(np);
> - if (!pdev) {
> - ret = -ENODEV;
> - goto err;
> - }
> + pwrseq = of_find_mmc_pwrseq(np);
>
> - match = mmc_pwrseq_find(np);
> - if (IS_ERR(match)) {
> - ret = PTR_ERR(match);
> - goto err;
> - }
> + if (pwrseq && pwrseq->ops && pwrseq->ops->alloc) {
> + host->pwrseq = pwrseq;
> + ret = pwrseq->ops->alloc(host);
> + if (IS_ERR_VALUE(ret)) {
> + host->pwrseq = NULL;
> + goto err;
> + }
>
> - pwrseq = match->alloc(host, &pdev->dev);
> - if (IS_ERR(pwrseq)) {
> - ret = PTR_ERR(pwrseq);
> - goto err;
> }
>
> - host->pwrseq = pwrseq;
> dev_info(host->parent, "allocated mmc-pwrseq\n");
>
> err:
> @@ -89,7 +62,7 @@ void mmc_pwrseq_pre_power_on(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->pre_power_on)
> + if (pwrseq && pwrseq->ops->pre_power_on)
> pwrseq->ops->pre_power_on(host);
> }
>
> @@ -97,7 +70,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->post_power_on)
> + if (pwrseq && pwrseq->ops->post_power_on)
> pwrseq->ops->post_power_on(host);
> }
>
> @@ -105,7 +78,7 @@ void mmc_pwrseq_power_off(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->power_off)
> + if (pwrseq && pwrseq->ops->power_off)
> pwrseq->ops->power_off(host);
> }
>
> @@ -113,8 +86,34 @@ void mmc_pwrseq_free(struct mmc_host *host)
> {
> struct mmc_pwrseq *pwrseq = host->pwrseq;
>
> - if (pwrseq && pwrseq->ops && pwrseq->ops->free)
> - pwrseq->ops->free(host);
> + if (pwrseq) {
> + if (pwrseq->ops->free)
> + pwrseq->ops->free(host);
> +
> + host->pwrseq = NULL;
> + }
>
> - host->pwrseq = NULL;
> }
> +
> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
> +{
> + if (!pwrseq || !pwrseq->ops || !pwrseq->dev)
> + return -EINVAL;
> +
> + mutex_lock(&pwrseq_list_mutex);
> + list_add(&pwrseq->list, &pwrseq_list);
> + mutex_unlock(&pwrseq_list_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mmc_pwrseq_register);
> +
> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
> +{
> + if (pwrseq) {
> + mutex_lock(&pwrseq_list_mutex);
> + list_del(&pwrseq->list);
> + mutex_unlock(&pwrseq_list_mutex);
> + }
> +}
> +EXPORT_SYMBOL_GPL(mmc_pwrseq_unregister);
> diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
> index 096da48..08a7d2d 100644
> --- a/drivers/mmc/core/pwrseq.h
> +++ b/drivers/mmc/core/pwrseq.h
> @@ -8,7 +8,10 @@
> #ifndef _MMC_CORE_PWRSEQ_H
> #define _MMC_CORE_PWRSEQ_H
>
> +#include <linux/mmc/host.h>
> +
> struct mmc_pwrseq_ops {
> + int (*alloc)(struct mmc_host *host);
> void (*pre_power_on)(struct mmc_host *host);
> void (*post_power_on)(struct mmc_host *host);
> void (*power_off)(struct mmc_host *host);
> @@ -17,21 +20,21 @@ struct mmc_pwrseq_ops {
>
> struct mmc_pwrseq {
> struct mmc_pwrseq_ops *ops;
> + struct device *dev;
> + struct list_head list;
> };
>
> #ifdef CONFIG_OF
>
> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
> +
> int mmc_pwrseq_alloc(struct mmc_host *host);
> void mmc_pwrseq_pre_power_on(struct mmc_host *host);
> void mmc_pwrseq_post_power_on(struct mmc_host *host);
> void mmc_pwrseq_power_off(struct mmc_host *host);
> void mmc_pwrseq_free(struct mmc_host *host);
>
> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
> - struct device *dev);
> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
> - struct device *dev);
> -
> #else
>
> static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
> index 7978fb7..75f7423 100644
> --- a/drivers/mmc/core/pwrseq_emmc.c
> +++ b/drivers/mmc/core/pwrseq_emmc.c
> @@ -9,6 +9,7 @@
> */
> #include <linux/delay.h>
> #include <linux/kernel.h>
> +#include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -48,14 +49,8 @@ static void mmc_pwrseq_emmc_free(struct mmc_host *host)
>
> unregister_restart_handler(&pwrseq->reset_nb);
> gpiod_put(pwrseq->reset_gpio);
> - kfree(pwrseq);
> }
>
> -static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
> - .post_power_on = mmc_pwrseq_emmc_reset,
> - .free = mmc_pwrseq_emmc_free,
> -};
> -
> static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
> unsigned long mode, void *cmd)
> {
> @@ -66,21 +61,14 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
> return NOTIFY_DONE;
> }
>
> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
> - struct device *dev)
> +static int mmc_pwrseq_emmc_alloc(struct mmc_host *host)
> {
> - struct mmc_pwrseq_emmc *pwrseq;
> - int ret = 0;
> -
> - pwrseq = kzalloc(sizeof(struct mmc_pwrseq_emmc), GFP_KERNEL);
> - if (!pwrseq)
> - return ERR_PTR(-ENOMEM);
> + struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
>
> - pwrseq->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> - if (IS_ERR(pwrseq->reset_gpio)) {
> - ret = PTR_ERR(pwrseq->reset_gpio);
> - goto free;
> - }
> + pwrseq->reset_gpio = gpiod_get(host->pwrseq->dev,
> + "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(pwrseq->reset_gpio))
> + return PTR_ERR(pwrseq->reset_gpio);
>
> /*
> * register reset handler to ensure emmc reset also from
> @@ -91,10 +79,41 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
> pwrseq->reset_nb.priority = 255;
> register_restart_handler(&pwrseq->reset_nb);
>
> + return 0;
> +}
> +
> +static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
> + .alloc = mmc_pwrseq_emmc_alloc,
> + .post_power_on = mmc_pwrseq_emmc_reset,
> + .free = mmc_pwrseq_emmc_free,
> +};
> +
> +static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
> +{
> + struct mmc_pwrseq_emmc *pwrseq;
> + struct device *dev = &pdev->dev;
> +
> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
> + if (!pwrseq)
> + return -ENOMEM;
> +
> pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops;
> + pwrseq->pwrseq.dev = dev;
>
> - return &pwrseq->pwrseq;
> -free:
> - kfree(pwrseq);
> - return ERR_PTR(ret);
> + return mmc_pwrseq_register(&pwrseq->pwrseq);
> }
> +
> +static const struct of_device_id mmc_pwrseq_emmc_of_match[] = {
> + { .compatible = "mmc-pwrseq-emmc",},
> + {/* sentinel */},
> +};
> +
> +static struct platform_driver mmc_pwrseq_emmc_driver = {
> + .probe = mmc_pwrseq_emmc_probe,
> + .driver = {
> + .name = "pwrseq_emmc",
> + .of_match_table = mmc_pwrseq_emmc_of_match,
> + },
> +};
> +
> +builtin_platform_driver(mmc_pwrseq_emmc_driver);
> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
> index c091f98..b04cc2d 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -9,6 +9,7 @@
> */
> #include <linux/clk.h>
> #include <linux/kernel.h>
> +#include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -82,46 +83,66 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
> if (!IS_ERR(pwrseq->ext_clk))
> clk_put(pwrseq->ext_clk);
>
> - kfree(pwrseq);
> }
>
> -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_power_off,
> - .free = mmc_pwrseq_simple_free,
> -};
> -
> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
> - struct device *dev)
> +int mmc_pwrseq_simple_alloc(struct mmc_host *host)
> {
> - struct mmc_pwrseq_simple *pwrseq;
> + struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
> + struct device *dev = host->pwrseq->dev;
> int ret = 0;
>
> - pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL);
> - if (!pwrseq)
> - return ERR_PTR(-ENOMEM);
> -
> pwrseq->ext_clk = clk_get(dev, "ext_clock");
> if (IS_ERR(pwrseq->ext_clk) &&
> PTR_ERR(pwrseq->ext_clk) != -ENOENT) {
> - ret = PTR_ERR(pwrseq->ext_clk);
> - goto free;
> + return PTR_ERR(pwrseq->ext_clk);
> +
> }
>
> pwrseq->reset_gpios = gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
> if (IS_ERR(pwrseq->reset_gpios)) {
> ret = PTR_ERR(pwrseq->reset_gpios);
> - goto clk_put;
> + clk_put(pwrseq->ext_clk);
> + return ret;
> }
>
> + return 0;
> +}
> +
> +static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
> + .alloc = mmc_pwrseq_simple_alloc,
> + .pre_power_on = mmc_pwrseq_simple_pre_power_on,
> + .post_power_on = mmc_pwrseq_simple_post_power_on,
> + .power_off = mmc_pwrseq_simple_power_off,
> + .free = mmc_pwrseq_simple_free,
> +};
> +
> +static const struct of_device_id mmc_pwrseq_simple_of_match[] = {
> + { .compatible = "mmc-pwrseq-simple",},
> + {/* sentinel */},
> +};
> +
> +static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
> +{
> + struct mmc_pwrseq_simple *pwrseq;
> + struct device *dev = &pdev->dev;
> +
> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
> + if (!pwrseq)
> + return ERR_PTR(-ENOMEM);
> +
> + pwrseq->pwrseq.dev = dev;
> pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
>
> - return &pwrseq->pwrseq;
> -clk_put:
> - if (!IS_ERR(pwrseq->ext_clk))
> - clk_put(pwrseq->ext_clk);
> -free:
> - kfree(pwrseq);
> - return ERR_PTR(ret);
> + return mmc_pwrseq_register(&pwrseq->pwrseq);
> }
> +
> +
> +static struct platform_driver mmc_pwrseq_simple_driver = {
> + .probe = mmc_pwrseq_simple_probe,
> + .driver = {
> + .name = "pwrseq_simple",
> + .of_match_table = mmc_pwrseq_simple_of_match,
> + },
> +};
> +
> +builtin_platform_driver(mmc_pwrseq_simple_driver);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device
2016-01-19 13:01 ` Ulf Hansson
(?)
@ 2016-01-19 14:05 ` Srinivas Kandagatla
-1 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 14:05 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, linux-kernel, linux-arm-kernel
On 19/01/16 13:01, Ulf Hansson wrote:
> On 19 January 2016 at 11:59, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> simple-pwrseq and emmc-pwrseq drivers rely on platform_device
>> structure from of_find_device_by_node(), this works mostly. But, as there
>> is no driver associated with this devices, cases like default/init pinctrl
>> setup would never be performed by pwrseq. This becomes problem when the
>> gpios used in pwrseq require pinctrl setup.
>>
>> Currently most of the common pinctrl setup is done in
>> drivers/base/pinctrl.c by pinctrl_bind_pins().
>>
>> There are two ways to slove this issue on either convert pwrseq drivers
>> to a proper platform drivers or copy the exact code from
>> pcintrl_bind_pins(). I prefer converting pwrseq to proper drivers so that
>> other cases like setting up clks/parents from dt would also be possible.
>
> Overall I like the approach, thanks for posting this!
>
> I have browsed through the patch, it looks okay, besides one issue...
>
> There is a possibility that the pwrseq drivers haven't been probed
> before mmc_pwrseq_alloc() gets called. This leads to that the
> pwrseq_list may be empty, which means that we would silently fail to
> find a corresponding pwrseq type even if the device node would say
> there should be one.
>
> Perhaps you should do a pre-validation of the device node in
> mmc_pwrseq_alloc(), to see if there is a pwrseq handle. If so, but no
> corresponding pwrseq method registerd (because the driver hasn’t been
> probed yet), you could return -EPROBE_DEFER. Or perhaps there are
> other alternatives to solved this issue!?
That's a good point.
Doing EPROBE_DEFER sounds more correct to me, we could return this error
in cases where the phandle for "mmc-pwrseq" exists and unable to find
the pwrseq in the list.
This would go well with mmc_of_parse() return types.
Will spin v2 with this fix.
thanks,
srini
>
> Kind regards
> Uffe
>
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
>> drivers/mmc/core/pwrseq.h | 13 ++++--
>> drivers/mmc/core/pwrseq_emmc.c | 65 ++++++++++++++++----------
>> drivers/mmc/core/pwrseq_simple.c | 71 ++++++++++++++++++----------
>> 4 files changed, 145 insertions(+), 103 deletions(-)
>>
>> diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
>> index 4c1d175..669608e 100644
>> --- a/drivers/mmc/core/pwrseq.c
>> +++ b/drivers/mmc/core/pwrseq.c
>> @@ -8,50 +8,30 @@
>> * MMC power sequence management
>> */
>> #include <linux/kernel.h>
>> -#include <linux/platform_device.h>
>> #include <linux/err.h>
>> #include <linux/of.h>
>> -#include <linux/of_platform.h>
>>
>> #include <linux/mmc/host.h>
>>
>> #include "pwrseq.h"
>>
>> -struct mmc_pwrseq_match {
>> - const char *compatible;
>> - struct mmc_pwrseq *(*alloc)(struct mmc_host *host, struct device *dev);
>> -};
>> -
>> -static struct mmc_pwrseq_match pwrseq_match[] = {
>> - {
>> - .compatible = "mmc-pwrseq-simple",
>> - .alloc = mmc_pwrseq_simple_alloc,
>> - }, {
>> - .compatible = "mmc-pwrseq-emmc",
>> - .alloc = mmc_pwrseq_emmc_alloc,
>> - },
>> -};
>> -
>> -static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
>> +static DEFINE_MUTEX(pwrseq_list_mutex);
>> +static LIST_HEAD(pwrseq_list);
>> +
>> +static struct mmc_pwrseq *of_find_mmc_pwrseq(struct device_node *np)
>> {
>> - struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
>> - int i;
>> + struct mmc_pwrseq *p;
>>
>> - for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
>> - if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
>> - match = &pwrseq_match[i];
>> - break;
>> - }
>> - }
>> + list_for_each_entry(p, &pwrseq_list, list)
>> + if (p->dev->of_node == np)
>> + return p;
>>
>> - return match;
>> + return NULL;
>> }
>>
>> int mmc_pwrseq_alloc(struct mmc_host *host)
>> {
>> - struct platform_device *pdev;
>> struct device_node *np;
>> - struct mmc_pwrseq_match *match;
>> struct mmc_pwrseq *pwrseq;
>> int ret = 0;
>>
>> @@ -59,25 +39,18 @@ int mmc_pwrseq_alloc(struct mmc_host *host)
>> if (!np)
>> return 0;
>>
>> - pdev = of_find_device_by_node(np);
>> - if (!pdev) {
>> - ret = -ENODEV;
>> - goto err;
>> - }
>> + pwrseq = of_find_mmc_pwrseq(np);
>>
>> - match = mmc_pwrseq_find(np);
>> - if (IS_ERR(match)) {
>> - ret = PTR_ERR(match);
>> - goto err;
>> - }
>> + if (pwrseq && pwrseq->ops && pwrseq->ops->alloc) {
>> + host->pwrseq = pwrseq;
>> + ret = pwrseq->ops->alloc(host);
>> + if (IS_ERR_VALUE(ret)) {
>> + host->pwrseq = NULL;
>> + goto err;
>> + }
>>
>> - pwrseq = match->alloc(host, &pdev->dev);
>> - if (IS_ERR(pwrseq)) {
>> - ret = PTR_ERR(pwrseq);
>> - goto err;
>> }
>>
>> - host->pwrseq = pwrseq;
>> dev_info(host->parent, "allocated mmc-pwrseq\n");
>>
>> err:
>> @@ -89,7 +62,7 @@ void mmc_pwrseq_pre_power_on(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->pre_power_on)
>> + if (pwrseq && pwrseq->ops->pre_power_on)
>> pwrseq->ops->pre_power_on(host);
>> }
>>
>> @@ -97,7 +70,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->post_power_on)
>> + if (pwrseq && pwrseq->ops->post_power_on)
>> pwrseq->ops->post_power_on(host);
>> }
>>
>> @@ -105,7 +78,7 @@ void mmc_pwrseq_power_off(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->power_off)
>> + if (pwrseq && pwrseq->ops->power_off)
>> pwrseq->ops->power_off(host);
>> }
>>
>> @@ -113,8 +86,34 @@ void mmc_pwrseq_free(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->free)
>> - pwrseq->ops->free(host);
>> + if (pwrseq) {
>> + if (pwrseq->ops->free)
>> + pwrseq->ops->free(host);
>> +
>> + host->pwrseq = NULL;
>> + }
>>
>> - host->pwrseq = NULL;
>> }
>> +
>> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
>> +{
>> + if (!pwrseq || !pwrseq->ops || !pwrseq->dev)
>> + return -EINVAL;
>> +
>> + mutex_lock(&pwrseq_list_mutex);
>> + list_add(&pwrseq->list, &pwrseq_list);
>> + mutex_unlock(&pwrseq_list_mutex);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_pwrseq_register);
>> +
>> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
>> +{
>> + if (pwrseq) {
>> + mutex_lock(&pwrseq_list_mutex);
>> + list_del(&pwrseq->list);
>> + mutex_unlock(&pwrseq_list_mutex);
>> + }
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_pwrseq_unregister);
>> diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
>> index 096da48..08a7d2d 100644
>> --- a/drivers/mmc/core/pwrseq.h
>> +++ b/drivers/mmc/core/pwrseq.h
>> @@ -8,7 +8,10 @@
>> #ifndef _MMC_CORE_PWRSEQ_H
>> #define _MMC_CORE_PWRSEQ_H
>>
>> +#include <linux/mmc/host.h>
>> +
>> struct mmc_pwrseq_ops {
>> + int (*alloc)(struct mmc_host *host);
>> void (*pre_power_on)(struct mmc_host *host);
>> void (*post_power_on)(struct mmc_host *host);
>> void (*power_off)(struct mmc_host *host);
>> @@ -17,21 +20,21 @@ struct mmc_pwrseq_ops {
>>
>> struct mmc_pwrseq {
>> struct mmc_pwrseq_ops *ops;
>> + struct device *dev;
>> + struct list_head list;
>> };
>>
>> #ifdef CONFIG_OF
>>
>> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
>> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
>> +
>> int mmc_pwrseq_alloc(struct mmc_host *host);
>> void mmc_pwrseq_pre_power_on(struct mmc_host *host);
>> void mmc_pwrseq_post_power_on(struct mmc_host *host);
>> void mmc_pwrseq_power_off(struct mmc_host *host);
>> void mmc_pwrseq_free(struct mmc_host *host);
>>
>> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
>> - struct device *dev);
>> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>> - struct device *dev);
>> -
>> #else
>>
>> static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
>> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
>> index 7978fb7..75f7423 100644
>> --- a/drivers/mmc/core/pwrseq_emmc.c
>> +++ b/drivers/mmc/core/pwrseq_emmc.c
>> @@ -9,6 +9,7 @@
>> */
>> #include <linux/delay.h>
>> #include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> #include <linux/slab.h>
>> #include <linux/device.h>
>> #include <linux/err.h>
>> @@ -48,14 +49,8 @@ static void mmc_pwrseq_emmc_free(struct mmc_host *host)
>>
>> unregister_restart_handler(&pwrseq->reset_nb);
>> gpiod_put(pwrseq->reset_gpio);
>> - kfree(pwrseq);
>> }
>>
>> -static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
>> - .post_power_on = mmc_pwrseq_emmc_reset,
>> - .free = mmc_pwrseq_emmc_free,
>> -};
>> -
>> static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
>> unsigned long mode, void *cmd)
>> {
>> @@ -66,21 +61,14 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
>> return NOTIFY_DONE;
>> }
>>
>> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>> - struct device *dev)
>> +static int mmc_pwrseq_emmc_alloc(struct mmc_host *host)
>> {
>> - struct mmc_pwrseq_emmc *pwrseq;
>> - int ret = 0;
>> -
>> - pwrseq = kzalloc(sizeof(struct mmc_pwrseq_emmc), GFP_KERNEL);
>> - if (!pwrseq)
>> - return ERR_PTR(-ENOMEM);
>> + struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
>>
>> - pwrseq->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> - if (IS_ERR(pwrseq->reset_gpio)) {
>> - ret = PTR_ERR(pwrseq->reset_gpio);
>> - goto free;
>> - }
>> + pwrseq->reset_gpio = gpiod_get(host->pwrseq->dev,
>> + "reset", GPIOD_OUT_LOW);
>> + if (IS_ERR(pwrseq->reset_gpio))
>> + return PTR_ERR(pwrseq->reset_gpio);
>>
>> /*
>> * register reset handler to ensure emmc reset also from
>> @@ -91,10 +79,41 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>> pwrseq->reset_nb.priority = 255;
>> register_restart_handler(&pwrseq->reset_nb);
>>
>> + return 0;
>> +}
>> +
>> +static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
>> + .alloc = mmc_pwrseq_emmc_alloc,
>> + .post_power_on = mmc_pwrseq_emmc_reset,
>> + .free = mmc_pwrseq_emmc_free,
>> +};
>> +
>> +static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
>> +{
>> + struct mmc_pwrseq_emmc *pwrseq;
>> + struct device *dev = &pdev->dev;
>> +
>> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
>> + if (!pwrseq)
>> + return -ENOMEM;
>> +
>> pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops;
>> + pwrseq->pwrseq.dev = dev;
>>
>> - return &pwrseq->pwrseq;
>> -free:
>> - kfree(pwrseq);
>> - return ERR_PTR(ret);
>> + return mmc_pwrseq_register(&pwrseq->pwrseq);
>> }
>> +
>> +static const struct of_device_id mmc_pwrseq_emmc_of_match[] = {
>> + { .compatible = "mmc-pwrseq-emmc",},
>> + {/* sentinel */},
>> +};
>> +
>> +static struct platform_driver mmc_pwrseq_emmc_driver = {
>> + .probe = mmc_pwrseq_emmc_probe,
>> + .driver = {
>> + .name = "pwrseq_emmc",
>> + .of_match_table = mmc_pwrseq_emmc_of_match,
>> + },
>> +};
>> +
>> +builtin_platform_driver(mmc_pwrseq_emmc_driver);
>> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
>> index c091f98..b04cc2d 100644
>> --- a/drivers/mmc/core/pwrseq_simple.c
>> +++ b/drivers/mmc/core/pwrseq_simple.c
>> @@ -9,6 +9,7 @@
>> */
>> #include <linux/clk.h>
>> #include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> #include <linux/slab.h>
>> #include <linux/device.h>
>> #include <linux/err.h>
>> @@ -82,46 +83,66 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
>> if (!IS_ERR(pwrseq->ext_clk))
>> clk_put(pwrseq->ext_clk);
>>
>> - kfree(pwrseq);
>> }
>>
>> -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_power_off,
>> - .free = mmc_pwrseq_simple_free,
>> -};
>> -
>> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
>> - struct device *dev)
>> +int mmc_pwrseq_simple_alloc(struct mmc_host *host)
>> {
>> - struct mmc_pwrseq_simple *pwrseq;
>> + struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
>> + struct device *dev = host->pwrseq->dev;
>> int ret = 0;
>>
>> - pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL);
>> - if (!pwrseq)
>> - return ERR_PTR(-ENOMEM);
>> -
>> pwrseq->ext_clk = clk_get(dev, "ext_clock");
>> if (IS_ERR(pwrseq->ext_clk) &&
>> PTR_ERR(pwrseq->ext_clk) != -ENOENT) {
>> - ret = PTR_ERR(pwrseq->ext_clk);
>> - goto free;
>> + return PTR_ERR(pwrseq->ext_clk);
>> +
>> }
>>
>> pwrseq->reset_gpios = gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
>> if (IS_ERR(pwrseq->reset_gpios)) {
>> ret = PTR_ERR(pwrseq->reset_gpios);
>> - goto clk_put;
>> + clk_put(pwrseq->ext_clk);
>> + return ret;
>> }
>>
>> + return 0;
>> +}
>> +
>> +static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
>> + .alloc = mmc_pwrseq_simple_alloc,
>> + .pre_power_on = mmc_pwrseq_simple_pre_power_on,
>> + .post_power_on = mmc_pwrseq_simple_post_power_on,
>> + .power_off = mmc_pwrseq_simple_power_off,
>> + .free = mmc_pwrseq_simple_free,
>> +};
>> +
>> +static const struct of_device_id mmc_pwrseq_simple_of_match[] = {
>> + { .compatible = "mmc-pwrseq-simple",},
>> + {/* sentinel */},
>> +};
>> +
>> +static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
>> +{
>> + struct mmc_pwrseq_simple *pwrseq;
>> + struct device *dev = &pdev->dev;
>> +
>> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
>> + if (!pwrseq)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + pwrseq->pwrseq.dev = dev;
>> pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
>>
>> - return &pwrseq->pwrseq;
>> -clk_put:
>> - if (!IS_ERR(pwrseq->ext_clk))
>> - clk_put(pwrseq->ext_clk);
>> -free:
>> - kfree(pwrseq);
>> - return ERR_PTR(ret);
>> + return mmc_pwrseq_register(&pwrseq->pwrseq);
>> }
>> +
>> +
>> +static struct platform_driver mmc_pwrseq_simple_driver = {
>> + .probe = mmc_pwrseq_simple_probe,
>> + .driver = {
>> + .name = "pwrseq_simple",
>> + .of_match_table = mmc_pwrseq_simple_of_match,
>> + },
>> +};
>> +
>> +builtin_platform_driver(mmc_pwrseq_simple_driver);
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device
@ 2016-01-19 14:05 ` Srinivas Kandagatla
0 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 14:05 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, linux-kernel, linux-arm-kernel
On 19/01/16 13:01, Ulf Hansson wrote:
> On 19 January 2016 at 11:59, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> simple-pwrseq and emmc-pwrseq drivers rely on platform_device
>> structure from of_find_device_by_node(), this works mostly. But, as there
>> is no driver associated with this devices, cases like default/init pinctrl
>> setup would never be performed by pwrseq. This becomes problem when the
>> gpios used in pwrseq require pinctrl setup.
>>
>> Currently most of the common pinctrl setup is done in
>> drivers/base/pinctrl.c by pinctrl_bind_pins().
>>
>> There are two ways to slove this issue on either convert pwrseq drivers
>> to a proper platform drivers or copy the exact code from
>> pcintrl_bind_pins(). I prefer converting pwrseq to proper drivers so that
>> other cases like setting up clks/parents from dt would also be possible.
>
> Overall I like the approach, thanks for posting this!
>
> I have browsed through the patch, it looks okay, besides one issue...
>
> There is a possibility that the pwrseq drivers haven't been probed
> before mmc_pwrseq_alloc() gets called. This leads to that the
> pwrseq_list may be empty, which means that we would silently fail to
> find a corresponding pwrseq type even if the device node would say
> there should be one.
>
> Perhaps you should do a pre-validation of the device node in
> mmc_pwrseq_alloc(), to see if there is a pwrseq handle. If so, but no
> corresponding pwrseq method registerd (because the driver hasn’t been
> probed yet), you could return -EPROBE_DEFER. Or perhaps there are
> other alternatives to solved this issue!?
That's a good point.
Doing EPROBE_DEFER sounds more correct to me, we could return this error
in cases where the phandle for "mmc-pwrseq" exists and unable to find
the pwrseq in the list.
This would go well with mmc_of_parse() return types.
Will spin v2 with this fix.
thanks,
srini
>
> Kind regards
> Uffe
>
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
>> drivers/mmc/core/pwrseq.h | 13 ++++--
>> drivers/mmc/core/pwrseq_emmc.c | 65 ++++++++++++++++----------
>> drivers/mmc/core/pwrseq_simple.c | 71 ++++++++++++++++++----------
>> 4 files changed, 145 insertions(+), 103 deletions(-)
>>
>> diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
>> index 4c1d175..669608e 100644
>> --- a/drivers/mmc/core/pwrseq.c
>> +++ b/drivers/mmc/core/pwrseq.c
>> @@ -8,50 +8,30 @@
>> * MMC power sequence management
>> */
>> #include <linux/kernel.h>
>> -#include <linux/platform_device.h>
>> #include <linux/err.h>
>> #include <linux/of.h>
>> -#include <linux/of_platform.h>
>>
>> #include <linux/mmc/host.h>
>>
>> #include "pwrseq.h"
>>
>> -struct mmc_pwrseq_match {
>> - const char *compatible;
>> - struct mmc_pwrseq *(*alloc)(struct mmc_host *host, struct device *dev);
>> -};
>> -
>> -static struct mmc_pwrseq_match pwrseq_match[] = {
>> - {
>> - .compatible = "mmc-pwrseq-simple",
>> - .alloc = mmc_pwrseq_simple_alloc,
>> - }, {
>> - .compatible = "mmc-pwrseq-emmc",
>> - .alloc = mmc_pwrseq_emmc_alloc,
>> - },
>> -};
>> -
>> -static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
>> +static DEFINE_MUTEX(pwrseq_list_mutex);
>> +static LIST_HEAD(pwrseq_list);
>> +
>> +static struct mmc_pwrseq *of_find_mmc_pwrseq(struct device_node *np)
>> {
>> - struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
>> - int i;
>> + struct mmc_pwrseq *p;
>>
>> - for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
>> - if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
>> - match = &pwrseq_match[i];
>> - break;
>> - }
>> - }
>> + list_for_each_entry(p, &pwrseq_list, list)
>> + if (p->dev->of_node == np)
>> + return p;
>>
>> - return match;
>> + return NULL;
>> }
>>
>> int mmc_pwrseq_alloc(struct mmc_host *host)
>> {
>> - struct platform_device *pdev;
>> struct device_node *np;
>> - struct mmc_pwrseq_match *match;
>> struct mmc_pwrseq *pwrseq;
>> int ret = 0;
>>
>> @@ -59,25 +39,18 @@ int mmc_pwrseq_alloc(struct mmc_host *host)
>> if (!np)
>> return 0;
>>
>> - pdev = of_find_device_by_node(np);
>> - if (!pdev) {
>> - ret = -ENODEV;
>> - goto err;
>> - }
>> + pwrseq = of_find_mmc_pwrseq(np);
>>
>> - match = mmc_pwrseq_find(np);
>> - if (IS_ERR(match)) {
>> - ret = PTR_ERR(match);
>> - goto err;
>> - }
>> + if (pwrseq && pwrseq->ops && pwrseq->ops->alloc) {
>> + host->pwrseq = pwrseq;
>> + ret = pwrseq->ops->alloc(host);
>> + if (IS_ERR_VALUE(ret)) {
>> + host->pwrseq = NULL;
>> + goto err;
>> + }
>>
>> - pwrseq = match->alloc(host, &pdev->dev);
>> - if (IS_ERR(pwrseq)) {
>> - ret = PTR_ERR(pwrseq);
>> - goto err;
>> }
>>
>> - host->pwrseq = pwrseq;
>> dev_info(host->parent, "allocated mmc-pwrseq\n");
>>
>> err:
>> @@ -89,7 +62,7 @@ void mmc_pwrseq_pre_power_on(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->pre_power_on)
>> + if (pwrseq && pwrseq->ops->pre_power_on)
>> pwrseq->ops->pre_power_on(host);
>> }
>>
>> @@ -97,7 +70,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->post_power_on)
>> + if (pwrseq && pwrseq->ops->post_power_on)
>> pwrseq->ops->post_power_on(host);
>> }
>>
>> @@ -105,7 +78,7 @@ void mmc_pwrseq_power_off(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->power_off)
>> + if (pwrseq && pwrseq->ops->power_off)
>> pwrseq->ops->power_off(host);
>> }
>>
>> @@ -113,8 +86,34 @@ void mmc_pwrseq_free(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->free)
>> - pwrseq->ops->free(host);
>> + if (pwrseq) {
>> + if (pwrseq->ops->free)
>> + pwrseq->ops->free(host);
>> +
>> + host->pwrseq = NULL;
>> + }
>>
>> - host->pwrseq = NULL;
>> }
>> +
>> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
>> +{
>> + if (!pwrseq || !pwrseq->ops || !pwrseq->dev)
>> + return -EINVAL;
>> +
>> + mutex_lock(&pwrseq_list_mutex);
>> + list_add(&pwrseq->list, &pwrseq_list);
>> + mutex_unlock(&pwrseq_list_mutex);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_pwrseq_register);
>> +
>> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
>> +{
>> + if (pwrseq) {
>> + mutex_lock(&pwrseq_list_mutex);
>> + list_del(&pwrseq->list);
>> + mutex_unlock(&pwrseq_list_mutex);
>> + }
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_pwrseq_unregister);
>> diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
>> index 096da48..08a7d2d 100644
>> --- a/drivers/mmc/core/pwrseq.h
>> +++ b/drivers/mmc/core/pwrseq.h
>> @@ -8,7 +8,10 @@
>> #ifndef _MMC_CORE_PWRSEQ_H
>> #define _MMC_CORE_PWRSEQ_H
>>
>> +#include <linux/mmc/host.h>
>> +
>> struct mmc_pwrseq_ops {
>> + int (*alloc)(struct mmc_host *host);
>> void (*pre_power_on)(struct mmc_host *host);
>> void (*post_power_on)(struct mmc_host *host);
>> void (*power_off)(struct mmc_host *host);
>> @@ -17,21 +20,21 @@ struct mmc_pwrseq_ops {
>>
>> struct mmc_pwrseq {
>> struct mmc_pwrseq_ops *ops;
>> + struct device *dev;
>> + struct list_head list;
>> };
>>
>> #ifdef CONFIG_OF
>>
>> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
>> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
>> +
>> int mmc_pwrseq_alloc(struct mmc_host *host);
>> void mmc_pwrseq_pre_power_on(struct mmc_host *host);
>> void mmc_pwrseq_post_power_on(struct mmc_host *host);
>> void mmc_pwrseq_power_off(struct mmc_host *host);
>> void mmc_pwrseq_free(struct mmc_host *host);
>>
>> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
>> - struct device *dev);
>> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>> - struct device *dev);
>> -
>> #else
>>
>> static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
>> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
>> index 7978fb7..75f7423 100644
>> --- a/drivers/mmc/core/pwrseq_emmc.c
>> +++ b/drivers/mmc/core/pwrseq_emmc.c
>> @@ -9,6 +9,7 @@
>> */
>> #include <linux/delay.h>
>> #include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> #include <linux/slab.h>
>> #include <linux/device.h>
>> #include <linux/err.h>
>> @@ -48,14 +49,8 @@ static void mmc_pwrseq_emmc_free(struct mmc_host *host)
>>
>> unregister_restart_handler(&pwrseq->reset_nb);
>> gpiod_put(pwrseq->reset_gpio);
>> - kfree(pwrseq);
>> }
>>
>> -static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
>> - .post_power_on = mmc_pwrseq_emmc_reset,
>> - .free = mmc_pwrseq_emmc_free,
>> -};
>> -
>> static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
>> unsigned long mode, void *cmd)
>> {
>> @@ -66,21 +61,14 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
>> return NOTIFY_DONE;
>> }
>>
>> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>> - struct device *dev)
>> +static int mmc_pwrseq_emmc_alloc(struct mmc_host *host)
>> {
>> - struct mmc_pwrseq_emmc *pwrseq;
>> - int ret = 0;
>> -
>> - pwrseq = kzalloc(sizeof(struct mmc_pwrseq_emmc), GFP_KERNEL);
>> - if (!pwrseq)
>> - return ERR_PTR(-ENOMEM);
>> + struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
>>
>> - pwrseq->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> - if (IS_ERR(pwrseq->reset_gpio)) {
>> - ret = PTR_ERR(pwrseq->reset_gpio);
>> - goto free;
>> - }
>> + pwrseq->reset_gpio = gpiod_get(host->pwrseq->dev,
>> + "reset", GPIOD_OUT_LOW);
>> + if (IS_ERR(pwrseq->reset_gpio))
>> + return PTR_ERR(pwrseq->reset_gpio);
>>
>> /*
>> * register reset handler to ensure emmc reset also from
>> @@ -91,10 +79,41 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>> pwrseq->reset_nb.priority = 255;
>> register_restart_handler(&pwrseq->reset_nb);
>>
>> + return 0;
>> +}
>> +
>> +static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
>> + .alloc = mmc_pwrseq_emmc_alloc,
>> + .post_power_on = mmc_pwrseq_emmc_reset,
>> + .free = mmc_pwrseq_emmc_free,
>> +};
>> +
>> +static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
>> +{
>> + struct mmc_pwrseq_emmc *pwrseq;
>> + struct device *dev = &pdev->dev;
>> +
>> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
>> + if (!pwrseq)
>> + return -ENOMEM;
>> +
>> pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops;
>> + pwrseq->pwrseq.dev = dev;
>>
>> - return &pwrseq->pwrseq;
>> -free:
>> - kfree(pwrseq);
>> - return ERR_PTR(ret);
>> + return mmc_pwrseq_register(&pwrseq->pwrseq);
>> }
>> +
>> +static const struct of_device_id mmc_pwrseq_emmc_of_match[] = {
>> + { .compatible = "mmc-pwrseq-emmc",},
>> + {/* sentinel */},
>> +};
>> +
>> +static struct platform_driver mmc_pwrseq_emmc_driver = {
>> + .probe = mmc_pwrseq_emmc_probe,
>> + .driver = {
>> + .name = "pwrseq_emmc",
>> + .of_match_table = mmc_pwrseq_emmc_of_match,
>> + },
>> +};
>> +
>> +builtin_platform_driver(mmc_pwrseq_emmc_driver);
>> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
>> index c091f98..b04cc2d 100644
>> --- a/drivers/mmc/core/pwrseq_simple.c
>> +++ b/drivers/mmc/core/pwrseq_simple.c
>> @@ -9,6 +9,7 @@
>> */
>> #include <linux/clk.h>
>> #include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> #include <linux/slab.h>
>> #include <linux/device.h>
>> #include <linux/err.h>
>> @@ -82,46 +83,66 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
>> if (!IS_ERR(pwrseq->ext_clk))
>> clk_put(pwrseq->ext_clk);
>>
>> - kfree(pwrseq);
>> }
>>
>> -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_power_off,
>> - .free = mmc_pwrseq_simple_free,
>> -};
>> -
>> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
>> - struct device *dev)
>> +int mmc_pwrseq_simple_alloc(struct mmc_host *host)
>> {
>> - struct mmc_pwrseq_simple *pwrseq;
>> + struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
>> + struct device *dev = host->pwrseq->dev;
>> int ret = 0;
>>
>> - pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL);
>> - if (!pwrseq)
>> - return ERR_PTR(-ENOMEM);
>> -
>> pwrseq->ext_clk = clk_get(dev, "ext_clock");
>> if (IS_ERR(pwrseq->ext_clk) &&
>> PTR_ERR(pwrseq->ext_clk) != -ENOENT) {
>> - ret = PTR_ERR(pwrseq->ext_clk);
>> - goto free;
>> + return PTR_ERR(pwrseq->ext_clk);
>> +
>> }
>>
>> pwrseq->reset_gpios = gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
>> if (IS_ERR(pwrseq->reset_gpios)) {
>> ret = PTR_ERR(pwrseq->reset_gpios);
>> - goto clk_put;
>> + clk_put(pwrseq->ext_clk);
>> + return ret;
>> }
>>
>> + return 0;
>> +}
>> +
>> +static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
>> + .alloc = mmc_pwrseq_simple_alloc,
>> + .pre_power_on = mmc_pwrseq_simple_pre_power_on,
>> + .post_power_on = mmc_pwrseq_simple_post_power_on,
>> + .power_off = mmc_pwrseq_simple_power_off,
>> + .free = mmc_pwrseq_simple_free,
>> +};
>> +
>> +static const struct of_device_id mmc_pwrseq_simple_of_match[] = {
>> + { .compatible = "mmc-pwrseq-simple",},
>> + {/* sentinel */},
>> +};
>> +
>> +static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
>> +{
>> + struct mmc_pwrseq_simple *pwrseq;
>> + struct device *dev = &pdev->dev;
>> +
>> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
>> + if (!pwrseq)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + pwrseq->pwrseq.dev = dev;
>> pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
>>
>> - return &pwrseq->pwrseq;
>> -clk_put:
>> - if (!IS_ERR(pwrseq->ext_clk))
>> - clk_put(pwrseq->ext_clk);
>> -free:
>> - kfree(pwrseq);
>> - return ERR_PTR(ret);
>> + return mmc_pwrseq_register(&pwrseq->pwrseq);
>> }
>> +
>> +
>> +static struct platform_driver mmc_pwrseq_simple_driver = {
>> + .probe = mmc_pwrseq_simple_probe,
>> + .driver = {
>> + .name = "pwrseq_simple",
>> + .of_match_table = mmc_pwrseq_simple_of_match,
>> + },
>> +};
>> +
>> +builtin_platform_driver(mmc_pwrseq_simple_driver);
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device
@ 2016-01-19 14:05 ` Srinivas Kandagatla
0 siblings, 0 replies; 14+ messages in thread
From: Srinivas Kandagatla @ 2016-01-19 14:05 UTC (permalink / raw)
To: linux-arm-kernel
On 19/01/16 13:01, Ulf Hansson wrote:
> On 19 January 2016 at 11:59, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> simple-pwrseq and emmc-pwrseq drivers rely on platform_device
>> structure from of_find_device_by_node(), this works mostly. But, as there
>> is no driver associated with this devices, cases like default/init pinctrl
>> setup would never be performed by pwrseq. This becomes problem when the
>> gpios used in pwrseq require pinctrl setup.
>>
>> Currently most of the common pinctrl setup is done in
>> drivers/base/pinctrl.c by pinctrl_bind_pins().
>>
>> There are two ways to slove this issue on either convert pwrseq drivers
>> to a proper platform drivers or copy the exact code from
>> pcintrl_bind_pins(). I prefer converting pwrseq to proper drivers so that
>> other cases like setting up clks/parents from dt would also be possible.
>
> Overall I like the approach, thanks for posting this!
>
> I have browsed through the patch, it looks okay, besides one issue...
>
> There is a possibility that the pwrseq drivers haven't been probed
> before mmc_pwrseq_alloc() gets called. This leads to that the
> pwrseq_list may be empty, which means that we would silently fail to
> find a corresponding pwrseq type even if the device node would say
> there should be one.
>
> Perhaps you should do a pre-validation of the device node in
> mmc_pwrseq_alloc(), to see if there is a pwrseq handle. If so, but no
> corresponding pwrseq method registerd (because the driver hasn?t been
> probed yet), you could return -EPROBE_DEFER. Or perhaps there are
> other alternatives to solved this issue!?
That's a good point.
Doing EPROBE_DEFER sounds more correct to me, we could return this error
in cases where the phandle for "mmc-pwrseq" exists and unable to find
the pwrseq in the list.
This would go well with mmc_of_parse() return types.
Will spin v2 with this fix.
thanks,
srini
>
> Kind regards
> Uffe
>
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/mmc/core/pwrseq.c | 99 ++++++++++++++++++++--------------------
>> drivers/mmc/core/pwrseq.h | 13 ++++--
>> drivers/mmc/core/pwrseq_emmc.c | 65 ++++++++++++++++----------
>> drivers/mmc/core/pwrseq_simple.c | 71 ++++++++++++++++++----------
>> 4 files changed, 145 insertions(+), 103 deletions(-)
>>
>> diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
>> index 4c1d175..669608e 100644
>> --- a/drivers/mmc/core/pwrseq.c
>> +++ b/drivers/mmc/core/pwrseq.c
>> @@ -8,50 +8,30 @@
>> * MMC power sequence management
>> */
>> #include <linux/kernel.h>
>> -#include <linux/platform_device.h>
>> #include <linux/err.h>
>> #include <linux/of.h>
>> -#include <linux/of_platform.h>
>>
>> #include <linux/mmc/host.h>
>>
>> #include "pwrseq.h"
>>
>> -struct mmc_pwrseq_match {
>> - const char *compatible;
>> - struct mmc_pwrseq *(*alloc)(struct mmc_host *host, struct device *dev);
>> -};
>> -
>> -static struct mmc_pwrseq_match pwrseq_match[] = {
>> - {
>> - .compatible = "mmc-pwrseq-simple",
>> - .alloc = mmc_pwrseq_simple_alloc,
>> - }, {
>> - .compatible = "mmc-pwrseq-emmc",
>> - .alloc = mmc_pwrseq_emmc_alloc,
>> - },
>> -};
>> -
>> -static struct mmc_pwrseq_match *mmc_pwrseq_find(struct device_node *np)
>> +static DEFINE_MUTEX(pwrseq_list_mutex);
>> +static LIST_HEAD(pwrseq_list);
>> +
>> +static struct mmc_pwrseq *of_find_mmc_pwrseq(struct device_node *np)
>> {
>> - struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
>> - int i;
>> + struct mmc_pwrseq *p;
>>
>> - for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
>> - if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
>> - match = &pwrseq_match[i];
>> - break;
>> - }
>> - }
>> + list_for_each_entry(p, &pwrseq_list, list)
>> + if (p->dev->of_node == np)
>> + return p;
>>
>> - return match;
>> + return NULL;
>> }
>>
>> int mmc_pwrseq_alloc(struct mmc_host *host)
>> {
>> - struct platform_device *pdev;
>> struct device_node *np;
>> - struct mmc_pwrseq_match *match;
>> struct mmc_pwrseq *pwrseq;
>> int ret = 0;
>>
>> @@ -59,25 +39,18 @@ int mmc_pwrseq_alloc(struct mmc_host *host)
>> if (!np)
>> return 0;
>>
>> - pdev = of_find_device_by_node(np);
>> - if (!pdev) {
>> - ret = -ENODEV;
>> - goto err;
>> - }
>> + pwrseq = of_find_mmc_pwrseq(np);
>>
>> - match = mmc_pwrseq_find(np);
>> - if (IS_ERR(match)) {
>> - ret = PTR_ERR(match);
>> - goto err;
>> - }
>> + if (pwrseq && pwrseq->ops && pwrseq->ops->alloc) {
>> + host->pwrseq = pwrseq;
>> + ret = pwrseq->ops->alloc(host);
>> + if (IS_ERR_VALUE(ret)) {
>> + host->pwrseq = NULL;
>> + goto err;
>> + }
>>
>> - pwrseq = match->alloc(host, &pdev->dev);
>> - if (IS_ERR(pwrseq)) {
>> - ret = PTR_ERR(pwrseq);
>> - goto err;
>> }
>>
>> - host->pwrseq = pwrseq;
>> dev_info(host->parent, "allocated mmc-pwrseq\n");
>>
>> err:
>> @@ -89,7 +62,7 @@ void mmc_pwrseq_pre_power_on(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->pre_power_on)
>> + if (pwrseq && pwrseq->ops->pre_power_on)
>> pwrseq->ops->pre_power_on(host);
>> }
>>
>> @@ -97,7 +70,7 @@ void mmc_pwrseq_post_power_on(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->post_power_on)
>> + if (pwrseq && pwrseq->ops->post_power_on)
>> pwrseq->ops->post_power_on(host);
>> }
>>
>> @@ -105,7 +78,7 @@ void mmc_pwrseq_power_off(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->power_off)
>> + if (pwrseq && pwrseq->ops->power_off)
>> pwrseq->ops->power_off(host);
>> }
>>
>> @@ -113,8 +86,34 @@ void mmc_pwrseq_free(struct mmc_host *host)
>> {
>> struct mmc_pwrseq *pwrseq = host->pwrseq;
>>
>> - if (pwrseq && pwrseq->ops && pwrseq->ops->free)
>> - pwrseq->ops->free(host);
>> + if (pwrseq) {
>> + if (pwrseq->ops->free)
>> + pwrseq->ops->free(host);
>> +
>> + host->pwrseq = NULL;
>> + }
>>
>> - host->pwrseq = NULL;
>> }
>> +
>> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq)
>> +{
>> + if (!pwrseq || !pwrseq->ops || !pwrseq->dev)
>> + return -EINVAL;
>> +
>> + mutex_lock(&pwrseq_list_mutex);
>> + list_add(&pwrseq->list, &pwrseq_list);
>> + mutex_unlock(&pwrseq_list_mutex);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_pwrseq_register);
>> +
>> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq)
>> +{
>> + if (pwrseq) {
>> + mutex_lock(&pwrseq_list_mutex);
>> + list_del(&pwrseq->list);
>> + mutex_unlock(&pwrseq_list_mutex);
>> + }
>> +}
>> +EXPORT_SYMBOL_GPL(mmc_pwrseq_unregister);
>> diff --git a/drivers/mmc/core/pwrseq.h b/drivers/mmc/core/pwrseq.h
>> index 096da48..08a7d2d 100644
>> --- a/drivers/mmc/core/pwrseq.h
>> +++ b/drivers/mmc/core/pwrseq.h
>> @@ -8,7 +8,10 @@
>> #ifndef _MMC_CORE_PWRSEQ_H
>> #define _MMC_CORE_PWRSEQ_H
>>
>> +#include <linux/mmc/host.h>
>> +
>> struct mmc_pwrseq_ops {
>> + int (*alloc)(struct mmc_host *host);
>> void (*pre_power_on)(struct mmc_host *host);
>> void (*post_power_on)(struct mmc_host *host);
>> void (*power_off)(struct mmc_host *host);
>> @@ -17,21 +20,21 @@ struct mmc_pwrseq_ops {
>>
>> struct mmc_pwrseq {
>> struct mmc_pwrseq_ops *ops;
>> + struct device *dev;
>> + struct list_head list;
>> };
>>
>> #ifdef CONFIG_OF
>>
>> +int mmc_pwrseq_register(struct mmc_pwrseq *pwrseq);
>> +void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq);
>> +
>> int mmc_pwrseq_alloc(struct mmc_host *host);
>> void mmc_pwrseq_pre_power_on(struct mmc_host *host);
>> void mmc_pwrseq_post_power_on(struct mmc_host *host);
>> void mmc_pwrseq_power_off(struct mmc_host *host);
>> void mmc_pwrseq_free(struct mmc_host *host);
>>
>> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
>> - struct device *dev);
>> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>> - struct device *dev);
>> -
>> #else
>>
>> static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
>> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
>> index 7978fb7..75f7423 100644
>> --- a/drivers/mmc/core/pwrseq_emmc.c
>> +++ b/drivers/mmc/core/pwrseq_emmc.c
>> @@ -9,6 +9,7 @@
>> */
>> #include <linux/delay.h>
>> #include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> #include <linux/slab.h>
>> #include <linux/device.h>
>> #include <linux/err.h>
>> @@ -48,14 +49,8 @@ static void mmc_pwrseq_emmc_free(struct mmc_host *host)
>>
>> unregister_restart_handler(&pwrseq->reset_nb);
>> gpiod_put(pwrseq->reset_gpio);
>> - kfree(pwrseq);
>> }
>>
>> -static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
>> - .post_power_on = mmc_pwrseq_emmc_reset,
>> - .free = mmc_pwrseq_emmc_free,
>> -};
>> -
>> static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
>> unsigned long mode, void *cmd)
>> {
>> @@ -66,21 +61,14 @@ static int mmc_pwrseq_emmc_reset_nb(struct notifier_block *this,
>> return NOTIFY_DONE;
>> }
>>
>> -struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>> - struct device *dev)
>> +static int mmc_pwrseq_emmc_alloc(struct mmc_host *host)
>> {
>> - struct mmc_pwrseq_emmc *pwrseq;
>> - int ret = 0;
>> -
>> - pwrseq = kzalloc(sizeof(struct mmc_pwrseq_emmc), GFP_KERNEL);
>> - if (!pwrseq)
>> - return ERR_PTR(-ENOMEM);
>> + struct mmc_pwrseq_emmc *pwrseq = to_pwrseq_emmc(host->pwrseq);
>>
>> - pwrseq->reset_gpio = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> - if (IS_ERR(pwrseq->reset_gpio)) {
>> - ret = PTR_ERR(pwrseq->reset_gpio);
>> - goto free;
>> - }
>> + pwrseq->reset_gpio = gpiod_get(host->pwrseq->dev,
>> + "reset", GPIOD_OUT_LOW);
>> + if (IS_ERR(pwrseq->reset_gpio))
>> + return PTR_ERR(pwrseq->reset_gpio);
>>
>> /*
>> * register reset handler to ensure emmc reset also from
>> @@ -91,10 +79,41 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>> pwrseq->reset_nb.priority = 255;
>> register_restart_handler(&pwrseq->reset_nb);
>>
>> + return 0;
>> +}
>> +
>> +static struct mmc_pwrseq_ops mmc_pwrseq_emmc_ops = {
>> + .alloc = mmc_pwrseq_emmc_alloc,
>> + .post_power_on = mmc_pwrseq_emmc_reset,
>> + .free = mmc_pwrseq_emmc_free,
>> +};
>> +
>> +static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
>> +{
>> + struct mmc_pwrseq_emmc *pwrseq;
>> + struct device *dev = &pdev->dev;
>> +
>> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
>> + if (!pwrseq)
>> + return -ENOMEM;
>> +
>> pwrseq->pwrseq.ops = &mmc_pwrseq_emmc_ops;
>> + pwrseq->pwrseq.dev = dev;
>>
>> - return &pwrseq->pwrseq;
>> -free:
>> - kfree(pwrseq);
>> - return ERR_PTR(ret);
>> + return mmc_pwrseq_register(&pwrseq->pwrseq);
>> }
>> +
>> +static const struct of_device_id mmc_pwrseq_emmc_of_match[] = {
>> + { .compatible = "mmc-pwrseq-emmc",},
>> + {/* sentinel */},
>> +};
>> +
>> +static struct platform_driver mmc_pwrseq_emmc_driver = {
>> + .probe = mmc_pwrseq_emmc_probe,
>> + .driver = {
>> + .name = "pwrseq_emmc",
>> + .of_match_table = mmc_pwrseq_emmc_of_match,
>> + },
>> +};
>> +
>> +builtin_platform_driver(mmc_pwrseq_emmc_driver);
>> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
>> index c091f98..b04cc2d 100644
>> --- a/drivers/mmc/core/pwrseq_simple.c
>> +++ b/drivers/mmc/core/pwrseq_simple.c
>> @@ -9,6 +9,7 @@
>> */
>> #include <linux/clk.h>
>> #include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> #include <linux/slab.h>
>> #include <linux/device.h>
>> #include <linux/err.h>
>> @@ -82,46 +83,66 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
>> if (!IS_ERR(pwrseq->ext_clk))
>> clk_put(pwrseq->ext_clk);
>>
>> - kfree(pwrseq);
>> }
>>
>> -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_power_off,
>> - .free = mmc_pwrseq_simple_free,
>> -};
>> -
>> -struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
>> - struct device *dev)
>> +int mmc_pwrseq_simple_alloc(struct mmc_host *host)
>> {
>> - struct mmc_pwrseq_simple *pwrseq;
>> + struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
>> + struct device *dev = host->pwrseq->dev;
>> int ret = 0;
>>
>> - pwrseq = kzalloc(sizeof(*pwrseq), GFP_KERNEL);
>> - if (!pwrseq)
>> - return ERR_PTR(-ENOMEM);
>> -
>> pwrseq->ext_clk = clk_get(dev, "ext_clock");
>> if (IS_ERR(pwrseq->ext_clk) &&
>> PTR_ERR(pwrseq->ext_clk) != -ENOENT) {
>> - ret = PTR_ERR(pwrseq->ext_clk);
>> - goto free;
>> + return PTR_ERR(pwrseq->ext_clk);
>> +
>> }
>>
>> pwrseq->reset_gpios = gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
>> if (IS_ERR(pwrseq->reset_gpios)) {
>> ret = PTR_ERR(pwrseq->reset_gpios);
>> - goto clk_put;
>> + clk_put(pwrseq->ext_clk);
>> + return ret;
>> }
>>
>> + return 0;
>> +}
>> +
>> +static struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
>> + .alloc = mmc_pwrseq_simple_alloc,
>> + .pre_power_on = mmc_pwrseq_simple_pre_power_on,
>> + .post_power_on = mmc_pwrseq_simple_post_power_on,
>> + .power_off = mmc_pwrseq_simple_power_off,
>> + .free = mmc_pwrseq_simple_free,
>> +};
>> +
>> +static const struct of_device_id mmc_pwrseq_simple_of_match[] = {
>> + { .compatible = "mmc-pwrseq-simple",},
>> + {/* sentinel */},
>> +};
>> +
>> +static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
>> +{
>> + struct mmc_pwrseq_simple *pwrseq;
>> + struct device *dev = &pdev->dev;
>> +
>> + pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
>> + if (!pwrseq)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + pwrseq->pwrseq.dev = dev;
>> pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
>>
>> - return &pwrseq->pwrseq;
>> -clk_put:
>> - if (!IS_ERR(pwrseq->ext_clk))
>> - clk_put(pwrseq->ext_clk);
>> -free:
>> - kfree(pwrseq);
>> - return ERR_PTR(ret);
>> + return mmc_pwrseq_register(&pwrseq->pwrseq);
>> }
>> +
>> +
>> +static struct platform_driver mmc_pwrseq_simple_driver = {
>> + .probe = mmc_pwrseq_simple_probe,
>> + .driver = {
>> + .name = "pwrseq_simple",
>> + .of_match_table = mmc_pwrseq_simple_of_match,
>> + },
>> +};
>> +
>> +builtin_platform_driver(mmc_pwrseq_simple_driver);
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-01-19 14:05 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-19 10:57 [RFC PATCH 0/3] mmc: pwrseq: convert to proper driver Srinivas Kandagatla
2016-01-19 10:57 ` Srinivas Kandagatla
2016-01-19 10:58 ` [RFC PATCH 1/3] mmc: pwrseq_simple: add to_pwrseq_simple() macro Srinivas Kandagatla
2016-01-19 10:58 ` Srinivas Kandagatla
2016-01-19 10:59 ` [RFC PATCH 2/3] mmc: pwrseq_emmc: add to_pwrseq_emmc() macro Srinivas Kandagatla
2016-01-19 10:59 ` Srinivas Kandagatla
2016-01-19 10:59 ` [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device Srinivas Kandagatla
2016-01-19 10:59 ` Srinivas Kandagatla
2016-01-19 13:01 ` Ulf Hansson
2016-01-19 13:01 ` Ulf Hansson
2016-01-19 13:01 ` Ulf Hansson
2016-01-19 14:05 ` Srinivas Kandagatla
2016-01-19 14:05 ` Srinivas Kandagatla
2016-01-19 14:05 ` Srinivas Kandagatla
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.