[PATCH] iio: Fix endianness issue in ak8975_read_axis()

[PATCH] iio: Fix endianness issue in ak8975_read_axis()

[Peter Meerwald]

i2c_smbus_read_word_data() does host endian conversion already,
no need for le16_to_cpu()

Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
---
 drivers/iio/magnetometer/ak8975.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 09ea5c4..ea08313 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -373,8 +373,6 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 {
 	struct ak8975_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
-	u16 meas_reg;
-	s16 raw;
 	int ret;
 
 	mutex_lock(&data->lock);
@@ -422,16 +420,11 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 		dev_err(&client->dev, "Read axis data fails\n");
 		goto exit;
 	}
-	meas_reg = ret;
 
 	mutex_unlock(&data->lock);
 
-	/* Endian conversion of the measured values. */
-	raw = (s16) (le16_to_cpu(meas_reg));
-
 	/* Clamp to valid range. */
-	raw = clamp_t(s16, raw, -4096, 4095);
-	*val = raw;
+	*val = clamp_t(s16, ret, -4096, 4095);
 	return IIO_VAL_INT;
 
 exit:
-- 
1.9.1

Re: [PATCH] iio: Fix endianness issue in ak8975_read_axis()

[Srinivas Pandruvada]

On 06/05/2014 01:53 AM, Peter Meerwald wrote:
> i2c_smbus_read_word_data() does host endian conversion already,
> no need for le16_to_cpu()
>
> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
> ---
>   drivers/iio/magnetometer/ak8975.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 09ea5c4..ea08313 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -373,8 +373,6 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>   {
>   	struct ak8975_data *data = iio_priv(indio_dev);
>   	struct i2c_client *client = data->client;
> -	u16 meas_reg;
> -	s16 raw;
>   	int ret;
>
>   	mutex_lock(&data->lock);
> @@ -422,16 +420,11 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>   		dev_err(&client->dev, "Read axis data fails\n");
>   		goto exit;
>   	}
> -	meas_reg = ret;
>
>   	mutex_unlock(&data->lock);
>
> -	/* Endian conversion of the measured values. */
> -	raw = (s16) (le16_to_cpu(meas_reg));
> -
>   	/* Clamp to valid range. */
> -	raw = clamp_t(s16, raw, -4096, 4095);
> -	*val = raw;
> +	*val = clamp_t(s16, ret, -4096, 4095);
>   	return IIO_VAL_INT;
>
>   exit:
>
I did some experiments on little-endian platforms,  so your change
won't hurt on x86.

Read as word data -> Reg 3 (LB) and Reg 4(HB)
[ 1529.699617] Word read reg->3:ffc8

Read as byte separately LB and HB
[ 1529.711619] Byte read reg->3:c8 reg->4:ff

x86 platform is le, so no change in output by call to le16_to_cpu

[ 1529.711634] Raw value final ffffffc8



Don't we need to convert for big-endian platforms?

When I trace the path for i2c, I don't see any host order
conversion.

Path:
i2c_smbus_read_word_data->i2c_smbus_xfer->i2c_smbus_xfer_emulated

i2c_smbus_xfer_emulated
{

(for designware) bus

// Send command part
adap->algo->master_xfer->i2c_dw_xfer_msg ->

// Rx data
via i2c_dw_read, don't see any host order conversion
\
{
order word according to SMBUS specification, low byte on the bus first
	case I2C_SMBUS_WORD_DATA:
		case I2C_SMBUS_PROC_CALL:
			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
			break;
}
	But there is no host order conversion?

}


Thanks,
Srinivas

Re: [PATCH] iio: Fix endianness issue in ak8975_read_axis()

[Peter Meerwald]

Hello Srinivas,

> > -	/* Endian conversion of the measured values. */
> > -	raw = (s16) (le16_to_cpu(meas_reg));
> > -
> >   	/* Clamp to valid range. */
> > -	raw = clamp_t(s16, raw, -4096, 4095);
> > -	*val = raw;
> > +	*val = clamp_t(s16, ret, -4096, 4095);
> >   	return IIO_VAL_INT;
> > 
> >   exit:
> > 
> I did some experiments on little-endian platforms,  so your change
> won't hurt on x86.

good!

> Don't we need to convert for big-endian platforms?
> 
> When I trace the path for i2c, I don't see any host order
> conversion.

> order word according to SMBUS specification, low byte on the bus first
> 	case I2C_SMBUS_WORD_DATA:
> 		case I2C_SMBUS_PROC_CALL:
> 			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
> 			break;
> }
> 	But there is no host order conversion?

data->word = msgbuf1[0] | (msgbuf1[1] << 8);
will work irrespective of host endianness

the story would be different for
data->word = *(short *)msgbuf1;
which works for little-endian, but not for big-endian

so: don't worry :)

p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH] iio: Fix endianness issue in ak8975_read_axis()

[Srinivas Pandruvada]

On 06/05/2014 11:17 AM, Peter Meerwald wrote:
> Hello Srinivas,
>
>>> -	/* Endian conversion of the measured values. */
>>> -	raw = (s16) (le16_to_cpu(meas_reg));
>>> -
>>>    	/* Clamp to valid range. */
>>> -	raw = clamp_t(s16, raw, -4096, 4095);
>>> -	*val = raw;
>>> +	*val = clamp_t(s16, ret, -4096, 4095);
>>>    	return IIO_VAL_INT;
>>>
>>>    exit:
>>>
>> I did some experiments on little-endian platforms,  so your change
>> won't hurt on x86.
>
> good!
>
>> Don't we need to convert for big-endian platforms?
>>
>> When I trace the path for i2c, I don't see any host order
>> conversion.
>
>> order word according to SMBUS specification, low byte on the bus first
>> 	case I2C_SMBUS_WORD_DATA:
>> 		case I2C_SMBUS_PROC_CALL:
>> 			data->word = msgbuf1[0] | (msgbuf1[1] << 8);
>> 			break;
>> }
>> 	But there is no host order conversion?
>
> data->word = msgbuf1[0] | (msgbuf1[1] << 8);
> will work irrespective of host endianness
> the story would be different for
> data->word = *(short *)msgbuf1;
> which works for little-endian, but not for big-endian
>
> so: don't worry :)
Oops, my bad. You are correct.

Thanks,
Srinivas
>
> p.
>

Re: [PATCH] iio: Fix endianness issue in ak8975_read_axis()

[Jonathan Cameron]

On 05/06/14 09:53, Peter Meerwald wrote:
> i2c_smbus_read_word_data() does host endian conversion already,
> no need for le16_to_cpu()
>
> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks

> ---
>   drivers/iio/magnetometer/ak8975.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 09ea5c4..ea08313 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -373,8 +373,6 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>   {
>   	struct ak8975_data *data = iio_priv(indio_dev);
>   	struct i2c_client *client = data->client;
> -	u16 meas_reg;
> -	s16 raw;
>   	int ret;
>
>   	mutex_lock(&data->lock);
> @@ -422,16 +420,11 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>   		dev_err(&client->dev, "Read axis data fails\n");
>   		goto exit;
>   	}
> -	meas_reg = ret;
>
>   	mutex_unlock(&data->lock);
>
> -	/* Endian conversion of the measured values. */
> -	raw = (s16) (le16_to_cpu(meas_reg));
> -
>   	/* Clamp to valid range. */
> -	raw = clamp_t(s16, raw, -4096, 4095);
> -	*val = raw;
> +	*val = clamp_t(s16, ret, -4096, 4095);
>   	return IIO_VAL_INT;
>
>   exit:
>