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

From: Jonathan Cameron <jic23@kernel.org>
To: Angel Iglesias <ang.iglesiasg@gmail.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>,
	linux-iio <linux-iio@vger.kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Paul Cercueil <paul@crapouillou.net>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/5] iio: pressure: bmp280: Fix alignment for DMA safety
Date: Sun, 14 Aug 2022 14:52:49 +0100	[thread overview]
Message-ID: <20220814145249.701f1261@jic23-huawei> (raw)
In-Reply-To: <ba71ba74e9115bebce82a2afbd5d62a2e4ecf666.camel@gmail.com>

On Fri, 12 Aug 2022 11:59:50 +0200
Angel Iglesias <ang.iglesiasg@gmail.com> wrote:

> 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@gmail.com> 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?  

I think the answer to this is which callback is set in the chip_data structure
defines which one of the calibration buffers is in use for that purpose.
The one used for everything else is defined by the code path, not an explicit
variable.

There might be some (I haven't looked) but lock protection is unnecessary as
_cal buffers used before we register the device so there is no concurrency yet.

> 
> 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?
yes.  That seems a good idea.

It's worth giving unions a name if you are using them such that you write via
one element and read via another (e.g. for type conversion) but where it's really
just a case of potential space saving like this, nice to use an anonymous union
and shorten all the lines accessing the elements.

> 
> > >  };  
> > 
> > ...
> >   
> > > +       /* 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.

Agreed. I was thinking the same thing.  Pulling out that conversion as a precursor
nop cleanup will make all the subsequent changes more readable. Great!

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

I'm not particularly fussy about reverse xmas tree so wouldn't normally
advocate a cleanup patch just to reorder local variable definitions.
However, I think in this case it would be good to have such a precursor
patch so as to make the ordering more logical for the additions made
by the rest of the series.

Thanks,

Jonathan

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