Re: [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing

[Esben Haabendal <esben@geanix.com>]

"Jonathan Cameron" <jic23@kernel.org> 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>
>> ---
>>  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);
>
> Sashiko raises the question of what happens if you actually get an error
> return from this. You may be deliberately ignoring those, but if
> so add a comment.

Yes, sounds like a good idea.

> The fun race around tear down is worth considering in particular (see
> sashiko comment).

I will look into that. Although very unlikely, it does sound like a real
issue that *could* occur.

Adding a boolean flag (remove_in_progress = true) to the mma8452_data
struct, and set that in mma8452_remove() before disabling runtime pm
seems like a KISS solution. Should we be returning IRQ_HANDLED or
IRQ_NONE in that case?  There is no IRQ_MAYBE return value :D

>> +	if (pm_status == 0)
>> +		return IRQ_NONE; /* device is powered down */
>
>> @@ -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);
> Mixing guards...

Yes, I know. Resolving that turned out to be a bit more painful than I
thought. I do have that resolved in the next series I keep on talking
about. But distilling that as a separate patch for adding to this series
turned out to be impossible, as it relies on some of the other work,
like refactoring of the ACTIVE/STANDBY state handling (synchronization),
and most importantly, the switch to using regmap caching to properly
handle restoring of register values on resume.

I am therefore hoping that we can find some way to agree on things here,
and then properly resolve it in the next series...

For what it is worth, in this very specific case, the only guard used is
first in the LIFO order, and will therefore be executed in exactly the
right order on exit, as it will run after all the goto cleanup, just as
the LIFO cleanup order requires.

Obviously a fragile and undesirable way of doing error handling, but it
will go away again in the next series where I will eliminate the goto
error handling in mma8452_runtime_suspend().

>> +
>> +	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;
>
> and goto is explicitly advised against in the docs in cleanup.h
> You need to restructure the code to avoid that, potentially via
> a helper function.

I agree. See above.

>>  	}
>>
>> +	/*
>> +	 * 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;
>>