On 28 January 2016 at 11:03, Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> 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 solve 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@xxxxxxxxxx>
Full review this time. :-)
And sorry for the delay in reviewing.
> ---
> drivers/mmc/Kconfig | 2 +
> drivers/mmc/core/Kconfig | 7 +++
> drivers/mmc/core/Makefile | 4 +-
> drivers/mmc/core/pwrseq.c | 115 +++++++++++++++++++++------------------
> drivers/mmc/core/pwrseq.h | 19 +++++--
> drivers/mmc/core/pwrseq_emmc.c | 81 +++++++++++++++++++--------
> drivers/mmc/core/pwrseq_simple.c | 85 ++++++++++++++++++++---------
> 7 files changed, 207 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index f2eeb38..7b2412a 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -5,6 +5,8 @@
> menuconfig MMC
> tristate "MMC/SD/SDIO card support"
> depends on HAS_IOMEM
> + select PWRSEQ_SIMPLE if OF
> + select PWRSEQ_EMMC if OF
In general I don't like "select" and for this case I think there is a
better way. See below.
> help
> This selects MultiMediaCard, Secure Digital and Secure
> Digital I/O support.
> diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
> index 4c33d76..b26f756 100644
> --- a/drivers/mmc/core/Kconfig
> +++ b/drivers/mmc/core/Kconfig
> @@ -1,3 +1,10 @@
> #
> # MMC core configuration
> #
> +config PWRSEQ_EMMC
> + tristate "PwrSeq EMMC"
I suggest change this to:
bool "HW reset support for eMMC"
> + depends on OF
Add:
default y
Also I think some brief "help" text, describing the feature would be nice.
> +
> +config PWRSEQ_SIMPLE
> + tristate "PwrSeq Simple"
> + depends on OF
Similar comments as above for PWRSEQ_EMMC.
> diff --git a/drivers/mmc/core/Makefile b/drivers/mmc/core/Makefile
> index 2c25138..f007151 100644
> --- a/drivers/mmc/core/Makefile
> +++ b/drivers/mmc/core/Makefile
> @@ -8,5 +8,7 @@ mmc_core-y := core.o bus.o host.o \
> sdio.o sdio_ops.o sdio_bus.o \
> sdio_cis.o sdio_io.o sdio_irq.o \
> quirks.o slot-gpio.o
> -mmc_core-$(CONFIG_OF) += pwrseq.o pwrseq_simple.o pwrseq_emmc.o
> +mmc_core-$(CONFIG_OF) += pwrseq.o
> +obj-$(CONFIG_PWRSEQ_SIMPLE) += pwrseq_simple.o
> +obj-$(CONFIG_PWRSEQ_EMMC) += pwrseq_emmc.o
> mmc_core-$(CONFIG_DEBUG_FS) += debugfs.o
> diff --git a/drivers/mmc/core/pwrseq.c b/drivers/mmc/core/pwrseq.c
> index 4c1d175..64c7c79 100644
> --- a/drivers/mmc/core/pwrseq.c
> +++ b/drivers/mmc/core/pwrseq.c
> @@ -8,80 +8,64 @@
> * MMC power sequence management
> */
> #include <linux/kernel.h>
> -#include <linux/platform_device.h>
> #include <linux/err.h>
> +#include <linux/module.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 mmc_host *host)
> {
> - struct mmc_pwrseq_match *match = ERR_PTR(-ENODEV);
> - int i;
> + struct device_node *np;
> + struct mmc_pwrseq *p, *pwrseq = NULL;
>
> - for (i = 0; i < ARRAY_SIZE(pwrseq_match); i++) {
> - if (of_device_is_compatible(np, pwrseq_match[i].compatible)) {
> - match = &pwrseq_match[i];
> + np = of_parse_phandle(host->parent->of_node, "mmc-pwrseq", 0);
> + if (!np)
> + return NULL;
> +
> + mutex_lock(&pwrseq_list_mutex);
> + list_for_each_entry(p, &pwrseq_list, list) {
> + if (p->dev->of_node == np) {
> + pwrseq = p;
> break;
> }
> }
>
> - return match;
> + of_node_put(np);
> + mutex_unlock(&pwrseq_list_mutex);
> +
> + return pwrseq ? : ERR_PTR(-EPROBE_DEFER);
> }
>
> 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;
>
> - np = of_parse_phandle(host->parent->of_node, "mmc-pwrseq", 0);
> - if (!np)
> - return 0;
> + pwrseq = of_find_mmc_pwrseq(host);
>
I think you can remove another empty line here.
> - pdev = of_find_device_by_node(np);
> - if (!pdev) {
> - ret = -ENODEV;
> - goto err;
> - }
> + if (IS_ERR_OR_NULL(pwrseq))
> + return PTR_ERR(pwrseq);
You need "return PTR_ERR_OR_ZERO(pwrseq);", as pwrseq can be NULL here.
>
> - match = mmc_pwrseq_find(np);
> - if (IS_ERR(match)) {
> - ret = PTR_ERR(match);
> - goto err;
> - }
> + if (pwrseq->ops && pwrseq->ops->alloc) {
1)
I think we need to decide whether the pwrseq->ops pointer should be
optional or not.
Currently from the mmc_pwrseq_register() API, you prevent a pwrseq
from being registered unless the ops is provided. That means the above
validation of the ops pointer is redundant.
Although, I am thinking that we should allow the ops to be NULL to
provide some more flexibility. Thus the above check could remain as
is.
2)
As a matter of fact, I don't think the ops->alloc|free() functions are
needed any more. The corresponding platform driver will now be able
alloc its resourses during ->probe() and drop them at ->remove() (or
even use devm_*() APIs).
> + host->pwrseq = pwrseq;
> + ret = pwrseq->ops->alloc(host);
>
> - pwrseq = match->alloc(host, &pdev->dev);
> - if (IS_ERR(pwrseq)) {
> - ret = PTR_ERR(pwrseq);
> - goto err;
> + if (IS_ERR_VALUE(ret)) {
> + host->pwrseq = NULL;
> + goto err;
> + }
> + try_module_get(pwrseq->owner);
I don't think this fragile.
For example, what happens if the pwrseq platform driver module becomes
removed and thus called mmc_pwrseq_unregister() before invoking
try_module_get()?
Perhaps extending the region for where pwrseq_list_mutex is held can
help and in combination of checking the return value from
try_module_get()?
Finally, pwrseq->owner may be NULL as you don't validate that in
mmc_pwrseq_register().
> }
>
> - host->pwrseq = pwrseq;
> dev_info(host->parent, "allocated mmc-pwrseq\n");
>
> err:
> - of_node_put(np);
> return ret;
> }
>
> @@ -89,7 +73,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)
See upper comment, whether we should allow ops to be NULL.
> + if (pwrseq && pwrseq->ops->pre_power_on)
> pwrseq->ops->pre_power_on(host);
> }
>
> @@ -97,7 +81,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)
Ditto.
> pwrseq->ops->post_power_on(host);
> }
>
> @@ -105,7 +89,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)
Ditto.
> pwrseq->ops->power_off(host);
> }
>
> @@ -113,8 +97,35 @@ 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)
See upper comment. I think the callback ops->free can be removed.
> + pwrseq->ops->free(host);
> + module_put(pwrseq->owner);
> +
> + 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);
>
> - host->pwrseq = NULL;
> +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 133de04..913587c 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,23 +20,29 @@ struct mmc_pwrseq_ops {
>
> struct mmc_pwrseq {
> const struct mmc_pwrseq_ops *ops;
> + struct device *dev;
> + struct list_head list;
I would prefer to rename "list" to "pwrseq_node", to reflect that it's
node in the pwrseq_list.
> + struct module *owner;
> };
>
> #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_register(struct mmc_pwrseq *pwrseq)
> +{
> + return -ENOSYS;
> +}
> +static inline void mmc_pwrseq_unregister(struct mmc_pwrseq *pwrseq) {}
> static inline int mmc_pwrseq_alloc(struct mmc_host *host) { return 0; }
> static inline void mmc_pwrseq_pre_power_on(struct mmc_host *host) {}
> static inline void mmc_pwrseq_post_power_on(struct mmc_host *host) {}
> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
> index c2d732a..1b14e32 100644
> --- a/drivers/mmc/core/pwrseq_emmc.c
> +++ b/drivers/mmc/core/pwrseq_emmc.c
> @@ -9,6 +9,9 @@
> */
> #include <linux/delay.h>
> #include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -48,14 +51,8 @@ static void mmc_pwrseq_emmc_free(struct mmc_host *host)
According to upper comments, this entire code should go into a
->remove() function.
>
> unregister_restart_handler(&pwrseq->reset_nb);
> gpiod_put(pwrseq->reset_gpio);
> - kfree(pwrseq);
> }
>
> -static const 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 +63,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)
According to upper comments, this entire code should go into a
->probe() function.
> {
> - 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 +81,55 @@ 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 const 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;
> + pwrseq->pwrseq.owner = THIS_MODULE;
> +
> + return mmc_pwrseq_register(&pwrseq->pwrseq);
> +}
> +
> +static int mmc_pwrseq_emmc_remove(struct platform_device *pdev)
> +{
> + struct mmc_pwrseq_emmc *spwrseq = platform_get_drvdata(pdev);
I think you need to call platform_set_drvdata() in ->probe() to allow
this to work.
> +
> + mmc_pwrseq_unregister(&spwrseq->pwrseq);
>
> - return &pwrseq->pwrseq;
> -free:
> - kfree(pwrseq);
> - return ERR_PTR(ret);
> + return 0;
> }
> +
> +static const struct of_device_id mmc_pwrseq_emmc_of_match[] = {
> + { .compatible = "mmc-pwrseq-emmc",},
> + {/* sentinel */},
> +};
> +
> +MODULE_DEVICE_TABLE(of, mmc_pwrseq_emmc_of_match);
> +
> +static struct platform_driver mmc_pwrseq_emmc_driver = {
> + .probe = mmc_pwrseq_emmc_probe,
> + .remove = mmc_pwrseq_emmc_remove,
> + .driver = {
> + .name = "pwrseq_emmc",
> + .of_match_table = mmc_pwrseq_emmc_of_match,
> + },
> +};
> +
> +module_platform_driver(mmc_pwrseq_emmc_driver);
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
Similar comment to changes in this file as for pwrseq_emmc.c.
> index 03573e1..2f509ca 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -8,7 +8,10 @@
> * Simple MMC power sequence management
> */
> #include <linux/clk.h>
> +#include <linux/init.h>
> #include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -86,31 +89,19 @@ static void mmc_pwrseq_simple_free(struct mmc_host *host)
> if (!IS_ERR(pwrseq->ext_clk))
> clk_put(pwrseq->ext_clk);
>
> - kfree(pwrseq);
> }
>
> -static const 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);
> @@ -118,16 +109,60 @@ struct mmc_pwrseq *mmc_pwrseq_simple_alloc(struct mmc_host *host,
> PTR_ERR(pwrseq->reset_gpios) != -ENOENT &&
> PTR_ERR(pwrseq->reset_gpios) != -ENOSYS) {
> ret = PTR_ERR(pwrseq->reset_gpios);
> - goto clk_put;
> + clk_put(pwrseq->ext_clk);
> + return ret;
> }
>
> + return 0;
> +}
> +
> +static const 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 */},
> +};
> +MODULE_DEVICE_TABLE(of, mmc_pwrseq_simple_of_match);
> +
> +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 -ENOMEM;
> +
> + pwrseq->pwrseq.dev = dev;
> pwrseq->pwrseq.ops = &mmc_pwrseq_simple_ops;
> + pwrseq->pwrseq.owner = THIS_MODULE;
>
> - 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 int mmc_pwrseq_simple_remove(struct platform_device *pdev)
> +{
> + struct mmc_pwrseq_simple *spwrseq = platform_get_drvdata(pdev);
> +
> + mmc_pwrseq_unregister(&spwrseq->pwrseq);
> +
> + return 0;
> +}
> +
> +static struct platform_driver mmc_pwrseq_simple_driver = {
> + .probe = mmc_pwrseq_simple_probe,
> + .remove = mmc_pwrseq_simple_remove,
> + .driver = {
> + .name = "pwrseq_simple",
> + .of_match_table = mmc_pwrseq_simple_of_match,
> + },
> +};
> +
> +module_platform_driver(mmc_pwrseq_simple_driver);
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
Overall I like where this is going, so please keep up the good work. I
am looking forward to review a new version.
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
