Messages in this thread | | | Date | Thu, 19 Jan 2023 10:22:37 +0100 | From | "Arnd Bergmann" <> | Subject | Re: [PATCH v9 15/15] spi: pensando-sr: Add AMD Pensando SoC System Resource |
| |
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
| |