Re: [PATCH v2 3/4] media: ov2685: add support for OV2685 sensor

[Sakari Ailus]

Hi Shunqian,

On Fri, Jan 12, 2018 at 10:30:57AM +0800, Shunqian Zheng wrote:
> Hi Sakari,
> 
> 
> On 2018年01月03日 19:43, Sakari Ailus wrote:
> > > +static int ov2685_s_stream(struct v4l2_subdev *sd, int on)
> > > +{
> > > + struct ov2685 *ov2685 = to_ov2685(sd);
> > > + struct i2c_client *client = ov2685->client;
> > > + int ret = 0;
> > > +
> > > + mutex_lock(>mutex);
> > > +
> > > + on = !!on;
> > > + if (on == ov2685->streaming)
> > > + goto unlock_and_return;
> > > +
> > > + if (on) {
> > > + /* In case these controls are set before streaming */
> > > + ov2685_set_exposure(ov2685, ov2685->exposure->val);
> > > + ov2685_set_gain(ov2685, ov2685->anal_gain->val);
> > > + ov2685_set_vts(ov2685, ov2685->vblank->val);
> > > + ov2685_enable_test_pattern(ov2685, ov2685->test_pattern->val);
> > You should use __v4l2_ctrl_handler_setup() here. Or put that to the
> > driver's runtime_resume function. That actually might be better.
> The v3 put __v4l2_ctrl_handler_setup() to the runtime_resume callback. But
> ov2685_s_stream()
>-> pm_runtime_get_sync()
>-> ov2685_runtime_resume()
> -> __v4l2_ctrl_handler_setup()
> -> pm_runtime_get_if_in_use(), always <= 0 because
> dev->power.runtime_status != RPM_ACTIVE.
> 
> Seems like  __v4l2_ctrl_handler_setup() has to be in ov2685_s_stream().

Right, indeed. Well spotted.

The smiapp driver uses a device specific variable for the purpose, thus I
missed this. Other drivers apply the controls while streaming on.

> 
> Thanks
> > > +
> > > + ret = ov2685_write_reg(client, REG_SC_CTRL_MODE,
> > > + OV2685_REG_VALUE_08BIT, SC_CTRL_MODE_STREAMING);
> > > + if (ret)
> > > + goto unlock_and_return;
> > > + } else {
> > > + ret = ov2685_write_reg(client, REG_SC_CTRL_MODE,
> > > + OV2685_REG_VALUE_08BIT, SC_CTRL_MODE_STANDBY);
> > > + if (ret)
> > > + goto unlock_and_return;
> > > + }
> > > +
> > > + ov2685->streaming = on;
> > > +
> > > +unlock_and_return:
> > > + mutex_unlock(>mutex);
> > > + return ret;
> > > +}
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v2 3/4] media: ov2685: add support for OV2685 sensor

[Sakari Ailus]

Hi Shunqian,

