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

From: Andy Shevchenko <andriy.shevchenko@intel•com>
To: Jae Hyun Yoo <jae.hyun.yoo@linux•intel.com>
Cc: Mark Rutland <mark.rutland@arm•com>,
	Randy Dunlap <rdunlap@infradead•org>,
	Tomer Maimon <tmaimon77@gmail•com>,
	devicetree@vger•kernel.org,
	Frederic Barrat <fbarrat@linux•vnet.ibm.com>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel•org>,
	Lee Jones <lee.jones@linaro•org>,
	Eric Sandeen <sandeen@redhat•com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm•com>,
	Jonathan Corbet <corbet@lwn•net>,
	openbmc@lists•ozlabs.org, linux-doc@vger•kernel.org,
	Kishon Vijay Abraham I <kishon@ti•com>,
	Joel Stanley <joel@jms•id.au>,
	"Bryant G . Ly" <bryantly@linux•vnet.ibm.com>,
	Uwe Kleine-Konig <u.kleine-koenig@pengutronix•de>,
	David Kershner <david.kershner@unisys•com>,
	Guenter Roeck <linux@roeck-us•net>, Wu Hao <hao.wu@intel•com>,
	linux-hwmon@vger•kernel.org, Jean Delvare <jdelvare@suse•com>,
	Arnd Bergmann <arnd@arndb•de>,
	Philippe Ombredanne <pombredanne@nexb•com>,
	Johan Hovold <johan@kernel•org>,
	Tomohiro Kusumi <kusumi.tomohiro@gmail•com>,
	Rob Herring <robh+dt@kernel•org>,
	Thomas Gleixner <tglx@linutronix•de>,
	Sagar Dharia <sdharia@codeaurora•org>,
	linux-arm-kernel@lists•infradead.org,
	Juergen Gross <jgross@suse•com>,
	Cyrille Pitchen <cyrille.pitchen@wedev4u•fr>,
	Ryan Chen <ryan_chen@aspeedtech•com>,
	Andrew Jeffery <andrew@aj•id.au>,
	Greg Kroah-Hartman <gregkh@linuxfoundation•org>,
	"Darrick J . Wong" <darrick.wong@oracle•com>,
	Stephen Boyd <sboyd@codeaurora•org>,
	Vinod Koul <vkoul@kernel•org>,
	Gustavo Pimentel <gustavo.pimentel@synopsys•com>,
	Andrew Morton <akpm@linux-foundation•org>,
	Robin Murphy <robin.murphy@arm•com>,
	"David S . Miller" <davem@davemloft•net>
Subject: Re: [PATCH v11 06/14] peci: Add Aspeed PECI adapter driver
Date: Wed, 11 Dec 2019 22:28:18 +0200	[thread overview]
Message-ID: <20191211202818.GD32742@smile.fi.intel.com> (raw)
In-Reply-To: <20191211194624.2872-7-jae.hyun.yoo@linux.intel.com>

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.

...

> +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.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel