| Message ID | 20240903163302.105268-1-gye976@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: imu: inv_mpu6050: Move setting 'wom_bits' to probe function | expand |
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
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? Thanks. On Thu, Sep 5, 2024 at 6:30 PM Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> 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@gmail.com> > Sent: Tuesday, September 3, 2024 18:33 > To: jic23@kernel.org <jic23@kernel.org> > Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Gyeyoung Baek <gye976@gmail.com> > 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@gmail.com> > --- > 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 > >
On Thu, 5 Sep 2024 09:29:58 +0000 Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> 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? Yes. It is too late for new cleanups to hit this cycle anyway, so better to just wait for that fix to be in my togreg branch (6.12-rc1 probably) > > And I would prefer the wom_bits being inside the inv_mpu6050_reg_map structure. > > Thanks, > JB > > ________________________________________ > From: Gyeyoung Baek <gye976@gmail.com> > Sent: Tuesday, September 3, 2024 18:33 > To: jic23@kernel.org <jic23@kernel.org> > Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Gyeyoung Baek <gye976@gmail.com> > 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@gmail.com> > --- > 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);
On Thu, 5 Sep 2024 23:32:47 +0900 gyeyoung <gye976@gmail.com> 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@tdk.com> 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@gmail.com> > > Sent: Tuesday, September 3, 2024 18:33 > > To: jic23@kernel.org <jic23@kernel.org> > > Cc: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Gyeyoung Baek <gye976@gmail.com> > > 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@gmail.com> > > --- > > 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 > > > >
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);
'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@gmail.com> --- 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(-)