On Tue, Feb 27, 2024 at 03:12:55PM +0100, Harald Mommer wrote:
> On 13.02.24 18:49, Mark Brown wrote:
> > On Tue, Feb 13, 2024 at 02:53:50PM +0100, Harald Mommer wrote:
> > > +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.
Without going and checking the spec IIRC cs_change only applies within a
message in the virtio spec but it has effects on the final transfer in
Linux.
>
> 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.
>
>
Attachment:
signature.asc
Description: PGP signature
