Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/29/23 15:59, Mehdi Djait wrote:
Hi Matti,

On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote:
On 4/25/23 10:24, Mehdi Djait wrote:
Hi Matti,

On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote:
On 4/25/23 01:22, Mehdi Djait wrote:
Add the chip_info structure to the driver's private data to hold all
the device specific infos.
Refactor the kx022a driver implementation to make it more generic and
extensible.

Signed-off-by: Mehdi Djait <mehdi.djait.k@xxxxxxxxx>
---
v3:
- added the change of the buffer's allocation in the __kx022a_fifo_flush
     to this patch
- added the chip_info to the struct kx022a_data

v2:
- mentioned the introduction of the i2c_device_id table in the commit
- get i2c_/spi_get_device_id only when device get match fails
- removed the generic KX_define
- removed the kx022a_device_type enum
- added comments for the chip_info struct elements
- fixed errors pointed out by the kernel test robot

    drivers/iio/accel/kionix-kx022a-i2c.c |  15 +++-
    drivers/iio/accel/kionix-kx022a-spi.c |  15 +++-
    drivers/iio/accel/kionix-kx022a.c     | 114 +++++++++++++++++---------
    drivers/iio/accel/kionix-kx022a.h     |  54 +++++++++++-
    4 files changed, 147 insertions(+), 51 deletions(-)

diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index 8f23631a1fd3..ce299d0446f7 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -15,6 +15,7 @@

...


    static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
@@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
    {
    	struct kx022a_data *data = iio_priv(idev);
    	struct device *dev = regmap_get_device(data->regmap);
-	__le16 buffer[KX022A_FIFO_LENGTH * 3];
+	__le16 *buffer;
    	uint64_t sample_period;
    	int count, fifo_bytes;
    	bool renable = false;
    	int64_t tstamp;
    	int ret, i;
+	buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;

Do you think we could get rid of allocating and freeing the buffer for each
flush? I feel it is a bit wasteful, and with high sampling frequencies this
function can be called quite often. Do you think there would be a way to
either use stack (always reserve big enough buffer no matter which chip we
have - or is the buffer too big to be safely taken from the stack?), or a
buffer stored in private data and allocated at probe or buffer enable?

I tried using the same allocation as before but a device like the KX127
has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a).
Allocating this much using the stack will result in a Warning.


Right. Maybe you could then have the buffer in private-data and allocate it
in buffer pre-enable? Do you think that would work?

Do you mean add a new function kx022a_buffer_preenable to iio_buffer_setup_ops ?

Sorry. I thought the kx022a already implemented the pre-enable callback but it was the postenable. I was mistaken.

Would adding the allocation to kx022a_fifo_enable and the free to
kx022a_fifo_disable be a good option also ?
Yes. I think that should work!

Yours,
	-- Matti


--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux