[PATCH V9 2/7] phy: Add drivers for PCIe and SATA phy on SPEAr13xx

[Viresh Kumar]

On 10 July 2014 18:47, Kishon Vijay Abraham I <kishon at ti.com> wrote:
> On Thursday 10 July 2014 12:56 PM, Viresh Kumar wrote:
>> From: Pratyush Anand <pratyush.anand at st.com>
>>
>> ARM based ST Microelectronics's SPEAr1310/40 platforms uses ST's phy (known as
>> 'miphy') for PCIe and SATA. This patch adds drivers for these miphys.
>>
>> This also adds proper bindings for miphys.
>>
>> Acked-by: Arnd Bergmann <arnd at arndb.de>
>> Signed-off-by: Pratyush Anand <pratyush.anand at st.com>
>> Tested-by: Mohit Kumar <mohit.kumar at st.com>
>> Cc: Kishon Vijay Abraham I <kishon at ti.com>
>> [viresh: fixed logs/cclist/checkpatch warnings, broken into smaller patches]
>> Signed-off-by: Viresh Kumar <viresh.kumar at linaro.org>
>> ---
>>  .../devicetree/bindings/phy/st-spear1310-miphy.txt |  12 +
>>  .../devicetree/bindings/phy/st-spear1340-miphy.txt |  11 +
>>  drivers/phy/Kconfig                                |  12 +
>>  drivers/phy/Makefile                               |   2 +
>>  drivers/phy/phy-spear1310-miphy.c                  | 274 +++++++++++++++++++
>>  drivers/phy/phy-spear1340-miphy.c                  | 302 +++++++++++++++++++++
>
> Please send separate patche for each driver.

These were both around SPEAr and were on the same lines.
So sending these in a single patch shouldn't be a big issue I believe.

In case another version is required, I may do it.

>> diff --git a/Documentation/devicetree/bindings/phy/st-spear1310-miphy.txt b/Documentation/devicetree/bindings/phy/st-spear1310-miphy.txt
>> new file mode 100644
>> index 0000000..b9b281a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/st-spear1310-miphy.txt
>
> We generally create a single document for a SoC vendor. So just use st-phy.txt.

Probably yes.

> <snip>

Please keep line containing file names while removing stuff, makes it easy
to locate code.

>> +
>> +static int spear1340_miphy_sata_init(struct spear1340_miphy_priv *priv)
>> +{
>> +     regmap_update_bits(priv->misc, SPEAR1340_PCIE_SATA_CFG,
>> +                        SPEAR1340_PCIE_SATA_CFG_MASK,
>> +                        SPEAR1340_SATA_CFG_VAL);
>> +     regmap_update_bits(priv->misc, SPEAR1340_PCIE_MIPHY_CFG,
>> +                        SPEAR1340_PCIE_MIPHY_CFG_MASK,
>> +                        SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK);
>> +     /* Switch on sata power domain */
>> +     regmap_update_bits(priv->misc, SPEAR1340_PCM_CFG,
>> +                        SPEAR1340_PCM_CFG_SATA_POWER_EN,
>> +                        SPEAR1340_PCM_CFG_SATA_POWER_EN);
>> +     msleep(20);
>> +     /* Disable PCIE SATA Controller reset */
>> +     regmap_update_bits(priv->misc, SPEAR1340_PERIP1_SW_RST,
>> +                        SPEAR1340_PERIP1_SW_RSATA, 0);
>> +     msleep(20);
>
> Please add a comment for all delays added.

@Pratyush/Mohit: please let me know what to add here.

>> +#ifdef CONFIG_PM_SLEEP
>> +static int spear1340_miphy_suspend(struct device *dev)
>> +{
>> +     struct spear1340_miphy_priv *priv = dev_get_drvdata(dev);
>> +     int ret = 0;
>> +
>> +     if (priv->mode == SATA)
>> +             ret = spear1340_miphy_sata_exit(priv);
>
> Shouldn't this be spear1340_miphy_init()?
>> +
>> +     return ret;
>> +}
>> +
>> +static int spear1340_miphy_resume(struct device *dev)
>> +{
>> +     struct spear1340_miphy_priv *priv = dev_get_drvdata(dev);
>> +     int ret = 0;
>> +
>> +     if (priv->mode == SATA)
>> +             ret = spear1340_miphy_sata_init(priv);
>
> And here spear1340_miphy_exit()? Why only for sata phys?

@Mohit/Pratyush ??

Thanks Kishon for another round of review :)