[RFC PATCH v8] media: add v4l2 subdev driver for S5K4ECGX sensor

2012-09-13 Thread Sangwook Lee
This patch adds driver for S5K4ECGX sensor with embedded ISP SoC,
S5K4ECGX, which is a 5M CMOS Image sensor from Samsung
The driver implements preview mode of the S5K4ECGX sensor.
capture (snapshot) operation, face detection are missing now.
Following controls are supported:
contrast/saturation/brightness/sharpness

Signed-off-by: Sangwook Lee 
Reviewed-by: Sylwester Nawrocki 
Cc: Francesco Lavra 
Cc: Scott Bambrough 
Cc: Homin Lee 
---
Changes since v7:
- added gpio free function
- fixed return value of power function

Changes since v6:
- fixed alignment issue from Francesco, Sylwester

Changes since v5:
- deleted dummy lines
- fixed pointer errors in handling firmware
- updated comments
- added le32_to_cpu,le16_to_cpu

Changes since v4:
- replaced register tables with the function from Sylwester
- updated firmware parsing function with CRC32 check
  firmware generator from user space:
  git://git.linaro.org/people/sangwook/fimc-v4l2-app.git

Changes since v3:
- used request_firmware to configure initial settings
- added parsing functions to read initial settings
- updated regulator API
- reduced preview setting tables by experiment

Changes since v2:
- added GPIO (reset/stby) and regulators
- updated I2C read/write, based on s5k6aa datasheet
- fixed set_fmt errors
- reduced register tables a bit
- removed vmalloc

Changes since v1:
- fixed s_stream(0) when it called twice
- changed mutex_X position to be used when strictly necessary
- add additional s_power(0) in case that error happens
- update more accurate debugging statements
- remove dummy else

 drivers/media/i2c/Kconfig|7 +
 drivers/media/i2c/Makefile   |1 +
 drivers/media/i2c/s5k4ecgx.c | 1017 ++
 include/media/s5k4ecgx.h |   37 ++
 4 files changed, 1062 insertions(+)
 create mode 100644 drivers/media/i2c/s5k4ecgx.c
 create mode 100644 include/media/s5k4ecgx.h

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 9a5a059..55b3bbb 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -484,6 +484,13 @@ config VIDEO_S5K6AA
  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
  camera sensor with an embedded SoC image signal processor.
 
+config VIDEO_S5K4ECGX
+tristate "Samsung S5K4ECGX sensor support"
+depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+---help---
+  This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
+  camera sensor with an embedded SoC image signal processor.
+
 source "drivers/media/i2c/smiapp/Kconfig"
 
 comment "Flash devices"
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 088a460..a720812 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
 obj-$(CONFIG_VIDEO_SR030PC30)  += sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)+= noon010pc30.o
 obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
+obj-$(CONFIG_VIDEO_S5K4ECGX)   += s5k4ecgx.o
 obj-$(CONFIG_VIDEO_ADP1653)+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)+= as3645a.o
 obj-$(CONFIG_VIDEO_SMIAPP_PLL) += smiapp-pll.o
diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
new file mode 100644
index 000..7b455e1
--- /dev/null
+++ b/drivers/media/i2c/s5k4ecgx.c
@@ -0,0 +1,1017 @@
+/*
+ * Driver for s5k4ecgx (5MP Camera) from Samsung
+ * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
+ * CMOS image sensor.
+ *
+ * Copyright (C) 2012, Linaro, Sangwook Lee 
+ * Copyright (C) 2012, Insignal Co,. Ltd,  Homin Lee 
+ *
+ * Based on s5k6aa, noon010pc30 driver
+ * Copyright (C) 2011, Samsung Electronics Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define S5K4ECGX_DRIVER_NAME   "s5k4ecgx"
+#define S5K4ECGX_FIRMWARE  "s5k4ecgx.bin"
+
+/* Firmware revision information */
+#define REG_FW_REVISION0x71a6
+#define REG_FW_VERSION 0x71a4
+#define S5K4ECGX_REVISION_1_1  0x11
+#define S5K4ECGX_FW_VERSION0x4ec0
+
+/* General purpose parameters */
+#define REG_USER_BRIGHTNESS0x722c
+#define REG_USER_CONTRAST  0x722e
+#define REG_USER_SATURATION0x7230
+
+#define REG_G_NEW_CFG_SYNC 0x724a
+#define REG_G_PREV_IN_WIDTH0x7250
+#define REG_G_PREV_IN_HEIGHT   0x7252
+#define REG_

Re: [RFC PATCH v7] media: add v4l2 subdev driver for S5K4ECGX sensor

2012-09-13 Thread Sangwook Lee
Hi Francesco

On 12 September 2012 19:07, Francesco Lavra  wrote:
> Hi Sangwook,
> two remarks from my review on September 9th haven't been addressed.

Thanks for the review.
I missed those, please let me correct them and send patch again.

Regards
Sangwook


> I believe those remarks are correct, but please let me know if I'm
> missing something.
> See below.
>
> On 09/12/2012 01:26 PM, Sangwook Lee wrote:
>> +static int s5k4ecgx_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> + struct s5k4ecgx *priv = to_s5k4ecgx(sd);
>> + int ret;
>> +
>> + v4l2_dbg(1, debug, sd, "Switching %s\n", on ? "on" : "off");
>> +
>> + if (on) {
>> + ret = __s5k4ecgx_power_on(priv);
>> + if (ret < 0)
>> + return ret;
>> + /* Time to stabilize sensor */
>> + msleep(100);
>> + ret = s5k4ecgx_init_sensor(sd);
>> + if (ret < 0)
>> + __s5k4ecgx_power_off(priv);
>> + else
>> + priv->set_params = 1;
>> + } else {
>> + ret = __s5k4ecgx_power_off(priv);
>> + }
>> +
>> + return 0;
>
> return ret;
>
>> +static int s5k4ecgx_probe(struct i2c_client *client,
>> +   const struct i2c_device_id *id)
>> +{
>> + int ret, i;
>> + struct v4l2_subdev *sd;
>> + struct s5k4ecgx *priv;
>> + struct s5k4ecgx_platform_data *pdata = client->dev.platform_data;
>> +
>> + if (pdata == NULL) {
>> + dev_err(&client->dev, "platform data is missing!\n");
>> + return -EINVAL;
>> + }
>> + priv = devm_kzalloc(&client->dev, sizeof(struct s5k4ecgx), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + mutex_init(&priv->lock);
>> + priv->streaming = 0;
>> +
>> + sd = &priv->sd;
>> + /* Registering subdev */
>> + v4l2_i2c_subdev_init(sd, client, &s5k4ecgx_ops);
>> + strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
>> +
>> + sd->internal_ops = &s5k4ecgx_subdev_internal_ops;
>> + /* Support v4l2 sub-device user space API */
>> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +
>> + priv->pad.flags = MEDIA_PAD_FL_SOURCE;
>> + sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
>> + ret = media_entity_init(&sd->entity, 1, &priv->pad, 0);
>> + if (ret)
>> + return ret;
>> +
>> + ret = s5k4ecgx_config_gpios(priv, pdata);
>> + if (ret) {
>> + dev_err(&client->dev, "Failed to set gpios\n");
>> + goto out_err1;
>> + }
>> + for (i = 0; i < S5K4ECGX_NUM_SUPPLIES; i++)
>> + priv->supplies[i].supply = s5k4ecgx_supply_names[i];
>> +
>> + ret = devm_regulator_bulk_get(&client->dev, S5K4ECGX_NUM_SUPPLIES,
>> +  priv->supplies);
>> + if (ret) {
>> + dev_err(&client->dev, "Failed to get regulators\n");
>> + goto out_err2;
>> + }
>> + ret = s5k4ecgx_init_v4l2_ctrls(priv);
>> + if (ret)
>> + goto out_err2;
>> +
>> + priv->curr_pixfmt = &s5k4ecgx_formats[0];
>> + priv->curr_frmsize = &s5k4ecgx_prev_sizes[0];
>> +
>> + return 0;
>> +
>> +out_err2:
>> + s5k4ecgx_free_gpios(priv);
>> +out_err1:
>> + media_entity_cleanup(&priv->sd.entity);
>> +
>> + return ret;
>> +}
>> +
>> +static int s5k4ecgx_remove(struct i2c_client *client)
>> +{
>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> + struct s5k4ecgx *priv = to_s5k4ecgx(sd);
>> +
>> + mutex_destroy(&priv->lock);
>> + v4l2_device_unregister_subdev(sd);
>> + v4l2_ctrl_handler_free(&priv->handler);
>> + media_entity_cleanup(&sd->entity);
>> +
>> + return 0;
>
> s5k4ecgx_free_gpios() should be called to release the GPIOs
>
> Thanks,
> Francesco
--
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


[RFC PATCH v7] media: add v4l2 subdev driver for S5K4ECGX sensor

2012-09-12 Thread Sangwook Lee
This patch adds driver for S5K4ECGX sensor with embedded ISP SoC,
S5K4ECGX, which is a 5M CMOS Image sensor from Samsung
The driver implements preview mode of the S5K4ECGX sensor.
capture (snapshot) operation, face detection are missing now.
Following controls are supported:
contrast/saturation/brightness/sharpness

Signed-off-by: Sangwook Lee 
Reviewed-by: Sylwester Nawrocki 
Cc: Francesco Lavra 
CC: Scott Bambrough 
Cc: Homin Lee 
---
Changes since v6:
- fix alignment issue from Francesco, Sylwester

Changes since v5:
- deleted dummy lines
- fixed pointer errors in handling firmware
- updated comments
- added le32_to_cpu,le16_to_cpu

Changes since v4:
- replaced register tables with the function from Sylwester
- updated firmware parsing function with CRC32 check
  firmware generator from user space:
  git://git.linaro.org/people/sangwook/fimc-v4l2-app.git

Changes since v3:
- used request_firmware to configure initial settings
- added parsing functions to read initial settings
- updated regulator API
- reduced preview setting tables by experiment

Changes since v2:
- added GPIO (reset/stby) and regulators
- updated I2C read/write, based on s5k6aa datasheet
- fixed set_fmt errors
- reduced register tables a bit
- removed vmalloc

Changes since v1:
- fixed s_stream(0) when it called twice
- changed mutex_X position to be used when strictly necessary
- add additional s_power(0) in case that error happens
- update more accurate debugging statements
- remove dummy else
 drivers/media/i2c/Kconfig|7 +
 drivers/media/i2c/Makefile   |1 +
 drivers/media/i2c/s5k4ecgx.c | 1016 ++
 include/media/s5k4ecgx.h |   37 ++
 4 files changed, 1061 insertions(+)
 create mode 100644 drivers/media/i2c/s5k4ecgx.c
 create mode 100644 include/media/s5k4ecgx.h

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 9a5a059..55b3bbb 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -484,6 +484,13 @@ config VIDEO_S5K6AA
  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
  camera sensor with an embedded SoC image signal processor.
 
+config VIDEO_S5K4ECGX
+tristate "Samsung S5K4ECGX sensor support"
+depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+---help---
+  This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
+  camera sensor with an embedded SoC image signal processor.
+
 source "drivers/media/i2c/smiapp/Kconfig"
 
 comment "Flash devices"
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 088a460..a720812 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
 obj-$(CONFIG_VIDEO_SR030PC30)  += sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)+= noon010pc30.o
 obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
+obj-$(CONFIG_VIDEO_S5K4ECGX)   += s5k4ecgx.o
 obj-$(CONFIG_VIDEO_ADP1653)+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)+= as3645a.o
 obj-$(CONFIG_VIDEO_SMIAPP_PLL) += smiapp-pll.o
diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
new file mode 100644
index 000..dd2f68d
--- /dev/null
+++ b/drivers/media/i2c/s5k4ecgx.c
@@ -0,0 +1,1016 @@
+/*
+ * Driver for s5k4ecgx (5MP Camera) from Samsung
+ * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
+ * CMOS image sensor.
+ *
+ * Copyright (C) 2012, Linaro, Sangwook Lee 
+ * Copyright (C) 2012, Insignal Co,. Ltd,  Homin Lee 
+ *
+ * Based on s5k6aa, noon010pc30 driver
+ * Copyright (C) 2011, Samsung Electronics Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define S5K4ECGX_DRIVER_NAME   "s5k4ecgx"
+#define S5K4ECGX_FIRMWARE  "s5k4ecgx.bin"
+
+/* Firmware revision information */
+#define REG_FW_REVISION0x71a6
+#define REG_FW_VERSION 0x71a4
+#define S5K4ECGX_REVISION_1_1  0x11
+#define S5K4ECGX_FW_VERSION0x4ec0
+
+/* General purpose parameters */
+#define REG_USER_BRIGHTNESS0x722c
+#define REG_USER_CONTRAST  0x722e
+#define REG_USER_SATURATION0x7230
+
+#define REG_G_NEW_CFG_SYNC 0x724a
+#define REG_G_PREV_IN_WIDTH0x7250
+#define REG_G_PREV_IN_HEIGHT   0x7252
+#define REG_G_PREV_IN_XOFFS0x7254
+#define REG_G_PREV_IN_YOFFS0x70

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

2012-09-11 Thread Sangwook Lee
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
 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
>>> Date: Sun, 9 Sep 2012 19:56:31 +0200
>>> Subject: [PATCH] s5k4ecgx: Fix unaligned access issues
>>>
>>> Signed-off-by: Sylwester Nawrocki
>>> ---
>>>   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
>>>   #include
>>>   #include
>>> +#include
>>>
>>>   #include
>>>   #include
>>> @@ -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((__le

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

2012-09-06 Thread Sangwook Lee
This patch adds driver for S5K4ECGX sensor with embedded ISP SoC,
S5K4ECGX, which is a 5M CMOS Image sensor from Samsung
The driver implements preview mode of the S5K4ECGX sensor.
capture (snapshot) operation, face detection are missing now.
Following controls are supported:
contrast/saturation/brightness/sharpness

Signed-off-by: Sangwook Lee 
Reviewed-by: Sylwester Nawrocki 
---
Changes since v5:
- deleted dummy lines
- fixed pointer errors in handling firmware
- updated comments
- added le32_to_cpu,le16_to_cpu

Changes since v4:
- replaced register tables with the function from Sylwester
- updated firmware parsing function with CRC32 check
  firmware generator from user space:
  git://git.linaro.org/people/sangwook/fimc-v4l2-app.git

Changes since v3:
- used request_firmware to configure initial settings
- added parsing functions to read initial settings
- updated regulator API
- reduced preview setting tables by experiment

Changes since v2:
- added GPIO (reset/stby) and regulators
- updated I2C read/write, based on s5k6aa datasheet
- fixed set_fmt errors
- reduced register tables a bit
- removed vmalloc

Changes since v1:
- fixed s_stream(0) when it called twice
- changed mutex_X position to be used when strictly necessary
- add additional s_power(0) in case that error happens
- update more accurate debugging statements
- remove dummy else

 drivers/media/i2c/Kconfig|7 +
 drivers/media/i2c/Makefile   |1 +
 drivers/media/i2c/s5k4ecgx.c | 1011 ++
 include/media/s5k4ecgx.h |   37 ++
 4 files changed, 1056 insertions(+)
 create mode 100644 drivers/media/i2c/s5k4ecgx.c
 create mode 100644 include/media/s5k4ecgx.h

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 9a5a059..55b3bbb 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -484,6 +484,13 @@ config VIDEO_S5K6AA
  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
  camera sensor with an embedded SoC image signal processor.
 
+config VIDEO_S5K4ECGX
+tristate "Samsung S5K4ECGX sensor support"
+depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+---help---
+  This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
+  camera sensor with an embedded SoC image signal processor.
+
 source "drivers/media/i2c/smiapp/Kconfig"
 
 comment "Flash devices"
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 088a460..a720812 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
 obj-$(CONFIG_VIDEO_SR030PC30)  += sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)+= noon010pc30.o
 obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
+obj-$(CONFIG_VIDEO_S5K4ECGX)   += s5k4ecgx.o
 obj-$(CONFIG_VIDEO_ADP1653)+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)+= as3645a.o
 obj-$(CONFIG_VIDEO_SMIAPP_PLL) += smiapp-pll.o
diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
new file mode 100644
index 000..66107c8
--- /dev/null
+++ b/drivers/media/i2c/s5k4ecgx.c
@@ -0,0 +1,1011 @@
+/*
+ * Driver for s5k4ecgx (5MP Camera) from Samsung
+ * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
+ * CMOS image sensor.
+ *
+ * Copyright (C) 2012, Linaro, Sangwook Lee 
+ * Copyright (C) 2012, Insignal Co,. Ltd,  Homin Lee 
+ *
+ * Based on s5k6aa, noon010pc30 driver
+ * Copyright (C) 2011, Samsung Electronics Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define S5K4ECGX_DRIVER_NAME   "s5k4ecgx"
+#define S5K4ECGX_FIRMWARE  "s5k4ecgx.bin"
+
+/* Firmware revision information */
+#define REG_FW_REVISION0x71a6
+#define REG_FW_VERSION 0x71a4
+#define S5K4ECGX_REVISION_1_1  0x11
+#define S5K4ECGX_FW_VERSION0x4ec0
+
+/* General purpose parameters */
+#define REG_USER_BRIGHTNESS0x722c
+#define REG_USER_CONTRAST  0x722e
+#define REG_USER_SATURATION0x7230
+
+#define REG_G_NEW_CFG_SYNC 0x724a
+#define REG_G_PREV_IN_WIDTH0x7250
+#define REG_G_PREV_IN_HEIGHT   0x7252
+#define REG_G_PREV_IN_XOFFS0x7254
+#define REG_G_PREV_IN_YOFFS0x7256
+#define REG_G_CAP_IN_WIDTH 0x7258
+#define REG_G_CAP_IN_HEIGHT0x725a
+#define REG_G_CAP_IN_XOFFS

Re: [RFC PATCH v5] media: add v4l2 subdev driver for S5K4ECGX sensor

2012-09-06 Thread Sangwook Lee
Hi Sylwester

Thank you for the review again.

On 5 September 2012 22:56, Sylwester Nawrocki
 wrote:
> Hi Sangwook,
>
> On 09/05/2012 02:28 PM, Sangwook Lee wrote:
[snip]
>> +#include
>
> What do we need this header for ?

Ok, let me delete this.

>
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
> ...
>> +
>> +static int s5k4ecgx_set_ahb_address(struct v4l2_subdev *sd)
>> +{
>> + int ret;
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> + /* Set APB peripherals start address */
>> + ret = s5k4ecgx_i2c_write(client, AHB_MSB_ADDR_PTR, GEN_REG_OFFSH);
>> + if (ret)
>> + return ret;
>> + /*
>> +  * FIXME: This is copied from s5k6aa, because of no information
>> +  * in s5k4ecgx's datasheet.
>> +  * sw_reset is activated to put device into idle status
>> +  */
>> + ret = s5k4ecgx_i2c_write(client, 0x0010, 0x0001);
>> + if (ret)
>> + return ret;
>> +
>> + /* FIXME: no information available about this register */
>
> Let's drop that comment, we will fix all magic numbers once proper
> documentation is available.

Ok.

>
>> + ret = s5k4ecgx_i2c_write(client, 0x1030, 0x);
>> + if (ret)
>> + return ret;
>> + /* Halt ARM CPU */
>> + ret = s5k4ecgx_i2c_write(client, 0x0014, 0x0001);
>> +
>> + return ret;
>
> Just do
>
> return s5k4ecgx_i2c_write(client, 0x0014, 0x0001);

OK, I will fix this.

>> +}
>> +
>> +#define FW_HEAD 6
>> +/* Register address, value are 4, 2 bytes */
>> +#define FW_REG_SIZE 6
>
> FW_REG_SIZE is a bit confusing, maybe we could name this FW_RECORD_SIZE
> or something similar ?

Fair enough

>
>> +/*
>> + * Firmware has the following format:
>> + *,<  record 0>,
>> + *<  record N - 1>,<  CRC32-CCITT (4-bytes)>
>> + * where "record" is a 4-byte register address followed by 2-byte
>> + * register value (little endian)
>> + */
>> +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;
>> + u8 *fwbuf;
>> +
>> + err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
>> + if (err) {
>> + v4l2_err(sd, "Failed to read firmware %s\n", 
>> S5K4ECGX_FIRMWARE);
>> + goto fw_out1;
>
> return err;

OK, I will fix this.

>
> ?
>> + }
>> + fwbuf = kmemdup(fw->data, fw->size, GFP_KERNEL);
>
> Why do we need this kmemdup ? Couldn't we just use fw->data ?

OK,  Iet me reconsider this.

>
>> + if (!fwbuf) {
>> + err = -ENOMEM;
>> + goto fw_out2;
>> + }
>> + crc_file = *(u32 *)(fwbuf + regs_num * FW_REG_SIZE);
>
> regs_num is uninitialized ?
>
>> + crc = crc32_le(~0, fwbuf, regs_num * FW_REG_SIZE);
>> + if (crc != crc_file) {
>> + v4l2_err(sd, "FW: invalid crc (%#x:%#x)\n", crc, crc_file);
>> + err = -EINVAL;
>> + goto fw_out3;
>> + }
>> + regs_num = *(u32 *)(fwbuf);
>
> I guess this needs to be moved up. I would make it
>
> regs_num = le32_to_cpu(*(u32 *)fw->data);
>
> And perhaps we need a check like:
>
> if (fw->size < regs_num * FW_REG_SIZE)
> return -EINVAL;
> ?
>> + v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",
>> +  S5K4ECGX_FIRMWARE, fw->size, regs_num);
>> + regs_num++; /* Add header */
>> + for (i = 1; i<  regs_num; i++) {
>> + addr = *(u32 *)(fwbuf + i * FW_REG_SIZE);
>> + val = *(u16 *)(fwbuf + i * FW_REG_SIZE + 4);
>
> I think you need to access addr and val through le32_to_cpu() as well,
> even though your ARM system might be little-endian by default, this
> driver could possibly be used on machines with different endianness.
>
> Something like this could be more optimal:
>
> const u8 *ptr = fw->data + FW_REG_SIZE;
>
> for (i = 1; i < regs_num; i++) {
> addr = le32_to_cpu(*(u32 *)ptr);
> ptr += 4;
> val = le16_to_cpu(*(u16 *)ptr);
> ptr += FW_REG_SIZE;
>

Thanks for your adv

[RFC PATCH v5] media: add v4l2 subdev driver for S5K4ECGX sensor

2012-09-05 Thread Sangwook Lee
This patch adds driver for S5K4ECGX sensor with embedded ISP SoC,
S5K4ECGX, which is a 5M CMOS Image sensor from Samsung
The driver implements preview mode of the S5K4ECGX sensor.
capture (snapshot) operation, face detection are missing now.
Following controls are supported:
contrast/saturation/brightness/sharpness

Signed-off-by: Sangwook Lee 
Cc: Sylwester Nawrocki 
Cc: Scott Bambrough 
---
Changes since v4:
- replaced register tables with the function from Sylwester
- updated firmware parsing function with CRC32 check
  firmware generator from user space:
  git://git.linaro.org/people/sangwook/fimc-v4l2-app.git

Changes since v3:
- used request_firmware to configure initial settings
- added parsing functions to read initial settings
- updated regulator API
- reduced preview setting tables by experiment 

Changes since v2:
- added GPIO (reset/stby) and regulators
- updated I2C read/write, based on s5k6aa datasheet
- fixed set_fmt errors
- reduced register tables a bit
- removed vmalloc

Changes since v1:
- fixed s_stream(0) when it called twice
- changed mutex_X position to be used when strictly necessary
- add additional s_power(0) in case that error happens
- update more accurate debugging statements
- remove dummy else

 drivers/media/i2c/Kconfig|7 +
 drivers/media/i2c/Makefile   |1 +
 drivers/media/i2c/s5k4ecgx.c | 1019 ++
 include/media/s5k4ecgx.h |   37 ++
 4 files changed, 1064 insertions(+)
 create mode 100644 drivers/media/i2c/s5k4ecgx.c
 create mode 100644 include/media/s5k4ecgx.h

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 9a5a059..55b3bbb 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -484,6 +484,13 @@ config VIDEO_S5K6AA
  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
  camera sensor with an embedded SoC image signal processor.
 
+config VIDEO_S5K4ECGX
+tristate "Samsung S5K4ECGX sensor support"
+depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+---help---
+  This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
+  camera sensor with an embedded SoC image signal processor.
+
 source "drivers/media/i2c/smiapp/Kconfig"
 
 comment "Flash devices"
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 088a460..a720812 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
 obj-$(CONFIG_VIDEO_SR030PC30)  += sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)+= noon010pc30.o
 obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
+obj-$(CONFIG_VIDEO_S5K4ECGX)   += s5k4ecgx.o
 obj-$(CONFIG_VIDEO_ADP1653)+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)+= as3645a.o
 obj-$(CONFIG_VIDEO_SMIAPP_PLL) += smiapp-pll.o
diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
new file mode 100644
index 000..0f12d46
--- /dev/null
+++ b/drivers/media/i2c/s5k4ecgx.c
@@ -0,0 +1,1019 @@
+/*
+ * Driver for s5k4ecgx (5MP Camera) from Samsung
+ * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
+ * CMOS image sensor.
+ *
+ * Copyright (C) 2012, Linaro, Sangwook Lee 
+ * Copyright (C) 2012, Insignal Co,. Ltd,  Homin Lee 
+ *
+ * Based on s5k6aa, noon010pc30 driver
+ * Copyright (C) 2011, Samsung Electronics Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define S5K4ECGX_DRIVER_NAME   "s5k4ecgx"
+#define S5K4ECGX_FIRMWARE  "s5k4ecgx.bin"
+
+/* Firmware revision information */
+#define REG_FW_REVISION0x71a6
+#define REG_FW_VERSION 0x71a4
+#define S5K4ECGX_REVISION_1_1  0x11
+#define S5K4ECGX_FW_VERSION0x4ec0
+
+/* General purpose parameters */
+#define REG_USER_BRIGHTNESS0x722c
+#define REG_USER_CONTRAST  0x722e
+#define REG_USER_SATURATION0x7230
+
+#define REG_G_NEW_CFG_SYNC 0x724a
+#define REG_G_PREV_IN_WIDTH0x7250
+#define REG_G_PREV_IN_HEIGHT   0x7252
+#define REG_G_PREV_IN_XOFFS0x7254
+#define REG_G_PREV_IN_YOFFS0x7256
+#define REG_G_CAP_IN_WIDTH 0x7258
+#define REG_G_CAP_IN_HEIGHT0x725a
+#define REG_G_CAP_IN_XOFFS 0x725c
+#define REG_G_CAP_IN_YOFFS 0x725e
+#define REG_G_INPUTS_CHANGE_REQ   

Re: [PATH v3 0/2] Add v4l2 subdev driver for S5K4ECGX sensor with embedded SoC ISP

2012-08-20 Thread Sangwook Lee
Hi Sylwester

On 20 August 2012 09:12, Sangwook Lee  wrote:
> Hi Sylwester
>
> On 19 August 2012 22:29, Sylwester Nawrocki
>  wrote:
>> Hi Sangwook,
>>
>> On 08/03/2012 04:24 PM, Sangwook Lee wrote:
>>> I was thinking about this, but this seems to be is a bit time-consuming 
>>> because
>>> I have to do this just due to lack of s5k4ecgx hardware information.
>>> let me try it later once
>>> this patch is accepted.
>>
>> I've converted this driver to use function calls instead of the register
>> arrays. It can be pulled, along with a couple of minor fixes/improvements,
>> from following git tree:
>>
>> git://linuxtv.org/snawrocki/media.git s5k4ecgx
>> (gitweb: http://git.linuxtv.org/snawrocki/media.git/s5k4ecgx)
>>
>> I don't own any Origen board thus it's untested. Could you give it a try ?

Sorry, It doesn't work. I will send pictures to you by another mail thread.
Previously, I tested preview array and found out that
+   /*
+* FIXME: according to the datasheet,
+* 0x7496~ 0x749c seems to be only for capture mode,
+* but without these value, it doesn't work with preview mode.
+*/

Do we need to set those values ?


Thanks
Sangwook


>
> Wow! Great,  let me download from this git and then test.
>


Thanks



>
--
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 0/2] Add v4l2 subdev driver for S5K4ECGX sensor with embedded SoC ISP

2012-08-20 Thread Sangwook Lee
Hi Sylwester

On 19 August 2012 22:29, Sylwester Nawrocki
 wrote:
> Hi Sangwook,
>
> On 08/03/2012 04:24 PM, Sangwook Lee wrote:
>> I was thinking about this, but this seems to be is a bit time-consuming 
>> because
>> I have to do this just due to lack of s5k4ecgx hardware information.
>> let me try it later once
>> this patch is accepted.
>
> I've converted this driver to use function calls instead of the register
> arrays. It can be pulled, along with a couple of minor fixes/improvements,
> from following git tree:
>
> git://linuxtv.org/snawrocki/media.git s5k4ecgx
> (gitweb: http://git.linuxtv.org/snawrocki/media.git/s5k4ecgx)
>
> I don't own any Origen board thus it's untested. Could you give it a try ?

Wow! Great,  let me download from this git and then test.


> The register write sequence should be identical as in the case of using
> the arrays.
>
> You won't find much regarding the face detection features in V4L2,
> unfortunately. _Maybe_ I'll try to address this as well on some day, for
> now private controls might be your only solution. Unless you feel like
> adding face detection features support to V4L2.. ;)
>
> BTW, are your requirements to support both EVT1.0 and EVT1.1 S5K4ECGX
> revisions ?

I have only EVT 1.1. So I have no chance to run EVT 1.0 version.

Thanks
Sangwook

>
> --
>
> 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


[PATCH v4 2/2] v4l: Add v4l2 subdev driver for S5K4ECGX sensor

2012-08-10 Thread Sangwook Lee
This driver implements preview mode of the S5K4ECGX sensor.
capture (snapshot) operation, face detection are missing now.

Following controls are supported:
contrast/saturation/brightness/sharpness

Signed-off-by: Sangwook Lee 
---
 drivers/media/video/Kconfig|8 +
 drivers/media/video/Makefile   |1 +
 drivers/media/video/s5k4ecgx.c |  941 
 include/media/s5k4ecgx.h   |   37 ++
 4 files changed, 987 insertions(+)
 create mode 100644 drivers/media/video/s5k4ecgx.c
 create mode 100644 include/media/s5k4ecgx.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index c128fac..d405cb1 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -580,6 +580,14 @@ config VIDEO_S5K6AA
  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
  camera sensor with an embedded SoC image signal processor.
 
+config VIDEO_S5K4ECGX
+   tristate "Samsung S5K4ECGX sensor support"
+   depends on MEDIA_CAMERA_SUPPORT
+   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   ---help---
+ This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
+ camera sensor with an embedded SoC image signal processor.
+
 source "drivers/media/video/smiapp/Kconfig"
 
 comment "Flash devices"
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index b7da9fa..ec39c47 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)+= noon010pc30.o
 obj-$(CONFIG_VIDEO_M5MOLS) += m5mols/
 obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
+obj-$(CONFIG_VIDEO_S5K4ECGX)+= s5k4ecgx.o
 obj-$(CONFIG_VIDEO_SMIAPP) += smiapp/
 obj-$(CONFIG_VIDEO_ADP1653)+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)+= as3645a.o
diff --git a/drivers/media/video/s5k4ecgx.c b/drivers/media/video/s5k4ecgx.c
new file mode 100644
index 000..4e57738
--- /dev/null
+++ b/drivers/media/video/s5k4ecgx.c
@@ -0,0 +1,941 @@
+/*
+ * Driver for s5k4ecgx (5MP Camera) from Samsung
+ * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
+ * CMOS image sensor.
+ *
+ * Copyright (C) 2012, Linaro, Sangwook Lee 
+ * Copyright (C) 2012, Insignal Co,. Ltd,  Homin Lee 
+ *
+ * Based on s5k6aa, noon010pc30 driver
+ * Copyright (C) 2011, Samsung Electronics Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "s5k4ecgx_regs.h"
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define S5K4ECGX_DRIVER_NAME   "s5k4ecgx"
+
+#define S5K4ECGX_FIRMWARE  "s5k4ecgx.fw"
+/* Firmware parsing token */
+#define FW_START_TOKEN '{'
+#define FW_END_TOKEN   '}'
+
+/* Firmware revision information */
+#define REG_FW_REVISION0x71a6
+#define REG_FW_VERSION 0x71a4
+#define S5K4ECGX_REVISION_1_1  0x11
+#define S5K4ECGX_FW_VERSION0x4ec0
+
+/* General purpose parameters */
+#define REG_USER_BRIGHTNESS0x722c /* Brigthness */
+#define REG_USER_CONTRAST  0x722e /* Contrast */
+#define REG_USER_SATURATION0x7230 /* Saturation */
+
+#define REG_USER_SHARP10x7a28
+#define REG_USER_SHARP20x7ade
+#define REG_USER_SHARP30x7b94
+#define REG_USER_SHARP40x7c4a
+#define REG_USER_SHARP50x7d00
+
+/* Reduce sharpness range for user space API */
+#define SHARPNESS_DIV  8208
+#define TOK_TERM   0x
+
+/*
+ * FIMXE: This is copied from s5k6aa, because of no information
+ * in s5k4ecgx's datasheet
+ * H/W register Interface (0xd000 - 0xdfff)
+ */
+#define AHB_MSB_ADDR_PTR   0xfcfc
+#define GEN_REG_OFFSH  0xd000
+#define REG_CMDWR_ADDRH0x0028
+#define REG_CMDWR_ADDRL0x002a
+#define REG_CMDRD_ADDRH0x002c
+#define REG_CMDRD_ADDRL0x002e
+#define REG_CMDBUF0_ADDR   0x0f12
+
+/*
+ * Preview size lists supported by sensor
+ */
+static const struct regval_list *prev_regs[] = {
+   s5k4ecgx_176_prev,
+   s5k4ecgx_352_prev,
+   s5k4ecgx_640_prev,
+   s5k4ecgx_720_prev,
+};
+
+struct s5k4ecgx_frmsize {
+   u32 idx; /* Should ind

[PATCH v4 1/2] v4l: Add factory register values form S5K4ECGX sensor

2012-08-10 Thread Sangwook Lee
Add preview default settings for S5K4ECGX sensor registers,
which was copied from the reference code of Samsung S.LSI.

Signed-off-by: Sangwook Lee 
---
 drivers/media/video/s5k4ecgx_regs.h |  138 +++
 1 file changed, 138 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..e99b0e6
--- /dev/null
+++ b/drivers/media/video/s5k4ecgx_regs.h
@@ -0,0 +1,138 @@
+/*
+ * 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;
+};
+
+/* Configure 720x480 preview size */
+static const struct regval_list s5k4ecgx_720_prev[] = {
+   { 0x7250, 0x0a00 },
+   { 0x7252, 0x06a8 },
+   { 0x7254, 0x0010 },
+   { 0x7256, 0x0078 },
+   { 0x7258, 0x0a00 },
+   { 0x725a, 0x06a8 },
+   { 0x725c, 0x0010 },
+   { 0x725e, 0x0078 },
+   { 0x7494, 0x0a00 },
+
+   /*
+* FIXME: according to the datasheet,
+* 0x7496~ 0x749c seems to be only for capture mode,
+* but without these value, it doesn't work with preview mode.
+*/
+   { 0x7496, 0x06a8 },
+   { 0x7498, 0x },
+   { 0x749a, 0x },
+   { 0x749c, 0x0a00 },
+
+   { 0x749e, 0x06a8 },
+   { 0x72a6, 0x02d0 },
+   { 0x72a8, 0x01e0 },
+   { 0x, 0x }, /* End token */
+};
+
+/* Configure 640x480 preview size */
+static const struct regval_list s5k4ecgx_640_prev[] = {
+   { 0x7250, 0x0a00 },
+   { 0x7252, 0x0780 },
+   { 0x7254, 0x0010 },
+   { 0x7256, 0x000c },
+   { 0x7258, 0x0a00 },
+   { 0x725a, 0x0780 },
+   { 0x725c, 0x0010 },
+   { 0x725e, 0x000c },
+   { 0x7494, 0x0a00 },
+   { 0x7496, 0x0780 },
+   { 0x7498, 0x },
+   { 0x749a, 0x },
+   { 0x749c, 0x0a00 },
+   { 0x749e, 0x0780 },
+   { 0x72a6, 0x0280 },
+   { 0x72a8, 0x01e0 },
+   { 0x, 0x }, /* End token */
+};
+
+/* Configure 352x288 preview size */
+static const struct regval_list s5k4ecgx_352_prev[] = {
+   { 0x7250, 0x0928 },
+   { 0x7252, 0x0780 },
+   { 0x7254, 0x007c },
+   { 0x7256, 0x000c },
+   { 0x7258, 0x0928 },
+   { 0x725a, 0x0780 },
+   { 0x725c, 0x007c },
+   { 0x725e, 0x000c },
+   { 0x7494, 0x0928 },
+   { 0x7496, 0x0780 },
+   { 0x7498, 0x },
+   { 0x749a, 0x },
+   { 0x749c, 0x0928 },
+   { 0x749e, 0x0780 },
+   { 0x72a6, 0x0160 },
+   { 0x72a8, 0x0120 },
+   { 0x, 0x }, /* End token */
+};
+
+/* Configure 176x144 preview size */
+static const struct regval_list s5k4ecgx_176_prev[] = {
+   { 0x7250, 0x0928 },
+   { 0x7252, 0x0780 },
+   { 0x7254, 0x007c },
+   { 0x7256, 0x000c },
+   { 0x7258, 0x0928 },
+   { 0x725a, 0x0780 },
+   { 0x725c, 0x007c },
+   { 0x725e, 0x000c },
+   { 0x7494, 0x0928 },
+   { 0x7496, 0x0780 },
+   { 0x7498, 0x },
+   { 0x749a, 0x },
+   { 0x749c, 0x0928 },
+   { 0x749e, 0x0780 },
+   { 0x72a6, 0x00b0 },
+   { 0x72a8, 0x0090 },
+   { 0x, 0x }, /* End token */
+};
+
+/* Common setting 1 for preview */
+static const struct regval_list s5k4ecgx_com1_prev[] = {
+   { 0x74a0, 0x },
+   { 0x74a2, 0x },
+   { 0x7262, 0x0001 },
+   { 0x7a1e, 0x0028 },
+   { 0x7ad4, 0x003c },
+   { 0x, 0x }, /* End token */
+};
+
+/* Common setting 2 for preview */
+static const struct regval_list s5k4ecgx_com2_prev[] = {
+   { 0x72aa, 0x0005 },
+   { 0x72b4, 0x0052 },
+   { 0x72be, 0x },
+   { 0x72c0, 0x0001 },
+   { 0x72c2, 0x029a },
+   { 0x72c4, 0x014d },
+   { 0x72d0, 0x },
+   { 0x72d2, 0x },
+   { 0x7266, 0x },
+   { 0x726a, 0x0001 },
+   { 0x724e, 0x0001 },
+   { 0x7268, 0x0001 },
+   { 0x, 0x }, /* End token */
+};
+
+#endif /* __DRIVERS_MEDIA_VIDEO_S5K4ECGX_H__ */
-- 
1.7.9.5

--
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


[PATCH v4 0/2] Add v4l2 subdev driver for S5K4ECGX sensor with embedded SoC ISP

2012-08-10 Thread Sangwook Lee
The following 2 patches add driver for S5K4ECGX sensor with embedded ISP SoC,
and minor v4l2 control API enhancement. S5K4ECGX is 5M CMOS Image sensor from 
Samsung

Changes since v3:
- used request_firmware to configure initial settings
- added parsing functions to read initial settings
- updated regulator API
- reduced preview setting tables by experiment 

Changes since v2:
- added GPIO (reset/stby) and regulators
- updated I2C read/write, based on s5k6aa datasheet
- fixed set_fmt errors
- reduced register tables a bit
- removed vmalloc

Changes since v1:
- fixed s_stream(0) when it called twice
- changed mutex_X position to be used when strictly necessary
- add additional s_power(0) in case that error happens
- update more accurate debugging statements
- remove dummy else

Sangwook Lee (2):
  v4l: Add factory register values form S5K4ECGX sensor
  v4l: Add v4l2 subdev driver for S5K4ECGX sensor

 drivers/media/video/Kconfig |8 +
 drivers/media/video/Makefile|1 +
 drivers/media/video/s5k4ecgx.c  |  941 +++
 drivers/media/video/s5k4ecgx_regs.h |  138 +
 include/media/s5k4ecgx.h|   37 ++
 5 files changed, 1125 insertions(+)
 create mode 100644 drivers/media/video/s5k4ecgx.c
 create mode 100644 drivers/media/video/s5k4ecgx_regs.h
 create mode 100644 include/media/s5k4ecgx.h

-- 
1.7.9.5

--
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

2012-08-03 Thread Sangwook Lee
Hi Sylwester

On 2 August 2012 21:50, Sylwester Nawrocki  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 Lee
>> ---
>>   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[] = {
>
> 
>
>> +/* 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 0/2] Add v4l2 subdev driver for S5K4ECGX sensor with embedded SoC ISP

2012-08-03 Thread Sangwook Lee
Hi  Sylwester

Thank you for the review.

On 2 August 2012 21:11, Sylwester Nawrocki  wrote:
>
> Hi Sangwook,
>
> On 08/02/2012 03:42 PM, Sangwook Lee wrote:
> > The following 2 patches add driver for S5K4ECGX sensor with embedded ISP 
> > SoC,
> > and minor v4l2 control API enhancement. S5K4ECGX is 5M CMOS Image sensor 
> > from Samsung
> >
> > Changes since v2:
> > - added GPIO (reset/stby) and regulators
> > - updated I2C read/write, based on s5k6aa datasheet
> > - fixed set_fmt errors
> > - reduced register tables a bit
> > - removed vmalloc
>
> It looks like a great improvement, well done! Thanks!
>
> In the S5K4CAGX sensor datasheet, that can be found on the internet,
> there is 0x...0x002E registers description, which look very much
> same as in S5K6AAFX case and likely is also valid for S5K4CAGX.


[snip]

>
>
> What do you think about converting s5k4ecgx_img_regs arrays (it has
> over 2700 entries) to a firmware file and adding some simple parser
> to the driver ? Then we would have the problem solved in most part.
>

Thanks, fair enough. let me try this.


>
> Regarding various preview resolution set up, the difference in all
> those s5k4ecgx_*_preview[] arrays is rather small, only register
> values differ, e.g. for 640x480 and 720x480 there is only 8 different
> entries:
>

Ok, let me reduce table size again.

>
> $ diff -a s5k4ec_640.txt s5k4ec_720.txt
> 1c1
> < static const struct regval_list s5k4ecgx_640_preview[] = {
> ---
> > static const struct regval_list s5k4ecgx_720_preview[] = {
> 3c3
> <   { 0x7252, 0x0780 },
> ---
> >   { 0x7252, 0x06a8 },
> 5c5



[snip]

>
> <   { 0x7256, 0x000c },
> >   { 0x72a6, 0x02d

[snip]
>
> Could you please try to implement a function that replaces those tables,
> based s5k6aa_set_prev_config() and s5k6aa_set_output_framefmt() ?
>
I was thinking about this, but this seems to be is a bit time-consuming because
I have to do this just due to lack of s5k4ecgx hardware information.
let me try it later once
this patch is accepted.

Thanks
Sangwook

> 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


[PATH v3 2/2] v4l: Add v4l2 subdev driver for S5K4ECGX sensor

2012-08-02 Thread Sangwook Lee
This driver implements preview mode of the S5K4ECGX sensor.
capture (snapshot) operation, face detection are missing now.

Following controls are supported:
contrast/saturation/brightness/sharpness

Signed-off-by: Sangwook Lee 
---
 drivers/media/video/Kconfig|8 +
 drivers/media/video/Makefile   |1 +
 drivers/media/video/s5k4ecgx.c |  839 
 include/media/s5k4ecgx.h   |   39 ++
 4 files changed, 887 insertions(+)
 create mode 100644 drivers/media/video/s5k4ecgx.c
 create mode 100644 include/media/s5k4ecgx.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index c128fac..2c3f434 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -580,6 +580,14 @@ config VIDEO_S5K6AA
  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
  camera sensor with an embedded SoC image signal processor.
 
+config VIDEO_S5K4ECGX
+tristate "Samsung S5K4ECGX sensor support"
+   depends on MEDIA_CAMERA_SUPPORT
+depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+---help---
+  This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
+  camera sensor with an embedded SoC image signal processor.
+
 source "drivers/media/video/smiapp/Kconfig"
 
 comment "Flash devices"
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index b7da9fa..ec39c47 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)+= noon010pc30.o
 obj-$(CONFIG_VIDEO_M5MOLS) += m5mols/
 obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
+obj-$(CONFIG_VIDEO_S5K4ECGX)+= s5k4ecgx.o
 obj-$(CONFIG_VIDEO_SMIAPP) += smiapp/
 obj-$(CONFIG_VIDEO_ADP1653)+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)+= as3645a.o
diff --git a/drivers/media/video/s5k4ecgx.c b/drivers/media/video/s5k4ecgx.c
new file mode 100644
index 000..cfc90b8
--- /dev/null
+++ b/drivers/media/video/s5k4ecgx.c
@@ -0,0 +1,839 @@
+/*
+ * Driver for s5k4ecgx (5MP Camera) from Samsung
+ * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
+ * CMOS image sensor.
+ *
+ * Copyright (C) 2012, Linaro, Sangwook Lee 
+ * Copyright (C) 2012, Insignal Co,. Ltd,  Homin Lee 
+ *
+ * Based on s5k6aa, noon010pc30 driver
+ * Copyright (C) 2011, Samsung Electronics Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "s5k4ecgx_regs.h"
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define S5K4ECGX_DRIVER_NAME   "s5k4ecgx"
+
+/* Firmware revision information */
+#define REG_FW_REVISION0x71a6
+#define REG_FW_VERSION 0x71a4
+#define S5K4ECGX_REVISION_1_1  0x11
+#define S5K4ECGX_FW_VERSION0x4ec0
+
+/* General purpose parameters */
+#define REG_USER_BRIGHTNESS0x722c /* Brigthness */
+#define REG_USER_CONTRAST  0x722e /* Contrast */
+#define REG_USER_SATURATION0x7230 /* Saturation */
+
+#define REG_USER_SHARP10x7a28
+#define REG_USER_SHARP20x7ade
+#define REG_USER_SHARP30x7b94
+#define REG_USER_SHARP40x7c4a
+#define REG_USER_SHARP50x7d00
+
+/* Reduce sharpness range for user space API */
+#define SHARPNESS_DIV  8208
+
+#define TOK_TERM   0x
+
+/*
+ * FIMXE: This is copied from s5k6aa, because of no information
+ * in s5k4ecgx's datasheet
+ * H/W register Interface (0xd000 - 0xdfff)
+ */
+#define AHB_MSB_ADDR_PTR   0xfcfc
+#define GEN_REG_OFFSH  0xd000
+#define REG_CMDWR_ADDRH0x0028
+#define REG_CMDWR_ADDRL0x002a
+#define REG_CMDRD_ADDRH0x002c
+#define REG_CMDRD_ADDRL0x002e
+#define REG_CMDBUF0_ADDR   0x0f12
+
+/*
+ * Preview size lists supported by sensor
+ */
+static const struct regval_list *prev_regs[] = {
+   s5k4ecgx_176_preview,
+   s5k4ecgx_352_preview,
+   s5k4ecgx_640_preview,
+   s5k4ecgx_720_preview,
+};
+
+struct s5k4ecgx_frmsize {
+   u32 idx; /* Should indicate index of prev_regs */
+   u32 width;
+   u32 height;
+};
+
+/*
+ * TODO: currently only preview is supported and snapshopt(capture)
+ * is not implemented yet
+ */
+static const struct s5k4ecgx_f

[PATH v3 0/2] Add v4l2 subdev driver for S5K4ECGX sensor with embedded SoC ISP

2012-08-02 Thread Sangwook Lee
The following 2 patches add driver for S5K4ECGX sensor with embedded ISP SoC,
and minor v4l2 control API enhancement. S5K4ECGX is 5M CMOS Image sensor from 
Samsung

Changes since v2:
- added GPIO (reset/stby) and regulators
- updated I2C read/write, based on s5k6aa datasheet
- fixed set_fmt errors
- reduced register tables a bit
- removed vmalloc

Changes since v1:
- fixed s_stream(0) when it called twice
- changed mutex_X position to be used when strictly necessary
- add additional s_power(0) in case that error happens
- update more accurate debugging statements
- remove dummy else 

Sangwook Lee (2):
  v4l: Add factory register values form S5K4ECGX sensor
  v4l: Add v4l2 subdev driver for S5K4ECGX sensor

 drivers/media/video/Kconfig |8 +
 drivers/media/video/Makefile|1 +
 drivers/media/video/s5k4ecgx.c  |  839 ++
 drivers/media/video/s5k4ecgx_regs.h | 3105 +++
 include/media/s5k4ecgx.h|   39 +
 5 files changed, 3992 insertions(+)
 create mode 100644 drivers/media/video/s5k4ecgx.c
 create mode 100644 drivers/media/video/s5k4ecgx_regs.h
 create mode 100644 include/media/s5k4ecgx.h

-- 
1.7.9.5

--
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: [PATCH v2 2/2] v4l: Add v4l2 subdev driver for S5K4ECGX sensor

2012-07-20 Thread Sangwook Lee
Hi Sylwester

Thank you for the great review!

On 19 July 2012 22:40, Sylwester Nawrocki  wrote:
>
> Hi Sangwook,
>
> A few review comments for you below...
>
> On 07/19/2012 02:14 PM, Sangwook Lee wrote:
> > This dirver implements preview mode of the S5K4ECGX sensor.
>
> dirver -> driver

OK, I will fix this.

> > capture (snapshot) operation, face detection are missing now.
> >
> > Following controls are supported:
> > contrast/saturation/birghtness/sharpness
>
> birghtness -> brightness
>

ditto

> > Signed-off-by: Sangwook Lee
> > ---
> > + * Driver for s5k4ecgx (5MP Camera) from SAMSUNG
> > + * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
> > + * CMOS image sensor, as reffering to s5k6aa.c
>
> I think this should be mentioned after your own copyright notice,
> e.g. in form of:
>
> Based on s5k6aa driver,
> Copyright (C) 2011, Samsung Electronics Co., Ltd.

ditto

> > + *
> > + * Copyright (C) 2012, Linaro, Sangwook Lee
> > + * Copyright (C) 2012, Insignal Co,. Ltd,  Homin
> > Lee
> > + * Copyright (C) 2011, SAMSUNG ELECTRONICS
>
> No need to shout, "Samsung Electronics Co., Ltd." would be just enough.

ditto

>

> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> Can we, please, have these sorted alphabetically ?

OK, I will fix this.

>
> > +#include "s5k4ecgx_regs.h"
> > +
> > +static int debug;
> > +module_param(debug, int, 0644);
> > +
[snip]
> > +/* General purpose parameters */
> > +#define REG_USER_BRIGHTNESS  0x722C /* Brigthness */
>
> availble -> available

ditto

>
> > +#define REG_USER_SHARP1  0x7A28
> > +#define REG_USER_SHARP2  0x7ADE
> > +#define REG_USER_SHARP3  0x7B94
> > +#define REG_USER_SHARP4  0x7C4A
> > +#define REG_USER_SHARP5  0x7D00
> > +
> > +#define LSB(X) (((X)&  0xFF))
> > +#define MSB(Y) (((Y)>>  8)&  0xFF)
>
> Lower case for hex numbers is preferred.
>

ditto

> > +
> > +/*
> > + * Preview size lists supported by sensor
> > + */
> > +struct regval_list *pview_size[] = {
> > + s5k4ecgx_176_preview,
> > + s5k4ecgx_352_preview,
> > + s5k4ecgx_640_preview,
> > + s5k4ecgx_720_preview,
> > +};
> > +
> > +struct s5k4ecgx_framesize {
> > + u32 idx; /* Should indicate index of pview_size */
> > + u32 width;
> > + u32 height;
> > +};
> > +
> > +/*
> > + * TODO: currently only preview is supported and snapshopt(capture)
> > + * is not implemented yet
> > + */
> > +static struct s5k4ecgx_framesize p_sets[] = {
>
> p_sets -> s5k4ecgx_framesizes ?

Hmm, the structure name needs to be changed properly.

>
> > + {0, 176, 144},
> > + {1, 352, 288},
> > + {2, 640, 480},
> > + {3, 720, 480},
>
> I believe we can do without presets for just the preview operation mode.

This (p_sets[]) is only used to configure preview size dynamically
when _s_fmt is called and then s_stream become on.

> Then, the number of registers to configure when device is powered on
> should then decrease significantly.
> Those presets are meant to speed up switching the device context, e.g.
> from preview to capture, but they just slow down initialization, because
> you have to configure all presets beforehand.
>
> > +};
> > +
> > +#define S5K4ECGX_NUM_PREV ARRAY_SIZE(p_sets)
> > +struct s5k4ecgx_pixfmt {
> > + enum v4l2_mbus_pixelcode code;
> > + u32 colorspace;
> > +};
> > +
> > +/* By defaut value, output from sensor will be YUV422 0-255 */
> > +static const struct s5k4ecgx_pixfmt s5k4ecgx_formats[] = {
> > + { V4L2_MBUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_JPEG},
>
> Nit: missing whitespace before }.

Thanks, I will fix this.

>
> > +};
> > +
> > +struct s5k4ecgx_preset {
> > + /* output pixel format and resolution */
> > + struct v4l2_mbus_framefmt mbus_fmt;
> > + u8 clk_id;
> > + u8 index;
> > +};
> > +
> > +struct s5k4ecgx {
> > + struct v4l2_subdev sd;
> > + struct media_pad pad;
> > + struct v4l2_ctrl_handler handler;
> > +
> > + struct s5k4ecgx_platform_data *pdata;
> > + struct s5k4ecgx_preset presets[S5K4ECGX_MAX_PRESETS];
> > + struct s5k4ecgx_preset *preset;
> > + struct s5k4ecgx_frames

Re: [PATCH v2 1/2] v4l: Add factory register values form S5K4ECGX sensor

2012-07-20 Thread Sangwook Lee
Opps, the previous email has a HTML part, so resending.




Hi Sylwester

Thank for the review.

On 19 July 2012 20:40, Sylwester Nawrocki  wrote:
>
> Hi Sangwook,
>
> On 07/19/2012 02:14 PM, Sangwook Lee wrote:
> > Add factory default settings for S5K4ECGX sensor registers.
> > I copied them from the reference code of Samsung S.LSI.
>
> I'm pretty sure we can do better than that. I've started S5K6AAFX sensor
> driver development with similar set of write-only register address/value
> arrays, that stored mainly register default values after the device reset,
> or were configuring presets that were never used.
>
> If you lok at the s5k6aa driver, you'll find only one relatively small
> array of register values for the analog processing block settings.
> It's true that I had to reverse engineer a couple of things, but I also
>
> had a relatively good datasheet for the sensor.
>

Yes, I already saw analog settings in s5k6aa. Compared to s5k6aa,
I couldn't also understand why the sensor has lots of initial values.
Is it because s5k4ecgx is slightly more complicated than s5k6aa ?

> > According to comments from the reference code, they do not
> > recommend any changes of these settings.
>
> Yes, but it doesn't mean cannot convert, at least part of, those ugly
> tables into function calls.


Yes, the biggest table seems to be one time for boot-up, at least I need to
remove one more macro (token)

>
> Have you tried to contact Samsung S.LSI for a datasheet that would
> contain better registers' description ?


As you might know, there is a limitation for me to get those information. :-)
Instead, if I look into the source code of Google Nexus S which uses s5k4ecgx,

  https://android.googlesource.com/kernel/samsung.git

I can discover that both Google and Samsung are using the same huge table
just for initial settings from the sensor booting-up. I added the
original author
of this sensor driver. Hopes he might add some comments :-)



