Hi Sakari,
> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Sent: Monday, April 24, 2023 2:57 PM
> To: Wu, Wentong <wentong.wu@xxxxxxxxx>
>
> > > > +
> > > > +/* configure CSI-2 link between host and IVSC */ static int
> > > > +csi_set_link_cfg(struct mei_csi *csi) {
> > > > + struct csi_cmd cmd = { 0 };
> > > > + size_t cmd_size;
> > > > + int ret;
> > > > +
> > > > + cmd.cmd_id = CSI_SET_CONF;
> > > > + cmd.param.conf.nr_of_lanes = csi->nr_of_lanes;
> > > > + cmd.param.conf.link_freq = CSI_LINK_FREQ(csi->link_freq);
> > > > + cmd_size = sizeof(cmd.cmd_id) + sizeof(cmd.param.conf);
> > >
> > > Could you initialise this in variable declaration as I requested earlier?
> > > The same for other similar cases.
> >
> > I believe we have discussed this on v3 patch set as below:
> >
> > > > > In some cases you're using memset and in others not. If you don't
> need memset,
> > > > > I'd prefer assigning the fields in variable declaration instead.
> > > >
> > > > The declaration will be like below, but it will break reverse x-mas
> tree style.
> > > >
> > > > struct csi_cmd cmd = { 0 };
> > > > size_t cmd_size = sizeof(cmd.cmd_id) + sizeof(cmd.param.param);
> > > > int ret;
> >
> > > It's not a problem if you have a dependency.
>
> Yes, I meant the non-Christmas tree (reverse or not) ordering is not an issue.
> Dependencies of course are of higher priority.
Thanks,
>
> >
> > >
> > > > +
> > > > + mutex_lock(&csi->lock);
> > > > +
> > > > + ret = mei_csi_send(csi, (u8 *)&cmd, cmd_size);
> > > > + /*
> > > > + * wait configuration ready if download success. placing
> > > > + * delay under mutex is to make sure current command flow
> > > > + * completed before starting a possible new one.
> > > > + */
> > > > + if (!ret)
> > > > + msleep(CSI_FW_READY_DELAY_MS);
> > > > +
> > > > + mutex_unlock(&csi->lock);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int mei_csi_pre_streamon(struct v4l2_subdev *sd, u32 flags) {
> > > > + struct v4l2_querymenu qm = { .id = V4L2_CID_LINK_FREQ, };
> > > > + struct mei_csi *csi = sd_to_csi(sd);
> > > > + struct v4l2_ctrl *ctrl;
> > > > + int ret = 0;
> > >
> > > No need to initialise this, it is always set.
> >
> > ack, thanks
> >
> > >
> > > > +
> > > > + 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?
>
> >
> > >
> > > > +
> > > > + csi->link_freq = qm.value;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +
> > > > +static const struct v4l2_async_notifier_operations mei_csi_notify_ops = {
> > > > + .bound = mei_csi_notify_bound,
> > > > + .unbind = mei_csi_notify_unbind, };
> > > > +
> > > > +static int mei_csi_init_control(struct mei_csi *csi) {
> > > > + v4l2_ctrl_handler_init(&csi->ctrl_handler, 1);
> > > > + csi->ctrl_handler.lock = &csi->lock;
> > > > +
> > > > + csi->privacy_ctrl = v4l2_ctrl_new_std(&csi->ctrl_handler, NULL,
> > > > + V4L2_CID_PRIVACY, 0, 1, 1, 0);
> > > > + if (csi->ctrl_handler.error)
> > > > + return csi->ctrl_handler.error;
> > > > + csi->privacy_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >
> > > You should also add the LINK_FREQUENCY control. Make it U64 and and
> > > VOLATILE, too. This way the driver's g_volatile_ctrl() gets called
> > > when the control value is read.
> >
> > ack, thanks
> >
> > >
> > > > +
> > > > + csi->subdev.ctrl_handler = &csi->ctrl_handler;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +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?
BR,
Wentong
>
> >
> > >
> > > > +
> > > > + ret = v4l2_async_subdev_nf_register(&csi->subdev, &csi->notifier);
> > > > + if (ret)
> > > > + v4l2_async_nf_cleanup(&csi->notifier);
> > > > +
> > > > + v4l2_fwnode_endpoint_free(&v4l2_ep);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int mei_csi_probe(struct mei_cl_device *cldev,
> > > > + const struct mei_cl_device_id *id) {
> > > > + struct device *dev = &cldev->dev;
> > > > + struct mei_csi *csi;
> > > > + int ret;
> > > > +
> > > > + if (!dev_fwnode(dev))
> > > > + return -EPROBE_DEFER;
> > > > +
> > > > + csi = devm_kzalloc(dev, sizeof(struct mei_csi), GFP_KERNEL);
> > > > + if (!csi)
> > > > + return -ENOMEM;
> > > > +
> > > > + csi->cldev = cldev;
> > > > + mutex_init(&csi->lock);
> > > > + init_completion(&csi->cmd_completion);
> > > > +
> > > > + mei_cldev_set_drvdata(cldev, csi);
> > > > +
> > > > + ret = mei_cldev_enable(cldev);
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "mei_cldev_enable failed: %d\n", ret);
> > > > + goto destroy_mutex;
> > > > + }
> > > > +
> > > > + ret = mei_cldev_register_rx_cb(cldev, mei_csi_rx);
> > > > + if (ret) {
> > > > + dev_err(dev, "event cb registration failed: %d\n", ret);
> > > > + goto err_disable;
> > > > + }
> > > > +
> > > > + ret = mei_csi_parse_firmware(csi);
> > > > + if (ret)
> > > > + goto err_disable;
> > > > +
> > > > + csi->subdev.dev = &cldev->dev;
> > > > + v4l2_subdev_init(&csi->subdev, &mei_csi_subdev_ops);
> > > > + v4l2_set_subdevdata(&csi->subdev, csi);
> > > > + csi->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > >
> > > Please add V4L2_SUBDEV_FL_HAS_EVENTS for control events.
> >
> > ack, thanks
>
> --
> Kind regards,
>
> Sakari Ailus
