diff mbox

[RFC,v6] media: add v4l2 subdev driver for S5K4ECGX sensor

Message ID 504E0175.80504@samsung.com
State New
Headers show

Commit Message

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics Sept. 10, 2012, 3:04 p.m. UTC 
Hi,

On 09/09/2012 06:01 PM, Francesco Lavra wrote:
>> +static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>> +{
>> +	const struct firmware *fw;
>> +	int err, i, regs_num;
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	u16 val;
>> +	u32 addr, crc, crc_file, addr_inc = 0;
>> +
>> +	err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
>> +	if (err) {
>> +		v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE);
>> +		return err;
>> +	}
>> +	regs_num = *(u32 *)(fw->data);
>> +	v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",
>> +		 S5K4ECGX_FIRMWARE, fw->size, regs_num);
>> +	regs_num++; /* Add header */
>> +	if (fw->size != regs_num * FW_RECORD_SIZE + FW_CRC_SIZE) {
>> +		err = -EINVAL;
>> +		goto fw_out;
>> +	}
>> +	crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE);
> 
> Depending on the value of regs_num, this may result in unaligned access

Thanks for the catch. I think it is not the only place where unaligned
issues are possible. Since the data records are 4-byte address + 2-byte
value there is also an issue with reading the address entries. Assuming
fw->data is aligned to at least 2-bytes (not quite sure if we can assume
that) there should be no problem with reading 2-byte register values.

We could change the data types of the register values from u16 to u32,
wasting some memory (there is approximately 3 000 records), so there is
no other data types in the file structure than u32. Or use a patch as
below. Not sure what's better.

8<---------------------------------------------------------------------
From a970480b99bdb74e2bf48e1a321724231e6516a0 Mon Sep 17 00:00:00 2001
From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
Date: Sun, 9 Sep 2012 19:56:31 +0200
Subject: [PATCH] s5k4ecgx: Fix unaligned access issues

Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
---
 drivers/media/i2c/s5k4ecgx.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

Comments

Francesco Lavra Sept. 10, 2012, 6:52 p.m. UTC | #1 
Hi,

On 09/10/2012 05:04 PM, Sylwester Nawrocki wrote:
> Hi,
> 
> On 09/09/2012 06:01 PM, Francesco Lavra wrote:
>>> +static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>> +{
>>> +	const struct firmware *fw;
>>> +	int err, i, regs_num;
>>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> +	u16 val;
>>> +	u32 addr, crc, crc_file, addr_inc = 0;
>>> +
>>> +	err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
>>> +	if (err) {
>>> +		v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE);
>>> +		return err;
>>> +	}
>>> +	regs_num = *(u32 *)(fw->data);
>>> +	v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",
>>> +		 S5K4ECGX_FIRMWARE, fw->size, regs_num);
>>> +	regs_num++; /* Add header */
>>> +	if (fw->size != regs_num * FW_RECORD_SIZE + FW_CRC_SIZE) {
>>> +		err = -EINVAL;
>>> +		goto fw_out;
>>> +	}
>>> +	crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE);
>>
>> Depending on the value of regs_num, this may result in unaligned access
> 
> Thanks for the catch. I think it is not the only place where unaligned
> issues are possible. Since the data records are 4-byte address + 2-byte
> value there is also an issue with reading the address entries. Assuming
> fw->data is aligned to at least 2-bytes (not quite sure if we can assume
> that) there should be no problem with reading 2-byte register values.

I'm not sure 2-byte alignment can be safely assumed, either.

> We could change the data types of the register values from u16 to u32,
> wasting some memory (there is approximately 3 000 records), so there is
> no other data types in the file structure than u32. Or use a patch as
> below. Not sure what's better.

I prefer the approach of your patch below, but I would use get_unaligned
to get the 2-byte values, too. Also there are another couple of
glitches, see below.

