[PATCH v1 1/8] media: ov2740: Remove duplicative pointer in struct nvm_data

[PATCH v1 1/8] media: ov2740: Remove duplicative pointer in struct nvm_data

[Andy Shevchenko]

The struct i2c_client pointer is used only to get driver data,
associated with a struct device or print messages on behalf.
Moreover, the very same pointer to a struct device is already
assigned by a regmap and can be retrieved from there.
No need to keep a duplicative pointer.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ov2740.c | 39 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index d5f0eabf20c6..c975db1bbe8c 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -77,7 +77,6 @@
 #define OV2740_REG_OTP_CUSTOMER		0x7010
 
 struct nvm_data {
-	struct i2c_client *client;
 	struct nvmem_device *nvmem;
 	struct regmap *regmap;
 	char *nvm_buffer;
@@ -649,34 +648,28 @@ static void ov2740_update_pad_format(const struct ov2740_mode *mode,
 
 static int ov2740_load_otp_data(struct nvm_data *nvm)
 {
-	struct i2c_client *client;
-	struct ov2740 *ov2740;
+	struct device *dev = regmap_get_device(nvm->regmap);
+	struct ov2740 *ov2740 = to_ov2740(dev_get_drvdata(dev));
 	u32 isp_ctrl00 = 0;
 	u32 isp_ctrl01 = 0;
 	int ret;
 
-	if (!nvm)
-		return -EINVAL;
-
 	if (nvm->nvm_buffer)
 		return 0;
 
-	client = nvm->client;
-	ov2740 = to_ov2740(i2c_get_clientdata(client));
-
 	nvm->nvm_buffer = kzalloc(CUSTOMER_USE_OTP_SIZE, GFP_KERNEL);
 	if (!nvm->nvm_buffer)
 		return -ENOMEM;
 
 	ret = ov2740_read_reg(ov2740, OV2740_REG_ISP_CTRL00, 1, &isp_ctrl00);
 	if (ret) {
-		dev_err(&client->dev, "failed to read ISP CTRL00\n");
+		dev_err(dev, "failed to read ISP CTRL00\n");
 		goto err;
 	}
 
 	ret = ov2740_read_reg(ov2740, OV2740_REG_ISP_CTRL01, 1, &isp_ctrl01);
 	if (ret) {
-		dev_err(&client->dev, "failed to read ISP CTRL01\n");
+		dev_err(dev, "failed to read ISP CTRL01\n");
 		goto err;
 	}
 
@@ -684,7 +677,7 @@ static int ov2740_load_otp_data(struct nvm_data *nvm)
 	ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL00, 1,
 			       isp_ctrl00 & ~BIT(5));
 	if (ret) {
-		dev_err(&client->dev, "failed to set ISP CTRL00\n");
+		dev_err(dev, "failed to set ISP CTRL00\n");
 		goto err;
 	}
 
@@ -692,14 +685,14 @@ static int ov2740_load_otp_data(struct nvm_data *nvm)
 	ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL01, 1,
 			       isp_ctrl01 & ~BIT(7));
 	if (ret) {
-		dev_err(&client->dev, "failed to set ISP CTRL01\n");
+		dev_err(dev, "failed to set ISP CTRL01\n");
 		goto err;
 	}
 
 	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
 			       OV2740_MODE_STREAMING);
 	if (ret) {
-		dev_err(&client->dev, "failed to set streaming mode\n");
+		dev_err(dev, "failed to set streaming mode\n");
 		goto err;
 	}
 
@@ -712,26 +705,26 @@ static int ov2740_load_otp_data(struct nvm_data *nvm)
 	ret = regmap_bulk_read(nvm->regmap, OV2740_REG_OTP_CUSTOMER,
 			       nvm->nvm_buffer, CUSTOMER_USE_OTP_SIZE);
 	if (ret) {
-		dev_err(&client->dev, "failed to read OTP data, ret %d\n", ret);
+		dev_err(dev, "failed to read OTP data, ret %d\n", ret);
 		goto err;
 	}
 
 	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
 			       OV2740_MODE_STANDBY);
 	if (ret) {
-		dev_err(&client->dev, "failed to set streaming mode\n");
+		dev_err(dev, "failed to set streaming mode\n");
 		goto err;
 	}
 
 	ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL01, 1, isp_ctrl01);
 	if (ret) {
-		dev_err(&client->dev, "failed to set ISP CTRL01\n");
+		dev_err(dev, "failed to set ISP CTRL01\n");
 		goto err;
 	}
 
 	ret = ov2740_write_reg(ov2740, OV2740_REG_ISP_CTRL00, 1, isp_ctrl00);
 	if (ret) {
-		dev_err(&client->dev, "failed to set ISP CTRL00\n");
+		dev_err(dev, "failed to set ISP CTRL00\n");
 		goto err;
 	}
 
@@ -746,7 +739,6 @@ static int ov2740_load_otp_data(struct nvm_data *nvm)
 static int ov2740_start_streaming(struct ov2740 *ov2740)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
-	struct nvm_data *nvm = ov2740->nvm;
 	const struct ov2740_reg_list *reg_list;
 	int link_freq_index;
 	int ret = 0;
@@ -755,7 +747,8 @@ static int ov2740_start_streaming(struct ov2740 *ov2740)
 	if (ret)
 		return ret;
 
-	ov2740_load_otp_data(nvm);
+	if (ov2740->nvm)
+		ov2740_load_otp_data(ov2740->nvm);
 
 	link_freq_index = ov2740->cur_mode->link_freq_index;
 	reg_list = &link_freq_configs[link_freq_index].reg_list;
