On Thursday 10 July 2014 07:00 PM, Viresh Kumar wrote:
> 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.
ah.. apologies..
>
>>> +
>>> +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
