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.
...
- if (is_of_node(fwnode))
fwnode = fwnode_get_named_child_node(fwnode, "spi");You can actually drop these is_of_node() tests and use another variable. In ACPI there can't be child node in small letters.
is_of_node feels pretty clear what the intent is, rather than relying on nodes not existing etc.
But main problem here (and in another driver where the similar is used) that you bumped reference count for fwnode. I haven't seen where you drop it back. Have you tested rmmod/modprobe in a loop?
Yeah it should drop the reference will add that.
- devm_pm_runtime_enable(priv->dev);
No error check? Why?
Happy to add one.
- ret = devm_spi_register_controller(priv->dev, priv->ctlr);
- if (ret) {
pm_runtime_disable(priv->dev);Ah! Are you sure you properly simulated faults when testing this code?
This one has already been fixed.
Thanks, Charles