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

[Andy Shevchenko]

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

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