>From: Jonathan Cameron <jic23@xxxxxxxxxx>
>Sent: Sunday, March 3, 2024 18:10
>To: INV Git Commit <INV.git-commit@xxxxxxx>
>Cc: lars@xxxxxxxxxx <lars@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@xxxxxxx>
>Subject: Re: [PATCH 3/4] iio: imu: inv_mpu6050: add new interrupt handler for WoM events
>
>On Sun, 25 Feb 2024 16: 00: 26 +0000 inv. git-commit@ tdk. com wrote: > From: Jean-Baptiste Maneyrol <jean-baptiste. maneyrol@ tdk. com> > > Add new interrupt handler for generating WoM event from int status > register bits. Launch
>ZjQcmQRYFpfptBannerStart
>This Message Is From an External Sender
>This message came from outside your organization.
>
>ZjQcmQRYFpfptBannerEnd
>On Sun, 25 Feb 2024 16:00:26 +0000
>inv.git-commit@xxxxxxx wrote:
>
>> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>>
>> Add new interrupt handler for generating WoM event from int status
>> register bits. Launch from interrupt the trigger poll function for
>> data buffer.
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>> ---
>> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 2 +
>> drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 11 ----
>> drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 56 +++++++++++++++++--
>> 3 files changed, 53 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> index 519c1eee96ad..9be67cebbd49 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
>> @@ -184,6 +184,7 @@ struct inv_mpu6050_hw {
>> * @magn_orient: magnetometer sensor chip orientation if available.
>> * @suspended_sensors: sensors mask of sensors turned off for suspend
>> * @data: read buffer used for bulk reads.
>> + * @it_timestamp: interrupt timestamp.
>> */
>> struct inv_mpu6050_state {
>> struct mutex lock;
>> @@ -209,6 +210,7 @@ struct inv_mpu6050_state {
>> unsigned int suspended_sensors;
>> bool level_shifter;
>> u8 *data;
>> + s64 it_timestamp;
>> };
>>
>> /*register and associated bit definition*/
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> index 13da6f523ca2..e282378ee2ca 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
>> @@ -51,21 +51,10 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>> u32 fifo_period;
>> s64 timestamp;
>> u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>> - int int_status;
>> size_t i, nb;
>>
>> mutex_lock(&st->lock);
>>
>> - /* ack interrupt and check status */
>> - result = regmap_read(st->map, st->reg->int_status, &int_status);
>> - if (result) {
>> - dev_err(regmap_get_device(st->map),
>> - "failed to ack interrupt\n");
>> - goto flush_fifo;
>> - }
>> - if (!(int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT))
>> - goto end_session;
>> -
>> if (!(st->chip_config.accl_fifo_enable |
>> st->chip_config.gyro_fifo_enable |
>> st->chip_config.magn_fifo_enable))
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> index ec2398a87f45..7ffbb9e7c100 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
>> @@ -6,6 +6,7 @@
>> #include <linux/pm_runtime.h>
>>
>> #include <linux/iio/common/inv_sensors_timestamp.h>
>> +#include <linux/iio/events.h>
>>
>> #include "inv_mpu_iio.h"
>>
>> @@ -223,6 +224,52 @@ static const struct iio_trigger_ops inv_mpu_trigger_ops = {
>> .set_trigger_state = &inv_mpu_data_rdy_trigger_set_state,
>> };
>>
>> +static irqreturn_t inv_mpu6050_interrupt_timestamp(int irq, void *p)
>> +{
>> + struct iio_dev *indio_dev = p;
>> + struct inv_mpu6050_state *st = iio_priv(indio_dev);
>> +
>> + st->it_timestamp = iio_get_time_ns(indio_dev);
>> +
>> + return IRQ_WAKE_THREAD;
>
>I think you can use iio_pollfunc_store_time().
The problem here is that interrupt can happen without buffer/trigger on, when only WoM is on.
In my understanding, poll function is not attached when buffer is off and poll function is NULL.
>
>> +}
>> +
>> +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 = 0;
>> + int result;
>> +
>> + mutex_lock(&st->lock);
>> +
>> + /* ack interrupt and check status */
>> + result = regmap_read(st->map, st->reg->int_status, &int_status);
>> + if (result) {
>> + dev_err(regmap_get_device(st->map),
>> + "failed to ack interrupt\n");
>> + goto exit_unlock;
>> + }
>> +
>> + /* handle WoM event */
>> + if (st->chip_config.wom_en && (int_status & INV_MPU6500_BIT_WOM_INT))
>> + iio_push_event(indio_dev,
>> + IIO_UNMOD_EVENT_CODE(IIO_ACCEL, 0, IIO_EV_TYPE_MAG_ADAPTIVE,
>> + IIO_EV_DIR_RISING),
>Maybe should be modified.
>
>Hmm. Is this magnitude of the overall acceleration or is it a per channel thing?
>
>If it's overall then we need to know how they are combined to describe this right.
>If it's an X or Y or Z thing we do have a modifier for that though I kind of
>regret adding that and wish now we'd just always reported 3 events.
>(problem with those X_OR_Y_OR_Z modifiers is they don't generalize well)
>
This an OR of the 3 axes. We can use X_OR_Y_OR_Z then.
>> + st->it_timestamp);
>> +
>> +exit_unlock:
>> + mutex_unlock(&st->lock);
>> +
>> + /* handle raw data interrupt */
>> + if (int_status & INV_MPU6050_BIT_RAW_DATA_RDY_INT) {
>> + indio_dev->pollfunc->timestamp = st->it_timestamp;
>
>As above, why not use the standard function for this.
>I don't think anything will have cleared it.
We need to use the timestamp taken in the hard interrupt handler, that's why I am not calling iio_pollfunc_store_time here.
And I am not calling it in the hard handler because in my understanding poll function is not attached when buffer is off.
>
>> + iio_trigger_poll_nested(st->trig);
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type)
>> {
>> int ret;
>> @@ -235,11 +282,10 @@ int inv_mpu6050_probe_trigger(struct iio_dev *indio_dev, int irq_type)
>> if (!st->trig)
>> return -ENOMEM;
>>
>> - ret = devm_request_irq(&indio_dev->dev, st->irq,
>> - &iio_trigger_generic_data_rdy_poll,
>> - irq_type,
>> - "inv_mpu",
>> - st->trig);
>> + ret = devm_request_threaded_irq(&indio_dev->dev, st->irq,
>> + &inv_mpu6050_interrupt_timestamp,
>> + &inv_mpu6050_interrupt_handle,
>> + irq_type, "inv_mpu", indio_dev);
>> if (ret)
>> return ret;
>>
>
