[PATCH v2 0/4] ROHM IIO fixes

[PATCH v2 0/4] ROHM IIO fixes

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]

Fix a few issues from IIO drivers for ROHM components.

I hired couple of gnomes to work for me. :) (Ran AI reviews).
Unsurprizingly some bugs were spotted. Time to try fix mess (mostly) I
have authored.

Bugs were found by AI review but fixes are made by hand. Hence no
AI-tooling tags are added. Please, let me know if one is needed.

Note that the patch 1/4 is already applied to Jonathan's testing repo.
Including it because I couldn't find it from IIO's public repo to rebase
the series on it.

Revision history:
v1 => v2:
 - Drop already applied patches (except the 1/4 which is in testing)
 - all: Fixes tag before SOB
 - patch 2/4: Clarify units for minimum integration time
 - patch 3/4: Fix the unwinding of fifo enable
 - patch 4/4: Fix the broken commit message

---

NOTE: Only _very_ shallow testing is done. Some of the fixes are not
tested in the hardware at all. All reviewing and testing is appreciated
as usual!

Matti Vaittinen (4):
  iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
  iio: light: rohm-bu27034: Fix infinite delay on error
  iio: accel: kionix-kx022a: Prevent memory leak and fix state
  iio: accel: kionix-kx022a: Fix IPOL macro name

 drivers/iio/accel/kionix-kx022a.c  | 28 +++++++++++++++++++++++-----
 drivers/iio/accel/kionix-kx022a.h  |  2 +-
 drivers/iio/light/rohm-bu27034.c   | 10 ++++++++++
 drivers/iio/pressure/rohm-bm1390.c |  4 +++-
 4 files changed, 37 insertions(+), 7 deletions(-)


base-commit: 6b9f23b5460818aaf199dda90210d5fc08d66c8f
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 1/4] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]

From: Matti Vaittinen <mazziesaccount@gmail.com>

The BM1390 tries to initialize the AVE_NUM to 110b at the start-up. The
field location is not taken into account, and value is written unsifted.
This causes the AVE_NUM to be initialized to zero.

Use FIELD_PREP() to shift the intended AVE_NUM value to correct field.

Fixes: 81ca5979b6ed ("iio: pressure: Support ROHM BU1390")
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
I believe this was already applied to Jonathan's 'testing' branch. I
didn't find that from public IIO repository though, so spinning it here
just for the sake of the completeness.
---
 drivers/iio/pressure/rohm-bm1390.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