> 
> 8<---------------------------------------------------------------------
> From a970480b99bdb74e2bf48e1a321724231e6516a0 Mon Sep 17 00:00:00 2001
> From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> Date: Sun, 9 Sep 2012 19:56:31 +0200
> Subject: [PATCH] s5k4ecgx: Fix unaligned access issues
> 
> Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com>
> ---
>  drivers/media/i2c/s5k4ecgx.c |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
> index 0ef0b7d..4c6439a 100644
> --- a/drivers/media/i2c/s5k4ecgx.c
> +++ b/drivers/media/i2c/s5k4ecgx.c
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
> +#include <asm/unaligned.h>
> 
>  #include <media/media-entity.h>
>  #include <media/s5k4ecgx.h>
> @@ -331,6 +332,7 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>  	const struct firmware *fw;
>  	int err, i, regs_num;
>  	u32 addr, crc, crc_file, addr_inc = 0;
> +	const u8 *ptr;
>  	u16 val;
> 
>  	err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
> @@ -338,7 +340,7 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>  		v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE);
>  		return err;
>  	}
> -	regs_num = le32_to_cpu(*(u32 *)fw->data);
> +	regs_num = le32_to_cpu(get_unaligned((__le32 *)fw->data));
> 
>  	v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",
>  		 S5K4ECGX_FIRMWARE, fw->size, regs_num);
> @@ -349,7 +351,8 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>  		goto fw_out;
>  	}
> 
> -	crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE);
> +	memcpy(&crc_file, fw->data + regs_num * FW_RECORD_SIZE, sizeof(u32));

crc_file should be converted from little endian to native endian.

> +
>  	crc = crc32_le(~0, fw->data, regs_num * FW_RECORD_SIZE);
>  	if (crc != crc_file) {
>  		v4l2_err(sd, "FW: invalid crc (%#x:%#x)\n", crc, crc_file);
> @@ -357,9 +360,14 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>  		goto fw_out;
>  	}
> 
> +	ptr = fw->data + FW_RECORD_SIZE;
> +
>  	for (i = 1; i < regs_num; i++) {
> -		addr = le32_to_cpu(*(u32 *)(fw->data + i * FW_RECORD_SIZE));
> -		val = le16_to_cpu(*(u16 *)(fw->data + i * FW_RECORD_SIZE + 4));
> +		addr = le32_to_cpu(get_unaligned((__le32 *)ptr));
> +		ptr += 4;
> +		val = le16_to_cpu(*(__le16 *)ptr);
> +		ptr += FW_RECORD_SIZE;

ptr is being incremented by (4 + FW_RECORD_SIZE) bytes at each iteration.

> +
>  		if (addr - addr_inc != 2)
>  			err = s5k4ecgx_write(client, addr, val);
>  		else

--
Francesco
Sylwester Nawrocki Sept. 10, 2012, 8:29 p.m. UTC | #2 
On 09/10/2012 08:52 PM, Francesco Lavra wrote:
> On 09/10/2012 05:04 PM, Sylwester Nawrocki wrote:
>> On 09/09/2012 06:01 PM, Francesco Lavra wrote:
>>>> +static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>>> +{
>>>> +	const struct firmware *fw;
>>>> +	int err, i, regs_num;
>>>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> +	u16 val;
>>>> +	u32 addr, crc, crc_file, addr_inc = 0;
>>>> +
>>>> +	err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
>>>> +	if (err) {
>>>> +		v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE);
>>>> +		return err;
>>>> +	}
>>>> +	regs_num = *(u32 *)(fw->data);
>>>> +	v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",
>>>> +		 S5K4ECGX_FIRMWARE, fw->size, regs_num);
>>>> +	regs_num++; /* Add header */
>>>> +	if (fw->size != regs_num * FW_RECORD_SIZE + FW_CRC_SIZE) {
>>>> +		err = -EINVAL;
>>>> +		goto fw_out;
>>>> +	}
>>>> +	crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE);
>>>
>>> Depending on the value of regs_num, this may result in unaligned access
>>
>> Thanks for the catch. I think it is not the only place where unaligned
>> issues are possible. Since the data records are 4-byte address + 2-byte
>> value there is also an issue with reading the address entries. Assuming
>> fw->data is aligned to at least 2-bytes (not quite sure if we can assume
>> that) there should be no problem with reading 2-byte register values.
> 
> I'm not sure 2-byte alignment can be safely assumed, either.
> 
>> We could change the data types of the register values from u16 to u32,
>> wasting some memory (there is approximately 3 000 records), so there is
>> no other data types in the file structure than u32. Or use a patch as
>> below. Not sure what's better.
> 
> I prefer the approach of your patch below, but I would use get_unaligned
> to get the 2-byte values, too. Also there are another couple of
> glitches, see below.

OK, thanks for the feedback. It was also my preference. The performance
impact seems insignificant, given a write of each record takes time of
1 ms order.

>> 8<---------------------------------------------------------------------
>>  From a970480b99bdb74e2bf48e1a321724231e6516a0 Mon Sep 17 00:00:00 2001
>> From: Sylwester Nawrocki<sylvester.nawrocki@gmail.com>
>> Date: Sun, 9 Sep 2012 19:56:31 +0200
>> Subject: [PATCH] s5k4ecgx: Fix unaligned access issues
>>
>> Signed-off-by: Sylwester Nawrocki<sylvester.nawrocki@gmail.com>
>> ---
>>   drivers/media/i2c/s5k4ecgx.c |   16 ++++++++++++----
>>   1 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
>> index 0ef0b7d..4c6439a 100644
>> --- a/drivers/media/i2c/s5k4ecgx.c
>> +++ b/drivers/media/i2c/s5k4ecgx.c
>> @@ -24,6 +24,7 @@
>>   #include<linux/module.h>
>>   #include<linux/regulator/consumer.h>
>>   #include<linux/slab.h>
>> +#include<asm/unaligned.h>
>>
>>   #include<media/media-entity.h>
>>   #include<media/s5k4ecgx.h>
>> @@ -331,6 +332,7 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>   	const struct firmware *fw;
>>   	int err, i, regs_num;
>>   	u32 addr, crc, crc_file, addr_inc = 0;
>> +	const u8 *ptr;
>>   	u16 val;
>>
>>   	err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
>> @@ -338,7 +340,7 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>   		v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE);
>>   		return err;
>>   	}
>> -	regs_num = le32_to_cpu(*(u32 *)fw->data);
>> +	regs_num = le32_to_cpu(get_unaligned((__le32 *)fw->data));
>>
>>   	v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",
>>   		 S5K4ECGX_FIRMWARE, fw->size, regs_num);
>> @@ -349,7 +351,8 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>   		goto fw_out;
>>   	}
>>
>> -	crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE);
>> +	memcpy(&crc_file, fw->data + regs_num * FW_RECORD_SIZE, sizeof(u32));
> 
> crc_file should be converted from little endian to native endian.

Right, I should have verified that crc32_le() return value is in native
endianness.

