Hi, Sakari, > -----Original Message----- > From: Sakari Ailus [mailto:sakari.ailus@xxxxxx] > Sent: Wednesday, October 11, 2017 11:20 PM > To: Zhi, Yong <yong.zhi@xxxxxxxxx> > Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; > hans.verkuil@xxxxxxxxx; Zheng, Jian Xu <jian.xu.zheng@xxxxxxxxx>; > tfiga@xxxxxxxxxxxx; Mani, Rajmohan <rajmohan.mani@xxxxxxxxx>; > Toivonen, Tuukka <tuukka.toivonen@xxxxxxxxx>; Yang, Hyungwoo > <hyungwoo.yang@xxxxxxxxx>; Vijaykumar, Ramya > <ramya.vijaykumar@xxxxxxxxx>; Rapolu, Chiranjeevi > <chiranjeevi.rapolu@xxxxxxxxx> > Subject: Re: [PATCH v5 3/3] intel-ipu3: cio2: Add new MIPI-CSI2 driver > > Hi Yong, > > One more comment below... > > On Thu, Oct 12, 2017 at 01:02:54AM +0000, Zhi, Yong wrote: > ... > > > > +/******* V4L2 sub-device asynchronous registration > > > callbacks***********/ > > > > + > > > > +struct sensor_async_subdev { > > > > + struct v4l2_async_subdev asd; > > > > + struct csi2_bus_info csi2; > > > > +}; > > > > + > > > > +static struct cio2_queue *cio2_find_queue_by_sensor_node(struct > > > cio2_queue *q, > > > > + struct fwnode_handle > > > *fwnode) > > > > +{ > > > > + unsigned int i; > > > > + > > > > + for (i = 0; i < CIO2_QUEUES; i++) { > > > > + if (q[i].sensor->fwnode == fwnode) > > > > + return &q[i]; > > > > + } > > > > + > > > > + return NULL; > > > > +} > > > > + > > > > +/* The .bound() notifier callback when a match is found */ static > > > > +int cio2_notifier_bound(struct v4l2_async_notifier *notifier, > > > > + struct v4l2_subdev *sd, > > > > + struct v4l2_async_subdev *asd) { > > > > + struct cio2_device *cio2 = container_of(notifier, > > > > + struct cio2_device, notifier); > > > > + struct sensor_async_subdev *s_asd = container_of(asd, > > > > + struct sensor_async_subdev, asd); > > > > + struct cio2_queue *q; > > > > + unsigned int i; > > > > + > > > > + > > > > + /* Find first free slot for the subdev */ > > > > + for (i = 0; i < CIO2_QUEUES; i++) > > > > + if (!cio2->queue[i].sensor) > > > > + break; > > The queues are related to sub-devices with the same number in the name, > whereas the number of the CSI-2 receiver is q->csi2.port. The problem here > is that the CSI-2 receiver that the sensor appears to be connected is a > incrementing number from zero onwards, depending on the order in which > the devices are bound rather than the real number of the receiver. > > The easiest way to address this would be to create 1:1 mapping between the > queues and CSI-2 receivers. > Sure, will fix according your suggestion. > > > > + > > > > + if (i >= CIO2_QUEUES) { > > > > + dev_err(&cio2->pci_dev->dev, "too many subdevs\n"); > > > > + return -ENOSPC; > > > > + } > > > > + q = &cio2->queue[i]; > > > > + > > > > + q->csi2 = s_asd->csi2; > > > > + q->sensor = sd; > > > > + q->csi_rx_base = cio2->base + CIO2_REG_PIPE_BASE(q->csi2.port); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +/* The .unbind callback */ > > > > +static void cio2_notifier_unbind(struct v4l2_async_notifier *notifier, > > > > + struct v4l2_subdev *sd, > > > > + struct v4l2_async_subdev *asd) { > > > > + struct cio2_device *cio2 = container_of(notifier, > > > > + struct cio2_device, notifier); > > > > + unsigned int i; > > > > + > > > > + /* Note: sd may here point to unallocated memory. Do not access. > > > > +*/ > > > > > > That may be the case but the patchset that this driver depends on > > > changes it. :-) So you can remove the comment. > > > > > > > Ack, will remove. > > > > > > + for (i = 0; i < CIO2_QUEUES; i++) { > > > > + if (cio2->queue[i].sensor == sd) { > > > > + cio2->queue[i].sensor = NULL; > > > > + return; > > > > + } > > > > + } > > > > +} > > > > + > > > > +/* .complete() is called after all subdevices have been located > > > > +*/ static int cio2_notifier_complete(struct v4l2_async_notifier > > > > +*notifier) { > > > > + struct cio2_device *cio2 = container_of(notifier, struct cio2_device, > > > > + notifier); > > > > + struct sensor_async_subdev *s_asd; > > > > + struct cio2_queue *q; > > > > + unsigned int i, pad; > > > > + int ret; > > > > + > > > > + for (i = 0; i < notifier->num_subdevs; i++) { > > > > + s_asd = container_of(cio2->notifier.subdevs[i], > > > > + struct sensor_async_subdev, > > > > + asd); > > > > + > > > > + q = cio2_find_queue_by_sensor_node( > > > > + cio2->queue, > > > > + s_asd- > > > >asd.match.fwnode.fwnode); > > > > + if (!q) { > > > > + dev_err(&cio2->pci_dev->dev, > > > > + "failed to find cio2 queue %d\n", ret); > > > > + return -ENXIO; > > > > + } > > > > + > > > > + for (pad = 0; pad < q->sensor->entity.num_pads; pad++) > > > > + if (q->sensor->entity.pads[pad].flags & > > > > + MEDIA_PAD_FL_SOURCE) > > > > + break; > > > > + > > > > + if (pad == q->sensor->entity.num_pads) { > > > > + dev_err(&cio2->pci_dev->dev, > > > > + "failed to find src pad for %s\n", > > > > + q->sensor->name); > > > > + return -ENXIO; > > > > + } > > > > + > > > > + ret = media_create_pad_link( > > > > + &q->sensor->entity, pad, > > > > + &q->subdev.entity, CIO2_PAD_SINK, > > > > + 0); > > > > + if (ret) { > > > > + dev_err(&cio2->pci_dev->dev, > > > > + "failed to create link for %s\n", > > > > + cio2->queue[i].sensor->name); > > > > + return ret; > > > > + } > > > > + } > > > > + > > > > + return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev); > > > > +} > > > > + > > > > +static const struct v4l2_async_notifier_operations cio2_async_ops = { > > > > + .bound = cio2_notifier_bound, > > > > + .unbind = cio2_notifier_unbind, > > > > + .complete = cio2_notifier_complete, }; > > > > + > > > > +static int cio2_fwnode_parse(struct device *dev, > > > > + struct v4l2_fwnode_endpoint *vep, > > > > + struct v4l2_async_subdev *asd) { > > > > + struct sensor_async_subdev *s_asd = > > > > + container_of(asd, struct sensor_async_subdev, asd); > > > > + > > > > + if (vep->bus_type != V4L2_MBUS_CSI2) { > > > > + dev_err(dev, "endpoint bus type error\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + s_asd->csi2.port = vep->base.port; > > > > + s_asd->csi2.lanes = vep->bus.mipi_csi2.num_data_lanes; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int cio2_notifier_init(struct cio2_device *cio2) { > > > > + int ret; > > > > + > > > > + ret = v4l2_async_notifier_parse_fwnode_endpoints( > > > > + &cio2->pci_dev->dev, &cio2->notifier, > > > > + sizeof(struct sensor_async_subdev), > > > > + cio2_fwnode_parse); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + if (!cio2->notifier.num_subdevs) > > > > + return 0; /* no endpoint */ > > > > > > You could make this an error as well: there device won't do anything > > > in that case anyway. -ENODEV, perhaps. > > > > > > > Ack. > > > > > > + > > > > + cio2->notifier.ops = &cio2_async_ops; > > > > + ret = v4l2_async_notifier_register(&cio2->v4l2_dev, &cio2->notifier); > > > > + if (ret) { > > > > + dev_err(&cio2->pci_dev->dev, > > > > + "failed to register async notifier : %d\n", ret); > > > > + goto error; > > > > + } > > > > + > > > > + return 0; > > > > + > > > > +error: > > > > + v4l2_async_notifier_cleanup(&cio2->notifier); > > > > + > > > > + return ret; > > > > +} > > -- > Sakari Ailus > e-mail: sakari.ailus@xxxxxx
