Add support Add support for Omnivision OV56477

Modes supported:
- 640x480 RAW 8

Signed-off-by Ramiro Oliveira <[email protected]>

---

drivers/media/i2c/ov5647.c new file +31 -0

+
+	ret = ov5647_read(sd, 0x4814, &channel_id);
+	if (ret < 0)
+		return ret;
+
+	channel_id &= ~(3 << 6);
+	return ov5647_write(sd, 0x4814, channel_id | (channel << 6));
+}
+
+static int ov5647_stream_on(struct v4l2_subdev *sd)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ov5647_write(sd, 0x4202, 0x00);
+
+	dev_dbg(&client->dev, "Stream on");
+
+	return ov5647_write(sd, 0x300D, 0x00);
+}
+
+static int ov5647_stream_off(struct v4l2_subdev *sd)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ov5647_write(sd, 0x4202, 0x0f);
+
+	dev_dbg(&client->dev, "Stream off");
+
+	return ov5647_write(sd, 0x300D, 0x01);
+}
+

drivers/media/i2c/ov5647.c new file +98 -0

+		return ret;
+
+	ret = ov5647_write_array(sd, ov5647_640x480,
+					ARRAY_SIZE(ov5647_640x480));
+	if (ret < 0) {
+		dev_err(&client->dev, "write sensor default regs error\n");
+		return ret;
+	}
+
+	ret = ov5647_set_virtual_channel(sd, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5647_read(sd, 0x0100, &resetval);
+	if (ret < 0)
+		return ret;
+
+	if (!(resetval & 0x01)) {
+		dev_err(&client->dev, "Device was in SW standby");
+		ret = ov5647_write(sd, 0x0100, 0x01);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ov5647_write(sd, 0x4800, 0x04);
+}
+
+/**
+ * @short Control sensor power state
+ * @param[in] sd v4l2 subdev
+ * @param[in] on Sensor power
+ * @return Error code
+ */
+static int sensor_power(struct v4l2_subdev *sd, int on)
+{
+	int ret;
+	struct ov5647 *ov5647 = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ret = 0;
+	mutex_lock(&ov5647->lock);
+
+	if (on && !ov5647->power_count)	{
+		dev_dbg(&client->dev, "OV5647 power on\n");
+
+		clk_set_rate(ov5647->xclk, ov5647->xclk_freq);
+
+		ret = clk_prepare_enable(ov5647->xclk);
+		if (ret < 0) {
+			dev_err(ov5647->dev, "clk prepare enable failed\n");
+			goto out;
+		}
+
+		ret = ov5647_write_array(sd, sensor_oe_enable_regs,
+				ARRAY_SIZE(sensor_oe_enable_regs));
+		if (ret < 0) {
+			clk_disable_unprepare(ov5647->xclk);
+			dev_err(&client->dev,
+				"write sensor_oe_enable_regs error\n");
+			goto out;
+		}
+
+		ret = __sensor_init(sd);
+		if (ret < 0) {
+			clk_disable_unprepare(ov5647->xclk);
+			dev_err(&client->dev,
+				"Camera not available, check Power\n");
+			goto out;
+		}
+	} else if (!on && ov5647->power_count == 1) {
+		dev_dbg(&client->dev, "OV5647 power off\n");
+
+		dev_dbg(&client->dev, "disable oe\n");
+		ret = ov5647_write_array(sd, sensor_oe_disable_regs,
+				ARRAY_SIZE(sensor_oe_disable_regs));
+
+		if (ret < 0)
+			dev_dbg(&client->dev, "disable oe failed\n");
+
+		ret = set_sw_standby(sd, true);
+
+		if (ret < 0)
+			dev_dbg(&client->dev, "soft stby failed\n");
+
+		clk_disable_unprepare(ov5647->xclk);
+	}
+
+	/* Update the power count. */
+	ov5647->power_count += on ? 1 : -1;
+	WARN_ON(ov5647->power_count < 0);
+
+out:
+	mutex_unlock(&ov5647->lock);
+
+	return ret;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG

drivers/media/i2c/ov5647.c new file +22 -0

+}
+
+/**
+ * @short Set register value
+ * @param[in] sd v4l2 subdev
+ * @param[in] reg register struct
+ * @return Error code
+ */
+static int sensor_set_register(struct v4l2_subdev *sd,
+				const struct v4l2_dbg_register *reg)
+{
+	return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
+}
+#endif
+
+/**
+ * @short Subdev core operations registration
+ */
+static const struct v4l2_subdev_core_ops subdev_core_ops = {
+	.s_power		= sensor_power,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register		= sensor_get_register,

drivers/media/i2c/ov5647.c new file +24 -0

+static int enum_mbus_code(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index > 0)
+		return -EINVAL;
+
+	code->code = MEDIA_BUS_FMT_SBGGR8_1X8;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_pad_ops subdev_pad_ops = {
+	.enum_mbus_code =	enum_mbus_code,
+};
+
+/**
+ * @short Subdev operations registration
+ *
+ */
+static const struct v4l2_subdev_ops subdev_ops = {
+	.core		= &subdev_core_ops,
+	.video		= &subdev_video_ops,
+	.pad		= &subdev_pad_ops,

drivers/media/i2c/ov5647.c new file +92 -0

+	int ret;
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &read);
+	if (ret < 0)
+		return ret;
+
+	if (read != 0x56) {
+		dev_err(&client->dev, "Wrong model version detected");
+		return -ENODEV;
+	}
+
+	ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &read);
+	if (ret < 0)
+		return ret;
+
+	if (read != 0x47) {
+		dev_err(&client->dev, "Wrong model version detected");
+		return -ENODEV;
+	}
+
+	return ov5647_write(sd, OV5647_SW_RESET, 0x00);
+}
+
+/**
+ * @short Detect if camera is registered
+ * @param[in] sd v4l2 subdev
+ * @return Error code
+ */
+static int ov5647_registered(struct v4l2_subdev *sd)
+{
+	return 0;
+}
+
+/**
+ * @short Open device
+ * @param[in] sd v4l2 subdev
+ * @param[in] fh v4l2 file handler
+ * @return Error code
+ */
+static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct v4l2_mbus_framefmt *format =
+				v4l2_subdev_get_try_format(sd, fh->pad, 0);
+	struct v4l2_rect *crop =
+				v4l2_subdev_get_try_crop(sd, fh->pad, 0);
+
+	crop->left = OV5647_COLUMN_START_DEF;
+	crop->top = OV5647_ROW_START_DEF;
+	crop->width = OV5647_WINDOW_WIDTH_DEF;
+	crop->height = OV5647_WINDOW_HEIGHT_DEF;
+
+	format->code = MEDIA_BUS_FMT_SBGGR8_1X8;
+
+	format->width = OV5647_WINDOW_WIDTH_DEF;
+	format->height = OV5647_WINDOW_HEIGHT_DEF;
+	format->field = V4L2_FIELD_NONE;
+	format->colorspace = V4L2_COLORSPACE_SRGB;
+
+	return sensor_power(sd, true);
+}
+
+/**
+ * @short Open device
+ * @param[in] sd v4l2 subdev
+ * @param[in] fh v4l2 file handler
+ * @return Error code
+ */
+static int ov5647_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return sensor_power(sd, false);
+}
+
+/**
+ * @short Subdev internal operations registration
+ *
+ */
+static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
+	.registered = ov5647_registered,
+	.open = ov5647_open,
+	.close = ov5647_close,
+};
+
+/**
+ * @short Initialization routine - Entry point of the driver
+ * @param[in] client pointer to the i2c client structure
+ * @param[in] id pointer to the i2c device id structure
+ * @return 0 on success and a negative number on failure

drivers/media/i2c/ov5647.c new file +11 -0

+
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+	if (sensor == NULL)
+		return -ENOMEM;
+
+	/* get system clock (xclk) */
+	sensor->xclk = devm_clk_get(dev, "xclk");
+	if (IS_ERR(sensor->xclk)) {
+		dev_err(dev, "could not get xclk");
+		return PTR_ERR(sensor->xclk);
+	}

drivers/media/i2c/ov5647.c new file +52 -0

+
+	sd = &sensor->sd;
+	v4l2_i2c_subdev_init(sd, client, &subdev_ops);
+	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	ret = media_entity_pads_init(&sd->entity, 1, &sensor->pad);
+	if (ret < 0)
+		goto mutex_remove;
+
+	ret = ov5647_detect(sd);
+	if (ret < 0)
+		goto error;
+
+	ret = v4l2_async_register_subdev(sd);
+	if (ret < 0)
+		goto error;
+
+	return 0;
+error:
+	media_entity_cleanup(&sd->entity);
+mutex_remove:
+	mutex_destroy(&sensor->lock);
+	return ret;
+}
+
+/**
+ * @short Exit routine - Exit point of the driver
+ * @param[in] client pointer to the i2c client structure
+ * @return 0 on success and a negative number on failure
+ */
+static int ov5647_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5647 *ov5647 = to_state(sd);
+
+	v4l2_async_unregister_subdev(&ov5647->sd);
+	media_entity_cleanup(&ov5647->sd.entity);
+	v4l2_device_unregister_subdev(sd);
+	mutex_destroy(&ov5647->lock);
+
+	return 0;
+}
+
+static const struct i2c_device_id ov5647_id[] = {
+	{ "ov5647", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ov5647_id);
+
+#if IS_ENABLED(CONFIG_OF)

From Kconfig the driver depends on OF.

drivers/media/i2c/ov5647.c new file +3 -0

+static const struct of_device_id ov5647_of_match[] = {
+	{ .compatible = "ovti,ov5647" },
+	{ /* sentinel */ },

Hello Ramiro,

Quoted 3 lines · Ramiro Oliveira

Modes supported:
- 640x480 RAW 8

It is a pretty short commit message, please consider to write a couple of words about the sensor.

Quoted 2 lines

Signed-off-by Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/[email protected]>

---

[snip]

drivers/media/i2c/ov5647.c line 74

Quoted 5 lines

drivers/media/i2c/ov5647.c new file +5 -0

+
+struct cfg_array {
+	struct regval_list *regs;
+	int size;
+};

struct cfg_array is apparently unused.

Quoted 5 lines

drivers/media/i2c/ov5647.c new file +5 -0

+
+/**
+ * @short I2C Write operation
+ * @param[in] i2c_client I2C client
+ * @param[in] reg register address

Review

Quoted 5 lines

+ * @param[in] val value to write
+ * @return Error code
+ */

(quoted diff) +2 -0

+static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
+{

The comment contents is probably outdated, because it does not describe the function input arguments properly.

drivers/media/i2c/ov5647.c line 215

Quoted 9 lines

drivers/media/i2c/ov5647.c new file +9 -0

+	int ret;
+	unsigned char data[3] = { reg >> 8, reg & 0xff, val};
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ret = i2c_master_send(client, data, 3);
+	if (ret != 3) {
+		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
+				__func__, reg);
+		return ret < 0 ? ret : -EIO;

Here i2c_master_send() returns only to a negative error or '3', thus the check is redundant.

Please do 'return ret';

Quoted 7 lines

drivers/media/i2c/ov5647.c new file +7 -0

+	}
+	return 0;
+}
+
+/**
+ * @short I2C Read operation
+ * @param[in] i2c_client I2C client

Review

Quoted 6 lines

+ * @param[in] reg register address
+ * @param[out] val value read
+ * @return Error code
+ */

(quoted diff) +2 -0

+static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
+{

Same as above, the comment must be updated.

drivers/media/i2c/ov5647.c line 238

Quoted 10 lines

drivers/media/i2c/ov5647.c new file +10 -0

+	int ret;
+	unsigned char data_w[2] = { reg >> 8, reg & 0xff };
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ret = i2c_master_send(client, data_w, 2);
+
+	if (ret < 2) {
+		dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
+			__func__, reg);
+		return ret < 0 ? ret : -EIO;

Here i2c_master_send() returns only to a negative error or '2', thus the check is redundant.

Please do 'return ret';

drivers/media/i2c/ov5647.c line 246

Quoted 8 lines

drivers/media/i2c/ov5647.c new file +8 -0

+	}
+
+	ret = i2c_master_recv(client, val, 1);
+
+	if (ret < 1) {
+		dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
+				__func__, reg);
+		return ret < 0 ? ret : -EIO;

Here i2c_master_recv() returns only to a negative error or '1', thus the check is redundant.

Please do 'return ret';

drivers/media/i2c/ov5647.c line 255

Quoted 9 lines

drivers/media/i2c/ov5647.c new file +9 -0

+	}
+	return 0;
+}
+
+static int ov5647_write_array(struct v4l2_subdev *sd,
+				struct regval_list *regs, int array_size)
+{
+	int i = 0;
+	int ret = 0;

Please don't assign a value to 'ret' and please do declarations on a single line.

drivers/media/i2c/ov5647.c line 258

Quoted 3 lines

drivers/media/i2c/ov5647.c new file +3 -0

+
+	if (!regs)
+		return -EINVAL;

!regs is a dead code check, please remove it.

drivers/media/i2c/ov5647.c line 266

Quoted 8 lines

drivers/media/i2c/ov5647.c new file +8 -0

+
+	while (i < array_size) {
+		ret = ov5647_write(sd, regs->addr, regs->data);
+		if (ret < 0)
+			return ret;
+		i++;
+		regs++;
+	}

Please do a for-loop, it will save two lines of code:

	for (i = 0; i < array_size; i++) {
		ret = ov5647_write(sd, regs[i].addr, regs[i].data);
		if (ret < 0)
			return ret;
	}

drivers/media/i2c/ov5647.c line 273

Quoted 7 lines

drivers/media/i2c/ov5647.c new file +7 -0

+	return 0;
+}
+
+static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
+{
+	u8 channel_id;
+	int ret = 0;

Please don't assign a value to 'ret' on declaration here.

Quoted 36 lines

drivers/media/i2c/ov5647.c new file +36 -0

+
+	ret = ov5647_read(sd, 0x4814, &channel_id);
+	if (ret < 0)
+		return ret;
+
+	channel_id &= ~(3 << 6);
+	return ov5647_write(sd, 0x4814, channel_id | (channel << 6));
+}
+
+static int ov5647_stream_on(struct v4l2_subdev *sd)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ov5647_write(sd, 0x4202, 0x00);
+
+	dev_dbg(&client->dev, "Stream on");
+
+	return ov5647_write(sd, 0x300D, 0x00);
+}
+
+static int ov5647_stream_off(struct v4l2_subdev *sd)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ov5647_write(sd, 0x4202, 0x0f);
+
+	dev_dbg(&client->dev, "Stream off");
+
+	return ov5647_write(sd, 0x300D, 0x01);
+}
+
+/**
+ * @short Set SW standby
+ * @param[in] sd v4l2 sd
+ * @param[in] stanby standby mode status (on or off)
+ * @return Error code

drivers/media/i2c/ov5647.c line 314

Quoted 5 lines

+ */

