* [PATCH v8 1/9] dt-bindings: iio: accel: mma8452: Add drive-open-drain
2026-09-07 14:50 [PATCH v8 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
@ 2026-09-07 14:50 ` Esben Haabendal
2026-09-07 14:50 ` [PATCH v8 2/9] iio: accel: mma8452: Fix use-after-free bug in error error path Esben Haabendal
` (7 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 14:50 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner
Cc: Esben Haabendal, linux-iio, devicetree, linux-kernel
Add new boolean to configure selected interrupt pin to open drain instead
of the default push-pull mode.
Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml b/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
index b0dd2b4e116a..20701aa725d0 100644
--- a/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
@@ -39,6 +39,12 @@ properties:
minItems: 1
maxItems: 2
+ drive-open-drain:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description: the interrupt line will be configured as open drain, which is
+ useful if several sensors share the same interrupt line. (This binding is
+ taken from pinctrl.)
+
vdd-supply: true
vddio-supply: true
--
2.55.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v8 2/9] iio: accel: mma8452: Fix use-after-free bug in error error path
2026-09-07 14:50 [PATCH v8 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
2026-09-07 14:50 ` [PATCH v8 1/9] dt-bindings: iio: accel: mma8452: Add drive-open-drain Esben Haabendal
@ 2026-09-07 14:50 ` Esben Haabendal
2026-09-07 15:04 ` sashiko-bot
2026-09-07 14:50 ` [PATCH v8 3/9] iio: accel: mma8452: Optimize struct mma8452_data member orders Esben Haabendal
` (6 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 14:50 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner
Cc: Esben Haabendal, linux-iio, devicetree, linux-kernel, stable,
Joshua Crofts
If mma8452_probe() fails in iio_device_register() or later, we could end up
with runtime suspend callback being called with a now freed device pointer.
Fixes: 96c0cb2bbfe0 ("iio: mma8452: add support for runtime power management")
Cc: stable@vger.kernel.org
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/iio/accel/mma8452.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index fe62a903f0e2..a937cbd84f30 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1681,7 +1681,7 @@ static int mma8452_probe(struct i2c_client *client)
ret = iio_device_register(indio_dev);
if (ret < 0)
- goto free_irq;
+ goto runtime_suspend;
ret = mma8452_set_freefall_mode(data, false);
if (ret < 0)
@@ -1692,6 +1692,10 @@ static int mma8452_probe(struct i2c_client *client)
unregister_device:
iio_device_unregister(indio_dev);
+runtime_suspend:
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+
free_irq:
if (client->irq)
free_irq(client->irq, indio_dev);
--
2.55.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 2/9] iio: accel: mma8452: Fix use-after-free bug in error error path
2026-09-07 14:50 ` [PATCH v8 2/9] iio: accel: mma8452: Fix use-after-free bug in error error path Esben Haabendal
@ 2026-09-07 15:04 ` sashiko-bot
2026-09-07 15:08 ` Esben Haabendal
0 siblings, 1 reply; 27+ messages in thread
From: sashiko-bot @ 2026-09-07 15:04 UTC (permalink / raw)
To: Esben Haabendal; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing runtime PM synchronization in mma8452_remove() leads to unbalanced regulator disables and I2C access on unpowered hardware.
- [High] AB-BA deadlock between data->lock and PM runtime suspend
- [High] PM runtime usage count leak on I2C read/write error paths.
- [Medium] Missing device standby transition in the probe error path leads to a power leak.
- [Medium] Unsafe hardware access during probe due to lack of runtime PM reference.
- [Low] The driver unconditionally assigns its dev_pm_ops structure to the driver.pm pointer without using the pm_ptr() macro.
--
commit 503a115bc0b7e45c93abce9002f8c8156306a6c0
Author: Esben Haabendal <esben@geanix.com>
iio: accel: mma8452: Fix use-after-free bug in error error path
If mma8452_probe() fails in iio_device_register() or later, we could end up
with runtime suspend callback being called with a now freed device pointer.
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index fe62a903f0e22..a937cbd84f300 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
[ ... ]
> static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
> {
> int ret = mma8452_drdy(data);
>
> if (ret < 0)
> return ret;
>
> ret = mma8452_set_runtime_pm_state(data->client, true);
> if (ret)
> return ret;
>
> ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
> 3 * sizeof(__be16), (u8 *)buf);
> if (ret < 0)
> return ret;
[Severity: High]
This is a pre-existing issue, but does this code permanently leak the PM usage
count on I2C read error paths?
If pm_runtime_resume_and_get() increments the usage count via
mma8452_set_runtime_pm_state(), but the subsequent i2c_smbus_read_i2c_block_data()
fails, this returns early without calling pm_runtime_put_autosuspend().
A similar leak appears to exist in mma8452_write_event_config():
drivers/iio/accel/mma8452.c:mma8452_write_event_config() {
...
ret = mma8452_set_runtime_pm_state(data->client, state);
if (ret)
return ret;
switch (dir) {
...
case IIO_EV_DIR_RISING:
val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
if (val < 0)
return val;
...
}
Does this permanently break power management for the device on any transient I2C
bus error?
[ ... ]
> static int mma8452_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> {
> struct mma8452_data *data = iio_priv(indio_dev);
> __be16 buffer[3];
> int i, ret;
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW: {
> IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> if (IIO_DEV_ACQUIRE_FAILED(claim))
> return -EBUSY;
>
> guard(mutex)(&data->lock);
>
> ret = mma8452_read(data, buffer);
[Severity: High]
This isn't a bug introduced by this patch, but does this create an AB-BA
deadlock between data->lock and PM runtime suspend?
If a userspace read via mma8452_read_raw() acquires data->lock and then calls
pm_runtime_resume_and_get() (inside mma8452_read()), the PM core will wait if a
suspend is in progress.
Concurrently, the autosuspend worker might execute mma8452_runtime_suspend()
and block waiting to acquire data->lock:
drivers/iio/accel/mma8452.c:mma8452_runtime_suspend() {
...
scoped_guard(mutex, &data->lock)
ret = mma8452_standby(data);
...
}
Could unprivileged userspace trigger a deadlock by repeatedly reading raw data
while the autosuspend timer expires?
[ ... ]
> @@ -1681,7 +1681,7 @@ static int mma8452_probe(struct i2c_client *client)
>
> ret = iio_device_register(indio_dev);
> if (ret < 0)
> - goto free_irq;
> + goto runtime_suspend;
>
> ret = mma8452_set_freefall_mode(data, false);
[Severity: Medium]
This is a pre-existing issue, but does this allow unsafe hardware access during
probe due to lack of a runtime PM reference?
Because probe does not hold a runtime PM reference (usage count is 0) after
iio_device_register(), userspace or a concurrent PM transition could suspend
the device. This might turn off regulators before mma8452_set_freefall_mode()
performs I2C access, resulting in a bus error.
> if (ret < 0)
> goto unregister_device;
>
> return 0;
>
> unregister_device:
> iio_device_unregister(indio_dev);
>
> +runtime_suspend:
> + pm_runtime_disable(dev);
> + pm_runtime_set_suspended(dev);
[Severity: Medium]
This is a pre-existing issue, but does the error path miss a device standby
transition, leading to a power leak?
If probe fails after iio_device_register() (e.g., mma8452_set_freefall_mode()
fails), the device is left in the ACTIVE state. The error path disables
regulators below, but if they are shared with other active devices, the
regulators stay on.
Since mma8452_standby() is not called here, could the device remain active and
continue to draw power indefinitely?
[ ... ]
> static void mma8452_remove(struct i2c_client *client)
> {
> struct iio_dev *indio_dev = i2c_get_clientdata(client);
> struct mma8452_data *data = iio_priv(indio_dev);
> struct device *dev = &client->dev;
>
> iio_device_unregister(indio_dev);
>
> pm_runtime_disable(dev);
> pm_runtime_set_suspended(dev);
>
> if (client->irq)
> free_irq(client->irq, indio_dev);
>
> iio_triggered_buffer_cleanup(indio_dev);
> mma8452_trigger_cleanup(indio_dev);
> mma8452_standby(iio_priv(indio_dev));
>
> regulator_bulk_disable(ARRAY_SIZE(data->regs), data->regs);
[Severity: High]
This is a pre-existing issue, but does missing runtime PM synchronization
in mma8452_remove() lead to unbalanced regulator disables and I2C access on
unpowered hardware?
If the device is runtime-suspended during module removal, its regulators are
already disabled. pm_runtime_disable() does not resume the device, so
mma8452_standby() attempts an I2C write on unpowered hardware, causing bus
errors.
Additionally, regulator_bulk_disable() would be called again on an already
disabled regulator, which triggers a kernel warning.
[ ... ]
> static struct i2c_driver mma8452_driver = {
> .driver = {
> .name = "mma8452",
> .of_match_table = mma8452_dt_ids,
> .pm = &mma8452_pm_ops,
[Severity: Low]
This isn't a bug introduced by this patch, but should this assignment use
the pm_ptr() macro?
The dev_pm_ops structure is unconditionally assigned here. When CONFIG_PM is
disabled, the PM ops structure will still be referenced, creating dead code
and violating the PM subsystem conditional compilation guidelines.
> },
> .probe = mma8452_probe,
> .remove = mma8452_remove,
> .id_table = mma8452_id,
> };
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-mma8452-open-drain-v8-0-c17407e22118@geanix.com?part=2
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 2/9] iio: accel: mma8452: Fix use-after-free bug in error error path
2026-09-07 15:04 ` sashiko-bot
@ 2026-09-07 15:08 ` Esben Haabendal
0 siblings, 0 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 15:08 UTC (permalink / raw)
To: sashiko-reviews; +Cc: conor+dt, devicetree, robh
<sashiko-bot@kernel.org> writes:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Missing runtime PM synchronization in mma8452_remove() leads to unbalanced regulator disables and I2C access on unpowered hardware.
> - [High] AB-BA deadlock between data->lock and PM runtime suspend
> - [High] PM runtime usage count leak on I2C read/write error paths.
> - [Medium] Missing device standby transition in the probe error path leads to a power leak.
> - [Medium] Unsafe hardware access during probe due to lack of runtime PM reference.
> - [Low] The driver unconditionally assigns its dev_pm_ops structure to the driver.pm pointer without using the pm_ptr() macro.
I believe all of this should be addressed in the next series I will send
with changes to this driver. I will not be adding them to this series.
/Esben
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 3/9] iio: accel: mma8452: Optimize struct mma8452_data member orders
2026-09-07 14:50 [PATCH v8 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
2026-09-07 14:50 ` [PATCH v8 1/9] dt-bindings: iio: accel: mma8452: Add drive-open-drain Esben Haabendal
2026-09-07 14:50 ` [PATCH v8 2/9] iio: accel: mma8452: Fix use-after-free bug in error error path Esben Haabendal
@ 2026-09-07 14:50 ` Esben Haabendal
2026-09-07 14:50 ` [PATCH v8 4/9] iio: accel: mma8452: Only apply trigger type when not set by firmware Esben Haabendal
` (5 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 14:50 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner
Cc: Esben Haabendal, linux-iio, devicetree, linux-kernel,
Joshua Crofts, Andy Shevchenko
Reorder struct mma8452_data members to avoid holes.
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/iio/accel/mma8452.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index a937cbd84f30..6c5a8d65c8a4 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -108,10 +108,7 @@ struct mma8452_data {
struct i2c_client *client;
struct mutex lock;
struct iio_mount_matrix orientation;
- u8 ctrl_reg1;
- u8 data_cfg;
const struct mma_chip_info *chip_info;
- int sleep_val;
struct regulator_bulk_data regs[2];
/* Ensure correct alignment of time stamp when present */
@@ -119,6 +116,10 @@ struct mma8452_data {
__be16 channels[3];
aligned_s64 ts;
} buffer;
+
+ int sleep_val;
+ u8 ctrl_reg1;
+ u8 data_cfg;
};
/**
--
2.55.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v8 4/9] iio: accel: mma8452: Only apply trigger type when not set by firmware
2026-09-07 14:50 [PATCH v8 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
` (2 preceding siblings ...)
2026-09-07 14:50 ` [PATCH v8 3/9] iio: accel: mma8452: Optimize struct mma8452_data member orders Esben Haabendal
@ 2026-09-07 14:50 ` Esben Haabendal
2026-09-07 15:01 ` sashiko-bot
2026-09-07 14:51 ` [PATCH v8 5/9] iio: accel: mma8452: Fix unintended comment indent Esben Haabendal
` (4 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 14:50 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner
Cc: Esben Haabendal, linux-iio, devicetree, linux-kernel,
Andy Shevchenko
Instead of unconditionally overriding the trigger type, it is better to
only apply a default when no trigger type is set by firmware. This should
be reasonably backward compatible, and should only potentially cause
problems if systems exist where firmware specifies an incorrect trigger
type. With a bit of luck, there are no such systems.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/iio/accel/mma8452.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 6c5a8d65c8a4..05851274596e 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1665,9 +1665,16 @@ static int mma8452_probe(struct i2c_client *client)
goto trigger_cleanup;
if (client->irq) {
+ unsigned long irq_flags;
+
+ irq_flags = irq_get_trigger_type(client->irq);
+ if (irq_flags == IRQ_TYPE_NONE) {
+ dev_info(dev, "invalid irq type, setting default active low\n");
+ irq_flags = IRQF_TRIGGER_LOW;
+ }
+ irq_flags |= IRQF_ONESHOT;
ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
- IRQF_TRIGGER_LOW | IRQF_ONESHOT,
- client->name, indio_dev);
+ irq_flags, client->name, indio_dev);
if (ret)
goto buffer_cleanup;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 4/9] iio: accel: mma8452: Only apply trigger type when not set by firmware
2026-09-07 14:50 ` [PATCH v8 4/9] iio: accel: mma8452: Only apply trigger type when not set by firmware Esben Haabendal
@ 2026-09-07 15:01 ` sashiko-bot
0 siblings, 0 replies; 27+ messages in thread
From: sashiko-bot @ 2026-09-07 15:01 UTC (permalink / raw)
To: Esben Haabendal; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware configuration mismatch for active-high interrupts
--
commit cc23812be1f638568d4c39e30f713017f6ba0b47
Author: Esben Haabendal <esben@geanix.com>
iio: accel: mma8452: Only apply trigger type when not set by firmware
Instead of unconditionally overriding the trigger type, it is better to
only apply a default when no trigger type is set by firmware. This should
be reasonably backward compatible, and should only potentially cause
problems if systems exist where firmware specifies an incorrect trigger
type. With a bit of luck, there are no such systems.
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 6c5a8d65c8a42..05851274596e5 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1665,9 +1665,16 @@ static int mma8452_probe(struct i2c_client *client)
> goto trigger_cleanup;
>
> if (client->irq) {
> + unsigned long irq_flags;
> +
> + irq_flags = irq_get_trigger_type(client->irq);
> + if (irq_flags == IRQ_TYPE_NONE) {
> + dev_info(dev, "invalid irq type, setting default active low\n");
> + irq_flags = IRQF_TRIGGER_LOW;
> + }
> + irq_flags |= IRQF_ONESHOT;
> ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
> - IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> - client->name, indio_dev);
> + irq_flags, client->name, indio_dev);
[Severity: High]
Will this cause a hardware configuration mismatch if the firmware specifies
an active-high or edge-rising interrupt?
In mma8452_probe(), when the firmware specifies an active-high interrupt
(e.g., IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_EDGE_RISING), this code passes the
active-high flag to request_threaded_irq() and configures the host
interrupt controller accordingly.
However, the MMA8452 sensor hardware defaults to an active-low output.
Without explicitly configuring the IPOL (Interrupt Polarity) bit in the
sensor's CTRL_REG3 register (0x2C) to match the requested firmware trigger
type, the sensor will continue driving an active-low signal.
This mismatch will break interrupts for systems that correctly specify an
active-high trigger.
It may also regress systems with incorrectly specified active-high device
trees that previously worked because the driver hardcoded IRQF_TRIGGER_LOW.
> if (ret)
> goto buffer_cleanup;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-mma8452-open-drain-v8-0-c17407e22118@geanix.com?part=4
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 5/9] iio: accel: mma8452: Fix unintended comment indent
2026-09-07 14:50 [PATCH v8 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
` (3 preceding siblings ...)
2026-09-07 14:50 ` [PATCH v8 4/9] iio: accel: mma8452: Only apply trigger type when not set by firmware Esben Haabendal
@ 2026-09-07 14:51 ` Esben Haabendal
2026-09-07 15:04 ` Joshua Crofts
2026-09-07 14:51 ` [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data Esben Haabendal
` (3 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 14:51 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner
Cc: Esben Haabendal, linux-iio, devicetree, linux-kernel
The extra space this block was indented with looks quite untraditional, and
does not align with the common style used in the kernel.
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/iio/accel/mma8452.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 05851274596e..c6af68d2a297 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -122,21 +122,21 @@ struct mma8452_data {
u8 data_cfg;
};
- /**
- * struct mma8452_event_regs - chip specific data related to events
- * @ev_cfg: event config register address
- * @ev_cfg_ele: latch bit in event config register
- * @ev_cfg_chan_shift: number of the bit to enable events in X
- * direction; in event config register
- * @ev_src: event source register address
- * @ev_ths: event threshold register address
- * @ev_ths_mask: mask for the threshold value
- * @ev_count: event count (period) register address
- *
- * Since not all chips supported by the driver support comparing high pass
- * filtered data for events (interrupts), different interrupt sources are
- * used for different chips and the relevant registers are included here.
- */
+/**
+ * struct mma8452_event_regs - chip specific data related to events
+ * @ev_cfg: event config register address
+ * @ev_cfg_ele: latch bit in event config register
+ * @ev_cfg_chan_shift: number of the bit to enable events in X
+ * direction; in event config register
+ * @ev_src: event source register address
+ * @ev_ths: event threshold register address
+ * @ev_ths_mask: mask for the threshold value
+ * @ev_count: event count (period) register address
+ *
+ * Since not all chips supported by the driver support comparing high pass
+ * filtered data for events (interrupts), different interrupt sources are
+ * used for different chips and the relevant registers are included here.
+ */
struct mma8452_event_regs {
u8 ev_cfg;
u8 ev_cfg_ele;
--
2.55.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 5/9] iio: accel: mma8452: Fix unintended comment indent
2026-09-07 14:51 ` [PATCH v8 5/9] iio: accel: mma8452: Fix unintended comment indent Esben Haabendal
@ 2026-09-07 15:04 ` Joshua Crofts
0 siblings, 0 replies; 27+ messages in thread
From: Joshua Crofts @ 2026-09-07 15:04 UTC (permalink / raw)
To: Esben Haabendal
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner, linux-iio, devicetree,
linux-kernel
On Mon, 07 Sep 2026 16:51:00 +0200
Esben Haabendal <esben@geanix.com> wrote:
> The extra space this block was indented with looks quite untraditional, and
> does not align with the common style used in the kernel.
>
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
--
Kind regards,
Joshua Crofts
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data
2026-09-07 14:50 [PATCH v8 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
` (4 preceding siblings ...)
2026-09-07 14:51 ` [PATCH v8 5/9] iio: accel: mma8452: Fix unintended comment indent Esben Haabendal
@ 2026-09-07 14:51 ` Esben Haabendal
2026-09-07 14:58 ` sashiko-bot
` (2 more replies)
2026-09-07 14:51 ` [PATCH v8 7/9] iio: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
` (2 subsequent siblings)
8 siblings, 3 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 14:51 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner
Cc: Esben Haabendal, linux-iio, devicetree, linux-kernel
The struct mma8452_data is central for this driver, and it makes sense to
have a description of the fields in it to make it easier to work with the
driver.
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/iio/accel/mma8452.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index c6af68d2a297..2c1b97b77bc1 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -104,6 +104,22 @@
#define MMA8452_AUTO_SUSPEND_DELAY_MS 2000
+/**
+ * struct mma8452_data - IIO device private data structure
+ * @client: the I2C client object
+ * @lock: mutex for synchronziation of register
+ * read-modify-write and holding chip in STANDBY
+ * mode while writing to registers
+ * @orientation: mounting matrix, flipped axis etc
+ * @chip_info: chip specific data
+ * @vdd_reg: reference to VDD regulator
+ * @vddio_reg: reference to VDDIO regulator
+ * @buffer: triggered buffer
+ * @sleep_val: time in ms to sleep while waiting for drdy
+ * @ctrl_reg1: CTRL_REG1 register shadow value
+ * @data_cfg: DATA_CFG register shadow value
+ * @open_drain: true for irq pin in open-drain mode
+ */
struct mma8452_data {
struct i2c_client *client;
struct mutex lock;
--
2.55.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data
2026-09-07 14:51 ` [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data Esben Haabendal
@ 2026-09-07 14:58 ` sashiko-bot
2026-09-07 15:07 ` Esben Haabendal
2026-09-07 15:14 ` Joshua Crofts
2026-09-08 10:36 ` Andy Shevchenko
2 siblings, 1 reply; 27+ messages in thread
From: sashiko-bot @ 2026-09-07 14:58 UTC (permalink / raw)
To: Esben Haabendal; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly added kernel-doc block for `struct mma8452_data` documents non-existent fields (`@vdd_reg` and `@vddio_reg`) and omits the existing field `regs`, leading to kernel-doc validation warnings.
--
commit 9d34500f2316c8f0c0c1541db98718ec0f302931
Author: Esben Haabendal <esben@geanix.com>
iio: accel: mma8452: Add comment block for struct mma8452_data
The struct mma8452_data is central for this driver, and it makes sense to
have a description of the fields in it to make it easier to work with the
driver.
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index c6af68d2a297a..2c1b97b77bc19 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -104,6 +104,22 @@
>
> #define MMA8452_AUTO_SUSPEND_DELAY_MS 2000
>
> +/**
> + * struct mma8452_data - IIO device private data structure
> + * @client: the I2C client object
> + * @lock: mutex for synchronziation of register
> + * read-modify-write and holding chip in STANDBY
> + * mode while writing to registers
> + * @orientation: mounting matrix, flipped axis etc
> + * @chip_info: chip specific data
> + * @vdd_reg: reference to VDD regulator
> + * @vddio_reg: reference to VDDIO regulator
[Severity: Low]
Does this kernel-doc block accurately reflect the struct definition?
The newly added documentation lists @vdd_reg and @vddio_reg, but these
fields do not actually exist in struct mma8452_data.
This mismatch will trigger kernel-doc validation warnings when building
with W=1 or running kernel-doc tooling. Note that while a later commit
in this series adds the missing @open_drain field, this @vdd_reg and
@vddio_reg mismatch appears to remain unresolved.
> + * @buffer: triggered buffer
> + * @sleep_val: time in ms to sleep while waiting for drdy
> + * @ctrl_reg1: CTRL_REG1 register shadow value
> + * @data_cfg: DATA_CFG register shadow value
> + * @open_drain: true for irq pin in open-drain mode
> + */
> struct mma8452_data {
> struct i2c_client *client;
> struct mutex lock;
[Severity: Low]
Is an existing field missing from this documentation block?
The actual struct mma8452_data contains the following field which is
completely omitted from the newly added kernel-doc:
struct regulator_bulk_data regs[2];
Like the non-existent fields above, omitting the documentation for the
existing regs field will result in kernel-doc validation warnings.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-mma8452-open-drain-v8-0-c17407e22118@geanix.com?part=6
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data
2026-09-07 14:58 ` sashiko-bot
@ 2026-09-07 15:07 ` Esben Haabendal
0 siblings, 0 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 15:07 UTC (permalink / raw)
To: sashiko-reviews; +Cc: robh, conor+dt, devicetree
<sashiko-bot@kernel.org> writes:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] The newly added kernel-doc block for `struct mma8452_data` documents
> non-existent fields (`@vdd_reg` and `@vddio_reg`) and omits the existing field
> `regs`, leading to kernel-doc validation warnings.
Argh, I missed that when rebasing from v7.3-rc1 to the togreg branch.
I will fix it for next version. So much for hoping this was the version
that actually got merged.
/Esben
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data
2026-09-07 14:51 ` [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data Esben Haabendal
2026-09-07 14:58 ` sashiko-bot
@ 2026-09-07 15:14 ` Joshua Crofts
2026-09-07 16:28 ` Esben Haabendal
2026-09-08 10:36 ` Andy Shevchenko
2 siblings, 1 reply; 27+ messages in thread
From: Joshua Crofts @ 2026-09-07 15:14 UTC (permalink / raw)
To: Esben Haabendal
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner, linux-iio, devicetree,
linux-kernel
On Mon, 07 Sep 2026 16:51:01 +0200
Esben Haabendal <esben@geanix.com> wrote:
> The struct mma8452_data is central for this driver, and it makes sense to
> have a description of the fields in it to make it easier to work with the
> driver.
>
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
> drivers/iio/accel/mma8452.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index c6af68d2a297..2c1b97b77bc1 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -104,6 +104,22 @@
>
> #define MMA8452_AUTO_SUSPEND_DELAY_MS 2000
>
> +/**
> + * struct mma8452_data - IIO device private data structure
> + * @client: the I2C client object
> + * @lock: mutex for synchronziation of register
> + * read-modify-write and holding chip in STANDBY
> + * mode while writing to registers
Perhaps add the word sequences after read-modify-write (that
could be a personal preference though)?
> + * @orientation: mounting matrix, flipped axis etc
Missing period in etc., but that's a small enough nit :)
> + * @chip_info: chip specific data
> + * @vdd_reg: reference to VDD regulator
> + * @vddio_reg: reference to VDDIO regulator
> + * @buffer: triggered buffer
> + * @sleep_val: time in ms to sleep while waiting for drdy
> + * @ctrl_reg1: CTRL_REG1 register shadow value
> + * @data_cfg: DATA_CFG register shadow value
> + * @open_drain: true for irq pin in open-drain mode
> + */
> struct mma8452_data {
> struct i2c_client *client;
> struct mutex lock;
>
Either way, with those added in a new version or fixed up
by Jonathan,
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
--
Kind regards,
Joshua Crofts
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data
2026-09-07 15:14 ` Joshua Crofts
@ 2026-09-07 16:28 ` Esben Haabendal
0 siblings, 0 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 16:28 UTC (permalink / raw)
To: Joshua Crofts
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner, linux-iio, devicetree,
linux-kernel
"Joshua Crofts" <joshua.crofts1@gmail.com> writes:
> On Mon, 07 Sep 2026 16:51:01 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> The struct mma8452_data is central for this driver, and it makes sense to
>> have a description of the fields in it to make it easier to work with the
>> driver.
>>
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>> drivers/iio/accel/mma8452.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index c6af68d2a297..2c1b97b77bc1 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>> @@ -104,6 +104,22 @@
>>
>> #define MMA8452_AUTO_SUSPEND_DELAY_MS 2000
>>
>> +/**
>> + * struct mma8452_data - IIO device private data structure
>> + * @client: the I2C client object
>> + * @lock: mutex for synchronziation of register
>> + * read-modify-write and holding chip in STANDBY
>> + * mode while writing to registers
>
> Perhaps add the word sequences after read-modify-write (that
> could be a personal preference though)?
Added for next version.
>> + * @orientation: mounting matrix, flipped axis etc
>
> Missing period in etc., but that's a small enough nit :)
Also added :)
>> + * @chip_info: chip specific data
>> + * @vdd_reg: reference to VDD regulator
>> + * @vddio_reg: reference to VDDIO regulator
>> + * @buffer: triggered buffer
>> + * @sleep_val: time in ms to sleep while waiting for drdy
>> + * @ctrl_reg1: CTRL_REG1 register shadow value
>> + * @data_cfg: DATA_CFG register shadow value
>> + * @open_drain: true for irq pin in open-drain mode
>> + */
>> struct mma8452_data {
>> struct i2c_client *client;
>> struct mutex lock;
>>
>
> Either way, with those added in a new version or fixed up
> by Jonathan,
>
> Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Thanks.
/Esben
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data
2026-09-07 14:51 ` [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data Esben Haabendal
2026-09-07 14:58 ` sashiko-bot
2026-09-07 15:14 ` Joshua Crofts
@ 2026-09-08 10:36 ` Andy Shevchenko
2 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2026-09-08 10:36 UTC (permalink / raw)
To: Esben Haabendal
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner, linux-iio, devicetree,
linux-kernel
On Mon, Sep 07, 2026 at 04:51:01PM +0200, Esben Haabendal wrote:
> The struct mma8452_data is central for this driver, and it makes sense to
> have a description of the fields in it to make it easier to work with the
> driver.
...
> +/**
> + * struct mma8452_data - IIO device private data structure
> + * @client: the I2C client object
> + * @lock: mutex for synchronziation of register
> + * read-modify-write and holding chip in STANDBY
> + * mode while writing to registers
> + * @orientation: mounting matrix, flipped axis etc
> + * @chip_info: chip specific data
> + * @vdd_reg: reference to VDD regulator
> + * @vddio_reg: reference to VDDIO regulator
> + * @buffer: triggered buffer
> + * @sleep_val: time in ms to sleep while waiting for drdy
> + * @ctrl_reg1: CTRL_REG1 register shadow value
> + * @data_cfg: DATA_CFG register shadow value
> + * @open_drain: true for irq pin in open-drain mode
> + */
Have you tried to generate *.html and *.pdf out of this? Does rendering look
okay in both cases?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 7/9] iio: accel: mma8452: Allow open drain interrupt pin configuration
2026-09-07 14:50 [PATCH v8 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
` (5 preceding siblings ...)
2026-09-07 14:51 ` [PATCH v8 6/9] iio: accel: mma8452: Add comment block for struct mma8452_data Esben Haabendal
@ 2026-09-07 14:51 ` Esben Haabendal
2026-09-07 15:05 ` sashiko-bot
2026-09-07 14:51 ` [PATCH v8 8/9] iio: accel: mma8452: Use proper error code when missing device model Esben Haabendal
2026-09-07 14:51 ` [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing Esben Haabendal
8 siblings, 1 reply; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 14:51 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner
Cc: Esben Haabendal, linux-iio, devicetree, linux-kernel,
Andy Shevchenko
When designing systems sharing the interrupt for mma8452 chips, it is
helpful to be able to configure the irq pin in open-drain mode (default is
push-pull).
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/iio/accel/mma8452.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 2c1b97b77bc1..b5ab85a86e72 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -83,6 +83,8 @@
#define MMA8452_CTRL_REG2_RST BIT(6)
#define MMA8452_CTRL_REG2_MODS_SHIFT 3
#define MMA8452_CTRL_REG2_MODS_MASK 0x1b
+#define MMA8452_CTRL_REG3 0x2c
+#define MMA8452_CTRL_REG3_PP_OD BIT(0)
#define MMA8452_CTRL_REG4 0x2d
#define MMA8452_CTRL_REG5 0x2e
#define MMA8452_OFF_X 0x2f
@@ -136,6 +138,7 @@ struct mma8452_data {
int sleep_val;
u8 ctrl_reg1;
u8 data_cfg;
+ bool open_drain;
};
/**
@@ -659,6 +662,22 @@ static int mma8452_set_power_mode(struct mma8452_data *data, u8 mode)
return mma8452_change_config(data, MMA8452_CTRL_REG2, reg);
}
+static int mma8452_set_interrupt_pin_mode(struct mma8452_data *data)
+{
+ int reg;
+
+ reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG3);
+ if (reg < 0)
+ return reg;
+
+ if (data->open_drain)
+ reg |= MMA8452_CTRL_REG3_PP_OD;
+ else
+ reg &= ~MMA8452_CTRL_REG3_PP_OD;
+
+ return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG3, reg);
+}
+
/* returns >0 if in freefall mode, 0 if not or <0 if an error occurred */
static int mma8452_freefall_mode_enabled(struct mma8452_data *data)
{
@@ -1665,6 +1684,11 @@ static int mma8452_probe(struct i2c_client *client)
goto disable_regulators;
}
+ data->open_drain = device_property_read_bool(dev, "drive-open-drain");
+ ret = mma8452_set_interrupt_pin_mode(data);
+ if (ret)
+ goto trigger_cleanup;
+
data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
@@ -1792,6 +1816,10 @@ static int mma8452_runtime_resume(struct device *dev)
return ret;
}
+ ret = mma8452_set_interrupt_pin_mode(data);
+ if (ret)
+ goto runtime_resume_failed;
+
ret = mma8452_active(data);
if (ret < 0)
goto runtime_resume_failed;
--
2.55.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 7/9] iio: accel: mma8452: Allow open drain interrupt pin configuration
2026-09-07 14:51 ` [PATCH v8 7/9] iio: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
@ 2026-09-07 15:05 ` sashiko-bot
2026-09-07 15:09 ` Esben Haabendal
0 siblings, 1 reply; 27+ messages in thread
From: sashiko-bot @ 2026-09-07 15:05 UTC (permalink / raw)
To: Esben Haabendal; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Hardware configuration context is lost across runtime suspend/resume cycles involving power loss.
- [High] Raw reads fail when the device is idle due to waiting for data ready before resuming the device.
--
commit 74793c7cf1159fd96130d4ae4c8de1b7780ff4b3
Author: Esben Haabendal <esben@geanix.com>
iio: accel: mma8452: Allow open drain interrupt pin configuration
When designing systems sharing the interrupt for mma8452 chips, it is
helpful to be able to configure the irq pin in open-drain mode (default is
push-pull).
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 2c1b97b77bc19..b5ab85a86e72a 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
[ ... ]
> @@ -1792,6 +1816,10 @@ static int mma8452_runtime_resume(struct device *dev)
> return ret;
> }
>
> + ret = mma8452_set_interrupt_pin_mode(data);
> + if (ret)
> + goto runtime_resume_failed;
> +
> ret = mma8452_active(data);
[Severity: High]
This isn't a bug introduced by this patch, but does mma8452_runtime_resume()
fail to restore the hardware configuration after regulators are turned off
during suspend?
The device autosuspends after 2 seconds of inactivity, which disables the
VDD/VDDIO regulators via regulator_bulk_disable(). When it resumes,
mma8452_runtime_resume() appears to only restore CTRL_REG1 (via
mma8452_active()), and now CTRL_REG3, permanently dropping the rest of the
configuration.
Will the sensor lose settings like full-scale range (DATA_CFG), interrupt
routing (CTRL_REG5), and threshold configurations (TRANSIENT_THS) since
they revert to factory defaults upon power loss?
[ ... ]
While reviewing this driver, I noticed another pre-existing issue:
drivers/iio/accel/mma8452.c:mma8452_read() {
...
int ret = mma8452_drdy(data);
if (ret < 0)
return ret;
ret = mma8452_set_runtime_pm_state(data->client, true);
...
}
[Severity: High]
This is a pre-existing issue, but will raw sysfs reads fail when the device
is idle?
The mma8452_set_runtime_pm_state() call to wake the device is placed after
the polling loop for data ready in mma8452_drdy().
If the device has been idle for 2 seconds and entered autosuspend, the
chip is in STANDBY (or completely unpowered). Because of this, the Data
Ready (DRDY) bit is never set. The function times out after polling and
aborts the read before it ever attempts to wake up the device.
Should the device be resumed before polling for data ready?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-mma8452-open-drain-v8-0-c17407e22118@geanix.com?part=7
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 7/9] iio: accel: mma8452: Allow open drain interrupt pin configuration
2026-09-07 15:05 ` sashiko-bot
@ 2026-09-07 15:09 ` Esben Haabendal
0 siblings, 0 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 15:09 UTC (permalink / raw)
To: sashiko-reviews; +Cc: conor+dt, devicetree, robh
<sashiko-bot@kernel.org> writes:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Hardware configuration context is lost across runtime suspend/resume cycles involving power loss.
> - [High] Raw reads fail when the device is idle due to waiting for data ready before resuming the device.
Adressed in the next series for this driver. I will finish it up and
send it when this series is merged.
/Esben
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v8 8/9] iio: accel: mma8452: Use proper error code when missing device model
2026-09-07 14:50 [PATCH v8 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
` (6 preceding siblings ...)
2026-09-07 14:51 ` [PATCH v8 7/9] iio: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
@ 2026-09-07 14:51 ` Esben Haabendal
2026-09-07 14:51 ` [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing Esben Haabendal
8 siblings, 0 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 14:51 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner
Cc: Esben Haabendal, linux-iio, devicetree, linux-kernel,
Joshua Crofts, Andy Shevchenko
Switch -ENODEV error on i2c_get_match_data() failure to -ENODATA to
satisfy the IIO coding style.
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/iio/accel/mma8452.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index b5ab85a86e72..fda29df5d109 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1591,7 +1591,7 @@ static int mma8452_probe(struct i2c_client *client)
data->chip_info = i2c_get_match_data(client);
if (!data->chip_info)
- return dev_err_probe(dev, -ENODEV, "unknown device model\n");
+ return dev_err_probe(dev, -ENODATA, "unknown device model\n");
ret = iio_read_mount_matrix(dev, &data->orientation);
if (ret)
--
2.55.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing
2026-09-07 14:50 [PATCH v8 0/9] io: accel: mma8452: Allow open drain interrupt pin configuration Esben Haabendal
` (7 preceding siblings ...)
2026-09-07 14:51 ` [PATCH v8 8/9] iio: accel: mma8452: Use proper error code when missing device model Esben Haabendal
@ 2026-09-07 14:51 ` Esben Haabendal
2026-09-07 15:08 ` sashiko-bot
2026-09-07 15:10 ` Joshua Crofts
8 siblings, 2 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 14:51 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, Christoph Muellner
Cc: Esben Haabendal, linux-iio, devicetree, linux-kernel
Adding support for sharing interrupt line with other device requires the
interrupt handler to handle runtime PM suspension properly, ignoring the
irq if the device is suspended (maybe even off). And while at it, we use
the PM reference to ensure we do not get suspended while processing an irq.
In order to prevent the chip from raising irq while suspended (that is when
using fixed regulator, where suspend just means setting the device in
STANDBY mode), we disable all interrupt sources by clearing CTRL_REG4, and
then restores the value again when resuming.
With that in place, it is safe to add the IRQF_SHARED flag.
Keep in mind that the device by default is using push-pull for the irq pin,
which might require additional hardware design to allow interrupt sharing.
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
drivers/iio/accel/mma8452.c | 67 +++++++++++++++++++++++++++++++++++++++------
1 file changed, 58 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index fda29df5d109..e521dca37f76 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -120,6 +120,7 @@
* @sleep_val: time in ms to sleep while waiting for drdy
* @ctrl_reg1: CTRL_REG1 register shadow value
* @data_cfg: DATA_CFG register shadow value
+ * @ctrl_reg4: CTRL_REG4 register value to restore on resume
* @open_drain: true for irq pin in open-drain mode
*/
struct mma8452_data {
@@ -138,6 +139,7 @@ struct mma8452_data {
int sleep_val;
u8 ctrl_reg1;
u8 data_cfg;
+ u8 ctrl_reg4;
bool open_drain;
};
@@ -1083,15 +1085,21 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
{
struct iio_dev *indio_dev = p;
struct mma8452_data *data = iio_priv(indio_dev);
+ struct device *dev = &data->client->dev;
irqreturn_t ret = IRQ_NONE;
+ int pm_status;
int src;
+ pm_status = pm_runtime_get_if_active(dev);
+ if (pm_status == 0)
+ return IRQ_NONE; /* device is powered down */
+
src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
if (src < 0)
- return IRQ_NONE;
+ goto out_runtime_put;
if (!(src & (data->chip_info->enabled_events | MMA8452_INT_DRDY)))
- return IRQ_NONE;
+ goto out_runtime_put;
if (src & MMA8452_INT_DRDY) {
iio_trigger_poll_nested(indio_dev->trig);
@@ -1117,6 +1125,10 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
ret = IRQ_HANDLED;
}
+out_runtime_put:
+ if (pm_status > 0)
+ pm_runtime_put_autosuspend(dev);
+
return ret;
}
@@ -1712,7 +1724,7 @@ static int mma8452_probe(struct i2c_client *client)
dev_info(dev, "invalid irq type, setting default active low\n");
irq_flags = IRQF_TRIGGER_LOW;
}
- irq_flags |= IRQF_ONESHOT;
+ irq_flags |= IRQF_ONESHOT | IRQF_SHARED;
ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
irq_flags, client->name, indio_dev);
if (ret)
@@ -1784,29 +1796,62 @@ static void mma8452_remove(struct i2c_client *client)
#ifdef CONFIG_PM
static int mma8452_runtime_suspend(struct device *dev)
{
- struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct i2c_client *client = to_i2c_client(dev);
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
struct mma8452_data *data = iio_priv(indio_dev);
int ret;
- scoped_guard(mutex, &data->lock)
- ret = mma8452_standby(data);
+ guard(mutex)(&data->lock);
+
+ ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG4);
if (ret < 0) {
- dev_err(dev, "powering off device failed\n");
+ dev_warn(dev, "backing up CTRL_REG4 failed\n");
return -EAGAIN;
+ } else
+ data->ctrl_reg4 = ret;
+
+ ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG4, 0);
+ if (ret) {
+ dev_warn(dev, "disabling interrupt sources (CTRL_REG4) failed\n");
+ return -EAGAIN;
+ }
+
+ ret = mma8452_standby(data);
+ if (ret < 0) {
+ dev_err(dev, "transition to STANDBY mode failed\n");
+ ret = -EAGAIN;
+ goto out_restore_ctrl_reg4;
}
+ /*
+ * Interrupt line should be deasserted now, so we just need ensure any
+ * mid-flight irq is completed (will return IRQ_NONE due to
+ * pm_status==0).
+ */
+ if (client->irq)
+ synchronize_irq(client->irq);
+
ret = regulator_bulk_disable(ARRAY_SIZE(data->regs), data->regs);
if (ret) {
dev_err(dev, "failed to disable regulators\n");
- return ret;
+ goto out_active;
}
return 0;
+
+out_active:
+ if (mma8452_active(data))
+ dev_warn(dev, "failed to switch back to ACTIVE mode\n");
+out_restore_ctrl_reg4:
+ if (i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG4, data->ctrl_reg4))
+ dev_warn(dev, "restoring CTRL_REG4 failed\n");
+ return ret;
}
static int mma8452_runtime_resume(struct device *dev)
{
- struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct i2c_client *client = to_i2c_client(dev);
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
struct mma8452_data *data = iio_priv(indio_dev);
int ret, sleep_val;
@@ -1820,6 +1865,10 @@ static int mma8452_runtime_resume(struct device *dev)
if (ret)
goto runtime_resume_failed;
+ ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG4, data->ctrl_reg4);
+ if (ret)
+ goto runtime_resume_failed;
+
ret = mma8452_active(data);
if (ret < 0)
goto runtime_resume_failed;
--
2.55.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing
2026-09-07 14:51 ` [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing Esben Haabendal
@ 2026-09-07 15:08 ` sashiko-bot
2026-09-07 15:58 ` Esben Haabendal
2026-09-07 15:10 ` Joshua Crofts
1 sibling, 1 reply; 27+ messages in thread
From: sashiko-bot @ 2026-09-07 15:08 UTC (permalink / raw)
To: Esben Haabendal; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [Critical] Writing to MMA8452_CTRL_REG4 to disable interrupt sources fails because the device is still in ACTIVE mode.
- [High] The IRQ handler incorrectly checks `pm_status == 0` instead of `pm_status <= 0`, allowing invalid hardware access when PM is disabled.
- [Low] Mixing goto-based error handling with guard() cleanup in the same function.
Pre-existing issues:
- [High] Hardware access occurs after dropping the runtime PM reference in mma8452_write_event_config.
- [High] AB-BA deadlock between the driver's data->lock and the PM core's runtime suspend wait.
--
commit 3d8bf763b3ae36284346684babbd11058f68e68a
Author: Esben Haabendal <esben@geanix.com>
Date: Mon Sep 7 14:51:04 2026 +0000
iio: accel: mma8452: Support interrupt sharing
Adding support for sharing interrupt line with other device requires the
interrupt handler to handle runtime PM suspension properly, ignoring the
irq if the device is suspended (maybe even off). And while at it, we use
the PM reference to ensure we do not get suspended while processing an irq.
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index fda29df5d1097..e521dca37f768 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
[ ... ]
> @@ -1083,15 +1085,21 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
> {
> struct iio_dev *indio_dev = p;
> struct mma8452_data *data = iio_priv(indio_dev);
> + struct device *dev = &data->client->dev;
> irqreturn_t ret = IRQ_NONE;
> + int pm_status;
> int src;
>
> + pm_status = pm_runtime_get_if_active(dev);
> + if (pm_status == 0)
> + return IRQ_NONE; /* device is powered down */
> +
[Severity: High]
Does this check fail to catch negative error codes, allowing the handler to
proceed and access unpowered hardware?
When runtime PM is disabled, pm_runtime_get_if_active() returns -EINVAL. This
can happen during driver unbind or removal, as pm_runtime_disable() is called
before free_irq():
drivers/iio/accel/mma8452.c:mma8452_remove() {
...
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
free_irq:
if (client->irq)
free_irq(client->irq, indio_dev);
...
}
If a shared IRQ fires concurrently after PM is disabled, this check allows
the handler to proceed, leading to I2C bus timeouts or system hangs.
> src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
[ ... ]
> @@ -1784,29 +1796,62 @@ static void mma8452_remove(struct i2c_client *client)
> #ifdef CONFIG_PM
> static int mma8452_runtime_suspend(struct device *dev)
> {
> - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct i2c_client *client = to_i2c_client(dev);
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret;
>
> - scoped_guard(mutex, &data->lock)
> - ret = mma8452_standby(data);
> + guard(mutex)(&data->lock);
> +
> + ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG4);
> if (ret < 0) {
> - dev_err(dev, "powering off device failed\n");
> + dev_warn(dev, "backing up CTRL_REG4 failed\n");
> return -EAGAIN;
> + } else
> + data->ctrl_reg4 = ret;
> +
> + ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG4, 0);
> + if (ret) {
> + dev_warn(dev, "disabling interrupt sources (CTRL_REG4) failed\n");
> + return -EAGAIN;
> + }
[Severity: Critical]
Does writing to MMA8452_CTRL_REG4 fail here because the device is still in
ACTIVE mode?
Since the write happens before mma8452_standby(), the hardware ignores it
(as modifying configs requires STANDBY mode). This leaves interrupt sources
enabled. If a shared IRQ fires, the handler will return IRQ_NONE due to the
pm_status check, leaving the interrupt asserted and causing a level-triggered
interrupt storm.
> +
> + ret = mma8452_standby(data);
> + if (ret < 0) {
> + dev_err(dev, "transition to STANDBY mode failed\n");
> + ret = -EAGAIN;
> + goto out_restore_ctrl_reg4;
> }
[Severity: Low]
Does this mix goto-based error handling with guard() cleanup in the same
function?
The cleanup subsystem guidelines state that mixing goto and scope-based cleanup
creates confusing ownership semantics.
[ ... ]
As a side note, there are a couple of pre-existing issues in this file that
are not introduced by this patch, but are worth mentioning:
[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-put in
mma8452_write_event_config() when disabling an event?
drivers/iio/accel/mma8452.c:mma8452_write_event_config() {
...
ret = mma8452_set_runtime_pm_state(data->client, state);
if (ret)
return ret;
switch (dir) {
case IIO_EV_DIR_FALLING:
return mma8452_set_freefall_mode(data, state);
case IIO_EV_DIR_RISING:
val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
...
}
When state is false, mma8452_set_runtime_pm_state() calls
pm_runtime_put_autosuspend(), dropping the PM reference early. The subsequent
i2c_smbus_read_byte_data() accesses hardware while the device could be
concurrently suspended, potentially causing I2C transaction failures.
[Severity: High]
This is a pre-existing issue, but does mma8452_read_raw() create an AB-BA
deadlock with PM suspend?
drivers/iio/accel/mma8452.c:mma8452_read_raw() {
...
guard(mutex)(&data->lock);
ret = mma8452_read(data, buffer);
...
}
mma8452_read() calls mma8452_set_runtime_pm_state(..., true), triggering a
synchronous PM resume. If a concurrent PM suspend is initiated by the PM
core, it sets the status to RPM_SUSPENDING and calls
mma8452_runtime_suspend():
drivers/iio/accel/mma8452.c:mma8452_runtime_suspend() {
...
guard(mutex)(&data->lock);
...
}
The suspend callback will block waiting for data->lock, while
mma8452_read_raw() blocks waiting for the RPM_SUSPENDING state to clear,
resulting in a deadlock.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-mma8452-open-drain-v8-0-c17407e22118@geanix.com?part=9
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing
2026-09-07 15:08 ` sashiko-bot
@ 2026-09-07 15:58 ` Esben Haabendal
2026-09-08 9:15 ` Esben Haabendal
0 siblings, 1 reply; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 15:58 UTC (permalink / raw)
To: sashiko-reviews; +Cc: devicetree, robh, conor+dt
<sashiko-bot@kernel.org> writes:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> New issues:
> - [Critical] Writing to MMA8452_CTRL_REG4 to disable interrupt sources fails because the device is still in ACTIVE mode.
> - [High] The IRQ handler incorrectly checks `pm_status == 0` instead of `pm_status <= 0`, allowing invalid hardware access when PM is disabled.
> - [Low] Mixing goto-based error handling with guard() cleanup in the same function.
>
> Pre-existing issues:
> - [High] Hardware access occurs after dropping the runtime PM reference in mma8452_write_event_config.
> - [High] AB-BA deadlock between the driver's data->lock and the PM core's runtime suspend wait.
> --
>
> commit 3d8bf763b3ae36284346684babbd11058f68e68a
> Author: Esben Haabendal <esben@geanix.com>
> Date: Mon Sep 7 14:51:04 2026 +0000
>
> iio: accel: mma8452: Support interrupt sharing
>
> Adding support for sharing interrupt line with other device requires the
> interrupt handler to handle runtime PM suspension properly, ignoring the
> irq if the device is suspended (maybe even off). And while at it, we use
> the PM reference to ensure we do not get suspended while processing an irq.
>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index fda29df5d1097..e521dca37f768 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
> [ ... ]
>> @@ -1083,15 +1085,21 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>> {
>> struct iio_dev *indio_dev = p;
>> struct mma8452_data *data = iio_priv(indio_dev);
>> + struct device *dev = &data->client->dev;
>> irqreturn_t ret = IRQ_NONE;
>> + int pm_status;
>> int src;
>>
>> + pm_status = pm_runtime_get_if_active(dev);
>> + if (pm_status == 0)
>> + return IRQ_NONE; /* device is powered down */
>> +
>
> [Severity: High]
> Does this check fail to catch negative error codes, allowing the handler to
> proceed and access unpowered hardware?
>
> When runtime PM is disabled, pm_runtime_get_if_active() returns -EINVAL. This
> can happen during driver unbind or removal, as pm_runtime_disable() is called
> before free_irq():
>
> drivers/iio/accel/mma8452.c:mma8452_remove() {
> ...
> pm_runtime_disable(dev);
> pm_runtime_set_suspended(dev);
>
> free_irq:
> if (client->irq)
> free_irq(client->irq, indio_dev);
> ...
> }
>
> If a shared IRQ fires concurrently after PM is disabled, this check allows
> the handler to proceed, leading to I2C bus timeouts or system hangs.
This is on purpose. If runtime PM is disabled, the assumption is that
the device is active. Otherwise, the driver would be useless when
CONFIG_PM was not enabled.
>> src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
> [ ... ]
>> @@ -1784,29 +1796,62 @@ static void mma8452_remove(struct i2c_client *client)
>> #ifdef CONFIG_PM
>> static int mma8452_runtime_suspend(struct device *dev)
>> {
>> - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> + struct i2c_client *client = to_i2c_client(dev);
>> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> struct mma8452_data *data = iio_priv(indio_dev);
>> int ret;
>>
>> - scoped_guard(mutex, &data->lock)
>> - ret = mma8452_standby(data);
>> + guard(mutex)(&data->lock);
>> +
>> + ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG4);
>> if (ret < 0) {
>> - dev_err(dev, "powering off device failed\n");
>> + dev_warn(dev, "backing up CTRL_REG4 failed\n");
>> return -EAGAIN;
>> + } else
>> + data->ctrl_reg4 = ret;
>> +
>> + ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG4, 0);
>> + if (ret) {
>> + dev_warn(dev, "disabling interrupt sources (CTRL_REG4) failed\n");
>> + return -EAGAIN;
>> + }
>
> [Severity: Critical]
> Does writing to MMA8452_CTRL_REG4 fail here because the device is still in
> ACTIVE mode?
Ouch, Yes. That is actually true according to the datasheet.
I will swap the order so that we switch to standby before modifying CTR_REG4.
> Since the write happens before mma8452_standby(), the hardware ignores it
> (as modifying configs requires STANDBY mode). This leaves interrupt sources
> enabled. If a shared IRQ fires, the handler will return IRQ_NONE due to the
> pm_status check, leaving the interrupt asserted and causing a level-triggered
> interrupt storm.
>
>> +
>> + ret = mma8452_standby(data);
>> + if (ret < 0) {
>> + dev_err(dev, "transition to STANDBY mode failed\n");
>> + ret = -EAGAIN;
>> + goto out_restore_ctrl_reg4;
>> }
>
> [Severity: Low]
> Does this mix goto-based error handling with guard() cleanup in the same
> function?
>
> The cleanup subsystem guidelines state that mixing goto and scope-based cleanup
> creates confusing ownership semantics.
>
> [ ... ]
I will try to convert to scope-based cleanup to avoid this, and see if I
can create something nice with that approach.
> As a side note, there are a couple of pre-existing issues in this file that
> are not introduced by this patch, but are worth mentioning:
>
> [Severity: High]
> This isn't a bug introduced by this patch, but is there a use-after-put in
> mma8452_write_event_config() when disabling an event?
>
> drivers/iio/accel/mma8452.c:mma8452_write_event_config() {
> ...
> ret = mma8452_set_runtime_pm_state(data->client, state);
> if (ret)
> return ret;
>
> switch (dir) {
> case IIO_EV_DIR_FALLING:
> return mma8452_set_freefall_mode(data, state);
> case IIO_EV_DIR_RISING:
> val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
> ...
> }
>
> When state is false, mma8452_set_runtime_pm_state() calls
> pm_runtime_put_autosuspend(), dropping the PM reference early. The subsequent
> i2c_smbus_read_byte_data() accesses hardware while the device could be
> concurrently suspended, potentially causing I2C transaction failures.
I will address this in another series.
> [Severity: High]
> This is a pre-existing issue, but does mma8452_read_raw() create an AB-BA
> deadlock with PM suspend?
>
> drivers/iio/accel/mma8452.c:mma8452_read_raw() {
> ...
> guard(mutex)(&data->lock);
>
> ret = mma8452_read(data, buffer);
> ...
> }
>
> mma8452_read() calls mma8452_set_runtime_pm_state(..., true), triggering a
> synchronous PM resume. If a concurrent PM suspend is initiated by the PM
> core, it sets the status to RPM_SUSPENDING and calls
> mma8452_runtime_suspend():
>
> drivers/iio/accel/mma8452.c:mma8452_runtime_suspend() {
> ...
> guard(mutex)(&data->lock);
> ...
> }
>
> The suspend callback will block waiting for data->lock, while
> mma8452_read_raw() blocks waiting for the RPM_SUSPENDING state to clear,
> resulting in a deadlock.
I will try to look out for this in the next series, where I am trying to
improve all the runtime PM handling in this driver.
/Esben
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing
2026-09-07 15:58 ` Esben Haabendal
@ 2026-09-08 9:15 ` Esben Haabendal
0 siblings, 0 replies; 27+ messages in thread
From: Esben Haabendal @ 2026-09-08 9:15 UTC (permalink / raw)
To: sashiko-reviews; +Cc: devicetree, robh, conor+dt
"Esben Haabendal" <esben@geanix.com> writes:
> <sashiko-bot@kernel.org> writes:
>>> +
>>> + ret = mma8452_standby(data);
>>> + if (ret < 0) {
>>> + dev_err(dev, "transition to STANDBY mode failed\n");
>>> + ret = -EAGAIN;
>>> + goto out_restore_ctrl_reg4;
>>> }
>>
>> [Severity: Low]
>> Does this mix goto-based error handling with guard() cleanup in the same
>> function?
>>
>> The cleanup subsystem guidelines state that mixing goto and scope-based cleanup
>> creates confusing ownership semantics.
>>
>> [ ... ]
>
> I will try to convert to scope-based cleanup to avoid this, and see if I
> can create something nice with that approach.
I hope it is okay that I delay doing this for the next series, where I
will introduce scope-based cleanup for runtime PM also.
While it is best not to mix goto and scope-based cleanup in the same
functions, in this particular case, it is working as expected for now.
The only scope-based cleanup is the guard which is first in the
function, and is supposed to be run after all the goto-based error
handling, which is exactly what will happen.
But I will try and rework it to use scope-based cleanup only as part of
the next series also.
/Esben
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing
2026-09-07 14:51 ` [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing Esben Haabendal
2026-09-07 15:08 ` sashiko-bot
@ 2026-09-07 15:10 ` Joshua Crofts
2026-09-07 16:36 ` Esben Haabendal
1 sibling, 1 reply; 27+ messages in thread
From: Joshua Crofts @ 2026-09-07 15:10 UTC (permalink / raw)
To: Esben Haabendal
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, linux-iio, devicetree, linux-kernel
On Mon, 07 Sep 2026 16:51:04 +0200
Esben Haabendal <esben@geanix.com> wrote:
> Adding support for sharing interrupt line with other device requires the
> interrupt handler to handle runtime PM suspension properly, ignoring the
> irq if the device is suspended (maybe even off). And while at it, we use
> the PM reference to ensure we do not get suspended while processing an irq.
>
> In order to prevent the chip from raising irq while suspended (that is when
> using fixed regulator, where suspend just means setting the device in
> STANDBY mode), we disable all interrupt sources by clearing CTRL_REG4, and
> then restores the value again when resuming.
>
> With that in place, it is safe to add the IRQF_SHARED flag.
>
> Keep in mind that the device by default is using push-pull for the irq pin,
> which might require additional hardware design to allow interrupt sharing.
>
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
...
> @@ -1784,29 +1796,62 @@ static void mma8452_remove(struct i2c_client *client)
> #ifdef CONFIG_PM
> static int mma8452_runtime_suspend(struct device *dev)
> {
> - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct i2c_client *client = to_i2c_client(dev);
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> struct mma8452_data *data = iio_priv(indio_dev);
> int ret;
>
> - scoped_guard(mutex, &data->lock)
> - ret = mma8452_standby(data);
> + guard(mutex)(&data->lock);
> +
Hmm, this patch might benefit from breaking it into multiple patches
(the change from scoped_guard to guard, the local pointers etc. along
the fact that these aren't mentioned in the commit message), but
that's up to Jonathan.
--
Kind regards,
Joshua Crofts
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing
2026-09-07 15:10 ` Joshua Crofts
@ 2026-09-07 16:36 ` Esben Haabendal
2026-09-09 9:21 ` Joshua Crofts
0 siblings, 1 reply; 27+ messages in thread
From: Esben Haabendal @ 2026-09-07 16:36 UTC (permalink / raw)
To: Joshua Crofts
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, linux-iio, devicetree, linux-kernel
"Joshua Crofts" <joshua.crofts1@gmail.com> writes:
> On Mon, 07 Sep 2026 16:51:04 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> Adding support for sharing interrupt line with other device requires the
>> interrupt handler to handle runtime PM suspension properly, ignoring the
>> irq if the device is suspended (maybe even off). And while at it, we use
>> the PM reference to ensure we do not get suspended while processing an irq.
>>
>> In order to prevent the chip from raising irq while suspended (that is when
>> using fixed regulator, where suspend just means setting the device in
>> STANDBY mode), we disable all interrupt sources by clearing CTRL_REG4, and
>> then restores the value again when resuming.
>>
>> With that in place, it is safe to add the IRQF_SHARED flag.
>>
>> Keep in mind that the device by default is using push-pull for the irq pin,
>> which might require additional hardware design to allow interrupt sharing.
>>
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>
> ...
>
>> @@ -1784,29 +1796,62 @@ static void mma8452_remove(struct i2c_client *client)
>> #ifdef CONFIG_PM
>> static int mma8452_runtime_suspend(struct device *dev)
>> {
>> - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> + struct i2c_client *client = to_i2c_client(dev);
>> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> struct mma8452_data *data = iio_priv(indio_dev);
>> int ret;
>>
>> - scoped_guard(mutex, &data->lock)
>> - ret = mma8452_standby(data);
>> + guard(mutex)(&data->lock);
>> +
>
> Hmm, this patch might benefit from breaking it into multiple patches
> (the change from scoped_guard to guard, the local pointers etc. along
> the fact that these aren't mentioned in the commit message), but
> that's up to Jonathan.
I will try and improve the commit message. I don't think that breaking
it up makes a lot of sense.
The change from guard() to scoped_guard() is closely tied to the rest of
the changes. At least I am not sure if it is even sane to make that
change without the rest of this commit. It might be fine. Probably is,
but is it worth to risk causing trouble for older kernels?
Regarding the local pointers, I assume you mean the added struct
i2c_client and struct device pointers. They are only used in the added
code, so I don't see how I can split that out in a meaningful way.
I don't mind making such changes, I just don't see what I am expected to
do.
/Esben
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing
2026-09-07 16:36 ` Esben Haabendal
@ 2026-09-09 9:21 ` Joshua Crofts
0 siblings, 0 replies; 27+ messages in thread
From: Joshua Crofts @ 2026-09-09 9:21 UTC (permalink / raw)
To: Esben Haabendal
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Martin Kepplinger,
Sean Nyekjaer, David Lechner, Nuno Sá, Andy Shevchenko,
Martin Kepplinger, linux-iio, devicetree, linux-kernel
On Mon, 07 Sep 2026 18:36:56 +0200
Esben Haabendal <esben@geanix.com> wrote:
> "Joshua Crofts" <joshua.crofts1@gmail.com> writes:
>
> > On Mon, 07 Sep 2026 16:51:04 +0200
> > Esben Haabendal <esben@geanix.com> wrote:
> >
> >> Adding support for sharing interrupt line with other device requires the
> >> interrupt handler to handle runtime PM suspension properly, ignoring the
> >> irq if the device is suspended (maybe even off). And while at it, we use
> >> the PM reference to ensure we do not get suspended while processing an irq.
> >>
> >> In order to prevent the chip from raising irq while suspended (that is when
> >> using fixed regulator, where suspend just means setting the device in
> >> STANDBY mode), we disable all interrupt sources by clearing CTRL_REG4, and
> >> then restores the value again when resuming.
> >>
> >> With that in place, it is safe to add the IRQF_SHARED flag.
> >>
> >> Keep in mind that the device by default is using push-pull for the irq pin,
> >> which might require additional hardware design to allow interrupt sharing.
> >>
> >> Signed-off-by: Esben Haabendal <esben@geanix.com>
> >> ---
> >
> > ...
> >
> >> @@ -1784,29 +1796,62 @@ static void mma8452_remove(struct i2c_client *client)
> >> #ifdef CONFIG_PM
> >> static int mma8452_runtime_suspend(struct device *dev)
> >> {
> >> - struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> >> + struct i2c_client *client = to_i2c_client(dev);
> >> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> >> struct mma8452_data *data = iio_priv(indio_dev);
> >> int ret;
> >>
> >> - scoped_guard(mutex, &data->lock)
> >> - ret = mma8452_standby(data);
> >> + guard(mutex)(&data->lock);
> >> +
> >
> > Hmm, this patch might benefit from breaking it into multiple patches
> > (the change from scoped_guard to guard, the local pointers etc. along
> > the fact that these aren't mentioned in the commit message), but
> > that's up to Jonathan.
>
> I will try and improve the commit message. I don't think that breaking
> it up makes a lot of sense.
>
> The change from guard() to scoped_guard() is closely tied to the rest of
> the changes. At least I am not sure if it is even sane to make that
> change without the rest of this commit. It might be fine. Probably is,
> but is it worth to risk causing trouble for older kernels?
>
> Regarding the local pointers, I assume you mean the added struct
> i2c_client and struct device pointers. They are only used in the added
> code, so I don't see how I can split that out in a meaningful way.
>
Ah fair enough, no need to split it then :)
--
Kind regards,
Joshua Crofts
^ permalink raw reply [flat|nested] 27+ messages in thread