Hi Jacopo,
On 2018年01月10日 17:08, jacopo mondi wrote:
Hello Shunqian,
On Wed, Jan 10, 2018 at 10:06:05AM +0800, Shunqian Zheng wrote:
[snip]
+static int __ov5695_start_stream(struct ov5695 *ov5695)
+{
+ int ret;
+
+ ret = ov5695_write_array(ov5695->client, ov5695_global_regs);
+ if (ret)
+ return ret;
+ ret = ov5695_write_array(ov5695->client, ov5695->cur_mode->reg_list);
+ if (ret)
+ return ret;
+
+ /* In case these controls are set before streaming */
+ ret = __v4l2_ctrl_handler_setup(&ov5695->ctrl_handler);
+ if (ret)
+ return ret;
+
+ return ov5695_write_reg(ov5695->client, OV5695_REG_CTRL_MODE,
+ OV5695_REG_VALUE_08BIT, OV5695_MODE_STREAMING);
+}
+
+static int __ov5695_stop_stream(struct ov5695 *ov5695)
+{
+ return ov5695_write_reg(ov5695->client, OV5695_REG_CTRL_MODE,
+ OV5695_REG_VALUE_08BIT, OV5695_MODE_SW_STANDBY);
+}
+
+static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
+{
+ struct ov5695 *ov5695 = to_ov5695(sd);
+ struct i2c_client *client = ov5695->client;
+ int ret = 0;
+
+ mutex_lock(&ov5695->mutex);
+ on = !!on;
+ if (on == ov5695->streaming)
+ goto unlock_and_return;
+
+ if (on) {
+ ret = pm_runtime_get_sync(&client->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&client->dev);
+ goto unlock_and_return;
+ }
+
+ ret = __ov5695_start_stream(ov5695);
+ if (ret) {
+ v4l2_err(sd, "start stream failed while write regs\n");
+ pm_runtime_put(&client->dev);
+ goto unlock_and_return;
+ }
+ } else {
+ __ov5695_stop_stream(ov5695);
+ ret = pm_runtime_put(&client->dev);
I would return the result of __ov5695_stop_stream() instead of
pm_runtime_put().
I know I asked for this, but if the first s_stream(0) fails, the
sensor may not have been stopped but the interface will be put in
"streaming = 0" state, preventing a second s_stream(0) to be issued
because of your check "on == ov5695->streaming" a few lines above.
I can't tell how bad this is. Imho is acceptable but I would like to
hear someone else opinion here :)
How about not checking the return values of s_stream(0) branch?
It seems not much this driver can do if pm_runtime_put() fails.
+ }
+
+ ov5695->streaming = on;
+
+unlock_and_return:
+ mutex_unlock(&ov5695->mutex);
+
+ return ret;
+}
+
+
[snip]
+static const struct of_device_id ov5695_of_match[] = {
+ { .compatible = "ovti,ov5695" },
+ {},
+};
If you don't list CONFIG_OF as a dependecy for this driver (which you
should not imho), please guard this with:
#if IS_ENABLED(CONFIG_OF)
#endif
+
+static struct i2c_driver ov5695_i2c_driver = {
+ .driver = {
+ .name = "ov5695",
+ .owner = THIS_MODULE,
+ .pm = &ov5695_pm_ops,
+ .of_match_table = ov5695_of_match
+ },
+ .probe = &ov5695_probe,
+ .remove = &ov5695_remove,
+};
+
+module_i2c_driver(ov5695_i2c_driver);
+
+MODULE_DESCRIPTION("OmniVision ov5695 sensor driver");
+MODULE_LICENSE("GPL v2");
As you've fixed my comments on v1, and with the above bits addressed:
Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
Thank you very much~
Shunqian
Thanks
j
--
1.9.1
