Re: [PATCH v4 5/6] iio: imu: adis16550: add adis16550 support

["Nuno Sá" <noname.nuno@gmail.com>]

On Sun, 2025-01-12 at 16:11 +0000, Jonathan Cameron wrote:
> On Fri, 10 Jan 2025 09:42:53 +0200
> Robert Budai <robert.budai@analog.com> wrote:
> 
> > The ADIS16550 is a complete inertial system that includes a triaxis
> > gyroscope and a triaxis accelerometer. Each inertial sensor in
> > the ADIS16550 combines industry leading MEMS only technology
> > with signal conditioning that optimizes dynamic performance. The
> > factory calibration characterizes each sensor for sensitivity, bias,
> > and alignment. As a result, each sensor has its own dynamic com-
> > pensation formulas that provide accurate sensor measurements
> > 
> > Co-developed-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> > Co-developed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > Signed-off-by: Robert Budai <robert.budai@analog.com>
> > ---
> > 
> > 4:
> > - reorganized channels to match the order in the datasheet
> > - removed extra checks and goto statements
> > - for buffer memory allocation used only kfree, since adis library already
> > takes care of freeing the buffer
> 
> That last bit makes for a mess wrt to who owns the buffer and lifetime
> management. Suggestions inline.
> 
> 

...

> > +
> > +	/*
> > +	 * Allocate the xfer and buffer for the burst read operation. The
> > buffer
> > +	 * is used to store the burst data and the xfer is used to send the
> > burst
> > +	 * command and receive the data. On device remove the adis libraary
> > is going
> 
> library.
> 
> > +	 * to free the xfer and buffer.
> 
> That is a horrible lifetime control.  They should either be allocated in the
> library
> and freed in the library or allocated and freed in the driver.  Mixing that
> doesn't
> make sense.

Agreed. TBH, I did not remembered at all to suggest ignoring the library and let
it call kfree() on NULL. I also don't like that much of how things are done in
the lib but I don't think it would be that straight to change things for the non
'burst mode' cases. For the devices implementing 'burst mode' doing the free +
allocation on every update_scan_modes seems unnecessary. Anyways, maybe
something worth looking at in the future.

- Nuno Sá