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

[Srinivas Kandagatla]

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
--
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/