Re: [Outreachy kernel] [PATCH] iio: imu: bmi160: bmi160_core: Fix sparse warning

[Alison Schofield <amsfield22@gmail.com>]

On Wed, Oct 05, 2016 at 03:17:22AM +0530, sayli karnik wrote:
> On Mon, Oct 3, 2016 at 10:46 PM, Alison Schofield <amsfield22@gmail.com> wrote:
> > On Mon, Oct 03, 2016 at 07:07:39PM +0530, sayli karnik wrote:
> >> On Mon, Oct 3, 2016 at 4:55 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> >> > On Sun, Oct 02, 2016 at 05:53:08PM +0200, Lars-Peter Clausen wrote:
> >> >> On 10/02/2016 07:00 AM, Alison Schofield wrote:
> >> >> [...]
> >> >> >> ---
> >> >> >>  drivers/iio/imu/bmi160/bmi160_core.c | 3 ++-
> >> >> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> >> >>
> >> >> >> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> >> >> >> index e0251b8..5355507 100644
> >> >> >> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> >> >> >> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> >> >> >> @@ -398,7 +398,8 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
> >> >> >>    struct iio_poll_func *pf = p;
> >> >> >>    struct iio_dev *indio_dev = pf->indio_dev;
> >> >> >>    struct bmi160_data *data = iio_priv(indio_dev);
> >> >> >> -  s16 buf[16]; /* 3 sens x 3 axis x s16 + 3 x s16 pad + 4 x s16 tstamp */
> >> >> >> +  __le16 buf[16];
> >> >> >> +  /* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */
> >> >> >>    int i, ret, j = 0, base = BMI160_REG_DATA_MAGN_XOUT_L;
> >> >> >>    __le16 sample;
> >> >> >
> >> >> > Wondering about this option below.  Data was read into an __le16, so that
> >> >> > was good diligence on drivers part.  Seems we can use le16_to_cpu() for the
> >> >> > conversion into the buf.
> >> >> >
> >> >> > --- a/drivers/iio/imu/bmi160/bmi160_core.c
> >> >> > +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> >> >> > @@ -408,7 +408,7 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
> >> >> >                                    &sample, sizeof(__le16));
> >> >> >             if (ret < 0)
> >> >> >                     goto done;
> >> >> > -           buf[j++] = sample;
> >> >> > +           buf[j++] = le16_to_cpu(sample);
> >> >> >     }
> >> >>
> >> >> This conversion is usually skipped on purpose and delayed until it is
> >> >> actually needed by the user. The IIO channel is accordingly marked that it
> >> >> will produce LE data.
> >> > Thanks Lars.  I knew that for buffers, overlooked it, now I'll know it
> >> > better!
> >> >
> >> > So, Sayli, you probably got this from the analysis of the last patch.
> >> > In buffered mode, we'll go ahead and return the data in it's 'native'
> >> > order.  So, my suggestion to convert it here, is wrong.  Ignore ;)
> >> >
> >> Oh I see! So should I resend the patch with an updated
> >> description?(cosmetic/bug fix)
> >
> > Yes.  In the commit message, you can leave out the subdirs (imu: bmc160:)
> > so that you have more space for a descriptive message of the change.
> 
> A quick question about this being a bug fix or not. This would have
> worked fine on little endian systems. But wouldn't the byte order have
> changed in case of a big endian buffer, when the little endian samples
> are stored in it?
> If not, then this will be a cosmetic patch.
> 
> thanks,
> sayli

Hi Sayli,
Pulling linux-iio back in.  Once we've started group thread, we need to
keep replying to 'all'.  It helps those invested in this particular
patch, and also helps those who search in the future with similar
issues.

To answer your question.  I say cosmetic, because the le16 is going
into a 16bit buf element, and is labelled as IIO_LE in the channel
buffer definition.  That's why Lars was saying we don't need to do
any conversion.  We'll pass the bits as they came in, and tell the
readers of the buf that they are in little endian format.  (And, also
note we weren't truncating any as was the case in your first endian fix.)

OK - having said that, I stare at this code more, and wonder why
we are even bothering to label the sample as __le16, and whether
we should just label it as s16.  Also, I'm wondering about the use
of sizeof().  Shouldn't we be saying sizeof(sample) not sizeof(__le16)?
It's not a checkpatch error, but I feel like I've seen coccichecks
or coccinelle script patches repairing these misuses of sizeof

See what you think, group reply with questions, and we'll get to the
bottom of this one soon!  

Thanks,
alisons