On Wed, Jan 31, 2024 at 5:41 PM Charles Keepax ckeepax@opensource.cirrus.com wrote:
On Fri, Jan 19, 2024 at 11:49:17AM +0000, Charles Keepax wrote:
On Thu, Jan 18, 2024 at 07:06:13PM +0200, andy.shevchenko@gmail.com wrote:
Fri, Aug 04, 2023 at 11:46:01AM +0100, Charles Keepax kirjoitti:
while (buf < block) {const u8 *word = min(buf + sizeof(u32), block);int pad = (buf + sizeof(u32)) - word;while (buf < word) {val >>= BITS_PER_BYTE;val |= FIELD_PREP(GENMASK(31, 24), *buf);buf++;}Is this a reinvented way of get_unaligned_*() APIs?
val >>= pad * BITS_PER_BYTE;regmap_write(regmap, CS42L43_TX_DATA, val);}...
while (buf < word) {*buf = FIELD_GET(GENMASK(7, 0), val);val >>= BITS_PER_BYTE;buf++;}put_unaligned_*() ?
Alas as it has been a while I have forgetten the exact context here and this one will take a little more time. I will try to find some spare time to work out if that would actual do the same thing, I have a vague feeling there was something here.
Ok found some time to refresh my memory on this.
The main issue here was as this is processing raw data for the SPI we shouldn't assume the data is a multiple of 4-bytes. You could write the code such that you used put_unaligned_le32 for most of the data and then special case the remainder, which would probably be slightly faster. But for the remainder you either end with the code here anyway or a special case for each of the cases 8,16,24 bits. So the code ends up looking much messier. Personally I am inclined to leave this unless performance on the SPI becomes a major issue.
Yes, the problem with the code is that it is a NIH existing API or patterns. We have already in the IIO subsystem a pattern where there is a switch case and put/get unaligned APIs per case. Perhaps this is what needs to be factored out for everybody.
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad4130.c#L472
(some shorter variants) https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ltc2497.c#L66 https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/ad4130.c#L472
Here is the abstraction for cameras, perhaps that's what ASoC might need. drivers/media/v4l2-core/v4l2-cci.c.
There is perhaps an argument for a comment in the code to explain this given it took me time to remember what was going on.
That's for sure.