On 10 July 2014 18:47, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
> On Thursday 10 July 2014 12:56 PM, Viresh Kumar wrote:
>> From: Pratyush Anand <pratyush.anand@xxxxxx>
>>
>> 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@xxxxxxxx>
>> Signed-off-by: Pratyush Anand <pratyush.anand@xxxxxx>
>> Tested-by: Mohit Kumar <mohit.kumar@xxxxxx>
>> Cc: Kishon Vijay Abraham I <kishon@xxxxxx>
>> [viresh: fixed logs/cclist/checkpatch warnings, broken into smaller patches]
>> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>> ---
>> .../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 :)
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
