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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux