Hi Hyungwoo,
On Mon, Jun 12, 2017 at 05:56:00PM -0700, Hyungwoo Yang wrote:
> This patch adds driver for Omnivision's ov13858
> sensor, the driver supports following features:
>
> - manual exposure/gain(analog and digital) control support
> - two link frequencies
> - VBLANK/HBLANK support
> - test pattern support
> - media controller support
> - runtime pm support
> - supported resolutions
> + 4224x3136 at 30FPS
> + 2112x1568 at 30FPS(default) and 60FPS
> + 2112x1188 at 30FPS(default) and 60FPS
> + 1056x784 at 30FPS(default) and 60FPS
>
> Signed-off-by: Hyungwoo Yang <hyungwoo.yang@xxxxxxxxx>
> ---
> drivers/media/i2c/Kconfig | 8 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/ov13858.c | 1920 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1929 insertions(+)
> create mode 100644 drivers/media/i2c/ov13858.c
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c380e24..26a9a3c 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -600,6 +600,14 @@ config VIDEO_OV9650
> This is a V4L2 sensor-level driver for the Omnivision
> OV9650 and OV9652 camera sensors.
>
> +config VIDEO_OV13858
> + tristate "OmniVision OV13858 sensor support"
> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on MEDIA_CAMERA_SUPPORT
select V4L2_FWNODE
> + ---help---
> + This is a Video4Linux2 sensor-level driver for the OmniVision
> + OV13858 camera.
> +
> config VIDEO_VS6624
> tristate "ST VS6624 sensor support"
> depends on VIDEO_V4L2 && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 62323ec..3f4dc02 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
> obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
> obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
> obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
> +obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
> obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
> obj-$(CONFIG_VIDEO_MT9M111) += mt9m111.o
> obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
> diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
> new file mode 100644
> index 0000000..0334cee
> --- /dev/null
> +++ b/drivers/media/i2c/ov13858.c
...
> +static int ov13858_read_platform_data(struct device *dev,
> + struct ov13858 *ov13858)
> +{
> + struct fwnode_handle *child, *fwn_freq;
> + struct ov13858_link_freq_config *freq_cfg;
> + struct ov13858_reg *pll_regs;
> + int i, freq_id = 0, ret, num_of_values, num_of_pairs;
> + u32 val, *val_array;
> +
> + /* For now, platform data is only available from fwnode */
> + if (!dev_fwnode(dev)) {
> + dev_err(dev, "no platform data\n");
> + return -EINVAL;
> + }
> +
> + ret = device_property_read_u32(dev, "skip-frames", &val);
This is a property of the sensor (or its configuration), not the hardware
platform. Please use a constant if the value does not depend on
configuration.
> + if (ret)
> + return ret;
> + ov13858->num_of_skip_frames = val;
> +
> + device_for_each_child_node(dev, child) {
> + if (!fwnode_property_present(child, "link"))
> + continue;
You shouldn't need these here.
> +
> + /* Limited number of link frequencies are allowed */
> + if (freq_id == OV13858_NUM_OF_LINK_FREQS) {
> + dev_err(dev, "no more than two freqs\n");
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + fwn_freq = fwnode_get_next_child_node(child, NULL);
> + if (!fwn_freq) {
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + /* Get link freq menu item for LINK_FREQ control */
> + ret = fwnode_property_read_u32(fwn_freq, "link-rate", &val);
If you need to know the valid link frequencies (among other things), please
use v4l2_fwnode_endpoint_alloc_parse() instead. It parses the firmware
tables for you.
> + if (ret) {
> + dev_err(dev, "link-rate error : %d\n", ret);
> + goto error;
> + }
> + link_freq_menu_items[freq_id] = val;
> +
> + freq_cfg = &link_freq_configs[freq_id];
> + ret = fwnode_property_read_u32(fwn_freq, "pixel-rate", &val);
This is something a sensor driver needs to know. It may not be present in
device firmware.
> + if (ret) {
> + dev_err(dev, "pixel-rate error : %d\n", ret);
> + goto error;
> + }
> + freq_cfg->pixel_rate = val;
> +
> + num_of_values = fwnode_property_read_u32_array(fwn_freq,
> + "pll-regs",
This is something that could go to device firmware but I don't really see
a reason for that at the moment. Can this continue to be a part of the
driver for now?
Could you also check that the external clock frequency matches with what
your register lists assume? The property should be called "clock-frequency"
and it's a u32.
> + NULL, 0);
> + if (num_of_values <= 0 || num_of_values & 0x01) {
> + dev_err(dev, "invalid pll-regs\n");
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + /* Get num of addr-value pairs */
> + num_of_pairs = num_of_values / 2;
> + val_array = devm_kzalloc(dev,
> + sizeof(u32) * num_of_values,
> + GFP_KERNEL);
> + if (!val_array) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + pll_regs = devm_kzalloc(dev,
> + sizeof(*pll_regs) * num_of_pairs,
> + GFP_KERNEL);
> + if (!pll_regs) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + fwnode_property_read_u32_array(fwn_freq, "pll-regs",
> + val_array, num_of_values);
> + for (i = 0; i < num_of_pairs; i++) {
> + pll_regs[i].address = val_array[i * 2];
> + pll_regs[i].val = val_array[i * 2 + 1];
> + }
> +
> + devm_kfree(dev, val_array);
> +
> + freq_cfg->reg_list.num_of_regs = num_of_pairs;
> + freq_cfg->reg_list.regs = pll_regs;
> +
> + freq_id++;
> + }
> +
> + if (freq_id != OV13858_NUM_OF_LINK_FREQS)
> + return -EINVAL;
> +
> + return 0;
> +
> +error:
> + fwnode_handle_put(child);
> + return ret;
> +}
> +
> +static int ov13858_probe(struct i2c_client *client,
> + const struct i2c_device_id *devid)
> +{
> + struct ov13858 *ov13858;
> + int ret;
> +
> + ov13858 = devm_kzalloc(&client->dev, sizeof(*ov13858), GFP_KERNEL);
> + if (!ov13858)
> + return -ENOMEM;
> +
> + ret = ov13858_read_platform_data(&client->dev, ov13858);
> + if (ret)
> + return ret;
> +
> + /* Initialize subdev */
> + v4l2_i2c_subdev_init(&ov13858->sd, client, &ov13858_subdev_ops);
> +
> + /* Check module identity */
> + ret = ov13858_identify_module(ov13858);
> + if (ret) {
> + dev_err(&client->dev, "failed to find sensor: %d\n", ret);
> + return ret;
> + }
> +
> + /* Set default mode to max resolution */
> + ov13858->cur_mode = &supported_modes[0];
> +
> + ret = ov13858_init_controls(ov13858);
> + if (ret)
> + return ret;
> +
> + /* Initialize subdev */
> + ov13858->sd.internal_ops = &ov13858_internal_ops;
> + ov13858->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + ov13858->sd.entity.ops = &ov13858_subdev_entity_ops;
> + ov13858->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> + /* Initialize source pad */
> + ov13858->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&ov13858->sd.entity, 1, &ov13858->pad);
> + if (ret) {
> + dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
> + goto error_handler_free;
> + }
> +
> + ret = v4l2_async_register_subdev(&ov13858->sd);
> + if (ret < 0)
> + goto error_media_entity;
> +
> + /*
> + * Device is already turned on by i2c-core with ACPI domain PM.
> + * Enable runtime PM and turn off the device.
> + */
> + pm_runtime_get_noresume(&client->dev);
> + pm_runtime_set_active(&client->dev);
> + pm_runtime_enable(&client->dev);
> + pm_runtime_put(&client->dev);
> +
> + return 0;
> +
> +error_media_entity:
> + media_entity_cleanup(&ov13858->sd.entity);
> +
> +error_handler_free:
> + ov13858_free_controls(ov13858);
> + dev_err(&client->dev, "%s failed:%d\n", __func__, ret);
> +
> + return ret;
> +}
> +
> +static int ov13858_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov13858 *ov13858 = to_ov13858(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + media_entity_cleanup(&sd->entity);
> + ov13858_free_controls(ov13858);
> +
> + /*
> + * Disable runtime PM but keep the device turned on.
> + * i2c-core with ACPI domain PM will turn off the device.
> + */
> + pm_runtime_get_sync(&client->dev);
> + pm_runtime_disable(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> + pm_runtime_put_noidle(&client->dev);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id ov13858_id_table[] = {
> + {"ov13858", 0},
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ov13858_id_table);
> +
> +static const struct dev_pm_ops ov13858_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(ov13858_suspend, ov13858_resume)
> +};
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id ov13858_acpi_ids[] = {
> + {"OVTID858"},
> + { /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, ov13858_acpi_ids);
> +#endif
> +
> +static struct i2c_driver ov13858_i2c_driver = {
> + .driver = {
> + .name = "ov13858",
> + .owner = THIS_MODULE,
> + .pm = &ov13858_pm_ops,
> + .acpi_match_table = ACPI_PTR(ov13858_acpi_ids),
> + },
> + .probe = ov13858_probe,
> + .remove = ov13858_remove,
> + .id_table = ov13858_id_table,
> +};
> +
> +module_i2c_driver(ov13858_i2c_driver);
> +
> +MODULE_AUTHOR("Kan, Chris <chris.kan@xxxxxxxxx>");
> +MODULE_AUTHOR("Rapolu, Chiranjeevi <chiranjeevi.rapolu@xxxxxxxxx>");
> +MODULE_AUTHOR("Yang, Hyungwoo <hyungwoo.yang@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Omnivision ov13858 sensor driver");
> +MODULE_LICENSE("GPL v2");
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