index d00d7ed54cb1..29454570f257 100644
--- a/drivers/iio/pressure/rohm-bm1390.c
+++ b/drivers/iio/pressure/rohm-bm1390.c
@@ -479,6 +479,7 @@ static const struct iio_info bm1390_info = {
 
 static int bm1390_chip_init(struct bm1390_data *data)
 {
+	u8 regval;
 	int ret;
 
 	ret = regmap_write_bits(data->regmap, BM1390_REG_POWER,
@@ -512,8 +513,9 @@ static int bm1390_chip_init(struct bm1390_data *data)
 	 * Default to use IIR filter in "middle" mode. Also the AVE_NUM must
 	 * be fixed when IIR is in use.
 	 */
+	regval = FIELD_PREP(BM1390_MASK_AVE_NUM, BM1390_IIR_AVE_NUM);
 	ret = regmap_update_bits(data->regmap, BM1390_REG_MODE_CTRL,
-				 BM1390_MASK_AVE_NUM, BM1390_IIR_AVE_NUM);
+				 BM1390_MASK_AVE_NUM, regval);
 	if (ret)
 		return ret;
 
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

From: Matti Vaittinen <mazziesaccount@gmail.com>

When reading an integration-time fails, the code will use error code to
compute the sleep time.

Fix this by using the smallest integration time as a default if
reading fails.

Fixes: e52afbd61039 ("iio: light: ROHM BU27034 Ambient Light Sensor")
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Revision history:
v1 => v2:
 - Moved Fixes before SOB
 - Clarified units for the smallest integration time as suggested by Andy
---
 drivers/iio/light/rohm-bu27034.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iio/light/rohm-bu27034.c b/drivers/iio/light/rohm-bu27034.c
index f9a421618406..cb2afddc75f6 100644
--- a/drivers/iio/light/rohm-bu27034.c
+++ b/drivers/iio/light/rohm-bu27034.c
@@ -137,6 +137,7 @@ static const struct iio_gain_sel_pair bu27034_gains[] = {
 #define BU27034_MEAS_MODE_200MS		2
 #define BU27034_MEAS_MODE_400MS		4
 
+#define BU27034_INT_TIME_US_MIN (55 * USEC_PER_MSEC)
 static const struct iio_itime_sel_mul bu27034_itimes[] = {
 	GAIN_SCALE_ITIME_US(400000, BU27034_MEAS_MODE_400MS, 8),
 	GAIN_SCALE_ITIME_US(200000, BU27034_MEAS_MODE_200MS, 4),
@@ -1162,6 +1163,15 @@ static int bu27034_buffer_thread(void *arg)
 	data = iio_priv(idev);
 
 	wait_ms = bu27034_get_int_time(data);
+
+	/*
+	 * If reading the integration time fails, default to the minimum so we
+	 * don't lose samples. This may waste CPU cycles, but as a hardening
+	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
+	 */
+	if (wait_ms < 0)
+		wait_ms = BU27034_INT_TIME_US_MIN;
+
 	wait_ms /= 1000;
 
 	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 3/4] iio: accel: kionix-kx022a: Prevent memory leak and fix state

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]

From: Matti Vaittinen <mazziesaccount@gmail.com>

The driver allocates memory for samples at buffer enable path. If regmap
operation fails in the kx022a_fifo_enable() at the buffer enable path, the
allocated memory is never freed. Furthermore, the state information and
previous hardware configuration(s) aren't undone, potentially leaving
WMI interrupts and buffers enabled, or driver state flags wrong.

Free the memory and revert the hardware configuration and state flags on
error path.

Fixes: e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

---
Revision history:
v1 => v2:
 - Fix unwinding the fifo enabling
 - Move Fixes before the SOB.
---
 drivers/iio/accel/kionix-kx022a.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
index 8a13f78aeab0..fa94bc0fd8cf 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -980,26 +980,44 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
 	guard(mutex)(&data->mutex);
 	ret = __kx022a_turn_on_off(data, false);
 	if (ret)
-		return ret;
+		goto err_free_out;
 
 	/* Update watermark to HW */
 	ret = kx022a_fifo_set_wmi(data);
 	if (ret)
-		return ret;
+		goto err_wmi_out;
 
 	/* Enable buffer */
 	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
 			      KX022A_MASK_BUF_EN);
 	if (ret)
-		return ret;
+		goto err_wmi_out;
 
 	data->state |= KX022A_STATE_FIFO;
 	ret = regmap_set_bits(data->regmap, data->ien_reg,
 			      KX022A_MASK_WMI);
 	if (ret)
-		return ret;
+		goto err_buf_en_out;
 
-	return __kx022a_turn_on_off(data, true);
+	ret = __kx022a_turn_on_off(data, true);
+	if (ret)
+		goto err_on_out;
+
+	return ret;
+
+err_on_out:
+	regmap_clear_bits(data->regmap, data->ien_reg,
+			  KX022A_MASK_WMI);
+err_buf_en_out:
+	regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
+			  KX022A_MASK_BUF_EN);
+	data->state &= ~KX022A_STATE_FIFO;
+err_wmi_out:
+	__kx022a_turn_on_off(data, true);
+err_free_out:
+	kfree(data->fifo_buffer);
+
+	return ret;
 }
 
 static int kx022a_buffer_postenable(struct iio_dev *idev)
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 4/4] iio: accel: kionix-kx022a: Fix IPOL macro name

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]

From: Matti Vaittinen <mazziesaccount@gmail.com>

