[PATCH 0/2] Single-lane CSI-2 operation on OmniVision OV5670

[PATCH 0/2] Single-lane CSI-2 operation on OmniVision OV5670

[Luca Weiss]

Add support for single-lane configuration of OV5670 sensor.

While I believe this series shouldn't break anything on ACPI systems I
don't not have one to test. Verified on DT/OF only using a single-lane
configuration.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Luca Weiss (2):
      media: i2c: ov5670: Use dev_err_probe in probe function
      media: i2c: ov5670: Support single-lane operation

 drivers/media/i2c/ov5670.c | 112 +++++++++++++++++++++++++++++++--------------
 1 file changed, 77 insertions(+), 35 deletions(-)
---
base-commit: 6ba8a227fd19d19779005fb66ad7562608e1df83
change-id: 20230210-ov5670-single-lane-259561b1a820

Best regards,
-- 
Luca Weiss <luca@z3ntu.xyz>

[PATCH 1/2] media: i2c: ov5670: Use dev_err_probe in probe function

[Luca Weiss]

Replace the unusual const char *err_msg usage with dev_err_probe which
also handles -EPROBE_DEFER better by not printing the message to kmsg.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 drivers/media/i2c/ov5670.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index f79d908f4531..189920571df2 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2648,17 +2648,13 @@ static int ov5670_gpio_probe(struct ov5670 *ov5670)
 static int ov5670_probe(struct i2c_client *client)
 {
 	struct ov5670 *ov5670;
-	const char *err_msg;
 	u32 input_clk = 0;
 	bool full_power;
 	int ret;
 
 	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
-	if (!ov5670) {
-		ret = -ENOMEM;
-		err_msg = "devm_kzalloc() error";
-		goto error_print;
-	}
+	if (!ov5670)
+		return -ENOMEM;
 
 	ov5670->xvclk = devm_clk_get(&client->dev, NULL);
 	if (!IS_ERR_OR_NULL(ov5670->xvclk))
@@ -2680,29 +2676,23 @@ static int ov5670_probe(struct i2c_client *client)
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
 	ret = ov5670_regulators_probe(ov5670);
-	if (ret) {
-		err_msg = "Regulators probe failed";
-		goto error_print;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Regulators probe failed\n");
 
 	ret = ov5670_gpio_probe(ov5670);
-	if (ret) {
-		err_msg = "GPIO probe failed";
-		goto error_print;
-	}
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "GPIO probe failed\n");
 
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		ret = ov5670_runtime_resume(&client->dev);
-		if (ret) {
-			err_msg = "Power up failed";
-			goto error_print;
-		}
+		if (ret)
+			return dev_err_probe(&client->dev, ret, "Power up failed\n");
 
 		/* Check module identity */
 		ret = ov5670_identify_module(ov5670);
 		if (ret) {
-			err_msg = "ov5670_identify_module() error";
+			dev_err_probe(&client->dev, ret, "ov5670_identify_module() error\n");
 			goto error_power_off;
 		}
 	}
@@ -2714,7 +2704,7 @@ static int ov5670_probe(struct i2c_client *client)
 
 	ret = ov5670_init_controls(ov5670);
 	if (ret) {
-		err_msg = "ov5670_init_controls() error";
+		dev_err_probe(&client->dev, ret, "ov5670_init_controls() error\n");
 		goto error_mutex_destroy;
 	}
 
@@ -2727,7 +2717,7 @@ static int ov5670_probe(struct i2c_client *client)
 	ov5670->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&ov5670->sd.entity, 1, &ov5670->pad);
 	if (ret) {
-		err_msg = "media_entity_pads_init() error";
+		dev_err_probe(&client->dev, ret, "media_entity_pads_init() error\n");
 		goto error_handler_free;
 	}
 
@@ -2741,7 +2731,7 @@ static int ov5670_probe(struct i2c_client *client)
 	/* Async register for subdev */
 	ret = v4l2_async_register_subdev_sensor(&ov5670->sd);
 	if (ret < 0) {
-		err_msg = "v4l2_async_register_subdev() error";
+		dev_err_probe(&client->dev, ret, "v4l2_async_register_subdev() error\n");
 		goto error_pm_disable;
 	}
 
@@ -2764,9 +2754,6 @@ static int ov5670_probe(struct i2c_client *client)
 	if (full_power)
 		ov5670_runtime_suspend(&client->dev);
 
-error_print:
-	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
-
 	return ret;
 }
 

-- 
2.39.1

[PATCH 2/2] media: i2c: ov5670: Support single-lane operation

[Luca Weiss]

Currently the driver always configures the sensor for dual-lane MIPI
output, but it also supports single-lane output. Add support for that by
checking the data-lanes fwnode property how many lanes are used and
configure the necessary registers based on that.

To achieve this we move setting register 0x3018 out of the general reg
sequence so we set it to the correct value. The pixel_rate value also
needs to be adjusted.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 drivers/media/i2c/ov5670.c | 85 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 70 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 189920571df2..4ca082455c46 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -29,6 +29,12 @@
 #define OV5670_REG_SOFTWARE_RST		0x0103
 #define OV5670_SOFTWARE_RST		0x01
 
+#define OV5670_MIPI_SC_CTRL0_REG		0x3018
+#define OV5670_MIPI_SC_CTRL0_LANES(v)		((((v) - 1) << 5) & \
+						 GENMASK(7, 5))
+#define OV5670_MIPI_SC_CTRL0_MIPI_EN		BIT(4)
+#define OV5670_MIPI_SC_CTRL0_RESERVED		BIT(1)
+
 /* vertical-timings from sensor */
 #define OV5670_REG_VTS			0x380e
 #define OV5670_VTS_30FPS		0x0808 /* default for 30 fps */
@@ -92,7 +98,6 @@ struct ov5670_reg_list {
 };
 
 struct ov5670_link_freq_config {
-	u32 pixel_rate;
 	const struct ov5670_reg_list reg_list;
 };
 
@@ -163,7 +168,6 @@ static const struct ov5670_reg mode_2592x1944_regs[] = {
 	{0x3005, 0xf0},
 	{0x3007, 0x00},
 	{0x3015, 0x0f},
-	{0x3018, 0x32},
 	{0x301a, 0xf0},
 	{0x301b, 0xf0},
 	{0x301c, 0xf0},
@@ -429,7 +433,6 @@ static const struct ov5670_reg mode_1296x972_regs[] = {
 	{0x3005, 0xf0},
 	{0x3007, 0x00},
 	{0x3015, 0x0f},
-	{0x3018, 0x32},
 	{0x301a, 0xf0},
 	{0x301b, 0xf0},
 	{0x301c, 0xf0},
@@ -695,7 +698,6 @@ static const struct ov5670_reg mode_648x486_regs[] = {
 	{0x3005, 0xf0},
 	{0x3007, 0x00},
 	{0x3015, 0x0f},
-	{0x3018, 0x32},
 	{0x301a, 0xf0},
 	{0x301b, 0xf0},
 	{0x301c, 0xf0},
@@ -961,7 +963,6 @@ static const struct ov5670_reg mode_2560x1440_regs[] = {
 	{0x3005, 0xf0},
 	{0x3007, 0x00},
 	{0x3015, 0x0f},
-	{0x3018, 0x32},
 	{0x301a, 0xf0},
 	{0x301b, 0xf0},
 	{0x301c, 0xf0},
@@ -1226,7 +1227,6 @@ static const struct ov5670_reg mode_1280x720_regs[] = {
 	{0x3005, 0xf0},
 	{0x3007, 0x00},
 	{0x3015, 0x0f},
-	{0x3018, 0x32},
 	{0x301a, 0xf0},
 	{0x301b, 0xf0},
 	{0x301c, 0xf0},
@@ -1492,7 +1492,6 @@ static const struct ov5670_reg mode_640x360_regs[] = {
 	{0x3005, 0xf0},
 	{0x3007, 0x00},
 	{0x3015, 0x0f},
-	{0x3018, 0x32},
 	{0x301a, 0xf0},
 	{0x301b, 0xf0},
 	{0x301c, 0xf0},
@@ -1762,8 +1761,6 @@ static const char * const ov5670_test_pattern_menu[] = {
 #define OV5670_LINK_FREQ_422MHZ_INDEX	0
 static const struct ov5670_link_freq_config link_freq_configs[] = {
 	{
-		/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-		.pixel_rate = (OV5670_LINK_FREQ_422MHZ * 2 * 2) / 10,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mipi_data_rate_840mbps),
 			.regs = mipi_data_rate_840mbps,
@@ -1859,6 +1856,7 @@ static const struct ov5670_mode supported_modes[] = {
 struct ov5670 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
+	struct v4l2_fwnode_endpoint endpoint;
 
 	struct v4l2_ctrl_handler ctrl_handler;
 	/* V4L2 Controls */
@@ -2101,9 +2099,13 @@ static const struct v4l2_ctrl_ops ov5670_ctrl_ops = {
 /* Initialize control handlers */
 static int ov5670_init_controls(struct ov5670 *ov5670)
 {
+	struct v4l2_mbus_config_mipi_csi2 *bus_mipi_csi2 =
+		&ov5670->endpoint.bus.mipi_csi2;
 	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
 	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
+	unsigned int lanes_count;
+	s64 mipi_pixel_rate;
 	s64 vblank_max;
 	s64 vblank_def;
 	s64 vblank_min;
@@ -2124,12 +2126,15 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
 		ov5670->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	/* By default, V4L2_CID_PIXEL_RATE is read only */
+	lanes_count = bus_mipi_csi2->num_data_lanes;
+	mipi_pixel_rate = OV5670_LINK_FREQ_422MHZ * 2 * lanes_count / 10;
+
 	ov5670->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov5670_ctrl_ops,
 					       V4L2_CID_PIXEL_RATE,
-					       link_freq_configs[0].pixel_rate,
-					       link_freq_configs[0].pixel_rate,
+					       mipi_pixel_rate,
+					       mipi_pixel_rate,
 					       1,
-					       link_freq_configs[0].pixel_rate);
+					       mipi_pixel_rate);
 
 	vblank_max = OV5670_VTS_MAX - ov5670->cur_mode->height;
 	vblank_def = ov5670->cur_mode->vts_def - ov5670->cur_mode->height;
@@ -2288,8 +2293,13 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_format *fmt)
 {
 	struct ov5670 *ov5670 = to_ov5670(sd);
+	struct v4l2_mbus_config_mipi_csi2 *bus_mipi_csi2 =
+		&ov5670->endpoint.bus.mipi_csi2;
 	const struct ov5670_mode *mode;
+	unsigned int lanes_count;
+	s64 mipi_pixel_rate;
 	s32 vblank_def;
+	s64 link_freq;
 	s32 h_blank;
 
 	mutex_lock(&ov5670->mutex);
@@ -2306,9 +2316,14 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
 	} else {
 		ov5670->cur_mode = mode;
 		__v4l2_ctrl_s_ctrl(ov5670->link_freq, mode->link_freq_index);
+
+		lanes_count = bus_mipi_csi2->num_data_lanes;
+		link_freq = link_freq_menu_items[mode->link_freq_index];
+		/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+		mipi_pixel_rate = link_freq * 2 * lanes_count / 10;
 		__v4l2_ctrl_s_ctrl_int64(
 			ov5670->pixel_rate,
-			link_freq_configs[mode->link_freq_index].pixel_rate);
+			mipi_pixel_rate);
 		/* Update limits and set FPS to default */
 		vblank_def = ov5670->cur_mode->vts_def -
 			     ov5670->cur_mode->height;
@@ -2361,6 +2376,19 @@ static int ov5670_identify_module(struct ov5670 *ov5670)
 	return 0;
 }
 
+static int ov5670_mipi_configure(struct ov5670 *ov5670)
+{
+	struct v4l2_mbus_config_mipi_csi2 *bus_mipi_csi2 =
+		&ov5670->endpoint.bus.mipi_csi2;
+	unsigned int lanes_count = bus_mipi_csi2->num_data_lanes;
+
+	return ov5670_write_reg(ov5670, OV5670_MIPI_SC_CTRL0_REG,
+				OV5670_REG_VALUE_08BIT,
+				OV5670_MIPI_SC_CTRL0_LANES(lanes_count) |
+				OV5670_MIPI_SC_CTRL0_MIPI_EN |
+				OV5670_MIPI_SC_CTRL0_RESERVED);
+}
+
 /* Prepare streaming by writing default values and customized values */
 static int ov5670_start_streaming(struct ov5670 *ov5670)
 {
@@ -2399,6 +2427,12 @@ static int ov5670_start_streaming(struct ov5670 *ov5670)
 		return ret;
 	}
 
+	ret = ov5670_mipi_configure(ov5670);
+	if (ret) {
+		dev_err(&client->dev, "%s failed to configure MIPI\n", __func__);
+		return ret;
+	}
+
 	ret = __v4l2_ctrl_handler_setup(ov5670->sd.ctrl_handler);
 	if (ret)
 		return ret;
@@ -2647,6 +2681,7 @@ static int ov5670_gpio_probe(struct ov5670 *ov5670)
 
 static int ov5670_probe(struct i2c_client *client)
 {
+	struct fwnode_handle *handle;
 	struct ov5670 *ov5670;
 	u32 input_clk = 0;
 	bool full_power;
@@ -2683,11 +2718,26 @@ static int ov5670_probe(struct i2c_client *client)
 	if (ret)
 		return dev_err_probe(&client->dev, ret, "GPIO probe failed\n");
 
+	/* Graph Endpoint */
+	handle = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
+	if (!handle)
+		return dev_err_probe(&client->dev, -ENXIO, "Endpoint for node get failed\n");
+
+	ov5670->endpoint.bus_type = V4L2_MBUS_CSI2_DPHY;
+	ov5670->endpoint.bus.mipi_csi2.num_data_lanes = 2;
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(handle, &ov5670->endpoint);
+	fwnode_handle_put(handle);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Endpoint parse failed\n");
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		ret = ov5670_runtime_resume(&client->dev);
-		if (ret)
-			return dev_err_probe(&client->dev, ret, "Power up failed\n");
+		if (ret) {
+			dev_err_probe(&client->dev, ret, "Power up failed\n");
+			goto error_endpoint;
+		}
 
 		/* Check module identity */
 		ret = ov5670_identify_module(ov5670);
@@ -2754,6 +2804,9 @@ static int ov5670_probe(struct i2c_client *client)
 	if (full_power)
 		ov5670_runtime_suspend(&client->dev);
 
+error_endpoint:
+	v4l2_fwnode_endpoint_free(&ov5670->endpoint);
+
 	return ret;
 }
 
@@ -2769,6 +2822,8 @@ static void ov5670_remove(struct i2c_client *client)
 
 	pm_runtime_disable(&client->dev);
 	ov5670_runtime_suspend(&client->dev);
+
+	v4l2_fwnode_endpoint_free(&ov5670->endpoint);
 }
 
 static const struct dev_pm_ops ov5670_pm_ops = {

-- 
2.39.1

Re: [PATCH 2/2] media: i2c: ov5670: Support single-lane operation

[kernel test robot]

Hi Luca,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6ba8a227fd19d19779005fb66ad7562608e1df83]

url:    https://github.com/intel-lab-lkp/linux/commits/Luca-Weiss/media-i2c-ov5670-Use-dev_err_probe-in-probe-function/20230211-043546
base:   6ba8a227fd19d19779005fb66ad7562608e1df83
patch link:    https://lore.kernel.org/r/20230210-ov5670-single-lane-v1-2-71835d39c1ce%40z3ntu.xyz
patch subject: [PATCH 2/2] media: i2c: ov5670: Support single-lane operation
config: openrisc-randconfig-r003-20230210 (https://download.01.org/0day-ci/archive/20230211/202302111713.VF9P2Gae-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/6cd856e5e8f62b0926959199d5a998de321c78e2
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Luca-Weiss/media-i2c-ov5670-Use-dev_err_probe-in-probe-function/20230211-043546
        git checkout 6cd856e5e8f62b0926959199d5a998de321c78e2
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=openrisc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=openrisc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302111713.VF9P2Gae-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_1g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_10g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_25g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_40g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_50g_base_r (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_50g_base_r2 (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_100g_base_r2 (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_100g_base_r4 (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_1g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_10g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_20g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_25g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_40g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_50g (section: .init.rodata)
WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_bb_100g (section: .init.rodata)
>> ERROR: modpost: "__udivdi3" [drivers/media/i2c/ov5670.ko] undefined!
>> ERROR: modpost: "__divdi3" [drivers/media/i2c/ov5670.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 1/2] media: i2c: ov5670: Use dev_err_probe in probe function

[Jacopo Mondi]

Hi Luca

On Fri, Feb 10, 2023 at 09:33:17PM +0100, Luca Weiss wrote:
> Replace the unusual const char *err_msg usage with dev_err_probe which
> also handles -EPROBE_DEFER better by not printing the message to kmsg.
>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>

Thanks! The *err_msg thing was weird indeed

> ---
>  drivers/media/i2c/ov5670.c | 37 ++++++++++++-------------------------
>  1 file changed, 12 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index f79d908f4531..189920571df2 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2648,17 +2648,13 @@ static int ov5670_gpio_probe(struct ov5670 *ov5670)
>  static int ov5670_probe(struct i2c_client *client)
>  {
>  	struct ov5670 *ov5670;
> -	const char *err_msg;
>  	u32 input_clk = 0;
>  	bool full_power;
>  	int ret;
>
>  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
> -	if (!ov5670) {
> -		ret = -ENOMEM;
> -		err_msg = "devm_kzalloc() error";
> -		goto error_print;
> -	}
> +	if (!ov5670)
> +		return -ENOMEM;
>
>  	ov5670->xvclk = devm_clk_get(&client->dev, NULL);
>  	if (!IS_ERR_OR_NULL(ov5670->xvclk))
> @@ -2680,29 +2676,23 @@ static int ov5670_probe(struct i2c_client *client)
>  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
>
>  	ret = ov5670_regulators_probe(ov5670);
> -	if (ret) {
> -		err_msg = "Regulators probe failed";
> -		goto error_print;
> -	}
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Regulators probe failed\n");
>
>  	ret = ov5670_gpio_probe(ov5670);
> -	if (ret) {
> -		err_msg = "GPIO probe failed";
> -		goto error_print;
> -	}
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "GPIO probe failed\n");

From now on, I don't think there are functions that can return
-EPROBE_DEFER and you could

        if (ret) {
                dev_err(...)
                goto ...;
        }

But if others are fine with what you have and consider using
dev_err_probe() better regardless if the called function can
return -EPROBE_DEFER or not, I'm fine with what you have here.

>
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		ret = ov5670_runtime_resume(&client->dev);
> -		if (ret) {
> -			err_msg = "Power up failed";
> -			goto error_print;
> -		}
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret, "Power up failed\n");
>
>  		/* Check module identity */
>  		ret = ov5670_identify_module(ov5670);
>  		if (ret) {
> -			err_msg = "ov5670_identify_module() error";
> +			dev_err_probe(&client->dev, ret, "ov5670_identify_module() error\n");
>  			goto error_power_off;
>  		}
>  	}
> @@ -2714,7 +2704,7 @@ static int ov5670_probe(struct i2c_client *client)
>
>  	ret = ov5670_init_controls(ov5670);
>  	if (ret) {
> -		err_msg = "ov5670_init_controls() error";
> +		dev_err_probe(&client->dev, ret, "ov5670_init_controls() error\n");
>  		goto error_mutex_destroy;
>  	}
>
> @@ -2727,7 +2717,7 @@ static int ov5670_probe(struct i2c_client *client)
>  	ov5670->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&ov5670->sd.entity, 1, &ov5670->pad);
>  	if (ret) {
> -		err_msg = "media_entity_pads_init() error";
> +		dev_err_probe(&client->dev, ret, "media_entity_pads_init() error\n");
>  		goto error_handler_free;
>  	}
>
> @@ -2741,7 +2731,7 @@ static int ov5670_probe(struct i2c_client *client)
>  	/* Async register for subdev */
>  	ret = v4l2_async_register_subdev_sensor(&ov5670->sd);
>  	if (ret < 0) {
> -		err_msg = "v4l2_async_register_subdev() error";
> +		dev_err_probe(&client->dev, ret, "v4l2_async_register_subdev() error\n");
>  		goto error_pm_disable;
>  	}
>
> @@ -2764,9 +2754,6 @@ static int ov5670_probe(struct i2c_client *client)
>  	if (full_power)
>  		ov5670_runtime_suspend(&client->dev);
>
> -error_print:
> -	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
> -
>  	return ret;
>  }
>
>
> --
> 2.39.1
>

Re: [PATCH 2/2] media: i2c: ov5670: Support single-lane operation

[Jacopo Mondi]

Hi Luca

On Fri, Feb 10, 2023 at 09:33:18PM +0100, Luca Weiss wrote:
> Currently the driver always configures the sensor for dual-lane MIPI
> output, but it also supports single-lane output. Add support for that by
> checking the data-lanes fwnode property how many lanes are used and
> configure the necessary registers based on that.
>
> To achieve this we move setting register 0x3018 out of the general reg
> sequence so we set it to the correct value. The pixel_rate value also
> needs to be adjusted.

This is not necessary right now as the driver supports a single pixel
rate, but I think it prepares for adding more frequencies, so good to
have it here!

>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>  drivers/media/i2c/ov5670.c | 85 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 70 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 189920571df2..4ca082455c46 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -29,6 +29,12 @@
>  #define OV5670_REG_SOFTWARE_RST		0x0103
>  #define OV5670_SOFTWARE_RST		0x01
>
> +#define OV5670_MIPI_SC_CTRL0_REG		0x3018
> +#define OV5670_MIPI_SC_CTRL0_LANES(v)		((((v) - 1) << 5) & \
> +						 GENMASK(7, 5))
> +#define OV5670_MIPI_SC_CTRL0_MIPI_EN		BIT(4)
> +#define OV5670_MIPI_SC_CTRL0_RESERVED		BIT(1)
> +
>  /* vertical-timings from sensor */
>  #define OV5670_REG_VTS			0x380e
>  #define OV5670_VTS_30FPS		0x0808 /* default for 30 fps */
> @@ -92,7 +98,6 @@ struct ov5670_reg_list {
>  };
>
>  struct ov5670_link_freq_config {
> -	u32 pixel_rate;
>  	const struct ov5670_reg_list reg_list;
>  };
>
> @@ -163,7 +168,6 @@ static const struct ov5670_reg mode_2592x1944_regs[] = {
>  	{0x3005, 0xf0},
>  	{0x3007, 0x00},
>  	{0x3015, 0x0f},
> -	{0x3018, 0x32},
>  	{0x301a, 0xf0},
>  	{0x301b, 0xf0},
>  	{0x301c, 0xf0},
> @@ -429,7 +433,6 @@ static const struct ov5670_reg mode_1296x972_regs[] = {
>  	{0x3005, 0xf0},
>  	{0x3007, 0x00},
>  	{0x3015, 0x0f},
> -	{0x3018, 0x32},
>  	{0x301a, 0xf0},
>  	{0x301b, 0xf0},
>  	{0x301c, 0xf0},
> @@ -695,7 +698,6 @@ static const struct ov5670_reg mode_648x486_regs[] = {
>  	{0x3005, 0xf0},
>  	{0x3007, 0x00},
>  	{0x3015, 0x0f},
> -	{0x3018, 0x32},
>  	{0x301a, 0xf0},
>  	{0x301b, 0xf0},
>  	{0x301c, 0xf0},
> @@ -961,7 +963,6 @@ static const struct ov5670_reg mode_2560x1440_regs[] = {
>  	{0x3005, 0xf0},
>  	{0x3007, 0x00},
>  	{0x3015, 0x0f},
> -	{0x3018, 0x32},
>  	{0x301a, 0xf0},
>  	{0x301b, 0xf0},
>  	{0x301c, 0xf0},
> @@ -1226,7 +1227,6 @@ static const struct ov5670_reg mode_1280x720_regs[] = {
>  	{0x3005, 0xf0},
>  	{0x3007, 0x00},
>  	{0x3015, 0x0f},
> -	{0x3018, 0x32},
>  	{0x301a, 0xf0},
>  	{0x301b, 0xf0},
>  	{0x301c, 0xf0},
> @@ -1492,7 +1492,6 @@ static const struct ov5670_reg mode_640x360_regs[] = {
>  	{0x3005, 0xf0},
>  	{0x3007, 0x00},
>  	{0x3015, 0x0f},
> -	{0x3018, 0x32},
>  	{0x301a, 0xf0},
>  	{0x301b, 0xf0},
>  	{0x301c, 0xf0},
> @@ -1762,8 +1761,6 @@ static const char * const ov5670_test_pattern_menu[] = {
>  #define OV5670_LINK_FREQ_422MHZ_INDEX	0
>  static const struct ov5670_link_freq_config link_freq_configs[] = {
>  	{
> -		/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> -		.pixel_rate = (OV5670_LINK_FREQ_422MHZ * 2 * 2) / 10,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mipi_data_rate_840mbps),
>  			.regs = mipi_data_rate_840mbps,
> @@ -1859,6 +1856,7 @@ static const struct ov5670_mode supported_modes[] = {
>  struct ov5670 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
> +	struct v4l2_fwnode_endpoint endpoint;
>
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	/* V4L2 Controls */
> @@ -2101,9 +2099,13 @@ static const struct v4l2_ctrl_ops ov5670_ctrl_ops = {
>  /* Initialize control handlers */
>  static int ov5670_init_controls(struct ov5670 *ov5670)
>  {
> +	struct v4l2_mbus_config_mipi_csi2 *bus_mipi_csi2 =
> +		&ov5670->endpoint.bus.mipi_csi2;
>  	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
>  	struct v4l2_fwnode_device_properties props;
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	unsigned int lanes_count;
> +	s64 mipi_pixel_rate;
>  	s64 vblank_max;
>  	s64 vblank_def;
>  	s64 vblank_min;
> @@ -2124,12 +2126,15 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
>  		ov5670->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
>  	/* By default, V4L2_CID_PIXEL_RATE is read only */
> +	lanes_count = bus_mipi_csi2->num_data_lanes;
> +	mipi_pixel_rate = OV5670_LINK_FREQ_422MHZ * 2 * lanes_count / 10;
> +
>  	ov5670->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov5670_ctrl_ops,
>  					       V4L2_CID_PIXEL_RATE,
> -					       link_freq_configs[0].pixel_rate,
> -					       link_freq_configs[0].pixel_rate,
> +					       mipi_pixel_rate,
> +					       mipi_pixel_rate,
>  					       1,
> -					       link_freq_configs[0].pixel_rate);
> +					       mipi_pixel_rate);
>
>  	vblank_max = OV5670_VTS_MAX - ov5670->cur_mode->height;
>  	vblank_def = ov5670->cur_mode->vts_def - ov5670->cur_mode->height;
> @@ -2288,8 +2293,13 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_format *fmt)
>  {
>  	struct ov5670 *ov5670 = to_ov5670(sd);
> +	struct v4l2_mbus_config_mipi_csi2 *bus_mipi_csi2 =
> +		&ov5670->endpoint.bus.mipi_csi2;
>  	const struct ov5670_mode *mode;
> +	unsigned int lanes_count;
> +	s64 mipi_pixel_rate;
>  	s32 vblank_def;
> +	s64 link_freq;
>  	s32 h_blank;
>
>  	mutex_lock(&ov5670->mutex);
> @@ -2306,9 +2316,14 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
>  	} else {
>  		ov5670->cur_mode = mode;
>  		__v4l2_ctrl_s_ctrl(ov5670->link_freq, mode->link_freq_index);
> +
> +		lanes_count = bus_mipi_csi2->num_data_lanes;
> +		link_freq = link_freq_menu_items[mode->link_freq_index];
> +		/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> +		mipi_pixel_rate = link_freq * 2 * lanes_count / 10;
>  		__v4l2_ctrl_s_ctrl_int64(
>  			ov5670->pixel_rate,
> -			link_freq_configs[mode->link_freq_index].pixel_rate);
> +			mipi_pixel_rate);
>  		/* Update limits and set FPS to default */
>  		vblank_def = ov5670->cur_mode->vts_def -
>  			     ov5670->cur_mode->height;
> @@ -2361,6 +2376,19 @@ static int ov5670_identify_module(struct ov5670 *ov5670)
>  	return 0;
>  }
>
> +static int ov5670_mipi_configure(struct ov5670 *ov5670)
> +{
> +	struct v4l2_mbus_config_mipi_csi2 *bus_mipi_csi2 =
> +		&ov5670->endpoint.bus.mipi_csi2;
> +	unsigned int lanes_count = bus_mipi_csi2->num_data_lanes;
> +
> +	return ov5670_write_reg(ov5670, OV5670_MIPI_SC_CTRL0_REG,
> +				OV5670_REG_VALUE_08BIT,
> +				OV5670_MIPI_SC_CTRL0_LANES(lanes_count) |
> +				OV5670_MIPI_SC_CTRL0_MIPI_EN |
> +				OV5670_MIPI_SC_CTRL0_RESERVED);
> +}
> +
>  /* Prepare streaming by writing default values and customized values */
>  static int ov5670_start_streaming(struct ov5670 *ov5670)
>  {
> @@ -2399,6 +2427,12 @@ static int ov5670_start_streaming(struct ov5670 *ov5670)
>  		return ret;
>  	}
>
> +	ret = ov5670_mipi_configure(ov5670);
> +	if (ret) {
> +		dev_err(&client->dev, "%s failed to configure MIPI\n", __func__);
> +		return ret;
> +	}
> +
>  	ret = __v4l2_ctrl_handler_setup(ov5670->sd.ctrl_handler);
>  	if (ret)
>  		return ret;
> @@ -2647,6 +2681,7 @@ static int ov5670_gpio_probe(struct ov5670 *ov5670)
>
>  static int ov5670_probe(struct i2c_client *client)
>  {
> +	struct fwnode_handle *handle;
>  	struct ov5670 *ov5670;
>  	u32 input_clk = 0;
>  	bool full_power;
> @@ -2683,11 +2718,26 @@ static int ov5670_probe(struct i2c_client *client)
>  	if (ret)
>  		return dev_err_probe(&client->dev, ret, "GPIO probe failed\n");
>
> +	/* Graph Endpoint */
> +	handle = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
> +	if (!handle)
> +		return dev_err_probe(&client->dev, -ENXIO, "Endpoint for node get failed\n");

As commented on the previous patch, I don't think parsing the endpoint
can return -EPROBE_DEFER

> +
> +	ov5670->endpoint.bus_type = V4L2_MBUS_CSI2_DPHY;
> +	ov5670->endpoint.bus.mipi_csi2.num_data_lanes = 2;
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(handle, &ov5670->endpoint);
> +	fwnode_handle_put(handle);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Endpoint parse failed\n");
> +

ditto

>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		ret = ov5670_runtime_resume(&client->dev);
> -		if (ret)
> -			return dev_err_probe(&client->dev, ret, "Power up failed\n");
> +		if (ret) {
> +			dev_err_probe(&client->dev, ret, "Power up failed\n");
> +			goto error_endpoint;
> +		}
>
>  		/* Check module identity */
>  		ret = ov5670_identify_module(ov5670);
> @@ -2754,6 +2804,9 @@ static int ov5670_probe(struct i2c_client *client)
>  	if (full_power)
>  		ov5670_runtime_suspend(&client->dev);
>
> +error_endpoint:
> +	v4l2_fwnode_endpoint_free(&ov5670->endpoint);
> +
>  	return ret;
>  }
>
> @@ -2769,6 +2822,8 @@ static void ov5670_remove(struct i2c_client *client)
>
>  	pm_runtime_disable(&client->dev);
>  	ov5670_runtime_suspend(&client->dev);
> +
> +	v4l2_fwnode_endpoint_free(&ov5670->endpoint);

Nits apart
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>  }
>
>  static const struct dev_pm_ops ov5670_pm_ops = {
>
> --
> 2.39.1
>

Re: [PATCH 2/2] media: i2c: ov5670: Support single-lane operation

[Jacopo Mondi]

On Sat, Feb 11, 2023 at 05:19:31PM +0800, kernel test robot wrote:
> Hi Luca,
>
> Thank you for the patch! Yet something to improve:

Is this a false postive ? Doesn't seem related to Luca's patches or
have I missed it ?


>
> [auto build test ERROR on 6ba8a227fd19d19779005fb66ad7562608e1df83]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Luca-Weiss/media-i2c-ov5670-Use-dev_err_probe-in-probe-function/20230211-043546
> base:   6ba8a227fd19d19779005fb66ad7562608e1df83
> patch link:    https://lore.kernel.org/r/20230210-ov5670-single-lane-v1-2-71835d39c1ce%40z3ntu.xyz
> patch subject: [PATCH 2/2] media: i2c: ov5670: Support single-lane operation
> config: openrisc-randconfig-r003-20230210 (https://download.01.org/0day-ci/archive/20230211/202302111713.VF9P2Gae-lkp@intel.com/config)
> compiler: or1k-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/6cd856e5e8f62b0926959199d5a998de321c78e2
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Luca-Weiss/media-i2c-ov5670-Use-dev_err_probe-in-probe-function/20230211-043546
>         git checkout 6cd856e5e8f62b0926959199d5a998de321c78e2
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=openrisc olddefconfig
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=openrisc SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Link: https://lore.kernel.org/oe-kbuild-all/202302111713.VF9P2Gae-lkp@intel.com/
>
> All errors (new ones prefixed by >>, old ones prefixed by <<):
>
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_1g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_10g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_25g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_40g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_50g_base_r (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_50g_base_r2 (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_100g_base_r2 (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_ext_maps (section: .data) -> qed_mfw_ext_100g_base_r4 (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_1g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_10g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_20g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_25g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_40g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_50g (section: .init.rodata)
> WARNING: modpost: drivers/net/ethernet/qlogic/qed/qed.o: section mismatch in reference: qed_mfw_legacy_maps (section: .data) -> qed_mfw_legacy_bb_100g (section: .init.rodata)
> >> ERROR: modpost: "__udivdi3" [drivers/media/i2c/ov5670.ko] undefined!
> >> ERROR: modpost: "__divdi3" [drivers/media/i2c/ov5670.ko] undefined!
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests

Re: [PATCH 1/2] media: i2c: ov5670: Use dev_err_probe in probe function

[Luca Weiss]

On Montag, 13. Februar 2023 19:04:29 CET Jacopo Mondi wrote:
> Hi Luca
> 
> On Fri, Feb 10, 2023 at 09:33:17PM +0100, Luca Weiss wrote:
> > Replace the unusual const char *err_msg usage with dev_err_probe which
> > also handles -EPROBE_DEFER better by not printing the message to kmsg.
> > 
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> 
> Thanks! The *err_msg thing was weird indeed
> 
> > ---
> > 
> >  drivers/media/i2c/ov5670.c | 37 ++++++++++++-------------------------
> >  1 file changed, 12 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index f79d908f4531..189920571df2 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -2648,17 +2648,13 @@ static int ov5670_gpio_probe(struct ov5670
> > *ov5670)
> > 
> >  static int ov5670_probe(struct i2c_client *client)
> >  {
> >  
> >  	struct ov5670 *ov5670;
> > 
> > -	const char *err_msg;
> > 
> >  	u32 input_clk = 0;
> >  	bool full_power;
> >  	int ret;
> >  	
> >  	ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
> > 
> > -	if (!ov5670) {
> > -		ret = -ENOMEM;
> > -		err_msg = "devm_kzalloc() error";
> > -		goto error_print;
> > -	}
> > +	if (!ov5670)
> > +		return -ENOMEM;
> > 
> >  	ov5670->xvclk = devm_clk_get(&client->dev, NULL);
> >  	if (!IS_ERR_OR_NULL(ov5670->xvclk))
> > 
> > @@ -2680,29 +2676,23 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
> >  	
> >  	ret = ov5670_regulators_probe(ov5670);
> > 
> > -	if (ret) {
> > -		err_msg = "Regulators probe failed";
> > -		goto error_print;
> > -	}
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "Regulators 
probe failed\n");
> > 
> >  	ret = ov5670_gpio_probe(ov5670);
> > 
> > -	if (ret) {
> > -		err_msg = "GPIO probe failed";
> > -		goto error_print;
> > -	}
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "GPIO probe 
failed\n");
> 
> From now on, I don't think there are functions that can return
> -EPROBE_DEFER and you could
> 
>         if (ret) {
>                 dev_err(...)
>                 goto ...;
>         }
> 
> But if others are fine with what you have and consider using
> dev_err_probe() better regardless if the called function can
> return -EPROBE_DEFER or not, I'm fine with what you have here.

I like using dev_err_probe more or less everywhere in _probe so if the 
maintainers don't mind, I'd keep it that way :)

Regards
Luca

> 
> >  	full_power = acpi_dev_state_d0(&client->dev);
> >  	if (full_power) {
> >  	
> >  		ret = ov5670_runtime_resume(&client->dev);
> > 
> > -		if (ret) {
> > -			err_msg = "Power up failed";
> > -			goto error_print;
> > -		}
> > +		if (ret)
> > +			return dev_err_probe(&client->dev, ret, 
"Power up failed\n");
> > 
> >  		/* Check module identity */
> >  		ret = ov5670_identify_module(ov5670);
> >  		if (ret) {
> > 
> > -			err_msg = "ov5670_identify_module() error";
> > +			dev_err_probe(&client->dev, ret, 
"ov5670_identify_module() error\n");
> > 
> >  			goto error_power_off;
> >  		
> >  		}
> >  	
> >  	}
> > 
> > @@ -2714,7 +2704,7 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	ret = ov5670_init_controls(ov5670);
> >  	if (ret) {
> > 
> > -		err_msg = "ov5670_init_controls() error";
> > +		dev_err_probe(&client->dev, ret, 
"ov5670_init_controls() error\n");
> > 
> >  		goto error_mutex_destroy;
> >  	
> >  	}
> > 
> > @@ -2727,7 +2717,7 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	ov5670->pad.flags = MEDIA_PAD_FL_SOURCE;
> >  	ret = media_entity_pads_init(&ov5670->sd.entity, 1, &ov5670->pad);
> >  	if (ret) {
> > 
> > -		err_msg = "media_entity_pads_init() error";
> > +		dev_err_probe(&client->dev, ret, 
"media_entity_pads_init() error\n");
> > 
> >  		goto error_handler_free;
> >  	
> >  	}
> > 
> > @@ -2741,7 +2731,7 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	/* Async register for subdev */
> >  	ret = v4l2_async_register_subdev_sensor(&ov5670->sd);
> >  	if (ret < 0) {
> > 
> > -		err_msg = "v4l2_async_register_subdev() error";
> > +		dev_err_probe(&client->dev, ret, 
"v4l2_async_register_subdev()
> > error\n");> 
> >  		goto error_pm_disable;
> >  	
> >  	}
> > 
> > @@ -2764,9 +2754,6 @@ static int ov5670_probe(struct i2c_client *client)
> > 
> >  	if (full_power)
> >  	
> >  		ov5670_runtime_suspend(&client->dev);
> > 
> > -error_print:
> > -	dev_err(&client->dev, "%s: %s %d\n", __func__, err_msg, ret);
> > -
> > 
> >  	return ret;
> >  
> >  }
> > 
> > --
> > 2.39.1