On Mon, 23 Mar 2020 12:45:43 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> 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;
> > > > +}
>