drivers/media/i2c/ov5647.c new file +4 -0

+static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
+{
+	int ret;
+	unsigned char rdval;

ov5647_read() return value type is 'u8', please change it here and everywhere else in the code.

drivers/media/i2c/ov5647.c line 317

Quoted 3 lines

drivers/media/i2c/ov5647.c new file +3 -0

+
+	ret = ov5647_read(sd, 0x0100, &rdval);
+	if (ret != 0)

if (ret < 0)

drivers/media/i2c/ov5647.c line 321

Quoted 4 lines

drivers/media/i2c/ov5647.c new file +4 -0

+		return ret;
+
+	if (standby)
+		rdval &= 0xfe;

Here 'rdval &= ~0x01' would be preferred to emphasize symmetry.

Quoted 7 lines

drivers/media/i2c/ov5647.c new file +7 -0

+	else
+		rdval |= 0x01;
+
+	return ov5647_write(sd, 0x0100, rdval);
+}
+
+/**

Quoted 6 lines

+ * @short Initialize sensor
+ * @param[in] sd v4l2 subdev
+ * @param[in] val not used
+ * @return Error code
+ */
+static int __sensor_init(struct v4l2_subdev *sd)

Same as above, the comment must be updated.

Quoted 4 lines

drivers/media/i2c/ov5647.c new file +4 -0

