[PATCH 1/2] em28xx: reduce stack usage in sensor probing functions

[PATCH 1/2] em28xx: reduce stack usage in sensor probing functions

[Frank Schäfer]

It's no longer necessary to keep the i2c_client in the device struct
unmodified until a sensor is found, so reduce stack usage in
em28xx_probe_sensor_micron() and em28xx_probe_sensor_omnivision() by using
a pointer to the client instead of a local copy.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-camera.c | 42 +++++++++++++++-----------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
index 89c890ba7dd6..7b4129ab1cf9 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -110,44 +110,44 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
 	__be16 id_be;
 	u16 id;
 
-	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
+	struct i2c_client *client = &dev->i2c_client[dev->def_i2c_bus];
 
 	dev->em28xx_sensor = EM28XX_NOSENSOR;
 	for (i = 0; micron_sensor_addrs[i] != I2C_CLIENT_END; i++) {
-		client.addr = micron_sensor_addrs[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, &reg, 1);
+		ret = i2c_master_send(client, &reg, 1);
 		if (ret < 0) {
 			if (ret != -ENXIO)
 				dev_err(&dev->intf->dev,
 					"couldn't read from i2c device 0x%02x: error %i\n",
-				       client.addr << 1, ret);
+				       client->addr << 1, ret);
 			continue;
 		}
-		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
+		ret = i2c_master_recv(client, (u8 *)&id_be, 2);
 		if (ret < 0) {
 			dev_err(&dev->intf->dev,
 				"couldn't read from i2c device 0x%02x: error %i\n",
-				client.addr << 1, ret);
+				client->addr << 1, ret);
 			continue;
 		}
 		id = be16_to_cpu(id_be);
 		/* Read chip ID from register 0xff */
 		reg = 0xff;
-		ret = i2c_master_send(&client, &reg, 1);
+		ret = i2c_master_send(client, &reg, 1);
 		if (ret < 0) {
 			dev_err(&dev->intf->dev,
 				"couldn't read from i2c device 0x%02x: error %i\n",
-				client.addr << 1, ret);
+				client->addr << 1, ret);
 			continue;
 		}
-		ret = i2c_master_recv(&client, (u8 *)&id_be, 2);
+		ret = i2c_master_recv(client, (u8 *)&id_be, 2);
 		if (ret < 0) {
 			dev_err(&dev->intf->dev,
 				"couldn't read from i2c device 0x%02x: error %i\n",
-				client.addr << 1, ret);
+				client->addr << 1, ret);
 			continue;
 		}
 		/* Validate chip ID to be sure we have a Micron device */
@@ -197,7 +197,6 @@ static int em28xx_probe_sensor_micron(struct em28xx *dev)
 			dev_info(&dev->intf->dev,
 				 "sensor %s detected\n", name);
 
-		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
 		return 0;
 	}
 
@@ -213,30 +212,30 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
 	char *name;
 	u8 reg;
 	u16 id;
-	struct i2c_client client = dev->i2c_client[dev->def_i2c_bus];
+	struct i2c_client *client = &dev->i2c_client[dev->def_i2c_bus];
 
 	dev->em28xx_sensor = EM28XX_NOSENSOR;
 	/* NOTE: these devices have the register auto incrementation disabled
 	 * by default, so we have to use single byte reads !              */
 	for (i = 0; omnivision_sensor_addrs[i] != I2C_CLIENT_END; i++) {
-		client.addr = omnivision_sensor_addrs[i];
+		client->addr = omnivision_sensor_addrs[i];
 		/* Read manufacturer ID from registers 0x1c-0x1d (BE) */
 		reg = 0x1c;
-		ret = i2c_smbus_read_byte_data(&client, reg);
+		ret = i2c_smbus_read_byte_data(client, reg);
 		if (ret < 0) {
 			if (ret != -ENXIO)
 				dev_err(&dev->intf->dev,
 					"couldn't read from i2c device 0x%02x: error %i\n",
-					client.addr << 1, ret);
+					client->addr << 1, ret);
 			continue;
 		}
 		id = ret << 8;
 		reg = 0x1d;
