Hi Stan,

Thanks for the review.

On 05/20/2016 06:20 PM, Stanimir Varbanov wrote:

Hi Todor,

Thanks for the patch, few comments below.

On 05/18/2016 02:50 PM, Todor Tomov wrote:

quoted

The ov5645 sensor from Omnivision supports up to 2592x1944
and CSI2 interface.

The driver adds support for the following modes:
- 1280x960
- 1920x1080
- 2592x1944

Output format is packed 8bit UYVY.

Signed-off-by: Todor Tomov <redacted>
---
 drivers/media/i2c/Kconfig  |   11 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/ov5645.c | 1425 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1437 insertions(+)
 create mode 100644 drivers/media/i2c/ov5645.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 521bbf1..aa17eba 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig

@@ -490,6 +490,17 @@ config VIDEO_OV2659
 	  To compile this driver as a module, choose M here: the
 	  module will be called ov2659.
 
+config VIDEO_OV5645
+	tristate "OmniVision OV5645 sensor support"
+	depends on I2C && VIDEO_V4L2
+	depends on MEDIA_CAMERA_SUPPORT

depends on OF ?

Yes, the driver expects to be able to parse data from DT, so I'll add "depends on OF".

<cut>

quoted

+
+struct ov5645 {
+	struct i2c_client *i2c_client;
+	struct device *dev;
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+	struct v4l2_of_endpoint ep;
+	struct v4l2_mbus_framefmt fmt;
+	struct v4l2_rect crop;
+	struct clk *xclk;
+	u32 xclk_freq;

this become unused?

Yes, I'll remove it.

quoted

+
+	struct regulator *io_regulator;
+	struct regulator *core_regulator;
+	struct regulator *analog_regulator;
+
+	enum ov5645_mode current_mode;
+
+	/* Cached control values */
+	struct v4l2_ctrl_handler ctrls;
+	struct v4l2_ctrl *saturation;
+	struct v4l2_ctrl *hflip;
+	struct v4l2_ctrl *vflip;
+	struct v4l2_ctrl *autogain;
+	struct v4l2_ctrl *autoexposure;
+	struct v4l2_ctrl *awb;
+	struct v4l2_ctrl *pattern;
+
+	struct mutex power_lock; /* lock to protect power state */
+	int power;
+
+	struct gpio_desc *pwdn_gpio;
+	struct gpio_desc *rst_gpio;
+};
+
+static inline struct ov5645 *to_ov5645(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct ov5645, sd);
+}

<cut>

quoted

+static int ov5645_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct ov5645 *ov5645 = to_ov5645(sd);
+	int ret = 0;
+
+	dev_dbg(ov5645->dev, "%s: on = %d\n", __func__, on);
+
+	mutex_lock(&ov5645->power_lock);
+
+	/* If the power count is modified from 0 to != 0 or from != 0 to 0,
+	 * update the power state.
+	 */
+	if (ov5645->power == !on) {
+		if (on) {
+			ret = ov5645_set_power_on(ov5645);
+			if (ret < 0) {
+				dev_err(ov5645->dev, "could not set power %s\n",
+					on ? "on" : "off");
+				goto exit;
+			}
+
+			ret = ov5645_init(ov5645);
+			if (ret < 0) {
+				dev_err(ov5645->dev,
+					"could not set init registers\n");
+				goto exit;
+			}
+
+			ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+					 OV5645_SYSTEM_CTRL0_STOP);

please check the error code.

Ok.

quoted

+		} else {
+			ov5645_set_power_off(ov5645);
+		}
+
+		/* Update the power count. */
+		ov5645->power += on ? 1 : -1;
+		WARN_ON(ov5645->power < 0);
+	}
+
+exit:
+	mutex_unlock(&ov5645->power_lock);
+
+	return ret;
+}
+

<cut>

quoted

+
+static int ov5645_registered(struct v4l2_subdev *subdev)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(subdev);
+	struct ov5645 *ov5645 = to_ov5645(subdev);
+	u8 chip_id_high, chip_id_low;
+	int ret;
+
+	ov5645_s_power(&ov5645->sd, true);

check for error here and on the other places where call s_power.

Ok.

quoted

+
+	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH_REG, &chip_id_high);
+	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH) {
+		dev_err(ov5645->dev, "could not read ID high\n");
+		ret = -ENODEV;
+		goto reg_power_off;
+	}
+	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW_REG, &chip_id_low);
+	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW) {
+		dev_err(ov5645->dev, "could not read ID low\n");
+		ret = -ENODEV;
+		goto reg_power_off;
+	}
+
+	dev_info(&client->dev, "OV5645 detected at address 0x%02x\n",
+		 client->addr);
+
+	ov5645_s_power(&ov5645->sd, false);
+
+	return 0;
+
+reg_power_off:
+	ov5645_s_power(&ov5645->sd, false);
+	return ret;
+}
+
+static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
+{
+	struct ov5645 *ov5645 = to_ov5645(subdev);
+	int ret;
+
+	dev_dbg(ov5645->dev, "%s: enable = %d\n", __func__, enable);
+
+	if (enable) {
+		ret = ov5645_change_mode(ov5645, ov5645->current_mode);
+		if (ret < 0) {
+			dev_err(ov5645->dev, "could not set mode %d\n",
+				ov5645->current_mode);
+			return ret;
+		}
+		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
+		if (ret < 0) {
+			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
+			return ret;
+		}
+		ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+				 OV5645_SYSTEM_CTRL0_START);

Error code check here and below.

Ok.

quoted

+	} else {
+		ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+				 OV5645_SYSTEM_CTRL0_STOP);
+	}
+
+	return 0;
+}

<cut>

quoted

+static int ov5645_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct device_node *endpoint;
+	struct ov5645 *ov5645;
+	int ret = 0;

no need to initialize

Ok.

<cut>

quoted

+
+	ov5645->io_regulator = devm_regulator_get(dev, "dovdd");
+	if (IS_ERR(ov5645->io_regulator)) {
+		switch(PTR_ERR(ov5645->io_regulator)) {
+		case -ENODEV:
+			/* Regulator is optional so this is ok - continue */

if the regulator is optional then use devm_regulator_get_optional, thus
you can avoid checking ov5645->io_regulator for NULL.

and the error handling will be simply

if (IS_ERR(ret_reg))
	return PTR_ERR(ret_reg);

As discussed in private, the regulators and gpios should not be optional
but required. I'll fix it and resend.

<cut>

quoted

+
+	ov5645->pwdn_gpio = devm_gpiod_get(dev, "pwdn", GPIOD_OUT_LOW);
+	if (IS_ERR(ov5645->pwdn_gpio)) {
+		switch(PTR_ERR(ov5645->pwdn_gpio)) {
+		case -ENOENT:
+			/* GPIO is optional so this is ok - continue */

you can use devm_gpiod_get_optional then.

quoted

+			ov5645->pwdn_gpio = NULL;
+			dev_dbg(dev, "power down gpio not present\n");
+			break;
+		case -EPROBE_DEFER:
+			dev_dbg(dev, "power down gpio probe defered\n");
+			return -EPROBE_DEFER;
+		default:
+			dev_err(dev, "cannot get power down gpio\n");
+			return PTR_ERR(ov5645->pwdn_gpio);
+		}
+	}

-- 
Best regards,
Todor Tomov