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