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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aQW9OnJSrOzn_Sws@lipo.home.arpa>
Date: Sat, 1 Nov 2025 09:56:42 +0200
From: Petre Rodan <petre.rodan@...dimension.ro>
To: Jonathan Cameron <jic23@...nel.org>
Cc: David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Jagath Jog J <jagathjog1996@...il.com>
Subject: Re: [PATCH 3/6] iio: accel: bma220: add tap detection

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)