Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

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

 




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;
+       }



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux