diff mbox series

[02/15] gpio: adnp: use lock guards for the I2C lock

Message ID 20250303-gpiochip-set-conversion-v1-2-1d5cceeebf8b@linaro.org (mailing list archive)
State New, archived
Headers show
Series gpio: convert more drivers to using the new value setters | expand

Commit Message

Bartosz Golaszewski March 3, 2025, 1:18 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reduce the code complexity by using automatic lock guards with the I2C
mutex.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-adnp.c | 118 +++++++++++++++++++----------------------------
 1 file changed, 47 insertions(+), 71 deletions(-)

Comments

kernel test robot March 4, 2025, 9:07 a.m. UTC | #1 
Hi Bartosz,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 9778568dede2166c7bd124d473f9ec365f782935]

url:    https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/gpio-74x164-use-new-line-value-setter-callbacks/20250303-212738
base:   9778568dede2166c7bd124d473f9ec365f782935
patch link:    https://lore.kernel.org/r/20250303-gpiochip-set-conversion-v1-2-1d5cceeebf8b%40linaro.org
patch subject: [PATCH 02/15] gpio: adnp: use lock guards for the I2C lock
config: x86_64-buildonly-randconfig-001-20250304 (https://download.01.org/0day-ci/archive/20250304/202503041612.G8O0Bdrg-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250304/202503041612.G8O0Bdrg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503041612.G8O0Bdrg-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpio/gpio-adnp.c:241:8: warning: variable 'isr' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
     241 |                         if (err < 0)
         |                             ^~~~~~~
   drivers/gpio/gpio-adnp.c:265:14: note: uninitialized use occurs here
     265 |                 pending &= isr & ier;
         |                            ^~~
   drivers/gpio/gpio-adnp.c:241:4: note: remove the 'if' if its condition is always false
     241 |                         if (err < 0)
         |                         ^~~~~~~~~~~~
     242 |                                 continue;
         |                                 ~~~~~~~~
   drivers/gpio/gpio-adnp.c:235:25: note: initialize the variable 'isr' to silence this warning
     235 |                 u8 changed, level, isr, ier;
         |                                       ^
         |                                        = '\0'
>> drivers/gpio/gpio-adnp.c:245:8: warning: variable 'ier' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
     245 |                         if (err < 0)
         |                             ^~~~~~~
   drivers/gpio/gpio-adnp.c:265:20: note: uninitialized use occurs here
     265 |                 pending &= isr & ier;
         |                                  ^~~
   drivers/gpio/gpio-adnp.c:245:4: note: remove the 'if' if its condition is always false
     245 |                         if (err < 0)
         |                         ^~~~~~~~~~~~
     246 |                                 continue;
         |                                 ~~~~~~~~
   drivers/gpio/gpio-adnp.c:241:8: warning: variable 'ier' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
     241 |                         if (err < 0)
         |                             ^~~~~~~
   drivers/gpio/gpio-adnp.c:265:20: note: uninitialized use occurs here
     265 |                 pending &= isr & ier;
         |                                  ^~~
   drivers/gpio/gpio-adnp.c:241:4: note: remove the 'if' if its condition is always false
     241 |                         if (err < 0)
         |                         ^~~~~~~~~~~~
     242 |                                 continue;
         |                                 ~~~~~~~~
   drivers/gpio/gpio-adnp.c:235:30: note: initialize the variable 'ier' to silence this warning
     235 |                 u8 changed, level, isr, ier;
         |                                            ^
         |                                             = '\0'
   3 warnings generated.


vim +241 drivers/gpio/gpio-adnp.c

   225	
   226	static irqreturn_t adnp_irq(int irq, void *data)
   227	{
   228		struct adnp *adnp = data;
   229		unsigned int num_regs, i;
   230	
   231		num_regs = 1 << adnp->reg_shift;
   232	
   233		for (i = 0; i < num_regs; i++) {
   234			unsigned int base = i << adnp->reg_shift, bit;
   235			u8 changed, level, isr, ier;
   236			unsigned long pending;
   237			int err;
   238	
   239			scoped_guard(mutex, &adnp->i2c_lock) {
   240				err = adnp_read(adnp, GPIO_PLR(adnp) + i, &level);
 > 241				if (err < 0)
   242					continue;
   243	
   244				err = adnp_read(adnp, GPIO_ISR(adnp) + i, &isr);
 > 245				if (err < 0)
   246					continue;
   247	
   248				err = adnp_read(adnp, GPIO_IER(adnp) + i, &ier);
   249				if (err < 0)
   250					continue;
   251			}
   252	
   253			/* determine pins that changed levels */
   254			changed = level ^ adnp->irq_level[i];
   255	
   256			/* compute edge-triggered interrupts */
   257			pending = changed & ((adnp->irq_fall[i] & ~level) |
   258					     (adnp->irq_rise[i] & level));
   259	
   260			/* add in level-triggered interrupts */
   261			pending |= (adnp->irq_high[i] & level) |
   262				   (adnp->irq_low[i] & ~level);
   263	
   264			/* mask out non-pending and disabled interrupts */
   265			pending &= isr & ier;
   266	
   267			for_each_set_bit(bit, &pending, 8) {
   268				unsigned int child_irq;
   269				child_irq = irq_find_mapping(adnp->gpio.irq.domain,
   270							     base + bit);
   271				handle_nested_irq(child_irq);
   272			}
   273		}
   274	
   275		return IRQ_HANDLED;
   276	}
   277
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
index 6dafab0cf964..dc87768276ec 100644
--- a/drivers/gpio/gpio-adnp.c
+++ b/drivers/gpio/gpio-adnp.c
@@ -3,6 +3,7 @@ 
  * Copyright (C) 2011-2012 Avionic Design GmbH
  */
 
+#include <linux/cleanup.h>
 #include <linux/gpio/driver.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -101,9 +102,9 @@  static void adnp_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 {
 	struct adnp *adnp = gpiochip_get_data(chip);
 
-	mutex_lock(&adnp->i2c_lock);
+	guard(mutex)(&adnp->i2c_lock);
+
 	__adnp_gpio_set(adnp, offset, value);
-	mutex_unlock(&adnp->i2c_lock);
 }
 
 static int adnp_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
@@ -114,32 +115,26 @@  static int adnp_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 	u8 value;
 	int err;
 
-	mutex_lock(&adnp->i2c_lock);
+	guard(mutex)(&adnp->i2c_lock);
 
 	err = adnp_read(adnp, GPIO_DDR(adnp) + reg, &value);
 	if (err < 0)
-		goto out;
+		return err;
 
 	value &= ~BIT(pos);
 
 	err = adnp_write(adnp, GPIO_DDR(adnp) + reg, value);
 	if (err < 0)
-		goto out;
+		return err;
 
 	err = adnp_read(adnp, GPIO_DDR(adnp) + reg, &value);
 	if (err < 0)
-		goto out;
+		return err;
 
-	if (value & BIT(pos)) {
-		err = -EPERM;
-		goto out;
-	}
+	if (value & BIT(pos))
+		return -EPERM;
 
-	err = 0;
-
-out:
-	mutex_unlock(&adnp->i2c_lock);
-	return err;
+	return 0;
 }
 
 static int adnp_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
@@ -151,33 +146,28 @@  static int adnp_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
 	int err;
 	u8 val;
 
-	mutex_lock(&adnp->i2c_lock);
+	guard(mutex)(&adnp->i2c_lock);
 
 	err = adnp_read(adnp, GPIO_DDR(adnp) + reg, &val);
 	if (err < 0)
-		goto out;
+		return err;
 
 	val |= BIT(pos);
 
 	err = adnp_write(adnp, GPIO_DDR(adnp) + reg, val);
 	if (err < 0)
-		goto out;
+		return err;
 
 	err = adnp_read(adnp, GPIO_DDR(adnp) + reg, &val);
 	if (err < 0)
-		goto out;
+		return err;
 
-	if (!(val & BIT(pos))) {
-		err = -EPERM;
-		goto out;
-	}
+	if (!(val & BIT(pos)))
+		return -EPERM;
 
 	__adnp_gpio_set(adnp, offset, value);
-	err = 0;
 
