From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-mmc <linux-mmc@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Martin Fuzzey <mfuzzey@parkeon.com>
Subject: Re: [PATCH v3 3/3] mmc: pwrseq: convert to proper platform device
Date: Thu, 14 Apr 2016 13:07:08 +0100 [thread overview]
Message-ID: <570F87EC.2070500@linaro.org> (raw)
In-Reply-To: <CAPDyKFr4rR3g_+TO2PzggVWAC0tKKYNNrAD+d+ywatX9vKpKzA@mail.gmail.com>
On 14/04/16 10:33, Ulf Hansson wrote:
> On 13 April 2016 at 19:54, 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 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
>
prev parent reply other threads:[~2016-04-14 12:07 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-13 17:54 [PATCH v3 0/3] mmc: pwrseq: convert to proper driver Srinivas Kandagatla
2016-04-13 17:54 ` [PATCH v3 1/3] mmc: pwrseq_simple: add to_pwrseq_simple() macro Srinivas Kandagatla
2016-04-13 17:54 ` [PATCH v3 2/3] mmc: pwrseq_emmc: add to_pwrseq_emmc() macro Srinivas Kandagatla
2016-04-13 17:54 ` [PATCH v3 3/3] mmc: pwrseq: convert to proper platform device Srinivas Kandagatla
2016-04-14 9:33 ` Ulf Hansson
2016-04-14 12:07 ` Srinivas Kandagatla [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=570F87EC.2070500@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=mfuzzey@parkeon.com \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).