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

[Petre Rodan <petre.rodan@subdimension.ro>]

[-- Attachment #1: Type: text/plain, Size: 2954 bytes --]


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.

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?

> > @@ -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.

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;
> >  }
> > 
> 

-- 
petre rodan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]