+{
+	int ret;
+	u8 resetval;
+	u8 rdval;

drivers/media/i2c/ov5647.c line 344

Quoted 6 lines

drivers/media/i2c/ov5647.c new file +6 -0

+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	dev_dbg(&client->dev, "sensor init\n");
+
+	ret = ov5647_read(sd, 0x0100, &rdval);
+	if (ret != 0)

if (ret < 0)

Quoted 98 lines

drivers/media/i2c/ov5647.c new file +98 -0

+		return ret;
+
+	ret = ov5647_write_array(sd, ov5647_640x480,
+					ARRAY_SIZE(ov5647_640x480));
+	if (ret < 0) {
+		dev_err(&client->dev, "write sensor default regs error\n");
+		return ret;
+	}
+
+	ret = ov5647_set_virtual_channel(sd, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5647_read(sd, 0x0100, &resetval);
+	if (ret < 0)
+		return ret;
+
+	if (!(resetval & 0x01)) {
+		dev_err(&client->dev, "Device was in SW standby");
+		ret = ov5647_write(sd, 0x0100, 0x01);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ov5647_write(sd, 0x4800, 0x04);
+}
+
+/**
+ * @short Control sensor power state
+ * @param[in] sd v4l2 subdev
+ * @param[in] on Sensor power
+ * @return Error code
+ */
+static int sensor_power(struct v4l2_subdev *sd, int on)
+{
+	int ret;
+	struct ov5647 *ov5647 = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ret = 0;
+	mutex_lock(&ov5647->lock);
+
+	if (on && !ov5647->power_count)	{
+		dev_dbg(&client->dev, "OV5647 power on\n");
+
+		clk_set_rate(ov5647->xclk, ov5647->xclk_freq);
+
+		ret = clk_prepare_enable(ov5647->xclk);
+		if (ret < 0) {
+			dev_err(ov5647->dev, "clk prepare enable failed\n");
+			goto out;
+		}
+
+		ret = ov5647_write_array(sd, sensor_oe_enable_regs,
+				ARRAY_SIZE(sensor_oe_enable_regs));
+		if (ret < 0) {
+			clk_disable_unprepare(ov5647->xclk);
+			dev_err(&client->dev,
+				"write sensor_oe_enable_regs error\n");
+			goto out;
+		}
+
+		ret = __sensor_init(sd);
+		if (ret < 0) {
+			clk_disable_unprepare(ov5647->xclk);
+			dev_err(&client->dev,
+				"Camera not available, check Power\n");
+			goto out;
+		}
+	} else if (!on && ov5647->power_count == 1) {
+		dev_dbg(&client->dev, "OV5647 power off\n");
+
+		dev_dbg(&client->dev, "disable oe\n");
+		ret = ov5647_write_array(sd, sensor_oe_disable_regs,
+				ARRAY_SIZE(sensor_oe_disable_regs));
+
+		if (ret < 0)
+			dev_dbg(&client->dev, "disable oe failed\n");
+
+		ret = set_sw_standby(sd, true);
+
+		if (ret < 0)
+			dev_dbg(&client->dev, "soft stby failed\n");
+
+		clk_disable_unprepare(ov5647->xclk);
+	}
+
+	/* Update the power count. */
+	ov5647->power_count += on ? 1 : -1;
+	WARN_ON(ov5647->power_count < 0);
+
+out:
+	mutex_unlock(&ov5647->lock);
+
+	return ret;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG

drivers/media/i2c/ov5647.c line 452

Quoted 10 lines

drivers/media/i2c/ov5647.c new file +10 -0

+/**
+ * @short Get register value
+ * @param[in] sd v4l2 subdev
+ * @param[in] reg register struct
+ * @return Error code
+ */
+static int sensor_get_register(struct v4l2_subdev *sd,
+				struct v4l2_dbg_register *reg)
+{
+	unsigned char val = 0;

ov5647_read() return value type is 'u8', please change it here and everywhere else in the code.

Also please don't assign a value to 'val' n declaration.

drivers/media/i2c/ov5647.c line 456

Quoted 4 lines

drivers/media/i2c/ov5647.c new file +4 -0

+	int ret;
+
+	ret = ov5647_read(sd, reg->reg & 0xff, &val);
+	if (ret != 0)

if (ret < 0)

drivers/media/i2c/ov5647.c line 462

Quoted 6 lines

drivers/media/i2c/ov5647.c new file +6 -0

+		return ret;
+
+	reg->val = val;
+	reg->size = 1;
+
+	return ret;

return 0;

Quoted 22 lines

drivers/media/i2c/ov5647.c new file +22 -0

+}
+
+/**
+ * @short Set register value
+ * @param[in] sd v4l2 subdev
+ * @param[in] reg register struct
+ * @return Error code
+ */
+static int sensor_set_register(struct v4l2_subdev *sd,
+				const struct v4l2_dbg_register *reg)
+{
+	return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
+}
+#endif
+
+/**
+ * @short Subdev core operations registration
+ */
+static const struct v4l2_subdev_core_ops subdev_core_ops = {
+	.s_power		= sensor_power,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register		= sensor_get_register,

drivers/media/i2c/ov5647.c line 494

Quoted 10 lines

drivers/media/i2c/ov5647.c new file +10 -0

+	.s_register		= sensor_set_register,
+#endif
+};
+
+static int s_stream(struct v4l2_subdev *sd, int enable)
+{
+	if (!enable)
+		return ov5647_stream_off(sd);
+	else
+		return ov5647_stream_on(sd);

To avoid a negation please do

	if (enable)
		return ov5647_stream_on(sd);
	else
		return ov5647_stream_off(sd);

drivers/media/i2c/ov5647.c line 501

Quoted 7 lines

drivers/media/i2c/ov5647.c new file +7 -0

+}
+
+static const struct v4l2_subdev_video_ops subdev_video_ops = {
+	.s_stream =		s_stream,
+};
+
+

Please remove one of two empty lines in a row.

Quoted 29 lines

drivers/media/i2c/ov5647.c new file +29 -0

+static int enum_mbus_code(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index > 0)
+		return -EINVAL;
+
+	code->code = MEDIA_BUS_FMT_SBGGR8_1X8;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_pad_ops subdev_pad_ops = {
+	.enum_mbus_code =	enum_mbus_code,
+};
+
+/**
+ * @short Subdev operations registration
+ *
+ */
+static const struct v4l2_subdev_ops subdev_ops = {
+	.core		= &subdev_core_ops,
+	.video		= &subdev_video_ops,
+	.pad		= &subdev_pad_ops,
+};
+
+/**
+ * @short Detect camera version and model
+ * @param[in] sd v4l2 subdev

drivers/media/i2c/ov5647.c line 535

Quoted 5 lines

+ * @return Error code
+ */

drivers/media/i2c/ov5647.c new file +3 -0

+static int ov5647_detect(struct v4l2_subdev *sd)
+{
+	unsigned char read;

ov5647_read() return value type is 'u8', please change it here and everywhere else in the code.

Quoted 97 lines

drivers/media/i2c/ov5647.c new file +97 -0

+	int ret;
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &read);
+	if (ret < 0)
+		return ret;
+
+	if (read != 0x56) {
+		dev_err(&client->dev, "Wrong model version detected");
+		return -ENODEV;
+	}
+
+	ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &read);
+	if (ret < 0)
+		return ret;
+
+	if (read != 0x47) {
+		dev_err(&client->dev, "Wrong model version detected");
+		return -ENODEV;
+	}
+
+	return ov5647_write(sd, OV5647_SW_RESET, 0x00);
+}
+
+/**
+ * @short Detect if camera is registered
+ * @param[in] sd v4l2 subdev
+ * @return Error code
+ */
+static int ov5647_registered(struct v4l2_subdev *sd)
+{
+	return 0;
+}
+
+/**
+ * @short Open device
+ * @param[in] sd v4l2 subdev
+ * @param[in] fh v4l2 file handler
+ * @return Error code
+ */
+static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct v4l2_mbus_framefmt *format =
+				v4l2_subdev_get_try_format(sd, fh->pad, 0);
+	struct v4l2_rect *crop =
+				v4l2_subdev_get_try_crop(sd, fh->pad, 0);
+
+	crop->left = OV5647_COLUMN_START_DEF;
+	crop->top = OV5647_ROW_START_DEF;
+	crop->width = OV5647_WINDOW_WIDTH_DEF;
+	crop->height = OV5647_WINDOW_HEIGHT_DEF;
+
+	format->code = MEDIA_BUS_FMT_SBGGR8_1X8;
+
+	format->width = OV5647_WINDOW_WIDTH_DEF;
+	format->height = OV5647_WINDOW_HEIGHT_DEF;
+	format->field = V4L2_FIELD_NONE;
+	format->colorspace = V4L2_COLORSPACE_SRGB;
+
+	return sensor_power(sd, true);
+}
+
+/**
+ * @short Open device
+ * @param[in] sd v4l2 subdev
+ * @param[in] fh v4l2 file handler
+ * @return Error code
+ */
+static int ov5647_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return sensor_power(sd, false);
+}
+
+/**
+ * @short Subdev internal operations registration
+ *
+ */
+static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
+	.registered = ov5647_registered,
+	.open = ov5647_open,
+	.close = ov5647_close,
+};
+
+/**
+ * @short Initialization routine - Entry point of the driver
+ * @param[in] client pointer to the i2c client structure
+ * @param[in] id pointer to the i2c device id structure
+ * @return 0 on success and a negative number on failure
+ */
+static int ov5647_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;

drivers/media/i2c/ov5647.c line 637

Quoted 5 lines

drivers/media/i2c/ov5647.c new file +5 -0

+	struct ov5647 *sensor;
+	int ret;
+	struct v4l2_subdev *sd;
+
+	dev_info(&client->dev, "Installing OmniVision OV5647 camera driver\n");

Please remove the informational line, it will pollute the kernel log for no good reason.

Quoted 15 lines

drivers/media/i2c/ov5647.c new file +15 -0

+
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+	if (sensor == NULL)
+		return -ENOMEM;
+
+	/* get system clock (xclk) */
+	sensor->xclk = devm_clk_get(dev, "xclk");
+	if (IS_ERR(sensor->xclk)) {
+		dev_err(dev, "could not get xclk");
+		return PTR_ERR(sensor->xclk);
+	}
+
+	ret = of_property_read_u32(dev->of_node, "clock-frequency",
+				    &sensor->xclk_freq);
+	if (ret) {

drivers/media/i2c/ov5647.c line 658

Quoted 6 lines

drivers/media/i2c/ov5647.c new file +6 -0

+		dev_err(dev, "could not get xclk frequency\n");
+		return ret;
+	}
+
+	mutex_init(&sensor->lock);
+	sensor->dev = dev;

sensor->dev is unused, please remove it from the 'struct ov5647' declaration.