The "interrupt polarity high"-macro for KX022A variant is defined as:
"#define KX022A_MASK_IPOL KX022A_MASK_IPOL1"

However, the KX022A_MASK_IPOL1 is not defined anywhere, so actually
using the KX022A_IPOL_HIGH would produce a compile error.

Fix the define by using correct mask.

Fixes: 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

---
Revision history:
v1 => v2:
 - Fixed the commit message where line: "#define KX022A_MASK_IPOL
   KX022A_MASK_IPOL1" was lost, as git treated it as a comment :)
 - Moved Fixes before SOB
---
 drivers/iio/accel/kionix-kx022a.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index 0ed54f584223..a2d122c1e234 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -65,7 +65,7 @@
 #define KX022A_MASK_IEN		BIT(5)
 #define KX022A_MASK_IPOL	BIT(4)
 #define KX022A_IPOL_LOW		0
-#define KX022A_IPOL_HIGH	KX022A_MASK_IPOL1
+#define KX022A_IPOL_HIGH	KX022A_MASK_IPOL
 #define KX022A_MASK_ITYP	BIT(3)
 #define KX022A_ITYP_PULSE	KX022A_MASK_ITYP
 #define KX022A_ITYP_LEVEL	0
-- 
2.55.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

[Andy Shevchenko]

On Fri, Aug 28, 2026 at 10:40:36AM +0300, Matti Vaittinen wrote:

> When reading an integration-time fails, the code will use error code to
> compute the sleep time.
> 
> Fix this by using the smallest integration time as a default if
> reading fails.

...

>  	wait_ms = bu27034_get_int_time(data);
> +
> +	/*
> +	 * If reading the integration time fails, default to the minimum so we
> +	 * don't lose samples. This may waste CPU cycles, but as a hardening
> +	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
> +	 */
> +	if (wait_ms < 0)
> +		wait_ms = BU27034_INT_TIME_US_MIN;
> +
>  	wait_ms /= 1000;

With the above being open coded the _ms feels not right.
I would expect the TIME_MIN to be in MS from the start
(and for the consistency's sake with the below) and having
all this to be written like

	ret = bu27034_get_int_time(data);
	if (ret < 0)
		wait_ms = _MS_MIN;
	else
		wait_ms = ret / USEC_PER_MSEC;

>  	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] iio: accel: kionix-kx022a: Prevent memory leak and fix state

[Andy Shevchenko]

On Fri, Aug 28, 2026 at 10:40:51AM +0300, Matti Vaittinen wrote:

> The driver allocates memory for samples at buffer enable path. If regmap
> operation fails in the kx022a_fifo_enable() at the buffer enable path, the
> allocated memory is never freed. Furthermore, the state information and
> previous hardware configuration(s) aren't undone, potentially leaving
> WMI interrupts and buffers enabled, or driver state flags wrong.
> 
> Free the memory and revert the hardware configuration and state flags on
> error path.

...

> static int kx022a_fifo_enable(struct kx022a_data *data)

With

	struct regmap *map = data->regmap;

>  	guard(mutex)(&data->mutex);
>  	ret = __kx022a_turn_on_off(data, false);
>  	if (ret)
> -		return ret;
> +		goto err_free_out;
>  
>  	/* Update watermark to HW */
>  	ret = kx022a_fifo_set_wmi(data);
>  	if (ret)
> -		return ret;
> +		goto err_wmi_out;
>  
>  	/* Enable buffer */
>  	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
>  			      KX022A_MASK_BUF_EN);
>  	if (ret)
> -		return ret;
> +		goto err_wmi_out;
>  
>  	data->state |= KX022A_STATE_FIFO;
>  	ret = regmap_set_bits(data->regmap, data->ien_reg,
>  			      KX022A_MASK_WMI);
>  	if (ret)
> -		return ret;
> +		goto err_buf_en_out;
>  
> -	return __kx022a_turn_on_off(data, true);
> +	ret = __kx022a_turn_on_off(data, true);
> +	if (ret)
> +		goto err_on_out;
> +
> +	return ret;
> +
> +err_on_out:
> +	regmap_clear_bits(data->regmap, data->ien_reg,
> +			  KX022A_MASK_WMI);

