diff mbox

[v2,3/4] m5mols: remove union in the m5mols_get_version(), and VERSION_SIZE

Message ID 1306827362-4064-4-git-send-email-riverful.kim@samsung.com (mailing list archive)
State RFC
Headers show

Commit Message

Kim, HeungJun May 31, 2011, 7:36 a.m. UTC 
Remove union version in the m5mols_get_version(), and read version information
directly. Also remove VERSION_SIZE.

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/m5mols/m5mols.h      |    1 -
 drivers/media/video/m5mols/m5mols_core.c |   42 +++++++++++++++---------------
 drivers/media/video/m5mols/m5mols_reg.h  |   13 ++++++++-
 3 files changed, 33 insertions(+), 23 deletions(-)

Comments

Sakari Ailus June 5, 2011, 12:03 p.m. UTC | #1 
Hi HeungJun,

Thanks for the patch.

On Tue, May 31, 2011 at 04:36:01PM +0900, HeungJun, Kim wrote:
> Remove union version in the m5mols_get_version(), and read version information
> directly. Also remove VERSION_SIZE.
> 
> Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/m5mols/m5mols.h      |    1 -
>  drivers/media/video/m5mols/m5mols_core.c |   42 +++++++++++++++---------------
>  drivers/media/video/m5mols/m5mols_reg.h  |   13 ++++++++-
>  3 files changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/video/m5mols/m5mols.h b/drivers/media/video/m5mols/m5mols.h
> index dbe8928..9ae1709 100644
> --- a/drivers/media/video/m5mols/m5mols.h
> +++ b/drivers/media/video/m5mols/m5mols.h
> @@ -154,7 +154,6 @@ struct m5mols_version {
>  	u8	str[VERSION_STRING_SIZE];
>  	u8	af;
>  };
> -#define VERSION_SIZE sizeof(struct m5mols_version)
>  
>  /**
>   * struct m5mols_info - M-5MOLS driver data structure
> diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
> index 2b1f23f..8ccab95 100644
> --- a/drivers/media/video/m5mols/m5mols_core.c
> +++ b/drivers/media/video/m5mols/m5mols_core.c
> @@ -386,33 +386,33 @@ int m5mols_mode(struct m5mols_info *info, u8 mode)
>  static int m5mols_get_version(struct v4l2_subdev *sd)
>  {
>  	struct m5mols_info *info = to_m5mols(sd);
> -	union {
> -		struct m5mols_version ver;
> -		u8 bytes[VERSION_SIZE];
> -	} version;
> -	u8 cmd = CAT0_VER_CUSTOMER;
> +	struct m5mols_version *ver = &info->ver;
> +	u8 *str = ver->str;
> +	int i;
>  	int ret;
>  
> -	do {
> -		ret = m5mols_read_u8(sd, SYSTEM_CMD(cmd), &version.bytes[cmd]);
> -		if (ret)
> -			return ret;
> -	} while (cmd++ != CAT0_VER_AWB);
> +	ret = m5mols_read_u8(sd, SYSTEM_VER_CUSTOMER, &ver->customer);
> +	if (!ret)
> +		ret = m5mols_read_u8(sd, SYSTEM_VER_PROJECT, &ver->project);
> +	if (!ret)
> +		ret = m5mols_read_u16(sd, SYSTEM_VER_FIRMWARE, &ver->fw);
> +	if (!ret)
> +		ret = m5mols_read_u16(sd, SYSTEM_VER_HARDWARE, &ver->hw);
> +	if (!ret)
> +		ret = m5mols_read_u16(sd, SYSTEM_VER_PARAMETER, &ver->param);
> +	if (!ret)
> +		ret = m5mols_read_u16(sd, SYSTEM_VER_AWB, &ver->awb);
> +	if (!ret)
> +		ret = m5mols_read_u8(sd, AF_VERSION, &ver->af);
> +	if (ret)
> +		return ret;
>  
> -	do {
> -		ret = m5mols_read_u8(sd, SYSTEM_VER_STRING, &version.bytes[cmd]);
> +	for (i = 0; i < VERSION_STRING_SIZE; i++) {
> +		ret = m5mols_read_u8(sd, SYSTEM_VER_STRING, &str[i]);
>  		if (ret)
>  			return ret;
> -		if (cmd >= VERSION_SIZE - 1)
> -			return -EINVAL;
> -	} while (version.bytes[cmd++]);
> -
> -	ret = m5mols_read_u8(sd, AF_VERSION, &version.bytes[cmd]);
> -	if (ret)
> -		return ret;
> +	}
>  
> -	/* store version information swapped for being readable */
> -	info->ver	= version.ver;
>  	info->ver.fw	= be16_to_cpu(info->ver.fw);
>  	info->ver.hw	= be16_to_cpu(info->ver.hw);
>  	info->ver.param	= be16_to_cpu(info->ver.param);

As you have a local variable ver pointing to info->ver, you should also use
it here.

