Hi Wentong,
On Mon, Apr 24, 2023 at 07:09:34AM +0000, Wu, Wentong wrote:
> > > > > + if (!csi->remote)
> > > > > + return -ENODEV;
> > > > > +
> > > > > + ctrl = v4l2_ctrl_find(csi->remote->ctrl_handler, V4L2_CID_LINK_FREQ);
> > > > > + if (!ctrl)
> > > > > + return -EINVAL;
> > > > > + qm.index = v4l2_ctrl_g_ctrl(ctrl);
> > > > > +
> > > > > + ret = v4l2_querymenu(csi->remote->ctrl_handler, &qm);
> > > > > + if (ret)
> > > > > + return ret;
> > > >
> > > > Please use v4l2_get_link_freq() as already discussed.
> > >
> > > ack, thanks,
> > >
> > > >
> > > > Didn't we also discuss that you could do this in the s_stream() callback?
> > >
> > > We don't need configure CSI2 link if call s_stream with enable = 0,
> > >
> > > But I'm ok to get this in s_stream, thanks
> >
> > Yes, you should only query this if streaming is being enabled.
>
> Can we just query the link freq in mei_csi_notify_bound and record it?
Yes. It can't change during streaming in any case.
> > > > > +static int mei_csi_parse_firmware(struct mei_csi *csi) {
> > > > > + struct v4l2_fwnode_endpoint v4l2_ep = {
> > > > > + .bus_type = V4L2_MBUS_CSI2_DPHY,
> > > > > + };
> > > > > + struct device *dev = &csi->cldev->dev;
> > > > > + struct v4l2_async_subdev *asd;
> > > > > + struct fwnode_handle *fwnode;
> > > > > + struct fwnode_handle *ep;
> > > > > + int ret;
> > > > > +
> > > > > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
> > > > > + if (!ep) {
> > > > > + dev_err(dev, "not connected to subdevice\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + ret = v4l2_fwnode_endpoint_parse(ep, &v4l2_ep);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "could not parse v4l2 endpoint\n");
> > > > > + fwnode_handle_put(ep);
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > > > + fwnode_handle_put(ep);
> > > > > +
> > > > > + v4l2_async_nf_init(&csi->notifier);
> > > > > + csi->notifier.ops = &mei_csi_notify_ops;
> > > > > +
> > > > > + asd = v4l2_async_nf_add_fwnode(&csi->notifier, fwnode,
> > > > > + struct v4l2_async_subdev);
> > > > > + if (IS_ERR(asd)) {
> > > > > + fwnode_handle_put(fwnode);
> > > > > + return PTR_ERR(asd);
> > > > > + }
> > > > > +
> > > > > + ret = v4l2_fwnode_endpoint_alloc_parse(fwnode, &v4l2_ep);
> > > >
> > > > It seems you're parsing the remote endpoint properties here. This
> > > > shouldn't be needed for any reason.
> > >
> > > We need sensor's lane number to configure IVSC's CSI2
> > >
> > > >
> > > > > + fwnode_handle_put(fwnode);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > + csi->nr_of_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
> > > >
> > > > Instead the number of lanes comes from the local endpoint.
> > >
> > > The lane number of IVSC follows sensor's lane number, why local endpoint?
> >
> > Because you shouldn't access other devices' endpoint properties in drivers.
> > They are outside of the scope of the device's bindings.
>
> Ok, in v3 patch set, it was trying to get the lane number as below:
>
> + ret = v4l2_subdev_call(csi->remote, pad, get_mbus_config,
> + csi->remote_pad, &mbus_config);
> + if (ret)
> + return ret;
> +
> + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY)
> + return -EINVAL;
> +
> + csi->nr_of_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;
>
> Can we use above subdev_call in mei_csi_notify_bound to get lane number?
My point is that you're not supposed to find this from an external device
or its firmware node. This information should instead be present in the
device's own firmware nodes. It's the IPU bridge's job to put it there.
Please see Documentation/firmware-guide/acpi/dsd/graph.rst and
Documentation/devicetree/bindings/media/video-interfaces.yaml .
--
Kind regards,
Sakari Ailus
