Re: [PATCH v3 2/3] iio: accel: Support Kionix/ROHM KX022A accelerometer

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

 



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




[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