Re: [PATCH v9 15/15] spi: pensando-sr: Add AMD Pensando SoC System Resource

[Arnd Bergmann]

On Thu, Jan 19, 2023, at 04:39, Brad Larson wrote:

> drivers/spi/spi-pensando-sr.c

I don't think that is the right place for this driver: it's not a spi
controller implementation but rather a user interface driver that should
be in another subsystem depending on its purpose. drivers/misc/ might
be an option, but ideally I think there should be a higher-level
interface.

I'm still confused about what this driver actually does, and how
the corresponding user space side is meant to be used.

> +config SPI_PENSANDO_SR
> +	bool "AMD Pensando SoC System Resource chip"
> +	depends on SPI_MASTER=y

Why can this not be a loadable module, i.e. a 'tristate' option?

> +
> +#define PENSR_MAX_REG		0xff
> +#define PENSR_CTRL0_REG		0x10
> +#define PENSR_SPI_CMD_REGRD	0x0b
> +#define PENSR_SPI_CMD_REGWR	0x02
> +#define SPI_IOC_MAGIC		'k'
> +
> +#define SPI_MSGSIZE(N) \
> +	((((N)*(sizeof(struct spi_ioc_transfer))) < (1 << _IOC_SIZEBITS)) \
> +		? ((N)*(sizeof(struct spi_ioc_transfer))) : 0)
> +#define SPI_IOC_MESSAGE(N)	_IOW(SPI_IOC_MAGIC, 0, char[SPI_MSGSIZE(N)])
> +
> +struct spi_ioc_transfer {
> +	__u64 tx_buf;
> +	__u64 rx_buf;
> +	__u32 len;
> +	__u32 speed_hz;
> +	__u16 delay_usecs;
> +	__u8 bits_per_word;
> +	__u8 cs_change;
> +	__u8 tx_nbits;
> +	__u8 rx_nbits;
> +	__u8 word_delay_usecs;
> +	__u8 pad;
> +};

When you create a new user interface, the interface definition should
be in include/uapi/linux/*.h. The structure name and command name should
indicate what driver they are used for, these names look overly
generic.

> +struct pensr_device {
> +	struct spi_device *spi_dev;
> +	struct reset_controller_dev rcdev;
> +	struct mutex buf_lock;
> +	spinlock_t spi_lock;
> +	u8 *tx_buffer;
> +	u8 *rx_buffer;
> +};
> +
> +static dev_t pensr_devt;
> +static struct pensr_device *pensr;
> +static struct class *pensr_class;
> +static unsigned int bufsiz = 4096;

Even if there is only ever a single instance of the device known to the
kernel, it is better style to avoid static variables but instead make
everything passed around as part of the device structure.


> +
> +	t[0].tx_buf = tx_buf;
> +	t[0].len = u_xfer->len;
> +	if (copy_from_user(tx_buf, (const u8 __user *) (uintptr_t) 
> u_xfer->tx_buf, u_xfer->len)) {
> +		ret = -EFAULT;
> +		goto done;
> +	}

Use u64_to_user_ptr() instead of open-coding the type cast.

> +static const struct file_operations pensr_spi_fops = {
> +	.owner =	THIS_MODULE,
> +	.unlocked_ioctl = pensr_spi_ioctl,

There should be a '.compat_ioctl = compat_ptr_ioctl,' line here to
allow the ioctl to work in 32-bit processes.

      Arnd