Re: [RFC PATCH 3/3] mmc: pwrseq: convert to proper platform device

[Srinivas Kandagatla]

On 19/01/16 13:01, Ulf Hansson wrote:
> On 19 January 2016 at 11:59, 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 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@xxxxxxxxxx>
> > ---
> >   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