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

[Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>]

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