>> +
>>   	crc = crc32_le(~0, fw->data, regs_num * FW_RECORD_SIZE);
>>   	if (crc != crc_file) {
>>   		v4l2_err(sd, "FW: invalid crc (%#x:%#x)\n", crc, crc_file);
>> @@ -357,9 +360,14 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>   		goto fw_out;
>>   	}
>>
>> +	ptr = fw->data + FW_RECORD_SIZE;
>> +
>>   	for (i = 1; i<  regs_num; i++) {
>> -		addr = le32_to_cpu(*(u32 *)(fw->data + i * FW_RECORD_SIZE));
>> -		val = le16_to_cpu(*(u16 *)(fw->data + i * FW_RECORD_SIZE + 4));
>> +		addr = le32_to_cpu(get_unaligned((__le32 *)ptr));
>> +		ptr += 4;
>> +		val = le16_to_cpu(*(__le16 *)ptr);
>> +		ptr += FW_RECORD_SIZE;
> 
> ptr is being incremented by (4 + FW_RECORD_SIZE) bytes at each iteration.

Oops, I was to quick in sending that patch out. Indeed, that's wrong.
Sangwook, FWIW, I just pushed the corrected patch to my tree
(http://git.linuxtv.org/snawrocki/media.git/commitdiff/4a0ecad6f08ccbba3)

--

Thanks,
Sylwester
Sangwook Sept. 11, 2012, 10 a.m. UTC | #3 
Hi  Francesco

Thanks for your advice.

@ Sylwester
Thanks for your nice patch, and I will squash  and then send it again.


Thanks
Sangwook


On 10 September 2012 21:29, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> On 09/10/2012 08:52 PM, Francesco Lavra wrote:
>> On 09/10/2012 05:04 PM, Sylwester Nawrocki wrote:
>>> On 09/09/2012 06:01 PM, Francesco Lavra wrote:
>>>>> +static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>>>> +{
>>>>> +  const struct firmware *fw;
>>>>> +  int err, i, regs_num;
>>>>> +  struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>>> +  u16 val;
>>>>> +  u32 addr, crc, crc_file, addr_inc = 0;
>>>>> +
>>>>> +  err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
>>>>> +  if (err) {
>>>>> +          v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE);
>>>>> +          return err;
>>>>> +  }
>>>>> +  regs_num = *(u32 *)(fw->data);
>>>>> +  v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",
>>>>> +           S5K4ECGX_FIRMWARE, fw->size, regs_num);
>>>>> +  regs_num++; /* Add header */
>>>>> +  if (fw->size != regs_num * FW_RECORD_SIZE + FW_CRC_SIZE) {
>>>>> +          err = -EINVAL;
>>>>> +          goto fw_out;
>>>>> +  }
>>>>> +  crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE);
>>>>
>>>> Depending on the value of regs_num, this may result in unaligned access
>>>
>>> Thanks for the catch. I think it is not the only place where unaligned
>>> issues are possible. Since the data records are 4-byte address + 2-byte
>>> value there is also an issue with reading the address entries. Assuming
>>> fw->data is aligned to at least 2-bytes (not quite sure if we can assume
>>> that) there should be no problem with reading 2-byte register values.
>>
>> I'm not sure 2-byte alignment can be safely assumed, either.
>>
>>> We could change the data types of the register values from u16 to u32,
>>> wasting some memory (there is approximately 3 000 records), so there is
>>> no other data types in the file structure than u32. Or use a patch as
>>> below. Not sure what's better.
>>
>> I prefer the approach of your patch below, but I would use get_unaligned
>> to get the 2-byte values, too. Also there are another couple of
>> glitches, see below.
>
> OK, thanks for the feedback. It was also my preference. The performance
> impact seems insignificant, given a write of each record takes time of
> 1 ms order.
>
>>> 8<---------------------------------------------------------------------
>>>  From a970480b99bdb74e2bf48e1a321724231e6516a0 Mon Sep 17 00:00:00 2001
>>> From: Sylwester Nawrocki<sylvester.nawrocki@gmail.com>
>>> Date: Sun, 9 Sep 2012 19:56:31 +0200
>>> Subject: [PATCH] s5k4ecgx: Fix unaligned access issues
>>>
>>> Signed-off-by: Sylwester Nawrocki<sylvester.nawrocki@gmail.com>
>>> ---
>>>   drivers/media/i2c/s5k4ecgx.c |   16 ++++++++++++----
>>>   1 files changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
>>> index 0ef0b7d..4c6439a 100644
>>> --- a/drivers/media/i2c/s5k4ecgx.c
>>> +++ b/drivers/media/i2c/s5k4ecgx.c
>>> @@ -24,6 +24,7 @@
>>>   #include<linux/module.h>
>>>   #include<linux/regulator/consumer.h>
>>>   #include<linux/slab.h>
>>> +#include<asm/unaligned.h>
>>>
>>>   #include<media/media-entity.h>
>>>   #include<media/s5k4ecgx.h>
>>> @@ -331,6 +332,7 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>>      const struct firmware *fw;
>>>      int err, i, regs_num;
>>>      u32 addr, crc, crc_file, addr_inc = 0;
>>> +    const u8 *ptr;
>>>      u16 val;
>>>
>>>      err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
>>> @@ -338,7 +340,7 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>>              v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE);
>>>              return err;
>>>      }
>>> -    regs_num = le32_to_cpu(*(u32 *)fw->data);
>>> +    regs_num = le32_to_cpu(get_unaligned((__le32 *)fw->data));
>>>
>>>      v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",
>>>               S5K4ECGX_FIRMWARE, fw->size, regs_num);
>>> @@ -349,7 +351,8 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>>              goto fw_out;
>>>      }
>>>
>>> -    crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE);
>>> +    memcpy(&crc_file, fw->data + regs_num * FW_RECORD_SIZE, sizeof(u32));
>>
>> crc_file should be converted from little endian to native endian.
>
> Right, I should have verified that crc32_le() return value is in native
> endianness.
>
>>> +
>>>      crc = crc32_le(~0, fw->data, regs_num * FW_RECORD_SIZE);
>>>      if (crc != crc_file) {
>>>              v4l2_err(sd, "FW: invalid crc (%#x:%#x)\n", crc, crc_file);
>>> @@ -357,9 +360,14 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>>              goto fw_out;
>>>      }
>>>
>>> +    ptr = fw->data + FW_RECORD_SIZE;
>>> +
>>>      for (i = 1; i<  regs_num; i++) {
>>> -            addr = le32_to_cpu(*(u32 *)(fw->data + i * FW_RECORD_SIZE));
>>> -            val = le16_to_cpu(*(u16 *)(fw->data + i * FW_RECORD_SIZE + 4));
>>> +            addr = le32_to_cpu(get_unaligned((__le32 *)ptr));
>>> +            ptr += 4;
>>> +            val = le16_to_cpu(*(__le16 *)ptr);
>>> +            ptr += FW_RECORD_SIZE;
>>
>> ptr is being incremented by (4 + FW_RECORD_SIZE) bytes at each iteration.
>
> Oops, I was to quick in sending that patch out. Indeed, that's wrong.
> Sangwook, FWIW, I just pushed the corrected patch to my tree
> (http://git.linuxtv.org/snawrocki/media.git/commitdiff/4a0ecad6f08ccbba3)
>
> --
>
> Thanks,
> Sylwester
diff mbox

Patch

diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
index 0ef0b7d..4c6439a 100644
--- a/drivers/media/i2c/s5k4ecgx.c
+++ b/drivers/media/i2c/s5k4ecgx.c
@@ -24,6 +24,7 @@ 
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <asm/unaligned.h>

 #include <media/media-entity.h>
 #include <media/s5k4ecgx.h>
@@ -331,6 +332,7 @@  static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
 	const struct firmware *fw;
 	int err, i, regs_num;
 	u32 addr, crc, crc_file, addr_inc = 0;
+	const u8 *ptr;
 	u16 val;

 	err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
@@ -338,7 +340,7 @@  static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
 		v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE);
 		return err;
 	}
