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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Apr 20, 2021 at 2:36 PM Tomas Melin <tomas.melin@vaisala.com> wrote:
> 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:

...

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