Received-SPF: pass (google.com: domain of linux-kernel+bounces-194804-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 29 May 2024 21:22:07 -0300
From: Val Packett <val@packett.cool>
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for
 Rockchip's camera interface
To: Mehdi Djait <mehdi.djait.k@gmail.com>
Cc: mchehab@kernel.org, heiko@sntech.de, hverkuil-cisco@xs4all.nl,
	krzysztof.kozlowski+dt@linaro.org, robh+dt@kernel.org, conor+dt@kernel.org,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com,
	alexandre.belloni@bootlin.com, maxime.chevallier@bootlin.com,
	paul.kocialkowski@bootlin.com, michael.riesch@wolfvision.net,
	laurent.pinchart@ideasonboard.com, Mehdi Djait <mehdi.djait@bootlin.com>
Message-Id: <VOV9ES.1808A7M5SYMM3@packett.cool>
In-Reply-To: <715d89214d1ed6a8bb16cbb6268718a737485560.1707677804.git.mehdi.djait.k@gmail.com>
References: <cover.1707677804.git.mehdi.djait.k@gmail.com>
	<715d89214d1ed6a8bb16cbb6268718a737485560.1707677804.git.mehdi.djait.k@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=windows-1251; format=flowed
Content-Transfer-Encoding: quoted-printable

Hi,

On Sun, Feb 11 2024 at 20:03:31 +01:00:00, Mehdi Djait=20
<mehdi.djait.k@gmail.com> wrote:
> This introduces a V4L2 driver for the Rockchip CIF video capture=20
> controller.
>=20
> This controller can be found on RK3066, PX30, RK1808, RK3128 and=20
> RK3288,
> but for now it's only been tested on the PX30.

I've been testing it on an RK3066 tablet I'm bringing up mainline on,=20
recently figured out the clock situation that made the cameras appear=20
on I2C and got a camera driver to attach, but still not getting a frame=20
and will need to debug more.. Since you're familiar with the hardware,=20
could you quickly tell me what exactly causes the CIF interrupt to=20
fire? Is it a signal from the camera?

Anyway, two things I noticed with the code:

> +static int cif_enum_input(struct file *file, void *priv,
> +			  struct v4l2_input *input)
> +{
> +	struct cif_stream *stream =3D video_drvdata(file);
> +	struct v4l2_subdev *sd =3D stream->cifdev->remote.sd;
> +	int ret;
> +
> +	if (input->index > 0)
> +		return -EINVAL;
> +
> +	ret =3D v4l2_subdev_call(sd, video, g_input_status, &input->status);
> +	if (ret && ret !=3D -EOPNOTSUPP)
> +		return ret;

v4l2_subdev_call actually returns -ENOIOCTLCMD when the camera driver=20
does not handle the requested call.

> +static int cif_plat_probe(struct platform_device *pdev)
> +{
[=85]
> +	ret =3D devm_clk_bulk_get(dev, cif_dev->match_data->clks_num,
> +				cif_dev->match_data->clks);

This looks like it's just modifying the `static` clocks array shared=20
between all instances of the driver and not copying the clock=20
references into the instance's private struct.

Thanks,
~val