@@ -1071,9 +1064,8 @@ static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
 			     size_t count)
 {
 	struct nvm_data *nvm = priv;
-	struct v4l2_subdev *sd = i2c_get_clientdata(nvm->client);
-	struct device *dev = &nvm->client->dev;
-	struct ov2740 *ov2740 = to_ov2740(sd);
+	struct device *dev = regmap_get_device(nvm->regmap);
+	struct ov2740 *ov2740 = to_ov2740(dev_get_drvdata(dev));
 	int ret = 0;
 
 	mutex_lock(&ov2740->mutex);
@@ -1120,7 +1112,6 @@ static int ov2740_register_nvmem(struct i2c_client *client,
 		return PTR_ERR(regmap);
 
 	nvm->regmap = regmap;
-	nvm->client = client;
 
 	nvmem_config.name = dev_name(dev);
 	nvmem_config.dev = dev;
-- 
2.35.1

[PATCH v1 2/8] media: ov2740: Replace voodoo coding with understandle flow

[Andy Shevchenko]

Besides not being understandable at the first glance, the code
might provoke a compiler or a static analyser tool to warn about
out-of-bound access (when len == 0).

Replace it with clear flow an understandable intention.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ov2740.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index c975db1bbe8c..81c0ab220339 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -377,10 +377,10 @@ static int ov2740_read_reg(struct ov2740 *ov2740, u16 reg, u16 len, u32 *val)
 	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
 	struct i2c_msg msgs[2];
 	u8 addr_buf[2];
-	u8 data_buf[4] = {0};
+	u8 data_buf[4];
 	int ret = 0;
 
-	if (len > sizeof(data_buf))
+	if (len > 4)
 		return -EINVAL;
 
 	put_unaligned_be16(reg, addr_buf);
@@ -391,13 +391,22 @@ static int ov2740_read_reg(struct ov2740 *ov2740, u16 reg, u16 len, u32 *val)
 	msgs[1].addr = client->addr;
 	msgs[1].flags = I2C_M_RD;
 	msgs[1].len = len;
-	msgs[1].buf = &data_buf[sizeof(data_buf) - len];
+	msgs[1].buf = data_buf;
 
 	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (ret != ARRAY_SIZE(msgs))
 		return ret < 0 ? ret : -EIO;
 
-	*val = get_unaligned_be32(data_buf);
+	if (len == 4)
+		*val = get_unaligned_be32(data_buf);
+	else if (len == 3)
+		*val = get_unaligned_be24(data_buf);
+	else if (len == 2)
+		*val = get_unaligned_be16(data_buf);
+	else if (len == 1)
+		*val = data_buf[0];
+	else
+		return -EINVAL;
 
 	return 0;
 }
@@ -412,7 +421,16 @@ static int ov2740_write_reg(struct ov2740 *ov2740, u16 reg, u16 len, u32 val)
 		return -EINVAL;
 
 	put_unaligned_be16(reg, buf);
-	put_unaligned_be32(val << 8 * (4 - len), buf + 2);
+	if (len == 4)
+		put_unaligned_be32(val, buf + 2);
+	else if (len == 3)
+		put_unaligned_be24(val, buf + 2);
+	else if (len == 2)
+		put_unaligned_be16(val, buf + 2);
+	else if (len == 1)
+		buf[2] = val;
+	else
+		return -EINVAL;
 
 	ret = i2c_master_send(client, buf, len + 2);
 	if (ret != len + 2)
-- 
2.35.1

[PATCH v1 3/8] media: ov2740: Switch from __maybe_unused to pm_sleep_ptr() etc

[Andy Shevchenko]

Letting the compiler remove these functions when the kernel is built
without CONFIG_PM_SLEEP support is simpler and less heavier for builds
than the use of __maybe_unused attributes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ov2740.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 81c0ab220339..9420258c6073 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -838,7 +838,7 @@ static int ov2740_set_stream(struct v4l2_subdev *sd, int enable)
 	return ret;
 }
 
-static int __maybe_unused ov2740_suspend(struct device *dev)
+static int ov2740_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov2740 *ov2740 = to_ov2740(sd);
@@ -852,7 +852,7 @@ static int __maybe_unused ov2740_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused ov2740_resume(struct device *dev)
+static int ov2740_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov2740 *ov2740 = to_ov2740(sd);
@@ -1229,9 +1229,7 @@ static int ov2740_probe(struct i2c_client *client)
 	return ret;
 }
 
