On Tue, May 17, 2022 at 10:28:35AM +0200, Oliver Neukum wrote:
> sorry for being a bit obnoxious about this, but there is a slight issue.
Pointing out potential issues is the opposite of obnoxious :)
> This is the old code:
>
> static void imon_ir_data(struct imon *imon)
> {
> struct ir_raw_event rawir = {};
> u64 data = be64_to_cpu(imon->ir_buf);
> u8 packet_no = data & 0xff;
> int offset = 40;
> int bit;
>
> if (packet_no == 0xff)
> return;
>
> dev_dbg(imon->dev, "data: %*ph", 8, &imon->ir_buf);
That should be imon->ir_buf (not &imon->ir_buf) after your changes.
> The dev_dbg() logs the data as it is in the buffer. If you use
> be64_to_cpup() instead of be64_to_cpu() you reverse
> the buffer on a little endian CPU and hence the debug
> output will be changed.
I'm confused. be64_to_cpup() does not do an in-place byte swap on little
endian.
I've just tested and the debug message still works fine (barring the issue
above).
> The actual driver code is unaffected, because the
> buffer is never used again, so this is not a big deal.
>
> The error is mine by changing the type of imon->ir_buf
> But the fix is not quite the best.
Your change is good.
Sean
