On 10/21/22 13:49, Andy Shevchenko wrote:
On Fri, Oct 21, 2022 at 10:10:08AM +0300, Matti Vaittinen wrote:
On 10/20/22 17:34, Andy Shevchenko wrote:
On Thu, Oct 20, 2022 at 02:37:15PM +0300, Matti Vaittinen wrote:
...
+ ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
+ sizeof(__le16));
+ if (ret)
+ return ret;
+
+ *val = le16_to_cpu(data->buffer[0]);
'p'-variant of the above would look better
*val = le16_to_cpup(data->buffer);
since it will be the same as above address without any additional arithmetics.
I guess there is no significant performance difference? To my eye the
le16_to_cpu(data->buffer[0]) is much more clear. I see right from the call
that we have an array here and use the first member. If there is no obvious
technical merit for using le16_to_cpup(data->buffer) over
le16_to_cpu(data->buffer[0]), then I do really prefer the latter for
clarity.
Then you probably wanted to have &data->buffer[0] as a parameter to
regmap_bulk_read()?
Yes! Thanks.
...
+ if (data->trigger_enabled) {
+ iio_trigger_poll_chained(data->trig);
+ ret = IRQ_HANDLED;
+ }
+
+ if (data->state & KX022A_STATE_FIFO) {
+ ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+ if (ret > 0)
+ ret = IRQ_HANDLED;
I don't like it. Perhaps
bool handled = false;
int ret;
...
ret = ...
if (ret > 0)
handled = true;
...
return IRQ_RETVAL(handled);
I don't see the benefit of adding another variable 'handled'.
If I understand correctly, it just introduces one extra 'if' in IRQ thread
handling while hiding the return value in IRQ_RETVAL() - macro.
I do like seeing the IRQ_NONE being returned by default and IRQ_HANDLED only
when "handlers" are successfully executed. Adding extra variable just
obfuscates this (from my eyes) while adding also the additional 'if'.
You assigning semantically different values to the same variable inside the
function.
Ah, yes! This was really a bug making it way in. I guess you may just
have saved me from some not-that-funny debugging session... Well spotted!
I still don't like hiding the IRQ_HANDLED / IRQ_NONE. I'll just go for
if (data->state & KX022A_STATE_FIFO) {
int ok;
ok = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH,
true);
if (ok > 0)
ret = IRQ_HANDLED;
}
for v4. (Which I try to send still today before my memory is flushed by
the weekend :])
Thanks a lot!
Yours
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