Quoted 46 lines

drivers/media/i2c/ov5647.c new file +46 -0

+
+	sd = &sensor->sd;
+	v4l2_i2c_subdev_init(sd, client, &subdev_ops);
+	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	ret = media_entity_pads_init(&sd->entity, 1, &sensor->pad);
+	if (ret < 0)
+		goto mutex_remove;
+
+	ret = ov5647_detect(sd);
+	if (ret < 0)
+		goto error;
+
+	ret = v4l2_async_register_subdev(sd);
+	if (ret < 0)
+		goto error;
+
+	return 0;
+error:
+	media_entity_cleanup(&sd->entity);
+mutex_remove:
+	mutex_destroy(&sensor->lock);
+	return ret;
+}
+
+/**
+ * @short Exit routine - Exit point of the driver
+ * @param[in] client pointer to the i2c client structure
+ * @return 0 on success and a negative number on failure
+ */
+static int ov5647_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5647 *ov5647 = to_state(sd);
+
+	v4l2_async_unregister_subdev(&ov5647->sd);
+	media_entity_cleanup(&ov5647->sd.entity);
+	v4l2_device_unregister_subdev(sd);
+	mutex_destroy(&ov5647->lock);
+
+	return 0;
+}
+
+static const struct i2c_device_id ov5647_id[] = {

drivers/media/i2c/ov5647.c line 710

Quoted 6 lines

drivers/media/i2c/ov5647.c new file +6 -0

+	{ "ov5647", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ov5647_id);
+
+#if IS_ENABLED(CONFIG_OF)

From Kconfig the driver depends on OF.

Quoted 7 lines

drivers/media/i2c/ov5647.c new file +7 -0

+static const struct of_device_id ov5647_of_match[] = {
+	{ .compatible = "ovti,ov5647" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov5647_of_match);
+#endif
+

drivers/media/i2c/ov5647.c line 723

Quoted 6 lines

drivers/media/i2c/ov5647.c new file +6 -0

+/**
+ * @short i2c driver structure
+ */
+static struct i2c_driver ov5647_driver = {
+	.driver = {
+		.of_match_table = of_match_ptr(ov5647_of_match),

Same comment as above, from Kconfig the driver depends on OF.

drivers/media/i2c/ov5647.c line 724

Quoted 1 line

drivers/media/i2c/ov5647.c new file +1 -0

+		.owner	= THIS_MODULE,

.owner is set by the core, please remove it.

drivers/media/i2c/ov5647.c line 725

Quoted 1 line

drivers/media/i2c/ov5647.c new file +1 -0

+		.name	= "ov5647",

May be .name = SENSOR_NAME, ?

Otherwise SENSOR_NAME macro is unused and it should be removed.

drivers/media/i2c/ov5647.c line 736

Quoted 12 lines

drivers/media/i2c/ov5647.c new file +11 -0

+	},
+	.probe		= ov5647_probe,
+	.remove		= ov5647_remove,
+	.id_table	= ov5647_id,
+};
+
+module_i2c_driver(ov5647_driver);
+
+MODULE_AUTHOR("Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/[email protected]>");
+MODULE_DESCRIPTION("A low-level driver for OmniVision ov5647 sensors");
+MODULE_LICENSE("GPL v2");

--
With best wishes,
Vladimir

Hi Vladimir,

Thank you for your feedback.

Quoted 6 lines · Vladimir Zapolskiy

Hello Ramiro,

Quoted 3 lines · Ramiro Oliveira

Modes supported:
- 640x480 RAW 8

Quoted 4 lines

Quoted 4 lines

drivers/media/i2c/ov5647.c new file +4 -0

+static int ov5647_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;

Quoted 9 lines

drivers/media/i2c/ov5647.c line 637

Quoted 5 lines

drivers/media/i2c/ov5647.c new file +5 -0

+	struct ov5647 *sensor;
+	int ret;
+	struct v4l2_subdev *sd;
+
+	dev_info(&client->dev, "Installing OmniVision OV5647 camera driver\n");

Please remove the informational line, it will pollute the kernel log for no good reason.

Is it okay if I change it to debug?

Quoted 11 lines

drivers/media/i2c/ov5647.c line 710

Quoted 8 lines

drivers/media/i2c/ov5647.c new file +8 -0

+
+static const struct i2c_device_id ov5647_id[] = {
+	{ "ov5647", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ov5647_id);
+
+#if IS_ENABLED(CONFIG_OF)

From Kconfig the driver depends on OF.

You're right. Do you think I should remove the dependency in Kconfig or the check here?

Quoted 7 lines

Quoted 7 lines

drivers/media/i2c/ov5647.c new file +7 -0

+static const struct of_device_id ov5647_of_match[] = {
+	{ .compatible = "ovti,ov5647" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov5647_of_match);
+#endif
+

Quoted 9 lines

drivers/media/i2c/ov5647.c line 723

Quoted 6 lines

drivers/media/i2c/ov5647.c new file +6 -0

+/**
+ * @short i2c driver structure
+ */
+static struct i2c_driver ov5647_driver = {
+	.driver = {
+		.of_match_table = of_match_ptr(ov5647_of_match),

Same comment as above, from Kconfig the driver depends on OF.

I'm sorry but I'm not understanding what you're trying to say.

Quoted 4 lines

drivers/media/i2c/ov5647.c line 724

Quoted 1 line

drivers/media/i2c/ov5647.c new file +1 -0

+		.owner	= THIS_MODULE,

.owner is set by the core, please remove it.

Ok.

Quoted 6 lines

drivers/media/i2c/ov5647.c line 725

Quoted 1 line

drivers/media/i2c/ov5647.c new file +1 -0

+		.name	= "ov5647",

May be .name = SENSOR_NAME, ?

Otherwise SENSOR_NAME macro is unused and it should be removed.

I'll change it to .name = SENSOR_NAME,

Quoted 13 lines

Quoted 12 lines

drivers/media/i2c/ov5647.c new file +11 -0

+	},
+	.probe		= ov5647_probe,
+	.remove		= ov5647_remove,
+	.id_table	= ov5647_id,
+};
+
+module_i2c_driver(ov5647_driver);
+
+MODULE_AUTHOR("Ramiro Oliveira <roliveir-HKixBCOQz3hWk0Htik3J/[email protected]>");
+MODULE_DESCRIPTION("A low-level driver for OmniVision ov5647 sensors");
+MODULE_LICENSE("GPL v2");