Re: [PATH v3 1/2] v4l: Add factory register values form S5K4ECGX sensor
Hi Sylwester
On 2 August 2012 21:50, Sylwester Nawrocki sylvester.nawro...@gmail.com wrote:
On 08/02/2012 03:42 PM, Sangwook Lee wrote:
Add factory default settings for S5K4ECGX sensor registers,
which was copied from the reference code of Samsung S.LSI.
Signed-off-by: Sangwook Leesangwook@linaro.org
---
drivers/media/video/s5k4ecgx_regs.h | 3105
+++
1 file changed, 3105 insertions(+)
create mode 100644 drivers/media/video/s5k4ecgx_regs.h
diff --git a/drivers/media/video/s5k4ecgx_regs.h
b/drivers/media/video/s5k4ecgx_regs.h
new file mode 100644
index 000..ef87c09
--- /dev/null
+++ b/drivers/media/video/s5k4ecgx_regs.h
@@ -0,0 +1,3105 @@
+/*
+ * Samsung S5K4ECGX register tables for default values
+ *
+ * Copyright (C) 2012 Linaro
+ * Copyright (C) 2012 Insignal Co,. Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __DRIVERS_MEDIA_VIDEO_S5K4ECGX_H__
+#define __DRIVERS_MEDIA_VIDEO_S5K4ECGX_H__
+
+struct regval_list {
+ u32 addr;
+ u16 val;
+};
+
+/*
+ * FIXME:
+ * The tables are default values of a S5K4ECGX sensor EVT1.1
+ * from Samsung LSI. currently there is no information available
+ * to the public in order to reduce these tables size.
+ */
+static const struct regval_list s5k4ecgx_apb_regs[] = {
sniiip
+/* configure 30 fps */
+static const struct regval_list s5k4ecgx_fps_30[] = {
It really depends on sensor master clock frequency (as specified
in FIMC driver platform data) and PLL setting what the resulting
frame rate will be.
+ { 0x72b4, 0x0052 },
Looks surprising! Are we really just disabling horizontal/vertical
image mirror here ?
I believe, this setting values are used still in Galaxy Nexus.
It might be some reasons to set this values in the product, but I am not
sure of this.
+ { 0x72d2, 0x },
REG_0TC_PCFG_uCaptureMirror
+ { 0x7266, 0x },
REG_TC_GP_ActivePrevConfig
+ { 0x726a, 0x0001 },
REG_TC_GP_PrevOpenAfterChange
+ { 0x724e, 0x0001 },
REG_TC_GP_NewConfigSync
+ { 0x7268, 0x0001 },
REG_TC_GP_PrevConfigChanged
Please have a look how it is handled in s5k6aa driver, it all looks
pretty similar.
+ { 0x, 0x },
+};
+
+static const struct regval_list s5k4ecgx_effect_normal[] = {
+ { 0x723c, 0x },
Just one register, why do we need an array for it ? And of course
0x is default value after reset, so it seems sort of pointless
doing this I2C write to set the default image effect value (disabled).
These are possible values as found in the datasheet:
0x723C REG_TC_GP_SpecialEffects 0x 2 RW Special effect
0 : Normal
1 : MONOCHROME (BW)
2 : Negative Mono
3 : Negative Color
4 : Sepia
5 : AQUA
6 : Reddish
7 : Bluish
8 : Greenish
9 : Sketch
10 : Emboss color
11 : Emboss Mono
+ { 0x, 0x },
+};
+
+static const struct regval_list s5k4ecgx_wb_auto[] = {
+ { 0x74e6, 0x077f },
Ditto - register REG_TC_DBG_AutoAlgEnBits. And 0x077f is the default
value after reset...
+ { 0x, 0x },
+};
+
+static const struct regval_list s5k4ecgx_iso_auto[] = {
+ { 0x7938, 0x },
+ { 0x7f2a, 0x0001 },
+ { 0x74e6, 0x077f },
+ { 0x74d0, 0x },
+ { 0x74d2, 0x },
+ { 0x74d4, 0x0001 },
+ { 0x76c2, 0x0200 },
+ { 0x, 0x },
+};
+
+static const struct regval_list s5k4ecgx_contrast_default[] = {
+ { 0x7232, 0x },
No need for an array, it's REG_TC_UserContrast.
+ { 0x, 0x },
+};
+
[snip]
+ { 0x, 0x },
+};
You already use a sequence of i2c writes in s5k4ecgx_s_ctrl() function
for V4L2_CID_SHARPNESS control. So why not just create e.g.
s5k4ecgx_set_saturation() and send this array to /dev/null ?
Also, invoking v4l2_ctrl_handler_setup() will cause a call to
s5k4ecgx_s_ctrl()
with default sharpness value (as specified during the control's creation).
So I would say this array is redundant in two ways... :)
Thanks, let me change this.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATH v3 1/2] v4l: Add factory register values form S5K4ECGX sensor
Hi Sangwook,
On 08/03/2012 05:05 PM, Sangwook Lee wrote:
+/* configure 30 fps */
+static const struct regval_list s5k4ecgx_fps_30[] = {
It really depends on sensor master clock frequency (as specified
in FIMC driver platform data) and PLL setting what the resulting
frame rate will be.
+ { 0x72b4, 0x0052 },
Looks surprising! Are we really just disabling horizontal/vertical
image mirror here ?
I believe, this setting values are used still in Galaxy Nexus.
It might be some reasons to set this values in the product, but I am not
sure of this.
My point was that some entries in this table allegedly are setting image
mirroring, even though the array name suggests it should be only setting
frame rate to 30 fps. This is just bad practice. If you would have added
HFLIP/VFLIP controls, the register values would have been trashed every
time frame rate is set. Someone would have eventually have had to get rid
of this s5k4ecgx_fps_30 array, in order to add new features.
[snip]
+ { 0x, 0x },
+};
You already use a sequence of i2c writes in s5k4ecgx_s_ctrl() function
for V4L2_CID_SHARPNESS control. So why not just create e.g.
s5k4ecgx_set_saturation() and send this array to /dev/null ?
Also, invoking v4l2_ctrl_handler_setup() will cause a call to
s5k4ecgx_s_ctrl()
with default sharpness value (as specified during the control's creation).
So I would say this array is redundant in two ways... :)
Thanks, let me change this.
Thanks, please at least remove those single entry arrays, with the
resolution arrays gone as well and the biggest array converted to
firmware blob I don't see a reason why this driver couldn't be accepted
upstream.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATH v3 1/2] v4l: Add factory register values form S5K4ECGX sensor
On 08/02/2012 03:42 PM, Sangwook Lee wrote:
Add factory default settings for S5K4ECGX sensor registers,
which was copied from the reference code of Samsung S.LSI.
Signed-off-by: Sangwook Leesangwook@linaro.org
---
drivers/media/video/s5k4ecgx_regs.h | 3105
+++
1 file changed, 3105 insertions(+)
create mode 100644 drivers/media/video/s5k4ecgx_regs.h
diff --git a/drivers/media/video/s5k4ecgx_regs.h
b/drivers/media/video/s5k4ecgx_regs.h
new file mode 100644
index 000..ef87c09
--- /dev/null
+++ b/drivers/media/video/s5k4ecgx_regs.h
@@ -0,0 +1,3105 @@
+/*
+ * Samsung S5K4ECGX register tables for default values
+ *
+ * Copyright (C) 2012 Linaro
+ * Copyright (C) 2012 Insignal Co,. Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __DRIVERS_MEDIA_VIDEO_S5K4ECGX_H__
+#define __DRIVERS_MEDIA_VIDEO_S5K4ECGX_H__
+
+struct regval_list {
+ u32 addr;
+ u16 val;
+};
+
+/*
+ * FIXME:
+ * The tables are default values of a S5K4ECGX sensor EVT1.1
+ * from Samsung LSI. currently there is no information available
+ * to the public in order to reduce these tables size.
+ */
+static const struct regval_list s5k4ecgx_apb_regs[] = {
sniiip
+/* configure 30 fps */
+static const struct regval_list s5k4ecgx_fps_30[] = {
It really depends on sensor master clock frequency (as specified
in FIMC driver platform data) and PLL setting what the resulting
frame rate will be.
+ { 0x72b4, 0x0052 },
REG_0TC_PCFG_PVIMask
+ { 0x72be, 0x },
REG_0TC_PCFG_usFrTimeType
+ { 0x72c0, 0x0001 },
REG_0TC_PCFG_FrRateQualityType
+ { 0x72c2, 0x014d },
REG_0TC_PCFG_usMaxFrTimeMsecMult10
+ { 0x72c4, 0x014d },
REG_0TC_PCFG_usMinFrTimeMsecMult10
+ { 0x72d0, 0x },
REG_0TC_PCFG_uPrevMirror
Looks surprising! Are we really just disabling horizontal/vertical
image mirror here ?
+ { 0x72d2, 0x },
REG_0TC_PCFG_uCaptureMirror
+ { 0x7266, 0x },
REG_TC_GP_ActivePrevConfig
+ { 0x726a, 0x0001 },
REG_TC_GP_PrevOpenAfterChange
+ { 0x724e, 0x0001 },
REG_TC_GP_NewConfigSync
+ { 0x7268, 0x0001 },
REG_TC_GP_PrevConfigChanged
Please have a look how it is handled in s5k6aa driver, it all looks
pretty similar.
+ { 0x, 0x },
+};
+
+static const struct regval_list s5k4ecgx_effect_normal[] = {
+ { 0x723c, 0x },
Just one register, why do we need an array for it ? And of course
0x is default value after reset, so it seems sort of pointless
doing this I2C write to set the default image effect value (disabled).
These are possible values as found in the datasheet:
0x723C REG_TC_GP_SpecialEffects 0x 2 RW Special effect
0 : Normal
1 : MONOCHROME (BW)
2 : Negative Mono
3 : Negative Color
4 : Sepia
5 : AQUA
6 : Reddish
7 : Bluish
8 : Greenish
9 : Sketch
10 : Emboss color
11 : Emboss Mono
+ { 0x, 0x },
+};
+
+static const struct regval_list s5k4ecgx_wb_auto[] = {
+ { 0x74e6, 0x077f },
Ditto - register REG_TC_DBG_AutoAlgEnBits. And 0x077f is the default
value after reset...
+ { 0x, 0x },
+};
+
+static const struct regval_list s5k4ecgx_iso_auto[] = {
+ { 0x7938, 0x },
+ { 0x7f2a, 0x0001 },
+ { 0x74e6, 0x077f },
+ { 0x74d0, 0x },
+ { 0x74d2, 0x },
+ { 0x74d4, 0x0001 },
+ { 0x76c2, 0x0200 },
+ { 0x, 0x },
+};
+
+static const struct regval_list s5k4ecgx_contrast_default[] = {
+ { 0x7232, 0x },
No need for an array, it's REG_TC_UserContrast.
+ { 0x, 0x },
+};
+
+static const struct regval_list s5k4ecgx_scene_default[] = {
+ { 0x70001492, 0x },
+ { 0x70001494, 0x0101 },
+ { 0x70001496, 0x0101 },
+ { 0x70001498, 0x },
+ { 0x7000149a, 0x0101 },
+ { 0x7000149c, 0x0101 },
+ { 0x7000149e, 0x0101 },
+ { 0x700014a0, 0x0101 },
+ { 0x700014a2, 0x0201 },
+ { 0x700014a4, 0x0303 },
+ { 0x700014a6, 0x0303 },
+ { 0x700014a8, 0x0102 },
+ { 0x700014aa, 0x0201 },
+ { 0x700014ac, 0x0403 },
+ { 0x700014ae, 0x0304 },
+ { 0x700014b0, 0x0102 },
+ { 0x700014b2, 0x0201 },
+ { 0x700014b4, 0x0403 },
+ { 0x700014b6, 0x0304 },
+ { 0x700014b8, 0x0102 },
+ { 0x700014ba, 0x0201 },
+ { 0x700014bc, 0x0403 },
+ { 0x700014be, 0x0304 },
+ { 0x700014c0, 0x0102 },
+ { 0x700014c2, 0x0201 },
+ { 0x700014c4, 0x0303 },
+ { 0x700014c6, 0x0303 },
+ { 0x700014c8, 0x0102 },
+ { 0x700014ca, 0x0201 },
+ { 0x700014cc, 0x0202 },
+ { 0x700014ce, 0x0202 },
+ { 0x700014d0, 0x0102 },
+ { 0x7938, 0x },
+ { 0x76b8,