in particular this will be

	regmap_clear_bits(map, data->ien_reg, KX022A_MASK_WMI);

> +err_buf_en_out:
> +	regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
> +			  KX022A_MASK_BUF_EN);

	regmap_clear_bits(map, data->chip_info->buf_cntl2, KX022A_MASK_BUF_EN);

Which saves a single line, and the followup may shorten the existing code even
more.

> +	data->state &= ~KX022A_STATE_FIFO;
> +err_wmi_out:
> +	__kx022a_turn_on_off(data, true);
> +err_free_out:
> +	kfree(data->fifo_buffer);
> +
> +	return ret;
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

[Matti Vaittinen]

On 28/08/2026 10:53, Andy Shevchenko wrote:
> On Fri, Aug 28, 2026 at 10:40:36AM +0300, Matti Vaittinen wrote:
> 
>> When reading an integration-time fails, the code will use error code to
>> compute the sleep time.
>>
>> Fix this by using the smallest integration time as a default if
>> reading fails.
> 
> ...
> 
>>   	wait_ms = bu27034_get_int_time(data);
>> +
>> +	/*
>> +	 * If reading the integration time fails, default to the minimum so we
>> +	 * don't lose samples. This may waste CPU cycles, but as a hardening
>> +	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
>> +	 */
>> +	if (wait_ms < 0)
>> +		wait_ms = BU27034_INT_TIME_US_MIN;
>> +
>>   	wait_ms /= 1000;
> 
> With the above being open coded the _ms feels not right.
> I would expect the TIME_MIN to be in MS from the start
> (and for the consistency's sake with the below) and having
> all this to be written like
> 
> 	ret = bu27034_get_int_time(data);
> 	if (ret < 0)
> 		wait_ms = _MS_MIN;
> 	else
> 		wait_ms = ret / USEC_PER_MSEC;
> 
>>   	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;

I don't like using 'ret' there.

At first glance, the
ret = bu27034_get_int_time(data);

looks like ret is containing just the success status. Furthermore,
 > 		wait_ms = ret / USEC_PER_MSEC;

forces one to go back and see WTF the 'ret' is (even if just couple of 
lines - but this is not an improvement, using ret is obfuscation).

I could change this to:

wait_ms = bu27034_get_int_time(data) / USEC_PER_MSEC;
if (wait_ms < BU27034_INT_TIME_MIN_MS)
                 wait_ms = BU27034_INT_TIME_MIN_MS;

(but for me this feels like unnecessary bikeshedding than anything else.)

Well, I'll change this if I respin.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v2 3/4] iio: accel: kionix-kx022a: Prevent memory leak and fix state

[Matti Vaittinen]

On 28/08/2026 10:56, Andy Shevchenko wrote:
> On Fri, Aug 28, 2026 at 10:40:51AM +0300, Matti Vaittinen wrote:
> 
>> The driver allocates memory for samples at buffer enable path. If regmap
>> operation fails in the kx022a_fifo_enable() at the buffer enable path, the
>> allocated memory is never freed. Furthermore, the state information and
>> previous hardware configuration(s) aren't undone, potentially leaving
>> WMI interrupts and buffers enabled, or driver state flags wrong.
>>
>> Free the memory and revert the hardware configuration and state flags on
>> error path.
> 
> ...
> 
>> static int kx022a_fifo_enable(struct kx022a_data *data)
> 
> With
> 
> 	struct regmap *map = data->regmap;
> 
>>   	guard(mutex)(&data->mutex);
>>   	ret = __kx022a_turn_on_off(data, false);
>>   	if (ret)
>> -		return ret;
>> +		goto err_free_out;
>>   
>>   	/* Update watermark to HW */
>>   	ret = kx022a_fifo_set_wmi(data);
>>   	if (ret)
>> -		return ret;
>> +		goto err_wmi_out;
>>   
>>   	/* Enable buffer */
>>   	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
>>   			      KX022A_MASK_BUF_EN);
>>   	if (ret)
>> -		return ret;
>> +		goto err_wmi_out;
>>   
>>   	data->state |= KX022A_STATE_FIFO;
>>   	ret = regmap_set_bits(data->regmap, data->ien_reg,
>>   			      KX022A_MASK_WMI);
>>   	if (ret)
>> -		return ret;
>> +		goto err_buf_en_out;
>>   
>> -	return __kx022a_turn_on_off(data, true);
>> +	ret = __kx022a_turn_on_off(data, true);
>> +	if (ret)
>> +		goto err_on_out;
>> +
>> +	return ret;
>> +
>> +err_on_out:
>> +	regmap_clear_bits(data->regmap, data->ien_reg,
>> +			  KX022A_MASK_WMI);
> 
> in particular this will be
> 
> 	regmap_clear_bits(map, data->ien_reg, KX022A_MASK_WMI);
> 
>> +err_buf_en_out:
>> +	regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
>> +			  KX022A_MASK_BUF_EN);
> 
> 	regmap_clear_bits(map, data->chip_info->buf_cntl2, KX022A_MASK_BUF_EN);
> 
> Which saves a single line, and the followup may shorten the existing code even
> more.

