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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <YjNg+T+J1YkOm9LE@aptenodytes>
Date:   Thu, 17 Mar 2022 17:25:29 +0100
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.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 Sakari,

On Wed 16 Mar 22, 15:27, Sakari Ailus wrote:
> 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.

Good to know, thanks!

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

I guess you mean checking that we don't get more than 4 lanes?
And maybe something related to lane ordering too?

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

That's correct, why not.

> The same applies to patch 8.
> 
> I think these could be done on top of this set after it is merged. Up to
> you.

I'll go for another iteration.

Thanks!

Paul

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

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)