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

[Haixu Cui <quic_haixcui@quicinc.com>]

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