Re: [PATCH v9 3/3] SPI: Add virtio SPI driver

[Haixu Cui <quic_haixcui@xxxxxxxxxxx>]

Hi Andy,

On 9/1/2025 8:07 PM, Andy Shevchenko wrote:
> On Thu, Aug 28, 2025 at 05:34:51PM +0800, Haixu Cui wrote:
> > This is the virtio SPI Linux kernel driver.
>
> ...
>
> > +#include <linux/completion.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/stddef.h>
>
> A lot of headers are still missing. See below.
>
> ...

This driver compiles successfully, and I believe all required 
definitions are resolved through indirect inclusion. For example, since 
I included virtio.h, there is no need to explicitly include device.h, 
scatterlist.h or types.h.

I avoided redundant #includes to keep the code clean and minimal.

If there are any essential headers I’ve overlooked, please feel free to 
highlight them—I’ll gladly include them in the next revision.



> > +static int virtio_spi_transfer_one(struct spi_controller *ctrl,
> > +				   struct spi_device *spi,
> > +				   struct spi_transfer *xfer)
> > +{
> > +	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
>
> > +	struct virtio_spi_req *spi_req __free(kfree);
>
> This is incorrect template. It's one of the exceptions when we mix code and
> definitions...
> > +	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
>
> ...so this should be
>
> 	struct virtio_spi_req *spi_req __free(kfree) =
> 		kzalloc(sizeof(*spi_req), GFP_KERNEL);
>
> (or on one line if you are okay with a 100 limit).

I plan to update the code as follows:

struct virtio_spi_req *spi_req __free(kfree) = NULL;
spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
if(!spi_req)
    return -ENOMEM;

This follows the pattern used in 
virtio_net(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/virtio_net.c?h=v6.17-rc4#n3746)

I'd like to check if this style is acceptable here, thanks.



> > +	if (ret || bus_num > S16_MAX)
>
> + limits.h
>
> > +		ctrl->bus_num = -1;
> > +	else
> > +		ctrl->bus_num = bus_num;
>
> But why do we need this property at all? And where is it documented in the
> device tree bindings?

I’ve reviewed other SPI drivers in the kernel, and it seems that bus_num 
is not a required property in most cases. I’ll remove the related code 
in the next revision.

Thanks again.

BR
Haixu Cui