On 13.02.24 18:49, Mark Brown wrote:
On Tue, Feb 13, 2024 at 02:53:50PM +0100, Harald Mommer wrote:
+/*
+ * 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)
Please write actual comments that can be read standalone, the reader has
absolutely no idea why they'd want to follow the link and there's
nothing being referenced by that "also".
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.
+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;
I got the comment originally from you, Mark Brown. After some digging
still unclear what should be wrong and in the meantime I think nothing
is wrong at all. To point you with the nose on the pending issue I added
this comment here.
I'll remove the comment because I think there is no problem. Please
protest if I'm wrong.
+static int virtio_spi_transfer_one_message(struct spi_controller *ctrl,
+ 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;
+ }
Why not just allocate this once, it's not like it's possible to send
more than one message simultaneously?
Will be done, struct virtio_spi_req spi_req will become a member of
struct virtio_spi_priv.
+ /*
+ * 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) {
This is processing transfers within a message rather than messages.
Obviously I did not get the terminology of messages and transfers not
correct which made the comment wrong. Comment to be corrected (and
shortened).
+ ret = virtio_spi_one_transfer(spi_req, ctrl, msg, xfer);
+ 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;
It's not clear why this isn't within _spi_transfer_one() and then we
don't just use a transfer_one() callback and factor everything out to
the core?
Lack of experience. I saw one way of doing the job which missing the
more simple way. Therefore we have reviews. Using now the alternative
callback which shortens and simplifies the code.
Applied code changes, have to run some more tests.
