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

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

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