Hi Sakari
On 21/09/2021 10:11, Sakari Ailus wrote:
> Hi Daniel,
>
> Thanks for the patch.
>
> On Mon, Sep 20, 2021 at 11:54:22PM +0100, Daniel Scally wrote:
>> The OV5693 is a 5 Mpx CMOS image sensor, connected via MIPI CSI-2. The
>> chip is capable of a single lane configuration, but currently only two
>> lanes are supported.
>>
>> Most of the sensor's features are supported, with the main exception
>> being the lens correction algorithm.
>>
>> The driver provides all mandatory, optional and recommended V4L2 controls
>> for maximum compatibility with libcamera.
>>
>> Signed-off-by: Daniel Scally <djrscally@xxxxxxxxx>
>> ---
>> MAINTAINERS | 7 +
>> drivers/media/i2c/Kconfig | 11 +
>> drivers/media/i2c/Makefile | 1 +
>> drivers/media/i2c/ov5693.c | 1558 ++++++++++++++++++++++++++++++++++++
>> 4 files changed, 1577 insertions(+)
>> create mode 100644 drivers/media/i2c/ov5693.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ca6d6fde85cf..83b4523ef899 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -13855,6 +13855,13 @@ S: Maintained
>> T: git git://linuxtv.org/media_tree.git
>> F: drivers/media/i2c/ov5675.c
>>
>> +OMNIVISION OV5693 SENSOR DRIVER
>> +M: Daniel Scally <djrscally@xxxxxxxxx>
>> +L: linux-media@xxxxxxxxxxxxxxx
>> +S: Maintained
>> +T: git git://linuxtv.org/media_tree.git
>> +F: drivers/media/i2c/ov5693.c
>> +
>> OMNIVISION OV5695 SENSOR DRIVER
>> M: Shunqian Zheng <zhengsq@xxxxxxxxxxxxxx>
>> L: linux-media@xxxxxxxxxxxxxxx
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 08feb3e8c1bf..82d5c9b7f540 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -1042,6 +1042,17 @@ config VIDEO_OV5675
>> To compile this driver as a module, choose M here: the
>> module will be called ov5675.
>>
>> +config VIDEO_OV5693
>> + tristate "OmniVision OV5693 sensor support"
>> + depends on I2C && VIDEO_V4L2
>> + select V4L2_FWNODE
>> + help
>> + This is a Video4Linux2 sensor driver for the OmniVision
>> + OV5693 camera.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called ov5693.
>> +
>> config VIDEO_OV5695
>> tristate "OmniVision OV5695 sensor support"
>> depends on I2C && VIDEO_V4L2
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index 83268f20aa3a..6b910ba2dde2 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -75,6 +75,7 @@ obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
>> obj-$(CONFIG_VIDEO_OV5648) += ov5648.o
>> obj-$(CONFIG_VIDEO_OV5670) += ov5670.o
>> obj-$(CONFIG_VIDEO_OV5675) += ov5675.o
>> +obj-$(CONFIG_VIDEO_OV5693) += ov5693.o
>> obj-$(CONFIG_VIDEO_OV5695) += ov5695.o
>> obj-$(CONFIG_VIDEO_OV6650) += ov6650.o
>> obj-$(CONFIG_VIDEO_OV7251) += ov7251.o
>> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
>> new file mode 100644
>> index 000000000000..1e5c465112c5
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov5693.c
>> @@ -0,0 +1,1558 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2013 Intel Corporation. All Rights Reserved.
> Are there any other copyrights? I'm just noting this, not necessarily
> requesting them.
Not to my knowledge; the Intel one is inherited from the atomisp-ov5693
driver, nobody else involved has mentioned it to me.
>
>> + *
>> + * Adapted from the atomisp-ov5693 driver, with contributions from:
>> + *
>> + * Daniel Scally
>> + * Jean-Michel Hautbois
>> + * Fabian Wuthrich
>> + * Tsuchiya Yuto
>> + * Jordan Hand
>> + * Jake Day
> Well done!
Thanks!
> +
> +static int ov5693_get_exposure(struct ov5693_device *ov5693, s32 *value)
> +{
> + u8 exposure_hh = 0, exposure_h = 0, exposure_l = 0;
> + int ret;
> +
> + ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_HH_REG, &exposure_hh);
> + if (ret)
> + return ret;
> +
> + ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_H_REG, &exposure_h);
> + if (ret)
> + return ret;
> +
> + ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_L_REG, &exposure_l);
> + if (ret)
> + return ret;
> Does the sensor not allow reading this register as a single operation?
>
> Just a question. Some sensors from the vendor do not. Same for the writes.
It does; if I'm honest I just preferred the individual read/writes. I
find it's easier to see exactly what's going on. Happy to change it if
you prefer though - it's less important now that the work is mostly done.
>> +static int ov5693_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct ov5693_device *ov5693 =
>> + container_of(ctrl->handler, struct ov5693_device, ctrls.handler);
> Tab for indentation, please. And wrap if needed to stay under 80.
Ack
>> +};
>> +
>> +/* System Control Functions */
>> +
>> +static int ov5693_mode_configure(struct ov5693_device *ov5693)
>> +{
>> + const struct ov5693_mode *mode = &ov5693->mode;
>> + int ret = 0;
>> +
>> + /* Crop Start X */
>> + ov5693_write_reg(ov5693, OV5693_CROP_START_X_H_REG,
>> + OV5693_CROP_START_X_H(mode->crop.left), &ret);
>> + ov5693_write_reg(ov5693, OV5693_CROP_START_X_L_REG,
>> + OV5693_CROP_START_X_L(mode->crop.left), &ret);
>> +
>> + /* Offset X */
>> + ov5693_write_reg(ov5693, OV5693_OFFSET_START_X_H_REG,
>> + OV5693_OFFSET_START_X_H(0), &ret);
>> + ov5693_write_reg(ov5693, OV5693_OFFSET_START_X_L_REG,
>> + OV5693_OFFSET_START_X_L(0), &ret);
>> +
>> + /* Output Size X */
>> + ov5693_write_reg(ov5693, OV5693_OUTPUT_SIZE_X_H_REG,
>> + OV5693_OUTPUT_SIZE_X_H(mode->format.width), &ret);
>> + ov5693_write_reg(ov5693, OV5693_OUTPUT_SIZE_X_L_REG,
>> + OV5693_OUTPUT_SIZE_X_L(mode->format.width), &ret);
>> +
>> + /* Crop End X */
>> + ov5693_write_reg(ov5693, OV5693_CROP_END_X_H_REG,
>> + OV5693_CROP_END_X_H(mode->crop.left + mode->crop.width),
>> + &ret);
>> + ov5693_write_reg(ov5693, OV5693_CROP_END_X_L_REG,
>> + OV5693_CROP_END_X_L(mode->crop.left + mode->crop.width),
>> + &ret);
>> +
>> + /* Horizontal Total Size */
>> + ov5693_write_reg(ov5693, OV5693_TIMING_HTS_H_REG,
>> + OV5693_TIMING_HTS_H(OV5693_FIXED_PPL), &ret);
>> + ov5693_write_reg(ov5693, OV5693_TIMING_HTS_L_REG,
>> + OV5693_TIMING_HTS_L(OV5693_FIXED_PPL), &ret);
>> +
>> + /* Crop Start Y */
>> + ov5693_write_reg(ov5693, OV5693_CROP_START_Y_H_REG,
>> + OV5693_CROP_START_Y_H(mode->crop.top), &ret);
>> + ov5693_write_reg(ov5693, OV5693_CROP_START_Y_L_REG,
>> + OV5693_CROP_START_Y_L(mode->crop.top), &ret);
>> +
>> + /* Offset Y */
>> + ov5693_write_reg(ov5693, OV5693_OFFSET_START_Y_H_REG,
>> + OV5693_OFFSET_START_Y_H(0), &ret);
>> + ov5693_write_reg(ov5693, OV5693_OFFSET_START_Y_L_REG,
>> + OV5693_OFFSET_START_Y_L(0), &ret);
>> +
>> + /* Output Size Y */
>> + ov5693_write_reg(ov5693, OV5693_OUTPUT_SIZE_Y_H_REG,
>> + OV5693_OUTPUT_SIZE_Y_H(mode->format.height), &ret);
>> + ov5693_write_reg(ov5693, OV5693_OUTPUT_SIZE_Y_L_REG,
>> + OV5693_OUTPUT_SIZE_Y_L(mode->format.height), &ret);
>> +
>> + /* Crop End Y */
>> + ov5693_write_reg(ov5693, OV5693_CROP_END_Y_H_REG,
>> + OV5693_CROP_END_Y_H(mode->crop.top + mode->crop.height),
>> + &ret);
>> + ov5693_write_reg(ov5693, OV5693_CROP_END_Y_L_REG,
>> + OV5693_CROP_END_Y_L(mode->crop.top + mode->crop.height),
>> + &ret);
>> +
>> + /* Vertical Total Size */
>> + ov5693_write_reg(ov5693, OV5693_TIMING_VTS_H_REG,
>> + OV5693_TIMING_VTS_H(mode->vts), &ret);
>> + ov5693_write_reg(ov5693, OV5693_TIMING_VTS_L_REG,
>> + OV5693_TIMING_VTS_L(mode->vts), &ret);
> ov5693_vts_configure() does write the same registers. Why is it being done
> here, too?
Ah, yes. And ov5693_vts_configure() will get called by
__v4l2_ctrl_handler_setup() in ov5693_s_stream(), so you're right that's
superfluous. I'll remove it, thanks.
> That said, it's very nice the driver does more than just rely on plain
> register lists.
Laurent's influence (and patience) :)
> +}
> +
> +static int ov5693_probe(struct i2c_client *client)
> +{
> + struct fwnode_handle *fwnode = dev_fwnode(&client->dev);
> + struct fwnode_handle *endpoint;
> + struct ov5693_device *ov5693;
> + u32 clk_rate;
> + int ret = 0;
> +
> + endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> + if (!endpoint && !IS_ERR_OR_NULL(fwnode->secondary))
> + endpoint = fwnode_graph_get_next_endpoint(fwnode->secondary, NULL);
> I think this should be done by fwnode_graph_get_next_endpoint() instead.
Argh; sorry. It actually _is_ done by fwnode_graph_get_next_endpoint(),
I patched it [1] a little while ago and then forgot to update this
driver. That patch went into v5.15-rc1 - I'll update this driver.
> There seem to be a large number of lines over 80 chars per line, for no
> apparent reason. Please wrap when it's feasible to do so.
I'll do a pass through and clean them up
>
>> + if (!endpoint)
>> + return -EPROBE_DEFER;
> Where is the endpoint put?
>
> I know this is does nothing on ACPI but fwnode-API-wise it does matter.
Ah, it's not - my bad. Thanks
>
> Where do you get the bus parameters, including link frequencies (that do
> not seem to be validated in any way at the moment)?
Nowhere currently, I'll add a section to fetch and validate.
>> +
>> + ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL);
>> + if (!ov5693)
>> + return -ENOMEM;
>> +
>> + ov5693->client = client;
>> + ov5693->dev = &client->dev;
>> +
>> + mutex_init(&ov5693->lock);
>> +
>> + v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops);
>> +
>> + ov5693->clk = devm_clk_get(&client->dev, "xvclk");
>> + if (IS_ERR(ov5693->clk)) {
>> + dev_err(&client->dev, "Error getting clock\n");
>> + return PTR_ERR(ov5693->clk);
>> + }
>> +
>> + clk_rate = clk_get_rate(ov5693->clk);
>> + if (clk_rate != OV5693_XVCLK_FREQ) {
>> + dev_err(&client->dev, "Unsupported clk freq %u, expected %u\n",
>> + clk_rate, OV5693_XVCLK_FREQ);
>> + return -EINVAL;
> There's been discussion on this and I think the current best practice is
> seen to be to warn, but still proceed. Sometimes there are slight
> differences from the intended frequency but it's hard to say how much
> tolerance there really is.
OK, no problem. I'll switch to doing that.
>
>> + }
>> +
>> + ret = ov5693_configure_gpios(ov5693);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ov5693_get_regulators(ov5693);
>> + if (ret) {
>> + dev_err(&client->dev, "Error fetching regulators\n");
>> + return ret;
>> + }
>> +
>> + ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> + ov5693->pad.flags = MEDIA_PAD_FL_SOURCE;
>> + ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>> +
>> + ov5693->mode.crop = ov5693_default_crop;
>> + ov5693->mode.format = ov5693_default_fmt;
>> + ov5693->mode.vts = __ov5693_calc_vts(ov5693->mode.format.height);
>> +
>> + ret = ov5693_init_controls(ov5693);
>> + if (ret)
>> + return ret;
>> +
>> + ret = media_entity_pads_init(&ov5693->sd.entity, 1, &ov5693->pad);
>> + if (ret)
>> + goto err_ctrl_handler_free;
>> +
>> + /*
>> + * We need the driver to work in the event that pm runtime is disable in
>> + * the kernel, so power up and verify the chip now. In the event that
>> + * runtime pm is disabled this will leave the chip on, so that streaming
>> + * will work.
>> + */
>> +
>> + ret = ov5693_sensor_powerup(ov5693);
>> + if (ret)
>> + goto err_media_entity_cleanup;
>> +
>> + ret = ov5693_detect(ov5693);
>> + if (ret)
>> + goto err_powerdown;
>> +
>> + pm_runtime_set_active(&client->dev);
>> + pm_runtime_get_noresume(&client->dev);
>> + pm_runtime_enable(&client->dev);
>> +
>> + ret = v4l2_async_register_subdev_sensor(&ov5693->sd);
>> + if (ret) {
>> + dev_err(&client->dev, "failed to register V4L2 subdev: %d",
>> + ret);
>> + goto err_pm_runtime;
>> + }
>> +
>> + pm_runtime_set_autosuspend_delay(&client->dev, 1000);
>> + pm_runtime_use_autosuspend(&client->dev);
>> + pm_runtime_put_autosuspend(&client->dev);
>> +
>> + return ret;
>> +
>> +err_pm_runtime:
>> + pm_runtime_disable(&client->dev);
>> + pm_runtime_put_noidle(&client->dev);
>> +err_powerdown:
>> + ov5693_sensor_powerdown(ov5693);
>> +err_media_entity_cleanup:
>> + media_entity_cleanup(&ov5693->sd.entity);
>> +err_ctrl_handler_free:
>> + v4l2_ctrl_handler_free(&ov5693->ctrls.handler);
>> +
>> + return ret;
>> +}
>> +
>> +static int ov5693_remove(struct i2c_client *client)
>> +{
>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> + struct ov5693_device *ov5693 = to_ov5693_sensor(sd);
>> +
>> + v4l2_async_unregister_subdev(sd);
>> + media_entity_cleanup(&ov5693->sd.entity);
>> + v4l2_ctrl_handler_free(&ov5693->ctrls.handler);
>> + mutex_destroy(&ov5693->lock);
>> +
>> + /*
>> + * Disable runtime PM. In case runtime PM is disabled in the kernel,
>> + * make sure to turn power off manually.
>> + */
>> + pm_runtime_disable(&client->dev);
>> + if (!pm_runtime_status_suspended(&client->dev))
>> + ov5693_sensor_powerdown(ov5693);
>> + pm_runtime_set_suspended(&client->dev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dev_pm_ops ov5693_pm_ops = {
>> + SET_RUNTIME_PM_OPS(ov5693_sensor_suspend, ov5693_sensor_resume, NULL)
>> +};
>> +
>> +static const struct acpi_device_id ov5693_acpi_match[] = {
>> + {"INT33BE"},
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match);
>> +
>> +static struct i2c_driver ov5693_driver = {
>> + .driver = {
>> + .name = "ov5693",
>> + .acpi_match_table = ov5693_acpi_match,
>> + .pm = &ov5693_pm_ops,
>> + },
>> + .probe_new = ov5693_probe,
>> + .remove = ov5693_remove,
>> +};
>> +module_i2c_driver(ov5693_driver);
>> +
>> +MODULE_DESCRIPTION("A low-level driver for OmniVision 5693 sensors");
>> +MODULE_LICENSE("GPL");
> MODULE_AUTHOR(), maybe?
[1] https://lore.kernel.org/all/20210806220906.326758-2-djrscally@xxxxxxxxx/
>