> diff --git a/drivers/media/video/m5mols/m5mols_reg.h b/drivers/media/video/m5mols/m5mols_reg.h
> index 8260f50..5f5bdcf 100644
> --- a/drivers/media/video/m5mols/m5mols_reg.h
> +++ b/drivers/media/video/m5mols/m5mols_reg.h
> @@ -56,13 +56,24 @@
>   * more specific contents, see definition if file m5mols.h.
>   */
>  #define CAT0_VER_CUSTOMER	0x00	/* customer version */
> -#define CAT0_VER_AWB		0x09	/* Auto WB version */
> +#define CAT0_VER_PROJECT	0x01	/* project version */
> +#define CAT0_VER_FIRMWARE	0x02	/* Firmware version */
> +#define CAT0_VER_HARDWARE	0x04	/* Hardware version */
> +#define CAT0_VER_PARAMETER	0x06	/* Parameter version */
> +#define CAT0_VER_AWB		0x08	/* Auto WB version */
>  #define CAT0_VER_STRING		0x0a	/* string including M-5MOLS */
>  #define CAT0_SYSMODE		0x0b	/* SYSTEM mode register */
>  #define CAT0_STATUS		0x0c	/* SYSTEM mode status register */
>  #define CAT0_INT_FACTOR		0x10	/* interrupt pending register */
>  #define CAT0_INT_ENABLE		0x11	/* interrupt enable register */
>  
> +#define SYSTEM_VER_CUSTOMER	I2C_REG(CAT_SYSTEM, CAT0_VER_CUSTOMER, 1)
> +#define SYSTEM_VER_PROJECT	I2C_REG(CAT_SYSTEM, CAT0_VER_PROJECT, 1)
> +#define SYSTEM_VER_FIRMWARE	I2C_REG(CAT_SYSTEM, CAT0_VER_FIRMWARE, 2)
> +#define SYSTEM_VER_HARDWARE	I2C_REG(CAT_SYSTEM, CAT0_VER_HARDWARE, 2)
> +#define SYSTEM_VER_PARAMETER	I2C_REG(CAT_SYSTEM, CAT0_VER_PARAMETER, 2)
> +#define SYSTEM_VER_AWB		I2C_REG(CAT_SYSTEM, CAT0_VER_AWB, 2)
> +
>  #define SYSTEM_SYSMODE		I2C_REG(CAT_SYSTEM, CAT0_SYSMODE, 1)
>  #define REG_SYSINIT		0x00	/* SYSTEM mode */
>  #define REG_PARAMETER		0x01	/* PARAMETER mode */
> -- 
> 1.7.0.4
> 

Cheers,
Sakari Ailus June 5, 2011, 12:11 p.m. UTC | #2 
On Sun, Jun 05, 2011 at 03:03:47PM +0300, Sakari Ailus wrote:
[clip]
> > -	/* store version information swapped for being readable */
> > -	info->ver	= version.ver;
> >  	info->ver.fw	= be16_to_cpu(info->ver.fw);
> >  	info->ver.hw	= be16_to_cpu(info->ver.hw);
> >  	info->ver.param	= be16_to_cpu(info->ver.param);
> 
> As you have a local variable ver pointing to info->ver, you should also use
> it here.

With this change,

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
riverfulkim June 6, 2011, 9:20 a.m. UTC | #3 
2011. 6. 5., ?? 9:11, Sakari Ailus ??:

> On Sun, Jun 05, 2011 at 03:03:47PM +0300, Sakari Ailus wrote:
> [clip]
>>> -	/* store version information swapped for being readable */
>>> -	info->ver	= version.ver;
>>> 	info->ver.fw	= be16_to_cpu(info->ver.fw);
>>> 	info->ver.hw	= be16_to_cpu(info->ver.hw);
>>> 	info->ver.param	= be16_to_cpu(info->ver.param);
>> 
>> As you have a local variable ver pointing to info->ver, you should also use
>> it here.
> 
> With this change,
Ok, I missed that. I'll fix this and resend another version.

Thanks!


> 
> Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> -- 
> Sakari Ailus
> sakari dot ailus at iki dot fi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/video/m5mols/m5mols.h b/drivers/media/video/m5mols/m5mols.h
index dbe8928..9ae1709 100644
--- a/drivers/media/video/m5mols/m5mols.h
+++ b/drivers/media/video/m5mols/m5mols.h
@@ -154,7 +154,6 @@  struct m5mols_version {
 	u8	str[VERSION_STRING_SIZE];
 	u8	af;
 };
