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

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

[Alison Schofield]

On Sat, Oct 01, 2016 at 04:34:18PM +0530, sayli karnik wrote:
> Fix the following sparse warning due to incorrect type in assignment:
> drivers/iio/imu/bmi160/bmi160_core.c:411:26: warning: incorrect type
> in assignment (different base types)
> 
> drivers/iio/imu/bmi160/bmi160_core.c:411:26: expected signed short
> [signed] [short] [explicitly-signed] <noident>
> drivers/iio/imu/bmi160/bmi160_core.c:411:26: got restricted __le16
> [addressable] [usertype] sample
> 
> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>

Hi Sayli,

Please claim this via the outreachy task page - section endianness coding
task.  Perhaps you can update the commit msg & changelog with the cosmetic
vs bug impact statement as in your earlier patch. 

question inline below

> ---
>  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);
 	}
 
 	iio_push_to_buffers_with_timestamp(indio_dev, buf,


alisons

>  
> -- 
> 2.7.4
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20161001110418.GA25515%40sayli-HP-15-Notebook-PC.
> For more options, visit https://groups.google.com/d/optout.

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

[Lars-Peter Clausen]

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.

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

[Alison Schofield]

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 ;)

alisons

> 

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

[sayli karnik]

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)

thanks,
sayli

> alisons
>
>>

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

[Alison Schofield]

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

> 
> thanks,
> sayli
> 
> > alisons
> >
> >>

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

[Alison Schofield]

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

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

[sayli karnik]

On Wed, Oct 5, 2016 at 5:17 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> 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.)
>
Noted!

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

Oh, in that case we could make both buf and sample 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
>
Yes sizeof(variable) instead of sizeof(type) makes code resistant to
future type changes.

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

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

[Jonathan Cameron]

On 05/10/16 19:00, sayli karnik wrote:
> On Wed, Oct 5, 2016 at 5:17 AM, Alison Schofield <amsfield22@gmail.com> wrote:
>> 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.)
>>
> Noted!
> 
>> 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.
> 
> Oh, in that case we could make both buf and sample s16.
I actually fall just the other way.  They are little endian so we
should mark them as such, whether or not we are going to do any
conversions on them in kernel. It acts as a form of documentation
and makes it a little more obvious what the code is doing.
It gets vaguer with buffer as that is a mixed endian beast.
If you really want that one right you could use a structure, but
honestly it's not worth the hassle.

> 
>  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
>>
> Yes sizeof(variable) instead of sizeof(type) makes code resistant to
> future type changes.
Absolutely (I missed that in the original reviews!)

Jonathan
> 
>> See what you think, group reply with questions, and we'll get to the
>> bottom of this one soon!
>>
>> Thanks,
>> alisons
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[sayli karnik]

On Sun, Oct 9, 2016 at 1:13 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 05/10/16 19:00, sayli karnik wrote:
>> On Wed, Oct 5, 2016 at 5:17 AM, Alison Schofield <amsfield22@gmail.com> wrote:
>>> 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.)
>>>
>> Noted!
>>
>>> 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.
>>
>> Oh, in that case we could make both buf and sample s16.
> I actually fall just the other way.  They are little endian so we
> should mark them as such, whether or not we are going to do any
> conversions on them in kernel. It acts as a form of documentation
> and makes it a little more obvious what the code is doing.
> It gets vaguer with buffer as that is a mixed endian beast.
> If you really want that one right you could use a structure, but
> honestly it's not worth the hassle.
>
>>
>>  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
>>>
>> Yes sizeof(variable) instead of sizeof(type) makes code resistant to
>> future type changes.
> Absolutely (I missed that in the original reviews!)
>
I have sent an updated patch, but forgot to mention the version.
Does this comment make sense with __le16 instead of s16?
/* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */

thanks,
sayli
> Jonathan
>>
>>> See what you think, group reply with questions, and we'll get to the
>>> bottom of this one soon!
>>>
>>> Thanks,
>>> alisons
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

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

[Daniel Baluta]

On Tue, Oct 11, 2016 at 1:50 AM, sayli karnik <karniksayli1995@gmail.com> wrote:
> On Sun, Oct 9, 2016 at 1:13 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 05/10/16 19:00, sayli karnik wrote:
>>> On Wed, Oct 5, 2016 at 5:17 AM, Alison Schofield <amsfield22@gmail.com> wrote:
>>>> 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.)
>>>>
>>> Noted!
>>>
>>>> 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.
>>>
>>> Oh, in that case we could make both buf and sample s16.
>> I actually fall just the other way.  They are little endian so we
>> should mark them as such, whether or not we are going to do any
>> conversions on them in kernel. It acts as a form of documentation
>> and makes it a little more obvious what the code is doing.
>> It gets vaguer with buffer as that is a mixed endian beast.
>> If you really want that one right you could use a structure, but
>> honestly it's not worth the hassle.
>>
>>>
>>>  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
>>>>
>>> Yes sizeof(variable) instead of sizeof(type) makes code resistant to
>>> future type changes.
>> Absolutely (I missed that in the original reviews!)
>>
> I have sent an updated patch, but forgot to mention the version.
> Does this comment make sense with __le16 instead of s16?
> /* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */

Yup, it makes sense if you change to type of the buf. Anyhow,
place put it above the declaration of buf.

Daniel.