| Message ID | 20181025223226.10292-1-denis.ciocca@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio:st_magn: enable trigger before enabling sensor | expand |
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, > }; >
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, > };
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, >> }; >
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, >> }; >
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, >>> }; >> >
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, > >>> }; > >> > > >
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, };