Re: [PATCH v3] iio: accel: Add support for the Bosch-Sensortec BMI088

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 23 Mar 2020 12:45:43 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Mar 23, 2020 at 10:21:07AM +0100, Mike Looijmans wrote:
> > On 22-03-2020 01:16, Andy Shevchenko wrote:  
> > > On Thu, Mar 19, 2020 at 04:48:42PM +0100, Mike Looijmans wrote:  
> 
> ...
> 
> > > > +static int bmi088_accel_get_temp(struct bmi088_accel_data *data, int *val)
> > > > +{
> > > > +	int ret;
> > > > +	__s16 temp;
> > > > +
> > > > +	mutex_lock(&data->mutex);  
> > >   
> > > > +	ret = regmap_bulk_read(data->regmap, BMI088_ACCEL_REG_TEMP,
> > > > +			       &data->buffer, 2);  
> > > 
> > > sizeof() ?  
> > 
> > The buffer is a shared buffer, it will grow to accommodate reading all axis
> > and timestamp in a single read (9 bytes) and for FIFO reads in foreseeable
> > future.
> > 
> > I could use sizeof(temp) here though, but that wouldn't that be more
> > confusing?  
> 
> Yeah, perhaps comment explaining why 2 is being used there and why you write
> directly to the buffer (no temporary variable being involved)?

This is all about ensuring it's a dma safe buffer without having to explicit
kmallocs on eveyr read.  buffer is the temporary variable.

I would suggest using sizeof(__be16) which would make it clearer perhaps.

Jonathan


> 
> > > > +	temp = get_unaligned_be16(data->buffer);
> > > > +
> > > > +	mutex_unlock(&data->mutex);
> > > > +
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	*val = temp >> 5;
> > > > +
> > > > +	return IIO_VAL_INT;
> > > > +}  
>