linux-kernel - Re: [PATCH v3 5/9] media: sunxi: Add support for the A31 MIPI CSI-2 controller

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 for Android: free password hash cracker in your pocket

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

Message-ID: <YjHluwVnbPyHo1kp@paasikivi.fi.intel.com>
Date:   Wed, 16 Mar 2022 15:27:23 +0200
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
        Yong Deng <yong.deng@...ewell.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Kishon Vijay Abraham I <kishon@...com>,
        Vinod Koul <vkoul@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v3 5/9] media: sunxi: Add support for the A31 MIPI CSI-2
 controller

Hi Paul,

Thanks for the set.

On Wed, Mar 02, 2022 at 11:07:35PM +0100, Paul Kocialkowski wrote:
...
> +static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> +{
> +	struct sun6i_mipi_csi2_device *csi2_dev = v4l2_get_subdevdata(subdev);
> +	struct v4l2_subdev *source_subdev = csi2_dev->bridge.source_subdev;
> +	union phy_configure_opts dphy_opts = { 0 };
> +	struct phy_configure_opts_mipi_dphy *dphy_cfg = &dphy_opts.mipi_dphy;
> +	struct v4l2_mbus_framefmt *mbus_format = &csi2_dev->bridge.mbus_format;
> +	const struct sun6i_mipi_csi2_format *format;
> +	struct phy *dphy = csi2_dev->dphy;
> +	struct device *dev = csi2_dev->dev;
> +	struct v4l2_ctrl *ctrl;
> +	unsigned int lanes_count =
> +		csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes;
> +	unsigned long pixel_rate;
> +	/* Initialize to 0 to use both in disable label (ret != 0) and off. */
> +	int ret = 0;
> +
> +	if (!source_subdev)
> +		return -ENODEV;
> +
> +	if (!on) {
> +		v4l2_subdev_call(source_subdev, video, s_stream, 0);
> +		goto disable;
> +	}
> +
> +	/* Runtime PM */
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Sensor Pixel Rate */
> +
> +	ctrl = v4l2_ctrl_find(source_subdev->ctrl_handler, V4L2_CID_PIXEL_RATE);
> +	if (!ctrl) {
> +		dev_err(dev, "missing sensor pixel rate\n");
> +		ret = -ENODEV;
> +		goto error_pm;
> +	}
> +
> +	pixel_rate = (unsigned long)v4l2_ctrl_g_ctrl_int64(ctrl);
> +	if (!pixel_rate) {
> +		dev_err(dev, "missing (zero) sensor pixel rate\n");
> +		ret = -ENODEV;
> +		goto error_pm;
> +	}
> +
> +	/* D-PHY */
> +
> +	if (!lanes_count) {

I first thought this check could be moved to the beginning, but it's also
redundant. v4l2_fwnode_endpoint_parse() will check the configuration is
valid, i.e. the number of lanes is not zero.

But should you add checks to make sure the hardware supports what has been
configured? I'd do that right after parsing the endpoint.

And you only seem to be using the number of data lanes, nothing more. So
I'd store that, instead of the entire parsed v4l2_fwnode_endpoint.

The same applies to patch 8.

I think these could be done on top of this set after it is merged. Up to
you.

...

> +static int
> +sun6i_mipi_csi2_bridge_source_setup(struct sun6i_mipi_csi2_device *csi2_dev)
> +{
> +	struct v4l2_async_notifier *notifier = &csi2_dev->bridge.notifier;
> +	struct v4l2_fwnode_endpoint *endpoint = &csi2_dev->bridge.endpoint;
> +	struct v4l2_async_subdev *subdev_async;
> +	struct fwnode_handle *handle;
> +	struct device *dev = csi2_dev->dev;
> +	int ret;
> +
> +	handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> +						 FWNODE_GRAPH_ENDPOINT_NEXT);
> +	if (!handle)
> +		return -ENODEV;
> +
> +	endpoint->bus_type = V4L2_MBUS_CSI2_DPHY;
> +
> +	ret = v4l2_fwnode_endpoint_parse(handle, endpoint);
> +	if (ret)
> +		goto complete;
> +
> +	subdev_async = v4l2_async_nf_add_fwnode_remote(notifier, handle,
> +		struct v4l2_async_subdev);
> +	if (IS_ERR(subdev_async))
> +		ret = PTR_ERR(subdev_async);
> +
> +complete:
> +	fwnode_handle_put(handle);
> +
> +	return ret;
> +}

-- 
Kind regards,

Sakari Ailus