iio:st_magn: enable trigger before enabling sensor
diff mbox series

Message ID 20181025223226.10292-1-denis.ciocca@st.com
State New
Headers show
Series
  • iio:st_magn: enable trigger before enabling sensor
Related show

Commit Message

Denis Ciocca Oct. 25, 2018, 10:32 p.m. UTC
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@martingkelly.com>
Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
---
 drivers/iio/magnetometer/st_magn_buffer.c | 33 +++++++++++------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Comments

Martin Kelly Oct. 26, 2018, 1:28 a.m. UTC | #1
On 10/25/18 3:32 PM, Denis Ciocca 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@martingkelly.com>
> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> ---
>   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;
>   	}
>   
>   	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;

I think it would be useful to log an error message when we fail, to help 
distinguish in the logs between the two different failure cases.

>   
> -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,
>   };
>
Jonathan Cameron Oct. 28, 2018, 4:20 p.m. UTC | #2
On Thu, 25 Oct 2018 15:32:26 -0700
Denis Ciocca <denis.ciocca@st.com> 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@martingkelly.com>
> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
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.

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,
>  };
Martin Kelly Oct. 28, 2018, 7:07 p.m. UTC | #3
On 10/28/18 9:20 AM, Jonathan Cameron wrote:
> On Thu, 25 Oct 2018 15:32:26 -0700
> Denis Ciocca <denis.ciocca@st.com> 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@martingkelly.com>
>> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> 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,
>>   };
>
Denis Ciocca Oct. 28, 2018, 10:55 p.m. UTC | #4
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@martingkelly.com> 
Sent: Sunday, October 28, 2018 12:07 PM
To: Jonathan Cameron <jic23@kernel.org>; Denis CIOCCA <denis.ciocca@st.com>
Cc: linux-iio@vger.kernel.org; lars@metafoo.de; lorenzo.bianconi83@gmail.com; pmeerw@pmeerw.net; knaack.h@gmx.de
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@st.com> 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@martingkelly.com>
>> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
> 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,
>>   };
>
Martin Kelly Oct. 28, 2018, 11:15 p.m. UTC | #5
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@martingkelly.com>
> Sent: Sunday, October 28, 2018 12:07 PM
> To: Jonathan Cameron <jic23@kernel.org>; Denis CIOCCA <denis.ciocca@st.com>
> Cc: linux-iio@vger.kernel.org; lars@metafoo.de; lorenzo.bianconi83@gmail.com; pmeerw@pmeerw.net; knaack.h@gmx.de
> 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@st.com> 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@martingkelly.com>
>>> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>
>> 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,
>>>    };
>>
>
Jonathan Cameron Nov. 3, 2018, 10:26 a.m. UTC | #6
On Sun, 28 Oct 2018 16:15:21 -0700
Martin Kelly <martin@martingkelly.com> wrote:

> 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.
Agreed. It would indeed be nice if that were possible.

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

Cool. Thanks for sorting this.

Jonathan

> 
> > 
> > 
> > -----Original Message-----
> > From: Martin Kelly <martin@martingkelly.com>
> > Sent: Sunday, October 28, 2018 12:07 PM
> > To: Jonathan Cameron <jic23@kernel.org>; Denis CIOCCA <denis.ciocca@st.com>
> > Cc: linux-iio@vger.kernel.org; lars@metafoo.de; lorenzo.bianconi83@gmail.com; pmeerw@pmeerw.net; knaack.h@gmx.de
> > 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@st.com> 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@martingkelly.com>
> >>> Signed-off-by: Denis Ciocca <denis.ciocca@st.com>  
> >> 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,
> >>>    };  
> >>  
> >   
>

Patch
diff mbox series

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