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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE3SzaST=w7f0yM1C2iGfD9fw7smzMDven5kOoSQ0jMMZHMkWw@mail.gmail.com>
Date: Tue, 7 Oct 2025 12:09:43 +0530
From: Akshay Jindal <akshayaj.lkd@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: dan@...obertson.com, dlechner@...libre.com, nuno.sa@...log.com, 
	andy@...nel.org, shuah@...nel.org, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/5] iio: accel: bma400: Reorganize and rename register
 and field macros

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

>
> > +#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