| Message ID | 20200523084103.31276-1-dongchun.zhu@mediatek.com |
|---|---|
| Headers | show |
| Series | media: i2c: Add support for OV02A10 sensor | expand |
Hi Tomasz, Sakari, and sirs, Could anyone help to review this patch? On Sat, 2020-05-23 at 16:41 +0800, Dongchun Zhu wrote: > Add a V4L2 sub-device driver for OV02A10 image sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > --- > MAINTAINERS | 1 + > drivers/media/i2c/Kconfig | 13 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1040 insertions(+) > create mode 100644 drivers/media/i2c/ov02a10.c > [snip] > +static int ov02a10_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct ov02a10 *ov02a10; > + unsigned int rotation; > + unsigned int clock_lane_tx_speed; > + unsigned int i; > + int ret; > + > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL); > + if (!ov02a10) > + return -ENOMEM; > + > + ret = ov02a10_check_hwcfg(dev, ov02a10); > + if (ret) { > + dev_err(dev, "failed to check HW configuration: %d", ret); > + return ret; > + } > + > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops); > + ov02a10->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT; > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10; > + > + /* Optional indication of physical rotation of sensor */ > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation); > + if (!ret && rotation == 180) { > + ov02a10->upside_down = true; > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > + } > + > + /* Optional indication of mipi TX speed */ > + ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed", > + &clock_lane_tx_speed); > + > + if (!ret) > + ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed; > + > + /* Get system clock (eclk) */ > + ov02a10->eclk = devm_clk_get(dev, "eclk"); > + if (IS_ERR(ov02a10->eclk)) { > + ret = PTR_ERR(ov02a10->eclk); > + dev_err(dev, "failed to get eclk %d\n", ret); > + return ret; > + } > + > + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", > + &ov02a10->eclk_freq); > + if (ret) { > + dev_err(dev, "failed to get eclk frequency\n"); > + return ret; > + } > + > + ret = clk_set_rate(ov02a10->eclk, ov02a10->eclk_freq); > + if (ret) { > + dev_err(dev, "failed to set eclk frequency (24MHz)\n"); > + return ret; > + } > + > + if (clk_get_rate(ov02a10->eclk) != OV02A10_ECLK_FREQ) { > + dev_warn(dev, "wrong eclk frequency %d Hz, expected: %d Hz\n", > + ov02a10->eclk_freq, OV02A10_ECLK_FREQ); > + return -EINVAL; > + } > + > + ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH); > + if (IS_ERR(ov02a10->pd_gpio)) { > + ret = PTR_ERR(ov02a10->pd_gpio); > + dev_err(dev, "failed to get powerdown-gpios %d\n", ret); > + return ret; > + } > + > + ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(ov02a10->n_rst_gpio)) { > + ret = PTR_ERR(ov02a10->n_rst_gpio); > + dev_err(dev, "failed to get reset-gpios %d\n", ret); > + return ret; > + } > + > + for (i = 0; i < ARRAY_SIZE(ov02a10_supply_names); i++) > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > + > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names), > + ov02a10->supplies); > + if (ret) { > + dev_err(dev, "failed to get regulators\n"); > + return ret; > + } > + > + mutex_init(&ov02a10->mutex); > + ov02a10->cur_mode = &supported_modes[0]; > + ret = ov02a10_initialize_controls(ov02a10); > + if (ret) { > + dev_err(dev, "failed to initialize controls\n"); > + goto err_destroy_mutex; > + } > + > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops; > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE; > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad); > + if (ret < 0) { > + dev_err(dev, "failed to init entity pads: %d", ret); > + goto err_free_handler; > + } > + > + pm_runtime_enable(dev); > + if (!pm_runtime_enabled(dev)) { > + ret = ov02a10_power_on(dev); > + if (ret < 0) { > + dev_err(dev, "failed to power on: %d\n", ret); > + goto err_free_handler; > + } > + } > + > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > + if (ret) { > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > + if (!pm_runtime_enabled(dev)) > + ov02a10_power_off(dev); > + goto err_clean_entity; > + } Tomasz, Sakari, is this ok? or coding like this: ret = v4l2_async_register_subdev(&ov02a10->subdev); if (!pm_runtime_enabled(dev)) ov02a10_power_off(dev); if (ret) { dev_err(dev, "failed to register V4L2 subdev: %d", ret); goto err_clean_entity; } What's your opinions about the change? > + > + return 0; > + > +err_clean_entity: > + media_entity_cleanup(&ov02a10->subdev.entity); > +err_free_handler: > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > +err_destroy_mutex: > + mutex_destroy(&ov02a10->mutex); > + > + return ret; > +} > + > +static int ov02a10_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + v4l2_async_unregister_subdev(sd); > + media_entity_cleanup(&sd->entity); > + v4l2_ctrl_handler_free(sd->ctrl_handler); > + pm_runtime_disable(&client->dev); > + if (!pm_runtime_status_suspended(&client->dev)) > + ov02a10_power_off(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + mutex_destroy(&ov02a10->mutex); > + > + return 0; > +} > + > +static const struct of_device_id ov02a10_of_match[] = { > + { .compatible = "ovti,ov02a10" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, ov02a10_of_match); > + > +static struct i2c_driver ov02a10_i2c_driver = { > + .driver = { > + .name = "ov02a10", > + .pm = &ov02a10_pm_ops, > + .of_match_table = ov02a10_of_match, > + }, > + .probe_new = &ov02a10_probe, > + .remove = &ov02a10_remove, > +}; > + > +module_i2c_driver(ov02a10_i2c_driver); > + > +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>"); > +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver"); > +MODULE_LICENSE("GPL v2"); > +
Hi Dongchun, On Thu, Jun 04, 2020 at 10:14:05AM +0800, Dongchun Zhu wrote: > Hi Tomasz, Sakari, and sirs, > > Could anyone help to review this patch? > > On Sat, 2020-05-23 at 16:41 +0800, Dongchun Zhu wrote: > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 13 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1040 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > [snip] > > > +static int ov02a10_probe(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct ov02a10 *ov02a10; > > + unsigned int rotation; > > + unsigned int clock_lane_tx_speed; > > + unsigned int i; > > + int ret; > > + > > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL); > > + if (!ov02a10) > > + return -ENOMEM; > > + > > + ret = ov02a10_check_hwcfg(dev, ov02a10); > > + if (ret) { > > + dev_err(dev, "failed to check HW configuration: %d", ret); > > + return ret; > > + } > > + > > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops); > > + ov02a10->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT; > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10; > > + > > + /* Optional indication of physical rotation of sensor */ > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation); > > + if (!ret && rotation == 180) { > > + ov02a10->upside_down = true; > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > > + } > > + > > + /* Optional indication of mipi TX speed */ > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed", > > + &clock_lane_tx_speed); > > + > > + if (!ret) > > + ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed; > > + > > + /* Get system clock (eclk) */ > > + ov02a10->eclk = devm_clk_get(dev, "eclk"); > > + if (IS_ERR(ov02a10->eclk)) { > > + ret = PTR_ERR(ov02a10->eclk); > > + dev_err(dev, "failed to get eclk %d\n", ret); > > + return ret; > > + } > > + > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", > > + &ov02a10->eclk_freq); > > + if (ret) { > > + dev_err(dev, "failed to get eclk frequency\n"); > > + return ret; > > + } > > + > > + ret = clk_set_rate(ov02a10->eclk, ov02a10->eclk_freq); > > + if (ret) { > > + dev_err(dev, "failed to set eclk frequency (24MHz)\n"); > > + return ret; > > + } > > + > > + if (clk_get_rate(ov02a10->eclk) != OV02A10_ECLK_FREQ) { > > + dev_warn(dev, "wrong eclk frequency %d Hz, expected: %d Hz\n", > > + ov02a10->eclk_freq, OV02A10_ECLK_FREQ); > > + return -EINVAL; > > + } > > + > > + ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH); > > + if (IS_ERR(ov02a10->pd_gpio)) { > > + ret = PTR_ERR(ov02a10->pd_gpio); > > + dev_err(dev, "failed to get powerdown-gpios %d\n", ret); > > + return ret; > > + } > > + > > + ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > > + if (IS_ERR(ov02a10->n_rst_gpio)) { > > + ret = PTR_ERR(ov02a10->n_rst_gpio); > > + dev_err(dev, "failed to get reset-gpios %d\n", ret); > > + return ret; > > + } > > + > > + for (i = 0; i < ARRAY_SIZE(ov02a10_supply_names); i++) > > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > > + > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names), > > + ov02a10->supplies); > > + if (ret) { > > + dev_err(dev, "failed to get regulators\n"); > > + return ret; > > + } > > + > > + mutex_init(&ov02a10->mutex); > > + ov02a10->cur_mode = &supported_modes[0]; > > + ret = ov02a10_initialize_controls(ov02a10); > > + if (ret) { > > + dev_err(dev, "failed to initialize controls\n"); > > + goto err_destroy_mutex; > > + } > > + > > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops; > > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE; > > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad); > > + if (ret < 0) { > > + dev_err(dev, "failed to init entity pads: %d", ret); > > + goto err_free_handler; > > + } > > + > > + pm_runtime_enable(dev); > > + if (!pm_runtime_enabled(dev)) { > > + ret = ov02a10_power_on(dev); > > + if (ret < 0) { > > + dev_err(dev, "failed to power on: %d\n", ret); > > + goto err_free_handler; > > + } > > + } > > + > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > + if (ret) { > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > + if (!pm_runtime_enabled(dev)) > > + ov02a10_power_off(dev); This should be moved to error handling section below. > > + goto err_clean_entity; > > + } > > Tomasz, Sakari, is this ok? > or coding like this: > > ret = v4l2_async_register_subdev(&ov02a10->subdev); > if (!pm_runtime_enabled(dev)) > ov02a10_power_off(dev); > if (ret) { > dev_err(dev, "failed to register V4L2 subdev: %d", ret); > goto err_clean_entity; > } > > What's your opinions about the change? This turns power off if runtime PM is disabled. I'd keep it as-is, as it'd require re-implementing what runtime PM is used for now --- and that's not a sensor driver's job. > > > + > > + return 0; > > + > > +err_clean_entity: > > + media_entity_cleanup(&ov02a10->subdev.entity); > > +err_free_handler: > > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > > +err_destroy_mutex: > > + mutex_destroy(&ov02a10->mutex); > > + > > + return ret; > > +} > > + > > +static int ov02a10_remove(struct i2c_client *client) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + v4l2_async_unregister_subdev(sd); > > + media_entity_cleanup(&sd->entity); > > + v4l2_ctrl_handler_free(sd->ctrl_handler); > > + pm_runtime_disable(&client->dev); > > + if (!pm_runtime_status_suspended(&client->dev)) > > + ov02a10_power_off(&client->dev); > > + pm_runtime_set_suspended(&client->dev); > > + mutex_destroy(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id ov02a10_of_match[] = { > > + { .compatible = "ovti,ov02a10" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, ov02a10_of_match); > > + > > +static struct i2c_driver ov02a10_i2c_driver = { > > + .driver = { > > + .name = "ov02a10", > > + .pm = &ov02a10_pm_ops, > > + .of_match_table = ov02a10_of_match, > > + }, > > + .probe_new = &ov02a10_probe, > > + .remove = &ov02a10_remove, > > +}; > > + > > +module_i2c_driver(ov02a10_i2c_driver); > > + > > +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>"); > > +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver"); > > +MODULE_LICENSE("GPL v2"); > > + >
On Thu, Jun 4, 2020 at 11:26 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Dongchun, > > On Thu, Jun 04, 2020 at 10:14:05AM +0800, Dongchun Zhu wrote: > > Hi Tomasz, Sakari, and sirs, > > > > Could anyone help to review this patch? > > > > On Sat, 2020-05-23 at 16:41 +0800, Dongchun Zhu wrote: > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/media/i2c/Kconfig | 13 + > > > drivers/media/i2c/Makefile | 1 + > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 1040 insertions(+) > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > [snip] > > > > > +static int ov02a10_probe(struct i2c_client *client) > > > +{ > > > + struct device *dev = &client->dev; > > > + struct ov02a10 *ov02a10; > > > + unsigned int rotation; > > > + unsigned int clock_lane_tx_speed; > > > + unsigned int i; > > > + int ret; > > > + > > > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL); > > > + if (!ov02a10) > > > + return -ENOMEM; > > > + > > > + ret = ov02a10_check_hwcfg(dev, ov02a10); > > > + if (ret) { > > > + dev_err(dev, "failed to check HW configuration: %d", ret); > > > + return ret; > > > + } > > > + > > > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops); > > > + ov02a10->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT; > > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10; > > > + > > > + /* Optional indication of physical rotation of sensor */ > > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation); > > > + if (!ret && rotation == 180) { > > > + ov02a10->upside_down = true; > > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > > > + } > > > + > > > + /* Optional indication of mipi TX speed */ > > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed", > > > + &clock_lane_tx_speed); > > > + > > > + if (!ret) > > > + ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed; > > > + > > > + /* Get system clock (eclk) */ > > > + ov02a10->eclk = devm_clk_get(dev, "eclk"); > > > + if (IS_ERR(ov02a10->eclk)) { > > > + ret = PTR_ERR(ov02a10->eclk); > > > + dev_err(dev, "failed to get eclk %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", > > > + &ov02a10->eclk_freq); > > > + if (ret) { > > > + dev_err(dev, "failed to get eclk frequency\n"); > > > + return ret; > > > + } > > > + > > > + ret = clk_set_rate(ov02a10->eclk, ov02a10->eclk_freq); > > > + if (ret) { > > > + dev_err(dev, "failed to set eclk frequency (24MHz)\n"); > > > + return ret; > > > + } > > > + > > > + if (clk_get_rate(ov02a10->eclk) != OV02A10_ECLK_FREQ) { > > > + dev_warn(dev, "wrong eclk frequency %d Hz, expected: %d Hz\n", > > > + ov02a10->eclk_freq, OV02A10_ECLK_FREQ); > > > + return -EINVAL; > > > + } > > > + > > > + ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH); > > > + if (IS_ERR(ov02a10->pd_gpio)) { > > > + ret = PTR_ERR(ov02a10->pd_gpio); > > > + dev_err(dev, "failed to get powerdown-gpios %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > > > + if (IS_ERR(ov02a10->n_rst_gpio)) { > > > + ret = PTR_ERR(ov02a10->n_rst_gpio); > > > + dev_err(dev, "failed to get reset-gpios %d\n", ret); > > > + return ret; > > > + } > > > + > > > + for (i = 0; i < ARRAY_SIZE(ov02a10_supply_names); i++) > > > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > > > + > > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names), > > > + ov02a10->supplies); > > > + if (ret) { > > > + dev_err(dev, "failed to get regulators\n"); > > > + return ret; > > > + } > > > + > > > + mutex_init(&ov02a10->mutex); > > > + ov02a10->cur_mode = &supported_modes[0]; > > > + ret = ov02a10_initialize_controls(ov02a10); > > > + if (ret) { > > > + dev_err(dev, "failed to initialize controls\n"); > > > + goto err_destroy_mutex; > > > + } > > > + > > > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops; > > > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE; > > > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad); > > > + if (ret < 0) { > > > + dev_err(dev, "failed to init entity pads: %d", ret); > > > + goto err_free_handler; > > > + } > > > + > > > + pm_runtime_enable(dev); > > > + if (!pm_runtime_enabled(dev)) { > > > + ret = ov02a10_power_on(dev); > > > + if (ret < 0) { > > > + dev_err(dev, "failed to power on: %d\n", ret); > > > + goto err_free_handler; > > > + } > > > + } > > > + > > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > > + if (ret) { > > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > > + if (!pm_runtime_enabled(dev)) > > > + ov02a10_power_off(dev); > > This should be moved to error handling section below. > > > > + goto err_clean_entity; > > > + } > > > > Tomasz, Sakari, is this ok? > > or coding like this: > > > > ret = v4l2_async_register_subdev(&ov02a10->subdev); > > if (!pm_runtime_enabled(dev)) > > ov02a10_power_off(dev); > > if (ret) { > > dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > goto err_clean_entity; > > } > > > > What's your opinions about the change? > > This turns power off if runtime PM is disabled. I'd keep it as-is, as it'd > require re-implementing what runtime PM is used for now --- and that's not > a sensor driver's job. That and in general I believe the expectations are: - runtime PM enabled - powered on only when it has something to do - runtime PM disabled - powered on when the driver is bound (probe succeeded), powered off when the driver unbinds (remove or probe error) Best regards, Tomasz > > > > > > + > > > + return 0; > > > + > > > +err_clean_entity: > > > + media_entity_cleanup(&ov02a10->subdev.entity); > > > +err_free_handler: > > > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > > > +err_destroy_mutex: > > > + mutex_destroy(&ov02a10->mutex); > > > + > > > + return ret; > > > +} > > > + > > > +static int ov02a10_remove(struct i2c_client *client) > > > +{ > > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > + > > > + v4l2_async_unregister_subdev(sd); > > > + media_entity_cleanup(&sd->entity); > > > + v4l2_ctrl_handler_free(sd->ctrl_handler); > > > + pm_runtime_disable(&client->dev); > > > + if (!pm_runtime_status_suspended(&client->dev)) > > > + ov02a10_power_off(&client->dev); > > > + pm_runtime_set_suspended(&client->dev); > > > + mutex_destroy(&ov02a10->mutex); > > > + > > > + return 0; > > > +} > > > + > > > +static const struct of_device_id ov02a10_of_match[] = { > > > + { .compatible = "ovti,ov02a10" }, > > > + {} > > > +}; > > > +MODULE_DEVICE_TABLE(of, ov02a10_of_match); > > > + > > > +static struct i2c_driver ov02a10_i2c_driver = { > > > + .driver = { > > > + .name = "ov02a10", > > > + .pm = &ov02a10_pm_ops, > > > + .of_match_table = ov02a10_of_match, > > > + }, > > > + .probe_new = &ov02a10_probe, > > > + .remove = &ov02a10_remove, > > > +}; > > > + > > > +module_i2c_driver(ov02a10_i2c_driver); > > > + > > > +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>"); > > > +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver"); > > > +MODULE_LICENSE("GPL v2"); > > > + > > > > -- > Sakari Ailus
Hi Sakari, On Thu, 2020-06-04 at 12:26 +0300, Sakari Ailus wrote: > Hi Dongchun, > > On Thu, Jun 04, 2020 at 10:14:05AM +0800, Dongchun Zhu wrote: > > Hi Tomasz, Sakari, and sirs, > > > > Could anyone help to review this patch? > > > > On Sat, 2020-05-23 at 16:41 +0800, Dongchun Zhu wrote: > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/media/i2c/Kconfig | 13 + > > > drivers/media/i2c/Makefile | 1 + > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 1040 insertions(+) > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > [snip] > > > > > +static int ov02a10_probe(struct i2c_client *client) > > > +{ > > > + struct device *dev = &client->dev; > > > + struct ov02a10 *ov02a10; > > > + unsigned int rotation; > > > + unsigned int clock_lane_tx_speed; > > > + unsigned int i; > > > + int ret; > > > + > > > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL); > > > + if (!ov02a10) > > > + return -ENOMEM; > > > + > > > + ret = ov02a10_check_hwcfg(dev, ov02a10); > > > + if (ret) { > > > + dev_err(dev, "failed to check HW configuration: %d", ret); > > > + return ret; > > > + } > > > + > > > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops); > > > + ov02a10->mipi_clock_tx_speed = OV02A10_MIPI_TX_SPEED_DEFAULT; > > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10; > > > + > > > + /* Optional indication of physical rotation of sensor */ > > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation); > > > + if (!ret && rotation == 180) { > > > + ov02a10->upside_down = true; > > > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10; > > > + } > > > + > > > + /* Optional indication of mipi TX speed */ > > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed", > > > + &clock_lane_tx_speed); > > > + > > > + if (!ret) > > > + ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed; > > > + > > > + /* Get system clock (eclk) */ > > > + ov02a10->eclk = devm_clk_get(dev, "eclk"); > > > + if (IS_ERR(ov02a10->eclk)) { > > > + ret = PTR_ERR(ov02a10->eclk); > > > + dev_err(dev, "failed to get eclk %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", > > > + &ov02a10->eclk_freq); > > > + if (ret) { > > > + dev_err(dev, "failed to get eclk frequency\n"); > > > + return ret; > > > + } > > > + > > > + ret = clk_set_rate(ov02a10->eclk, ov02a10->eclk_freq); > > > + if (ret) { > > > + dev_err(dev, "failed to set eclk frequency (24MHz)\n"); > > > + return ret; > > > + } > > > + > > > + if (clk_get_rate(ov02a10->eclk) != OV02A10_ECLK_FREQ) { > > > + dev_warn(dev, "wrong eclk frequency %d Hz, expected: %d Hz\n", > > > + ov02a10->eclk_freq, OV02A10_ECLK_FREQ); > > > + return -EINVAL; > > > + } > > > + > > > + ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH); > > > + if (IS_ERR(ov02a10->pd_gpio)) { > > > + ret = PTR_ERR(ov02a10->pd_gpio); > > > + dev_err(dev, "failed to get powerdown-gpios %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > > > + if (IS_ERR(ov02a10->n_rst_gpio)) { > > > + ret = PTR_ERR(ov02a10->n_rst_gpio); > > > + dev_err(dev, "failed to get reset-gpios %d\n", ret); > > > + return ret; > > > + } > > > + > > > + for (i = 0; i < ARRAY_SIZE(ov02a10_supply_names); i++) > > > + ov02a10->supplies[i].supply = ov02a10_supply_names[i]; > > > + > > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov02a10_supply_names), > > > + ov02a10->supplies); > > > + if (ret) { > > > + dev_err(dev, "failed to get regulators\n"); > > > + return ret; > > > + } > > > + > > > + mutex_init(&ov02a10->mutex); > > > + ov02a10->cur_mode = &supported_modes[0]; > > > + ret = ov02a10_initialize_controls(ov02a10); > > > + if (ret) { > > > + dev_err(dev, "failed to initialize controls\n"); > > > + goto err_destroy_mutex; > > > + } > > > + > > > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops; > > > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE; > > > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad); > > > + if (ret < 0) { > > > + dev_err(dev, "failed to init entity pads: %d", ret); > > > + goto err_free_handler; > > > + } > > > + > > > + pm_runtime_enable(dev); > > > + if (!pm_runtime_enabled(dev)) { > > > + ret = ov02a10_power_on(dev); > > > + if (ret < 0) { > > > + dev_err(dev, "failed to power on: %d\n", ret); > > > + goto err_free_handler; This is actually wrong, which should be replaced of "err_clean_entity". > > > + } > > > + } > > > + > > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > > + if (ret) { > > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > > + if (!pm_runtime_enabled(dev)) > > > + ov02a10_power_off(dev); > > This should be moved to error handling section below. > Fine. It would be abstracted as "err_async_register" in next release. Something like: err_async_register: if (!pm_runtime_enabled(dev)) ov02a10_power_off(dev); err_clean_entity: media_entity_cleanup(&ov02a10->subdev.entity); ... > > > + goto err_clean_entity; > > > + } > > > > Tomasz, Sakari, is this ok? > > or coding like this: > > > > ret = v4l2_async_register_subdev(&ov02a10->subdev); > > if (!pm_runtime_enabled(dev)) > > ov02a10_power_off(dev); > > if (ret) { > > dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > goto err_clean_entity; > > } > > > > What's your opinions about the change? > > This turns power off if runtime PM is disabled. I'd keep it as-is, as it'd > require re-implementing what runtime PM is used for now --- and that's not > a sensor driver's job. > > > > > > + > > > + return 0; > > > + > > > +err_clean_entity: > > > + media_entity_cleanup(&ov02a10->subdev.entity); > > > +err_free_handler: > > > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > > > +err_destroy_mutex: > > > + mutex_destroy(&ov02a10->mutex); > > > + > > > + return ret; > > > +} > > > + > > > +static int ov02a10_remove(struct i2c_client *client) > > > +{ > > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > + > > > + v4l2_async_unregister_subdev(sd); > > > + media_entity_cleanup(&sd->entity); > > > + v4l2_ctrl_handler_free(sd->ctrl_handler); > > > + pm_runtime_disable(&client->dev); > > > + if (!pm_runtime_status_suspended(&client->dev)) > > > + ov02a10_power_off(&client->dev); > > > + pm_runtime_set_suspended(&client->dev); > > > + mutex_destroy(&ov02a10->mutex); > > > + > > > + return 0; > > > +} > > > + > > > +static const struct of_device_id ov02a10_of_match[] = { > > > + { .compatible = "ovti,ov02a10" }, > > > + {} > > > +}; > > > +MODULE_DEVICE_TABLE(of, ov02a10_of_match); > > > + > > > +static struct i2c_driver ov02a10_i2c_driver = { > > > + .driver = { > > > + .name = "ov02a10", > > > + .pm = &ov02a10_pm_ops, > > > + .of_match_table = ov02a10_of_match, > > > + }, > > > + .probe_new = &ov02a10_probe, > > > + .remove = &ov02a10_remove, > > > +}; > > > + > > > +module_i2c_driver(ov02a10_i2c_driver); > > > + > > > +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>"); > > > +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver"); > > > +MODULE_LICENSE("GPL v2"); > > > + > > >
Hi Dongchun, On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > Add a V4L2 sub-device driver for OV02A10 image sensor. > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > --- > MAINTAINERS | 1 + > drivers/media/i2c/Kconfig | 13 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1040 insertions(+) > create mode 100644 drivers/media/i2c/ov02a10.c > Thank you for the patch. Please see my comments inline. [snip] > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > new file mode 100644 > index 0000000..160a0b5 > --- /dev/null > +++ b/drivers/media/i2c/ov02a10.c [snip] > +static const char * const ov02a10_test_pattern_menu[] = { > + "Disabled", > + "Color Bar", nit: We should normalize this to one of the standard names. What is the pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > +}; [snip] > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > + > + mutex_lock(&ov02a10->mutex); > + Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, which is used for trying the format, but not applying it to the hardware? > + if (ov02a10->streaming) { > + mutex_unlock(&ov02a10->mutex); > + return -EBUSY; > + } > + > + /* Only one sensor mode supported */ > + mbus_fmt->code = ov02a10->fmt.code; > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > + ov02a10->fmt = fmt->format; > + > + mutex_unlock(&ov02a10->mutex); > + > + return 0; > +} > + > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > + > + mutex_lock(&ov02a10->mutex); > + > + fmt->format = ov02a10->fmt; Ditto. > + mbus_fmt->code = ov02a10->fmt.code; > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > + > + mutex_unlock(&ov02a10->mutex); > + > + return 0; > +} > + > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > + > + if (code->index >= ARRAY_SIZE(supported_modes)) > + return -EINVAL; Hmm, supported_modes[] doesn't seem to hold the information about mbus codes. Should this just perhaps be "!= 0"? > + > + code->code = ov02a10->fmt.code; > + > + return 0; > +} [snip] > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg) > +{ > + struct v4l2_subdev_format fmt = { > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > + .format = { > + .width = 1600, > + .height = 1200, > + } > + }; > + > + ov02a10_set_fmt(sd, cfg, &fmt); > + > + return 0; > +} > + I'm not familiar with this init_cfg operation and the documentation is very sparse about it. Sakari, is this a correct implementation? [snip] > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + int ret; > + > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE); > + if (ret < 0) > + return ret; > + > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_TEST_PATTERN, > + pattern); > + if (ret < 0) > + return ret; > + > + ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + > + return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE, > + SC_CTRL_MODE_STREAMING); Why is this needed? Does writing the test pattern register stop streaming? [snip] > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + const struct ov02a10_mode *mode; > + struct v4l2_ctrl_handler *handler; > + struct v4l2_ctrl *ctrl; > + u64 exposure_max; > + u32 pixel_rate; > + int ret; > + > + handler = &ov02a10->ctrl_handler; > + mode = ov02a10->cur_mode; > + ret = v4l2_ctrl_handler_init(handler, 7); > + if (ret) > + return ret; > + > + handler->lock = &ov02a10->mutex; > + > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0, > + link_freq_menu_items); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + pixel_rate = to_pixel_rate(0); > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1, > + pixel_rate); > + > + exposure_max = mode->vts_def - 4; > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > + V4L2_CID_EXPOSURE, > + OV02A10_EXPOSURE_MIN, > + exposure_max, > + OV02A10_EXPOSURE_STEP, > + mode->exp_def); > + > + v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > + V4L2_CID_ANALOGUE_GAIN, > + OV02A10_GAIN_MIN, > + OV02A10_GAIN_MAX, > + OV02A10_GAIN_STEP, > + OV02A10_GAIN_DEFAULT); > + > + v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(ov02a10_test_pattern_menu) - 1, > + 0, 0, ov02a10_test_pattern_menu); > + I can see that we're missing some controls here now, VBLANK and HBLANK if I remember correctly. Even though read-only, some userspace need those to get information about how the sensor operates. > + if (handler->error) { > + ret = handler->error; > + dev_err(&client->dev, "failed to init controls(%d)\n", ret); > + goto err_free_handler; > + } > + > + ov02a10->subdev.ctrl_handler = handler; > + > + return 0; > + > +err_free_handler: > + v4l2_ctrl_handler_free(handler); > + > + return ret; > +} [snip] > + pm_runtime_enable(dev); > + if (!pm_runtime_enabled(dev)) { > + ret = ov02a10_power_on(dev); > + if (ret < 0) { > + dev_err(dev, "failed to power on: %d\n", ret); > + goto err_free_handler; > + } > + } > + > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > + if (ret) { > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > + if (!pm_runtime_enabled(dev)) > + ov02a10_power_off(dev); Please don't mix inline and error-path error handling, as it makes it difficult to tell if it's correct. Please move this below the appropriate err label instead. > + goto err_clean_entity; > + } > + > + return 0; > + > +err_clean_entity: If one calls pm_runtime_enable() in the probe path, one needs to call pm_runtime_disable() on the error and remove paths. > + media_entity_cleanup(&ov02a10->subdev.entity); > +err_free_handler: > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > +err_destroy_mutex: > + mutex_destroy(&ov02a10->mutex); > + > + return ret; > +} > + Best regards, Tomasz
Hi Tomasz, On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > Hi Dongchun, > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 13 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1040 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > Thank you for the patch. Please see my comments inline. > > [snip] > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > new file mode 100644 > > index 0000000..160a0b5 > > --- /dev/null > > +++ b/drivers/media/i2c/ov02a10.c > [snip] > > +static const char * const ov02a10_test_pattern_menu[] = { > > + "Disabled", > > + "Color Bar", > > nit: We should normalize this to one of the standard names. What is the > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > +}; > [snip] > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > which is used for trying the format, but not applying it to the hardware? Yes. > > > + if (ov02a10->streaming) { > > + mutex_unlock(&ov02a10->mutex); > > + return -EBUSY; > > + } > > + > > + /* Only one sensor mode supported */ > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + ov02a10->fmt = fmt->format; > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + fmt->format = ov02a10->fmt; > > Ditto. > > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > + return -EINVAL; > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > codes. Should this just perhaps be "!= 0"? > > > + > > + code->code = ov02a10->fmt.code; > > + > > + return 0; > > +} > [snip] > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg) > > +{ > > + struct v4l2_subdev_format fmt = { > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > + .format = { > > + .width = 1600, > > + .height = 1200, > > + } > > + }; > > + > > + ov02a10_set_fmt(sd, cfg, &fmt); > > + > > + return 0; > > +} > > + > > I'm not familiar with this init_cfg operation and the documentation is very > sparse about it. Sakari, is this a correct implementation? The purpose is to initialise a pad configuration (format and selection rectangles) to the device defaults. As there seem to be no selection rectangles, this seems fine to me.
On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Tomasz, > > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > > Hi Dongchun, > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/media/i2c/Kconfig | 13 + > > > drivers/media/i2c/Makefile | 1 + > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 1040 insertions(+) > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > Thank you for the patch. Please see my comments inline. > > > > [snip] > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > new file mode 100644 > > > index 0000000..160a0b5 > > > --- /dev/null > > > +++ b/drivers/media/i2c/ov02a10.c > > [snip] > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > + "Disabled", > > > + "Color Bar", > > > > nit: We should normalize this to one of the standard names. What is the > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > +}; > > [snip] > > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_pad_config *cfg, > > > + struct v4l2_subdev_format *fmt) > > > +{ > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > + > > > + mutex_lock(&ov02a10->mutex); > > > + > > > > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > > which is used for trying the format, but not applying it to the hardware? > > Yes. > > > > > > + if (ov02a10->streaming) { > > > + mutex_unlock(&ov02a10->mutex); > > > + return -EBUSY; > > > + } > > > + > > > + /* Only one sensor mode supported */ > > > + mbus_fmt->code = ov02a10->fmt.code; > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > + ov02a10->fmt = fmt->format; > > > + > > > + mutex_unlock(&ov02a10->mutex); > > > + > > > + return 0; > > > +} > > > + > > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_pad_config *cfg, > > > + struct v4l2_subdev_format *fmt) > > > +{ > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > + > > > + mutex_lock(&ov02a10->mutex); > > > + > > > + fmt->format = ov02a10->fmt; > > > > Ditto. > > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > + > > > + mutex_unlock(&ov02a10->mutex); > > > + > > > + return 0; > > > +} > > > + > > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_pad_config *cfg, > > > + struct v4l2_subdev_mbus_code_enum *code) > > > +{ > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > + > > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > > + return -EINVAL; > > > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > > codes. Should this just perhaps be "!= 0"? > > > > > + > > > + code->code = ov02a10->fmt.code; > > > + > > > + return 0; > > > +} > > [snip] > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_pad_config *cfg) > > > +{ > > > + struct v4l2_subdev_format fmt = { > > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > > + .format = { > > > + .width = 1600, > > > + .height = 1200, > > > + } > > > + }; > > > + > > > + ov02a10_set_fmt(sd, cfg, &fmt); > > > + > > > + return 0; > > > +} > > > + > > > > I'm not familiar with this init_cfg operation and the documentation is very > > sparse about it. Sakari, is this a correct implementation? > > The purpose is to initialise a pad configuration (format and selection > rectangles) to the device defaults. As there seem to be no selection > rectangles, this seems fine to me. Thanks. I traced the code and could only see one place where the callback is being called and that was with cfg != NULL. Still, the code above uses "cfg ?" as a check to determine whether TRY or ACTIVE should be passed to which. Is that also correct? Best regards, Tomasz
On Thu, Jun 11, 2020 at 11:57:43AM +0200, Tomasz Figa wrote: > On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Tomasz, > > > > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/media/i2c/Kconfig | 13 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 1040 insertions(+) > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > [snip] > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > new file mode 100644 > > > > index 0000000..160a0b5 > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/ov02a10.c > > > [snip] > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > + "Disabled", > > > > + "Color Bar", > > > > > > nit: We should normalize this to one of the standard names. What is the > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > +}; > > > [snip] > > > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_format *fmt) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > + > > > > + mutex_lock(&ov02a10->mutex); > > > > + > > > > > > > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > > > which is used for trying the format, but not applying it to the hardware? > > > > Yes. > > > > > > > > > + if (ov02a10->streaming) { > > > > + mutex_unlock(&ov02a10->mutex); > > > > + return -EBUSY; > > > > + } > > > > + > > > > + /* Only one sensor mode supported */ > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > + ov02a10->fmt = fmt->format; > > > > + > > > > + mutex_unlock(&ov02a10->mutex); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_format *fmt) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > + > > > > + mutex_lock(&ov02a10->mutex); > > > > + > > > > + fmt->format = ov02a10->fmt; > > > > > > Ditto. > > > > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > + > > > > + mutex_unlock(&ov02a10->mutex); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg, > > > > + struct v4l2_subdev_mbus_code_enum *code) > > > > +{ > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > + > > > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > > > + return -EINVAL; > > > > > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > > > codes. Should this just perhaps be "!= 0"? > > > > > > > + > > > > + code->code = ov02a10->fmt.code; > > > > + > > > > + return 0; > > > > +} > > > [snip] > > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_pad_config *cfg) > > > > +{ > > > > + struct v4l2_subdev_format fmt = { > > > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > > > + .format = { > > > > + .width = 1600, > > > > + .height = 1200, > > > > + } > > > > + }; > > > > + > > > > + ov02a10_set_fmt(sd, cfg, &fmt); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > I'm not familiar with this init_cfg operation and the documentation is very > > > sparse about it. Sakari, is this a correct implementation? > > > > The purpose is to initialise a pad configuration (format and selection > > rectangles) to the device defaults. As there seem to be no selection > > rectangles, this seems fine to me. > > Thanks. I traced the code and could only see one place where the > callback is being called and that was with cfg != NULL. Still, the > code above uses "cfg ?" as a check to determine whether TRY or ACTIVE > should be passed to which. Is that also correct? It could be used in setting the active format in probe. That would probably be cleaner than what it currently does. But apart from that, the framework always calls init_cfg with cfg non-NULL.
Hi Tomasz, On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > Hi Dongchun, > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > --- > > MAINTAINERS | 1 + > > drivers/media/i2c/Kconfig | 13 + > > drivers/media/i2c/Makefile | 1 + > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 1040 insertions(+) > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > Thank you for the patch. Please see my comments inline. > > [snip] > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > new file mode 100644 > > index 0000000..160a0b5 > > --- /dev/null > > +++ b/drivers/media/i2c/ov02a10.c > [snip] > > +static const char * const ov02a10_test_pattern_menu[] = { > > + "Disabled", > > + "Color Bar", > > nit: We should normalize this to one of the standard names. What is the > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > Yes. It is one kind of 'Eight Vertical Colour Bars'. This pattern is called as 'MIPI color bar' per the datasheet. Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > > +}; > [snip] > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > which is used for trying the format, but not applying it to the hardware? > Got it :-) > > + if (ov02a10->streaming) { > > + mutex_unlock(&ov02a10->mutex); > > + return -EBUSY; > > + } > > + > > + /* Only one sensor mode supported */ > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + ov02a10->fmt = fmt->format; > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > + > > + mutex_lock(&ov02a10->mutex); > > + > > + fmt->format = ov02a10->fmt; > > Ditto. > > > + mbus_fmt->code = ov02a10->fmt.code; > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > + > > + mutex_unlock(&ov02a10->mutex); > > + > > + return 0; > > +} > > + > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > + > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > + return -EINVAL; > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > codes. Should this just perhaps be "!= 0"? > Understood. > > + > > + code->code = ov02a10->fmt.code; > > + > > + return 0; > > +} > [snip] > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg) > > +{ > > + struct v4l2_subdev_format fmt = { > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > + .format = { > > + .width = 1600, > > + .height = 1200, > > + } > > + }; > > + > > + ov02a10_set_fmt(sd, cfg, &fmt); > > + > > + return 0; > > +} > > + > > I'm not familiar with this init_cfg operation and the documentation is very > sparse about it. Sakari, is this a correct implementation? > > [snip] > > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + int ret; > > + > > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_TEST_PATTERN, > > + pattern); > > + if (ret < 0) > > + return ret; > > + > > + ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + > > + return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE, > > + SC_CTRL_MODE_STREAMING); > > Why is this needed? Does writing the test pattern register stop streaming? > Looking back to the setting history, I found it was suggested by OV. I would leave your question to OV, and update their feedback. > [snip] > > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + const struct ov02a10_mode *mode; > > + struct v4l2_ctrl_handler *handler; > > + struct v4l2_ctrl *ctrl; > > + u64 exposure_max; > > + u32 pixel_rate; > > + int ret; > > + > > + handler = &ov02a10->ctrl_handler; > > + mode = ov02a10->cur_mode; > > + ret = v4l2_ctrl_handler_init(handler, 7); > > + if (ret) > > + return ret; > > + > > + handler->lock = &ov02a10->mutex; > > + > > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0, > > + link_freq_menu_items); > > + if (ctrl) > > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > + > > + pixel_rate = to_pixel_rate(0); > > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1, > > + pixel_rate); > > + > > + exposure_max = mode->vts_def - 4; > > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_EXPOSURE, > > + OV02A10_EXPOSURE_MIN, > > + exposure_max, > > + OV02A10_EXPOSURE_STEP, > > + mode->exp_def); > > + > > + v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_ANALOGUE_GAIN, > > + OV02A10_GAIN_MIN, > > + OV02A10_GAIN_MAX, > > + OV02A10_GAIN_STEP, > > + OV02A10_GAIN_DEFAULT); > > + > > + v4l2_ctrl_new_std_menu_items(handler, &ov02a10_ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(ov02a10_test_pattern_menu) - 1, > > + 0, 0, ov02a10_test_pattern_menu); > > + > > I can see that we're missing some controls here now, VBLANK and HBLANK if I > remember correctly. Even though read-only, some userspace need those to > get information about how the sensor operates. > Yes. I made a mistake. > > + if (handler->error) { > > + ret = handler->error; > > + dev_err(&client->dev, "failed to init controls(%d)\n", ret); > > + goto err_free_handler; > > + } > > + > > + ov02a10->subdev.ctrl_handler = handler; > > + > > + return 0; > > + > > +err_free_handler: > > + v4l2_ctrl_handler_free(handler); > > + > > + return ret; > > +} > [snip] > > + pm_runtime_enable(dev); > > + if (!pm_runtime_enabled(dev)) { > > + ret = ov02a10_power_on(dev); > > + if (ret < 0) { > > + dev_err(dev, "failed to power on: %d\n", ret); > > + goto err_free_handler; > > + } > > + } > > + > > + ret = v4l2_async_register_subdev(&ov02a10->subdev); > > + if (ret) { > > + dev_err(dev, "failed to register V4L2 subdev: %d", ret); > > + if (!pm_runtime_enabled(dev)) > > + ov02a10_power_off(dev); > > Please don't mix inline and error-path error handling, as it makes it > difficult to tell if it's correct. Please move this below the appropriate > err label instead. > Fixed in next release. > > + goto err_clean_entity; > > + } > > + > > + return 0; > > + > > +err_clean_entity: > > If one calls pm_runtime_enable() in the probe path, one needs to call > pm_runtime_disable() on the error and remove paths. > Yes, fixed in next release. > > + media_entity_cleanup(&ov02a10->subdev.entity); > > +err_free_handler: > > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler); > > +err_destroy_mutex: > > + mutex_destroy(&ov02a10->mutex); > > + > > + return ret; > > +} > > + > > Best regards, > Tomasz
Hi Sakari, On Thu, 2020-06-11 at 13:03 +0300, Sakari Ailus wrote: > On Thu, Jun 11, 2020 at 11:57:43AM +0200, Tomasz Figa wrote: > > On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Tomasz, > > > > > > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote: > > > > Hi Dongchun, > > > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > --- > > > > > MAINTAINERS | 1 + > > > > > drivers/media/i2c/Kconfig | 13 + > > > > > drivers/media/i2c/Makefile | 1 + > > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 1040 insertions(+) > > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > > > [snip] > > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > > new file mode 100644 > > > > > index 0000000..160a0b5 > > > > > --- /dev/null > > > > > +++ b/drivers/media/i2c/ov02a10.c > > > > [snip] > > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > > + "Disabled", > > > > > + "Color Bar", > > > > > > > > nit: We should normalize this to one of the standard names. What is the > > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > > > +}; > > > > [snip] > > > > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd, > > > > > + struct v4l2_subdev_pad_config *cfg, > > > > > + struct v4l2_subdev_format *fmt) > > > > > +{ > > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > > + > > > > > + mutex_lock(&ov02a10->mutex); > > > > > + > > > > > > > > > > > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY, > > > > which is used for trying the format, but not applying it to the hardware? > > > > > > Yes. > > > > > > > > > > > > + if (ov02a10->streaming) { > > > > > + mutex_unlock(&ov02a10->mutex); > > > > > + return -EBUSY; > > > > > + } > > > > > + > > > > > + /* Only one sensor mode supported */ > > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > > + ov02a10->fmt = fmt->format; > > > > > + > > > > > + mutex_unlock(&ov02a10->mutex); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd, > > > > > + struct v4l2_subdev_pad_config *cfg, > > > > > + struct v4l2_subdev_format *fmt) > > > > > +{ > > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > > > > > + > > > > > + mutex_lock(&ov02a10->mutex); > > > > > + > > > > > + fmt->format = ov02a10->fmt; > > > > > > > > Ditto. > > > > > > > > > + mbus_fmt->code = ov02a10->fmt.code; > > > > > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt); > > > > > + > > > > > + mutex_unlock(&ov02a10->mutex); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd, > > > > > + struct v4l2_subdev_pad_config *cfg, > > > > > + struct v4l2_subdev_mbus_code_enum *code) > > > > > +{ > > > > > + struct ov02a10 *ov02a10 = to_ov02a10(sd); > > > > > + > > > > > + if (code->index >= ARRAY_SIZE(supported_modes)) > > > > > + return -EINVAL; > > > > > > > > Hmm, supported_modes[] doesn't seem to hold the information about mbus > > > > codes. Should this just perhaps be "!= 0"? > > > > > > > > > + > > > > > + code->code = ov02a10->fmt.code; > > > > > + > > > > > + return 0; > > > > > +} > > > > [snip] > > > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd, > > > > > + struct v4l2_subdev_pad_config *cfg) > > > > > +{ > > > > > + struct v4l2_subdev_format fmt = { > > > > > + .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE, > > > > > + .format = { > > > > > + .width = 1600, > > > > > + .height = 1200, > > > > > + } > > > > > + }; > > > > > + > > > > > + ov02a10_set_fmt(sd, cfg, &fmt); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > > > > I'm not familiar with this init_cfg operation and the documentation is very > > > > sparse about it. Sakari, is this a correct implementation? > > > > > > The purpose is to initialise a pad configuration (format and selection > > > rectangles) to the device defaults. As there seem to be no selection > > > rectangles, this seems fine to me. > > > > Thanks. I traced the code and could only see one place where the > > callback is being called and that was with cfg != NULL. Still, the > > code above uses "cfg ?" as a check to determine whether TRY or ACTIVE > > should be passed to which. Is that also correct? > > It could be used in setting the active format in probe. That would probably > be cleaner than what it currently does. > Sakari, did you mean that we need to update _probe API? Like this: struct v4l2_subdev_format fmt = { .which = V4L2_SUBDEV_FORMAT_ACTIVE, }; > But apart from that, the framework always calls init_cfg with cfg non-NULL. >
On Fri, Jun 12, 2020 at 12:49 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > Hi Tomasz, > > On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > > Hi Dongchun, > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/media/i2c/Kconfig | 13 + > > > drivers/media/i2c/Makefile | 1 + > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 1040 insertions(+) > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > Thank you for the patch. Please see my comments inline. > > > > [snip] > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > new file mode 100644 > > > index 0000000..160a0b5 > > > --- /dev/null > > > +++ b/drivers/media/i2c/ov02a10.c > > [snip] > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > + "Disabled", > > > + "Color Bar", > > > > nit: We should normalize this to one of the standard names. What is the > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > Yes. It is one kind of 'Eight Vertical Colour Bars'. > This pattern is called as 'MIPI color bar' per the datasheet. > Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > We should try to stick to the names as exposed by existing drivers. There was an attempt to unify the naming of some Sony sensors some time ago [1]. Perhaps one of the names there matches the pattern of this sensor? [1] https://patchwork.kernel.org/patch/10711777/ Best regards, Tomasz
Hi Tomasz, On Fri, 2020-06-12 at 20:39 +0200, Tomasz Figa wrote: > On Fri, Jun 12, 2020 at 12:49 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > Hi Tomasz, > > > > On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > > > Hi Dongchun, > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/media/i2c/Kconfig | 13 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 1040 insertions(+) > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > [snip] > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > new file mode 100644 > > > > index 0000000..160a0b5 > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/ov02a10.c > > > [snip] > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > + "Disabled", > > > > + "Color Bar", > > > > > > nit: We should normalize this to one of the standard names. What is the > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > Yes. It is one kind of 'Eight Vertical Colour Bars'. > > This pattern is called as 'MIPI color bar' per the datasheet. > > Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > > > > We should try to stick to the names as exposed by existing drivers. > There was an attempt to unify the naming of some Sony sensors some > time ago [1]. Perhaps one of the names there matches the pattern of > this sensor? > > [1] https://patchwork.kernel.org/patch/10711777/ > Sounds great. It is one good idea to summarize test patterns from various sensors. But one question plaguing me is that it seems even for the same "Eight Vertical Colour Bars", different sensors may have different RGB color map. Moreover, definition standards of color bar style may differ among different sensor chip vendors. For instance, Sony often uses "Solid Color", "Color Bars With Fade to Grey", "PN9" to abstract test pattern output type; while OmniVision adopts color bar type 1, 2, 3, 4 or "MIPI Color Bar", "ISP Color Bar" instead. > Best regards, > Tomasz
Hi Dongchun, On Mon, Jun 15, 2020 at 7:57 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > Hi Tomasz, > > On Fri, 2020-06-12 at 20:39 +0200, Tomasz Figa wrote: > > On Fri, Jun 12, 2020 at 12:49 PM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote: > > > > > > Hi Tomasz, > > > > > > On Wed, 2020-06-10 at 19:44 +0000, Tomasz Figa wrote: > > > > Hi Dongchun, > > > > > > > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote: > > > > > Add a V4L2 sub-device driver for OV02A10 image sensor. > > > > > > > > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com> > > > > > --- > > > > > MAINTAINERS | 1 + > > > > > drivers/media/i2c/Kconfig | 13 + > > > > > drivers/media/i2c/Makefile | 1 + > > > > > drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 1040 insertions(+) > > > > > create mode 100644 drivers/media/i2c/ov02a10.c > > > > > > > > > > > > > Thank you for the patch. Please see my comments inline. > > > > > > > > [snip] > > > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c > > > > > new file mode 100644 > > > > > index 0000000..160a0b5 > > > > > --- /dev/null > > > > > +++ b/drivers/media/i2c/ov02a10.c > > > > [snip] > > > > > +static const char * const ov02a10_test_pattern_menu[] = { > > > > > + "Disabled", > > > > > + "Color Bar", > > > > > > > > nit: We should normalize this to one of the standard names. What is the > > > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"? > > > > > > > > > > Yes. It is one kind of 'Eight Vertical Colour Bars'. > > > This pattern is called as 'MIPI color bar' per the datasheet. > > > Can we here use 'Vertical Color Bar' or 'MIPI Color Bar'? > > > > > > > We should try to stick to the names as exposed by existing drivers. > > There was an attempt to unify the naming of some Sony sensors some > > time ago [1]. Perhaps one of the names there matches the pattern of > > this sensor? > > > > [1] https://patchwork.kernel.org/patch/10711777/ > > > > Sounds great. > It is one good idea to summarize test patterns from various sensors. > But one question plaguing me is that it seems even for the same "Eight > Vertical Colour Bars", different sensors may have different RGB color > map. > > Moreover, definition standards of color bar style may differ among > different sensor chip vendors. > For instance, Sony often uses "Solid Color", "Color Bars With Fade to > Grey", "PN9" to abstract test pattern output type; while OmniVision > adopts color bar type 1, 2, 3, 4 or "MIPI Color Bar", "ISP Color Bar" > instead. The test patterns are commonly very similar, just different vendors call them with different names. There is actually a specification called MIPI CCS [1], which standardizes many aspects of sensor programming, including test patterns. Most of the sensors don't really follow the entire specification yet, but often some aspects happen to match. In the copy of the specification that I have, the test patterns are described in section 10 (Test modes), page 203, with pictures on following pages. Perhaps we could standardize the names based on that, when the pattern matches? Sakari, what do you think? [1] http://resources.mipi.org/camera-command-set-v1-1-download Best regards, Tomasz