diff mbox series

[v3,7/7] iio: accel: kx022a: align with subsystem way

Message ID 9b63813ecf10b1cd0126cb950bc09514c4287b9a.1732783834.git.mazziesaccount@gmail.com (mailing list archive)
State New
Headers show
Series Support ROHM KX134ACR-LBZ | expand

Commit Message

Matti Vaittinen Nov. 28, 2024, 9:03 a.m. UTC
Many of the Kionix/ROHM accelerometers have a "PC1 - bit" which enables
the accelerometer. While a sensor configuration like ODR, g-range, FIFO
status etc. are changed, the PC1 bit must be cleared (sensor must be
disabled). (See the description for different CNTL registers [1])

In order to ensure this the kx022a driver uses a mutex, which is locked
when the PC1 bit is cleared, and held for the duration of the
configuration, and released after PC1 bit is set again (enabling the
sensor).

The locking and PC1 bit toggling was implemented using functions:
kx022a_turn_off_lock() and kx022a_turn_on_unlock().

Based on a discussions [2], the IIO subsystem prefers open-coding the
locking with scoped_guard() over these functions.

Drop the kx022a_turn_off_lock() and kx022a_turn_on_unlock() and use
scoped_guard() instead.

[1]: https://fscdn.rohm.com/kionix/en/datasheet/kx022acr-z-e.pdf
[2]: https://lore.kernel.org/all/20241126175550.4a8bedf3@jic23-huawei/

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Revision history:
v2 => v3:
 - New patch

NOTE: This patch uses the if_not_cond_guard() which is currently missing
the iio_testing.
https://lore.kernel.org/all/20241001-cleanup-if_not_cond_guard-v1-1-7753810b0f7a@baylibre.com/T/#m69982b23da9f71e72d84855b34e9b142cb3a1920
---
 drivers/iio/accel/kionix-kx022a.c | 121 ++++++++++++------------------
 1 file changed, 48 insertions(+), 73 deletions(-)

Comments

kernel test robot Nov. 28, 2024, 5:20 p.m. UTC | #1
Hi Matti,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a61ff7eac77e86de828fe28c4e42b8ae9ec2b195]

url:    https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/iio-accel-kx022a-Use-cleanup-h-helpers/20241128-170626
base:   a61ff7eac77e86de828fe28c4e42b8ae9ec2b195
patch link:    https://lore.kernel.org/r/9b63813ecf10b1cd0126cb950bc09514c4287b9a.1732783834.git.mazziesaccount%40gmail.com
patch subject: [PATCH v3 7/7] iio: accel: kx022a: align with subsystem way
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20241129/202411290107.KXHPQXRf-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241129/202411290107.KXHPQXRf-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/202411290107.KXHPQXRf-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/iio/accel/kionix-kx022a.c: In function 'kx022a_write_raw':
   drivers/iio/accel/kionix-kx022a.c:507:9: error: implicit declaration of function 'if_not_cond_guard' [-Wimplicit-function-declaration]
     507 |         if_not_cond_guard(iio_claim_direct_try, idev)
         |         ^~~~~~~~~~~~~~~~~
   drivers/iio/accel/kionix-kx022a.c:507:27: error: 'iio_claim_direct_try' undeclared (first use in this function); did you mean 'class_iio_claim_direct_try_t'?
     507 |         if_not_cond_guard(iio_claim_direct_try, idev)
         |                           ^~~~~~~~~~~~~~~~~~~~
         |                           class_iio_claim_direct_try_t
   drivers/iio/accel/kionix-kx022a.c:507:27: note: each undeclared identifier is reported only once for each function it appears in
   drivers/iio/accel/kionix-kx022a.c:507:54: error: expected ';' before 'return'
     507 |         if_not_cond_guard(iio_claim_direct_try, idev)
         |                                                      ^
         |                                                      ;
     508 |                 return -EBUSY;
         |                 ~~~~~~                                
   In file included from drivers/iio/accel/kionix-kx022a.c:8:
