[PATCH 0/3] pinctrl-mcp23s08: Fine-tuning for two function implementations

[PATCH 0/3] pinctrl-mcp23s08: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 30 Oct 2017 16:40:20 +0100

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Improve unlocking of a mutex in mcp23s08_irq()
  Use common error handling code in mcp23s08_dbg_show()
  Combine two function calls in mcp23s08_dbg_show()

 drivers/pinctrl/pinctrl-mcp23s08.c | 80 ++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 43 deletions(-)

-- 
2.14.3

[PATCH 1/3] pinctrl: mcp23s08: Improve unlocking of a mutex in mcp23s08_irq()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 30 Oct 2017 16:03:12 +0100

* Add a jump target so that a call of the function "mutex_unlock" is stored
  only twice in this function implementation.

* Replace five calls by goto statements.

* Adjust five condition checks.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 3e40d4245512..919eb7268331 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -457,31 +457,22 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 		defval_changed, gpio_set;
 
 	mutex_lock(&mcp->lock);
-	if (mcp_read(mcp, MCP_INTF, &intf) < 0) {
-		mutex_unlock(&mcp->lock);
-		return IRQ_HANDLED;
-	}
+	if (mcp_read(mcp, MCP_INTF, &intf))
+		goto unlock;
 
-	if (mcp_read(mcp, MCP_INTCAP, &intcap) < 0) {
-		mutex_unlock(&mcp->lock);
-		return IRQ_HANDLED;
-	}
+	if (mcp_read(mcp, MCP_INTCAP, &intcap))
+		goto unlock;
 
-	if (mcp_read(mcp, MCP_INTCON, &intcon) < 0) {
-		mutex_unlock(&mcp->lock);
-		return IRQ_HANDLED;
-	}
+	if (mcp_read(mcp, MCP_INTCON, &intcon))
+		goto unlock;
 
-	if (mcp_read(mcp, MCP_DEFVAL, &defval) < 0) {
-		mutex_unlock(&mcp->lock);
-		return IRQ_HANDLED;
-	}
+	if (mcp_read(mcp, MCP_DEFVAL, &defval))
+		goto unlock;
 
 	/* This clears the interrupt(configurable on S18) */
-	if (mcp_read(mcp, MCP_GPIO, &gpio) < 0) {
-		mutex_unlock(&mcp->lock);
-		return IRQ_HANDLED;
-	}
+	if (mcp_read(mcp, MCP_GPIO, &gpio))
+		goto unlock;
+
 	gpio_orig = mcp->cached_gpio;
 	mcp->cached_gpio = gpio;
 	mutex_unlock(&mcp->lock);
@@ -543,6 +534,10 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 	}
 
 	return IRQ_HANDLED;
+
+unlock:
+	mutex_unlock(&mcp->lock);
+	return IRQ_HANDLED;
 }
 
 static void mcp23s08_irq_mask(struct irq_data *data)
-- 
2.14.3

Re: [PATCH 1/3] pinctrl: mcp23s08: Improve unlocking of a mutex in mcp23s08_irq()

[Linus Walleij]

On Mon, Oct 30, 2017 at 4:46 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 30 Oct 2017 16:03:12 +0100
>
> * Add a jump target so that a call of the function "mutex_unlock" is stored
>   only twice in this function implementation.
>
> * Replace five calls by goto statements.
>
> * Adjust five condition checks.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Nice, patch applied.

Yours,
Linus Walleij

[PATCH 2/3] pinctrl: mcp23s08: Use common error handling code in mcp23s08_dbg_show()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 30 Oct 2017 16:26:09 +0100

* The script "checkpatch.pl" pointed information out like the following.

  WARNING: Prefer seq_puts to seq_printf

  Thus fix the affected source code places.

* Adjust jump targets so that a bit of exception handling can be better
  reused at the end of this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 919eb7268331..d1a7c627dbb9 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -723,25 +723,20 @@ static void mcp23s08_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	mutex_lock(&mcp->lock);
 
 	t = __check_mcp23s08_reg_cache(mcp);
-	if (t) {
-		seq_printf(s, " I/O Error\n");
-		goto done;
-	}
+	if (t)
+		goto report_failure;
+
 	t = mcp_read(mcp, MCP_IODIR, &iodir);
-	if (t) {
-		seq_printf(s, " I/O Error\n");
-		goto done;
-	}
+	if (t)
+		goto report_failure;
+
 	t = mcp_read(mcp, MCP_GPIO, &gpio);
-	if (t) {
-		seq_printf(s, " I/O Error\n");
-		goto done;
-	}
+	if (t)
+		goto report_failure;
+
 	t = mcp_read(mcp, MCP_GPPU, &gppu);
-	if (t) {
-		seq_printf(s, " I/O Error\n");
-		goto done;
-	}
+	if (t)
+		goto report_failure;
 
 	for (t = 0, mask = BIT(0); t < chip->ngpio; t++, mask <<= 1) {
 		const char *label;
@@ -758,8 +753,13 @@ static void mcp23s08_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 		/* NOTE:  ignoring the irq-related registers */
 		seq_puts(s, "\n");
 	}
