Re: [PATCH v11 06/14] peci: Add Aspeed PECI adapter driver

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

 



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



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux