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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 7 Oct 2025 12:09:43 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> On Sat, Oct 4, 2025 at 6:23 PM Jonathan Cameron <jic23@kernel.org> 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

Yes, I definitely wnt the register name association in there (or a wild
card type thing if it turns up in multiple similar registers.

> 
> >  
> > > +#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.
ok. But then why not INT_STAT2?  Could also do a burst read but still
treat them as separate registers. Bulk read into a u8[2];
  
Cropping out the stuff that needs no more discussion (you could have cropped
the bits you agree with btw!)  Removing them is an implicit 'agreed'

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

I think it is fairly new and the gain here is minor (plus it would be an additional
change anyway) so fine to leave it as it stands.

Thanks,

Jonathan


> 
> Thanks,
> Akshay
>