On Sun, 28 Oct 2018 16:15:21 -0700
Martin Kelly <martin@xxxxxxxxxxxxxxxx> 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@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,
> >>> };
> >>
> >
>
