Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal <esben@geanix.com>]

"Joshua Crofts" <joshua.crofts1@gmail.com> writes:

> On Wed, 15 Jul 2026 10:07:39 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> When sharing interrupt line with other chips, the interrupt pin most
>> likely needs to be configured in open-drain mode instead of push-pull.
>> If this is needed, you must add drive-open-drain property to the
>> device-tree.
>
> Why are you mentioning the device tree in the commit message? Just keep
> the first sentence + a short description of what you
> added/changed/removed.

Sure. Will do that for v2.

>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  drivers/iio/accel/mma8452.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 7d683686dd9d..a20c02ce0b9c 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>> @@ -81,6 +81,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)
>
> I know that the defines are completely incorrectly aligned, but please
> ensure that at least all the defines in this block are aligned.
>
> Also, consider sending a patch which aligns all the other defines.

How are they incorrectly aligned?
The all look perfectly fine here (visual tabs space set to 8).
Should I convert all the tabs used for alignment to spaces?

AFAICS, I have added the defines with same alignment as the other
defines in that block. I believe the misalignment is only a visual
artifact caused by the diff format.

>>  #define MMA8452_CTRL_REG4			0x2d
>>  #define MMA8452_CTRL_REG5			0x2e
>>  #define MMA8452_OFF_X				0x2f
>> @@ -108,6 +110,7 @@ struct mma8452_data {
>>  	struct iio_mount_matrix orientation;
>>  	u8 ctrl_reg1;
>>  	u8 data_cfg;
>> +	bool open_drain;
>
> Hmm, i checked pahole and it says there is a 1 byte hole, maybe try some more
> reordering to pack it?

On aarch64 it was this:

struct mma8452_data {
        struct i2c_client *        client;               /*     0     8 */
        struct mutex               lock __attribute__((__aligned__(8))); /*     8    24 */
        struct iio_mount_matrix    orientation;          /*    32    72 */
        /* --- cacheline 1 boundary (64 bytes) was 40 bytes ago --- */
        u8                         ctrl_reg1;            /*   104     1 */
        u8                         data_cfg;             /*   105     1 */
        bool                       open_drain;           /*   106     1 */

        /* XXX 5 bytes hole, try to pack */

        const struct mma_chip_info  * chip_info;         /*   112     8 */
        int                        sleep_val;            /*   120     4 */

        /* XXX 4 bytes hole, try to pack */

        /* --- cacheline 2 boundary (128 bytes) --- */
        struct regulator *         vdd_reg;              /*   128     8 */
        struct regulator *         vddio_reg;            /*   136     8 */
        struct {
                __be16             channels[3];          /*   144     6 */

                /* XXX 2 bytes hole, try to pack */

                __s64              ts __attribute__((__aligned__(8))); /*   152     8 */
        } __attribute__((__aligned__(8))) buffer __attribute__((__aligned__(8)));        /*   144    16 */

        /* XXX last struct has 1 hole */

        /* size: 160, cachelines: 3, members: 11 */
        /* sum members: 151, holes: 2, sum holes: 9 */
        /* member types with holes: 1, total: 1 */
        /* forced alignments: 2 */
        /* last cacheline: 32 bytes */
} __attribute__((__aligned__(8)));

After reordering member fields, I get this:

struct mma8452_data {
        struct i2c_client *        client;               /*     0     8 */
        struct mutex               lock __attribute__((__aligned__(8))); /*     8    24 */
        struct iio_mount_matrix    orientation;          /*    32    72 */
        /* --- cacheline 1 boundary (64 bytes) was 40 bytes ago --- */
        const struct mma_chip_info  * chip_info;         /*   104     8 */
        struct regulator *         vdd_reg;              /*   112     8 */
        struct regulator *         vddio_reg;            /*   120     8 */
        /* --- cacheline 2 boundary (128 bytes) --- */
        struct {
                __be16             channels[3];          /*   128     6 */

                /* XXX 2 bytes hole, try to pack */

                __s64              ts __attribute__((__aligned__(8))); /*   136     8 */
        } __attribute__((__aligned__(8))) buffer __attribute__((__aligned__(8)));        /*   128    16 */

        /* XXX last struct has 1 hole */

        int                        sleep_val;            /*   144     4 */
        u8                         ctrl_reg1;            /*   148     1 */
        u8                         data_cfg;             /*   149     1 */
        bool                       open_drain;           /*   150     1 */

        /* size: 152, cachelines: 3, members: 11 */
        /* padding: 1 */
        /* member types with holes: 1, total: 1 */
        /* forced alignments: 2 */
        /* last cacheline: 24 bytes */
} __attribute__((__aligned__(8)));

>>  	const struct mma_chip_info *chip_info;
>>  	int sleep_val;
>>  	struct regulator *vdd_reg;
>> @@ -646,6 +649,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)
>>  {
>> @@ -1666,6 +1685,9 @@ static int mma8452_probe(struct i2c_client *client)
>>  			goto disable_regulators;
>>  	}
>>
>> +	data->open_drain = device_property_read_bool(&client->dev, "drive-open-drain");
>> +	mma8452_set_interrupt_pin_mode(data);
>
> You're not checking the return value here.

Sorry, I will propagate the error code up.

>>  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>>  			  (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>>
>> @@ -1683,7 +1705,8 @@ static int mma8452_probe(struct i2c_client *client)
>>
>>  	if (client->irq) {
>>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
>> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> +					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
>> +					   data->open_drain ? IRQF_SHARED : 0,
>
> Sashiko raises a pretty fun issue: the statement
>
> IRQF_TRIGGER_LOW | IRQF_ONESHOT | data->open_drain ? IRQF_SHARED : 0
>
> is actually evaluated as
>
> (IRQF_TRIGGER_LOW | IRQF_ONESHOT | data->open_drain) ? IRQF_SHARED : 0
>
> Bitwise OR precedes the ternary operator.
>
> You should wrap the data->open_drain ternary in parenthesis.

Yep. That was a nice catch indeed. Fixed.

>>  					   client->name, indio_dev);
>>  		if (ret)
>>  			goto buffer_cleanup;
>> @@ -1800,6 +1823,10 @@ static int mma8452_runtime_resume(struct device *dev)
>>  		return ret;
>>  	}
>>
>> +	ret = mma8452_set_interrupt_pin_mode(data);
>> +	if (ret < 0)
>> +		goto runtime_resume_failed;
>
> You can just have if (ret), as only 0 is successful.

Will do.

>> +
>>  	ret = mma8452_active(data);
>>  	if (ret < 0)
>>  		goto runtime_resume_failed;