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
Attachment:
signature.asc
Description: PGP signature
