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

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: <yudy5vbwblqzkx34pgekqi3noyctaxo77n2lb6mqidb4veqadm@534j4towopou>
Date: Sat, 26 Apr 2025 21:57:21 -0300
From: Gustavo Silva <gustavograzs@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Alex Lanzano <lanzano.alex@...il.com>, 
	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
Subject: Re: [PATCH 2/3] iio: imu: bmi270: add step counter watermark event

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@...il.com> wrote:
> 
> > Add support for generating events when the step counter reaches the
> > configurable watermark.
> > 
> > Signed-off-by: Gustavo Silva <gustavograzs@...il.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.

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

> 
> > +
> > +static int bmi270_step_wtrmrk_en(struct bmi270_data *data, bool state)
> > +{
> > +	int ret, reg, field_value;
> > +
> > +	guard(mutex)(&data->mutex);
> > +	if (!data->steps_enabled)
> > +		return -EINVAL;
> > +
> > +	reg = bmi270_int_map_reg(data->irq_pin);
> > +	if (reg < 0)
> > +		return -EINVAL;
> > +
> > +	field_value = FIELD_PREP(BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, state);
> > +	ret = regmap_update_bits(data->regmap, reg,
> > +				 BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK,
> > +				 field_value);
> > +	if (ret)
> > +		return ret;
> > +
> > +	set_mask_bits(&data->feature_events,
> > +		      BMI270_INT_MAP_FEAT_STEP_CNT_WTRMRK_MSK, field_value);
> 
> Given we wrote the register, why do we need a cached value?  Can't we just read
> it back again (or rely on a regmap cache for it if enabled in this driver)
>
Ditto.

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

> > +							     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)
>  
Using rising threshold event is fine by me, but then shouldn't we
update the bmi323 driver as well?

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

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