Re: [PATCH v3 1/5] iio: accel: bma400: Reorganize and rename register and field macros

[Akshay Jindal]

On Sat, Oct 4, 2025 at 6:23 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>This is much easier to review. Thanks for breaking it all up.

Hi Jonathan,
Thank you for the review.
Keeping v3 feedback in mind, I have floated a v4 patch series.
Have some follow-ups in some comments. Please read below
for those.

Thanks,
Akshay.

> >  #define BMA400_INT_STAT0_REG        0x0e
> >  #define BMA400_INT_STAT1_REG        0x0f
> >  #define BMA400_INT_STAT2_REG        0x10
> > -#define BMA400_INT12_MAP_REG        0x23
> > -#define BMA400_INT_ENG_OVRUN_MSK    BIT(4)
> > +#define BMA400_ENG_OVRUN_INT_STAT_MASK               BIT(4)
>
> This is an odd field as it applies to all the INT_STATX registers
> However  I would still try to make that connection with a name
> such as BMA500_INT_STAT_OVRUN_MASK
The connection is still there Jonathan.
The name in the spec is Interrupt Engine Overrun.
BMA400_ENG_OVRUN_INT_STAT_MASK can be read as
Engine Overrun Interrupt Status Mask.
Here for Interrupt Status fields, I have intentionally taken a little
deviation from the naming convention established.
Original convention:BMA400_<reg_name>_<field_name>_<suffix>
Convention here: BMA400_<INT NAME>_<INT_STAT>_<suffix>
so that it can be read as <INT_NAME> Interrupt Status mask.

I would understand, if you want to adhere to original convention.
Will make the change in next version.

>
> > +#define BMA400_STEP_INT_STAT_MASK            GENMASK(9, 8)
>
> This bit is a little odd.  We are treating INT_STAT0 and INT_STAT1
> (but not 2) as a single 16 bit register. That makes it hard to
> associate the field with the register name. I wonder if we shouldn't
> break that and just handle it as a pair of u8 instead.
The spec talks about doing a burst read for such multipart registers
to avoid reading one, while the other one is being updated.
Hence did not touch it.

> >  /*
> >   * Read-write configuration registers
> >   */
> > -#define BMA400_ACC_CONFIG0_REG      0x19
> > -#define BMA400_ACC_CONFIG1_REG      0x1a
> > +#define BMA400_ACC_CONFIG0_REG               0x19
> > +#define BMA400_ACC_CONFIG0_LP_OSR_MASK               GENMASK(6, 5)
> > +#define BMA400_LP_OSR_SHIFT          5
> #
> Should never need a explicit shift. Use FIELD_PREP() and FIELD_GET() to
> allow the MASK to be used in all cases.
>
done

> > +#define BMA400_NP_OSR_SHIFT          4
> Similarly on this shift.
done

> > +#define BMA400_ACC_CONFIG1_ACC_RANGE_MASK    GENMASK(7, 6)
> > +#define BMA400_ACC_RANGE_SHIFT               6
>
> and this one.  Might be a good idea to switch away from using the shifts
> as a precursor patch as it's really a different sort of change from
> the rest of this.
Added a separate patch for this.

>
> > -             osr = (val & BMA400_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
> > +             osr = (val & BMA400_ACC_CONFIG0_LP_OSR_MASK) >> BMA400_LP_OSR_SHIFT;
>
> Here is one of those cases with the shift that could just be
yes, fixed as stated above.


> > -             osr = (val & BMA400_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
> > +             osr = (val & BMA400_ACC_CONFIG1_NP_OSR_MASK) >> BMA400_NP_OSR_SHIFT;
> her as well.
yes, fixed.

> >               ret = regmap_write(data->regmap, BMA400_ACC_CONFIG0_REG,
> > -                                (acc_config & ~BMA400_LP_OSR_MASK) |
> > +                                (acc_config & ~BMA400_ACC_CONFIG0_LP_OSR_MASK) |
> >                                  (val << BMA400_LP_OSR_SHIFT));
> FIELD_PREP for this one.
fixed.

> >               ret = regmap_write(data->regmap, BMA400_ACC_CONFIG1_REG,
> > -                                (acc_config & ~BMA400_NP_OSR_MASK) |
> > +                                (acc_config & ~BMA400_ACC_CONFIG1_NP_OSR_MASK) |
> >                                  (val << BMA400_NP_OSR_SHIFT));
> here as well.  Anyhow, from a quick look it appears that getting rid of the _SHIFT defines
> should be easy.
yes, fixed.


> >       ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG,
> > -                              BMA400_STEP_INT_MSK,
> > -                              FIELD_PREP(BMA400_STEP_INT_MSK, val ? 1 : 0));
> > +                              BMA400_INT_CONFIG1_STEP_INT_MASK,
> > +                              FIELD_PREP(BMA400_INT_CONFIG1_STEP_INT_MASK, val ? 1 : 0));
>
> Could use regmap_assign_bits() to simplify this a bit - but separate change
> so different patch.
regmap_assign_bits calls regmap_set_bits which itself uses
regmap_update_bits_base similar to regmap_update_bits.
Moreover adoption of regmap_assign_bits is not much in drivers.
Hence would request to keep it as it is.

Thanks,
Akshay