-		ret = i2c_smbus_read_byte_data(&client, reg);
+		ret = i2c_smbus_read_byte_data(client, reg);
 		if (ret < 0) {
 			dev_err(&dev->intf->dev,
 				"couldn't read from i2c device 0x%02x: error %i\n",
-				client.addr << 1, ret);
+				client->addr << 1, ret);
 			continue;
 		}
 		id += ret;
@@ -245,20 +244,20 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
 			continue;
 		/* Read product ID from registers 0x0a-0x0b (BE) */
 		reg = 0x0a;
-		ret = i2c_smbus_read_byte_data(&client, reg);
+		ret = i2c_smbus_read_byte_data(client, reg);
 		if (ret < 0) {
 			dev_err(&dev->intf->dev,
 				"couldn't read from i2c device 0x%02x: error %i\n",
-				client.addr << 1, ret);
+				client->addr << 1, ret);
 			continue;
 		}
 		id = ret << 8;
 		reg = 0x0b;
-		ret = i2c_smbus_read_byte_data(&client, reg);
+		ret = i2c_smbus_read_byte_data(client, reg);
 		if (ret < 0) {
 			dev_err(&dev->intf->dev,
 				"couldn't read from i2c device 0x%02x: error %i\n",
-				client.addr << 1, ret);
+				client->addr << 1, ret);
 			continue;
 		}
 		id += ret;
@@ -309,7 +308,6 @@ static int em28xx_probe_sensor_omnivision(struct em28xx *dev)
 			dev_info(&dev->intf->dev,
 				 "sensor %s detected\n", name);
 
-		dev->i2c_client[dev->def_i2c_bus].addr = client.addr;
 		return 0;
 	}
 
-- 
2.11.0

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

[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 <fschaefer.oss@googlemail.com>
---
 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 = &dev->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, &reg, 1);
+		ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
 		if (ret < 0) {
 			if (ret != -ENXIO)
 				dev_err(&dev->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 *)&id_be, 2);
