On Tue, Oct 11, 2016 at 1:50 AM, sayli karnik <karniksayli1995@xxxxxxxxx> wrote: > On Sun, Oct 9, 2016 at 1:13 PM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: >> 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!) >> > 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. -- 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
