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

[Srinivas Kandagatla]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/