Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <64728e90-48a7-43d0-b3d3-bfceb94884d7@gmail.com>
Date:   Sat, 29 Apr 2023 16:56:38 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v3 5/7] iio: accel: kionix-kx022a: Refactor driver and add
 chip_info structure
Content-Language: en-US, en-GB
To:     Mehdi Djait <mehdi.djait.k@gmail.com>
Cc:     jic23@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        andriy.shevchenko@linux.intel.com, robh+dt@kernel.org,
        lars@metafoo.de, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
References: <cover.1682373451.git.mehdi.djait.k@gmail.com>
 <bf0269aff66483f2323914170707203749b33f0f.1682373451.git.mehdi.djait.k@gmail.com>
 <867ac7b4-b666-854f-69f7-2d7d7d92c94e@gmail.com> <ZEeAGN3TBcao3CNA@carbian>
 <c0958e31-b477-34e0-d824-b017efadd0df@gmail.com> <ZE0Unz0lxMWY4pRf@carbian>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <ZE0Unz0lxMWY4pRf@carbian>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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