Re: [PATCH v5 07/14] ahci-platform: "Library-ise" ahci_probe functionality

[Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>]

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:
>>>
>>> <snip>
>>>
>>>>> --- 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 <linux/platform_device.h>
 #include <linux/libata.h>
 #include <linux/ahci_platform.h>
+#include <linux/pm_runtime.h>
 #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: