Hi Michael,

On Thu, Jan 15, 2026 at 03:50:59PM +0100, Michael Riesch wrote:

quoted

quoted

+	/* set mult and div to 0, thus completely rely on V4L2_CID_LINK_FREQ */
+	link_freq = v4l2_get_link_freq(source_pad, 0, 0);
+	if (link_freq <= 0)
+		return -EINVAL;

Could you return link_freq here?

Ack.

You should also test for < 0; v4l2_get_link_freq() returns the frequency or
an error (and 0 isn't valid).

...

quoted

quoted

+static int rkcsi_register(struct rkcsi_device *csi_dev)
+{
+	struct media_pad *pads = csi_dev->pads;
+	struct v4l2_subdev *sd = &csi_dev->sd;
+	int ret;
+
+	ret = rkcsi_register_notifier(csi_dev);
+	if (ret)
+		goto err;
+
+	v4l2_subdev_init(sd, &rkcsi_ops);
+	sd->dev = csi_dev->dev;
+	sd->entity.ops = &rkcsi_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 = &rkcsi_internal_ops;
+	sd->owner = THIS_MODULE;
+	snprintf(sd->name, sizeof(sd->name), "rockchip-mipi-csi %s",
+		 dev_name(csi_dev->dev));

Do you need to give it an explicit name? (v4l2_i2c_subdev_init() already
does that.)

I do, actually. I don't call the _i2c_ variant. This is a platform device.

Ah, I somehow thought this would be an I²C device. Please ignore the
comment then.

...

quoted

quoted

+	pm_runtime_enable(dev);

You're not resuming the device in probe(). I guess you depend on CONFIG_PM?

I do depend on CONFIG_PM.

I don't think there is need to resume in probe -> resume in
_enable_streams should do the trick, right?

Ack; that's fine with CONFIG_PM.

-- 
Kind regards,

Sakari Ailus