As far as I can see, the driver is consistently using the 'data->regmap' 
everywhere. Using 'map' just in one function would raise question if it 
is different regmap from every other place.

I am 100% Ok with anyone changing the data->regmap to 'map' throughout 
the whole driver in order to cut the amount of lines - but it is not 
something to be done in a bugfix commit.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH v2 2/4] iio: light: rohm-bu27034: Fix infinite delay on error

[Andy Shevchenko]

On Fri, Aug 28, 2026 at 12:46:24PM +0300, Matti Vaittinen wrote:
> On 28/08/2026 10:53, Andy Shevchenko wrote:
> > On Fri, Aug 28, 2026 at 10:40:36AM +0300, Matti Vaittinen wrote:


...

> > >   	wait_ms = bu27034_get_int_time(data);
> > > +
> > > +	/*
> > > +	 * If reading the integration time fails, default to the minimum so we
> > > +	 * don't lose samples. This may waste CPU cycles, but as a hardening
> > > +	 * against theoretical, once-in-a-blue-moon error, this should be Ok.
> > > +	 */
> > > +	if (wait_ms < 0)
> > > +		wait_ms = BU27034_INT_TIME_US_MIN;
> > > +
> > >   	wait_ms /= 1000;
> > 
> > With the above being open coded the _ms feels not right.
> > I would expect the TIME_MIN to be in MS from the start
> > (and for the consistency's sake with the below) and having
> > all this to be written like
> > 
> > 	ret = bu27034_get_int_time(data);
> > 	if (ret < 0)
> > 		wait_ms = _MS_MIN;
> > 	else
> > 		wait_ms = ret / USEC_PER_MSEC;
> > 
> > >   	wait_ms -= BU27034_MEAS_WAIT_PREMATURE_MS;
> 
> I don't like using 'ret' there.
> 
> At first glance, the
> ret = bu27034_get_int_time(data);
> 
> looks like ret is containing just the success status. Furthermore,
> > 		wait_ms = ret / USEC_PER_MSEC;
> 
> forces one to go back and see WTF the 'ret' is (even if just couple of lines
> - but this is not an improvement, using ret is obfuscation).
> 
> I could change this to:

I suggested without knowing the possible ranges of the returned value.

> wait_ms = bu27034_get_int_time(data) / USEC_PER_MSEC;
> if (wait_ms < BU27034_INT_TIME_MIN_MS)
>                 wait_ms = BU27034_INT_TIME_MIN_MS;

This looks sane to me and  removes the confusion I was talking about.

> (but for me this feels like unnecessary bikeshedding than anything else.)
> 
> Well, I'll change this if I respin.

-- 
With Best Regards,
Andy Shevchenko