Re: [RFC PATCH v5] media: add v4l2 subdev driver for S5K4ECGX sensor
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 advice. I will take le32(16)_to_cpu.
>> + if (addr - addr_inc != 2)
>> + err = s5k4ecgx_write(client, addr, val);
>> + else
>> + err = s5k4ecgx_i2c_write(client, REG_CMDBUF0_ADDR,
>> val);
>> + if (err)
>> + goto fw_out3;
>
> nit: break instead of goto ?
Ok, I will fix this.
>
>> + addr_inc = addr;
>> + }
>> +fw_out3:
>> + kfree(fwbuf);
>> +fw_out2:
>> + release_firmware(fw);
>> +fw_out1:
>> +
>> + return err;
>> +}
> ...
>> +static int s5k4ecgx_init_sensor(struct v4l2_subdev *sd)
>> +{
>> + int ret;
>> +
>> + ret = s5k4ecgx_set_ahb_address(sd);
>> + /* The delay is from manufacturer's settings */
>> + msleep(100);
Re: [RFC PATCH v5] media: add v4l2 subdev driver for S5K4ECGX sensor
Hi Sangwook,
On 09/05/2012 02:28 PM, Sangwook Lee wrote:
> 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
Overall it looks good, thank you for patiently addressing all my
comments ;) There might be some rough edges here and there, but it's
all easy to fix. Please see below.
Reviewed-by: Sylwester Nawrocki
> ---
> 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
What do we need this header for ?
> +
> +#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
