Received-SPF: pass (google.com: domain of linux-kernel+bounces-83392-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 27 Feb 2024 14:25:03 +0000
From: Mark Brown <broonie@kernel.org>
To: Harald Mommer <harald.mommer@opensynergy.com>
Cc: virtio-dev@lists.oasis-open.org, Haixu Cui <quic_haixcui@quicinc.com>,
	Viresh Kumar <viresh.kumar@linaro.org>, linux-spi@vger.kernel.org,
	linux-kernel@vger.kernel.org, quic_ztu@quicinc.com,
	Matti Moell <Matti.Moell@opensynergy.com>,
	Mikhail Golubev <Mikhail.Golubev@opensynergy.com>
Subject: Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.
Message-ID: <f02f1784-123d-44f8-afca-63a593c9d1da@sirena.org.uk>
References: <20240213135350.5878-1-Harald.Mommer@opensynergy.com>
 <20240213135350.5878-4-Harald.Mommer@opensynergy.com>
 <3b8e947c-51fb-461e-bb9e-7bc1bb547471@sirena.org.uk>
 <67c59e4a-7f2f-4d74-a9fc-24346233c8c0@opensynergy.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="6PkmX3O9usm2B74U"
Content-Disposition: inline
In-Reply-To: <67c59e4a-7f2f-4d74-a9fc-24346233c8c0@opensynergy.com>


--6PkmX3O9usm2B74U
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 Li=
nux
> > > +	 * cs_change field does, this will not do the right thing."
> > > +	 * TODO: Understand/discuss this, still unclear what may be wrong h=
ere
> > > +	 */

> > > +	th->cs_change =3D xfer->cs_change;

> I got the comment originally from you, Mark Brown. After some digging sti=
ll
> 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. =20

>=20
> I'll remove the comment because I think there is no problem. Please prote=
st
> if I'm wrong.
>=20
> > > +static int virtio_spi_transfer_one_message(struct spi_controller *ct=
rl,
> > > +					   struct spi_message *msg)
> > > +{
> > > +	struct virtio_spi_priv *priv =3D spi_controller_get_devdata(ctrl);
> > > +	struct virtio_spi_req *spi_req;
> > > +	struct spi_transfer *xfer;
> > > +	int ret =3D 0;
> > > +
> > > +	spi_req =3D kzalloc(sizeof(*spi_req), GFP_KERNEL);
> > > +	if (!spi_req) {
> > > +		ret =3D -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 e=
ach
> > > +	 * 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 =3D 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 =3D (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?
>=20
> 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.
>=20
> Applied code changes, have to run some more tests.
>=20
>=20

--6PkmX3O9usm2B74U
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmXd8L4ACgkQJNaLcl1U
h9AaHAgAhBmpwymQNbLcXJ6oSG8Ft2jRG56FDmXOnjNtuyzElqXU7Bb9fqv2zxi0
YciewdNgHM18s8m7USmne8F9eMHOyHsM91UqgSKfIE1UwZ2ugLYxcdoqNDOj808x
qt7etMjNgE8e7wuHrNxtFfcCnX6RH4OX3+HQffFc9gvtQDZguA+y/PrZE0HG6p67
b6CwpBRkkxzfRZelQrXFN3h/fpa+YkM6YXZowlrPiQwYoVlCkjAjXKpmVEDvGAkt
S8ELc1Kpvn+T2Ef1coFWwPOgBPjUa/u/wxgYcTQ1vPdUEAvmzn4r6MlK3YjjGLo9
UNJfcMmERF8YwfagFciRcVOkXAfxqQ==
=fBqF
-----END PGP SIGNATURE-----

--6PkmX3O9usm2B74U--