[Linuxarm] Re: [PATCH] spi: Use pm_runtime_resume_and_get() to replace open coding

On Thu, 20 May 2021 19:12:36 +0800 Tian Tao tiantao6@hisilicon.com wrote:

Found using coccicheck script under review at: https://lore.kernel.org/lkml/20210427141946.2478411-1-Julia.Lawall@inria.fr/

Say why. Boilerplate reduction and easier to understand flow as the new function has no side effects if it returns an error.

Also highlight that it avoids the problem of positive return values and hence lets you cleanup some special handling in the driver.

Signed-off-by: Tian Tao tiantao6@hisilicon.com

drivers/spi/spi.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 57120de..2664b4a 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1486,9 +1486,8 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) mutex_lock(&ctlr->io_mutex);

if (!was_busy && ctlr->auto_runtime_pm) {

• ret = pm_runtime_get_sync(ctlr->dev.parent);
  

• ret = pm_runtime_resume_and_get(ctlr->dev.parent);
  

  if (ret < 0) {

• 	pm_runtime_put_noidle(ctlr->dev.parent);
  

An additional advantage of pm_runtime_resume_and_get() is that it returns <= 0 so you can change the error check to if (ret) which seems the more common pattern in this file.

	dev_err(&ctlr->dev, "Failed to power device: %d\n",
		ret);
	mutex_unlock(&ctlr->io_mutex);

@@ -3426,10 +3425,9 @@ int spi_setup(struct spi_device *spi) }

if (spi->controller->auto_runtime_pm && spi->controller->set_cs) {

• status = pm_runtime_get_sync(spi->controller->dev.parent);
  

• status = pm_runtime_resume_and_get(spi->controller->dev.parent);
  

  if (status < 0) {

Read the comment below this. Rather helpfully you can get rid of the special handling of status = 0 line and the comment ;)

With that gone, this patch will be of clearer benefit!

	mutex_unlock(&spi->controller->io_mutex);

• 	pm_runtime_put_noidle(spi->controller->dev.parent);
  dev_err(&spi->controller->dev, "Failed to power device: %d\n",
  	status);
  return status;
  

@@ -3491,10 +3489,9 @@ int spi_set_cs_timing(struct spi_device *spi, struct spi_delay *setup, mutex_lock(&spi->controller->io_mutex);

if (spi->controller->auto_runtime_pm) {

• 	status = pm_runtime_get_sync(parent);
  

• 	status = pm_runtime_resume_and_get(parent);
  if (status < 0) {
  	mutex_unlock(&spi->controller->io_mutex);
  

• 		pm_runtime_put_noidle(parent);
  	dev_err(&spi->controller->dev, "Failed to power device: %d\n",
  		status);
  	return status;