Re: [PATCH 2/3] iio: imu: bmi270: add step counter watermark event

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 26 Apr 2025 21:57:21 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> On Sat, Apr 26, 2025 at 02:47:39PM +0100, Jonathan Cameron wrote:
> > On Thu, 24 Apr 2025 21:14:51 -0300
> > Gustavo Silva <gustavograzs@gmail.com> wrote:
> >   
> > > Add support for generating events when the step counter reaches the
> > > configurable watermark.
> > > 
> > > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>  
> > 
> > Main thing in here is I think the event type isn't the right one.
> >  
> 
> Hi Jonathan,
> 
> Thanks for the review.
> I think the answers to your questions in this patch come down to me
> trying to keep this driver consistency with the bmi323 driver, since
> the two devices are very similar.
> However I have no objections to change the event type if you think it
> is appropriate.

Yeah. From the discussion with Lothar, it's clear we have some inconsistency
on these event types :(  Anyhow, I may well be wrong - see below.

> 
> > > @@ -119,6 +128,7 @@ struct bmi270_data {
> > >  	 /* Protect device's private data from concurrent access */
> > >  	struct mutex mutex;
> > >  	int steps_enabled;
> > > +	unsigned int feature_events;  
> > 
> > Why do we need this rather than just checking the register?
> >   
> Not really needed, I just tried to keep it similar to the bmi323 driver.

Generally I'd prefer drives to use regmap caching to get benefits of caching everything
appropriate rather than use their own local caches.


> > > +	return 0;
> > > +}
> > > +
> > >  static int bmi270_set_scale(struct bmi270_data *data, int chan_type, int uscale)
> > >  {
> > >  	int i;
> > > @@ -539,19 +585,32 @@ static irqreturn_t bmi270_irq_thread_handler(int irq, void *private)
> > >  {
> > >  	struct iio_dev *indio_dev = private;
> > >  	struct bmi270_data *data = iio_priv(indio_dev);
> > > -	unsigned int status;
> > > +	unsigned int status0, status1;
> > > +	s64 timestamp = iio_get_time_ns(indio_dev);
> > >  	int ret;
> > >  
> > >  	scoped_guard(mutex, &data->mutex) {
> > > +		ret = regmap_read(data->regmap, BMI270_INT_STATUS_0_REG,
> > > +				  &status0);
> > > +		if (ret)
> > > +			return IRQ_NONE;
> > > +
> > >  		ret = regmap_read(data->regmap, BMI270_INT_STATUS_1_REG,
> > > -				  &status);
> > > +				  &status1);
> > >  		if (ret)
> > >  			return IRQ_NONE;
> > >  	}
> > >  
> > > -	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status))
> > > +	if (FIELD_GET(BMI270_INT_STATUS_1_ACC_GYR_DRDY_MSK, status1))
> > >  		iio_trigger_poll_nested(data->trig);
> > >  
> > > +	if (FIELD_GET(BMI270_INT_STATUS_0_STEP_CNT_MSK, status0))
> > > +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
> > > +							     IIO_NO_MOD,  
> > why use IIO_MOD_EVENT_CODE() if not modified?
> >   
> My bad, I blindly followed what is implemented in the bmi323 driver.
> I'll fix it in v2.

It's not wrong as such, just that we have a more specific macro for this case.
> 
> > > +							     IIO_EV_TYPE_CHANGE,
> > > +							     IIO_EV_DIR_NONE),  
> > As below. This looks like a rising threshold event.
> > 
> > Change tends to be for things like activity detection (walking/standing etc)

I got this wrong.  Forgot about how this ABI was defined until reading it earlier
today for the discussion with Lothar!

> >    
> Using rising threshold event is fine by me, but then shouldn't we
> update the bmi323 driver as well?

If it is an event that occurs every step?  In that case CHANGE is correct.
If it is an event on a particular threshold of steps being passed.  I.e.
1000 steps, then a threshold is appropriate.   Vs every 1000 steps
in which case change is appropriate.  Seems from below comment it is
every N so this is fine as is. 

> 
> > > +			       timestamp);
> > > +
> > >  	return IRQ_HANDLED;
> > >  }
> > >  
> > > @@ -761,10 +820,111 @@ static int bmi270_read_avail(struct iio_dev *indio_dev,
> > >  	}
> > >  }
> > >
> > > +
> > > +static const struct iio_event_spec bmi270_step_wtrmrk_event = {
> > > +	.type = IIO_EV_TYPE_CHANGE,  
> > 
> > Change would be a per step event.
> > IIUC this is a rising threshold.
> >  
> Yeah, if the watermark level is configured to N steps, an event is
> generated every time the step counter counts N steps.

This is fine then as change.  My mistake!

Jonathan



> 
> > > +	.dir = IIO_EV_DIR_NONE,
> > > +	.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
> > > +			       BIT(IIO_EV_INFO_VALUE),
> > > +};  
> > 
> >