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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aWor2m3TNJ-kHJha@kekkonen.localdomain>
Date: Fri, 16 Jan 2026 14:15:22 +0200
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Michael Riesch <michael.riesch@...labora.com>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.com>,
	Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Hans Verkuil <hverkuil@...nel.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Sebastian Reichel <sebastian.reichel@...labora.com>,
	Nicolas Dufresne <nicolas.dufresne@...labora.com>,
	Collabora Kernel Team <kernel@...labora.com>,
	linux-media@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] media: rockchip: add driver for the rockchip mipi
 csi-2 receiver

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