Hi Andy,
On 12/11/2019 12:28 PM, Andy Shevchenko wrote:
On Wed, Dec 11, 2019 at 11:46:16AM -0800, Jae Hyun Yoo wrote:
This commit adds Aspeed PECI adapter driver for Aspeed
AST24xx/25xx/26xx SoCs.
...
+#define ASPEED_PECI_CMD_IDLE_MASK (ASPEED_PECI_CMD_STS_MASK | \
+ ASPEED_PECI_CMD_PIN_MON)
Better looking when the value completely occupies second line without touching
the first.
Yes. Will change it.
...
+static int aspeed_peci_check_idle(struct aspeed_peci *priv)
+{
+ ulong timeout = jiffies + usecs_to_jiffies(ASPEED_PECI_IDLE_CHECK_TIMEOUT_USEC);
+ u32 cmd_sts;
Like in the previous patch this one has hard to read timeout loops with inefficient code.
+ for (;;) {
+ cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
+ if (!(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK))
+ break;
+ if (time_after(jiffies, timeout)) {
This is actually main exit condition (vs. infinite loop).
+ cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
This make no sense. If you would like to have one more iteration, just spell it
explicitly.
+ break;
+ }
+ usleep_range((ASPEED_PECI_IDLE_CHECK_INTERVAL_USEC >> 2) + 1,
+ ASPEED_PECI_IDLE_CHECK_INTERVAL_USEC);
+ }
+
+ return !(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK) ? 0 : -ETIMEDOUT;
Ditto.
+}
Now look at the other variant:
do {
...do something...
if (success)
return 0;
usleep(...);
} while (time_before(...));
return -ETIMEDOUT;
* Easy
* less LOCs
* guaranteed always to be at least one iteration
* has explicitly spelled exit condition
BUT!
In this very case you may do even better if you read iopoll.h, i.e
readl_poll_timeout() has this functionality embedded in the macro.
I see. I'll simplify this function like below:
#include <linux/iopoll.h>
static inline int aspeed_peci_check_idle(struct aspeed_peci *priv)
{
u32 cmd_sts;
return readl_poll_timeout(priv->base + ASPEED_PECI_CMD,
cmd_sts,
!(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK),
ASPEED_PECI_IDLE_CHECK_INTERVAL_USEC,
ASPEED_PECI_IDLE_CHECK_TIMEOUT_USEC);
}
Thanks a lot for your review!
-Jae
