Re: [PATCH v2 1/2] phy: qcom: Add driver for QCOM APQ8064 SATA PHY

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thankyou for the comments,

On 11/07/14 13:38, Bartlomiej Zolnierkiewicz wrote:

Hi,

On Wednesday, July 09, 2014 12:04:06 PM Srinivas Kandagatla wrote:
Add a PHY driver for uses with AHCI based SATA controller driver on the
APQ8064 family of SoCs.

This patch is a forward port from Qualcomm's v3.4 andriod kernel.

Android?
yep.

unused define, please remove it

Will fix these instances in next version.
+
+#define UNIPHY_PLL_LOCK		BIT(0)
+#define SATA_PHY_TX_CAL		BIT(0)
+#define SATA_PHY_RX_CAL		BIT(0)
+
+/* default timeout set to 1 sec */
+#define TIMEOUT_MS		10000
+
+struct qcom_apq8064_sata_phy {
+	void __iomem *mmio;
+	struct clk *cfg_clk;
+	struct device *dev;
+};
+
+/* Helper function to do poll and timeout */
+static int read_poll_timeout(void __iomem *addr, u32 mask)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(TIMEOUT_MS);
+	u32 val;
+
+	do {
+		cpu_relax();
+		val = readl_relaxed(addr);
+		if (val & mask)
+			break;
+	} while (!time_after(jiffies, timeout));

It would be better to use usleep_[range]() (or even msleep() if needed)
instead of just using cpu_relax().

We really want to poll the register here, usleep/msleep would be useful if we know already know how much delay is required, but in this case the its not true.

+
+static int qcom_apq8064_sata_phy_remove(struct platform_device *pdev)
+{
+	struct qcom_apq8064_sata_phy *phy = platform_get_drvdata(pdev);

Doesn't this require to use platform_set_drvdata() first in the ->probe
method?
Good spot, I will fix this in next version.

Thanks,
srini
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux