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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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?

>>
>> Also, please avoid such long lines. I know many people don't care about the
>> line length - but for example I tend to have 3 terminal windows open
>> side-by-side on my laptop screen. Hence long lines tend to be harder to read
>> for me.
> 
> That is the case for me also, but Jonathan asked me to change
> "fifo_length * 6" and the KX022A_FIFO_SAMPLES_SIZE_BYTES is already
> defined.

then please maybe split the line from appropriate point like:
buffer = kmalloc(data->chip_info->fifo_length *
		 KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL);

> 
>>
>>> +
>>>    	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
>>>    	if (ret) {
>>>    		dev_err(dev, "Error reading buffer status\n");
>>> @@ -621,8 +625,10 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
>>>    		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
>>>    	count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
>>> -	if (!count)
>>> +	if (!count) {
>>> +		kfree(buffer);
>>>    		return 0;
>>> +	}
>>>    	/*
>>>    	 * If we are being called from IRQ handler we know the stored timestamp
>>> @@ -679,7 +685,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
>>>    	}
>>>    	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
>>> -	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
>>> +	ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
>>>    				&buffer[0], fifo_bytes);
>>>    	if (ret)
>>>    		goto renable_out;
>>> @@ -704,6 +710,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
>>>    	if (renable)
>>>    		enable_irq(data->irq);
>>> +	kfree(buffer);
>>>    	return ret;
>>>    }
>>>
>> ...
>>
>>> -int kx022a_probe_internal(struct device *dev)
>>> +const struct kx022a_chip_info kx022a_chip_info = {
>>> +	.name		  = "kx022-accel",
>>> +	.regmap_config	  = &kx022a_regmap_config,
>>> +	.channels	  = kx022a_channels,
>>> +	.num_channels	  = ARRAY_SIZE(kx022a_channels),
>>> +	.fifo_length	  = KX022A_FIFO_LENGTH,
>>> +	.who		  = KX022A_REG_WHO,
>>> +	.id		  = KX022A_ID,
>>> +	.cntl		  = KX022A_REG_CNTL,
>>> +	.cntl2		  = KX022A_REG_CNTL2,
>>> +	.odcntl		  = KX022A_REG_ODCNTL,
>>> +	.buf_cntl1	  = KX022A_REG_BUF_CNTL1,
>>> +	.buf_cntl2	  = KX022A_REG_BUF_CNTL2,
>>> +	.buf_clear	  = KX022A_REG_BUF_CLEAR,
>>> +	.buf_status1	  = KX022A_REG_BUF_STATUS_1,
>>> +	.buf_read	  = KX022A_REG_BUF_READ,
>>> +	.inc1		  = KX022A_REG_INC1,
>>> +	.inc4		  = KX022A_REG_INC4,
>>> +	.inc5		  = KX022A_REG_INC5,
>>> +	.inc6		  = KX022A_REG_INC6,
>>> +	.xout_l		  = KX022A_REG_XOUT_L,
>>> +};
>>> +EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
>>
>> Do you think the fields (or at least some of them) in this struct could be
>> named based on the (main) functionality being used, not based on the
>> register name? Something like "watermark_reg", "buf_en_reg", "reset_reg",
>> "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg",
>> "int1_src_reg" ...
>>
>> I would not be at all surprized to see for example some IRQ control to be
>> shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to
>> cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially
>> when new cool feature is added to next sensor, resulting also adding a new
>> cntl<Z> or buf_cntl<Z> or INC<Z>.
>>
>> I, however, believe the _functionality_ will be there (in some register) -
>> at least for the ICs for which we can re-use this driver. Hence, it might be
>> nice - and if you can think of better names for these fields - to rename
>> them based on the _functionality_ we use.
>>
>> Another benefit would be added clarity to the code. Writing a value to
>> "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me)
>> than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if
>> you don't have a datasheet at your hands.
>>
>> I am not "demanding" this (at least not for now :]) because it seems these
>> two Kionix sensors have been pretty consistent what comes to maintaining the
>> same functionality in the registers with same naming - but I believe this is
>> something that may in any case be lurking around the corner.
> 
> I agree, this seems to be the better solution. I will look into this.
> 

Thanks for going the extra mile :)

Yours,
	-- Matti

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

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