lkml.org 
[lkml]   [2021]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer
From
Date

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

\
 
 \ /
  Last update: 2021-04-20 13:37    [W:0.072 / U:1.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site