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

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

+dw_mipi_csi2_write(struct dw_mipi_csi2_device *csi2, unsigned int addr, u32 val)
+{
+	writel(val, csi2->base_addr + addr);
+}
+
+static inline __maybe_unused u32
+dw_mipi_csi2_read(struct dw_mipi_csi2_device *csi2, unsigned int addr)
+{
+	return readl(csi2->base_addr + addr);
+}
+
+static const struct dw_mipi_csi2_format *
+dw_mipi_csi2_find_format(struct dw_mipi_csi2_device *csi2, u32 mbus_code)
+{
+	WARN_ON(csi2->formats_num == 0);
+
+	for (unsigned int i = 0; i < csi2->formats_num; i++) {
+		const struct dw_mipi_csi2_format *format = &csi2->formats[i];
+
+		if (format->code == mbus_code)
+			return format;
+	}
+
+	return NULL;
+}
+
+static int dw_mipi_csi2_start(struct dw_mipi_csi2_device *csi2)
+{
+	struct media_pad *source_pad;
+	union phy_configure_opts opts;
+	s64 link_freq;
+	u32 control = 0;
+	u32 lanes = csi2->lanes_num;
+	int ret;

try keep reverise christmas tree order.

Ack.

quoted

quoted

+
+	if (lanes < 1 || lanes > 4)
+		return -EINVAL;
+

...

quoted

+
+static int dw_mipi_csi2_register_notifier(struct dw_mipi_csi2_device *csi2)
+{
+	struct v4l2_async_connection *asd;
+	struct v4l2_async_notifier *ntf = &csi2->notifier;
+	struct v4l2_fwnode_endpoint vep;
+	struct v4l2_subdev *sd = &csi2->sd;
+	struct device *dev = csi2->dev;
+	struct fwnode_handle *ep;
+	int ret;
+
+	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.

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;
+	sd->internal_ops = &dw_mipi_csi2_internal_ops;
+	sd->owner = THIS_MODULE;

I remeber needn't set owner, v4l2_async_register_subdev() do it for you.

Indeed, nice catch.

quoted

quoted

+	snprintf(sd->name, sizeof(sd->name), "dw-mipi-csi2 %s",
+		 dev_name(csi2->dev));
+

...

quoted

+
+static int dw_mipi_csi2_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_mipi_csi2_device *csi2;
+	int ret;
+
+	csi2 = devm_kzalloc(dev, sizeof(*csi2), GFP_KERNEL);
+	if (!csi2)
+		return -ENOMEM;
+	csi2->dev = dev;
+	dev_set_drvdata(dev, csi2);
+
+	csi2->base_addr = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(csi2->base_addr))
+		return PTR_ERR(csi2->base_addr);
+
+	ret = devm_clk_bulk_get_all(dev, &csi2->clks);
+	if (ret != DW_MIPI_CSI2_CLKS_MAX)
+		return dev_err_probe(dev, -ENODEV, "failed to get clocks\n");
+	csi2->clks_num = ret;
+
+	csi2->phy = devm_phy_get(dev, NULL);
+	if (IS_ERR(csi2->phy))
+		return dev_err_probe(dev, PTR_ERR(csi2->phy),
+				     "failed to get MIPI CSI-2 PHY\n");
+
+	csi2->reset = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(csi2->reset))
+		return dev_err_probe(dev, PTR_ERR(csi2->reset),
+				     "failed to get reset\n");
+
+	csi2->formats = formats;
+	csi2->formats_num = ARRAY_SIZE(formats);
+
+	pm_runtime_enable(dev);

devm_pm_runtime_enable() will simple error handle.

Ack.

quoted

quoted

+
+	ret = phy_init(csi2->phy);
+	if (ret) {
+		ret = dev_err_probe(dev, ret,
+				    "failed to initialize MIPI CSI-2 PHY\n");
+		goto err_pm_runtime_disable;
+	}
+

...

quoted

+
+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.

Using fsleep() by default, unless there's a specific need to use
udelay() or msleep(), is usually preferred. It "does the right thing"
(TM) automatically, freeing developers and reviewers from having to
think about it.

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.

quoted

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");

-- 
Regards,

Laurent Pinchart