From mboxrd@z Thu Jan 1 00:00:00 1970 From: kishon@ti.com (Kishon Vijay Abraham I) Date: Thu, 10 Jul 2014 19:02:42 +0530 Subject: [PATCH V9 2/7] phy: Add drivers for PCIe and SATA phy on SPEAr13xx In-Reply-To: References: <53BE926A.2070005@ti.com> Message-ID: <53BE95FA.5080100@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thursday 10 July 2014 07:00 PM, Viresh Kumar wrote: > On 10 July 2014 18:47, Kishon Vijay Abraham I wrote: >> On Thursday 10 July 2014 12:56 PM, Viresh Kumar wrote: >>> From: Pratyush Anand >>> >>> 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 >>> Signed-off-by: Pratyush Anand >>> Tested-by: Mohit Kumar >>> Cc: Kishon Vijay Abraham I >>> [viresh: fixed logs/cclist/checkpatch warnings, broken into smaller patches] >>> Signed-off-by: Viresh Kumar >>> --- >>> .../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. > >> > > 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 :) >