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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sylwester

Thank you for the review again.

On 5 September 2012 22:56, Sylwester Nawrocki
<sylvester.nawrocki@xxxxxxxxx> wrote:
> Hi Sangwook,
>
> On 09/05/2012 02:28 PM, Sangwook Lee wrote:
[snip]
>> +#include<linux/vmalloc.h>
>
> What do we need this header for ?

Ok, let me delete this.

>
>> +
>> +#include<media/media-entity.h>
>> +#include<media/s5k4ecgx.h>
>> +#include<media/v4l2-ctrls.h>
>> +#include<media/v4l2-device.h>
>> +#include<media/v4l2-mediabus.h>
>> +#include<media/v4l2-subdev.h>
> ...
>> +
>> +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, 0x0000);
>> +     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:
>> + *<total number of records (4-bytes + 2-bytes padding) N>,<  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);
>> +
>> +     ret |= s5k4ecgx_load_firmware(sd);
>
>         if (!ret)
>                 ret = s5k4ecgx_load_firmware(sd);
>         else
> ?

Ok, I will fix this.

>> +
>> +     if (ret)
>> +             v4l2_err(sd, "Failed to write initial settings\n");
>> +
>> +     return 0;
>
>         return ret; ?
> ...
>> +module_i2c_driver(v4l2_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("Samsung S5K4ECGX 5MP SOC camera");
>> +MODULE_AUTHOR("Sangwook Lee<sangwook.lee@xxxxxxxxxx>");
>> +MODULE_AUTHOR("Seok-Young Jang<quartz.jang@xxxxxxxxxxx>");
>
> Was there anything really contributed by this person ?

I think I have to give him some credits because I started to work on
the sensor driver which
was originally based on his driver source code. In fact I didn't add
the name, simply
his information was there, so I didn't delete the name, even though
later the driver was getting
more and more from s5k6aa driver.


>
>> +MODULE_LICENSE("GPL");
>> +MODULE_FIRMWARE(S5K4ECGX_FIRMWARE);
>> diff --git a/include/media/s5k4ecgx.h b/include/media/s5k4ecgx.h
>> new file mode 100644
>> index 0000000..fbed5cb
>> --- /dev/null
>> +++ b/include/media/s5k4ecgx.h
>> @@ -0,0 +1,37 @@
>> +/*
>> + * S5K4ECGX image sensor header file
>> + *
>> + * Copyright (C) 2012, Linaro
>> + * Copyright (C) 2011, Samsung Electronics Co., Ltd.
>
> 2011 -> 2012 ?
Ok, I will fix this.

>
> Otherwise looks good. Would be interesting to add capture support
> to this driver later. I've seen it supports JPEG compressed stream,
> also with interleaved preview raw data inside it.

Yes, we can tackle this later.

>
> We have similar problem with S5C73M3 sensor, you have to configure
> two resolutions for JPEG and YUV for a single stream. Here you
> additionally could choose from various preview "sub-stream" pixel
> formats (YCBCR, RGB, etc.).
>
> --
>
> Regards,
> Sylwester

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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux