Move the fifo handling into a separate function. This is a preparation
for a generic handling of the interrupt status register results.
The interrupt status register is read into a variable int_stat. It carries
status for various sensor events, handling of which is added in follow up
patches. Evaluation of the int_stat variable is common for sensor events,
such as tap detection, freefall, activity,... and for fifo events, such as
data ready, overrun, watermark,... Also, dealing with in case error
returns shall be common to all events. Thus migrate fifo read-out and push
fifo content to iio channels into this function to be built up with
additional event handling.
Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index eec3f0e17e1e..d4c1a6f1559f 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -420,6 +420,24 @@ static int adxl345_fifo_push(struct iio_dev *indio_dev,
return 0;
}
+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;
+
+ if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
+ samples = adxl345_get_samples(st);
+ if (samples < 0)
+ return -EINVAL;
+
+ if (adxl345_fifo_push(indio_dev, samples) < 0)
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
/**
* adxl345_irq_handler() - Handle irqs of the ADXL345.
* @irq: The irq being handled.
@@ -432,19 +450,12 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
struct iio_dev *indio_dev = p;
struct adxl345_state *st = iio_priv(indio_dev);
int int_stat;
- int samples;
if (regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, &int_stat))
return IRQ_NONE;
- if (FIELD_GET(ADXL345_INT_WATERMARK, int_stat)) {
- samples = adxl345_get_samples(st);
- if (samples < 0)
- goto err;
-
- if (adxl345_fifo_push(indio_dev, samples) < 0)
- goto err;
- }
+ if (adxl345_push_event(indio_dev, int_stat))
+ goto err;
if (FIELD_GET(ADXL345_INT_OVERRUN, int_stat))
goto err;
--
2.39.5Thu, Mar 13, 2025 at 04:50:39PM +0000, Lothar Rubusch kirjoitti:
> Move the fifo handling into a separate function. This is a preparation
> for a generic handling of the interrupt status register results.
>
> The interrupt status register is read into a variable int_stat. It carries
> status for various sensor events, handling of which is added in follow up
> patches. Evaluation of the int_stat variable is common for sensor events,
> such as tap detection, freefall, activity,... and for fifo events, such as
> data ready, overrun, watermark,... Also, dealing with in case error
> returns shall be common to all events. Thus migrate fifo read-out and push
> fifo content to iio channels into this function to be built up with
> additional event handling.
...
> +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;
> +
> + 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.
> + 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.
--
With Best Regards,
Andy Shevchenko
On Sun, 16 Mar 2025 21:58:00 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> Thu, Mar 13, 2025 at 04:50:39PM +0000, Lothar Rubusch kirjoitti:
> > Move the fifo handling into a separate function. This is a preparation
> > for a generic handling of the interrupt status register results.
> >
> > The interrupt status register is read into a variable int_stat. It carries
> > status for various sensor events, handling of which is added in follow up
> > patches. Evaluation of the int_stat variable is common for sensor events,
> > such as tap detection, freefall, activity,... and for fifo events, such as
> > data ready, overrun, watermark,... Also, dealing with in case error
> > returns shall be common to all events. Thus migrate fifo read-out and push
> > fifo content to iio channels into this function to be built up with
> > additional event handling.
>
> ...
>
> > +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;
> > +
> > + 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.
>
>
> > + 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)
>
On Mon, Mar 17, 2025 at 12:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 16 Mar 2025 21:58:00 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> 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.
> > > +
> > > + 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.
> >
> >
> > > + 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
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@gmail.com> wrote:
>
> On Mon, Mar 17, 2025 at 12:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sun, 16 Mar 2025 21:58:00 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> 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
On Tue, Mar 18, 2025 at 11:45 AM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> On Mon, Mar 17, 2025 at 4:52 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Mar 17, 2025 at 12:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > On Sun, 16 Mar 2025 21:58:00 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> 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
Hello Andy & IIO ML
On Wed, Mar 26, 2025 at 10:33 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Mar 18, 2025 at 11:45 AM Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > On Mon, Mar 17, 2025 at 4:52 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Mon, Mar 17, 2025 at 12:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > > On Sun, 16 Mar 2025 21:58:00 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> 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
On Thu, 13 Mar 2025 16:50:39 +0000 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Move the fifo handling into a separate function. This is a preparation > for a generic handling of the interrupt status register results. > > The interrupt status register is read into a variable int_stat. It carries > status for various sensor events, handling of which is added in follow up > patches. Evaluation of the int_stat variable is common for sensor events, > such as tap detection, freefall, activity,... and for fifo events, such as > data ready, overrun, watermark,... Also, dealing with in case error > returns shall be common to all events. Thus migrate fifo read-out and push > fifo content to iio channels into this function to be built up with > additional event handling. > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> Applied
© 2016 - 2025 Red Hat, Inc.