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