Re: [PATCH v4 04/14] iio: accel: adxl345: introduce adxl345_push_event function

[Lothar Rubusch]

Hello Andy & IIO ML

On Wed, Mar 26, 2025 at 10:33 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Tue, Mar 18, 2025 at 11:45 AM Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> > 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:
>
> ...
>
> > > > > > +   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.
>
> Just do it when it's needed, I mean that this patch can survive
> without ret variable.
>
> > 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.
>
> But does the 'samples' contain an error code? Perhaps you should just
> make it do so...

I'd like to direct you to v5 of the patches. I'm returning 0 now for handled
interrupt and/or read FIFO elements, or a negative error code in case of error.
I guess somehow I could not see the problem initially. Nevertheless, when
preparing v5 it became clear. I did a small adjustement and hope v5 addresses
the issue in a better way.

Thank you, Andy, for pointing this out to me. So, no direct answers here,
hope it's ok.

Best,
L


>
> > > +   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?
>
> Perhaps renaming helps, but still, I don't see how a negative return
> value can fit here. I would expect a negative to be a meaningful Linux
> error code.
>
> --
> With Best Regards,
> Andy Shevchenko