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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel