Re: [PATCH 1/3] iio: imu: bmi270: add channel for step counter

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 26 Apr 2025 21:19:13 -0300
Gustavo Silva <gustavograzs@gmail.com> wrote:

> On Sat, Apr 26, 2025 at 02:40:20PM +0100, Jonathan Cameron wrote:
> > On Thu, 24 Apr 2025 21:14:50 -0300
> > Gustavo Silva <gustavograzs@gmail.com> wrote:
> >   
> > > Add a channel for enabling/disabling the step counter, reading the
> > > number of steps and resetting the counter.
> > > 
> > > Signed-off-by: Gustavo Silva <gustavograzs@gmail.com>  
> > Hi Gustavo,
> > 
> > This is tripping over the somewhat theoretical requirement for
> > regmap to be provided with DMA safe buffers for bulk accesses.
> > 
> > Jonathan
> >   
> 
> Hi Jonathan,
> 
> Thanks for the review. I've got a few questions inline.
> 
> > > ---
> > >  drivers/iio/imu/bmi270/bmi270_core.c | 127 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 127 insertions(+)
> > > 
> > > diff --git a/drivers/iio/imu/bmi270/bmi270_core.c b/drivers/iio/imu/bmi270/bmi270_core.c
> > > index a86be5af5ccb1f010f2282ee31c47f284c1bcc86..f09d8dead9df63df5ae8550cf473b5573374955b 100644
> > > --- a/drivers/iio/imu/bmi270/bmi270_core.c
> > > +++ b/drivers/iio/imu/bmi270/bmi270_core.c
> > > @@ -31,6 +31,8 @@  
> >   
> > >  /* See datasheet section 4.6.14, Temperature Sensor */
> > >  #define BMI270_TEMP_OFFSET				11776
> > >  #define BMI270_TEMP_SCALE				1953125
> > > @@ -111,6 +118,7 @@ struct bmi270_data {
> > >  	struct iio_trigger *trig;
> > >  	 /* Protect device's private data from concurrent access */
> > >  	struct mutex mutex;
> > > +	int steps_enabled;  
> > 
> > Seems a little odd to have a thing called _enabled as an integer.
> > Probably better as a bool even though that will require slightly more
> > code to handle read / write.
> >   
> I agree that a bool might be more appropriate in this case. I decided to
> use an int to keep consistency with other drivers, specifically bma400
> and the iio dummy driver.
> If you prefer, I'll use a bool here and after this series submit a patch
> updating those drivers as well.

Yes. I think that would be a nice little logical improvement here and in those
drivers.  Not particularly critical though!

> 
> >   
> > >  
> > >  	/*
> > >  	 * Where IIO_DMA_MINALIGN may be larger than 8 bytes, align to
> > > @@ -282,6 +290,99 @@ static const struct  bmi270_odr_item bmi270_odr_table[] = {
> > >  	},
> > >  };
> > >  
> > > +enum bmi270_feature_reg_id {
> > > +	BMI270_SC_26_REG,
> > > +};
> > > +
> > > +struct bmi270_feature_reg {
> > > +	u8 page;
> > > +	u8 addr;
> > > +};
> > > +
> > > +static const struct bmi270_feature_reg bmi270_feature_regs[] = {
> > > +	[BMI270_SC_26_REG] = {
> > > +		.page = 6,
> > > +		.addr = 0x32,
> > > +	},
> > > +};
> > > +
> > > +static int bmi270_write_feature_reg(struct bmi270_data *data,
> > > +				    enum bmi270_feature_reg_id id,
> > > +				    u16 val)
> > > +{
> > > +	const struct bmi270_feature_reg *reg = &bmi270_feature_regs[id];
> > > +	int ret;
> > > +
> > > +	ret = regmap_write(data->regmap, BMI270_FEAT_PAGE_REG, reg->page);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return regmap_bulk_write(data->regmap, reg->addr, &val, sizeof(val));  
> > 
> > For a regmap on top of an SPI bus. I think we are still requiring DMA safe
> > buffers until we can get confirmation that the API guarantees that isn't
> > needed.  (there is another thread going on where we are trying to get that
> > confirmation).
> > 
> > That is a pain here because this can run concurrently with buffered
> > capture and as such I think we can't just put a additional element after
> > data->data but instead need to mark that new element __aligned(IIO_DMA_MINALIGN)
> > as well (and add a comment that it can be used concurrently with data->data).
> >  
> Just to clarify, when you say data->data, are you referring to the
> bmi270_data::buffer variable? That used to be called 'data' but it was
> changed to 'buffer' in commit 16c94de2a.

Yes.  The one marked __aligned(IIO_DMA_MINALIGN)
I was looking at old code I guess!

Jonathan