ROHM IIO fixes
Subsystems (6)
cover ROHM IIO fixes
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: 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 (12):
iio: adc: rohm-bd79124: Fix rising alarm
iio: adc: rohm-bd79124: Fix channel initialization
iio: adc: rohm-bd79124: Fix GPIO mask check
iio: adc: rohm-bd79124: Catch regmap errors at measurement start/stop
iio: dac: rohm-bd79703: Do not allow writing SCALE
iio: pressure: rohm-bm1390: Return error when read fails
iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
iio: light: rohm-bu27034: Fix error return
iio: light: rohm-bu27034: Fix infinite delay on error
iio: accel: kionix-kx022a: Fix array boundary check
iio: accel: kionix-kx022a: Prevent memory leak and fix state
iio: accel: kionix-kx022a: Fix IPOL macro name
drivers/iio/accel/kionix-kx022a.c | 29 +++++++++++++++++++++++------ drivers/iio/accel/kionix-kx022a.h | 2 +- drivers/iio/adc/rohm-bd79124.c | 20 ++++++++++++++------ drivers/iio/dac/rohm-bd79703.c | 3 +++ drivers/iio/light/rohm-bu27034.c | 12 +++++++++++- drivers/iio/pressure/rohm-bm1390.c | 6 ++++-- 6 files changed, 56 insertions(+), 16 deletions(-)
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
Signature · 1 line
2.55.0
PATCH 1/12 iio: adc: rohm-bd79124: Fix rising alarm
From: Matti Vaittinen <[email protected]>
The rising alarm is using the cached falling alarm value causing wrong value to be used.
Fix this by using the correct cached value for rising alarm.
--- drivers/iio/adc/rohm-bd79124.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/adc/rohm-bd79124.c +1 -1
@@ -525,7 +525,7 @@ static int bd79124_enable_event(struct bd79124_data *data,return ret;if (dir ==IIO_EV_DIR_RISING ) { - limit = &data->alarm_f_limit[channel]; + limit = &data->alarm_r_limit[channel]; reg =BD79124_GET_HIGH_LIMIT_REG (channel); }else { limit = &data->alarm_f_limit[channel];
Signature · 1 line
2.55.0
Re: [PATCH 01/12] iio: adc: rohm-bd79124: Fix rising alarm
Quoted 9 lines · On Mon, 10 Aug 2026 10:49:56 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The rising alarm is using the cached falling alarm value causing wrong value to be used.
Fix this by using the correct cached value for rising alarm.
Signed-off-by Matti Vaittinen <[email protected]>Fixes 3f57a3b9ab74 ("iio: adc: Support ROHM BD79124 ADC")
Applied to the fixes-togreg branch of iio.git and marked for stable.
Thanks,
Jonathan
Quoted 17 lines
--- drivers/iio/adc/rohm-bd79124.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/adc/rohm-bd79124.c +1
-1@@ -525,7 +525,7 @@ static int bd79124_enable_event(struct bd79124_data *data,return ret;if (dir ==IIO_EV_DIR_RISING ) { - limit = &data->alarm_f_limit[channel]; + limit = &data->alarm_r_limit[channel]; reg =BD79124_GET_HIGH_LIMIT_REG (channel); }else { limit = &data->alarm_f_limit[channel];
PATCH 2/12 iio: adc: rohm-bd79124: Fix channel initialization
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
The alarm limit register on BD79124 span to two consecutive registers. The first register for high-limit containing also hysteresis configuration. The initialization in driver writes only one 8-bit register, overwriting the hysteresis and leaving the other limit register uninitialized.
Fix the limit initialization by using the bd79124_write_int_to_reg(), which correctly initializes the limit on both of the registers.
--- drivers/iio/adc/rohm-bd79124.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/iio/adc/rohm-bd79124.c +4 -4
@@ -920,13 +920,13 @@ static int bd79124_chan_init(struct bd79124_data *data, int channel) {int ret; - ret =regmap_write (data->map,BD79124_GET_HIGH_LIMIT_REG (channel), -BD79124_HIGH_LIMIT_MAX ); + ret =bd79124_write_int_to_reg (data,BD79124_GET_HIGH_LIMIT_REG (channel), +BD79124_HIGH_LIMIT_MAX );if (ret)return ret; -return regmap_write (data->map,BD79124_GET_LOW_LIMIT_REG (channel), -BD79124_LOW_LIMIT_MIN ); +return bd79124_write_int_to_reg (data,BD79124_GET_LOW_LIMIT_REG (channel), +BD79124_LOW_LIMIT_MIN ); }static int bd79124_get_gpio_pins (const struct iio_chan_spec *cs,int num_channels)
Signature · 1 line
2.55.0
Jonathan Cameron
maintainerRe: [PATCH 02/12] iio: adc: rohm-bd79124: Fix channel initialization
Quoted 8 lines · On Mon, 10 Aug 2026 10:50:21 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The alarm limit register on BD79124 span to two consecutive registers. The first register for high-limit containing also hysteresis configuration. The initialization in driver writes only one 8-bit register, overwriting the hysteresis and leaving the other limit register uninitialized.
Quoted 5 lines
Fix the limit initialization by using the
bd79124_write_int_to_reg(), which correctly initializes the limit on both of the registers.Signed-off-by Matti Vaittinen <[email protected]>Fixes 3f57a3b9ab74 ("iio: adc: Support ROHM BD79124 ADC")
Applied and marked for stable.
PATCH 3/12 iio: adc: rohm-bd79124: Fix GPIO mask check
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
The ROHM BD79124 has pins which can be configured as GPOs or as ADC inputs. The bd79124gpo_set_multiple() is intended to ensure that a pin which is requested to be toggled, is indeed configured as a GPO.
The check uses XOR, causing it to fail if mask is not exactly same as GPO configuration. Eg, if not all GPOs are asked to be toggled at once.
Fix this by checking that mask does not contain pins that are not configured as GPO, allowing some of the pins which are configured as GPO to be untouched.
--- drivers/iio/adc/rohm-bd79124.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/adc/rohm-bd79124.c +1 -1
@@ -201,7 +201,7 @@ static int bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,if (ret)return ret; -if (all_gpos ^ *mask) { +if (*mask & ~all_gpos) {dev_dbg (data->dev,"Invalid mux config. Can't set value.\n" );return -EINVAL ;
Signature · 1 line
2.55.0
Jonathan Cameron
maintainerRe: [PATCH 03/12] iio: adc: rohm-bd79124: Fix GPIO mask check
Quoted 9 lines · On Mon, 10 Aug 2026 10:50:49 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The ROHM BD79124 has pins which can be configured as GPOs or as ADC inputs. The
bd79124gpo_set_multiple()is intended to ensure that a pin which is requested to be toggled, is indeed configured as a GPO.The check uses XOR, causing it to fail if mask is not exactly same as GPO configuration. Eg, if not all GPOs are asked to be toggled at once.
Quoted 6 lines
Fix this by checking that mask does not contain pins that are not configured as GPO, allowing some of the pins which are configured as GPO to be untouched.
Signed-off-by Matti Vaittinen <[email protected]>Fixes 3f57a3b9ab74 ("iio: adc: Support ROHM BD79124 ADC")
Applied and marked for stable.
PATCH 4/12 iio: adc: rohm-bd79124: Catch regmap errors at measurement start/stop
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
The bd79124_start_measurement() and bd79124_stop_measurement() ignore errors from the regmap reads, causing potential use of uninitialized stack variable when deciding whether the measurement is already started/stopped. The bd79124_stop_measurement() may also ignore failure to clear the sequencer state bits, which may make the hardware to ignore the setting and leave hardware and driver states out of sync.
Check the return value and bail-out if error is detected.
--- drivers/iio/adc/rohm-bd79124.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/iio/adc/rohm-bd79124.c +8 -0
@@ -382,6 +382,9 @@ static int bd79124_start_measurement(struct bd79124_data *data, int chan)/* See if already started */ ret =regmap_read (data->map,BD79124_REG_AUTO_CHANNELS , &val); +if (ret) +return ret; +if (val &BIT (chan))return 0 ;
@@ -421,11 +424,16 @@ static int bd79124_stop_measurement(struct bd79124_data *data, int chan)/* See if already stopped */ ret =regmap_read (data->map,BD79124_REG_AUTO_CHANNELS , &enabled_chans); +if (ret) +return ret; +if (!(enabled_chans &BIT (chan)))return 0 ; ret =regmap_clear_bits (data->map,BD79124_REG_SEQ_CFG ,BD79124_MSK_SEQ_START ); +if (ret) +return ret;/* Clear the channel from the measured channels */ enabled_chans &= ~BIT (chan);
Signature · 1 line
2.55.0
Jonathan Cameron
maintainerRe: [PATCH 04/12] iio: adc: rohm-bd79124: Catch regmap errors at measurement start/stop
Quoted 9 lines · On Mon, 10 Aug 2026 10:51:20 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The
bd79124_start_measurement()andbd79124_stop_measurement()ignore errors from the regmap reads, causing potential use of uninitialized stack variable when deciding whether the measurement is already started/stopped. Thebd79124_stop_measurement()may also ignore failure to clear the sequencer state bits, which may make the hardware to ignore the setting and leave hardware and driver states out of sync.
Quoted 4 lines
Check the return value and bail-out if error is detected.
Signed-off-by Matti Vaittinen <[email protected]>Fixes 3f57a3b9ab74 ("iio: adc: Support ROHM BD79124 ADC")
Applied and marked for stable
Thanks,
J
PATCH 5/12 iio: dac: rohm-bd79703: Do not allow writing SCALE
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
The BD79703 has adds IIO_CHAN_INFO_SCALE in the info_mask_shared_by_type so users can read the scale, which depends on the used reference voltage. This, however, enables users to try writing the scale as well. This isn't really supported but the bd79703_write_raw() does not check the mask, and if written scale values pass the validation, the driver will proceed writing the DAC value when users writes the scale.
Prevent the unsupported scale setting and return an error.
--- drivers/iio/dac/rohm-bd79703.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/iio/dac/rohm-bd79703.c +3 -0
@@ -73,6 +73,9 @@ static int bd79703_write_raw(struct iio_dev *idev, {struct bd79703_data *data =iio_priv (idev); +if (mask !=IIO_CHAN_INFO_RAW ) +return -EINVAL ; +if (val <0 || val >=1 <<BD79703_DAC_BITS )return -EINVAL ;
Signature · 1 line
2.55.0
Re: [PATCH 05/12] iio: dac: rohm-bd79703: Do not allow writing SCALE
Quoted 9 lines · On Mon, 10 Aug 2026 10:51:38 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The BD79703 has adds
IIO_CHAN_INFO_SCALEin theinfo_mask_shared_by_typeso users can read the scale, which depends on the used reference voltage. This, however, enables users to try writing the scale as well. This isn't really supported but thebd79703_write_raw()does not check the mask, and if written scale values pass the validation, the driver will proceed writing the DAC value when users writes the scale.
Quoted 4 lines
Prevent the unsupported scale setting and return an error.
Signed-off-by Matti Vaittinen <[email protected]>Fixes af6aca656a85 ("iio: dac: Support ROHM BD79703 DAC")
Applied.
BTW convention is Fixes goes before SoB.
I did wonder if is documented anywhere and claude sent me to the tip docs.
https://www.kernel.org/doc/Documentation/process/maintainer-tip.rst
IIO can follow that.
Anyhow, I'll swap them around whilst applying.
Quoted 18 lines
--- drivers/iio/dac/rohm-bd79703.c | 3 +++ 1 file changed, 3 insertions(+)drivers/iio/dac/rohm-bd79703.c +3
-0@@ -73,6 +73,9 @@ static int bd79703_write_raw(struct iio_dev *idev, {struct bd79703_data *data =iio_priv (idev); +if (mask !=IIO_CHAN_INFO_RAW ) +return -EINVAL ; +if (val <0 || val >=1 <<BD79703_DAC_BITS )return -EINVAL ;
Matti Vaittinen
maintainerRe: [PATCH 05/12] iio: dac: rohm-bd79703: Do not allow writing SCALE
Quoted 19 lines · Jonathan Cameron
Quoted 9 lines · On Mon, 10 Aug 2026 10:51:38 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The BD79703 has adds
IIO_CHAN_INFO_SCALEin theinfo_mask_shared_by_typeso users can read the scale, which depends on the used reference voltage. This, however, enables users to try writing the scale as well. This isn't really supported but thebd79703_write_raw()does not check the mask, and if written scale values pass the validation, the driver will proceed writing the DAC value when users writes the scale.Quoted 4 lines
Prevent the unsupported scale setting and return an error.
Signed-off-by Matti Vaittinen <[email protected]>Fixes af6aca656a85 ("iio: dac: Support ROHM BD79703 DAC")Applied.
Quoted 8 lines
BTW convention is Fixes goes before SoB.
I did wonder if is documented anywhere and claude sent me to the tip docs.
https://www.kernel.org/doc/Documentation/process/maintainer-tip.rstIIO can follow that.
Anyhow, I'll swap them around whilst applying.
Thanks x2. (Once for pointing it out and once for fixing it!)
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
PATCH 6/12 iio: pressure: rohm-bm1390: Return error when read fails
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
The data reading function ignores the cached error value, and unconditionally returns 0. Return cached 'ret' -value after stopping the measurement so user knows if read failed and data is garbage.
--- drivers/iio/pressure/rohm-bm1390.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/pressure/rohm-bm1390.c +1 -1
@@ -289,7 +289,7 @@ static int bm1390_read_data(struct bm1390_data *data,if (warn)dev_warn (data->dev,"Failed to stop measurement (%d)\n" , warn); -return 0 ; +return ret; } static int bm1390_read_raw(struct iio_dev *idev,
Signature · 1 line
2.55.0
Re: [PATCH 06/12] iio: pressure: rohm-bm1390: Return error when read fails
Quoted 8 lines · On Mon, 10 Aug 2026 10:52:47 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The data reading function ignores the cached error value, and unconditionally returns 0. Return cached 'ret' -value after stopping the measurement so user knows if read failed and data is garbage.
Signed-off-by Matti Vaittinen <[email protected]>Fixes 534674463a59 ("iio: bm1390: simplify using guard(mutex)")
Applied.
Thanks,
J
Quoted 17 lines
--- drivers/iio/pressure/rohm-bm1390.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/pressure/rohm-bm1390.c +1
-1@@ -289,7 +289,7 @@ static int bm1390_read_data(struct bm1390_data *data,if (warn)dev_warn (data->dev,"Failed to stop measurement (%d)\n" , warn); -return 0 ; +return ret; } static int bm1390_read_raw(struct iio_dev *idev,
PATCH 7/12 iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
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.
--- drivers/iio/pressure/rohm-bm1390.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/iio/pressure/rohm-bm1390.c +3 -1
@@ -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;
Signature · 1 line
2.55.0
Re: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
Quoted 7 lines · Matti Vaittinen
From: Matti Vaittinen <[email protected]>
The BM1390 tries to initialize the
AVE_NUMto 110b at the start-up. The field location is not taken into account, and value is written unsifted. This causes theAVE_NUMto be initialized to zero.Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.
Can it be FIELD_PREP_CONST()?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Matti Vaittinen
maintainerRe: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
Quoted 10 lines · Andy Shevchenko
Quoted 7 lines · Matti Vaittinen
From: Matti Vaittinen <[email protected]>
The BM1390 tries to initialize the
AVE_NUMto 110b at the start-up. The field location is not taken into account, and value is written unsifted. This causes theAVE_NUMto be initialized to zero.Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Can it be
FIELD_PREP_CONST()?
Can you please elaborate why it should in this case?
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Re: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Mon, Aug 10, 2026 at 10:53:07AM +0300, Matti Vaittinen wrote:
...
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Can it be
FIELD_PREP_CONST()?Can you please elaborate why it should in this case?
I'm not sure why it's not obvious, but the parameters are constants, why would we need an additional check or even checks?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
Quoted 6 lines · On Tue, 11 Aug 2026 13:08:14 +0300 Andy Shevchenko…
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Mon, Aug 10, 2026 at 10:53:07AM +0300, Matti Vaittinen wrote:
...
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Can it be
FIELD_PREP_CONST()?Can you please elaborate why it should in this case?
I'm not sure why it's not obvious, but the parameters are constants, why would we need an additional check or even checks?
I'm not sure it is as obvious as the name would suggest. FIELD_PREP() has a whole bunch of stuff that runs if it happens to have a build time constant value + FIELD_PREP_CONST() is documented as primarily being there when the code requires a constant to be correct. Whether FIELD_PREP() collapses to a constant in a recent compiler is something I'm not sure of.
Jonathan
Matti Vaittinen
maintainerRe: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
Quoted 15 lines · Jonathan Cameron
Quoted 12 lines · On Tue, 11 Aug 2026 13:08:14 +0300 Andy Shevchenko…
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Mon, Aug 10, 2026 at 10:53:07AM +0300, Matti Vaittinen wrote:
...
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Can it be
FIELD_PREP_CONST()?Can you please elaborate why it should in this case?
Quoted 8 lines
Quoted 3 lines
I'm not sure why it's not obvious, but the parameters are constants, why would we need an additional check or even checks?
I'm not sure it is as obvious as the name would suggest.
FIELD_PREP()has a whole bunch of stuff that runs if it happens to have a build time constant value +FIELD_PREP_CONST()is documented as primarily being there when the code requires a constant to be correct.
This is what I read. My first thought was that FIELD_PREP_CONST() is useful when constant initializer is needed. So no, not obvious for me.
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Jonathan Cameron
maintainerRe: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
Quoted 19 lines · On Mon, 17 Aug 2026 08:43:26 +0300 Matti Vaittinen <[email protected]>…
Quoted 18 lines · Jonathan Cameron
Quoted 15 lines · On Tue, 11 Aug 2026 13:08:14 +0300 Andy Shevchenko…
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Mon, Aug 10, 2026 at 10:53:07AM +0300, Matti Vaittinen wrote:
...
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Can it be
FIELD_PREP_CONST()?Can you please elaborate why it should in this case?
I'm not sure why it's not obvious, but the parameters are constants, why would we need an additional check or even checks?
Quoted 9 lines
Quoted 5 lines
I'm not sure it is as obvious as the name would suggest.
FIELD_PREP()has a whole bunch of stuff that runs if it happens to have a build time constant value +FIELD_PREP_CONST()is documented as primarily being there when the code requires a constant to be correct.This is what I read. My first thought was that
FIELD_PREP_CONST()is useful when constant initializer is needed. So no, not obvious for me.
Applied. It's in the testing branch, so I can rebase if this discussion restarts, but it seems to have died out.
Jonathan
Quoted 3 lines
Yours,
-- Matti
Jonathan Cameron
maintainerRe: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
In reply to Andy ShevchenkoQuoted 6 lines · On Tue, 11 Aug 2026 13:08:14 +0300 Andy Shevchenko…
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Mon, Aug 10, 2026 at 10:53:07AM +0300, Matti Vaittinen wrote:
...
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Can it be
FIELD_PREP_CONST()?Can you please elaborate why it should in this case?
I'm not sure why it's not obvious, but the parameters are constants, why would we need an additional check or even checks?
Hmm. I went and had a look as this macro turns up a fair bit. I'm fairly sure in the case of a pair of compile time constants FIELD_PREP() both performs additional checks at compile time and probably collapses to a constant.
The docs for FIELD_PREP_CONST() observe it is for when you need it to be a constant rather than when it might be (even if you know it is)
So I think it's not necessarily appropriate here.
Re: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
Quoted 26 lines · Jonathan Cameron
Quoted 6 lines · On Tue, 11 Aug 2026 13:08:14 +0300 Andy Shevchenko…
Quoted 2 lines · Matti Vaittinen
Quoted 1 line · Andy Shevchenko
On Mon, Aug 10, 2026 at 10:53:07AM +0300, Matti Vaittinen wrote:
...
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Can it be
FIELD_PREP_CONST()?Can you please elaborate why it should in this case?
I'm not sure why it's not obvious, but the parameters are constants, why would we need an additional check or even checks?
Hmm. I went and had a look as this macro turns up a fair bit. I'm fairly sure in the case of a pair of compile time constants
FIELD_PREP()both performs additional checks at compile time and probably collapses to a constant.
The docs forFIELD_PREP_CONST()observe it is for when you need it to be a constant rather than when it might be (even if you know it is)
Quoted 1 line
So I think it's not necessarily appropriate here.
Thanks for digging deeper into this. :) Sounds like you confirm what I understood from the docs.
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Jonathan Cameron
maintainerRe: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
In reply to Matti VaittinenQuoted 27 lines · On Mon, 10 Aug 2026 10:53:07 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The BM1390 tries to initialize the
AVE_NUMto 110b at the start-up. The field location is not taken into account, and value is written unsifted. This causes theAVE_NUMto be initialized to zero.Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Signed-off-by Matti Vaittinen <[email protected]>Fixes 81ca5979b6ed ("iio: pressure: Support ROHM BU1390")--- drivers/iio/pressure/rohm-bm1390.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/iio/pressure/rohm-bm1390.c +1
-0@@ -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,
drivers/iio/pressure/rohm-bm1390.c static int bm1390_chip_init(struct bm1390_data *data) · line 518
Quoted 7 lines
drivers/iio/pressure/rohm-bm1390.c +2
-1@@ -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,
FIELD_PREP(BM1390_MASK_AVE_NUM, BM1390_IIR_AVE_NUM));
If respining I would drop the local variable and just go a bit long on the line. I like the mask to be clearly visible in both parameters and a local variable prevents that. Fine if the line is really long, but it's only about 83 chars here.
Jonathan
Quoted 3 lines
drivers/iio/pressure/rohm-bm1390.c +0
-0if (ret)return ret;
Matti Vaittinen
maintainerRe: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
Quoted 39 lines · Jonathan Cameron
Quoted 27 lines · On Mon, 10 Aug 2026 10:53:07 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The BM1390 tries to initialize the
AVE_NUMto 110b at the start-up. The field location is not taken into account, and value is written unsifted. This causes theAVE_NUMto be initialized to zero.Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Signed-off-by Matti Vaittinen <[email protected]>Fixes 81ca5979b6ed ("iio: pressure: Support ROHM BU1390")--- drivers/iio/pressure/rohm-bm1390.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/iio/pressure/rohm-bm1390.c +1
-0@@ -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,drivers/iio/pressure/rohm-bm1390.c static int bm1390_chip_init(struct bm1390_data *data) · line 518
Quoted 7 lines
drivers/iio/pressure/rohm-bm1390.c +2
-1@@ -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,FIELD_PREP(BM1390_MASK_AVE_NUM, BM1390_IIR_AVE_NUM));
Quoted 3 lines
If respining I would drop the local variable and just go a bit long on the line. I like the mask to be clearly visible in both parameters and a local variable prevents that. Fine if the line is really long, but it's only about 83 chars here.
I am not going to insist - but I will explain my view - which will sound like insisting XD
For me it is always a tad harder to read code when function or macro "calls" are done inside a parameter list. Hence I like a local variable here. I do 100% agree with you that mask (and sometimes raw numbers) should be visible in code - and not hidden into variables / defines. Still, in this case, the mask is assigned to the variable right above the line it gets used. So, I could argue the mask is still pretty visible, and FIELD_PREP() is called on it's own line and not inside the regmap_update_bits() parameter list - which is more readable for me.
Well, if you feel strongly about the variable - then I can ditch it. 83 chars is still okay(ish) - even on my "old eyes, big font and 3 terminals in parallel" -display setup ;)
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Jonathan Cameron
maintainerRe: [PATCH 07/12] iio: pressure: rohm-bm1390: Fix AVE_NUM initialization
Quoted 56 lines · On Mon, 17 Aug 2026 08:51:46 +0300 Matti Vaittinen <[email protected]>…
Quoted 39 lines · Jonathan Cameron
Quoted 27 lines · On Mon, 10 Aug 2026 10:53:07 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The BM1390 tries to initialize the
AVE_NUMto 110b at the start-up. The field location is not taken into account, and value is written unsifted. This causes theAVE_NUMto be initialized to zero.Use
FIELD_PREP()to shift the intendedAVE_NUMvalue to correct field.Signed-off-by Matti Vaittinen <[email protected]>Fixes 81ca5979b6ed ("iio: pressure: Support ROHM BU1390")--- drivers/iio/pressure/rohm-bm1390.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/iio/pressure/rohm-bm1390.c +1
-0@@ -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,drivers/iio/pressure/rohm-bm1390.c static int bm1390_chip_init(struct bm1390_data *data) · line 518
Quoted 7 lines
drivers/iio/pressure/rohm-bm1390.c +2
-1@@ -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,FIELD_PREP(BM1390_MASK_AVE_NUM, BM1390_IIR_AVE_NUM));Quoted 3 lines
If respining I would drop the local variable and just go a bit long on the line. I like the mask to be clearly visible in both parameters and a local variable prevents that. Fine if the line is really long, but it's only about 83 chars here.
I am not going to insist - but I will explain my view - which will sound like insisting XD
For me it is always a tad harder to read code when function or macro "calls" are done inside a parameter list. Hence I like a local variable here. I do 100% agree with you that mask (and sometimes raw numbers) should be visible in code - and not hidden into variables / defines. Still, in this case, the mask is assigned to the variable right above the line it gets used. So, I could argue the mask is still pretty visible, and
FIELD_PREP()is called on it's own line and not inside theregmap_update_bits()parameter list - which is more readable for me.
Quoted 3 lines
Well, if you feel strongly about the variable - then I can ditch it. 83 chars is still okay(ish) - even on my "old eyes, big font and 3 terminals in parallel" -display setup ;)
I don't care enough to insist :)
J
Quoted 4 lines
Yours,
-- Matti
PATCH 8/12 iio: light: rohm-bu27034: Fix error return
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
When BU27034 fails to read gain value at integration-time setting, it returns 0. This leaves caller unaware of the fact that setting the integration time failed.
Fix this by returning the relevant error instead of zero.
--- drivers/iio/light/rohm-bu27034.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/light/rohm-bu27034.c +1 -1
@@ -415,7 +415,7 @@ static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)for (i =0 ; i < numg; i++) { ret =bu27034_get_gain (data, gains[i].chan, &gains[i].old_gain);if (ret) -return 0 ; +return ret; ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gains[i].old_gain,
Signature · 1 line
2.55.0
Re: [PATCH 08/12] iio: light: rohm-bu27034: Fix error return
Quoted 10 lines · On Mon, 10 Aug 2026 10:53:35 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
When BU27034 fails to read gain value at integration-time setting, it returns 0. This leaves caller unaware of the fact that setting the integration time failed.
Fix this by returning the relevant error instead of zero.
Signed-off-by Matti Vaittinen <[email protected]>Fixes 439c2cef8157 ("iio: bu27034: simplify using guard(mutex)")
Applied.
Note I skipped 7 until that discussion comes to a conclusion.
Thanks,
Jonathan
Quoted 17 lines
--- drivers/iio/light/rohm-bu27034.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/light/rohm-bu27034.c +1
-1@@ -415,7 +415,7 @@ static int bu27034_try_set_int_time(struct bu27034_data *data, int time_us)for (i =0 ; i < numg; i++) { ret =bu27034_get_gain (data, gains[i].chan, &gains[i].old_gain);if (ret) -return 0 ; +return ret; ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gains[i].old_gain,
PATCH 9/12 iio: light: rohm-bu27034: Fix infinite delay on error
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
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.
--- drivers/iio/light/rohm-bu27034.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/iio/light/rohm-bu27034.c +10 -0
@@ -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_MIN 55000static 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_MIN ; + wait_ms /=1000 ; wait_ms -=BU27034_MEAS_WAIT_PREMATURE_MS ;
Signature · 1 line
2.55.0
Re: [PATCH 09/12] iio: light: rohm-bu27034: Fix infinite delay on error
Quoted 5 lines · Matti Vaittinen
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.
...
Quoted 1 line
+#define
BU27034_INT_TIME_MIN55000
#define BU27034_INT_TIME_US_MIN (55 * USEC_PER_MSEC)
?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Matti Vaittinen
maintainerRe: [PATCH 09/12] iio: light: rohm-bu27034: Fix infinite delay on error
Quoted 5 lines · Andy Shevchenko
Quoted 3 lines · Matti Vaittinen
When reading an integration-time fails, the code will use error code to compute the sleep time.
Quoted 8 lines
Quoted 2 lines
Fix this by using the smallest integration time as a default if reading fails.
...
Quoted 1 line
+#define
BU27034_INT_TIME_MIN55000#define
BU27034_INT_TIME_US_MIN(55 *USEC_PER_MSEC)
Yes. That looks better, thanks.
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
PATCH 10/12 iio: accel: kionix-kx022a: Fix array boundary check
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
The driver performs a sanity check for an array size, when converting a register value to an array index. The check incorrectly accepts the sizeof(array) as a last index, when last valid index should be sizeof(array) - 1.
Fix the check by bailing out when index >= sizeof(array).
--- drivers/iio/accel/kionix-kx022a.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/accel/kionix-kx022a.c +1 -1
@@ -649,7 +649,7 @@ static int kx022a_read_raw(struct iio_dev *idev,if (ret)return ret; -if ((regval &KX022A_MASK_ODR ) > +if ((regval &KX022A_MASK_ODR ) >=ARRAY_SIZE (kx022a_accel_samp_freq_table)) {dev_err (data->dev,"Invalid ODR\n" );return -EINVAL ;
Signature · 1 line
2.55.0
Re: [PATCH 10/12] iio: accel: kionix-kx022a: Fix array boundary check
Hi Matti,
Thank you for that patch!
Quoted 9 lines · Matti Vaittinen
From: Matti Vaittinen <[email protected]>
The driver performs a sanity check for an array size, when converting a register value to an array index. The check incorrectly accepts the sizeof(array) as a last index, when last valid index should be sizeof(array) - 1.
Fix the check by bailing out when index >= sizeof(array).
Quoted 2 lines
Signed-off-by Matti Vaittinen <[email protected]>Fixes 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
--
Kind Regards
Mehdi Djait
Jonathan Cameron
maintainerRe: [PATCH 10/12] iio: accel: kionix-kx022a: Fix array boundary check
Quoted 12 lines · On Wed, 12 Aug 2026 13:43:35 +0200 Mehdi Djait <[email protected]>…
Hi Matti,
Thank you for that patch!
Quoted 7 lines · Matti Vaittinen
From: Matti Vaittinen <[email protected]>
The driver performs a sanity check for an array size, when converting a register value to an array index. The check incorrectly accepts the sizeof(array) as a last index, when last valid index should be sizeof(array) - 1.
Quoted 4 lines
Quoted 2 lines
Fix the check by bailing out when index >= sizeof(array).
Reviewed-by Mehdi Djait <[email protected]>
Applied thanks,
Jonathan
Quoted 7 lines
Quoted 3 lines
Signed-off-by Matti Vaittinen <[email protected]>Fixes 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")--
Kind Regards
Mehdi Djait
PATCH 11/12 iio: accel: kionix-kx022a: Prevent memory leak and fix state
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
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.
--- drivers/iio/accel/kionix-kx022a.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
drivers/iio/accel/kionix-kx022a.c +22 -5
@@ -980,26 +980,43 @@ 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_free_out;/* Enable buffer */ ret =regmap_set_bits (data->regmap, data->chip_info->buf_cntl2,KX022A_MASK_BUF_EN );if (ret) -return ret; + goto err_free_out; data->state |=KX022A_STATE_FIFO ; ret =regmap_set_bits (data->regmap, data->ien_reg,KX022A_MASK_WMI );if (ret) -return ret; + goto err_wmi_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_wmi_out: +regmap_clear_bits (data->regmap, data->chip_info->buf_cntl2, +KX022A_MASK_BUF_EN ); +err_free_out: +kfree (data->fifo_buffer); + data->state &= ~KX022A_STATE_FIFO ; +__kx022a_turn_on_off (data, true); + +return ret; }static int kx022a_buffer_postenable (struct iio_dev *idev)
Signature · 1 line
2.55.0
Re: [PATCH 11/12] iio: accel: kionix-kx022a: Prevent memory leak and fix statey
Hi Matti,
Thank you for the patch!
Quoted 10 lines · Matti Vaittinen
From: Matti Vaittinen <[email protected]>
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.
drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data)
Quoted 11 lines
Signed-off-by Matti Vaittinen <[email protected]>Fixes e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")--- drivers/iio/accel/kionix-kx022a.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)drivers/iio/accel/kionix-kx022a.c +0
-0@@ -980,26 +980,43 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
Do you think this error handling should also be done for kx022a_fifo_disable():
in case __kx022a_turn_on_off() or one of the regmap_clear_bits() fails before we free the fifo_buffer ?
--
Kind Regards
Mehdi Djait
Matti Vaittinen
maintainerRe: [PATCH 11/12] iio: accel: kionix-kx022a: Prevent memory leak and fix statey
Quoted 24 lines · Mehdi Djait
Hi Matti,
Thank you for the patch!
Quoted 10 lines · Matti Vaittinen
From: Matti Vaittinen <[email protected]>
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.
Reviewed-by Mehdi Djait <[email protected]>Quoted 6 lines
Signed-off-by Matti Vaittinen <[email protected]>Fixes e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")--- drivers/iio/accel/kionix-kx022a.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
Quoted 9 lines
drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data)
Quoted 5 lines
drivers/iio/accel/kionix-kx022a.c +0
-0@@ -980,26 +980,43 @@ static int kx022a_fifo_enable(struct kx022a_data *data)Do you think this error handling should also be done for
kx022a_fifo_disable():
in case__kx022a_turn_on_off()or one of theregmap_clear_bits()fails before we free thefifo_buffer?
Good question. I am not really sure what IIO thinks if disable returns an error. Should the driver be in a state where it can handle re-try of a disable (Eg, a new call)?
If so, then returning an error from "turning on" the IC
return __kx022a_turn_on_off(data, true);
is problematic as fifo_buffer is already freed. New call to the disable would then cause a double free. If this is the case, it should be fixed, perhaps by just "NULLing" the fifo_buffer after the free.
If IIO does not allow "re-disable", then failures before the free() will cause a memory leak. Furthermore, the hardware state may be left inconsistent. Considering that such errors should be quite unexpected, and probably a symptom of shomething being off - I am not really terribly worried about the leak or inconsistent state - and I don't really know what else the driver could be except re-try - which might be a never ending story if problem persists.
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Jonathan Cameron
maintainerRe: [PATCH 11/12] iio: accel: kionix-kx022a: Prevent memory leak and fix statey
Quoted 34 lines · On Fri, 14 Aug 2026 10:39:48 +0300 Matti Vaittinen <[email protected]>…
Quoted 24 lines · Mehdi Djait
Hi Matti,
Thank you for the patch!
Quoted 10 lines · Matti Vaittinen
From: Matti Vaittinen <[email protected]>
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.
Reviewed-by Mehdi Djait <[email protected]>Quoted 6 lines
Signed-off-by Matti Vaittinen <[email protected]>Fixes e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")--- drivers/iio/accel/kionix-kx022a.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)Quoted 9 lines
drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data)
Quoted 5 lines
drivers/iio/accel/kionix-kx022a.c +0
-0@@ -980,26 +980,43 @@ static int kx022a_fifo_enable(struct kx022a_data *data)Do you think this error handling should also be done for
kx022a_fifo_disable():
in case__kx022a_turn_on_off()or one of theregmap_clear_bits()fails before we free thefifo_buffer?
Quoted 3 lines
Good question. I am not really sure what IIO thinks if disable returns an error. Should the driver be in a state where it can handle re-try of a disable (Eg, a new call)?
It carries on when it gets a disable on the buffer tear down path. It isn't obvious what the right choice is and we can't really just back off and let things try again as some of the reasons this occurs are things like driver unbind which doesn't fail.
So best effort for error handling only on the tear down + ideally some prints so we know what has gone wrong if it does happen.
Writing a driver that survives random bus transmission failures in every uncommon path is really hard and the complexity is often not worthwhile.
Jonathan
Quoted 18 lines
If so, then returning an error from "turning on" the IC return
__kx022a_turn_on_off(data, true);
is problematic asfifo_bufferis already freed. New call to the disable would then cause a double free. If this is the case, it should be fixed, perhaps by just "NULLing" thefifo_bufferafter the free.If IIO does not allow "re-disable", then failures before the
free()will cause a memory leak. Furthermore, the hardware state may be left inconsistent. Considering that such errors should be quite unexpected, and probably a symptom of shomething being off - I am not really terribly worried about the leak or inconsistent state - and I don't really know what else the driver could be except re-try - which might be a never ending story if problem persists.Yours,
-- Matti
Jonathan Cameron
maintainerRe: [PATCH 11/12] iio: accel: kionix-kx022a: Prevent memory leak and fix state
In reply to Matti VaittinenQuoted 22 lines · On Mon, 10 Aug 2026 10:55:03 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
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.
Signed-off-by Matti Vaittinen <[email protected]>Fixes e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")--- drivers/iio/accel/kionix-kx022a.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)drivers/iio/accel/kionix-kx022a.c +0
-0
drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 983
Quoted 5 lines
drivers/iio/accel/kionix-kx022a.c +1
-1@@ -980,26 +980,43 @@ 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;
If we follow this path we are assuming that turn_on_off hasn't had any side effects in failing...
Quoted 28 lines
drivers/iio/accel/kionix-kx022a.c +7
-4/* Update watermark to HW */ ret =kx022a_fifo_set_wmi (data);if (ret) -return ret; + goto err_free_out;/* Enable buffer */ ret =regmap_set_bits (data->regmap, data->chip_info->buf_cntl2,KX022A_MASK_BUF_EN );if (ret) -return ret; + goto err_free_out; data->state |=KX022A_STATE_FIFO ; ret =regmap_set_bits (data->regmap, data->ien_reg,KX022A_MASK_WMI );if (ret) -return ret; + goto err_wmi_out; -return __kx022a_turn_on_off (data, true); + ret =__kx022a_turn_on_off (data, true); +if (ret) + goto err_on_out; +
drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data)
Quoted 10 lines
drivers/iio/accel/kionix-kx022a.c +10
-0+return ret; + +err_on_out: +regmap_clear_bits (data->regmap, data->ien_reg, +KX022A_MASK_WMI ); +err_wmi_out: +regmap_clear_bits (data->regmap, data->chip_info->buf_cntl2, +KX022A_MASK_BUF_EN ); +err_free_out: +kfree (data->fifo_buffer);
This thing is fine here.
drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1016
Quoted 1 line
drivers/iio/accel/kionix-kx022a.c +1
-0+ data->state &= ~KX022A_STATE_FIFO ;
This should also only occur when we have set it in the first place - so under err_wmi_out:
drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1017
Quoted 1 line
drivers/iio/accel/kionix-kx022a.c +1
-0+__kx022a_turn_on_off (data, true);
So following path above we should not be calling this. It might be safe to do so but it isn't logically correct. It should be a few lines earlier.
Quoted 5 lines
drivers/iio/accel/kionix-kx022a.c +2
-0+ +return ret; }static int
kx022a_buffer_postenable(struct iio_dev *idev)
Matti Vaittinen
maintainerRe: [PATCH 11/12] iio: accel: kionix-kx022a: Prevent memory leak and fix state
Quoted 88 lines · Jonathan Cameron
Quoted 22 lines · On Mon, 10 Aug 2026 10:55:03 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
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.
Signed-off-by Matti Vaittinen <[email protected]>Fixes e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")--- drivers/iio/accel/kionix-kx022a.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)drivers/iio/accel/kionix-kx022a.c +0
-0drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 983
Quoted 5 lines
drivers/iio/accel/kionix-kx022a.c +1
-1@@ -980,26 +980,43 @@ 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;If we follow this path we are assuming that
turn_on_offhasn't had any side effects in failing...Quoted 28 lines
drivers/iio/accel/kionix-kx022a.c +7
-4/* Update watermark to HW */ ret =kx022a_fifo_set_wmi (data);if (ret) -return ret; + goto err_free_out;/* Enable buffer */ ret =regmap_set_bits (data->regmap, data->chip_info->buf_cntl2,KX022A_MASK_BUF_EN );if (ret) -return ret; + goto err_free_out; data->state |=KX022A_STATE_FIFO ; ret =regmap_set_bits (data->regmap, data->ien_reg,KX022A_MASK_WMI );if (ret) -return ret; + goto err_wmi_out; -return __kx022a_turn_on_off (data, true); + ret =__kx022a_turn_on_off (data, true); +if (ret) + goto err_on_out; +drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data)
Quoted 10 lines
drivers/iio/accel/kionix-kx022a.c +10
-0+return ret; + +err_on_out: +regmap_clear_bits (data->regmap, data->ien_reg, +KX022A_MASK_WMI ); +err_wmi_out: +regmap_clear_bits (data->regmap, data->chip_info->buf_cntl2, +KX022A_MASK_BUF_EN ); +err_free_out: +kfree (data->fifo_buffer);This thing is fine here.
drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1016
Quoted 1 line
drivers/iio/accel/kionix-kx022a.c +1
-0+ data->state &= ~KX022A_STATE_FIFO ;This should also only occur when we have set it in the first place - so under
err_wmi_out:drivers/iio/accel/kionix-kx022a.c static int kx022a_fifo_enable(struct kx022a_data *data) · line 1017
Quoted 1 line
drivers/iio/accel/kionix-kx022a.c +1
-0+__kx022a_turn_on_off (data, true);So following path above we should not be calling this. It might be safe to do so but it isn't logically correct. It should be a few lines earlier.
Quoted 6 lines
Quoted 5 lines
drivers/iio/accel/kionix-kx022a.c +2
-0+ +return ret; }static int
kx022a_buffer_postenable(struct iio_dev *idev)
Thanks Jonathan. I think you're right. I've no idea what I was thinking when writing this fix...
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
PATCH 12/12 iio: accel: kionix-kx022a: Fix IPOL macro name
In reply to Matti VaittinenFrom: Matti Vaittinen <[email protected]>
The "interrupt polarity high" -macro for KX022A variant is defined as:
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.
---
It appears the KX022A_IPOL_HIGH is unused as the IRQ is currently, unconditionally set level low in kx022a_prepare_irq_pin(). The KX022A hardware would support other configurations though. So, another potential fix would be removing the define altogether. I decided to leave the define there, as having it will allow users who need different configuration to simply change the KX022A_IPOL_LOW to KX022A_IPOL_HIGH, which is kind of a "easy to try" thing.
I don't have a strong preference here though.
--- drivers/iio/accel/kionix-kx022a.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/accel/kionix-kx022a.h +1 -1
@@ -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
Signature · 1 line
2.55.0
Re: [PATCH 12/12] iio: accel: kionix-kx022a: Fix IPOL macro name
Hi Matti,
Thank you for the patch!
Quoted 3 lines · Matti Vaittinen
From: Matti Vaittinen <[email protected]>
The "interrupt polarity high" -macro for KX022A variant is defined as:
-macro ?
defined as what ?
Quoted 4 lines
However, the
KX022A_MASK_IPOL1is not defined anywhere, so actually using theKX022A_IPOL_HIGHwould produce a compile error.Fix the define by using correct mask.
Quoted 13 lines
Signed-off-by Matti Vaittinen <[email protected]>Fixes 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")---
It appears theKX022A_IPOL_HIGHis unused as the IRQ is currently, unconditionally set level low inkx022a_prepare_irq_pin(). The KX022A hardware would support other configurations though. So, another potential fix would be removing the define altogether. I decided to leave the define there, as having it will allow users who need different configuration to simply change theKX022A_IPOL_LOWtoKX022A_IPOL_HIGH, which is kind of a "easy to try" thing.
Quoted 1 line
I don't have a strong preference here though.
This is up to Jonathan. Removing this also makes sense (and if you decide to do then probably do it for KX132 also)
--
Kind Regards
Mehdi Djait
Matti Vaittinen
maintainerRe: [PATCH 12/12] iio: accel: kionix-kx022a: Fix IPOL macro name
Thanks for all the reviews Mehdi. Much appreciated!
Quoted 10 lines · Mehdi Djait
Hi Matti,
Thank you for the patch!
Quoted 3 lines · Matti Vaittinen
From: Matti Vaittinen <[email protected]>
The "interrupt polarity high" -macro for KX022A variant is defined as:
-macro ?
Yes, the "interrupt polarity high"-macro ;) I should probably have used the proper name of the macro here ^_^;
Quoted 1 line
defined as what ?
as "KX022A_MASK_IPOL1". Seems like my brains moved to the next topic before hands finished writing this one :| Thanks for pointing it out :)
Quoted 7 lines
Quoted 4 lines
However, the
KX022A_MASK_IPOL1is not defined anywhere, so actually using theKX022A_IPOL_HIGHwould produce a compile error.Fix the define by using correct mask.
Reviewed-by Mehdi Djait <[email protected]>
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Jonathan Cameron
maintainerRe: [PATCH 12/12] iio: accel: kionix-kx022a: Fix IPOL macro name
In reply to Matti VaittinenQuoted 11 lines · On Mon, 10 Aug 2026 10:55:25 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The "interrupt polarity high" -macro for KX022A variant is defined as:
However, theKX022A_MASK_IPOL1is not defined anywhere, so actually using theKX022A_IPOL_HIGHwould produce a compile error.Fix the define by using correct mask.
Signed-off-by Matti Vaittinen <[email protected]>Fixes 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")
Quoted 8 lines
---
It appears theKX022A_IPOL_HIGHis unused as the IRQ is currently, unconditionally set level low inkx022a_prepare_irq_pin(). The KX022A hardware would support other configurations though. So, another potential fix would be removing the define altogether. I decided to leave the define there, as having it will allow users who need different configuration to simply change theKX022A_IPOL_LOWtoKX022A_IPOL_HIGH, which is kind of a "easy to try" thing.
Handling interrupt polarity options at the device end is typically a 'feature' rather than a must have given in most cases we don't have sharing going on and these days interrupt controllers can cope with most types of input. If anyone needs it they can propose support.
Note I've never been happy with how we do this when we do support it because the argument for letting DT set the polarity is there may be not gates or similar in the path so the driver can't know, yet if we read the type to set the polarity at the driver end we are saying we do know there aren't any such not gate (trick for level shifting on the cheap). I haven't figured out a better binding.
Jonathan
Quoted 19 lines
I don't have a strong preference here though.
--- drivers/iio/accel/kionix-kx022a.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/accel/kionix-kx022a.h +1
-1@@ -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
Matti Vaittinen
maintainerRe: [PATCH 12/12] iio: accel: kionix-kx022a: Fix IPOL macro name
Quoted 23 lines · Jonathan Cameron
Quoted 11 lines · On Mon, 10 Aug 2026 10:55:25 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The "interrupt polarity high" -macro for KX022A variant is defined as:
However, theKX022A_MASK_IPOL1is not defined anywhere, so actually using theKX022A_IPOL_HIGHwould produce a compile error.Fix the define by using correct mask.
Signed-off-by Matti Vaittinen <[email protected]>Fixes 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")Quoted 8 lines
---
It appears theKX022A_IPOL_HIGHis unused as the IRQ is currently, unconditionally set level low inkx022a_prepare_irq_pin(). The KX022A hardware would support other configurations though. So, another potential fix would be removing the define altogether. I decided to leave the define there, as having it will allow users who need different configuration to simply change theKX022A_IPOL_LOWtoKX022A_IPOL_HIGH, which is kind of a "easy to try" thing.
Quoted 4 lines
Handling interrupt polarity options at the device end is typically a 'feature' rather than a must have given in most cases we don't have sharing going on and these days interrupt controllers can cope with most types of input. If anyone needs it they can propose support.
I take this as a "please, drop the unused define rather than fix it".
Quoted 6 lines
Note I've never been happy with how we do this when we do support it because the argument for letting DT set the polarity is there may be not gates or similar in the path so the driver can't know, yet if we read the type to set the polarity at the driver end we are saying we do know there aren't any such not gate (trick for level shifting on the cheap). I haven't figured out a better binding.
I was more thinking of people who do downstream projects than a proper upstream support including nice mechanism to select appropriate polarity. It's quite easy to experiment (and carry a downstream patch) with one-line define change. Especially so when defines are already there.
Yours,
-- Matti
Signature · 5 lines
Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
Jonathan Cameron
maintainerRe: [PATCH 12/12] iio: accel: kionix-kx022a: Fix IPOL macro name
Quoted 24 lines · On Mon, 17 Aug 2026 14:43:18 +0300 Matti Vaittinen <[email protected]>…
Quoted 23 lines · Jonathan Cameron
Quoted 19 lines · On Mon, 10 Aug 2026 10:55:25 +0300 Matti Vaittinen <[email protected]>…
From: Matti Vaittinen <[email protected]>
The "interrupt polarity high" -macro for KX022A variant is defined as:
However, theKX022A_MASK_IPOL1is not defined anywhere, so actually using theKX022A_IPOL_HIGHwould produce a compile error.Fix the define by using correct mask.
Signed-off-by Matti Vaittinen <[email protected]>Fixes 7c1d1677b322 ("iio: accel: Support Kionix/ROHM KX022A accelerometer")---
It appears theKX022A_IPOL_HIGHis unused as the IRQ is currently, unconditionally set level low inkx022a_prepare_irq_pin(). The KX022A hardware would support other configurations though. So, another potential fix would be removing the define altogether. I decided to leave the define there, as having it will allow users who need different configuration to simply change theKX022A_IPOL_LOWtoKX022A_IPOL_HIGH, which is kind of a "easy to try" thing.
Quoted 6 lines
Quoted 4 lines
Handling interrupt polarity options at the device end is typically a 'feature' rather than a must have given in most cases we don't have sharing going on and these days interrupt controllers can cope with most types of input. If anyone needs it they can propose support.
I take this as a "please, drop the unused define rather than fix it".
Nope. I'm fine with fixing the define being there (fixed). This was more of a comment on whether we should 'fix' whether we ignored the flag from firmware or not.
Quoted 8 lines
Quoted 7 lines
Note I've never been happy with how we do this when we do support it because the argument for letting DT set the polarity is there may be not gates or similar in the path so the driver can't know, yet if we read the type to set the polarity at the driver end we are saying we do know there aren't any such not gate (trick for level shifting on the cheap). I haven't figured out a better binding.
Quoted 4 lines
I was more thinking of people who do downstream projects than a proper upstream support including nice mechanism to select appropriate polarity. It's quite easy to experiment (and carry a downstream patch) with one-line define change. Especially so when defines are already there.
I'd rather we support them properly if they actually need it. Ideal case they'd ask even for a downstream project and at that time we can add support - via the ugly path or maybe something better.
Jonathan
Quoted 4 lines
Yours,
-- Matti