-static const struct dev_pm_ops ov2740_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(ov2740_suspend, ov2740_resume)
-};
+static DEFINE_SIMPLE_DEV_PM_OPS(ov2740_pm_ops, ov2740_suspend, ov2740_resume);
 
 static const struct acpi_device_id ov2740_acpi_ids[] = {
 	{"INT3474"},
@@ -1243,7 +1241,7 @@ MODULE_DEVICE_TABLE(acpi, ov2740_acpi_ids);
 static struct i2c_driver ov2740_i2c_driver = {
 	.driver = {
 		.name = "ov2740",
-		.pm = &ov2740_pm_ops,
+		.pm = pm_sleep_ptr(&ov2740_pm_ops),
 		.acpi_match_table = ov2740_acpi_ids,
 	},
 	.probe_new = ov2740_probe,
-- 
2.35.1

[PATCH v1 4/8] media: ov2740: Remove duplicate check for NULL fwnode

[Andy Shevchenko]

fwnode API does proper checks and returns correct codes, no need
to repeat it in the caller.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ov2740.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 9420258c6073..07fe8fda274e 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -1009,9 +1009,6 @@ static int ov2740_check_hwcfg(struct device *dev)
 	int ret;
 	unsigned int i, j;
 
-	if (!fwnode)
-		return -ENXIO;
-
 	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
 	if (ret)
 		return ret;
-- 
2.35.1

[PATCH v1 5/8] media: ov2740: Drop redundant assignments of ret = 0

[Andy Shevchenko]

In some cases it might hide real bugs, in most cases here it's just
redundant as it's being reassigned immediately after initial assignment.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ov2740.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 07fe8fda274e..212190cb14e4 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -378,7 +378,7 @@ static int ov2740_read_reg(struct ov2740 *ov2740, u16 reg, u16 len, u32 *val)
 	struct i2c_msg msgs[2];
 	u8 addr_buf[2];
 	u8 data_buf[4];
-	int ret = 0;
+	int ret;
 
 	if (len > 4)
 		return -EINVAL;
@@ -415,7 +415,7 @@ static int ov2740_write_reg(struct ov2740 *ov2740, u16 reg, u16 len, u32 val)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
 	u8 buf[6];
-	int ret = 0;
+	int ret;
 
 	if (len > 4)
 		return -EINVAL;
@@ -444,7 +444,7 @@ static int ov2740_write_reg_list(struct ov2740 *ov2740,
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
 	unsigned int i;
-	int ret = 0;
+	int ret;
 
 	for (i = 0; i < r_list->num_of_regs; i++) {
 		ret = ov2740_write_reg(ov2740, r_list->regs[i].address, 1,
@@ -486,7 +486,7 @@ static int ov2740_identify_module(struct ov2740 *ov2740)
 
 static int ov2740_update_digital_gain(struct ov2740 *ov2740, u32 d_gain)
 {
-	int ret = 0;
+	int ret;
 
 	ret = ov2740_write_reg(ov2740, OV2740_REG_GROUP_ACCESS, 1,
 			       OV2740_GROUP_HOLD_START);
@@ -530,7 +530,7 @@ static int ov2740_set_ctrl(struct v4l2_ctrl *ctrl)
 					     struct ov2740, ctrl_handler);
 	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
 	s64 exposure_max;
-	int ret = 0;
+	int ret;
 
 	/* Propagate change of current control to all related controls */
 	if (ctrl->id == V4L2_CID_VBLANK) {
@@ -593,7 +593,7 @@ static int ov2740_init_controls(struct ov2740 *ov2740)
 	s64 exposure_max, h_blank, pixel_rate;
 	u32 vblank_min, vblank_max, vblank_default;
 	int size;
-	int ret = 0;
+	int ret;
 
 	ctrl_hdlr = &ov2740->ctrl_handler;
 	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
@@ -759,7 +759,7 @@ static int ov2740_start_streaming(struct ov2740 *ov2740)
 	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
 	const struct ov2740_reg_list *reg_list;
 	int link_freq_index;
-	int ret = 0;
+	int ret;
 
 	ret = ov2740_identify_module(ov2740);
 	if (ret)
@@ -1154,8 +1154,8 @@ static int ov2740_register_nvmem(struct i2c_client *client,
 static int ov2740_probe(struct i2c_client *client)
 {
 	struct ov2740 *ov2740;
-	int ret = 0;
 	bool full_power;
+	int ret;
 
 	ret = ov2740_check_hwcfg(&client->dev);
 	if (ret) {
-- 
2.35.1

[PATCH v1 6/8] media: ov2740: Switch to use dev_err_probe()

[Andy Shevchenko]

Switch to use dev_err_probe() to simpify error path and unify message
template.

While at it, add missed \n to the end of the messages.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ov2740.c | 44 ++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 212190cb14e4..7271e3d011c9 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -1013,10 +1013,10 @@ static int ov2740_check_hwcfg(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (mclk != OV2740_MCLK) {
-		dev_err(dev, "external clock %d is not supported", mclk);
-		return -EINVAL;
-	}
+	if (mclk != OV2740_MCLK)
+		return dev_err_probe(dev, -EINVAL,
+				     "external clock %d is not supported\n",
+				     mclk);
 
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep)
@@ -1028,15 +1028,14 @@ static int ov2740_check_hwcfg(struct device *dev)
 		return ret;
 
 	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV2740_DATA_LANES) {
-		dev_err(dev, "number of CSI2 data lanes %d is not supported",
-			bus_cfg.bus.mipi_csi2.num_data_lanes);
-		ret = -EINVAL;
+		ret = dev_err_probe(dev, -EINVAL,
+				    "number of CSI2 data lanes %d is not supported\n",
+				    bus_cfg.bus.mipi_csi2.num_data_lanes);
 		goto check_hwcfg_error;
 	}
 
 	if (!bus_cfg.nr_of_link_frequencies) {
-		dev_err(dev, "no link frequencies defined");
-		ret = -EINVAL;
+		ret = dev_err_probe(dev, -EINVAL, "no link frequencies defined\n");
 		goto check_hwcfg_error;
 	}
 
@@ -1048,9 +1047,9 @@ static int ov2740_check_hwcfg(struct device *dev)
 		}
 
 		if (j == bus_cfg.nr_of_link_frequencies) {
-			dev_err(dev, "no link frequency %lld supported",
-				link_freq_menu_items[i]);
-			ret = -EINVAL;
+			ret = dev_err_probe(dev, -EINVAL,
+					    "no link frequency %lld supported\n",
+					    link_freq_menu_items[i]);
 			goto check_hwcfg_error;
 		}
 	}
@@ -1153,16 +1152,14 @@ static int ov2740_register_nvmem(struct i2c_client *client,
 
 static int ov2740_probe(struct i2c_client *client)
 {
+	struct device *dev = &client->dev;
 	struct ov2740 *ov2740;
 	bool full_power;
 	int ret;
 
 	ret = ov2740_check_hwcfg(&client->dev);
-	if (ret) {
-		dev_err(&client->dev, "failed to check HW configuration: %d",
-			ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to check HW configuration\n");
 
 	ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL);
 	if (!ov2740)
@@ -1172,17 +1169,15 @@ static int ov2740_probe(struct i2c_client *client)
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		ret = ov2740_identify_module(ov2740);
-		if (ret) {
-			dev_err(&client->dev, "failed to find sensor: %d", ret);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to find sensor\n");
 	}
 
 	mutex_init(&ov2740->mutex);
 	ov2740->cur_mode = &supported_modes[0];
 	ret = ov2740_init_controls(ov2740);
 	if (ret) {
-		dev_err(&client->dev, "failed to init controls: %d", ret);
+		dev_err_probe(dev, ret, "failed to init controls\n");
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
@@ -1193,14 +1188,13 @@ static int ov2740_probe(struct i2c_client *client)
 	ov2740->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&ov2740->sd.entity, 1, &ov2740->pad);
 	if (ret) {
-		dev_err(&client->dev, "failed to init entity pads: %d", ret);
+		dev_err_probe(dev, ret, "failed to init entity pads\n");
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
 	ret = v4l2_async_register_subdev_sensor(&ov2740->sd);
 	if (ret < 0) {
-		dev_err(&client->dev, "failed to register V4L2 subdev: %d",
-			ret);
+		dev_err_probe(dev, ret, "failed to register V4L2 subdev\n");
 		goto probe_error_media_entity_cleanup;
 	}
 
-- 
2.35.1

[PATCH v1 7/8] media: ov2740: Add missed \n to the end of the messages

[Andy Shevchenko]

Add missed \n to the end of the messages.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ov2740.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 7271e3d011c9..c4c511f90257 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -451,7 +451,7 @@ static int ov2740_write_reg_list(struct ov2740 *ov2740,
 				       r_list->regs[i].val);
 		if (ret) {
 			dev_err_ratelimited(&client->dev,
-					    "write reg 0x%4.4x return err = %d",
+					    "write reg 0x%4.4x return err = %d\n",
 					    r_list->regs[i].address, ret);
 			return ret;
 		}
@@ -474,7 +474,7 @@ static int ov2740_identify_module(struct ov2740 *ov2740)
 		return ret;
 
 	if (val != OV2740_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x",
+		dev_err(&client->dev, "chip id mismatch: %x != %x\n",
 			OV2740_CHIP_ID, val);
 		return -ENXIO;
 	}
@@ -772,14 +772,14 @@ static int ov2740_start_streaming(struct ov2740 *ov2740)
 	reg_list = &link_freq_configs[link_freq_index].reg_list;
 	ret = ov2740_write_reg_list(ov2740, reg_list);
 	if (ret) {
-		dev_err(&client->dev, "failed to set plls");
+		dev_err(&client->dev, "failed to set plls\n");
 		return ret;
 	}
 
 	reg_list = &ov2740->cur_mode->reg_list;
 	ret = ov2740_write_reg_list(ov2740, reg_list);
 	if (ret) {
-		dev_err(&client->dev, "failed to set mode");
+		dev_err(&client->dev, "failed to set mode\n");
 		return ret;
 	}
 
@@ -790,7 +790,7 @@ static int ov2740_start_streaming(struct ov2740 *ov2740)
 	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
 			       OV2740_MODE_STREAMING);
 	if (ret)
-		dev_err(&client->dev, "failed to start streaming");
+		dev_err(&client->dev, "failed to start streaming\n");
 
 	return ret;
 }
@@ -801,7 +801,7 @@ static void ov2740_stop_streaming(struct ov2740 *ov2740)
 
 	if (ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
 			     OV2740_MODE_STANDBY))
-		dev_err(&client->dev, "failed to stop streaming");
+		dev_err(&client->dev, "failed to stop streaming\n");
 }
 
 static int ov2740_set_stream(struct v4l2_subdev *sd, int enable)
-- 
2.35.1

[PATCH v1 8/8] media: ov2740: Use traditional pattern when checking error codes

[Andy Shevchenko]

Instead of 'if (!ret)' switch to "check for the error first" rule.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/media/i2c/ov2740.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index c4c511f90257..afd591be6526 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -1112,7 +1112,6 @@ static int ov2740_register_nvmem(struct i2c_client *client,
 	struct nvmem_config nvmem_config = { };
 	struct regmap *regmap;
 	struct device *dev = &client->dev;
-	int ret;
 
 	nvm = devm_kzalloc(dev, sizeof(*nvm), GFP_KERNEL);
 	if (!nvm)
@@ -1142,12 +1141,11 @@ static int ov2740_register_nvmem(struct i2c_client *client,
 	nvmem_config.size = CUSTOMER_USE_OTP_SIZE;
 
 	nvm->nvmem = devm_nvmem_register(dev, &nvmem_config);
+	if (IS_ERR(nvm->nvmem))
+		return PTR_ERR(nvm->nvmem);
 
-	ret = PTR_ERR_OR_ZERO(nvm->nvmem);
-	if (!ret)
-		ov2740->nvm = nvm;
-
-	return ret;
+	ov2740->nvm = nvm;
+	return 0;
 }
 
 static int ov2740_probe(struct i2c_client *client)
-- 
2.35.1

RE: [PATCH v1 4/8] media: ov2740: Remove duplicate check for NULL fwnode

[Cao, Bingbu]

Hi, Andy

Thanks for your patch.

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

________________________
BRs,  
Bingbu Cao 

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Tuesday, July 26, 2022 8:06 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Qiu, Tian Shu <tian.shu.qiu@intel.com>; Tu, ShawnX
> <shawnx.tu@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>; Mauro Carvalho
> Chehab <mchehab@kernel.org>
> Subject: [PATCH v1 4/8] media: ov2740: Remove duplicate check for NULL
> fwnode
> 
> fwnode API does proper checks and returns correct codes, no need to repeat
> it in the caller.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index
> 9420258c6073..07fe8fda274e 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -1009,9 +1009,6 @@ static int ov2740_check_hwcfg(struct device *dev)
>  	int ret;
>  	unsigned int i, j;
> 
> -	if (!fwnode)
> -		return -ENXIO;
> -
>  	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
>  	if (ret)
>  		return ret;
> --
> 2.35.1

RE: [PATCH v1 2/8] media: ov2740: Replace voodoo coding with understandle flow

[Cao, Bingbu]

Andy, 

Thanks for your patch.

Although I am not familiar with the voodoo programming, it looks
good for me.

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

________________________
BRs,  
Bingbu Cao 

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Tuesday, July 26, 2022 8:06 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Qiu, Tian Shu <tian.shu.qiu@intel.com>; Tu, ShawnX
> <shawnx.tu@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>; Mauro Carvalho
> Chehab <mchehab@kernel.org>
> Subject: [PATCH v1 2/8] media: ov2740: Replace voodoo coding with
> understandle flow
> 
> Besides not being understandable at the first glance, the code might
> provoke a compiler or a static analyser tool to warn about out-of-bound
> access (when len == 0).
> 
> Replace it with clear flow an understandable intention.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index
> c975db1bbe8c..81c0ab220339 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -377,10 +377,10 @@ static int ov2740_read_reg(struct ov2740 *ov2740,
> u16 reg, u16 len, u32 *val)
>  	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
>  	struct i2c_msg msgs[2];
>  	u8 addr_buf[2];
> -	u8 data_buf[4] = {0};
> +	u8 data_buf[4];
>  	int ret = 0;
> 
> -	if (len > sizeof(data_buf))
> +	if (len > 4)
>  		return -EINVAL;
> 
>  	put_unaligned_be16(reg, addr_buf);
> @@ -391,13 +391,22 @@ static int ov2740_read_reg(struct ov2740 *ov2740,
> u16 reg, u16 len, u32 *val)
>  	msgs[1].addr = client->addr;
>  	msgs[1].flags = I2C_M_RD;
>  	msgs[1].len = len;
> -	msgs[1].buf = &data_buf[sizeof(data_buf) - len];
> +	msgs[1].buf = data_buf;
> 
>  	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>  	if (ret != ARRAY_SIZE(msgs))
>  		return ret < 0 ? ret : -EIO;
> 
> -	*val = get_unaligned_be32(data_buf);
> +	if (len == 4)
> +		*val = get_unaligned_be32(data_buf);
> +	else if (len == 3)
> +		*val = get_unaligned_be24(data_buf);
> +	else if (len == 2)
> +		*val = get_unaligned_be16(data_buf);
> +	else if (len == 1)
> +		*val = data_buf[0];
> +	else
> +		return -EINVAL;
> 
>  	return 0;
>  }
> @@ -412,7 +421,16 @@ static int ov2740_write_reg(struct ov2740 *ov2740,
> u16 reg, u16 len, u32 val)
>  		return -EINVAL;
> 
>  	put_unaligned_be16(reg, buf);
> -	put_unaligned_be32(val << 8 * (4 - len), buf + 2);
> +	if (len == 4)
> +		put_unaligned_be32(val, buf + 2);
> +	else if (len == 3)
> +		put_unaligned_be24(val, buf + 2);
> +	else if (len == 2)
> +		put_unaligned_be16(val, buf + 2);
> +	else if (len == 1)
> +		buf[2] = val;
> +	else
> +		return -EINVAL;
> 
>  	ret = i2c_master_send(client, buf, len + 2);
>  	if (ret != len + 2)
> --
> 2.35.1

RE: [PATCH v1 4/8] media: ov2740: Remove duplicate check for NULL fwnode

[Cao, Bingbu]

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

________________________
BRs,  
Bingbu Cao 

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Tuesday, July 26, 2022 8:06 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Qiu, Tian Shu <tian.shu.qiu@intel.com>; Tu, ShawnX
> <shawnx.tu@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>; Mauro Carvalho
> Chehab <mchehab@kernel.org>
> Subject: [PATCH v1 4/8] media: ov2740: Remove duplicate check for NULL
> fwnode
> 
> fwnode API does proper checks and returns correct codes, no need to repeat
> it in the caller.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index
> 9420258c6073..07fe8fda274e 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -1009,9 +1009,6 @@ static int ov2740_check_hwcfg(struct device *dev)
>  	int ret;
>  	unsigned int i, j;
> 
> -	if (!fwnode)
> -		return -ENXIO;
> -
>  	ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
>  	if (ret)
>  		return ret;
> --
> 2.35.1

RE: [PATCH v1 7/8] media: ov2740: Add missed \n to the end of the messages

[Cao, Bingbu]

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

________________________
BRs,  
Bingbu Cao 

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Tuesday, July 26, 2022 8:06 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Qiu, Tian Shu <tian.shu.qiu@intel.com>; Tu, ShawnX
> <shawnx.tu@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>; Mauro Carvalho
> Chehab <mchehab@kernel.org>
> Subject: [PATCH v1 7/8] media: ov2740: Add missed \n to the end of the
> messages
> 
> Add missed \n to the end of the messages.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index
> 7271e3d011c9..c4c511f90257 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -451,7 +451,7 @@ static int ov2740_write_reg_list(struct ov2740 *ov2740,
>  				       r_list->regs[i].val);
>  		if (ret) {
>  			dev_err_ratelimited(&client->dev,
> -					    "write reg 0x%4.4x return err = %d",
> +					    "write reg 0x%4.4x return err = %d\n",
>  					    r_list->regs[i].address, ret);
>  			return ret;
>  		}
> @@ -474,7 +474,7 @@ static int ov2740_identify_module(struct ov2740
> *ov2740)
>  		return ret;
> 
>  	if (val != OV2740_CHIP_ID) {
> -		dev_err(&client->dev, "chip id mismatch: %x!=%x",
> +		dev_err(&client->dev, "chip id mismatch: %x != %x\n",
>  			OV2740_CHIP_ID, val);
>  		return -ENXIO;
>  	}
> @@ -772,14 +772,14 @@ static int ov2740_start_streaming(struct ov2740
> *ov2740)
>  	reg_list = &link_freq_configs[link_freq_index].reg_list;
>  	ret = ov2740_write_reg_list(ov2740, reg_list);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to set plls");
> +		dev_err(&client->dev, "failed to set plls\n");
>  		return ret;
>  	}
> 
>  	reg_list = &ov2740->cur_mode->reg_list;
>  	ret = ov2740_write_reg_list(ov2740, reg_list);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to set mode");
> +		dev_err(&client->dev, "failed to set mode\n");
>  		return ret;
>  	}
> 
> @@ -790,7 +790,7 @@ static int ov2740_start_streaming(struct ov2740
> *ov2740)
>  	ret = ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
>  			       OV2740_MODE_STREAMING);
>  	if (ret)
> -		dev_err(&client->dev, "failed to start streaming");
> +		dev_err(&client->dev, "failed to start streaming\n");
> 
>  	return ret;
>  }
> @@ -801,7 +801,7 @@ static void ov2740_stop_streaming(struct ov2740
> *ov2740)
> 
>  	if (ov2740_write_reg(ov2740, OV2740_REG_MODE_SELECT, 1,
>  			     OV2740_MODE_STANDBY))
> -		dev_err(&client->dev, "failed to stop streaming");
> +		dev_err(&client->dev, "failed to stop streaming\n");
>  }
> 
>  static int ov2740_set_stream(struct v4l2_subdev *sd, int enable)
> --
> 2.35.1

RE: [PATCH v1 6/8] media: ov2740: Switch to use dev_err_probe()

[Cao, Bingbu]

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
________________________
BRs,  
Bingbu Cao 

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Tuesday, July 26, 2022 8:06 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Qiu, Tian Shu <tian.shu.qiu@intel.com>; Tu, ShawnX
> <shawnx.tu@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>; Mauro Carvalho
> Chehab <mchehab@kernel.org>
> Subject: [PATCH v1 6/8] media: ov2740: Switch to use dev_err_probe()
> 
> Switch to use dev_err_probe() to simpify error path and unify message
> template.
> 
> While at it, add missed \n to the end of the messages.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/media/i2c/ov2740.c | 44 ++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index
> 212190cb14e4..7271e3d011c9 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -1013,10 +1013,10 @@ static int ov2740_check_hwcfg(struct device *dev)
>  	if (ret)
>  		return ret;
> 
> -	if (mclk != OV2740_MCLK) {
> -		dev_err(dev, "external clock %d is not supported", mclk);
> -		return -EINVAL;
> -	}
> +	if (mclk != OV2740_MCLK)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "external clock %d is not supported\n",
> +				     mclk);
> 
>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>  	if (!ep)
> @@ -1028,15 +1028,14 @@ static int ov2740_check_hwcfg(struct device *dev)
>  		return ret;
> 
>  	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV2740_DATA_LANES) {
> -		dev_err(dev, "number of CSI2 data lanes %d is not supported",
> -			bus_cfg.bus.mipi_csi2.num_data_lanes);
> -		ret = -EINVAL;
> +		ret = dev_err_probe(dev, -EINVAL,
> +				    "number of CSI2 data lanes %d is not
> supported\n",
> +				    bus_cfg.bus.mipi_csi2.num_data_lanes);
>  		goto check_hwcfg_error;
>  	}
> 
>  	if (!bus_cfg.nr_of_link_frequencies) {
> -		dev_err(dev, "no link frequencies defined");
> -		ret = -EINVAL;
> +		ret = dev_err_probe(dev, -EINVAL, "no link frequencies
> defined\n");
>  		goto check_hwcfg_error;
>  	}
> 
> @@ -1048,9 +1047,9 @@ static int ov2740_check_hwcfg(struct device *dev)
>  		}
> 
>  		if (j == bus_cfg.nr_of_link_frequencies) {
> -			dev_err(dev, "no link frequency %lld supported",
> -				link_freq_menu_items[i]);
> -			ret = -EINVAL;
> +			ret = dev_err_probe(dev, -EINVAL,
> +					    "no link frequency %lld supported\n",
> +					    link_freq_menu_items[i]);
>  			goto check_hwcfg_error;
>  		}
>  	}
> @@ -1153,16 +1152,14 @@ static int ov2740_register_nvmem(struct i2c_client
> *client,
> 
>  static int ov2740_probe(struct i2c_client *client)  {
> +	struct device *dev = &client->dev;
>  	struct ov2740 *ov2740;
>  	bool full_power;
>  	int ret;
> 
>  	ret = ov2740_check_hwcfg(&client->dev);
> -	if (ret) {
> -		dev_err(&client->dev, "failed to check HW configuration: %d",
> -			ret);
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to check HW
> configuration\n");
> 
>  	ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL);
>  	if (!ov2740)
> @@ -1172,17 +1169,15 @@ static int ov2740_probe(struct i2c_client *client)
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		ret = ov2740_identify_module(ov2740);
> -		if (ret) {
> -			dev_err(&client->dev, "failed to find sensor: %d",
> ret);
> -			return ret;
> -		}
> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to find
> sensor\n");
>  	}
> 
>  	mutex_init(&ov2740->mutex);
>  	ov2740->cur_mode = &supported_modes[0];
>  	ret = ov2740_init_controls(ov2740);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to init controls: %d", ret);
> +		dev_err_probe(dev, ret, "failed to init controls\n");
>  		goto probe_error_v4l2_ctrl_handler_free;
>  	}
> 
> @@ -1193,14 +1188,13 @@ static int ov2740_probe(struct i2c_client *client)
>  	ov2740->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&ov2740->sd.entity, 1, &ov2740->pad);
>  	if (ret) {
> -		dev_err(&client->dev, "failed to init entity pads: %d", ret);
> +		dev_err_probe(dev, ret, "failed to init entity pads\n");
>  		goto probe_error_v4l2_ctrl_handler_free;
>  	}
> 
>  	ret = v4l2_async_register_subdev_sensor(&ov2740->sd);
>  	if (ret < 0) {
> -		dev_err(&client->dev, "failed to register V4L2 subdev: %d",
> -			ret);
> +		dev_err_probe(dev, ret, "failed to register V4L2 subdev\n");
>  		goto probe_error_media_entity_cleanup;
>  	}
> 
> --
> 2.35.1

Re: [PATCH v1 1/8] media: ov2740: Remove duplicative pointer in struct nvm_data

[Andy Shevchenko]

On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote:
> The struct i2c_client pointer is used only to get driver data,
> associated with a struct device or print messages on behalf.
> Moreover, the very same pointer to a struct device is already
> assigned by a regmap and can be retrieved from there.
> No need to keep a duplicative pointer.

Thanks, Bungbu, for the review. Can it be now applied?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/8] media: ov2740: Remove duplicative pointer in struct nvm_data

[Andy Shevchenko]

On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote:
> > The struct i2c_client pointer is used only to get driver data,
> > associated with a struct device or print messages on behalf.
> > Moreover, the very same pointer to a struct device is already
> > assigned by a regmap and can be retrieved from there.
> > No need to keep a duplicative pointer.
> 
> Thanks, Bungbu, for the review. Can it be now applied?

Don't see this being applied or commented why not...

Mauro? Or who is taking care of this driver nowadays?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/8] media: ov2740: Remove duplicative pointer in struct nvm_data

[Andy Shevchenko]

On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote:
> On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote:
> > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote:
> > > The struct i2c_client pointer is used only to get driver data,
> > > associated with a struct device or print messages on behalf.
> > > Moreover, the very same pointer to a struct device is already
> > > assigned by a regmap and can be retrieved from there.
> > > No need to keep a duplicative pointer.
> > 
> > Thanks, Bungbu, for the review. Can it be now applied?
> 
> Don't see this being applied or commented why not...
> 
> Mauro? Or who is taking care of this driver nowadays?

Okay, found a private response by Mauro where he tells that Sakari can take
care of this. Sakari, should I resend this to you with all tags applied?
Or you can use `b4` tool that allows to avoid unneeded resend.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/8] media: ov2740: Remove duplicative pointer in struct nvm_data

[Sakari Ailus]

On Fri, Nov 11, 2022 at 02:08:48PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote:
> > On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote:
> > > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote:
> > > > The struct i2c_client pointer is used only to get driver data,
> > > > associated with a struct device or print messages on behalf.
> > > > Moreover, the very same pointer to a struct device is already
> > > > assigned by a regmap and can be retrieved from there.
> > > > No need to keep a duplicative pointer.
> > > 
> > > Thanks, Bungbu, for the review. Can it be now applied?
> > 
> > Don't see this being applied or commented why not...
> > 
> > Mauro? Or who is taking care of this driver nowadays?
> 
> Okay, found a private response by Mauro where he tells that Sakari can take
> care of this. Sakari, should I resend this to you with all tags applied?
> Or you can use `b4` tool that allows to avoid unneeded resend.

No need to. But please cc me on the next time. I'll take a look now...

-- 
Sakari Ailus

Re: [PATCH v1 2/8] media: ov2740: Replace voodoo coding with understandle flow

[Sakari Ailus]

Hi Andy,

On Tue, Jul 26, 2022 at 03:05:50PM +0300, Andy Shevchenko wrote:
> Besides not being understandable at the first glance, the code
> might provoke a compiler or a static analyser tool to warn about
> out-of-bound access (when len == 0).

I've never seen one.

However the same pattern is repeatedly used by many, many drivers and
addressing just one doesn't make much sense.

The proper way to fix this would be to have a set of common CCI (Camera
Control Interface) functions that all drivers could use, and then switch
the drivers to use them.

This isn't currently a great fit for e.g. regmap but perhaps something
light on top of regmap-i2c could do the trick?

The rest of the set seems good to me.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v1 1/8] media: ov2740: Remove duplicative pointer in struct nvm_data

[Andy Shevchenko]

On Fri, Nov 11, 2022 at 02:58:45PM +0000, Sakari Ailus wrote:
> On Fri, Nov 11, 2022 at 02:08:48PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote:
> > > On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote:
> > > > > The struct i2c_client pointer is used only to get driver data,
> > > > > associated with a struct device or print messages on behalf.
> > > > > Moreover, the very same pointer to a struct device is already
> > > > > assigned by a regmap and can be retrieved from there.
> > > > > No need to keep a duplicative pointer.
> > > > 
> > > > Thanks, Bungbu, for the review. Can it be now applied?
> > > 
> > > Don't see this being applied or commented why not...
> > > 
> > > Mauro? Or who is taking care of this driver nowadays?
> > 
> > Okay, found a private response by Mauro where he tells that Sakari can take
> > care of this. Sakari, should I resend this to you with all tags applied?
> > Or you can use `b4` tool that allows to avoid unneeded resend.
> 
> No need to. But please cc me on the next time. I'll take a look now...

How should I know whom to Cc? Can we update MAINTAINERS accordingly, please?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/8] media: ov2740: Replace voodoo coding with understandle flow

[Andy Shevchenko]

On Fri, Nov 11, 2022 at 05:02:22PM +0200, Sakari Ailus wrote:
> Hi Andy,
> 
> On Tue, Jul 26, 2022 at 03:05:50PM +0300, Andy Shevchenko wrote:
> > Besides not being understandable at the first glance, the code
> > might provoke a compiler or a static analyser tool to warn about
> > out-of-bound access (when len == 0).
> 
> I've never seen one.
> 
> However the same pattern is repeatedly used by many, many drivers and
> addressing just one doesn't make much sense.
> 
> The proper way to fix this would be to have a set of common CCI (Camera
> Control Interface) functions that all drivers could use, and then switch
> the drivers to use them.
> 
> This isn't currently a great fit for e.g. regmap but perhaps something
> light on top of regmap-i2c could do the trick?

So, then we can skip this one, right?

> The rest of the set seems good to me.

Thank you for the review, can you apply them, or should I send a v2 with
dropped first patch?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/8] media: ov2740: Remove duplicative pointer in struct nvm_data

[Sakari Ailus]

Hi Andy,

On Fri, Nov 11, 2022 at 05:29:16PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 11, 2022 at 02:58:45PM +0000, Sakari Ailus wrote:
> > On Fri, Nov 11, 2022 at 02:08:48PM +0200, Andy Shevchenko wrote:
> > > On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote:
> > > > > > The struct i2c_client pointer is used only to get driver data,
> > > > > > associated with a struct device or print messages on behalf.
> > > > > > Moreover, the very same pointer to a struct device is already
> > > > > > assigned by a regmap and can be retrieved from there.
> > > > > > No need to keep a duplicative pointer.
> > > > > 
> > > > > Thanks, Bungbu, for the review. Can it be now applied?
> > > > 
> > > > Don't see this being applied or commented why not...
> > > > 
> > > > Mauro? Or who is taking care of this driver nowadays?
> > > 
> > > Okay, found a private response by Mauro where he tells that Sakari can take
> > > care of this. Sakari, should I resend this to you with all tags applied?
> > > Or you can use `b4` tool that allows to avoid unneeded resend.
> > 
> > No need to. But please cc me on the next time. I'll take a look now...
> 
> How should I know whom to Cc? Can we update MAINTAINERS accordingly, please?

Good question. In media tree we've listed the maintainers in wiki, as
the information would be hard to keep up-to-date file-wise:

<URL:https://www.linuxtv.org/wiki/index.php/Media_Maintainers>

So it helps if you cc me to camera sensor driver patches, but they're
neither ignored if you don't. It usually takes a little bit more time
but not nearly as much as this time.

Cc Hans.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v1 2/8] media: ov2740: Replace voodoo coding with understandle flow

[Sakari Ailus]

On Fri, Nov 11, 2022 at 05:30:09PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 11, 2022 at 05:02:22PM +0200, Sakari Ailus wrote:
> > Hi Andy,
> > 
> > On Tue, Jul 26, 2022 at 03:05:50PM +0300, Andy Shevchenko wrote:
> > > Besides not being understandable at the first glance, the code
> > > might provoke a compiler or a static analyser tool to warn about
> > > out-of-bound access (when len == 0).
> > 
> > I've never seen one.
> > 
> > However the same pattern is repeatedly used by many, many drivers and
> > addressing just one doesn't make much sense.
> > 
> > The proper way to fix this would be to have a set of common CCI (Camera
> > Control Interface) functions that all drivers could use, and then switch
> > the drivers to use them.
> > 
> > This isn't currently a great fit for e.g. regmap but perhaps something
> > light on top of regmap-i2c could do the trick?
> 
> So, then we can skip this one, right?

Yes.

> 
> > The rest of the set seems good to me.
> 
> Thank you for the review, can you apply them, or should I send a v2 with
> dropped first patch?

Already done. I'm still doing more testing before pushing.

Thanks!

-- 
Sakari Ailus

Re: [PATCH v1 1/8] media: ov2740: Remove duplicative pointer in struct nvm_data

[Andy Shevchenko]

On Fri, Nov 11, 2022 at 07:41:28PM +0000, Sakari Ailus wrote:
> On Fri, Nov 11, 2022 at 05:29:16PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 11, 2022 at 02:58:45PM +0000, Sakari Ailus wrote:
> > > On Fri, Nov 11, 2022 at 02:08:48PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Nov 11, 2022 at 02:05:37PM +0200, Andy Shevchenko wrote:
> > > > > On Tue, Aug 23, 2022 at 05:10:35PM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Jul 26, 2022 at 03:05:49PM +0300, Andy Shevchenko wrote:
> > > > > > > The struct i2c_client pointer is used only to get driver data,
> > > > > > > associated with a struct device or print messages on behalf.
> > > > > > > Moreover, the very same pointer to a struct device is already
> > > > > > > assigned by a regmap and can be retrieved from there.
> > > > > > > No need to keep a duplicative pointer.
> > > > > > 
> > > > > > Thanks, Bungbu, for the review. Can it be now applied?
> > > > > 
> > > > > Don't see this being applied or commented why not...
> > > > > 
> > > > > Mauro? Or who is taking care of this driver nowadays?
> > > > 
> > > > Okay, found a private response by Mauro where he tells that Sakari can take
> > > > care of this. Sakari, should I resend this to you with all tags applied?
> > > > Or you can use `b4` tool that allows to avoid unneeded resend.
> > > 
> > > No need to. But please cc me on the next time. I'll take a look now...
> > 
> > How should I know whom to Cc? Can we update MAINTAINERS accordingly, please?
> 
> Good question. In media tree we've listed the maintainers in wiki, as
> the information would be hard to keep up-to-date file-wise:
> 
> <URL:https://www.linuxtv.org/wiki/index.php/Media_Maintainers>

Unfortunately get_maintainer.pl doesn't know about this.

> So it helps if you cc me to camera sensor driver patches, but they're
> neither ignored if you don't. It usually takes a little bit more time
> but not nearly as much as this time.
> 
> Cc Hans.

-- 
With Best Regards,
Andy Shevchenko