-#define VERSION_SIZE sizeof(struct m5mols_version)
 
 /**
  * struct m5mols_info - M-5MOLS driver data structure
diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
index 2b1f23f..8ccab95 100644
--- a/drivers/media/video/m5mols/m5mols_core.c
+++ b/drivers/media/video/m5mols/m5mols_core.c
@@ -386,33 +386,33 @@  int m5mols_mode(struct m5mols_info *info, u8 mode)
 static int m5mols_get_version(struct v4l2_subdev *sd)
 {
 	struct m5mols_info *info = to_m5mols(sd);
-	union {
-		struct m5mols_version ver;
-		u8 bytes[VERSION_SIZE];
-	} version;
-	u8 cmd = CAT0_VER_CUSTOMER;
+	struct m5mols_version *ver = &info->ver;
+	u8 *str = ver->str;
+	int i;
 	int ret;
 
-	do {
-		ret = m5mols_read_u8(sd, SYSTEM_CMD(cmd), &version.bytes[cmd]);
-		if (ret)
-			return ret;
-	} while (cmd++ != CAT0_VER_AWB);
+	ret = m5mols_read_u8(sd, SYSTEM_VER_CUSTOMER, &ver->customer);
+	if (!ret)
+		ret = m5mols_read_u8(sd, SYSTEM_VER_PROJECT, &ver->project);
+	if (!ret)
+		ret = m5mols_read_u16(sd, SYSTEM_VER_FIRMWARE, &ver->fw);
+	if (!ret)
+		ret = m5mols_read_u16(sd, SYSTEM_VER_HARDWARE, &ver->hw);
+	if (!ret)
+		ret = m5mols_read_u16(sd, SYSTEM_VER_PARAMETER, &ver->param);
+	if (!ret)
+		ret = m5mols_read_u16(sd, SYSTEM_VER_AWB, &ver->awb);
+	if (!ret)
+		ret = m5mols_read_u8(sd, AF_VERSION, &ver->af);
+	if (ret)
+		return ret;
 
-	do {
-		ret = m5mols_read_u8(sd, SYSTEM_VER_STRING, &version.bytes[cmd]);
+	for (i = 0; i < VERSION_STRING_SIZE; i++) {
+		ret = m5mols_read_u8(sd, SYSTEM_VER_STRING, &str[i]);
 		if (ret)
 			return ret;
-		if (cmd >= VERSION_SIZE - 1)
-			return -EINVAL;
-	} while (version.bytes[cmd++]);
-
-	ret = m5mols_read_u8(sd, AF_VERSION, &version.bytes[cmd]);
-	if (ret)
-		return ret;
+	}
 
-	/* store version information swapped for being readable */
-	info->ver	= version.ver;
 	info->ver.fw	= be16_to_cpu(info->ver.fw);
 	info->ver.hw	= be16_to_cpu(info->ver.hw);
 	info->ver.param	= be16_to_cpu(info->ver.param);
diff --git a/drivers/media/video/m5mols/m5mols_reg.h b/drivers/media/video/m5mols/m5mols_reg.h
index 8260f50..5f5bdcf 100644
--- a/drivers/media/video/m5mols/m5mols_reg.h
+++ b/drivers/media/video/m5mols/m5mols_reg.h
@@ -56,13 +56,24 @@ 
  * more specific contents, see definition if file m5mols.h.
  */
 #define CAT0_VER_CUSTOMER	0x00	/* customer version */
-#define CAT0_VER_AWB		0x09	/* Auto WB version */
+#define CAT0_VER_PROJECT	0x01	/* project version */
+#define CAT0_VER_FIRMWARE	0x02	/* Firmware version */
+#define CAT0_VER_HARDWARE	0x04	/* Hardware version */
+#define CAT0_VER_PARAMETER	0x06	/* Parameter version */
+#define CAT0_VER_AWB		0x08	/* Auto WB version */
 #define CAT0_VER_STRING		0x0a	/* string including M-5MOLS */
 #define CAT0_SYSMODE		0x0b	/* SYSTEM mode register */
 #define CAT0_STATUS		0x0c	/* SYSTEM mode status register */
 #define CAT0_INT_FACTOR		0x10	/* interrupt pending register */
 #define CAT0_INT_ENABLE		0x11	/* interrupt enable register */
 
+#define SYSTEM_VER_CUSTOMER	I2C_REG(CAT_SYSTEM, CAT0_VER_CUSTOMER, 1)
+#define SYSTEM_VER_PROJECT	I2C_REG(CAT_SYSTEM, CAT0_VER_PROJECT, 1)
+#define SYSTEM_VER_FIRMWARE	I2C_REG(CAT_SYSTEM, CAT0_VER_FIRMWARE, 2)
+#define SYSTEM_VER_HARDWARE	I2C_REG(CAT_SYSTEM, CAT0_VER_HARDWARE, 2)
+#define SYSTEM_VER_PARAMETER	I2C_REG(CAT_SYSTEM, CAT0_VER_PARAMETER, 2)
+#define SYSTEM_VER_AWB		I2C_REG(CAT_SYSTEM, CAT0_VER_AWB, 2)
+
 #define SYSTEM_SYSMODE		I2C_REG(CAT_SYSTEM, CAT0_SYSMODE, 1)
 #define REG_SYSINIT		0x00	/* SYSTEM mode */
 #define REG_PARAMETER		0x01	/* PARAMETER mode */