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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 29 Apr 2023 16:56:38 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

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

Separation between what should be done in preenable and postenable has been
vague for a long time. These days only really matters if you need to
order them wrt updating the scan mode I think.

> 
> > 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!

Agreed that these allocations should be taken out of this hot path.
Either of these options should work so up to you.

> 
> Yours,
> 	-- Matti
> 
>