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] [day] [month] [year] [list]

Message-ID: <20251102122053.49ee2632@jic23-huawei>
Date: Sun, 2 Nov 2025 12:20:53 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Petre Rodan <petre.rodan@...dimension.ro>
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

On Sat, 1 Nov 2025 09:56:42 +0200
Petre Rodan <petre.rodan@...dimension.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;
> > >  }
> > >   
> >   
>