-		if (ret < 0) {
-			dev_err(&dev->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, &reg, 1);
-		if (ret < 0) {
-			dev_err(&dev->intf->dev,
-				"couldn't read from i2c device 0x%02x: error %i\n",
-				client->addr << 1, ret);
-			continue;
-		}
-		ret = i2c_master_recv(client, (u8 *)&id_be, 2);
+		ret = i2c_smbus_read_word_data(client, 0xff);
 		if (ret < 0) {
 			dev_err(&dev->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

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

[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 <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:
>>> Em Sun, 19 Feb 2017 19:29:18 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> 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 <fschaefer.oss@googlemail.com>
>>>> ---
>>>>    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 = &dev->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, &reg, 1);
>>>> +		ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
>>>>    		if (ret < 0) {
>>>>    			if (ret != -ENXIO)
>>>>    				dev_err(&dev->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 *)&id_be, 2);
>>>> -		if (ret < 0) {
>>>> -			dev_err(&dev->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

[Mauro Carvalho Chehab]

Em Thu, 23 Mar 2017 19:03:20 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 23.03.2017 um 13:56 schrieb Mauro Carvalho Chehab:
> > Em Thu, 23 Mar 2017 13:01:32 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >  
> >> Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:  
> >>> Em Sun, 19 Feb 2017 19:29:18 +0100
> >>> Frank Schäfer <fschaefer.oss@googlemail.com> 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 <fschaefer.oss@googlemail.com>
> >>>> ---
> >>>>    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 = &dev->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, &reg, 1);
> >>>> +		ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
> >>>>    		if (ret < 0) {
> >>>>    			if (ret != -ENXIO)
> >>>>    				dev_err(&dev->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 *)&id_be, 2);
> >>>> -		if (ret < 0) {
> >>>> -			dev_err(&dev->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

[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 <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 23.03.2017 um 13:56 schrieb Mauro Carvalho Chehab:
>>> Em Thu, 23 Mar 2017 13:01:32 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>   
>>>> Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:
>>>>> Em Sun, 19 Feb 2017 19:29:18 +0100
>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> 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 <fschaefer.oss@googlemail.com>
>>>>>> ---
>>>>>>     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 = &dev->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, &reg, 1);
>>>>>> +		ret = i2c_smbus_read_word_data(client, 0x00); /* assumes LE */
>>>>>>     		if (ret < 0) {
>>>>>>     			if (ret != -ENXIO)
>>>>>>     				dev_err(&dev->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 *)&id_be, 2);
>>>>>> -		if (ret < 0) {
>>>>>> -			dev_err(&dev->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

[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 <fschaefer.oss@googlemail.com> escreveu:
>>
>>> Am 23.03.2017 um 13:56 schrieb Mauro Carvalho Chehab:
>>>> Em Thu, 23 Mar 2017 13:01:32 +0100
>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>  
>>>>> Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:
>>>>>> Em Sun, 19 Feb 2017 19:29:18 +0100
>>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> 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 <fschaefer.oss@googlemail.com>
>>>>>>> ---
>>>>>>>     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 =
>>>>>>> &dev->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, &reg, 1);
>>>>>>> +        ret = i2c_smbus_read_word_data(client, 0x00); /*
>>>>>>> assumes LE */
>>>>>>>             if (ret < 0) {
>>>>>>>                 if (ret != -ENXIO)
>>>>>>>                     dev_err(&dev->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 *)&id_be, 2);
>>>>>>> -        if (ret < 0) {
>>>>>>> -            dev_err(&dev->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

[Mauro Carvalho Chehab]

Em Mon, 10 Apr 2017 20:06:03 +0200
Frank Schäfer <fschaefer.oss@googlemail.com> 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 <fschaefer.oss@googlemail.com> escreveu:
> >>  
> >>> Am 23.03.2017 um 13:56 schrieb Mauro Carvalho Chehab:  
> >>>> Em Thu, 23 Mar 2017 13:01:32 +0100
> >>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>>    
> >>>>> Am 22.03.2017 um 15:46 schrieb Mauro Carvalho Chehab:  
> >>>>>> Em Sun, 19 Feb 2017 19:29:18 +0100
> >>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> 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 <fschaefer.oss@googlemail.com>
> >>>>>>> ---
> >>>>>>>     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 =
> >>>>>>> &dev->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, &reg, 1);
> >>>>>>> +        ret = i2c_smbus_read_word_data(client, 0x00); /*
> >>>>>>> assumes LE */
> >>>>>>>             if (ret < 0) {
> >>>>>>>                 if (ret != -ENXIO)
> >>>>>>>                     dev_err(&dev->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 *)&id_be, 2);
> >>>>>>> -        if (ret < 0) {
> >>>>>>> -            dev_err(&dev->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 !?

Just found time today to read the code implementation for
i2c_smbus_read_word_data().

i2c_smbus_read_word_data() indeed does:

		case I2C_SMBUS_WORD_DATA:
		case I2C_SMBUS_PROC_CALL:
			data->word = msgbuf1[0] | (msgbuf1[1] << 8);

So, it indeed stores data correctly, no matter the CPU endiannes.

So, the patch itself looks correct, but I found the description
misleading.

I would describe it as:

Use i2c_smbus_read_word_data() instead of i2c_master_send() and
i2c_master_recv() for reading the ID of Micorn sensors.

i2c_smbus_read_word_data() assumes that byes are in little-endian,
so, it uses:
	data->word = msgbuf1[0] | (msgbuf1[1] << 8);

However, Micron datasheet describes the ID as if they were read
in big-endian. So, we need to change the byte order in order to
match the ID number as described on their datasheets.

With changing the description to something like that:

Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Thanks,
Mauro