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
-----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,
>> };
>
