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: 23746 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-ve0-f200.google.com (mail-ve0-f200.google.com [209.85.128.200]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 0E18B20143 for ; Mon, 27 Jan 2014 14:28:07 +0000 (UTC) Received: by mail-ve0-f200.google.com with SMTP id c14sf12603724vea.3 for ; Mon, 27 Jan 2014 06:28:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:message-id:date:from:user-agent :mime-version:to:cc:subject:references:in-reply-to:sender:precedence :list-id:x-original-sender:x-original-authentication-results :mailing-list:list-post:list-help:list-archive:list-unsubscribe :content-type:content-transfer-encoding; bh=EqnMpnIr4fNOuPFECXtQJM0e2jMjuLY++DWtlAV5HxQ=; b=SNnvdyR0YDvYm+RNMkKplrtvlNpomcgLKZcH2xAc8wpjGv2UW7sTn2jof3TKN1APHz ymtAhPk8A54agru/hoEUa15G70HXKTCWqg+rHG6uo8QZRreXUHuiRtXfDYsNJkHjx0ye G7HJGdtSJMM9U11QQXrL3xvycIfcunkGpXvdLikVBur9+JYcF3worESJ88OGQnCZSv8W 3M2A7H3acgQ0zW5SA5yZ0plBx+etCHWfyDBPn053TRRZBXfhWlUYJaCY14QSuXejbi9N 2jivd0t+obLjMPTyBRzIUZvAZ7NCBH0da7t7jLEMaI62pARokmHbzM1RbrTkevApkvxy aWzw== X-Gm-Message-State: ALoCoQmJh3mfIQl6lFKVmz9dhrIDnyXXl6c3+I4paHyX+gypCiJ7jZO1zL0Lqarg4aYPchaGm+sY X-Received: by 10.52.184.228 with SMTP id ex4mr2122558vdc.1.1390832887213; Mon, 27 Jan 2014 06:28:07 -0800 (PST) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.32.116 with SMTP id g107ls1730836qgg.49.gmail; Mon, 27 Jan 2014 06:28:07 -0800 (PST) X-Received: by 10.220.170.68 with SMTP id c4mr62594vcz.41.1390832887074; Mon, 27 Jan 2014 06:28:07 -0800 (PST) Received: from mail-ve0-f170.google.com (mail-ve0-f170.google.com [209.85.128.170]) by mx.google.com with ESMTPS id we7si5156719vcb.7.2014.01.27.06.28.07 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 27 Jan 2014 06:28:07 -0800 (PST) Received-SPF: neutral (google.com: 209.85.128.170 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.128.170; Received: by mail-ve0-f170.google.com with SMTP id cz12so3559140veb.15 for ; Mon, 27 Jan 2014 06:28:07 -0800 (PST) X-Received: by 10.58.172.132 with SMTP id bc4mr45000vec.45.1390832886979; Mon, 27 Jan 2014 06:28:06 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.220.174.196 with SMTP id u4csp132857vcz; Mon, 27 Jan 2014 06:28:06 -0800 (PST) X-Received: by 10.68.40.138 with SMTP id x10mr17121424pbk.8.1390832885744; Mon, 27 Jan 2014 06:28:05 -0800 (PST) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id nf8si11630915pbc.330.2014.01.27.06.28.05; Mon, 27 Jan 2014 06:28:05 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of devicetree-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753473AbaA0O2D (ORCPT + 9 others); Mon, 27 Jan 2014 09:28:03 -0500 Received: from comal.ext.ti.com ([198.47.26.152]:34062 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753138AbaA0O2C (ORCPT ); Mon, 27 Jan 2014 09:28:02 -0500 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 CC: Oliver Schinagl , Maxime Ripard , Richard Zhu , , , devicetree , 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> Sender: devicetree-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: devicetree@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: rogerq@ti.com X-Original-Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.128.170 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , 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 --- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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: