Messages in this thread | | | Date | Sat, 29 Apr 2023 16:56:38 +0300 | Subject | Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add chip_info structure | From | Matti Vaittinen <> |
| |
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@gmail.com> >>>>> --- >>>>> 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! ~~
| |