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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html