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

[Tomas Melin]

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