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
