Messages in this thread | | | Subject | Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer | From | Tomas Melin <> | Date | Tue, 20 Apr 2021 14:36:11 +0300 |
| |
On 4/20/21 1:47 PM, Andy Shevchenko wrote: > On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin <tomas.melin@vaisala.com> 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; >>>> + }
| |