lkml.org 
[lkml]   [2016]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 2/2] Add support for OV5647 sensor
    From
    Date
    Hi Pavel!

    On 11/15/2016 12:10 PM, Pavel Machek wrote:
    > Hi!
    >
    >> Add support for OV5647 sensor.
    >>
    >
    >> +static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
    >> +{
    >> + int ret;
    >> + unsigned char data[3] = { reg >> 8, reg & 0xff, val};
    >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
    >> +
    >> + ret = i2c_master_send(client, data, 3);
    >> + if (ret != 3) {
    >> + dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
    >> + __func__, reg);
    >> + return ret < 0 ? ret : -EIO;
    >> + }
    >> + return 0;
    >> +}
    >
    > Sorry, this is wrong. It should something <0 any time error is detected.
    >
    >> +static int ov5647_write_array(struct v4l2_subdev *sd,
    >> + struct regval_list *regs, int array_size)
    >> +{
    >> + int i = 0;
    >> + int ret = 0;
    >> +
    >> + if (!regs)
    >> + return -EINVAL;
    >> +
    >> + while (i < array_size) {
    >> + ret = ov5647_write(sd, regs->addr, regs->data);
    >> + if (ret < 0)
    >> + return ret;
    >> + i++;
    >> + regs++;
    >> + }
    >> + return 0;
    >> +}
    >
    > For example this expects <0 on error.
    >
    >> +static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
    >> +{
    >> + int ret;
    >> + unsigned char rdval;
    >> +
    >> + ret = ov5647_read(sd, 0x0100, &rdval);
    >> + if (ret != 0)
    >> + return ret;
    >> +
    >> + if (standby)
    >> + ret = ov5647_write(sd, 0x0100, rdval&0xfe);
    >> + else
    >> + ret = ov5647_write(sd, 0x0100, rdval|0x01);
    >> +
    >> + return ret;
    >
    > if (standby)
    > rdval &= 0xfe;
    > else
    > rdval |= 0x01;
    >
    > ret = ov5647_write(sd, 0x0100, rdval);
    >
    > ?
    >
    >
    >> +/**
    >> + * @short Store information about the video data format.
    >> + */
    >> +static struct sensor_format_struct {
    >> + __u8 *desc;
    >> + u32 mbus_code;
    >
    > u8 is suitable here.
    >
    >
    >> + ov5647_read(sd, 0x0100, &resetval);
    >> + if (!resetval&0x01) {
    >
    > add ()s here.
    >
    >> +static int sensor_power(struct v4l2_subdev *sd, int on)
    >> +{
    >> + int ret;
    >> + struct ov5647 *ov5647 = to_state(sd);
    >> + struct i2c_client *client = v4l2_get_subdevdata(sd);
    >> +
    >> + ret = 0;
    >> + mutex_lock(&ov5647->lock);
    >> +
    >> + if (on) {
    >> + dev_dbg(&client->dev, "OV5647 power on!\n");
    >> +
    >> + ret = ov5647_write_array(sd, sensor_oe_enable_regs,
    >> + ARRAY_SIZE(sensor_oe_enable_regs));
    >> +
    >> + ret = __sensor_init(sd);
    >> +
    >> + if (ret < 0)
    >> + dev_err(&client->dev,
    >> + "Camera not available! Check Power!\n");
    >> + } else {
    >> + dev_dbg(&client->dev, "OV5647 power off!\n");
    >> +
    >> + dev_dbg(&client->dev, "disable oe\n");
    >> + ret = ov5647_write_array(sd, sensor_oe_disable_regs,
    >> + ARRAY_SIZE(sensor_oe_disable_regs));
    >> +
    >> + if (ret < 0)
    >> + dev_dbg(&client->dev, "disable oe failed!\n");
    >> +
    >> + ret = set_sw_standby(sd, true);
    >> +
    >> + if (ret < 0)
    >> + dev_dbg(&client->dev, "soft stby failed!\n");
    >
    > dev_err for errors? Little less "!"s in the output?
    >
    >> +static int sensor_get_register(struct v4l2_subdev *sd,
    >> + struct v4l2_dbg_register *reg)
    >> +{
    >> + unsigned char val = 0;
    >> + int ret;
    >> +
    >> + ret = ov5647_read(sd, reg->reg & 0xff, &val);
    >> + reg->val = val;
    >> + reg->size = 1;
    >> + return ret;
    >> +}
    >
    > Filling reg->* when read failed is strange.
    >
    >> +static int sensor_set_register(struct v4l2_subdev *sd,
    >> + const struct v4l2_dbg_register *reg)
    >> +{
    >> + ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
    >> + return 0;
    >> +}
    >
    > error handling?
    >
    > Best regards,
    > Pavel
    >

    Thanks for the feedback, I've corrected all the issues you pointed out, except
    the one regarding the error handling in the ov5647_write function.

    Best Regards,
    Ramiro

    \
     
     \ /
      Last update: 2016-11-21 16:01    [W:3.887 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site