Re: [PATCH] media: i2c: add support for omnivision's ov2659 sensor

[Lad, Prabhakar]

Hi Laurent,

Thanks for the review.

On Wed, Feb 4, 2015 at 5:03 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hi Prabhakar,
>
> Thank you for the patch. Here's a partial review.
>
> On Thursday 15 January 2015 23:39:23 Lad, Prabhakar wrote:
>> From: Benoit Parrot <bparrot@xxxxxx>
>>
>> this patch adds support for omnivision's ov2659
>> sensor.
>>
>> Signed-off-by: Benoit Parrot <bparrot@xxxxxx>
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
>> ---
>>  .../devicetree/bindings/media/i2c/ov2659.txt       |   33 +
>>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>>  MAINTAINERS                                        |   10 +
>>  drivers/media/i2c/Kconfig                          |   11 +
>>  drivers/media/i2c/Makefile                         |    1 +
>>  drivers/media/i2c/ov2659.c                         | 1623 +++++++++++++++++
>>  include/media/ov2659.h                             |   33 +
>>  7 files changed, 1712 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2659.txt
>>  create mode 100644 drivers/media/i2c/ov2659.c
>>  create mode 100644 include/media/ov2659.h
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
>> b/Documentation/devicetree/bindings/media/i2c/ov2659.txt new file mode
>> 100644
>> index 0000000..fc49f44
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
>> @@ -0,0 +1,33 @@
>> +* OV2659 1/5-Inch 2Mp SOC Camera
>> +
>> +The OV2659 is a 1/5-inch SOC camera, with an active array size of 1632H x
>> 1212V.
>> +It is programmable through a SCCB.
>> +
>> +Required Properties:
>> +- compatible: Must be "ovt,ov2659"
>> +
>> +- reg: I2C slave address
>> +
>> +- clock-frequency: Input clock frequency.
>> +
>> +For further reading on port node refer to
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +
>> +     i2c0@1c22000 {
>> +             ...
>> +             ...
>> +              ov2659@30 {
>> +                     compatible = "ovt,ov2659";
>> +                     reg = <0x30>;
>> +
>> +                     port {
>> +                             ov2659_0: endpoint {
>> +                                     clock-frequency = <12000000>;
>> +                                     remote-endpoint = <&vpfe_ep>;
>> +                             };
>> +                     };
>> +             };
>> +             ...
>> +     };
>
> [snip]
>
>> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
>> new file mode 100644
>> index 0000000..ce8ec8d
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov2659.c
>> @@ -0,0 +1,1623 @@
>
> [snip]
>
>> +static int debug;
>> +module_param(debug, int, 0644);
>> +MODULE_PARM_DESC(debug, "Debug level (0-2)");
>
> The debug parameter is printed in the probe function and then unused. You can
> remove it.
>
OK will drop it.

> [snip]
>
>> +struct ov2659 {
>> +     struct v4l2_subdev sd;
>> +     struct media_pad pad;
>> +     enum v4l2_mbus_type bus_type;
>> +     const struct ov2659_platform_data *pdata;
>> +
>> +     /* Protects the struct fields below */
>> +     struct mutex lock;
>> +
>> +     struct i2c_client *client;
>> +
>> +     unsigned short id;
>
> The id field is only used at probe time, you can make it a local variable.
>
OK will make local.

>> +     const struct ov2659_framesize *frame_size;
>> +     /* Current Output format Register Value (REG_FORMAT_CTRL00) */
>> +     struct sensor_register *format_ctrl_regs;
>> +
>> +     struct v4l2_mbus_framefmt format;
>> +
>> +     /* Sensor specific feq/pll config */
>> +     struct ov2659_pll_ctrl pll;
>> +
>> +     int streaming;
>> +     int power;
>> +};
>
> [snip]
>
>> +static const struct ov2659_framesize ov2659_framesizes[] = {
>> +     { /* QVGA */
>> +             .width          = 320,
>> +             .height         = 240,
>> +             .regs           = ov2659_qvga,
>> +             .max_exp_lines  = 248,
>> +     }, { /* VGA */
>> +             .width          = 640,
>> +             .height         = 480,
>> +             .regs           = ov2659_vga,
>> +             .max_exp_lines  = 498,
>> +     }, { /* SVGA */
>> +             .width          = 800,
>> +             .height         = 600,
>> +             .regs           = ov2659_svga,
>> +             .max_exp_lines  = 498,
>> +     }, { /* XGA */
>> +             .width          = 1024,
>> +             .height         = 768,
>> +             .regs           = ov2659_xga,
>> +             .max_exp_lines  = 498,
>> +     }, { /* 720P */
>> +             .width          = 1280,
>> +             .height         = 720,
>> +             .regs           = ov2659_720p,
>> +             .max_exp_lines  = 498,
>> +     }, { /* SXGA */
>> +             .width          = 1280,
>> +             .height         = 1024,
>> +             .regs           = ov2659_sxga,
>> +             .max_exp_lines  = 1048,
>> +     }, { /* UXGA */
>> +             .width          = 1600,
>> +             .height         = 1200,
>> +             .regs           = ov2659_uxga,
>> +             .max_exp_lines  = 498,
>> +     },
>> +};
>
> That's what bothers me the most about drivers for Omnivision sensors. For some
> reason (I'd bet on lack of proper documentation) they list a couple of
> supported resolutions with corresponding register values, instead of computing
> the register values from the format configured by userspace. That's not the
> way we want to go. Prabhakar, do you have enough documentation to fix that ?
>
I am afraid I have limited documentation here.

> [snip]
>
>> +/* sensor register write */
>> +static int ov2659_write(struct i2c_client *client, u16 reg, u8 val)
>> +{
>> +     struct i2c_msg msg[1];
>> +     int ret, i;
>> +     u8 buf[3];
>> +
>> +     buf[0] = reg >> 8;
>> +     buf[1] = reg & 0xFF;
>> +     buf[2] = val;
>> +
>> +     msg->addr = client->addr;
>> +     msg->flags = client->flags;
>> +     msg->buf = buf;
>> +     msg->len = sizeof(buf);
>> +
>> +     for (i = 0; i < 5; i++) {
>
> Is the loop really needed, or just copied from a different driver ?
>
not really, just incase if transfer fails retrys.

>> +             ret = i2c_transfer(client->adapter, msg, 1);
>> +             if (ret >= 0)
>> +                     return 0;
>> +
>> +             dev_dbg(&client->dev,
>> +                     "ov2659 write reg(0x%x val:0x%x) failed !\n", reg, val);
>> +             udelay(10);
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +/* sensor register read */
>> +static int ov2659_read(struct i2c_client *client, u16 reg, u8 *val)
>> +{
>> +     struct i2c_msg msg[2];
>> +     int ret, i;
>> +     u8 buf[2];
>> +
>> +     buf[0] = reg >> 8;
>> +     buf[1] = reg & 0xFF;
>> +
>> +     msg[0].addr = client->addr;
>> +     msg[0].flags = client->flags;
>> +     msg[0].buf = buf;
>> +     msg[0].len = sizeof(buf);
>> +
>> +     msg[1].addr = client->addr;
>> +     msg[1].flags = client->flags|I2C_M_RD;
>
> checkpatch.pl should have warned about this. Could you please fix checkpatch
> errors and warnings ?
>
strange this was checkpatched, anyway will fix it.

>> +     msg[1].buf = buf;
>> +     msg[1].len = 1;
>> +
>> +     for (i = 0; i < 5; i++) {
>> +             ret = i2c_transfer(client->adapter, msg, 2);
>> +             if (ret >= 0) {
>> +                     *val = buf[0];
>> +                     return 0;
>> +             }
>> +
>> +             dev_dbg(&client->dev,
>> +                     "ov2659 read reg(0x%x val:0x%x) failed !\n", reg, *val);
>> +             udelay(10);
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int ov2659_write_array(struct i2c_client *client,
>> +                           const struct sensor_register *regs)
>> +{
>> +     int i, ret = 0;
>> +
>> +     for (i = 0; ret == 0 && regs[i].addr; i++) {
>> +             ret = ov2659_write(client, regs[i].addr, regs[i].value);
>> +             usleep_range(5000, 6000);
>
> That will result in a large delay when the array grows. Is it really needed ?
> Waiting might be needed after writing some of the registers, but most probably
> not most of them.
>
Ok will verify it.


>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int dump_reg(struct i2c_client *client, u16 reg)
>> +{
>> +     u8 val = 0;
>> +     int ret;
>> +
>> +     ret = ov2659_read(client, reg, &val);
>> +     dev_dbg(&client->dev, "%s: 0x%04x: 0x%02x\n",
>> +                             __func__, reg, val);
>> +     return ret;
>> +}
>> +
>> +static void ov2659_reg_dump(struct i2c_client *client)
>> +{
>> +     int i;
>> +
>> +     dump_reg(client, REG_SOFTWARE_STANDBY);
>> +     dump_reg(client, REG_SOFTWARE_RESET);
>> +
>> +     for (i = 0x3000; i <= 0x302f; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3400; i <= 0x3406; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3500; i <= 0x3513; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3600; i <= 0x3640; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3800; i <= 0x3821; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3a00; i <= 0x3a26; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4000; i <= 0x4009; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4201; i <= 0x4202; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4300; i <= 0x4301; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4600; i <= 0x4609; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4700; i <= 0x4709; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x5000; i <= 0x50a0; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x5600; i <= 0x5606; i++)
>> +             dump_reg(client, i);
>
> That's pretty rough. I'm not sure such a detailed level of debugging is
> needed. You should at least return at the beginning of the function if DEBUG
> isn't defined, to avoid reading all registers without printing their value.
>
OK will fix it.

>> +}
>
> [snip]
>
>> +static void __ov2659_set_power(struct ov2659 *ov2659, int on)
>> +{
>> +     struct i2c_client *client = ov2659->client;
>> +
>> +     on = !!on;
>> +
>> +     dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
>> +
>> +     if (ov2659->power == on)
>> +             return;
>> +
>> +     ov2659->power = on;
>
> This seems to be a no-op. I would either remove this function, or, better, add
> regulators support to the driver.
>
Will drop it.

>> +}
>> +
>> +static int ov2659_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> +     struct ov2659 *ov2659 = to_ov2659(sd);
>> +     struct i2c_client *client = ov2659->client;
>> +
>> +     dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
>> +
>> +     mutex_lock(&ov2659->lock);
>> +     __ov2659_set_power(ov2659, on);
>> +     mutex_unlock(&ov2659->lock);
>> +
>> +     return 0;
>> +}
>
> [snip]
>
>> +static int ov2659_s_stream(struct v4l2_subdev *sd, int on)
>> +{
>> +     struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +     struct ov2659 *ov2659 = to_ov2659(sd);
>> +     int ret;
>> +
>> +     on = !!on;
>> +
>> +     dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
>> +
>> +     if (ov2659->streaming == on)
>> +             return 0;
>> +
>> +     mutex_lock(&ov2659->lock);
>
> Shouldn't you protect the above check as well ?
>
Yes, will fix it.

>> +
>> +     if (!on) {
>> +             /* Stop Streaming Sequence */
>> +             ov2659_set_streaming(ov2659, 0);
>> +             ov2659->streaming = on;
>> +             __ov2659_set_power(ov2659, 0);
>> +             mutex_unlock(&ov2659->lock);
>> +
>> +             return 0;
>> +     }
>> +
>> +     /* Start Streaming Sequence */
>> +     __ov2659_set_power(ov2659, 1);
>> +
>> +     ret = ov2659_write_array(client, ov2659_init_regs);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ov2659_set_pixel_clock(ov2659);
>> +     ov2659_set_frame_size(ov2659);
>> +     ov2659_set_format(ov2659);
>> +
>> +     ov2659_set_streaming(ov2659, 1);
>> +
>> +     ov2659_reg_dump(client);
>> +     ov2659->streaming = on;
>> +     mutex_unlock(&ov2659->lock);
>> +
>> +     return 0;
>> +}
>
> [snip]
>
>> +static int ov2659_detect_sensor(struct v4l2_subdev *sd)
>> +{
>> +     struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +     struct ov2659 *ov2659 = to_ov2659(sd);
>> +     u8 pid, ver;
>> +     int ret;
>> +
>> +     dev_dbg(&client->dev, "%s:\n", __func__);
>> +
>> +     mutex_lock(&ov2659->lock);
>
> As sensor detection is only performed from the probe function before
> registering the subdev I don't think locking is needed here.
>
OK will drop it.

>> +      __ov2659_set_power(ov2659, 1);
>> +     usleep_range(25000, 26000);
>> +
>> +     /* soft reset */
>> +     ret = ov2659_write(client, REG_SOFTWARE_RESET, 0x01);
>> +     if (ret != 0) {
>> +             dev_err(&client->dev, "Sensor soft reset failed\n");
>> +             ret = -ENODEV;
>
> ret is then ignored.
>
will return ret.

>> +     }
>> +     mdelay(5);
>
> Don't delay, please sleep.
>
OK

>> +
>> +     /* Check sensor revision */
>> +     ret = ov2659_read(client, REG_SC_CHIP_ID_H, &pid);
>> +     if (!ret)
>> +             ret = ov2659_read(client, REG_SC_CHIP_ID_L, &ver);
>> +
>> +     __ov2659_set_power(ov2659, 0);
>> +
>> +     if (!ret) {
>> +             ov2659->id = OV265X_ID(pid, ver);
>> +             if (ov2659->id != OV2659_ID) {
>> +                     dev_err(&client->dev,
>> +                             "Sensor detection failed (%04X, %d)\n",
>> +                             ov2659->id, ret);
>> +                     ret = -ENODEV;
>> +             } else {
>> +                     dev_info(&client->dev, "Found OV%04X sensor\n",
>> +                             ov2659->id);
>> +             }
>> +     }
>> +     mutex_unlock(&ov2659->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static struct ov2659_platform_data *
>> +ov2659_get_pdata(struct i2c_client *client)
>> +{
>> +     struct ov2659_platform_data *pdata;
>> +     struct device_node *endpoint;
>> +     int ret;
>> +
>> +     dev_dbg(&client->dev, "ov2659_get_pdata invoked\n");
>> +
>> +     if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
>> +             return client->dev.platform_data;
>> +
>> +     dev_dbg(&client->dev, "ov2659_get_pdata: DT Node found\n");
>> +
>> +     endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL);
>> +     if (!endpoint)
>> +             return NULL;
>> +
>> +     dev_dbg(&client->dev, "ov2659_get_pdata: endpoint found\n");
>> +
>> +     pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>> +     if (!pdata)
>> +             goto done;
>> +
>> +     ret = of_property_read_u32(endpoint, "clock-frequency",
>> +                                &pdata->mclk_frequency);
>> +     if (ret < 0) {
>> +             pdata->mclk_frequency = OV2659_DEFAULT_MCLK_FREQ;
>> +             dev_info(&client->dev, "using default %u Hz clock frequency\n",
>> +                      pdata->mclk_frequency);
>> +     }
>
> The clock-frequency property is defined as mandatory, a default case is thus
> not needed.
>
OK will drop it.

> I would add CCF support to the driver and request the input clock (which is
> called xvclk and not mclk) at probe time. You could then set the clock
> frequency to the value specified in DT, and read it back to get the exact
> frequency provided by the clock source.
>
Ok will add CCF.

Regards,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/