Hi Andy, Jonathan and IIO ML!
Pls, can you help me clarify a bit what to do best here. Questions
inlined down below.
On Mon, Mar 17, 2025 at 4:52 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Mon, Mar 17, 2025 at 12:56 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > On Sun, 16 Mar 2025 21:58:00 +0200
> > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > > Thu, Mar 13, 2025 at 04:50:39PM +0000, Lothar Rubusch kirjoitti:
>
> ...
>
> > > > +static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat)
> > > > +{
> > > > + struct adxl345_state *st = iio_priv(indio_dev);
> > > > + int samples;
>
> > > > + int ret = -ENOENT;
>
> Also note, this variable is redundant as far as I can see, just return
> the error code directly.
The pre-initialization of ret is actually needed in the follow up
patches. Anyway, I can return -ENOENT directly here.
Evaluation of the sensor events in follow up patches then uses the
ret. It is also possible that reading sensor events fails, then the
error is returned. It is possible, that no sensor event happened, then
it will fallback to -ENOENT. And, of course, if sensor event happened
and could be handled - no error is returned.
Is this approach acceptable? Say, if I'd describe it better in the
commit comment? Could you think of a better approach here? I think
returning 'samples' here does not make fully sense, though. First,
'samples' is not be used outside the called function. Second, I have
to distinguish a case "no event handled" - This covers then all
remaining events like e.g. OVERRUN, DATA READY,... which still need to
have status registers reset, but won't be pushed - currently this is
coveredy by the 'return -ENOENT;' fallback. Third, I need to be able
to return error codes.
>
> > > > +
> > > > + if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
> > > > + samples = adxl345_get_samples(st);
> > > > + if (samples < 0)
> > >
> > > > + return -EINVAL;
> > >
> > > In the original code it makes no difference, but if you are going to share
> > > this, I would expect to see
> > >
> > > return samples;
> > >
> > > here. Why the error code is shadowed? If it's trully needed, it has to be
> > > explained in the comment.
As said above, 'samples' is just internally used inside this function.
Basic question here also,
since intuitively you'd expect it rather returning a samples number -
should I rename the function
to make it clearer?
Best,
L
> > >
> > >
> > > > + if (adxl345_fifo_push(indio_dev, samples) < 0)
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + return ret;
> > > > +}
>
> ...
>
> > > Jonathan, I saw that you had applied it, but I guess the above needs
> > > a clarification.
> > Was right at the top of a tree I don't mind rebasing. So dropped
> > this patch (kept 1-3)
>
> Thank you!
>
> --
> With Best Regards,
> Andy Shevchenko
