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

[Srinivas Kandagatla]

On 14/04/16 10:33, Ulf Hansson wrote:
> On 13 April 2016 at 19:54, 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.
>
> [...]
>
> >   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;
> > +       struct mmc_pwrseq *p, *pwrseq = NULL;
> >
> >          np = of_parse_phandle(host->parent->of_node, "mmc-pwrseq", 0);
> >          if (!np)
> >                  return 0;
> >
> > -       pdev = of_find_device_by_node(np);
> > -       if (!pdev) {
> > -               ret = -ENODEV;
> > -               goto err;
> > +       mutex_lock(&pwrseq_list_mutex);
> > +       list_for_each_entry(p, &pwrseq_list, pwrseq_node) {
> > +               if (p->dev->of_node == np) {
> > +                       pwrseq = p;
> > +                       try_module_get(pwrseq->owner);
>
> This can fail, so please add error handling.
Yes, I will add that and -EPROBE_DEFER in failure cases.

--srini

> > +                       host->pwrseq = pwrseq;
> > +                       break;
> > +               }
> >          }
> >
> > -       match = mmc_pwrseq_find(np);
> > -       if (IS_ERR(match)) {
> > -               ret = PTR_ERR(match);
> > -               goto err;
> > -       }
> > +       of_node_put(np);
> > +       mutex_unlock(&pwrseq_list_mutex);
> >
> > -       pwrseq = match->alloc(host, &pdev->dev);
> > -       if (IS_ERR(pwrseq)) {
> > -               ret = PTR_ERR(pwrseq);
> > -               goto err;
> > -       }
> > +       if (!pwrseq)
> > +               return -EPROBE_DEFER;
> >
> > -       host->pwrseq = pwrseq;
> >          dev_info(host->parent, "allocated mmc-pwrseq\n");
> >
> > -err:
> > -       of_node_put(np);
> > -       return ret;
> > +       return 0;
> >   }
>
> Besides the minor thing above, this looks good to me!
>
> Kind regards
> Uffe