From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932080AbcDNMHQ (ORCPT ); Thu, 14 Apr 2016 08:07:16 -0400 Received: from mail-wm0-f43.google.com ([74.125.82.43]:36938 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754292AbcDNMHL (ORCPT ); Thu, 14 Apr 2016 08:07:11 -0400 Subject: Re: [PATCH v3 3/3] mmc: pwrseq: convert to proper platform device To: Ulf Hansson References: <1460570081-1679-1-git-send-email-srinivas.kandagatla@linaro.org> <1460570081-1679-4-git-send-email-srinivas.kandagatla@linaro.org> Cc: linux-mmc , "linux-kernel@vger.kernel.org" , Martin Fuzzey From: Srinivas Kandagatla Message-ID: <570F87EC.2070500@linaro.org> Date: Thu, 14 Apr 2016 13:07:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 14/04/16 10:33, Ulf Hansson wrote: > On 13 April 2016 at 19:54, Srinivas Kandagatla > 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 >