Thanks
Sangwook

>
> --
>
> Thanks,
> 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


[PATCH v2 2/2] v4l: Add v4l2 subdev driver for S5K4ECGX sensor

2012-07-19 Thread Sangwook Lee
This dirver implements preview mode of the S5K4ECGX sensor.
capture (snapshot) operation, face detection are missing now.

Following controls are supported:
contrast/saturation/birghtness/sharpness

Signed-off-by: Sangwook Lee 
---
 drivers/media/video/Kconfig|7 +
 drivers/media/video/Makefile   |1 +
 drivers/media/video/s5k4ecgx.c |  881 
 include/media/s5k4ecgx.h   |   29 ++
 4 files changed, 918 insertions(+)
 create mode 100644 drivers/media/video/s5k4ecgx.c
 create mode 100644 include/media/s5k4ecgx.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 99937c9..45d7f99 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -559,6 +559,13 @@ config VIDEO_S5K6AA
  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
  camera sensor with an embedded SoC image signal processor.
 
+config VIDEO_S5K4ECGX
+tristate "Samsung S5K4ECGX sensor support"
+depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+---help---
+  This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
+  camera sensor with an embedded SoC image signal processor.
+
 source "drivers/media/video/smiapp/Kconfig"
 
 comment "Flash devices"
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index d209de0..605bf35 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)+= noon010pc30.o
 obj-$(CONFIG_VIDEO_M5MOLS) += m5mols/
 obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
+obj-$(CONFIG_VIDEO_S5K4ECGX)+= s5k4ecgx.o
 obj-$(CONFIG_VIDEO_SMIAPP) += smiapp/
 obj-$(CONFIG_VIDEO_ADP1653)+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)+= as3645a.o
diff --git a/drivers/media/video/s5k4ecgx.c b/drivers/media/video/s5k4ecgx.c
new file mode 100644
index 000..e277d71
--- /dev/null
+++ b/drivers/media/video/s5k4ecgx.c
@@ -0,0 +1,881 @@
+/*
+ * Driver for s5k4ecgx (5MP Camera) from SAMSUNG
+ * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
+ * CMOS image sensor, as reffering to s5k6aa.c
+ *
+ * Copyright (C) 2012, Linaro, Sangwook Lee 
+ * Copyright (C) 2012, Insignal Co,. Ltd,  Homin Lee 
+ * Copyright (C) 2011, SAMSUNG ELECTRONICS
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "s5k4ecgx_regs.h"
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define S5K4ECGX_DRIVER_NAME   "s5k4ecgx"
+
+/* Basic windows sizes */
+#define S5K4ECGX_OUT_WIDTH_DEF 640
+#define S5K4ECGX_OUT_HEIGHT_DEF480
+#define S5K4ECGX_WIN_WIDTH_MAX 1024
+#define S5K4ECGX_WIN_HEIGHT_MAX600
+#define S5K4ECGX_WIN_WIDTH_MIN 176
+#define S5K4ECGX_WIN_HEIGHT_MIN144
+
+/* Firmware revision information */
+#define S5K4ECGX_REVISION_1_1  0x11
+#define S5K4ECGX_FW_VERSION0x4EC0
+#define REG_FW_VERSION 0x71A4
+#define REG_FW_REVISION0x71A6
+
+/* For now we use only one user configuration register set */
+#define S5K4ECGX_MAX_PRESETS   1
+
+/* Review this depending on system */
+#define S5K4ECGX_POLL_TIME 1 /* ms */
+
+/* General purpose parameters */
+#define REG_USER_BRIGHTNESS0x722C /* Brigthness */
+#define REG_USER_CONTRAST  0x722E /* Contrast */
+#define REG_USER_SATURATION0x7230 /* Saturation */
+
+/* FIXME: No information availble about these register from the datasheet */
+#define REG_USER_SHARP10x7A28
+#define REG_USER_SHARP20x7ADE
+#define REG_USER_SHARP30x7B94
+#define REG_USER_SHARP40x7C4A
+#define REG_USER_SHARP50x7D00
+
+#define LSB(X) (((X) & 0xFF))
+#define MSB(Y) (((Y) >> 8) & 0xFF)
+
+/*
+ * Preview size lists supported by sensor
+ */
+struct regval_list *pview_size[] = {
+   s5k4ecgx_176_preview,
+   s5k4ecgx_352_preview,
+   s5k4ecgx_640_preview,
+   s5k4ecgx_720_preview,
+};
+
+struct s5k4ecgx_framesize {
+   u32 idx; /* Should indicate index of pview_size */
+   u32 width;
+   u32 height;
+};
+
+/*
+ * TODO: currently only preview is supported and snapshopt(capture)
+ * is not implemented yet
+ */
+static struct s5k4ecgx_framesize p_sets[] = {
+   {0, 176, 144},
+   {1, 352, 288},
+   {2, 640, 480},
+   {3, 720, 4

[PATCH v2 0/2] Add v4l2 subdev driver for S5K4ECGX sensor with embedded SoC ISP

2012-07-19 Thread Sangwook Lee
The following 2 patches add driver for S5K4ECGX sensor with embedded ISP SoC,
and minor v4l2 control API enhancement. S5K4ECGX is 5M CMOS Image sensor from 
Samsung.

Changes since v1:
- fixed s_stream(0) when it called twice
- changed mutex_X position to be used when strictly necessary
- add additional s_power(0) in case that error happens
- update more accurate debugging statements
- remove dummy else 

Sangwook Lee (2):
  v4l: Add factory register values form S5K4ECGX sensor
  v4l: Add v4l2 subdev driver for S5K4ECGX sensor

 drivers/media/video/Kconfig |7 +
 drivers/media/video/Makefile|1 +
 drivers/media/video/s5k4ecgx.c  |  881 ++
 drivers/media/video/s5k4ecgx_regs.h | 3121 +++
 include/media/s5k4ecgx.h|   29 +
 5 files changed, 4039 insertions(+)
 create mode 100644 drivers/media/video/s5k4ecgx.c
 create mode 100644 drivers/media/video/s5k4ecgx_regs.h
 create mode 100644 include/media/s5k4ecgx.h

-- 
1.7.9.5

--
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


[PATCH 2/2] v4l: Add v4l2 subdev driver for S5K4ECGX sensor

2012-07-17 Thread Sangwook Lee
This dirver implements preview mode of the S5K4ECGX sensor.
capture (snapshot) operation, face detection are missing now.

Following controls are supported:
contrast/saturation/birghtness/sharpness

Signed-off-by: Sangwook Lee 
---
 drivers/media/video/Kconfig|7 +
 drivers/media/video/Makefile   |1 +
 drivers/media/video/s5k4ecgx.c |  871 
 include/media/s5k4ecgx.h   |   29 ++
 4 files changed, 908 insertions(+)
 create mode 100644 drivers/media/video/s5k4ecgx.c
 create mode 100644 include/media/s5k4ecgx.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 99937c9..45d7f99 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -559,6 +559,13 @@ config VIDEO_S5K6AA
  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
  camera sensor with an embedded SoC image signal processor.
 
+config VIDEO_S5K4ECGX
+tristate "Samsung S5K4ECGX sensor support"
+depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+---help---
+  This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
+  camera sensor with an embedded SoC image signal processor.
+
 source "drivers/media/video/smiapp/Kconfig"
 
 comment "Flash devices"
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index d209de0..605bf35 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)+= noon010pc30.o
 obj-$(CONFIG_VIDEO_M5MOLS) += m5mols/
 obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o
+obj-$(CONFIG_VIDEO_S5K4ECGX)+= s5k4ecgx.o
 obj-$(CONFIG_VIDEO_SMIAPP) += smiapp/
 obj-$(CONFIG_VIDEO_ADP1653)+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)+= as3645a.o
