On 05/10/16 19:00, sayli karnik wrote: > On Wed, Oct 5, 2016 at 5:17 AM, Alison Schofield <amsfield22@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
