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

[Joshua Crofts]

On Wed, 15 Jul 2026 13:29:17 +0200
Esben Haabendal <esben@xxxxxxxxxx> wrote:

...

> >> 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.

Ah, I meant it as currently the defines look like this:

#define MMA8452_WHO_AM_I			0x0d
#define MMA8452_DATA_CFG			0x0e
#define  MMA8452_DATA_CFG_FS_MASK		GENMASK(1, 0)
#define  MMA8452_DATA_CFG_FS_2G			0

but instead should look like this:

#define MMA8452_WHO_AM_I			0x0d
#define MMA8452_DATA_CFG			0x0e
#define MMA8452_DATA_CFG_FS_MASK		GENMASK(1, 0)
#define MMA8452_DATA_CFG_FS_2G			0

I'm not sure if this was intentional or not in the original implementation.

...

> 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)));

This looks great!

-- 
Kind regards

CJD