Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Jonathan Cameron <jic23@kernel.org>]

> > >  static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > >  {
> > >       struct adxl313_data *data = iio_priv(indio_dev);
> > > @@ -502,19 +705,32 @@ static int adxl313_fifo_push(struct iio_dev *indio_dev, int samples)
> > >
> > >  static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)  
> >
> > Ah. This does not also have events.  Still it's a mix, so maybe
> > adxl313_handle_interrupts() or something like that.  
> 
> I also could break it up into:
> - handle interrupt source register events
> - drain fifo watermark samples
Sure - if that makes more logical sense that break up is fine.

> ?
> 
> > >  {
> > > +     s64 ts = iio_get_time_ns(indio_dev);
> > >       struct adxl313_data *data = iio_priv(indio_dev);
> > >       int samples;
> > > +     int ret = -ENOENT;
> > > +
> > > +     if (FIELD_GET(ADXL313_INT_ACTIVITY, int_stat)) {
> > > +             ret = iio_push_event(indio_dev,
> > > +                                  IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > +                                                     IIO_MOD_X_OR_Y_OR_Z,
> > > +                                                     IIO_EV_TYPE_MAG,
> > > +                                                     IIO_EV_DIR_RISING),
> > > +                                  ts);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > >
> > >       if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> > >               samples = adxl313_get_samples(data);
> > >               if (samples < 0)
> > >                       return samples;
> > >
> > > -             return adxl313_fifo_push(indio_dev, samples);
> > > +             ret = adxl313_fifo_push(indio_dev, samples);
> > >       }
> > >
> > >       /* Return error if no event data was pushed to the IIO channel. */
> > > -     return -ENOENT;
> > > +     return ret;  
> > This handling works, but as Andy observed maybe the comment is now confusing
> > given ret is mostly not an error.  Perhaps put that where ret is declared
> > instead, or use a separate mask check at the start to quickly
> > error out if no bits that we handle are set.  
> > >  }  
> 
> Yes. Andy also pointed out here. I already developed a feeling for
> "something's smelly" with this code, but cannot really think of a
> better approach. Actually it works, and for me it is somehow logical.
> Probably there are better ways to solve this situation in a cleaner
> way?

The check against a mask at the start is pretty common way to deal with
no status bits (that we understand) set.  Then update that mask
define as you support more bits.

That deals with the odd error case.  It is technically an additional
conditional but the compiler may squash it anyway or if on a decent CPU
it'll be a very easy to predict branch as it should never happen.

Jonathan

>