On 10/28/18 3:55 PM, Denis CIOCCA wrote:
Hey Martin, Jonathan,
Sorry my bad. I had pushed what I've modified performing the tests not considering at all the procedures. I didn't mean to take advantage. I apologize.
Martin, if you can, please just take the part needed and update accordantly the first patch you sent please. I think I just apply a 'minor' change in there. The major stuff here was the detection of the wrong order and Martin did the job!
Thanks!
Denis
No problem, patch "authorship" can be a rather fuzzy concept in a
collaborative open source project. I sometimes wish git would allow for
multiple authors like academic papers have, as it's sometimes the most
correct expression of how a patch comes to be.
I'll send the original patch in the next few days, making sure I cover
the goto-errors properly and re-testing (I think I may have missed them
the first time). I'll add a Fixes tag too.
After that, Denis may want to issue some of the cleanups that were
included here as a separate patch.
-----Original Message-----
From: Martin Kelly <martin@xxxxxxxxxxxxxxxx>
Sent: Sunday, October 28, 2018 12:07 PM
To: Jonathan Cameron <jic23@xxxxxxxxxx>; Denis CIOCCA <denis.ciocca@xxxxxx>
Cc: linux-iio@xxxxxxxxxxxxxxx; lars@xxxxxxxxxx; lorenzo.bianconi83@xxxxxxxxx; pmeerw@xxxxxxxxxx; knaack.h@xxxxxx
Subject: Re: [PATCH] iio:st_magn: enable trigger before enabling sensor
On 10/28/18 9:20 AM, Jonathan Cameron wrote:
On Thu, 25 Oct 2018 15:32:26 -0700
Denis Ciocca <denis.ciocca@xxxxxx> wrote:
From logical point of view driver should be ready to receive irqs
before enabling the sensor itself.
This patch is fixing also an issue related to sensors that generate
interrupts unconditionally, (DRDY pads) when interrupt level is used.
Signed-off-by: Martin Kelly <martin@xxxxxxxxxxxxxxxx>
Signed-off-by: Denis Ciocca <denis.ciocca@xxxxxx>
A couple of very minor points inline.
Also, author should match the first sign off. I kind of lost touch of
how much got modified, but a Reported-by: or the one you occasionally
get is Originated-by: for martin would be preferred if the changes are
major, if they are minor, then the author should be martin with a sign
off from Denis.
I would say the changes are minor, although it's subjective exactly what "minor" vs. "major" are, of course. In this case, Denis added some goto-error cleanup that I had missed, but the bug fix was from the original patch.
Also, as I understand it this is a fix for an issue seen, so make that
clear in the naming of the patch and give a fixes-tag to indicate how
far back we should apply this in stable.
Thanks,
Jonathan
---
drivers/iio/magnetometer/st_magn_buffer.c | 33 +++++++++++------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/drivers/iio/magnetometer/st_magn_buffer.c
b/drivers/iio/magnetometer/st_magn_buffer.c
index 0a9e8fadfa9d..097e6e88a464 100644
--- a/drivers/iio/magnetometer/st_magn_buffer.c
+++ b/drivers/iio/magnetometer/st_magn_buffer.c
@@ -30,11 +30,6 @@ int st_magn_trig_set_state(struct iio_trigger *trig, bool state)
return st_sensors_set_dataready_irq(indio_dev, state);
}
-static int st_magn_buffer_preenable(struct iio_dev *indio_dev) -{
- return st_sensors_set_enable(indio_dev, true);
-}
-
static int st_magn_buffer_postenable(struct iio_dev *indio_dev)
{
int err;
@@ -42,40 +37,44 @@ static int st_magn_buffer_postenable(struct
iio_dev *indio_dev)
mdata->buffer_data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
if (mdata->buffer_data == NULL) {
- err = -ENOMEM;
- goto allocate_memory_error;
+ return -ENOMEM;
I would have a very slight preference for that having been in a separate patch.
Good cleanup but not part of the main point of this patch.
}
err = iio_triggered_buffer_postenable(indio_dev);
if (err < 0)
- goto st_magn_buffer_postenable_error;
+ goto st_magn_buffer_free_buffer_data;
+
+ err = st_sensors_set_enable(indio_dev, true);
+ if (err < 0)
+ goto st_magn_buffer_buffer_predisable;
return err;
-st_magn_buffer_postenable_error:
+st_magn_buffer_buffer_predisable:
+ iio_triggered_buffer_predisable(indio_dev);
+st_magn_buffer_free_buffer_data:
kfree(mdata->buffer_data);
-allocate_memory_error:
return err;
}
static int st_magn_buffer_predisable(struct iio_dev *indio_dev)
{
- int err;
+ int err = 0, err2;
struct st_sensor_data *mdata = iio_priv(indio_dev);
- err = iio_triggered_buffer_predisable(indio_dev);
- if (err < 0)
- goto st_magn_buffer_predisable_error;
+ err2 = st_sensors_set_enable(indio_dev, false);
+ if (err2 < 0)
+ err = err2;
- err = st_sensors_set_enable(indio_dev, false);
+ err2 = iio_triggered_buffer_predisable(indio_dev);
+ if (err2 < 0)
+ err = err2;
There is a small argument that we should have some visibility of the
value of the error from st_sensors_set_enable if both error paths
trigger. I think the suggestion made in the other review of an error
comment would solve that fairly well.
-st_magn_buffer_predisable_error:
kfree(mdata->buffer_data);
return err;
}
static const struct iio_buffer_setup_ops st_magn_buffer_setup_ops = {
- .preenable = &st_magn_buffer_preenable,
.postenable = &st_magn_buffer_postenable,
.predisable = &st_magn_buffer_predisable,
};