diff --git a/drivers/media/video/s5k4ecgx.c b/drivers/media/video/s5k4ecgx.c
new file mode 100644
index 000..68a1977
--- /dev/null
+++ b/drivers/media/video/s5k4ecgx.c
@@ -0,0 +1,871 @@
+/*
+ * Driver for s5k4ecgx (5MP Camera) from SAMSUNG
+ * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
+ * CMOS image sensor, as reffering to s5k6aa.c
+ *
+ * Copyright (C) 2012, Linaro, Sangwook Lee 
+ * Copyright (C) 2012, Insignal Co,. Ltd,  Homin Lee 
+ * Copyright (C) 2011, SAMSUNG ELECTRONICS
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "s5k4ecgx_regs.h"
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define S5K4ECGX_DRIVER_NAME   "s5k4ecgx"
+
+/* Basic windows sizes */
+#define S5K4ECGX_OUT_WIDTH_DEF 640
+#define S5K4ECGX_OUT_HEIGHT_DEF480
+#define S5K4ECGX_WIN_WIDTH_MAX 1024
+#define S5K4ECGX_WIN_HEIGHT_MAX600
+#define S5K4ECGX_WIN_WIDTH_MIN 176
+#define S5K4ECGX_WIN_HEIGHT_MIN144
+
+/* Firmware revision information */
+#define S5K4ECGX_REVISION_1_1  0x11
+#define S5K4ECGX_FW_VERSION0x4EC0
+#define REG_FW_VERSION 0x71A4
+#define REG_FW_REVISION0x71A6
+
+/* For now we use only one user configuration register set */
+#define S5K4ECGX_MAX_PRESETS   1
+
+/* Review this depending on system */
+#define S5K4ECGX_POLL_TIME 1 /* ms */
+
+/* General purpose parameters */
+#define REG_USER_BRIGHTNESS0x722C /* Brigthness */
+#define REG_USER_CONTRAST  0x722E /* Contrast */
+#define REG_USER_SATURATION0x7230 /* Saturation */
+
+/* FIXME: No information availble about these register from the datasheet */
+#define REG_USER_SHARP10x7A28
+#define REG_USER_SHARP20x7ADE
+#define REG_USER_SHARP30x7B94
+#define REG_USER_SHARP40x7C4A
+#define REG_USER_SHARP50x7D00
+
+#define LSB(X) (((X) & 0xFF))
+#define MSB(Y) (((Y) >> 8) & 0xFF)
+
+/*
+ * Preview size lists supported by sensor
+ */
+struct regval_list *pview_size[] = {
+   s5k4ecgx_176_preview,
+   s5k4ecgx_352_preview,
+   s5k4ecgx_640_preview,
+   s5k4ecgx_720_preview,
+};
+
+struct s5k4ecgx_framesize {
+   u32 idx; /* Should indicate index of pview_size */
+   u32 width;
+   u32 height;
+};
+
+/*
+ * TODO: currently only preview is supported and snapshopt(capture)
+ * is not implemented yet
+ */
+static struct s5k4ecgx_framesize p_sets[] = {
+   {0, 176, 144},
+   {1, 352, 288},
+   {2, 640, 480},
+   {3, 720, 4

[PATCH 0/2] Add v4l2 subdev driver for S5K4ECGX sensor with embedded SoC ISP

2012-07-17 Thread Sangwook Lee
The following 2 patches add driver for S5K4ECGX sensor with embedded ISP SoC,
and minor v4l2 control API enhancement. S5K4ECGX is 5M CMOS Image sensor from 
Samsung.

Currenlty ony preview mode is supported. (no capture mode/face detection)

Sangwook Lee (2):
  v4l: Add factory register values form S5K4ECGX sensor
  v4l: Add v4l2 subdev driver for S5K4ECGX sensor

 drivers/media/video/Kconfig |7 +
 drivers/media/video/Makefile|1 +
 drivers/media/video/s5k4ecgx.c  |  871 ++
 drivers/media/video/s5k4ecgx_regs.h | 3121 +++
 include/media/s5k4ecgx.h|   29 +
 5 files changed, 4029 insertions(+)
 create mode 100644 drivers/media/video/s5k4ecgx.c
 create mode 100644 drivers/media/video/s5k4ecgx_regs.h
 create mode 100644 include/media/s5k4ecgx.h

-- 
1.7.9.5

--
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