Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 3/21/23 17:56, Mehdi Djait wrote:
> Hello Matti,
> 
>>> +static int kx022a_get_fifo_bytes(struct kx022a_data *data)
>>> +{
>>> +	struct device *dev = regmap_get_device(data->regmap);
>>> +	__le16 buf_status;
>>> +	int ret, fifo_bytes;
>>> +
>>> +	ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status));
>>> +	if (ret) {
>>> +		dev_err(dev, "Error reading buffer status\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	buf_status &= data->chip_info->buf_smp_lvl_mask;
>>> +	fifo_bytes = le16_to_cpu(buf_status);
>>> +
>>> +	/*
>>> +	 * The KX022A has FIFO which can store 43 samples of HiRes data from 2
>>> +	 * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
>>> +	 * 258 bytes of sample data. The quirk to know is that the amount of bytes in
>>> +	 * the FIFO is advertised via 8 bit register (max value 255). The thing to note
>>> +	 * is that full 258 bytes of data is indicated using the max value 255.
>>> +	 */
>>> +	if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
>>> +		fifo_bytes = KX022A_FIFO_MAX_BYTES;
>>> +
>>> +	if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
>>> +		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
>>> +
>>> +	return fifo_bytes;
>>> +}
>>
>> I like adding this function. Here I agree with Jonathan - having a device
>> specific functions would clarify this a bit. The KX022A "quirk" is a bit
>> confusing. You could then get rid of the buf_smp_lvl_mask.
> 
> my bad here, I should have made a separate patch and explained more ...
> buf_smp_lvl_mask is essential because kionix products use different
> number of bits to report "the number of data bytes that have been stored in the
> sample buffer" using the registers BUF_STATUS_1 and BUF_STATUS_2

Yes, they have different size of FIFO, and the KX022A does also have the 
nasty "FIFO FULL" quirk. Due to this quirk and other differences I was 
suggesting you created own functions for kx022a and kx132. Eg something 
along the lines:

static int kx022a_get_fifo_bytes(struct kx022a_data *data)
{
...
}
static int kx132_get_fifo_bytes(struct kx022a_data *data)
{
...
}

struct chip_info {
	...
	int (*fifo_bytes)(struct kx022a_data *);
};

and do the:
fifo_bytes = kx022a_get_fifo_bytes;
or
fifo_bytes = kx132_get_fifo_bytes;

in probe. That will also remove the need to check the IC variant for 
each FIFO read.

If you did that you could remove the buf_smp_lvl_mask and maybe also the 
buf_statusX members from the chip_info struct (at least for now). You 
could also do regular read for KX022A and drop the endianess conversion 
for it. Bulk read is only needed for ICs with more than 8bits of FIFO 
status. Furthermore, the IC-type check could then go away and the above 
mentioned KX022A-specific handling would not be obfuscating the kx132 code.

> 
> kx022a: 8bits
> kx132: 10bits
> kx12x: 11bits
> kx126: 12bits
> 
> I think this function is quite generic and can be used for different
> kionix devices:
> 
> - It reads BUF_STATUS_1 and BUF_STATUS_2 and then uses a chip specific
> mask
> - It takes care of the quirk of kx022a which is just a simple if statement

Yes. Your function definitely works. And I do like the fact that you did 
own function for the "amount of data in fifo"-check. Still, the code 
would be little simpler and perform a tiny bit better if you did two 
functions instead of one.

Yours,
	-- Matti

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

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