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". > +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; > +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? > + /* > + * 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. > + 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?
Attachment:
signature.asc
Description: PGP signature
