Re: [PATCH v5 2/5] iio: pressure: bmp280: Fix alignment for DMA safety

[Angel Iglesias]

On lun, 2022-08-08 at 10:53 +0200, Andy Shevchenko wrote:
> On Sun, Aug 7, 2022 at 1:56 PM Angel Iglesias <ang.iglesiasg@xxxxxxxxx> wrote:
> > 
> > Adds DMA-safe buffers to driver data struct to store raw data from sensors
> > 
> > The multiple buffers used thorough the driver share the same memory
> > allocated as part of the device data instance. The union containing
> > the buffers is aligned to allow safe usage with DMA operations, such
> > as regmap bulk read calls.
> 
> ...
> 
> >  #include <linux/completion.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/random.h>
> 
> + Blank line.
> 
> > +#include <asm/unaligned.h>
> 
> ...
> 
> > +       union {
> > +               /* sensor data buffer */
> > +               u8 data[3];
> > +               /* calibration data buffers */
> > +               __le16 bmp280_cal[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> > +               __be16 bmp180_cal[BMP180_REG_CALIB_COUNT / 2];
> > +       } buf __aligned(IIO_DMA_MINALIGN);
> 
> Hmm... And which field in the struct defines which of the buffers is being
> used?

There's no concurrent use of the buffers. Calibration data is read during the
initialization of the sensor. The data buffer is then used to store the raw data
read from the measurement regs, and is also used a few times to read a the humid
calibration on BME280, but again, in a sequential, non concurrent manner.

Regarding which calibration buffer is used, is the same situation as the
calibration data union, helper functions and callback for the sensor use the
buffer reserved for the sensor. I don't know if this is the best approach, I
just followed what I saw previously on this drivers and others from IIO
subsystem.

> Also, do you need a non-anonymous union?

No I could use an anonymous function. Should I change it to an anonymous union?

> >  };
> 
> ...
> 
> > +       /* parse temperature calibration data */
> 
> Be consistent! Check all your patches for the consistency (comments,
> other code style, etc).
> 
> ...
> 
> > +       calib->H5 = sign_extend32(((get_unaligned_le16(data->buf.data) >> 4)
> > & 0xfff), 11);
> 
> (It's not directly related to this change, but good to ask)
> Are you going to change all those masks to use GENMASK()?

I thought I made sense refresh previous code on the driver to use GENMASK() and
FIELD_PREP and FIELD_GET helpers to use the same standards on the BMP380
codepath. Having in mind other feedback you gave me on this iteration, this
GENMASK() and FIELD_PREP/FIELD_GET changes make more sense in a prerequisite
patch and not as part of patch 1.

> ...
> 
> > +       struct bmp180_calib *calib = &data->calib.bmp180;
> >         int ret;
> >         int i;
> > -       struct bmp180_calib *calib = &data->calib.bmp180;
> 
> Exactly my point given the previous patch, now you have a ping-pong
> style of changes: the introduced line in the series is being touched
> again in the very same series without any need.

Yup, apologies. I'll be more careful

> ...
> 
> >         u8 oss = data->oversampling_press;
> > +       int ret;
> 
> Ditto.
>