-	regs_num = le32_to_cpu(*(u32 *)fw->data);
+	regs_num = le32_to_cpu(get_unaligned((__le32 *)fw->data));

 	v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",
 		 S5K4ECGX_FIRMWARE, fw->size, regs_num);
@@ -349,7 +351,8 @@  static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
 		goto fw_out;
 	}

-	crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE);
+	memcpy(&crc_file, fw->data + regs_num * FW_RECORD_SIZE, sizeof(u32));
+
 	crc = crc32_le(~0, fw->data, regs_num * FW_RECORD_SIZE);
 	if (crc != crc_file) {
 		v4l2_err(sd, "FW: invalid crc (%#x:%#x)\n", crc, crc_file);
@@ -357,9 +360,14 @@  static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
 		goto fw_out;
 	}

+	ptr = fw->data + FW_RECORD_SIZE;
+
 	for (i = 1; i < regs_num; i++) {
-		addr = le32_to_cpu(*(u32 *)(fw->data + i * FW_RECORD_SIZE));
-		val = le16_to_cpu(*(u16 *)(fw->data + i * FW_RECORD_SIZE + 4));
+		addr = le32_to_cpu(get_unaligned((__le32 *)ptr));
+		ptr += 4;
+		val = le16_to_cpu(*(__le16 *)ptr);
+		ptr += FW_RECORD_SIZE;
+
 		if (addr - addr_inc != 2)
 			err = s5k4ecgx_write(client, addr, val);
 		else