From patchwork Mon Jan 27 14:27:12 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roger Quadros X-Patchwork-Id: 3542401 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 159139F391 for ; Mon, 27 Jan 2014 14:28:04 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id C5AA220154 for ; Mon, 27 Jan 2014 14:28:02 +0000 (UTC) Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D2FD6200FF for ; Mon, 27 Jan 2014 14:27:57 +0000 (UTC) Received: from merlin.infradead.org ([2001:4978:20e::2]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1W7nAH-0001LR-AN; Mon, 27 Jan 2014 14:27:53 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1W7nAE-0007dU-Vj; Mon, 27 Jan 2014 14:27:50 +0000 Received: from comal.ext.ti.com ([198.47.26.152]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1W7nAB-0007cf-Px for linux-arm-kernel@lists.infradead.org; Mon, 27 Jan 2014 14:27:48 +0000 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by comal.ext.ti.com (8.13.7/8.13.7) with ESMTP id s0RERGR8008716; Mon, 27 Jan 2014 08:27:16 -0600 Received: from DFLE72.ent.ti.com (dfle72.ent.ti.com [128.247.5.109]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id s0RERFdC013937; Mon, 27 Jan 2014 08:27:16 -0600 Received: from dflp33.itg.ti.com (10.64.6.16) by DFLE72.ent.ti.com (128.247.5.109) with Microsoft SMTP Server id 14.2.342.3; Mon, 27 Jan 2014 08:27:15 -0600 Received: from [192.168.2.6] (ileax41-snat.itg.ti.com [10.172.224.153]) by dflp33.itg.ti.com (8.14.3/8.13.8) with ESMTP id s0RERCrR013471; Mon, 27 Jan 2014 08:27:13 -0600 Message-ID: <52E66CC0.3080205@ti.com> Date: Mon, 27 Jan 2014 16:27:12 +0200 From: Roger Quadros User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Hans de Goede , Tejun Heo Subject: Re: [PATCH v5 07/14] ahci-platform: "Library-ise" ahci_probe functionality References: <1390417489-5354-1-git-send-email-hdegoede@redhat.com> <1390417489-5354-8-git-send-email-hdegoede@redhat.com> <52E63778.5000509@ti.com> <52E63A1F.6080301@redhat.com> <52E63D08.6080704@ti.com> <52E642F7.3000308@redhat.com> In-Reply-To: <52E642F7.3000308@redhat.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140127_092747_922806_3FEB9CB2 X-CRM114-Status: GOOD ( 30.68 ) X-Spam-Score: -7.4 (-------) Cc: devicetree , linux-ide@vger.kernel.org, Oliver Schinagl , Richard Zhu , linux-sunxi@googlegroups.com, Maxime Ripard , linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 01/27/2014 01:28 PM, Hans de Goede wrote: > Hi, > > On 01/27/2014 12:03 PM, Roger Quadros wrote: >> On 01/27/2014 12:51 PM, Hans de Goede wrote: >>> Hi, >>> >>> On 01/27/2014 11:39 AM, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 01/22/2014 09:04 PM, Hans de Goede wrote: >>> >>> >>> >>>>> --- a/include/linux/ahci_platform.h >>>>> +++ b/include/linux/ahci_platform.h >>>>> @@ -20,7 +20,13 @@ >>>>> struct device; >>>>> struct ata_port_info; >>>>> struct ahci_host_priv; >>>>> +struct platform_device; >>>>> >>>>> +/* >>>>> + * Note ahci_platform_data is deprecated. New drivers which need to override >>>>> + * any of these, should instead declare there own platform_driver struct, and >>>>> + * use ahci_platform* functions in their own probe, suspend and resume methods. >>>>> + */ >>>>> struct ahci_platform_data { >>>>> int (*init)(struct device *dev, struct ahci_host_priv *hpriv); >>>>> void (*exit)(struct device *dev); >>>>> @@ -35,5 +41,13 @@ int ahci_platform_enable_clks(struct ahci_host_priv *hpriv); >>>>> void ahci_platform_disable_clks(struct ahci_host_priv *hpriv); >>>>> int ahci_platform_enable_resources(struct ahci_host_priv *hpriv); >>>>> void ahci_platform_disable_resources(struct ahci_host_priv *hpriv); >>>>> +struct ahci_host_priv *ahci_platform_get_resources( >>>>> + struct platform_device *pdev); >>>> >>>> Why not use 'struct device' as the argument? >>> >>> Because of calls to platform_get_resource inside the function. >>> >>>>> +void ahci_platform_put_resources(struct ahci_host_priv *hpriv); >>>> >>>> Can we have 'struct device' as the argument? Else it becomes >>>> impossible to get 'struct device' from 'hpriv' if we need to call e.g. >>>> pm_runtime_*() APIs. >>> >>> The plan for is for this function to go away once we have a >>> devm version of of_clk_get, so if you need to put pm_runtime_calls >>> somewhere, please don't put them here. This sounds like something which >>> should go in enable / disable resources instead ? >> >> OK. I need to add pm_runtime_enable() + pm_runtime_get_sync() during >> initialization and pm_runtime_put_sync() + pm_runtime_disable() during cleanup. > > Note that enable / disable resources will get called by (the default implementations > of) suspend / resume too. > > If that is undesirable then I take back what I said before and > ahci_platform_put_resources' prototype should be changed to: > > void ahci_platform_put_resources(struct device *dev, struct ahci_host_priv *hpriv); > > And we will need to keep it around even after we get devm_of_clk_get. > >> If ahci_platform_enable/disable_resources is the right place then we must be >> able to access struct device from there. > > Right, and if not we need to access it from ahci_platform_put_resources(), > which is in essence the same problem. > >> Is it a good to add 'struct device *dev' into the 'struct ahci_host_priv'? >> Then you can leave this series as is and i'll add a new patch for that. > > Normally we get a device * as argument, and get to hpriv like this: > > struct ata_host *host = dev_get_drvdata(dev); > struct ahci_host_priv *hpriv = host->private_data; > > So having a dev * in hpriv is normally not useful. > > But the ata_host gets allocated after the first ahci_platform_enable_resources > call, so we cannot use this there. Likewise disable_resources / put_resources > is used in error handling paths in probe where we don't have an ata_host yet, > so my vote goes to adding a "struct device *dev" as first argument, like I > suggested above for ahci_platform_put_resources. > > This can be done as an add-on patch (if you do don't forget to also fix > ahci_sunxi.c and ahci_imx.c), or I can respin my series to have this from > day one. > > If you want me to do a respin, please let me know which fix you'll need > (the put_resources or the enable/disable one). > For now I'm using get/put_resources to enable runtime PM and enable the device like in the below patch. I'll make the necessary changes to ahci_platform_put_resources(); cheers, -roger diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c index 5ec6fe6..965f4b4 100644 --- a/drivers/ata/ahci_platform.c +++ b/drivers/ata/ahci_platform.c @@ -23,6 +23,7 @@ #include #include #include +#include #include "ahci.h" static void ahci_host_stop(struct ata_host *host); @@ -233,6 +234,9 @@ struct ahci_host_priv *ahci_platform_get_resources( } } + pm_runtime_enable(dev); + pm_runtime_get_sync(dev); + return hpriv; free_clk: @@ -246,6 +250,9 @@ void ahci_platform_put_resources(struct ahci_host_priv *hpriv) { int c; + pm_runtime_put_sync(dev); + pm_runtime_disable(dev); + for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) clk_put(hpriv->clks[c]); } @@ -478,6 +485,11 @@ int ahci_platform_resume(struct device *dev) if (rc) goto disable_resources; + /* We resumed so update PM runtime state */ + pm_runtime_disable(dev); + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + return 0; disable_resources: