Hi Frank,

On 1/19/26 16:35, Frank Li wrote:

On Mon, Jan 19, 2026 at 10:49:20AM +0100, Michael Riesch wrote:

quoted

Hi Frank,

Thanks for your review.

On 1/16/26 17:08, Frank Li wrote:

quoted

On Fri, Jan 16, 2026 at 02:02:47PM +0100, Michael Riesch wrote:

quoted

The Synopsys DesignWare MIPI CSI-2 Receiver is a CSI-2 bridge with
one input port and one output port. It receives the data with the
help of an external MIPI PHY (C-PHY or D-PHY) and passes it to e.g.,
the Rockchip Video Capture (VICAP) block on recent Rockchip SoCs.

Add a V4L2 subdevice driver for this unit.

Signed-off-by: Michael Riesch <redacted>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>
Signed-off-by: Michael Riesch <michael.riesch@collabora.com>
---

...

quoted

+
+static inline struct dw_mipi_csi2_device *to_csi2(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct dw_mipi_csi2_device, sd);
+}
+
+static inline __maybe_unused void

why need '__maybe_unused', needn't inline. compiler can auto decide and
report unused function if no 'inline'.

The __maybe_unused was helpful during development and is not really
required now. It doesn't hurt either, so I left it in. I can remove it
if you wish.

quoted

quoted

+
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);

use  struct fwnode_handle *ep __free(fwnode_handle) can simplify err
handler.

Sorry, I don't see the benefit of that.

I remember reduce one goto

Since Sakari requested this variant as well, I changed it to as you
suggested in v7.

quoted

quoted

quoted

+	if (!ep)
+		return dev_err_probe(dev, -ENODEV, "failed to get endpoint\n");
+

...

quoted

+{
+	struct media_pad *pads = csi2->pads;
+	struct v4l2_subdev *sd = &csi2->sd;
+	int ret;
+
+	ret = dw_mipi_csi2_register_notifier(csi2);
+	if (ret)
+		goto err;
+
+	v4l2_subdev_init(sd, &dw_mipi_csi2_ops);
+	sd->dev = csi2->dev;
+	sd->entity.ops = &dw_mipi_csi2_media_ops;
+	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
+
+static int dw_mipi_csi2_runtime_resume(struct device *dev)
+{
+	struct dw_mipi_csi2_device *csi2 = dev_get_drvdata(dev);
+	int ret;
+
+	reset_control_assert(csi2->reset);
+	udelay(5);

Now prefer use fsleep(), which auto choose difference sleep function
according to delay number.

I'll keep that in mind, but here the first thing that fsleep does is to
check whether the parameter is <= 10 and (since this is true) call
udelay. So here I don't see the point really.

Thank.

quoted

quoted

quoted

+	reset_control_deassert(csi2->reset);
+
+	ret = clk_bulk_prepare_enable(csi2->clks_num, csi2->clks);
+	if (ret) {
+		dev_err(dev, "failed to enable clocks\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(dw_mipi_csi2_pm_ops,
+				 dw_mipi_csi2_runtime_suspend,
+				 dw_mipi_csi2_runtime_resume, NULL);
+
+static struct platform_driver dw_mipi_csi2_drv = {
+	.driver = {
+		.name = "dw-mipi-csi2",
+		.of_match_table = dw_mipi_csi2_of_match,
+		.pm = &dw_mipi_csi2_pm_ops,

pm_ptr( &dw_mipi_csi2_pm_ops)

Shouldn't make a difference here since this driver depends on CONFIG_PM.

Avoid some static scan tools to report the problem, no harmful to add
pm_ptr().

Interesting. Which tools to you use?

I hope v7 is accepted as sent earlier today, and this should not be a
blocker. We can add pm_ptr() easily in a follow-up patch.

Thanks and regards,
Michael

Frank

quoted

Best regards,
Michael

quoted

Frank

quoted

+	},
+	.probe = dw_mipi_csi2_probe,
+	.remove = dw_mipi_csi2_remove,
+};
+module_platform_driver(dw_mipi_csi2_drv);
+
+MODULE_DESCRIPTION("Synopsys DesignWare MIPI CSI-2 Receiver platform driver");
+MODULE_LICENSE("GPL");

--
2.39.5