| 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
| ||
|
Message-ID: <CAE3SzaReUnhWyzA8RtdizKeRU2zMsGbvQaVT-ug6v+=Pqq8WzA@mail.gmail.com>
Date: Sun, 21 Sep 2025 15:18:09 +0530
From: Akshay Jindal <akshayaj.lkd@...il.com>
To: Nuno Sá <noname.nuno@...il.com>
Cc: dan@...obertson.com, jic23@...nel.org, 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] iio: accel: bma400: Refactor generic interrupt configuration
Hi Nuno,
Thank you so much for your review. Please find follow-ups inline.
On Fri, Sep 19, 2025 at 5:34 PM Nuno Sá <noname.nuno@...il.com> wrote:
>
> Hi,
>
> Thanks for your patch.
>
> On Fri, 2025-09-19 at 00:37 +0530, Akshay Jindal wrote:
> > Refactor the generic interrupt configuration logic to improve readability
> > and reduce repetitive code. Replace hard-coded register values with macros
> > and enums, making it clearer what configuration is written to hardware.
> >
> > Introduce a const struct to map event direction to the corresponding
> > generic interrupt. This removes the need for switch statements in multiple
> > callbacks, including activity event setup, read_event_value, and
> > write_event_value, while still performing the selection at runtime.
> >
> > This change has no functional impact but simplifies the code and makes it
> > easier to maintain and extend in future updates.
> >
> > Signed-off-by: Akshay Jindal <akshayaj.lkd@...il.com>
> > ---
>
> You may be trying to refactor too much in one single patch. I would maybe think
> about splitting this into 2/3 logical changes.
>
> > drivers/iio/accel/bma400.h | 71 +++++++++++++++---
> > drivers/iio/accel/bma400_core.c | 128 ++++++++++++++++----------------
> > 2 files changed, 125 insertions(+), 74 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index 932358b45f17..ab7d1d139b66 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -68,7 +68,19 @@
> > #define BMA400_CMD_REG 0x7e
> >
> > /* Interrupt registers */
> > -#define BMA400_INT_CONFIG0_REG 0x1f
> > +#define BMA400_INT_CONFIG0_REG 0x1f
> > +#define BMA400_INT_CONFIG0_ORTN_CHG_MASK BIT(1)
> > +#define BMA400_INT_CONFIG0_GEN1_MASK BIT(2)
> > +#define BMA400_INT_CONFIG0_GEN2_MASK BIT(3)
> > +#define BMA400_INT_CONFIG0_FIFO_FULL_MASK BIT(5)
> > +#define BMA400_INT_CONFIG0_FIFO_WTRMRK_MASK BIT(6)
> > +#define BMA400_INT_CONFIG0_DRDY_MASK BIT(7)
> > +
> > +enum generic_intr {
> > + GEN1_INTR,
> > + GEN2_INTR
> > +};
> > +
> > #define BMA400_INT_CONFIG1_REG 0x20
> > #define BMA400_INT1_MAP_REG 0x21
> > #define BMA400_INT_IO_CTRL_REG 0x24
> > @@ -96,15 +108,53 @@
> > #define BMA400_ACC_ODR_MIN_HZ 12
> >
> > /* Generic interrupts register */
> > -#define BMA400_GEN1INT_CONFIG0 0x3f
> > -#define BMA400_GEN2INT_CONFIG0 0x4A
> > -#define BMA400_GEN_CONFIG1_OFF 0x01
> > -#define BMA400_GEN_CONFIG2_OFF 0x02
> > -#define BMA400_GEN_CONFIG3_OFF 0x03
> > -#define BMA400_GEN_CONFIG31_OFF 0x04
> > -#define BMA400_INT_GEN1_MSK BIT(2)
> > -#define BMA400_INT_GEN2_MSK BIT(3)
> > -#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> > +#define BMA400_GENINT_CONFIG_REG_BASE 0x3f
> > +#define BMA400_NUM_GENINT_CONFIG_REGS 11
> > +#define BMA400_GENINT_CONFIG_REG(gen_intr, config_idx) \
> > + (BMA400_GENINT_CONFIG_REG_BASE + \
> > + (gen_intr) * BMA400_NUM_GENINT_CONFIG_REGS + \
> > + (config_idx))
>
> Not sure the above macro helps that much. More on this below...
In my last patch regarding ltr390, I was given feedback towards
reducing the number of macro definitions. This may look little cryptic,
but it enables me to access GEN1(11) + GEN2 (11) registers with only
2 definitions. I see it as a reasonable tradeoff between readability and
code volume.
> > static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
> > {
> > switch (reg) {
> > @@ -1114,10 +1137,10 @@ static int bma400_read_event_config(struct iio_dev
> > *indio_dev,
> > case IIO_ACCEL:
> > switch (dir) {
> > case IIO_EV_DIR_RISING:
> > - return FIELD_GET(BMA400_INT_GEN1_MSK,
> > + return FIELD_GET(BMA400_INT_CONFIG0_GEN1_MASK,
> > data->generic_event_en);
>
> Like the above is above renaming defines in a more logical name. Not going to
> opinate whether this name is better or not but you could put that (and other
> similar changes) in one patch.
Initially, I thought of doing this, but if you look at the diff in bma400.h,
only GEN1_MASK and GEN2_MASK have been renamed and re-placed closer
to their register definition macro. Rest all of them are either new
introductions
or have gone for good. Hence I felt it might be an overkill to create
a new patch
just for 2 reg bit renaming. At the same time, I feel new
introductions should be
clubbed with their usage. Ofcourse, all of them are not being used, but even if
the code is using 1 bit of a reg, then all of its fields should be defined.
Due to the above logic, I kept it in a single patch. Although happy to change if
you still feel otherwise.
>
> > case IIO_EV_DIR_FALLING:
> > - return FIELD_GET(BMA400_INT_GEN2_MSK,
> > + return FIELD_GET(BMA400_INT_CONFIG0_GEN2_MASK,
> > data->generic_event_en);
> > case IIO_EV_DIR_SINGLETAP:
> > return FIELD_GET(BMA400_S_TAP_MSK,
> > @@ -1159,59 +1182,50 @@ static int bma400_activity_event_en(struct bma400_data
> > *data,
> > enum iio_event_direction dir,
> > int state)
> > {
> > - int ret, reg, msk, value;
> > - int field_value = 0;
> > -
> > - switch (dir) {
> > - case IIO_EV_DIR_RISING:
> > - reg = BMA400_GEN1INT_CONFIG0;
> > - msk = BMA400_INT_GEN1_MSK;
> > - value = 2;
> > - set_mask_bits(&field_value, BMA400_INT_GEN1_MSK,
> > - FIELD_PREP(BMA400_INT_GEN1_MSK, state));
> > - break;
> > - case IIO_EV_DIR_FALLING:
> > - reg = BMA400_GEN2INT_CONFIG0;
> > - msk = BMA400_INT_GEN2_MSK;
> > - value = 0;
> > - set_mask_bits(&field_value, BMA400_INT_GEN2_MSK,
> > - FIELD_PREP(BMA400_INT_GEN2_MSK, state));
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > + int ret, regval;
> > + u8 genintr = bma400_genintrs[dir].genintr;
> > + u8 detect_criterion = bma400_genintrs[dir].detect_mode;
> > + unsigned int intrmask = bma400_genintrs[dir].intrmask;
> >
>
> Hmm, you should still have the switch case. Again, not sure the const struct
> adds that much but I'm also fine with it. But I would do:
>
> switch (dir) {
> case IIO_EV_DIR_RISING:
> case IIO_EV_DIR_FALLING:
> genintr = bma400_genintrs[dir].genintr;
> detect_criterion = bma400_genintrs[dir].detect_mode;
> intrmask = bma400_genintrs[dir].intrmask;
> default:
> return -EINVAL;
>
Thank you for detailing it out. Yes this gives the best of both worlds.
Uses the centralized lookup struct as well as makes sure dir is sanitized.
Will wrap it up in a function and apply in the v2.
Thanks,
Akshay.
Powered by blists - more mailing lists