On Thu, 5 Sep 2024 23:32:47 +0900 gyeyoung <gye976@xxxxxxxxx> wrote: > Thank you for your response. I understand what you mean. > > But I think it's better to have 'wom_bits' in 'inv_mpu6050_state', > because the 'wom_bits' variable is only a variable for bit operation > with the 'INT_STATUS' register, not an actual register in register > manual. > Isn't it? I'd like it somewhere static const. Either in the register map (which indeed currently only has register addresses) or in the hw_info structures. I'd prefer the reg_map. The current handling as 'code' based on the ID should be replaced with data. Jonathan > > Thanks. > > On Thu, Sep 5, 2024 at 6:30 PM Jean-Baptiste Maneyrol > <Jean-Baptiste.Maneyrol@xxxxxxx> wrote: > > > > Hello, > > > > nice improvement, thanks. > > > > But beware there is a fix pending in fixes-togreg branch and missing in testing branch that is changing this part of code. > > To avoid a painful merge, it should be better to wait for the fix to be integrated inside testing. > > > > Is that correct Jonathan? > > > > And I would prefer the wom_bits being inside the inv_mpu6050_reg_map structure. > > > > Thanks, > > JB > > > > ________________________________________ > > From: Gyeyoung Baek <gye976@xxxxxxxxx> > > Sent: Tuesday, September 3, 2024 18:33 > > To: jic23@xxxxxxxxxx <jic23@xxxxxxxxxx> > > Cc: linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; Gyeyoung Baek <gye976@xxxxxxxxx> > > Subject: [PATCH] iio: imu: inv_mpu6050: Move setting 'wom_bits' to probe function > > > > This Message Is From an Untrusted Sender > > You have not previously corresponded with this sender. > > > > 'wom_bits' variable is defined by chip type, > > and chip type is statically defined by device tree. > > so 'wom_bits' need to be set once during probe function. > > > > but before code set it every time using 'switch statement' during > > threaded irq handler, so i move that to probe function. > > > > Signed-off-by: Gyeyoung Baek <gye976@xxxxxxxxx> > > --- > > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 16 +++++++++++++++ > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 1 + > > drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 20 ++----------------- > > 3 files changed, 19 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > > index 14d95f34e981..322ae664adc0 100644 > > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > > @@ -2076,6 +2076,22 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name, > > return result; > > } > > > > + switch (chip_type) { > > + case INV_MPU6050: > > + case INV_MPU6500: > > + case INV_MPU6515: > > + case INV_MPU6880: > > + case INV_MPU6000: > > + case INV_MPU9150: > > + case INV_MPU9250: > > + case INV_MPU9255: > > + st->wom_bits = INV_MPU6500_BIT_WOM_INT; > > + break; > > + default: > > + st->wom_bits = INV_ICM20608_BIT_WOM_INT; > > + break; > > + } > > + > > return 0; > > > > error_power_off: > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h > > index e1c0c5146876..a91b9c2b26e4 100644 > > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h > > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h > > @@ -212,6 +212,7 @@ struct inv_mpu6050_state { > > bool level_shifter; > > u8 *data; > > s64 it_timestamp; > > + unsigned int wom_bits; > > }; > > > > /*register and associated bit definition*/ > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c > > index 84273660ca2e..b19556df1801 100644 > > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c > > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c > > @@ -243,26 +243,10 @@ static irqreturn_t inv_mpu6050_interrupt_handle(int irq, void *p) > > { > > struct iio_dev *indio_dev = p; > > struct inv_mpu6050_state *st = iio_priv(indio_dev); > > - unsigned int int_status, wom_bits; > > + unsigned int int_status; > > u64 ev_code; > > int result; > > > > - switch (st->chip_type) { > > - case INV_MPU6050: > > - case INV_MPU6500: > > - case INV_MPU6515: > > - case INV_MPU6880: > > - case INV_MPU6000: > > - case INV_MPU9150: > > - case INV_MPU9250: > > - case INV_MPU9255: > > - wom_bits = INV_MPU6500_BIT_WOM_INT; > > - break; > > - default: > > - wom_bits = INV_ICM20608_BIT_WOM_INT; > > - break; > > - } > > - > > scoped_guard(mutex, &st->lock) { > > /* ack interrupt and check status */ > > result = regmap_read(st->map, st->reg->int_status, &int_status); > > @@ -272,7 +256,7 @@ static irqreturn_t inv_mpu6050_interrupt_handle(int irq, void *p) > > } > > > > /* handle WoM event */ > > - if (st->chip_config.wom_en && (int_status & wom_bits)) { > > + if (st->chip_config.wom_en && (int_status & st->wom_bits)) { > > ev_code = IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X_OR_Y_OR_Z, > > IIO_EV_TYPE_ROC, IIO_EV_DIR_RISING); > > iio_push_event(indio_dev, ev_code, st->it_timestamp); > > -- > > 2.34.1 > > > >