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

On Sun, 2025-01-12 at 16:11 +0000, Jonathan Cameron wrote:

On Fri, 10 Jan 2025 09:42:53 +0200
Robert Budai [off-list ref] wrote:

quoted

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.

...

quoted

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

quoted

+	 * 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á