Re: [PATCH 3/6] iio: accel: bma220: add tap detection

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 1 Nov 2025 09:56:42 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Hello Jonathan,
> 
> thank you for the review.
> 
> On Sat, Oct 18, 2025 at 06:16:32PM +0100, Jonathan Cameron wrote:
> > > +			ret = regmap_read(data->regmap, BMA220_REG_CONF3, &reg_val);
> > > +			if (ret)
> > > +				return ret;
> > > +			*val = FIELD_GET(BMA220_TT_DUR_MSK, reg_val);  
> > 
> > This needs to be in second if you are using duration. Is the register really in seconds?  
> 
> this IC has a very small number of bits that configure
> duration/hysteresis/threshold levels. it's between 2 and 6 for each
> of them. in the case of high and low G events the duration is not
> even directly defined as a time interval, but as a count of samples
> that are over a threshold value.

The ABI is in seconds, so you have to deal with scaling wrt to the sampling
frequency at the time.  I know it can be a pain to do, but consistent userspace
is the aim and so we need to match the ABI.

> 
> I was hoping that simply passing along a unitless value between 0 and
> parameter_max would be enough to customize all the event parameters.
> this does mean that the driver makes the assumption that the user is
> familiar with the device datasheet and knows the number of bits every
> parameter has been allocated. should the driver provide a conversion
> table for tt_duration just like for _scale_table and
> _lpf_3dB_freq_Hz_table?

Exactly.

> 
> > > @@ -506,13 +777,36 @@ static irqreturn_t bma220_irq_handler(int
> > > irq, void *private) struct bma220_data *data =
> > > iio_priv(indio_dev); int ret;
> > >  	unsigned int bma220_reg_if1;
> > > +	s64 timestamp = iio_get_time_ns(indio_dev);
> > > +
> > > +	guard(mutex)(&data->lock);
> > >  
> > >  	ret = regmap_read(data->regmap, BMA220_REG_IF1,
> > > &bma220_reg_if1); if (ret)
> > >  		return IRQ_NONE;
> > >  
> > > -	if (FIELD_GET(BMA220_IF_DRDY, bma220_reg_if1))
> > > +	if (FIELD_GET(BMA220_IF_DRDY, bma220_reg_if1)) {  
> > 
> > Is it an either / or case? I.e. we can only have buffered reads with
> > the data ready interrupt or events?   That does happen in some
> > devices but is fairly unusual.  
> 
> the driver got an interrupt, so it checks the source - it's either a
> data ready when the sensor is used to sample the environment or it's
> an event in which case it just sets the event. now that you mention
> it I think I would miss events if both happened before the kernel
> executes the _irq_handler(), so I will rewrite this bit. if you ment
> something else please tell me.

That was exactly what I was getting at.  Given both occur on a particular
sample, it is guaranteed this will sometimes happen.

Jonathan

> 
> best regards,
> peter
> 
> >   
> > >  		iio_trigger_poll_nested(data->trig);
> > > +		return IRQ_HANDLED;
> > > +	}
> > > +
> > > +	if (FIELD_GET(BMA220_IF_TT, bma220_reg_if1)) {
> > > +
> > > +		if (data->tap_type == BMA220_TAP_TYPE_SINGLE)
> > > +			iio_push_event(indio_dev,
> > > +
> > > IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > +
> > > IIO_MOD_X_OR_Y_OR_Z,
> > > +
> > > IIO_EV_TYPE_GESTURE,
> > > +
> > > IIO_EV_DIR_SINGLETAP),
> > > +				       timestamp);
> > > +		else
> > > +			iio_push_event(indio_dev,
> > > +
> > > IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > +
> > > IIO_MOD_X_OR_Y_OR_Z,
> > > +
> > > IIO_EV_TYPE_GESTURE,
> > > +
> > > IIO_EV_DIR_DOUBLETAP),
> > > +				       timestamp);
> > > +	}
> > >  
> > >  	return IRQ_HANDLED;
> > >  }
> > >   
> >   
>