On Mon, Jan 08, 2018 at 09:37:20PM +0800, Shunqian Zheng wrote:
...
> > > +static int ov2685_probe(struct i2c_client *client,
> > > + const struct i2c_device_id *id)
> > > +{
> > > + struct device *dev = >dev;
> > > + struct ov2685 *ov2685;
> > > + int ret;
> > > +
> > > + ov2685 = devm_kzalloc(dev, sizeof(*ov2685), GFP_KERNEL);
> > > + if (!ov2685)
> > > + return -ENOMEM;
> > > +
> > > + ov2685->client = client;
> > > + ov2685->cur_mode = _modes[0];
> > > +
> > > + ov2685->xvclk = devm_clk_get(dev, "xvclk");
> > > + if (IS_ERR(ov2685->xvclk)) {
> > > + dev_err(dev, "Failed to get xvclk\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ov2685->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > > + if (IS_ERR(ov2685->reset_gpio)) {
> > > + dev_err(dev, "Failed to get reset-gpios\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ov2685->avdd_regulator = devm_regulator_get(dev, "avdd");
> > > + if (IS_ERR(ov2685->avdd_regulator)) {
> > > + dev_err(dev, "Failed to get avdd-supply\n");
> > > + return -EINVAL;
> > > + }
> > > + ov2685->dovdd_regulator = devm_regulator_get(dev, "dovdd");
> > > + if (IS_ERR(ov2685->dovdd_regulator)) {
> > > + dev_err(dev, "Failed to get dovdd-supply\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + mutex_init(>mutex);
> > > + v4l2_i2c_subdev_init(>subdev, client, _subdev_ops);
> > > + ret = ov2685_initialize_controls(ov2685);
> > > + if (ret)
> > > + goto destroy_mutex;
> > > +
> > > + ret = ov2685_check_sensor_id(ov2685, client);
> > > + if (ret)
> > > + return ret;
> > > +
> > > +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> > > + ov2685->subdev.internal_ops = _internal_ops;
> > > +#endif
> > > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > > + ov2685->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > + ov2685->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > + ov2685->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > + ret = media_entity_pads_init(>subdev.entity, 1, >pad);
> > > + if (ret < 0)
> > > + goto free_ctrl_handler;
> > > +#endif
> > > +
> > > + ret = v4l2_async_register_subdev(>subdev);
> > > + if (ret) {
> > > + dev_err(dev, "v4l2 async register subdev failed\n");
> > > + goto clean_entity;
> > > + }
> > > +
> > The device should be already powered up here... and then:
> > 
> > pm_runtime_set_active(dev);
> In my test case, sensor is not powered (power on and off in
> ov2685_check_sensor_id()).
> Checking the ov13858.c which is turned on by i2c-core with ACPI domain PM,
> this driver
> doesn't depend on ACPI.

Yes, on ACPI based systems the drivers aren't responsible for powering the
devices on and off themselves. They're powered on in probe() without usin
runtime PM.

With DT, the driver should power on the device before using runtime PM.
That way the device gets powered on even if runtime PM is disabled.

Please see the smiapp driver, it works with both DT and ACPI.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH v2 3/4] media: ov2685: add support for OV2685 sensor

[Sakari Ailus]

Hi Shunqian,

Please see my comments below.

On Fri, Dec 29, 2017 at 04:08:24PM +0800, Shunqian Zheng wrote:
> This patch adds driver for Omnivision's ov2685 sensor.
> Though the ov2685 can output yuv data, this driver only
> supports the raw bayer format, including the following features:
>   - output 1600x1200 at 30fps
>   - test patterns
>   - manual exposure/gain control
>   - vblank and hblank
>   - media controller
>   - runtime pm
> 
> Signed-off-by: Shunqian Zheng 
> ---
>  drivers/media/i2c/Kconfig  |  12 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/ov2685.c | 841 
> +
>  3 files changed, 854 insertions(+)
>  create mode 100644 drivers/media/i2c/ov2685.c
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 55b37c8..63a175d 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -586,6 +586,18 @@ config VIDEO_OV2659
> To compile this driver as a module, choose M here: the
> module will be called ov2659.
>  
> +config VIDEO_OV2685
> + tristate "OmniVision OV2685 sensor support"
> + depends on VIDEO_V4L2 && I2C && MEDIA_CONTROLLER
> + depends on MEDIA_CAMERA_SUPPORT
> + select V4L2_FWNODE
> + ---help---
> +   This is a Video4Linux2 sensor-level driver for the OmniVision
> +   OV2685 camera.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called ov2685.
> +
>  config VIDEO_OV5640
>   tristate "OmniVision OV5640 sensor support"
>   depends on OF
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index a063030..3054c69 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>  obj-$(CONFIG_VIDEO_UPD64031A) += upd64031a.o
>  obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
>  obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
> +obj-$(CONFIG_VIDEO_OV2685) += ov2685.o
>  obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
>  obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
>  obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
> diff --git a/drivers/media/i2c/ov2685.c b/drivers/media/i2c/ov2685.c
> new file mode 100644
> index 000..e037d20
> --- /dev/null
> +++ b/drivers/media/i2c/ov2685.c
> @@ -0,0 +1,841 @@
> +/*
> + * ov2685 driver
> + *
> + * Copyright (C) 2017 Fuzhou Rockchip Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define CHIP_ID  0x2685
> +#define OV2685_REG_CHIP_ID   0x300a
> +
> +#define REG_SC_CTRL_MODE 0x0100
> +#define SC_CTRL_MODE_STANDBY 0x0
> +#define SC_CTRL_MODE_STREAMING   BIT(0)
> +
> +#define OV2685_REG_EXPOSURE  0x3500
> +#define  OV2685_EXPOSURE_MIN 4
> +#define  OV2685_EXPOSURE_STEP1
> +
> +#define OV2685_REG_VTS   0x380e
> +#define OV2685_VTS_MAX   0x7fff
> +
> +#define OV2685_REG_GAIN  0x350a
> +#define OV2685_GAIN_MIN  0
> +#define OV2685_GAIN_MAX  0x07ff
> +#define OV2685_GAIN_STEP 0x1
> +#define OV2685_GAIN_DEFAULT  0x0036
> +
> +#define OV2685_REG_TEST_PATTERN  0x5080
> +#define OV2685_TEST_PATTERN_DISABLED 0x00
> +#define OV2685_TEST_PATTERN_COLOR_BAR0x80
> +#define OV2685_TEST_PATTERN_RND  0x81
> +#define OV2685_TEST_PATTERN_COLOR_BAR_FADE   0x88
> +#define OV2685_TEST_PATTERN_BW_SQUARE0x92
> +#define OV2685_TEST_PATTERN_COLOR_SQUARE 0x82
> +
> +#define REG_NULL 0x
> +
> +#define OV2685_REG_VALUE_08BIT   1
> +#define OV2685_REG_VALUE_16BIT   2
> +#define OV2685_REG_VALUE_24BIT   3
> +
> +#define OV2685_LANES 1
> +#define OV2685_BITS_PER_SAMPLE   10
> +
> +struct regval {
> + u16 addr;
> + u8 val;
> +};
> +
> +struct ov2685_mode {
> + u32 width;
> + u32 height;
> + u32 exp_def;
> + u32 hts_def;
> + u32 vts_def;
> + const struct regval *reg_list;
> +};
> +
> +struct ov2685 {
> + struct i2c_client   *client;
> + struct clk  *xvclk;
> + struct regulator*avdd_regulator;/* Analog power */
> + struct regulator*dovdd_regulator;   /* Digital I/O power */
> + /* use internal DVDD power */
> + struct gpio_desc*reset_gpio;
> +
> + bool