-out:
-	mutex_unlock(&adnp->i2c_lock);
-	return err;
+	return 0;
 }
 
 static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -189,25 +179,24 @@  static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	for (i = 0; i < num_regs; i++) {
 		u8 ddr, plr, ier, isr;
 
-		mutex_lock(&adnp->i2c_lock);
+		scoped_guard(mutex, &adnp->i2c_lock) {
+			err = adnp_read(adnp, GPIO_DDR(adnp) + i, &ddr);
+			if (err < 0)
+				return;
 
-		err = adnp_read(adnp, GPIO_DDR(adnp) + i, &ddr);
-		if (err < 0)
-			goto unlock;
+			err = adnp_read(adnp, GPIO_PLR(adnp) + i, &plr);
+			if (err < 0)
+				return;
 
-		err = adnp_read(adnp, GPIO_PLR(adnp) + i, &plr);
-		if (err < 0)
-			goto unlock;
+			err = adnp_read(adnp, GPIO_IER(adnp) + i, &ier);
+			if (err < 0)
+				return;
 
-		err = adnp_read(adnp, GPIO_IER(adnp) + i, &ier);
-		if (err < 0)
-			goto unlock;
+			err = adnp_read(adnp, GPIO_ISR(adnp) + i, &isr);
+			if (err < 0)
+				return;
 
-		err = adnp_read(adnp, GPIO_ISR(adnp) + i, &isr);
-		if (err < 0)
-			goto unlock;
-
-		mutex_unlock(&adnp->i2c_lock);
+		}
 
 		for (j = 0; j < 8; j++) {
 			unsigned int bit = (i << adnp->reg_shift) + j;
@@ -232,11 +221,6 @@  static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 				   direction, level, interrupt, pending);
 		}
 	}
-
-	return;
-
-unlock:
-	mutex_unlock(&adnp->i2c_lock);
 }
 
 static irqreturn_t adnp_irq(int irq, void *data)
@@ -252,28 +236,20 @@  static irqreturn_t adnp_irq(int irq, void *data)
 		unsigned long pending;
 		int err;
 
-		mutex_lock(&adnp->i2c_lock);
+		scoped_guard(mutex, &adnp->i2c_lock) {
+			err = adnp_read(adnp, GPIO_PLR(adnp) + i, &level);
+			if (err < 0)
+				continue;
 
-		err = adnp_read(adnp, GPIO_PLR(adnp) + i, &level);
-		if (err < 0) {
-			mutex_unlock(&adnp->i2c_lock);
-			continue;
+			err = adnp_read(adnp, GPIO_ISR(adnp) + i, &isr);
+			if (err < 0)
+				continue;
+
+			err = adnp_read(adnp, GPIO_IER(adnp) + i, &ier);
+			if (err < 0)
+				continue;
 		}
 
-		err = adnp_read(adnp, GPIO_ISR(adnp) + i, &isr);
-		if (err < 0) {
-			mutex_unlock(&adnp->i2c_lock);
-			continue;
-		}
-
-		err = adnp_read(adnp, GPIO_IER(adnp) + i, &ier);
-		if (err < 0) {
-			mutex_unlock(&adnp->i2c_lock);
-			continue;
-		}
-
-		mutex_unlock(&adnp->i2c_lock);
-
 		/* determine pins that changed levels */
 		changed = level ^ adnp->irq_level[i];
 
@@ -365,12 +341,12 @@  static void adnp_irq_bus_unlock(struct irq_data *d)
 	struct adnp *adnp = gpiochip_get_data(gc);
 	unsigned int num_regs = 1 << adnp->reg_shift, i;
 
-	mutex_lock(&adnp->i2c_lock);
+	scoped_guard(mutex, &adnp->i2c_lock) {
+		for (i = 0; i < num_regs; i++)
+			adnp_write(adnp, GPIO_IER(adnp) + i,
+				   adnp->irq_enable[i]);
+	}
 
-	for (i = 0; i < num_regs; i++)
-		adnp_write(adnp, GPIO_IER(adnp) + i, adnp->irq_enable[i]);
-
-	mutex_unlock(&adnp->i2c_lock);
 	mutex_unlock(&adnp->irq_lock);
 }