Re: [PATCH v3 2/3] media: rockchip: add driver for the rockchip mipi csi-2 receiver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Michael,

On Thu, Jan 15, 2026 at 03:50:59PM +0100, Michael Riesch wrote:
> >> +	/* 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).

...

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

...

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