Hi Sakari, Here's my comments.
-Hyungwoo -----Original Message----- > From: Sakari Ailus [mailto:sakari.ai...@iki.fi] > Sent: Saturday, May 27, 2017 1:31 PM > To: Yang, Hyungwoo <hyungwoo.y...@intel.com> > Cc: linux-media@vger.kernel.org; sakari.ai...@linux.intel.com; Zheng, Jian Xu > <jian.xu.zh...@intel.com>; Hsu, Cedric <cedric....@intel.com>; > tf...@chromium.org > Subject: Re: [PATCH v3 1/1] [media] i2c: add support for OV13858 sensor > > Hi Hyungwoo, > > Thanks for the update. A few comments below. > > > + > > +/* Mode : resolution and related config&values */ > > +struct ov13858_mode > > +{ > > + /* Frame width */ > > + u32 width; > > + /* Frame height */ > > + u32 height; > > + > > + /* V-timing */ > > + u32 vts; > > Aren't the three fields here unused? ?? Yes, they are used. : : : > > > +/* Mode configs */ > > +static const struct ov13858_mode supported_modes[] = { > > + { > > + .width = 4224, > > + .height = 3136, > > + .vts = OV13858_VTS_30FPS, > > + .reg_list = { > > + .num_of_regs = ARRAY_SIZE(mode_4224x3136_regs), > > + .regs = mode_4224x3136_regs, > > + }, > > + .link_freq_index = OV13858_LINK_FREQ_INDEX_0, > > + }, > > + { > > + .width = 2112, > > + .height = 1568, > > + .vts = OV13858_VTS_30FPS, > > + .reg_list = { > > + .num_of_regs = ARRAY_SIZE(mode_2112x1568_regs), > > + .regs = mode_2112x1568_regs, > > + }, > > + .link_freq_index = OV13858_LINK_FREQ_INDEX_1, > > + }, > > + { > > + .width = 2112, > > + .height = 1188, > > + .vts = OV13858_VTS_30FPS, > > + .reg_list = { > > + .num_of_regs = ARRAY_SIZE(mode_2112x1188_regs), > > + .regs = mode_2112x1188_regs, > > + }, > > + .link_freq_index = OV13858_LINK_FREQ_INDEX_1, > > + }, > > + { > > + .width = 1056, > > + .height = 784, > > + .vts = OV13858_VTS_30FPS, > > + .reg_list = { > > + .num_of_regs = ARRAY_SIZE(mode_1056x784_regs), > > + .regs = mode_1056x784_regs, > > + }, > > + .link_freq_index = OV13858_LINK_FREQ_INDEX_1, > > + } > > +}; > > + > > +struct ov13858 { > > + struct v4l2_subdev sd; > > + struct media_pad pad; > > + > > + struct v4l2_ctrl_handler ctrl_handler; > > + /* V4L2 Controls */ > > + struct v4l2_ctrl *link_freq; > > + struct v4l2_ctrl *pixel_rate; > > + struct v4l2_ctrl *vblank; > > + struct v4l2_ctrl *exposure; > > + > > + /* Current mode */ > > + const struct ov13858_mode *cur_mode; > > + > > + /* Num of skip frames */ > > + u32 num_of_skip_frames; > > If you always tell the receiver to skip OV13858_NUM_OF_SKIP_FRAMES frames in > the beginning, you could just return this from your g_skip_frames callback. > Oops!!! Yes, you're right. My original version gets some platform dependent values including this through fwnode API. I removed them to make this simple but didn't think much. I'll remove this. > > + > > + /* Mutex for serialized access */ > > + struct mutex mutex; > > + > > + /* Streaming on/off */ > > + bool streaming; > > +}; > > + > > +#define to_ov13858(_sd) container_of(_sd, struct ov13858, sd) > > + > > +/* Read registers up to 4 at a time */ static int > > +ov13858_read_reg(struct ov13858 *ov13858, u16 reg, u32 len, u32 *val) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd); > > + struct i2c_msg msgs[2]; > > + u8 *data_be_p; > > + int ret; > > + u32 data_be = 0; > > + u16 reg_addr_be = cpu_to_be16(reg); > > + > > + if (len > 4) > > + return -EINVAL; > > + > > + data_be_p = (u8 *)&data_be; > > + /* Write register address */ > > + msgs[0].addr = client->addr; > > + msgs[0].flags = 0; > > + msgs[0].len = 2; > > + msgs[0].buf = (u8 *)®_addr_be; > > + > > + /* Read data from register */ > > + msgs[1].addr = client->addr; > > + msgs[1].flags = I2C_M_RD; > > + msgs[1].len = len; > > + msgs[1].buf = &data_be_p[4 - len]; > > + > > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > > + if (ret != ARRAY_SIZE(msgs)) > > + return -EIO; > > + > > + *val = be32_to_cpu(data_be); > > + > > + return 0; > > +} > > + > > +/* Write registers up to 4 at a time */ static int > > +ov13858_write_reg(struct ov13858 *ov13858, u16 reg, u32 len, u32 val) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd); > > + int buf_i, val_i; > > + u8 buf[6], *val_p; > > + > > + if (len > 4) > > + return -EINVAL; > > + > > + buf[0] = reg >> 8; > > + buf[1] = reg & 0xff; > > + > > + buf_i = 2; > > + val_p = (u8 *)&val; > > + val_i = len - 1; > > + > > + while (val_i >= 0) > > + buf[buf_i++] = val_p[val_i--]; > > + > > + if (i2c_master_send(client, buf, len + 2) != len + 2) > > + return -EIO; > > + > > + return 0; > > +} > > + > > +/* Write a list of registers */ > > +static int ov13858_write_regs(struct ov13858 *ov13858, > > + const struct ov13858_reg *regs, u32 len) { > > + struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd); > > + int ret; > > + u32 i; > > + > > + for (i = 0; i < len; i++) { > > + ret = ov13858_write_reg(ov13858, regs[i].address, 1, > > + regs[i].val); > > + if (ret) { > > + dev_err_ratelimited( > > + &client->dev, > > + "Failed to write reg 0x%4.4x. error = %d\n", > > + regs[i].address, ret); > > + > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int ov13858_write_reg_list(struct ov13858 *ov13858, > > + const struct ov13858_reg_list *r_list) { > > + return ov13858_write_regs(ov13858, r_list->regs, > > +r_list->num_of_regs); } > > + > > +/* Open sub-device */ > > +static int ov13858_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh > > +*fh) { > > + struct ov13858 *ov13858 = to_ov13858(sd); > > + struct v4l2_mbus_framefmt *try_fmt; > > + > > + mutex_lock(&ov13858->mutex); > > + > > + /* Initialize try_fmt */ > > + try_fmt = v4l2_subdev_get_try_format(sd, fh->pad, 0); > > + try_fmt->width = ov13858->cur_mode->width; > > + try_fmt->height = ov13858->cur_mode->height; > > + try_fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10; > > + try_fmt->field = V4L2_FIELD_NONE; > > + > > + /* No crop or compose */ > > + mutex_unlock(&ov13858->mutex); > > + > > + return 0; > > +} > > + > > +/* Update max exposure while meeting expected vertial blanking */ > > +static void ov13858_update_exposure_limits(struct ov13858 *ov13858) { > > + s64 max; > > + > > + max = ov13858->cur_mode->height + ov13858->vblank->val - 8; > > + __v4l2_ctrl_modify_range(ov13858->exposure, ov13858->exposure->minimum, > > + max, ov13858->exposure->step, max); } > > + > > +/* Update exposure */ > > +static int ov13858_update_exposure(struct ov13858 *ov13858, > > + struct v4l2_ctrl *ctrl) > > +{ > > + return ov13858_write_reg(ov13858, OV13858_REG_EXPOSURE, > > + OV13858_REG_VALUE_24BIT, ctrl->val << 4); } > > + > > +/* Update VTS that meets expected vertical blanking */ static int > > +ov13858_update_vblank(struct ov13858 *ov13858, > > + struct v4l2_ctrl *ctrl) > > +{ > > + return ov13858_write_reg( > > + ov13858, OV13858_REG_VTS, > > + OV13858_REG_VALUE_16BIT, > > + ov13858->cur_mode->height + ov13858->vblank->val); } > > + > > +/* Update analog gain */ > > +static int ov13858_update_analog_gain(struct ov13858 *ov13858, > > + struct v4l2_ctrl *ctrl) > > +{ > > + return ov13858_write_reg(ov13858, OV13858_REG_ANALOG_GAIN, > > + OV13858_REG_VALUE_16BIT, ctrl->val); > > I think I'd move what the four above functions do to ov13858_set_ctrl() > unless they're used in more than one location. Why ? Personally I like this. Since there wouldn't be any difference in generated machine code, I want to keep this if there's no strict rule on this. > > > +} > > + > > +static int ov13858_set_ctrl(struct v4l2_ctrl *ctrl) { > > + struct ov13858 *ov13858 = container_of(ctrl->handler, > > + struct ov13858, ctrl_handler); > > + struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd); > > + int ret; > > + > > + /* Propagate change of current control to all related controls */ > > + switch (ctrl->id) { > > + case V4L2_CID_VBLANK: > > + ov13858_update_exposure_limits(ov13858); > > + break; > > + }; > > + > > + /* > > + * Applying V4L2 control value only happens > > + * when power is up for streaming > > + */ > > + if (pm_runtime_get_if_in_use(&client->dev) <= 0) > > + return 0; > > + > > + ret = 0; > > + switch (ctrl->id) { > > + case V4L2_CID_ANALOGUE_GAIN: > > + ret = ov13858_update_analog_gain(ov13858, ctrl); > > + break; > > + case V4L2_CID_EXPOSURE: > > + ret = ov13858_update_exposure(ov13858, ctrl); > > + break; > > + case V4L2_CID_VBLANK: > > + ret = ov13858_update_vblank(ov13858, ctrl); > > + break; > > + default: > > + dev_info(&client->dev, > > + "ctrl(id:0x%x,val:0x%x) is not handled\n", > > + ctrl->id, ctrl->val); > > + break; > > + }; > > + > > + pm_runtime_put(&client->dev); > > + > > + return ret; > > +} > > + : : > > +/* > > + * Prepare streaming by writing default values and customized values. > > + * This should be called with ov13858->mutex acquired. > > + */ > > +static int ov13858_prepare_streaming(struct ov13858 *ov13858) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd); > > + const struct ov13858_reg_list *reg_list; > > + int ret, link_freq_index; > > + > > + /* Get out of from software reset */ > > + ret = ov13858_write_reg(ov13858, OV13858_REG_SOFTWARE_RST, > > + OV13858_REG_VALUE_08BIT, OV13858_SOFTWARE_RST); > > + if (ret) { > > + dev_err(&client->dev, "%s failed to set powerup registers\n", > > + __func__); > > + return ret; > > + } > > + > > + /* Setup PLL */ > > + link_freq_index = ov13858->cur_mode->link_freq_index; > > + reg_list = &link_freq_configs[link_freq_index].reg_list; > > + ret = ov13858_write_reg_list(ov13858, reg_list); > > + if (ret) { > > + dev_err(&client->dev, "%s failed to set plls\n", __func__); > > + return ret; > > + } > > + > > + /* Apply default values of current mode */ > > + reg_list = &ov13858->cur_mode->reg_list; > > + ret = ov13858_write_reg_list(ov13858, reg_list); > > + if (ret) { > > + dev_err(&client->dev, "%s failed to set mode\n", __func__); > > + return ret; > > + } > > + > > + /* Apply customized values from user */ > > + return __v4l2_ctrl_handler_setup(ov13858->sd.ctrl_handler); > > +} > > + > > +/* Start streaming */ > > +static int ov13858_start_streaming(struct ov13858 *ov13858) > > +{ > > + int ret; > > + > > + /* Write default & customized values */ > > + ret = ov13858_prepare_streaming(ov13858); > > Could you merge this with ov13858_prepare_streaming()? > Why ? I want to keep this. If you want to worry about 1 more jump then, if it is really there, I can make this function "inline" > > + if (ret) > > + return ret; > > + > > + return ov13858_write_reg(ov13858, OV13858_REG_MODE_SELECT, > > + OV13858_REG_VALUE_08BIT, > > + OV13858_MODE_STREAMING); > > +} > > + > > +/* Stop streaming */ > > +static int ov13858_stop_streaming(struct ov13858 *ov13858) > > +{ > > + return ov13858_write_reg(ov13858, OV13858_REG_MODE_SELECT, > > + OV13858_REG_VALUE_08BIT, OV13858_MODE_STANDBY); > > +} > > + : : > > +MODULE_AUTHOR("Kan, Chris <chris....@intel.com>"); > > +MODULE_AUTHOR("Rapolu, Chiranjeevi <chiranjeevi.rap...@intel.com>"); > > +MODULE_AUTHOR("Yang, Hyungwoo <hyungwoo.y...@intel.com>"); > > +MODULE_DESCRIPTION("Omnivision ov13858 sensor driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > Kind regards, > > Sakari Ailus > e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk >
