Re: [RFC PATCH 08/15] ata: ahci_platform: Manage SATA PHY

[Roger Quadros <rogerq@ti.com>]

Hi,

On 09/22/2013 09:22 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/19/2013 05:05 PM, Roger Quadros wrote:
> 
>> From: Balaji T K <balajitk@ti.com>
> 
>> Some platforms have a PHY hooked up to the
>> SATA controller. The PHY needs to be initialized
>> and powered up for SATA to work. We do that
>> using the PHY framework.
> 
>> [Roger Q] Cleaned up.
> 
>> CC: Tejun Heo <tj@kernel.org>
>> Signed-off-by: Balaji T K <balajitk@ti.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> [...]
> 
>> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
>> index 1145637..94484cb 100644
>> --- a/drivers/ata/ahci.h
>> +++ b/drivers/ata/ahci.h
>> @@ -37,6 +37,7 @@
>>
>>   #include <linux/clk.h>
>>   #include <linux/libata.h>
>> +#include <linux/phy/phy.h>
> 
> struct phy;
> 
> should suffice.
> 
>> @@ -322,6 +323,7 @@ struct ahci_host_priv {
>>       u32            em_buf_sz;    /* EM buffer size in byte */
>>       u32            em_msg_type;    /* EM message type */
>>       struct clk        *clk;        /* Only for platforms supporting clk */
>> +    struct phy        *phy;        /* If platforms use phy */
>>       void            *plat_data;    /* Other platform data */
>>   };
>>
>> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
>> index 2daaee0..f812ffa 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/phy/phy.h>
> 
>    Why include it from both ahci.h and here?
> 
OK. will move it to just .c file.

>>   #include "ahci.h"
>>
>>   static void ahci_host_stop(struct ata_host *host);
>> @@ -141,16 +142,32 @@ static int ahci_probe(struct platform_device *pdev)
>>           }
>>       }
>>
>> +    hpriv->phy = devm_phy_get(dev, "sata-phy");
>> +    if (IS_ERR(hpriv->phy)) {
>> +        dev_err(dev, "can't get phy\n");
> 
>    Don't think it's a good idea to complain about missing PHY when the driver doesn't even use it.

OK. will change it to dev_dbg() instead.
> 
>> +        /* return only if -EPROBE_DEFER */
>> +        if (PTR_ERR(hpriv->phy) == -EPROBE_DEFER) {
>> +            rc = -EPROBE_DEFER;
>> +            goto disable_unprepare_clk;
>> +        }
>> +    }
>> +
>>       /*
>>        * Some platforms might need to prepare for mmio region access,
>>        * which could be done in the following init call. So, the mmio
>>        * region shouldn't be accessed before init (if provided) has
>>        * returned successfully.
>>        */
>> +
>> +    if (!(IS_ERR(hpriv->phy))) {
> 
>    () not needed around IS_ERR() invocation.

OK.

> 
>> +        phy_init(hpriv->phy);
>> +        phy_power_on(hpriv->phy);
>> +    }
>> +
> 
>    I think this is misplaced, i.e. it should precede the comment.
> 

OK.

>>       if (pdata && pdata->init) {
>>           rc = pdata->init(dev, hpriv->mmio);
>>           if (rc)
>> -            goto disable_unprepare_clk;
>> +            goto disable_phy;
>>       }
>>
>>       ahci_save_initial_config(dev, hpriv,
> [...]
>> @@ -328,6 +356,7 @@ static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_suspend, ahci_resume);
>>   static const struct of_device_id ahci_of_match[] = {
>>       { .compatible = "snps,spear-ahci", },
>>       { .compatible = "snps,exynos5440-ahci", },
>> +    { .compatible = "snps,dwc-ahci", },
> 
>    Looks like the binding documentation is incomplete -- it doesn't list "snps,exynos5440-ahci"...

OK, I'll update it. Thanks for review.

cheers,
-roger