-done:
+unlock:
 	mutex_unlock(&mcp->lock);
+	return;
+
+report_failure:
+	seq_puts(s, " I/O Error\n");
+	goto unlock;
 }
 
 #else
-- 
2.14.3

Re: [PATCH 2/3] pinctrl: mcp23s08: Use common error handling code in mcp23s08_dbg_show()

[Linus Walleij]

On Mon, Oct 30, 2017 at 4:47 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:

> +       if (t)
> +               goto report_failure;
>
>         for (t = 0, mask = BIT(0); t < chip->ngpio; t++, mask <<= 1) {
>                 const char *label;
> @@ -758,8 +753,13 @@ static void mcp23s08_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>                 /* NOTE:  ignoring the irq-related registers */
>                 seq_puts(s, "\n");
>         }
> -done:
> +unlock:
>         mutex_unlock(&mcp->lock);
> +       return;
> +
> +report_failure:
> +       seq_puts(s, " I/O Error\n");
> +       goto unlock;
>  }

This is spaghetti coding. Not applied.
https://en.wikipedia.org/wiki/Spaghetti_code

Yours,
Linus Walleij

[PATCH 3/3] pinctrl: mcp23s08: Combine two function calls in mcp23s08_dbg_show()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 30 Oct 2017 16:34:44 +0100

* Print a line break together with other data in a single function call.

* Adjust indentation.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index d1a7c627dbb9..79b585d46289 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -745,13 +745,12 @@ static void mcp23s08_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 		if (!label)
 			continue;
 
-		seq_printf(s, " gpio-%-3d P%c.%d (%-12s) %s %s %s",
-			chip->base + t, bank, t, label,
-			(iodir & mask) ? "in " : "out",
-			(gpio & mask) ? "hi" : "lo",
-			(gppu & mask) ? "up" : "  ");
+		seq_printf(s, " gpio-%-3d P%c.%d (%-12s) %s %s %s\n",
+			   chip->base + t, bank, t, label,
+			   (iodir & mask) ? "in " : "out",
+			   (gpio & mask) ? "hi" : "lo",
+			   (gppu & mask) ? "up" : "  ");
 		/* NOTE:  ignoring the irq-related registers */
-		seq_puts(s, "\n");
 	}
 unlock:
 	mutex_unlock(&mcp->lock);
-- 
2.14.3

Re: [PATCH 3/3] pinctrl: mcp23s08: Combine two function calls in mcp23s08_dbg_show()

[Linus Walleij]

On Mon, Oct 30, 2017 at 4:49 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 30 Oct 2017 16:34:44 +0100
>
> * Print a line break together with other data in a single function call.
>
> * Adjust indentation.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

This does not apply.
Possibly depends on 2/3.

Yours,
Linus Walleij

Re: [3/3] pinctrl: mcp23s08: Combine two function calls in mcp23s08_dbg_show()

[SF Markus Elfring]

>> * Print a line break together with other data in a single function call.
>>
>> * Adjust indentation.
…
> 
> This does not apply.
> Possibly depends on 2/3.

Partly, yes.


How do you think about the following possibilities?

* Omit two context lines at the end from this update step.
  or
* Refer to the jump label “done” (instead of renaming it to “unlock” before).


Would you try once more to integrate the shown small change?

Regards,
Markus

[PATCH v2] pinctrl: mcp23s08: Combine two function calls into one in mcp23s08_dbg_show()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 6 Jan 2018 20:40:04 +0100

* Print a line break together with other data in a single function call.

* Adjust indentation.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
This update suggestion was rebased on source files from the software
"Linux next-20180105".

 drivers/pinctrl/pinctrl-mcp23s08.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index e6ad36dea3e5..95d548f3ba53 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -748,13 +748,12 @@ static void mcp23s08_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 		if (!label)
 			continue;
 
-		seq_printf(s, " gpio-%-3d P%c.%d (%-12s) %s %s %s",
-			chip->base + t, bank, t, label,
-			(iodir & mask) ? "in " : "out",
-			(gpio & mask) ? "hi" : "lo",
-			(gppu & mask) ? "up" : "  ");
+		seq_printf(s, " gpio-%-3d P%c.%d (%-12s) %s %s %s\n",
+			   chip->base + t, bank, t, label,
+			   (iodir & mask) ? "in " : "out",
+			   (gpio & mask) ? "hi" : "lo",
+			   (gppu & mask) ? "up" : "  ");
 		/* NOTE:  ignoring the irq-related registers */
-		seq_puts(s, "\n");
 	}
 done:
 	mutex_unlock(&mcp->lock);
-- 
2.15.1

Re: [PATCH v2] pinctrl: mcp23s08: Combine two function calls into one in mcp23s08_dbg_show()

[Linus Walleij]

On Sat, Jan 6, 2018 at 8:52 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 6 Jan 2018 20:40:04 +0100
>
> * Print a line break together with other data in a single function call.
>
> * Adjust indentation.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Patch applied.

Yours,
Linus Walleij