Re: [PATCH 2/2] em28xx: simplify ID-reading from Micron sensors

2017-04-15 Thread Mauro Carvalho Chehab 
Em Mon, 10 Apr 2017 20:06:03 +0200
Frank Schäfer  escreveu:

> Am 26.03.2017 um 16:24 schrieb Frank Schäfer:
> >
> >
> > Am 24.03.2017 um 20:16 schrieb Mauro Carvalho Chehab:  
> >> Em Thu, 23 Mar 2017 19:03:20 +0100
> >> Frank Schäfer  escreveu:
> >>  
> >>> Am 23.03.2017 um 13:56 schrieb Mauro Carvalho Chehab:  
>  Em Thu, 23 Mar 2017 13:01:32 +0100
>  Frank Schäfer  escreveu:
> 
> > Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:  
> >> Em Sun, 19 Feb 2017 19:29:18 +0100
> >> Frank Schäfer  escreveu:
> >>   
> >>> Use i2c_smbus_read_word_data() instead of i2c_master_send() and
> >>> i2c_master_recv() for reading the ID of Micorn sensors.
> >>> Bytes need to be swapped afterwards, because
> >>> i2c_smbus_read_word_data()
> >>> assumes that the received bytes are little-endian byte order (as
> >>> specified
> >>> by smbus), while Micron sensors with 16 bit register width use
> >>> big endian
> >>> byte order.
> >>>
> >>> Signed-off-by: Frank Schäfer 
> >>> ---
> >>> drivers/media/usb/em28xx/em28xx-camera.c | 28
> >>> 
> >>> 1 file changed, 4 insertions(+), 24 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
> >>> b/drivers/media/usb/em28xx/em28xx-camera.c
> >>> index 7b4129ab1cf9..4839479624e7 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> >>> @@ -106,8 +106,6 @@ static int em28xx_probe_sensor_micron(struct
> >>> em28xx *dev)
> >>> {
> >>> int ret, i;
> >>> char *name;
> >>> -u8 reg;
> >>> -__be16 id_be;
> >>> u16 id;
> >>> struct i2c_client *client =
> >>> >i2c_client[dev->def_i2c_bus];
> >>> @@ -115,10 +113,8 @@ static int
> >>> em28xx_probe_sensor_micron(struct em28xx *dev)
> >>> dev->em28xx_sensor = EM28XX_NOSENSOR;
> >>> for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END;
> >>> i++) {
> >>> client->addr = micron_sensor_addrs[i];
> >>> -/* NOTE: i2c_smbus_read_word_data() doesn't work with
> >>> BE data */
> >>> /* Read chip ID from register 0x00 */
> >>> -reg = 0x00;
> >>> -ret = i2c_master_send(client, , 1);
> >>> +ret = i2c_smbus_read_word_data(client, 0x00); /*
> >>> assumes LE */
> >>> if (ret < 0) {
> >>> if (ret != -ENXIO)
> >>> dev_err(>intf->dev,
> >>> @@ -126,24 +122,9 @@ static int
> >>> em28xx_probe_sensor_micron(struct em28xx *dev)
> >>>client->addr << 1, ret);
> >>> continue;
> >>> }
> >>> -ret = i2c_master_recv(client, (u8 *)_be, 2);
> >>> -if (ret < 0) {
> >>> -dev_err(>intf->dev,
> >>> -"couldn't read from i2c device 0x%02x: error
> >>> %i\n",
> >>> -client->addr << 1, ret);
> >>> -continue;
> >>> -}
> >>> -id = be16_to_cpu(id_be);
> >>> +id = swab16(ret); /* LE -> BE */  
> >> That's wrong! You can't assume that CPU is BE, as some archs use LE.
> >>
> >> You should, instead, call le16_to_cpu(), to be sure that it will be
> >> doing the right thing.
> >>
> >> Something like:
> >>
> >> id = le16_to_cpu((__le16)ret);  
> > SMBus read/write word transfers are always LE (see SMBus spec section
> > 6.5.5),
> > which is also what i2c_smbus_xfer_emulated() assumes:
> > http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L3485  
>  I got that part, but, if the CPU is also LE, doing swab16() is
>  wrong. It should swap it *only* if the CPU is BE.  
> >>> No, it should always be swapped, because the bytes are always
> >>> transfered
> >>> in the wrong order.
> >>> The cpu endianess doesn't matter, (0x12 << 8) | 0x34 is always 0x1234.  
> >> You still didn't get it.
> >>
> >> Let's assume that the ID is 0x148c (MT9M112).
> >>
> >> This value, represented in low endian, is stored in memory as:
> >>
> >> unsigned char __id[2] = { 0x8c, 0x14 };
> >>
> >> If we do:
> >> u16 ret = *(u16 *)__id;
> >>
> >> What's stored at "ret" will depend if the sistem is LE or BE:
> >>
> >> on LE, ret == 0x148c
> >> on BE, ret == 0x8c14
> >>
> >> If you do:
> >> u16 id = swapb16(val)
> >>
> >> you'll get:
> >>
> >> on LE, id == 0x8c14
> >> on BE, id == 0x148c
> >>
> >> So, the value will be *wrong* at LE.
> >>
> >> However, if you do:
> >> id = le16_to_cpu((__le16)ret);
> >>
> >> On LE, this will evaluate to id = ret, and on BE, to id =

Re: [PATCH 2/2] em28xx: simplify ID-reading from Micron sensors

2017-04-10 Thread Frank Schäfer 

Am 26.03.2017 um 16:24 schrieb Frank Schäfer:
>
>
> Am 24.03.2017 um 20:16 schrieb Mauro Carvalho Chehab:
>> Em Thu, 23 Mar 2017 19:03:20 +0100
>> Frank Schäfer  escreveu:
>>
>>> Am 23.03.2017 um 13:56 schrieb Mauro Carvalho Chehab:
 Em Thu, 23 Mar 2017 13:01:32 +0100
 Frank Schäfer  escreveu:
  
> Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:
>> Em Sun, 19 Feb 2017 19:29:18 +0100
>> Frank Schäfer  escreveu:
>> 
>>> Use i2c_smbus_read_word_data() instead of i2c_master_send() and
>>> i2c_master_recv() for reading the ID of Micorn sensors.
>>> Bytes need to be swapped afterwards, because
>>> i2c_smbus_read_word_data()
>>> assumes that the received bytes are little-endian byte order (as
>>> specified
>>> by smbus), while Micron sensors with 16 bit register width use
>>> big endian
>>> byte order.
>>>
>>> Signed-off-by: Frank Schäfer 
>>> ---
>>> drivers/media/usb/em28xx/em28xx-camera.c | 28
>>> 
>>> 1 file changed, 4 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
>>> b/drivers/media/usb/em28xx/em28xx-camera.c
>>> index 7b4129ab1cf9..4839479624e7 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>>> @@ -106,8 +106,6 @@ static int em28xx_probe_sensor_micron(struct
>>> em28xx *dev)
>>> {
>>> int ret, i;
>>> char *name;
>>> -u8 reg;
>>> -__be16 id_be;
>>> u16 id;
>>> struct i2c_client *client =
>>> >i2c_client[dev->def_i2c_bus];
>>> @@ -115,10 +113,8 @@ static int
>>> em28xx_probe_sensor_micron(struct em28xx *dev)
>>> dev->em28xx_sensor = EM28XX_NOSENSOR;
>>> for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END;
>>> i++) {
>>> client->addr = micron_sensor_addrs[i];
>>> -/* NOTE: i2c_smbus_read_word_data() doesn't work with
>>> BE data */
>>> /* Read chip ID from register 0x00 */
>>> -reg = 0x00;
>>> -ret = i2c_master_send(client, , 1);
>>> +ret = i2c_smbus_read_word_data(client, 0x00); /*
>>> assumes LE */
>>> if (ret < 0) {
>>> if (ret != -ENXIO)
>>> dev_err(>intf->dev,
>>> @@ -126,24 +122,9 @@ static int
>>> em28xx_probe_sensor_micron(struct em28xx *dev)
>>>client->addr << 1, ret);
>>> continue;
>>> }
>>> -ret = i2c_master_recv(client, (u8 *)_be, 2);
>>> -if (ret < 0) {
>>> -dev_err(>intf->dev,
>>> -"couldn't read from i2c device 0x%02x: error
>>> %i\n",
>>> -client->addr << 1, ret);
>>> -continue;
>>> -}
>>> -id = be16_to_cpu(id_be);
>>> +id = swab16(ret); /* LE -> BE */
>> That's wrong! You can't assume that CPU is BE, as some archs use LE.
>>
>> You should, instead, call le16_to_cpu(), to be sure that it will be
>> doing the right thing.
>>
>> Something like:
>>
>> id = le16_to_cpu((__le16)ret);
> SMBus read/write word transfers are always LE (see SMBus spec section
> 6.5.5),
> which is also what i2c_smbus_xfer_emulated() assumes:
> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L3485
 I got that part, but, if the CPU is also LE, doing swab16() is
 wrong. It should swap it *only* if the CPU is BE.
>>> No, it should always be swapped, because the bytes are always
>>> transfered
>>> in the wrong order.
>>> The cpu endianess doesn't matter, (0x12 << 8) | 0x34 is always 0x1234.
>> You still didn't get it.
>>
>> Let's assume that the ID is 0x148c (MT9M112).
>>
>> This value, represented in low endian, is stored in memory as:
>>
>> unsigned char __id[2] = { 0x8c, 0x14 };
>>
>> If we do:
>> u16 ret = *(u16 *)__id;
>>
>> What's stored at "ret" will depend if the sistem is LE or BE:
>>
>> on LE, ret == 0x148c
>> on BE, ret == 0x8c14
>>
>> If you do:
>> u16 id = swapb16(val)
>>
>> you'll get:
>>
>> on LE, id == 0x8c14
>> on BE, id == 0x148c
>>
>> So, the value will be *wrong* at LE.
>>
>> However, if you do:
>> id = le16_to_cpu((__le16)ret);
>>
>> On LE, this will evaluate to id = ret, and on BE, to id = swab16(ret).
>> So, on both, you'll have:
>> id = 0x148c.
>
> Can you please show me the code line(s) that make the value of the
> word returned by i2c_smbus_read_word_data() cpu endianess dependent ? :)
>
Ping !?

Cheers,
Frank

Re: [PATCH 2/2] em28xx: simplify ID-reading from Micron sensors

2017-03-26 Thread Frank Schäfer 



Am 24.03.2017 um 20:16 schrieb Mauro Carvalho Chehab:

Em Thu, 23 Mar 2017 19:03:20 +0100
Frank Schäfer  escreveu:


Am 23.03.2017 um 13:56 schrieb Mauro Carvalho Chehab:

Em Thu, 23 Mar 2017 13:01:32 +0100
Frank Schäfer  escreveu:
  

Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:

Em Sun, 19 Feb 2017 19:29:18 +0100
Frank Schäfer  escreveu:
 

Use i2c_smbus_read_word_data() instead of i2c_master_send() and
i2c_master_recv() for reading the ID of Micorn sensors.
Bytes need to be swapped afterwards, because i2c_smbus_read_word_data()
assumes that the received bytes are little-endian byte order (as specified
by smbus), while Micron sensors with 16 bit register width use big endian
byte order.

Signed-off-by: Frank Schäfer 
---
drivers/media/usb/em28xx/em28xx-camera.c | 28 
1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-camera.c 
b/drivers/media/usb/em28xx/em28xx-camera.c
index 7b4129ab1cf9..4839479624e7 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -106,8 +106,6 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
{
int ret, i;
char *name;
-   u8 reg;
-   __be16 id_be;
u16 id;

	struct i2c_client *client = >i2c_client[dev->def_i2c_bus];

@@ -115,10 +113,8 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
dev->em28xx_sensor = EM28XX_NOSENSOR;
for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
client->addr = micron_sensor_addrs[i];
-   /* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
/* Read chip ID from register 0x00 */
-   reg = 0x00;
-   ret = i2c_master_send(client, , 1);
+   ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
if (ret < 0) {
if (ret != -ENXIO)
dev_err(>intf->dev,
@@ -126,24 +122,9 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
   client->addr << 1, ret);
continue;
}
-   ret = i2c_master_recv(client, (u8 *)_be, 2);
-   if (ret < 0) {
-   dev_err(>intf->dev,
-   "couldn't read from i2c device 0x%02x: error 
%i\n",
-   client->addr << 1, ret);
-   continue;
-   }
-   id = be16_to_cpu(id_be);
+   id = swab16(ret); /* LE -> BE */

That's wrong! You can't assume that CPU is BE, as some archs use LE.

You should, instead, call le16_to_cpu(), to be sure that it will be
doing the right thing.

Something like:

id = le16_to_cpu((__le16)ret);

SMBus read/write word transfers are always LE (see SMBus spec section
6.5.5),
which is also what i2c_smbus_xfer_emulated() assumes:
http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L3485

I got that part, but, if the CPU is also LE, doing swab16() is
wrong. It should swap it *only* if the CPU is BE.

No, it should always be swapped, because the bytes are always transfered
in the wrong order.
The cpu endianess doesn't matter, (0x12 << 8) | 0x34 is always 0x1234.

You still didn't get it.

Let's assume that the ID is 0x148c (MT9M112).

This value, represented in low endian, is stored in memory as:

unsigned char __id[2] = { 0x8c, 0x14 };

If we do:
u16 ret = *(u16 *)__id;

What's stored at "ret" will depend if the sistem is LE or BE:

on LE, ret == 0x148c
on BE, ret == 0x8c14

If you do:
u16 id = swapb16(val)

you'll get:

on LE, id == 0x8c14
on BE, id == 0x148c

So, the value will be *wrong* at LE.

However, if you do:
id = le16_to_cpu((__le16)ret);

On LE, this will evaluate to id = ret, and on BE, to id = swab16(ret).
So, on both, you'll have:
id = 0x148c.


Can you please show me the code line(s) that make the value of the word 
returned by i2c_smbus_read_word_data() cpu endianess dependent ? :)


Cheers,
Frank




Thanks,
Mauro

Re: [PATCH 2/2] em28xx: simplify ID-reading from Micron sensors

2017-03-24 Thread Mauro Carvalho Chehab 
Em Thu, 23 Mar 2017 19:03:20 +0100
Frank Schäfer  escreveu:

> Am 23.03.2017 um 13:56 schrieb Mauro Carvalho Chehab:
> > Em Thu, 23 Mar 2017 13:01:32 +0100
> > Frank Schäfer  escreveu:
> >  
> >> Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:  
> >>> Em Sun, 19 Feb 2017 19:29:18 +0100
> >>> Frank Schäfer  escreveu:
> >>> 
>  Use i2c_smbus_read_word_data() instead of i2c_master_send() and
>  i2c_master_recv() for reading the ID of Micorn sensors.
>  Bytes need to be swapped afterwards, because i2c_smbus_read_word_data()
>  assumes that the received bytes are little-endian byte order (as 
>  specified
>  by smbus), while Micron sensors with 16 bit register width use big endian
>  byte order.
> 
>  Signed-off-by: Frank Schäfer 
>  ---
> drivers/media/usb/em28xx/em28xx-camera.c | 28 
>  
> 1 file changed, 4 insertions(+), 24 deletions(-)
> 
>  diff --git a/drivers/media/usb/em28xx/em28xx-camera.c 
>  b/drivers/media/usb/em28xx/em28xx-camera.c
>  index 7b4129ab1cf9..4839479624e7 100644
>  --- a/drivers/media/usb/em28xx/em28xx-camera.c
>  +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>  @@ -106,8 +106,6 @@ static int em28xx_probe_sensor_micron(struct em28xx 
>  *dev)
> {
>   int ret, i;
>   char *name;
>  -u8 reg;
>  -__be16 id_be;
>   u16 id;
> 
>   struct i2c_client *client = >i2c_client[dev->def_i2c_bus];
>  @@ -115,10 +113,8 @@ static int em28xx_probe_sensor_micron(struct em28xx 
>  *dev)
>   dev->em28xx_sensor = EM28XX_NOSENSOR;
>   for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
>   client->addr = micron_sensor_addrs[i];
>  -/* NOTE: i2c_smbus_read_word_data() doesn't work with 
>  BE data */
>   /* Read chip ID from register 0x00 */
>  -reg = 0x00;
>  -ret = i2c_master_send(client, , 1);
>  +ret = i2c_smbus_read_word_data(client, 0x00); /* 
>  assumes LE */
>   if (ret < 0) {
>   if (ret != -ENXIO)
>   dev_err(>intf->dev,
>  @@ -126,24 +122,9 @@ static int em28xx_probe_sensor_micron(struct em28xx 
>  *dev)
>  client->addr << 1, ret);
>   continue;
>   }
>  -ret = i2c_master_recv(client, (u8 *)_be, 2);
>  -if (ret < 0) {
>  -dev_err(>intf->dev,
>  -"couldn't read from i2c device 0x%02x: 
>  error %i\n",
>  -client->addr << 1, ret);
>  -continue;
>  -}
>  -id = be16_to_cpu(id_be);
>  +id = swab16(ret); /* LE -> BE */  
> >>> That's wrong! You can't assume that CPU is BE, as some archs use LE.
> >>>
> >>> You should, instead, call le16_to_cpu(), to be sure that it will be
> >>> doing the right thing.
> >>>
> >>> Something like:
> >>>
> >>>   id = le16_to_cpu((__le16)ret);  
> >> SMBus read/write word transfers are always LE (see SMBus spec section
> >> 6.5.5),
> >> which is also what i2c_smbus_xfer_emulated() assumes:
> >> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L3485  
> > I got that part, but, if the CPU is also LE, doing swab16() is
> > wrong. It should swap it *only* if the CPU is BE.  
> No, it should always be swapped, because the bytes are always transfered 
> in the wrong order.
> The cpu endianess doesn't matter, (0x12 << 8) | 0x34 is always 0x1234.

You still didn't get it.

Let's assume that the ID is 0x148c (MT9M112).

This value, represented in low endian, is stored in memory as:

unsigned char __id[2] = { 0x8c, 0x14 };

If we do:
u16 ret = *(u16 *)__id;

What's stored at "ret" will depend if the sistem is LE or BE:

on LE, ret == 0x148c
on BE, ret == 0x8c14

If you do:
u16 id = swapb16(val)

you'll get:

on LE, id == 0x8c14
on BE, id == 0x148c

So, the value will be *wrong* at LE.

However, if you do:
id = le16_to_cpu((__le16)ret); 

On LE, this will evaluate to id = ret, and on BE, to id = swab16(ret).
So, on both, you'll have:
id = 0x148c.


Thanks,
Mauro

Re: [PATCH 2/2] em28xx: simplify ID-reading from Micron sensors

2017-03-23 Thread Frank Schäfer 



Am 23.03.2017 um 13:56 schrieb Mauro Carvalho Chehab:

Em Thu, 23 Mar 2017 13:01:32 +0100
Frank Schäfer  escreveu:


Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:

Em Sun, 19 Feb 2017 19:29:18 +0100
Frank Schäfer  escreveu:
  

Use i2c_smbus_read_word_data() instead of i2c_master_send() and
i2c_master_recv() for reading the ID of Micorn sensors.
Bytes need to be swapped afterwards, because i2c_smbus_read_word_data()
assumes that the received bytes are little-endian byte order (as specified
by smbus), while Micron sensors with 16 bit register width use big endian
byte order.

Signed-off-by: Frank Schäfer 
---
   drivers/media/usb/em28xx/em28xx-camera.c | 28 
   1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-camera.c 
b/drivers/media/usb/em28xx/em28xx-camera.c
index 7b4129ab1cf9..4839479624e7 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -106,8 +106,6 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
   {
int ret, i;
char *name;
-   u8 reg;
-   __be16 id_be;
u16 id;
   
   	struct i2c_client *client = >i2c_client[dev->def_i2c_bus];

@@ -115,10 +113,8 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
dev->em28xx_sensor = EM28XX_NOSENSOR;
for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
client->addr = micron_sensor_addrs[i];
-   /* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
/* Read chip ID from register 0x00 */
-   reg = 0x00;
-   ret = i2c_master_send(client, , 1);
+   ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
if (ret < 0) {
if (ret != -ENXIO)
dev_err(>intf->dev,
@@ -126,24 +122,9 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
   client->addr << 1, ret);
continue;
}
-   ret = i2c_master_recv(client, (u8 *)_be, 2);
-   if (ret < 0) {
-   dev_err(>intf->dev,
-   "couldn't read from i2c device 0x%02x: error 
%i\n",
-   client->addr << 1, ret);
-   continue;
-   }
-   id = be16_to_cpu(id_be);
+   id = swab16(ret); /* LE -> BE */

That's wrong! You can't assume that CPU is BE, as some archs use LE.

You should, instead, call le16_to_cpu(), to be sure that it will be
doing the right thing.

Something like:

id = le16_to_cpu((__le16)ret);

SMBus read/write word transfers are always LE (see SMBus spec section
6.5.5),
which is also what i2c_smbus_xfer_emulated() assumes:
http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L3485

I got that part, but, if the CPU is also LE, doing swab16() is
wrong. It should swap it *only* if the CPU is BE.
No, it should always be swapped, because the bytes are always transfered 
in the wrong order.

The cpu endianess doesn't matter, (0x12 << 8) | 0x34 is always 0x1234.

Regards,
Frank




le16_to_cpu() should do the right thing, e. g. swap for BE
CPUs or not swap otherwise.

Thanks,
Mauro

Re: [PATCH 2/2] em28xx: simplify ID-reading from Micron sensors

2017-03-23 Thread Mauro Carvalho Chehab 
Em Thu, 23 Mar 2017 13:01:32 +0100
Frank Schäfer  escreveu:

> Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:
> > Em Sun, 19 Feb 2017 19:29:18 +0100
> > Frank Schäfer  escreveu:
> >  
> >> Use i2c_smbus_read_word_data() instead of i2c_master_send() and
> >> i2c_master_recv() for reading the ID of Micorn sensors.
> >> Bytes need to be swapped afterwards, because i2c_smbus_read_word_data()
> >> assumes that the received bytes are little-endian byte order (as specified
> >> by smbus), while Micron sensors with 16 bit register width use big endian
> >> byte order.
> >>
> >> Signed-off-by: Frank Schäfer 
> >> ---
> >>   drivers/media/usb/em28xx/em28xx-camera.c | 28 
> >> 
> >>   1 file changed, 4 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c 
> >> b/drivers/media/usb/em28xx/em28xx-camera.c
> >> index 7b4129ab1cf9..4839479624e7 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> >> @@ -106,8 +106,6 @@ static int em28xx_probe_sensor_micron(struct em28xx 
> >> *dev)
> >>   {
> >>int ret, i;
> >>char *name;
> >> -  u8 reg;
> >> -  __be16 id_be;
> >>u16 id;
> >>   
> >>struct i2c_client *client = >i2c_client[dev->def_i2c_bus];
> >> @@ -115,10 +113,8 @@ static int em28xx_probe_sensor_micron(struct em28xx 
> >> *dev)
> >>dev->em28xx_sensor = EM28XX_NOSENSOR;
> >>for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
> >>client->addr = micron_sensor_addrs[i];
> >> -  /* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
> >>/* Read chip ID from register 0x00 */
> >> -  reg = 0x00;
> >> -  ret = i2c_master_send(client, , 1);
> >> +  ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
> >>if (ret < 0) {
> >>if (ret != -ENXIO)
> >>dev_err(>intf->dev,
> >> @@ -126,24 +122,9 @@ static int em28xx_probe_sensor_micron(struct em28xx 
> >> *dev)
> >>   client->addr << 1, ret);
> >>continue;
> >>}
> >> -  ret = i2c_master_recv(client, (u8 *)_be, 2);
> >> -  if (ret < 0) {
> >> -  dev_err(>intf->dev,
> >> -  "couldn't read from i2c device 0x%02x: error 
> >> %i\n",
> >> -  client->addr << 1, ret);
> >> -  continue;
> >> -  }
> >> -  id = be16_to_cpu(id_be);
> >> +  id = swab16(ret); /* LE -> BE */  
> > That's wrong! You can't assume that CPU is BE, as some archs use LE.
> >
> > You should, instead, call le16_to_cpu(), to be sure that it will be
> > doing the right thing.
> >
> > Something like:
> >
> > id = le16_to_cpu((__le16)ret);  
> 
> SMBus read/write word transfers are always LE (see SMBus spec section 
> 6.5.5),
> which is also what i2c_smbus_xfer_emulated() assumes:
> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L3485

I got that part, but, if the CPU is also LE, doing swab16() is
wrong. It should swap it *only* if the CPU is BE.

le16_to_cpu() should do the right thing, e. g. swap for BE
CPUs or not swap otherwise.

Thanks,
Mauro

Re: [PATCH 2/2] em28xx: simplify ID-reading from Micron sensors

2017-03-23 Thread Frank Schäfer 



Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:

Em Sun, 19 Feb 2017 19:29:18 +0100
Frank Schäfer  escreveu:


Use i2c_smbus_read_word_data() instead of i2c_master_send() and
i2c_master_recv() for reading the ID of Micorn sensors.
Bytes need to be swapped afterwards, because i2c_smbus_read_word_data()
assumes that the received bytes are little-endian byte order (as specified
by smbus), while Micron sensors with 16 bit register width use big endian
byte order.

Signed-off-by: Frank Schäfer 
---
  drivers/media/usb/em28xx/em28xx-camera.c | 28 
  1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-camera.c 
b/drivers/media/usb/em28xx/em28xx-camera.c
index 7b4129ab1cf9..4839479624e7 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -106,8 +106,6 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
  {
int ret, i;
char *name;
-   u8 reg;
-   __be16 id_be;
u16 id;
  
  	struct i2c_client *client = >i2c_client[dev->def_i2c_bus];

@@ -115,10 +113,8 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
dev->em28xx_sensor = EM28XX_NOSENSOR;
for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
client->addr = micron_sensor_addrs[i];
-   /* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
/* Read chip ID from register 0x00 */
-   reg = 0x00;
-   ret = i2c_master_send(client, , 1);
+   ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
if (ret < 0) {
if (ret != -ENXIO)
dev_err(>intf->dev,
@@ -126,24 +122,9 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
   client->addr << 1, ret);
continue;
}
-   ret = i2c_master_recv(client, (u8 *)_be, 2);
-   if (ret < 0) {
-   dev_err(>intf->dev,
-   "couldn't read from i2c device 0x%02x: error 
%i\n",
-   client->addr << 1, ret);
-   continue;
-   }
-   id = be16_to_cpu(id_be);
+   id = swab16(ret); /* LE -> BE */

That's wrong! You can't assume that CPU is BE, as some archs use LE.

You should, instead, call le16_to_cpu(), to be sure that it will be
doing the right thing.

Something like:

id = le16_to_cpu((__le16)ret);


SMBus read/write word transfers are always LE (see SMBus spec section 
6.5.5),

which is also what i2c_smbus_xfer_emulated() assumes:
http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L3485

Regards,
Frank



Regards,

Thanks,
Mauro

Re: [PATCH 2/2] em28xx: simplify ID-reading from Micron sensors

2017-03-22 Thread Mauro Carvalho Chehab 
Em Sun, 19 Feb 2017 19:29:18 +0100
Frank Schäfer  escreveu:

> Use i2c_smbus_read_word_data() instead of i2c_master_send() and
> i2c_master_recv() for reading the ID of Micorn sensors.
> Bytes need to be swapped afterwards, because i2c_smbus_read_word_data()
> assumes that the received bytes are little-endian byte order (as specified
> by smbus), while Micron sensors with 16 bit register width use big endian
> byte order.
> 
> Signed-off-by: Frank Schäfer 
> ---
>  drivers/media/usb/em28xx/em28xx-camera.c | 28 
>  1 file changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c 
> b/drivers/media/usb/em28xx/em28xx-camera.c
> index 7b4129ab1cf9..4839479624e7 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -106,8 +106,6 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>  {
>   int ret, i;
>   char *name;
> - u8 reg;
> - __be16 id_be;
>   u16 id;
>  
>   struct i2c_client *client = >i2c_client[dev->def_i2c_bus];
> @@ -115,10 +113,8 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>   dev->em28xx_sensor = EM28XX_NOSENSOR;
>   for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
>   client->addr = micron_sensor_addrs[i];
> - /* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
>   /* Read chip ID from register 0x00 */
> - reg = 0x00;
> - ret = i2c_master_send(client, , 1);
> + ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
>   if (ret < 0) {
>   if (ret != -ENXIO)
>   dev_err(>intf->dev,
> @@ -126,24 +122,9 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
>  client->addr << 1, ret);
>   continue;
>   }
> - ret = i2c_master_recv(client, (u8 *)_be, 2);
> - if (ret < 0) {
> - dev_err(>intf->dev,
> - "couldn't read from i2c device 0x%02x: error 
> %i\n",
> - client->addr << 1, ret);
> - continue;
> - }
> - id = be16_to_cpu(id_be);
> + id = swab16(ret); /* LE -> BE */

That's wrong! You can't assume that CPU is BE, as some archs use LE.

You should, instead, call le16_to_cpu(), to be sure that it will be
doing the right thing.

Something like:

id = le16_to_cpu((__le16)ret);

Regards,

Thanks,
Mauro

[PATCH 2/2] em28xx: simplify ID-reading from Micron sensors

2017-02-19 Thread Frank Schäfer 
Use i2c_smbus_read_word_data() instead of i2c_master_send() and
i2c_master_recv() for reading the ID of Micorn sensors.
Bytes need to be swapped afterwards, because i2c_smbus_read_word_data()
assumes that the received bytes are little-endian byte order (as specified
by smbus), while Micron sensors with 16 bit register width use big endian
byte order.

Signed-off-by: Frank Schäfer 
---
 drivers/media/usb/em28xx/em28xx-camera.c | 28 
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-camera.c 
b/drivers/media/usb/em28xx/em28xx-camera.c
index 7b4129ab1cf9..4839479624e7 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -106,8 +106,6 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
 {
int ret, i;
char *name;
-   u8 reg;
-   __be16 id_be;
u16 id;
 
struct i2c_client *client = >i2c_client[dev->def_i2c_bus];
@@ -115,10 +113,8 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
dev->em28xx_sensor = EM28XX_NOSENSOR;
for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
client->addr = micron_sensor_addrs[i];
-   /* NOTE: i2c_smbus_read_word_data() doesn't work with BE data */
/* Read chip ID from register 0x00 */
-   reg = 0x00;
-   ret = i2c_master_send(client, , 1);
+   ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
if (ret < 0) {
if (ret != -ENXIO)
dev_err(>intf->dev,
@@ -126,24 +122,9 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
   client->addr << 1, ret);
continue;
}
-   ret = i2c_master_recv(client, (u8 *)_be, 2);
-   if (ret < 0) {
-   dev_err(>intf->dev,
-   "couldn't read from i2c device 0x%02x: error 
%i\n",
-   client->addr << 1, ret);
-   continue;
-   }
-   id = be16_to_cpu(id_be);
+   id = swab16(ret); /* LE -> BE */
/* Read chip ID from register 0xff */
-   reg = 0xff;
-   ret = i2c_master_send(client, , 1);
-   if (ret < 0) {
-   dev_err(>intf->dev,
-   "couldn't read from i2c device 0x%02x: error 
%i\n",
-   client->addr << 1, ret);
-   continue;
-   }
-   ret = i2c_master_recv(client, (u8 *)_be, 2);
+   ret = i2c_smbus_read_word_data(client, 0xff);
if (ret < 0) {
dev_err(>intf->dev,
"couldn't read from i2c device 0x%02x: error 
%i\n",
@@ -151,10 +132,9 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
continue;
}
/* Validate chip ID to be sure we have a Micron device */
-   if (id != be16_to_cpu(id_be))
+   if (id != swab16(ret))
continue;
/* Check chip ID */
-   id = be16_to_cpu(id_be);
switch (id) {
case 0x1222:
name = "MT9V012"; /* MI370 */ /* 640x480 */
-- 
2.11.0