>> include/linux/cleanup.h:308:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
     308 |         for (CLASS(_name, scope)(args),                                 \
         |         ^~~
   drivers/iio/accel/kionix-kx022a.c:521:17: note: in expansion of macro 'scoped_guard'
     521 |                 scoped_guard(mutex, &data->mutex) {
         |                 ^~~~~~~~~~~~
   drivers/iio/accel/kionix-kx022a.c:532:9: note: here
     532 |         case IIO_CHAN_INFO_SCALE:
         |         ^~~~


vim +308 include/linux/cleanup.h

e4ab322fbaaaf8 Peter Zijlstra 2023-09-17  306  
54da6a0924311c Peter Zijlstra 2023-05-26  307  #define scoped_guard(_name, args...)					\
54da6a0924311c Peter Zijlstra 2023-05-26 @308  	for (CLASS(_name, scope)(args),					\
e4ab322fbaaaf8 Peter Zijlstra 2023-09-17  309  	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
e4ab322fbaaaf8 Peter Zijlstra 2023-09-17  310
kernel test robot Nov. 28, 2024, 5:31 p.m. UTC | #2
Hi Matti,

kernel test robot noticed the following build errors:

[auto build test ERROR on a61ff7eac77e86de828fe28c4e42b8ae9ec2b195]

url:    https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/iio-accel-kx022a-Use-cleanup-h-helpers/20241128-170626
base:   a61ff7eac77e86de828fe28c4e42b8ae9ec2b195
patch link:    https://lore.kernel.org/r/9b63813ecf10b1cd0126cb950bc09514c4287b9a.1732783834.git.mazziesaccount%40gmail.com
patch subject: [PATCH v3 7/7] iio: accel: kx022a: align with subsystem way
config: i386-buildonly-randconfig-003-20241128 (https://download.01.org/0day-ci/archive/20241129/202411290140.7k2Z9JSi-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241129/202411290140.7k2Z9JSi-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/202411290140.7k2Z9JSi-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/iio/accel/kionix-kx022a.c:17:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/iio/accel/kionix-kx022a.c:507:2: error: call to undeclared function 'if_not_cond_guard'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     507 |         if_not_cond_guard(iio_claim_direct_try, idev)
         |         ^
>> drivers/iio/accel/kionix-kx022a.c:507:47: error: expected ';' after expression
     507 |         if_not_cond_guard(iio_claim_direct_try, idev)
         |                                                      ^
         |                                                      ;
>> drivers/iio/accel/kionix-kx022a.c:507:20: error: use of undeclared identifier 'iio_claim_direct_try'
     507 |         if_not_cond_guard(iio_claim_direct_try, idev)
         |                           ^
   1 warning and 3 errors generated.


vim +/if_not_cond_guard +507 drivers/iio/accel/kionix-kx022a.c

   490	
   491	static int kx022a_write_raw(struct iio_dev *idev,
   492				    struct iio_chan_spec const *chan,
   493				    int val, int val2, long mask)
   494	{
   495		struct kx022a_data *data = iio_priv(idev);
   496		int ret, n;
   497	
   498		/*
   499		 * We should not allow changing scale or frequency when FIFO is running
   500		 * as it will mess the timestamp/scale for samples existing in the
   501		 * buffer. If this turns out to be an issue we can later change logic
   502		 * to internally flush the fifo before reconfiguring so the samples in
   503		 * fifo keep matching the freq/scale settings. (Such setup could cause
   504		 * issues if users trust the watermark to be reached within known
   505		 * time-limit).
   506		 */
 > 507		if_not_cond_guard(iio_claim_direct_try, idev)
   508			return -EBUSY;
   509	
   510		switch (mask) {
   511		case IIO_CHAN_INFO_SAMP_FREQ:
   512			n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
   513	
   514			while (n--)
   515				if (val == kx022a_accel_samp_freq_table[n][0] &&
   516				    val2 == kx022a_accel_samp_freq_table[n][1])
   517					break;
   518			if (n < 0)
   519				return -EINVAL;
   520	
   521			scoped_guard(mutex, &data->mutex) {
   522				ret = kx022a_turn_on_off(data, false);
   523				if (ret)
   524					return ret;
   525	
   526				ret = regmap_update_bits(data->regmap,
   527							 data->chip_info->odcntl,
   528							 KX022A_MASK_ODR, n);
   529				data->odr_ns = kx022a_odrs[n];
   530				return kx022a_turn_on_off(data, true);
   531			}
   532		case IIO_CHAN_INFO_SCALE:
   533			n = data->chip_info->scale_table_size / 2;
   534	
   535			while (n-- > 0)
   536				if (val == data->chip_info->scale_table[n][0] &&
   537				    val2 == data->chip_info->scale_table[n][1])
   538					break;
   539			if (n < 0)
   540				return -EINVAL;
   541	
   542			scoped_guard(mutex, &data->mutex) {
   543				ret = kx022a_turn_on_off(data, false);
   544				if (ret)
   545					return ret;
   546	
   547				ret = regmap_update_bits(data->regmap,
   548							 data->chip_info->cntl,
   549							 KX022A_MASK_GSEL,
   550							 n << KX022A_GSEL_SHIFT);
   551				kx022a_turn_on_off(data, true);
   552	
   553				return ret;
   554			}
   555		default:
   556			break;
   557		}
   558	
   559		return -EINVAL;
   560	}
   561
kernel test robot Nov. 28, 2024, 5:52 p.m. UTC | #3
Hi Matti,

kernel test robot noticed the following build errors:

[auto build test ERROR on a61ff7eac77e86de828fe28c4e42b8ae9ec2b195]

url:    https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/iio-accel-kx022a-Use-cleanup-h-helpers/20241128-170626
base:   a61ff7eac77e86de828fe28c4e42b8ae9ec2b195
patch link:    https://lore.kernel.org/r/9b63813ecf10b1cd0126cb950bc09514c4287b9a.1732783834.git.mazziesaccount%40gmail.com
patch subject: [PATCH v3 7/7] iio: accel: kx022a: align with subsystem way
config: x86_64-buildonly-randconfig-004-20241128 (https://download.01.org/0day-ci/archive/20241129/202411290148.Jdoj8IqZ-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241129/202411290148.Jdoj8IqZ-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/202411290148.Jdoj8IqZ-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/iio/accel/kionix-kx022a.c: In function 'kx022a_write_raw':
>> drivers/iio/accel/kionix-kx022a.c:507:9: error: implicit declaration of function 'if_not_cond_guard' [-Werror=implicit-function-declaration]
     507 |         if_not_cond_guard(iio_claim_direct_try, idev)
         |         ^~~~~~~~~~~~~~~~~
>> drivers/iio/accel/kionix-kx022a.c:507:27: error: 'iio_claim_direct_try' undeclared (first use in this function); did you mean 'class_iio_claim_direct_try_t'?
     507 |         if_not_cond_guard(iio_claim_direct_try, idev)
         |                           ^~~~~~~~~~~~~~~~~~~~
         |                           class_iio_claim_direct_try_t
   drivers/iio/accel/kionix-kx022a.c:507:27: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/iio/accel/kionix-kx022a.c:507:54: error: expected ';' before 'return'
     507 |         if_not_cond_guard(iio_claim_direct_try, idev)
         |                                                      ^
         |                                                      ;
     508 |                 return -EBUSY;
         |                 ~~~~~~                                
   In file included from drivers/iio/accel/kionix-kx022a.c:8:
   include/linux/cleanup.h:308:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
     308 |         for (CLASS(_name, scope)(args),                                 \
         |         ^~~
   drivers/iio/accel/kionix-kx022a.c:521:17: note: in expansion of macro 'scoped_guard'
     521 |                 scoped_guard(mutex, &data->mutex) {
         |                 ^~~~~~~~~~~~
   drivers/iio/accel/kionix-kx022a.c:532:9: note: here
     532 |         case IIO_CHAN_INFO_SCALE:
         |         ^~~~
   cc1: some warnings being treated as errors


vim +/if_not_cond_guard +507 drivers/iio/accel/kionix-kx022a.c

   490	
   491	static int kx022a_write_raw(struct iio_dev *idev,
   492				    struct iio_chan_spec const *chan,
   493				    int val, int val2, long mask)
   494	{
   495		struct kx022a_data *data = iio_priv(idev);
   496		int ret, n;
   497	
   498		/*
   499		 * We should not allow changing scale or frequency when FIFO is running
   500		 * as it will mess the timestamp/scale for samples existing in the
   501		 * buffer. If this turns out to be an issue we can later change logic
   502		 * to internally flush the fifo before reconfiguring so the samples in
   503		 * fifo keep matching the freq/scale settings. (Such setup could cause
   504		 * issues if users trust the watermark to be reached within known
   505		 * time-limit).
   506		 */
 > 507		if_not_cond_guard(iio_claim_direct_try, idev)
   508			return -EBUSY;
   509	
   510		switch (mask) {
   511		case IIO_CHAN_INFO_SAMP_FREQ:
   512			n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
   513	
   514			while (n--)
   515				if (val == kx022a_accel_samp_freq_table[n][0] &&
   516				    val2 == kx022a_accel_samp_freq_table[n][1])
   517					break;
   518			if (n < 0)
   519				return -EINVAL;
   520	
   521			scoped_guard(mutex, &data->mutex) {
   522				ret = kx022a_turn_on_off(data, false);
   523				if (ret)
   524					return ret;
   525	
   526				ret = regmap_update_bits(data->regmap,
   527							 data->chip_info->odcntl,
   528							 KX022A_MASK_ODR, n);
   529				data->odr_ns = kx022a_odrs[n];
   530				return kx022a_turn_on_off(data, true);
   531			}
   532		case IIO_CHAN_INFO_SCALE:
   533			n = data->chip_info->scale_table_size / 2;
   534	
   535			while (n-- > 0)
   536				if (val == data->chip_info->scale_table[n][0] &&
   537				    val2 == data->chip_info->scale_table[n][1])
   538					break;
   539			if (n < 0)
   540				return -EINVAL;
   541	
   542			scoped_guard(mutex, &data->mutex) {
   543				ret = kx022a_turn_on_off(data, false);
   544				if (ret)
   545					return ret;
   546	
   547				ret = regmap_update_bits(data->regmap,
   548							 data->chip_info->cntl,
   549							 KX022A_MASK_GSEL,
   550							 n << KX022A_GSEL_SHIFT);
   551				kx022a_turn_on_off(data, true);
   552	
   553				return ret;
   554			}
   555		default:
   556			break;
   557		}
   558	
   559		return -EINVAL;
   560	}
   561
diff mbox series

Patch

diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index e3986dd65337..a34cf8da2860 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -458,7 +458,7 @@  static void kx022a_reg2scale(struct kx022a_data *data, unsigned int val,
 	*val2 = data->chip_info->scale_table[val][1];
 }
 
-static int __kx022a_turn_on_off(struct kx022a_data *data, bool on)
+static int kx022a_turn_on_off(struct kx022a_data *data, bool on)
 {
 	int ret;
 
@@ -474,28 +474,6 @@  static int __kx022a_turn_on_off(struct kx022a_data *data, bool on)
 	return ret;
 }
 
-static int kx022a_turn_off_lock(struct kx022a_data *data)
-{
-	int ret;
-
-	mutex_lock(&data->mutex);
-	ret = __kx022a_turn_on_off(data, false);
-	if (ret)
-		mutex_unlock(&data->mutex);
-
-	return ret;
-}
-
-static int kx022a_turn_on_unlock(struct kx022a_data *data)
-{
-	int ret;
-
-	ret = __kx022a_turn_on_off(data, true);
-	mutex_unlock(&data->mutex);
-
-	return ret;
-}
-
 static int kx022a_write_raw_get_fmt(struct iio_dev *idev,
 				    struct iio_chan_spec const *chan,
 				    long mask)
@@ -526,9 +504,8 @@  static int kx022a_write_raw(struct iio_dev *idev,
 	 * issues if users trust the watermark to be reached within known
 	 * time-limit).
 	 */
-	ret = iio_device_claim_direct_mode(idev);
-	if (ret)
-		return ret;
+	if_not_cond_guard(iio_claim_direct_try, idev)
+		return -EBUSY;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
@@ -538,20 +515,20 @@  static int kx022a_write_raw(struct iio_dev *idev,
 			if (val == kx022a_accel_samp_freq_table[n][0] &&
 			    val2 == kx022a_accel_samp_freq_table[n][1])
 				break;
-		if (n < 0) {
-			ret = -EINVAL;
-			goto unlock_out;
-		}
-		ret = kx022a_turn_off_lock(data);
-		if (ret)
-			break;
+		if (n < 0)
+			return -EINVAL;
 
-		ret = regmap_update_bits(data->regmap,
-					 data->chip_info->odcntl,
-					 KX022A_MASK_ODR, n);
-		data->odr_ns = kx022a_odrs[n];
-		kx022a_turn_on_unlock(data);
-		break;
+		scoped_guard(mutex, &data->mutex) {
+			ret = kx022a_turn_on_off(data, false);
+			if (ret)
+				return ret;
+
+			ret = regmap_update_bits(data->regmap,
+						 data->chip_info->odcntl,
+						 KX022A_MASK_ODR, n);
+			data->odr_ns = kx022a_odrs[n];
+			return kx022a_turn_on_off(data, true);
+		}
 	case IIO_CHAN_INFO_SCALE:
 		n = data->chip_info->scale_table_size / 2;
 
@@ -559,29 +536,27 @@  static int kx022a_write_raw(struct iio_dev *idev,
 			if (val == data->chip_info->scale_table[n][0] &&
 			    val2 == data->chip_info->scale_table[n][1])
 				break;
-		if (n < 0) {
-			ret = -EINVAL;
-			goto unlock_out;
-		}
+		if (n < 0)
+			return -EINVAL;
 
-		ret = kx022a_turn_off_lock(data);
-		if (ret)
-			break;
+		scoped_guard(mutex, &data->mutex) {
+			ret = kx022a_turn_on_off(data, false);
+			if (ret)
+				return ret;
 
-		ret = regmap_update_bits(data->regmap, data->chip_info->cntl,
-					 KX022A_MASK_GSEL,
-					 n << KX022A_GSEL_SHIFT);
-		kx022a_turn_on_unlock(data);
-		break;
+			ret = regmap_update_bits(data->regmap,
+						 data->chip_info->cntl,
+						 KX022A_MASK_GSEL,
+						 n << KX022A_GSEL_SHIFT);
+			kx022a_turn_on_off(data, true);
+
+			return ret;
+		}
 	default:
-		ret = -EINVAL;
 		break;
 	}
 
-unlock_out:
-	iio_device_release_direct_mode(idev);
-
-	return ret;
+	return -EINVAL;
 }
 
 static int kx022a_fifo_set_wmi(struct kx022a_data *data)
@@ -923,7 +898,7 @@  static int kx022a_fifo_disable(struct kx022a_data *data)
 	int ret = 0;
 
 	guard(mutex)(&data->mutex);
-	ret = __kx022a_turn_on_off(data, false);
+	ret = kx022a_turn_on_off(data, false);
 	if (ret)
 		return ret;
 
@@ -942,7 +917,7 @@  static int kx022a_fifo_disable(struct kx022a_data *data)
 
 	kfree(data->fifo_buffer);
 
-	return __kx022a_turn_on_off(data, true);
+	return kx022a_turn_on_off(data, true);
 }
 
 static int kx022a_buffer_predisable(struct iio_dev *idev)
@@ -966,7 +941,7 @@  static int kx022a_fifo_enable(struct kx022a_data *data)
 		return -ENOMEM;
 
 	guard(mutex)(&data->mutex);
-	ret = __kx022a_turn_on_off(data, false);
+	ret = kx022a_turn_on_off(data, false);
 	if (ret)
 		return ret;
 
@@ -987,7 +962,7 @@  static int kx022a_fifo_enable(struct kx022a_data *data)
 	if (ret)
 		return ret;
 
-	return __kx022a_turn_on_off(data, true);
+	return kx022a_turn_on_off(data, true);
 }
 
 static int kx022a_buffer_postenable(struct iio_dev *idev)
@@ -1089,7 +1064,7 @@  static int kx022a_trigger_set_state(struct iio_trigger *trig,
 		return -EBUSY;
 	}
 
-	ret = __kx022a_turn_on_off(data, false);
+	ret = kx022a_turn_on_off(data, false);
 	if (ret)
 		return ret;
 
@@ -1098,7 +1073,7 @@  static int kx022a_trigger_set_state(struct iio_trigger *trig,
 	if (ret)
 		return ret;
 
-	return __kx022a_turn_on_off(data, true);
+	return kx022a_turn_on_off(data, true);
 }
 
 static const struct iio_trigger_ops kx022a_trigger_ops = {
@@ -1379,19 +1354,19 @@  int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chi
 		return ret;
 
 	/* The sensor must be turned off for configuration */
-	ret = kx022a_turn_off_lock(data);
-	if (ret)
-		return ret;
+	scoped_guard(mutex, &data->mutex) {
+		ret = kx022a_turn_on_off(data, false);
+		if (ret)
+			return ret;
 
-	ret = kx022a_chip_init(data);
-	if (ret) {
-		mutex_unlock(&data->mutex);
-		return ret;
-	}
+		ret = kx022a_chip_init(data);
+		if (ret)
+			return ret;
 
-	ret = kx022a_turn_on_unlock(data);
-	if (ret)
-		return ret;
+		ret = kx022a_turn_on_off(data, true);
+		if (ret)
+			return ret;
+	}
 
 	ret = devm_iio_triggered_buffer_setup_ext(dev, idev,
 						  &iio_pollfunc_store_time,