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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

Thanks Bartlomiej for the advice,

I will give it a try and see.

On 11/07/14 15:33, Bartlomiej Zolnierkiewicz wrote:
>>>> 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.
> I don't mean replacing the whole function, you can still do polling with
> i.e. doing usleep_range(1000, 2000) with 1000 retries.  The advantage of
> doing it this way would be that processor could do some useful work or
> sleep during wait time instead of just busy waiting.
>
> One example of many how to do it:
>
> drivers/i2c/busses/i2c-s3c2410.c:
>
> static bool is_ack(struct s3c24xx_i2c *i2c)
> {
>          int tries;
>
>          for (tries = 50; tries; --tries) {
>                  if (readl(i2c->regs + S3C2410_IICCON)
>                          & S3C2410_IICCON_IRQPEND) {
>                          if (!(readl(i2c->regs + S3C2410_IICSTAT)
>                                  & S3C2410_IICSTAT_LASTBIT))
>                                  return true;
>                  }
>                  usleep_range(1000, 2000);
>          }
>          dev_err(i2c->dev, "ack was not recieved\n");
>          return false;
> }


thanks,
srini