On 4/20/21 1:47 PM, Andy Shevchenko wrote:
On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin <tomas.melin@xxxxxxxxxxx> wrote:
+ sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
Seems you ignored my comment. What is this 0x0? What is the meaning of it?
Same for all the rest magic numbers in the code.
Sorry, not ignored but will remove this redundant 0x0 for next round.
Maybe it's not redundant after all (I noticed other magic numbers in
the same position)? Please, comment your intention case-by-case.
0x0 is for read operation, but since it's just or'd, end result should
be the same. It was there in v1 for clarity (also #defined). Basically
thinking perhaps it's cleaner to just leave it out.
Other magics should be cleaned up now.
...
+ for_each_set_bit(bit, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+ ret = sca3300_read_reg(data, sca3300_channels[bit].address,
+ &val);
+ if (ret) {
+ dev_err(&data->spi->dev,
+ "failed to read register, error: %d\n", ret);
+ goto out;
Does it mean interrupt is handled in this case?
Perhaps a comment why it's okay to consider so?
IRQ_HANDLED seemed more correct than IRQ_NONE.
Why? Care to explain?
Thinking that IRQ was for the device and it was indeed handled. There
were errors when handling
it, but it was handled as much as possible.
Or did You have some
other option in mind?
How about something like:
/* handled with errors */
But what if this is the very first interrupt (bit in the loop) that
failed? What about the rest?
Aah, right. Other option could be to simply continue loop and set 'val'
to e.g. 0 for
readings with errors. But perhaps it is after all better to bail out,
and only for cases
when _all_ data is reliable, it is pushed to buffers(?)
Comes to mind that perhaps better to have error message in this irq
handler as
dev_err_ratelimited(), to avoid possible flooding.
So to conclude, proposing:
*change to dev_err_ratelimited()
* comment goto:
/* handled, but bailing out this round due to errors */
Would this be OK?
Thanks,
Tomas
goto out;
+ }
+ data->scan.channels[i++] = val;
+ }
