On Tue, Feb 13, 2024 at 02:53:50PM +0100, Harald Mommer wrote:Replace link by Haixu Cui's diagram + explanations.The explanations were helpful so I wanted to keep this but the comment may now be too long to be accepted. We will see what happens.
+/*Please write actual comments that can be read standalone, the reader has
+ * See also
+ *https://lore.kernel.org/all/6171c1c3-55ba-4f74-ae60-764820cf1caf@xxxxxxxxxxx
+ */
+static int virtio_spi_set_delays(struct spi_transfer_head *th,
+ struct spi_device *spi,
+ struct spi_transfer *xfer)
absolutely no idea why they'd want to follow the link and there's
nothing being referenced by that "also".
+static int virtio_spi_one_transfer(struct virtio_spi_req *spi_req,
+ struct spi_controller *ctrl,
+ struct spi_message *msg,
+ struct spi_transfer *xfer)
+ /*
+ * Got comment: "The virtio spec for cs_change is *not* what the Linux
+ * cs_change field does, this will not do the right thing."
+ * TODO: Understand/discuss this, still unclear what may be wrong here
+ */
+ th->cs_change = xfer->cs_change;
Will be done, struct virtio_spi_req spi_req will become a member of struct virtio_spi_priv.+static int virtio_spi_transfer_one_message(struct spi_controller *ctrl,Why not just allocate this once, it's not like it's possible to send
+ struct spi_message *msg)
+{
+ struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
+ struct virtio_spi_req *spi_req;
+ struct spi_transfer *xfer;
+ int ret = 0;
+
+ spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
+ if (!spi_req) {
+ ret = -ENOMEM;
+ goto no_mem;
+ }
more than one message simultaneously?
Obviously I did not get the terminology of messages and transfers not correct which made the comment wrong. Comment to be corrected (and shortened).+ /*This is processing transfers within a message rather than messages.
+ * Simple implementation: Process message by message and wait for each
+ * message to be completed by the device side.
+ */
+ list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+ ret = virtio_spi_one_transfer(spi_req, ctrl, msg, xfer);It's not clear why this isn't within _spi_transfer_one() and then we
+ if (ret)
+ goto msg_done;
+
+ virtqueue_kick(priv->vq);
+
+ wait_for_completion(&spi_req->completion);
+
+ /* Read result from message */
+ ret = (int)spi_req->result.result;
+ if (ret)
+ goto msg_done;
don't just use a transfer_one() callback and factor everything out to
the core?