On Tue, Apr 20, 2021 at 2:36 PM Tomas Melin <tomas.melin@xxxxxxxxxxx> wrote:
> 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:
...
> >>>> + 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?
Sounds like a plan!
> >> goto out;
> >>
> >>>> + }
> >>>> + data->scan.channels[i++] = val;
> >>>> + }
--
With Best Regards,
Andy Shevchenko
