[PATCH v3 0/3] Spidev usability patchset update

[PATCH v3 0/3] Spidev usability patchset update

[Michal Suchanek]

Hello,

this simplifies spidev to not pose as SPI driver but instead attach a character
device on each SPI slave indiscriminately. This does not require spidev to be
matched during driver binding and hence does not require it to be specified in
devicetree.

Any SPI slave device for which a driver is not bound to start with or from
which it is unbound is accessible with spidev.

Constructive comments on improving this patchset are welcome.

Thanks

Michal

Michal Suchanek (3):
  spi: spidev: create spidev device for all spi slaves.
  spi: of: allow instantiating slaves without a driver
  spi: core: allow creating devices without spi-max-frequency

 Documentation/spi/spidev |  40 ++---
 drivers/spi/Kconfig      |   2 +-
 drivers/spi/spi.c        |  28 +++-
 drivers/spi/spidev.c     | 370 ++++++++++++++++++++---------------------------
 include/linux/spi/spi.h  |  14 ++
 5 files changed, 201 insertions(+), 253 deletions(-)

-- 
2.8.1

[PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

This patch makes spidev into a bus addon rather than a separate driver.
spidev character device is created for each spi slave. When a spi slave
driver is bound the spidev IOCTLs return -EBUSY.

This is similar to i2c bus which alows access to any address not claimed
by a kernel driver and should provide interface backwards compatible
with existing spidev tools.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 Documentation/spi/spidev |  40 ++---
 drivers/spi/Kconfig      |   2 +-
 drivers/spi/spi.c        |  19 ++-
 drivers/spi/spidev.c     | 370 ++++++++++++++++++++---------------------------
 include/linux/spi/spi.h  |  14 ++
 5 files changed, 197 insertions(+), 248 deletions(-)

diff --git a/Documentation/spi/spidev b/Documentation/spi/spidev
index 3d14035..dd8eac2 100644
--- a/Documentation/spi/spidev
+++ b/Documentation/spi/spidev
@@ -21,23 +21,14 @@ they need to access kernel interfaces (such as IRQ handlers or other layers
 of the driver stack) that are not accessible to userspace.
 
 
-DEVICE CREATION, DRIVER BINDING
-===============================
-The simplest way to arrange to use this driver is to just list it in the
-spi_board_info for a device as the driver it should use:  the "modalias"
-entry is "spidev", matching the name of the driver exposing this API.
-Set up the other device characteristics (bits per word, SPI clocking,
-chipselect polarity, etc) as usual, so you won't always need to override
-them later.
-
-(Sysfs also supports userspace driven binding/unbinding of drivers to
-devices.  That mechanism might be supported here in the future.)
-
-When you do that, the sysfs node for the SPI device will include a child
-device node with a "dev" attribute that will be understood by udev or mdev.
-(Larger systems will have "udev".  Smaller ones may configure "mdev" into
-busybox; it's less featureful, but often enough.)  For a SPI device with
-chipselect C on bus B, you should see:
+DEVICE CREATION
+===============
+
+When spidev is included in the kernel, the sysfs node for the SPI device will
+include a child device node with a "dev" attribute that will be understood by
+udev or mdev.  (Larger systems will have "udev".  Smaller ones may configure
+"mdev" into busybox; it's less featureful, but often enough.)  For a SPI device
+with chipselect C on bus B, you should see:
 
     /dev/spidevB.C ... character special device, major number 153 with
 	a dynamically chosen minor device number.  This is the node
@@ -54,18 +45,9 @@ Do not try to manage the /dev character device special file nodes by hand.
 That's error prone, and you'd need to pay careful attention to system
 security issues; udev/mdev should already be configured securely.
 
-If you unbind the "spidev" driver from that device, those two "spidev" nodes
-(in sysfs and in /dev) should automatically be removed (respectively by the
-kernel and by udev/mdev).  You can unbind by removing the "spidev" driver
-module, which will affect all devices using this driver.  You can also unbind
-by having kernel code remove the SPI device, probably by removing the driver
-for its SPI controller (so its spi_master vanishes).
-
-Since this is a standard Linux device driver -- even though it just happens
-to expose a low level API to userspace -- it can be associated with any number
-of devices at a time.  Just provide one spi_board_info record for each such
-SPI device, and you'll get a /dev device node for each device.
-
+When a device driver is loaded for the SPI device operations on the spidev
+character device return EBUSY. You can unbind a driver from the device using
+the driver unbind file in sysfs.
 
 BASIC CHARACTER DEVICE API
 ==========================
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index f2bbb16..3806e3c 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -699,7 +699,7 @@ config SPI_ZYNQMP_GQSPI
 comment "SPI Protocol Masters"
 
 config SPI_SPIDEV
-	tristate "User mode SPI device driver support"
+	bool "User mode SPI device driver support"
 	help
 	  This supports user mode SPI protocol drivers.
 
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 77e6e45..f3ea768 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -41,6 +41,8 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/spi.h>
 
+#include "spidev.h"
+
 static void spidev_release(struct device *dev)
 {
 	struct spi_device	*spi = to_spi_device(dev);
@@ -544,11 +546,15 @@ int spi_add_device(struct spi_device *spi)
 
 	/* Device may be bound to an active driver when this returns */
 	status = device_add(&spi->dev);
-	if (status < 0)
+	if (status < 0) {
 		dev_err(dev, "can't add %s, status %d\n",
 				dev_name(&spi->dev), status);
-	else
-		dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev));
+		goto done;
+	}
+	dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev));
+
+	if (IS_ENABLED(CONFIG_SPI_SPIDEV))
+		spi_attach_spidev(spi);
 
 done:
 	mutex_unlock(&spi_add_lock);
@@ -622,6 +628,10 @@ void spi_unregister_device(struct spi_device *spi)
 
 	if (spi->dev.of_node)
 		of_node_clear_flag(spi->dev.of_node, OF_POPULATED);
+
+	if (IS_ENABLED(CONFIG_SPI_SPIDEV))
+		spi_detach_spidev(spi);
+
 	device_unregister(&spi->dev);
 }
 EXPORT_SYMBOL_GPL(spi_unregister_device);
@@ -3128,6 +3138,9 @@ static int __init spi_init(void)
 	if (IS_ENABLED(CONFIG_OF_DYNAMIC))
 		WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));
 
+	if (IS_ENABLED(CONFIG_SPI_SPIDEV))
+		WARN_ON(spidev_init());
+
 	return 0;
 
 err2:
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index e3c19f3..57b47d0 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -27,14 +27,14 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/compat.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/spidev.h>
 
 #include <linux/uaccess.h>
 
+#include "spidev.h"
+
 
 /*
  * This supports access to SPI devices using normal userspace I/O calls.
@@ -72,23 +72,6 @@ static DECLARE_BITMAP(minors, N_SPI_MINORS);
 				| SPI_NO_CS | SPI_READY | SPI_TX_DUAL \
 				| SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD)
 
-struct spidev_data {
-	dev_t			devt;
-	spinlock_t		spi_lock;
-	struct spi_device	*spi;
-	struct list_head	device_entry;
-
-	/* TX/RX buffers are NULL unless this device is open (users > 0) */
-	struct mutex		buf_lock;
-	unsigned		users;
-	u8			*tx_buffer;
-	u8			*rx_buffer;
-	u32			speed_hz;
-};
-
-static LIST_HEAD(device_list);
-static DEFINE_MUTEX(device_list_lock);
-
 static unsigned bufsiz = 4096;
 module_param(bufsiz, uint, S_IRUGO);
 MODULE_PARM_DESC(bufsiz, "data bytes in biggest supported SPI message");
@@ -96,20 +79,11 @@ MODULE_PARM_DESC(bufsiz, "data bytes in biggest supported SPI message");
 /*-------------------------------------------------------------------------*/
 
 static ssize_t
-spidev_sync(struct spidev_data *spidev, struct spi_message *message)
+spidev_sync(struct spi_device *spi, struct spi_message *message)
 {
-	DECLARE_COMPLETION_ONSTACK(done);
 	int status;
-	struct spi_device *spi;
-
-	spin_lock_irq(&spidev->spi_lock);
-	spi = spidev->spi;
-	spin_unlock_irq(&spidev->spi_lock);
 
-	if (spi == NULL)
-		status = -ESHUTDOWN;
-	else
-		status = spi_sync(spi, message);
+	status = spi_sync(spi, message);
 
 	if (status == 0)
 		status = message->actual_length;
@@ -118,33 +92,33 @@ spidev_sync(struct spidev_data *spidev, struct spi_message *message)
 }
 
 static inline ssize_t
-spidev_sync_write(struct spidev_data *spidev, size_t len)
+spidev_sync_write(struct spi_device *spi, size_t len)
 {
 	struct spi_transfer	t = {
-			.tx_buf		= spidev->tx_buffer,
+			.tx_buf		= spi->tx_buffer,
 			.len		= len,
-			.speed_hz	= spidev->speed_hz,
+			.speed_hz	= spi->spidev_speed_hz,
 		};
 	struct spi_message	m;
 
 	spi_message_init(&m);
 	spi_message_add_tail(&t, &m);
-	return spidev_sync(spidev, &m);
+	return spidev_sync(spi, &m);
 }
 
 static inline ssize_t
-spidev_sync_read(struct spidev_data *spidev, size_t len)
+spidev_sync_read(struct spi_device *spi, size_t len)
 {
 	struct spi_transfer	t = {
-			.rx_buf		= spidev->rx_buffer,
+			.rx_buf		= spi->rx_buffer,
 			.len		= len,
-			.speed_hz	= spidev->speed_hz,
+			.speed_hz	= spi->spidev_speed_hz,
 		};
 	struct spi_message	m;
 
 	spi_message_init(&m);
 	spi_message_add_tail(&t, &m);
-	return spidev_sync(spidev, &m);
+	return spidev_sync(spi, &m);
 }
 
 /*-------------------------------------------------------------------------*/
@@ -153,27 +127,27 @@ spidev_sync_read(struct spidev_data *spidev, size_t len)
 static ssize_t
 spidev_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos)
 {
-	struct spidev_data	*spidev;
+	struct spi_device	*spi;
 	ssize_t			status = 0;
 
 	/* chipselect only toggles at start or end of operation */
 	if (count > bufsiz)
 		return -EMSGSIZE;
 
-	spidev = filp->private_data;
+	spi = filp->private_data;
 
-	mutex_lock(&spidev->buf_lock);
-	status = spidev_sync_read(spidev, count);
+	mutex_lock(&spi->buf_lock);
+	status = spidev_sync_read(spi, count);
 	if (status > 0) {
 		unsigned long	missing;
 
-		missing = copy_to_user(buf, spidev->rx_buffer, status);
+		missing = copy_to_user(buf, spi->rx_buffer, status);
 		if (missing == status)
 			status = -EFAULT;
 		else
 			status = status - missing;
 	}
-	mutex_unlock(&spidev->buf_lock);
+	mutex_unlock(&spi->buf_lock);
 
 	return status;
 }
@@ -183,7 +157,7 @@ static ssize_t
 spidev_write(struct file *filp, const char __user *buf,
 		size_t count, loff_t *f_pos)
 {
-	struct spidev_data	*spidev;
+	struct spi_device	*spi;
 	ssize_t			status = 0;
 	unsigned long		missing;
 
@@ -191,20 +165,20 @@ spidev_write(struct file *filp, const char __user *buf,
 	if (count > bufsiz)
 		return -EMSGSIZE;
 
-	spidev = filp->private_data;
+	spi = filp->private_data;
 
-	mutex_lock(&spidev->buf_lock);
-	missing = copy_from_user(spidev->tx_buffer, buf, count);
+	mutex_lock(&spi->buf_lock);
+	missing = copy_from_user(spi->tx_buffer, buf, count);
 	if (missing == 0)
-		status = spidev_sync_write(spidev, count);
+		status = spidev_sync_write(spi, count);
 	else
 		status = -EFAULT;
-	mutex_unlock(&spidev->buf_lock);
+	mutex_unlock(&spi->buf_lock);
 
 	return status;
 }
 
-static int spidev_message(struct spidev_data *spidev,
+static int spidev_message(struct spi_device *spi,
 		struct spi_ioc_transfer *u_xfers, unsigned n_xfers)
 {
 	struct spi_message	msg;
@@ -224,8 +198,8 @@ static int spidev_message(struct spidev_data *spidev,
 	 * We walk the array of user-provided transfers, using each one
 	 * to initialize a kernel version of the same transfer.
 	 */
-	tx_buf = spidev->tx_buffer;
-	rx_buf = spidev->rx_buffer;
+	tx_buf = spi->tx_buffer;
+	rx_buf = spi->rx_buffer;
 	total = 0;
 	tx_total = 0;
 	rx_total = 0;
@@ -281,27 +255,27 @@ static int spidev_message(struct spidev_data *spidev,
 		k_tmp->delay_usecs = u_tmp->delay_usecs;
 		k_tmp->speed_hz = u_tmp->speed_hz;
 		if (!k_tmp->speed_hz)
-			k_tmp->speed_hz = spidev->speed_hz;
+			k_tmp->speed_hz = spi->spidev_speed_hz;
 #ifdef VERBOSE
-		dev_dbg(&spidev->spi->dev,
+		dev_dbg(&spi->dev,
 			"  xfer len %u %s%s%s%dbits %u usec %uHz\n",
 			u_tmp->len,
 			u_tmp->rx_buf ? "rx " : "",
 			u_tmp->tx_buf ? "tx " : "",
 			u_tmp->cs_change ? "cs " : "",
-			u_tmp->bits_per_word ? : spidev->spi->bits_per_word,
+			u_tmp->bits_per_word ? : spi->bits_per_word,
 			u_tmp->delay_usecs,
-			u_tmp->speed_hz ? : spidev->spi->max_speed_hz);
+			u_tmp->speed_hz ? : spi->max_speed_hz);
 #endif
 		spi_message_add_tail(k_tmp, &msg);
 	}
 
-	status = spidev_sync(spidev, &msg);
+	status = spidev_sync(spi, &msg);
 	if (status < 0)
 		goto done;
 
 	/* copy any rx data out of bounce buffer */
-	rx_buf = spidev->rx_buffer;
+	rx_buf = spi->rx_buffer;
 	for (n = n_xfers, u_tmp = u_xfers; n; n--, u_tmp++) {
 		if (u_tmp->rx_buf) {
 			if (__copy_to_user((u8 __user *)
@@ -356,7 +330,6 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	int			err = 0;
 	int			retval = 0;
-	struct spidev_data	*spidev;
 	struct spi_device	*spi;
 	u32			tmp;
 	unsigned		n_ioc;
@@ -382,21 +355,22 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	/* guard against device removal before, or while,
 	 * we issue this ioctl.
 	 */
-	spidev = filp->private_data;
-	spin_lock_irq(&spidev->spi_lock);
-	spi = spi_dev_get(spidev->spi);
-	spin_unlock_irq(&spidev->spi_lock);
+	spi = filp->private_data;
+	spi = spi_dev_get(spi);
 
 	if (spi == NULL)
 		return -ESHUTDOWN;
 
+	if (spi->dev.driver)
+		return -EBUSY;
+
 	/* use the buffer lock here for triple duty:
 	 *  - prevent I/O (from us) so calling spi_setup() is safe;
 	 *  - prevent concurrent SPI_IOC_WR_* from morphing
 	 *    data fields while SPI_IOC_RD_* reads them;
 	 *  - SPI_IOC_MESSAGE needs the buffer locked "normally".
 	 */
-	mutex_lock(&spidev->buf_lock);
+	mutex_lock(&spi->buf_lock);
 
 	switch (cmd) {
 	/* read requests */
@@ -416,7 +390,7 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		retval = __put_user(spi->bits_per_word, (__u8 __user *)arg);
 		break;
 	case SPI_IOC_RD_MAX_SPEED_HZ:
-		retval = __put_user(spidev->speed_hz, (__u32 __user *)arg);
+		retval = __put_user(spi->spidev_speed_hz, (__u32 __user *)arg);
 		break;
 
 	/* write requests */
@@ -474,14 +448,14 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		}
 		break;
 	case SPI_IOC_WR_MAX_SPEED_HZ:
-		retval = __get_user(tmp, (__u32 __user *)arg);
+	retval = __get_user(tmp, (__u32 __user *)arg);
 		if (retval == 0) {
 			u32	save = spi->max_speed_hz;
 
 			spi->max_speed_hz = tmp;
 			retval = spi_setup(spi);
 			if (retval >= 0)
-				spidev->speed_hz = tmp;
+				spi->spidev_speed_hz = tmp;
 			else
 				dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
 			spi->max_speed_hz = save;
@@ -501,12 +475,12 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			break;	/* n_ioc is also 0 */
 
 		/* translate to spi_message, execute */
-		retval = spidev_message(spidev, ioc, n_ioc);
+		retval = spidev_message(spi, ioc, n_ioc);
 		kfree(ioc);
 		break;
 	}
 
-	mutex_unlock(&spidev->buf_lock);
+	mutex_unlock(&spi->buf_lock);
 	spi_dev_put(spi);
 	return retval;
 }
@@ -518,7 +492,6 @@ spidev_compat_ioc_message(struct file *filp, unsigned int cmd,
 {
 	struct spi_ioc_transfer __user	*u_ioc;
 	int				retval = 0;
-	struct spidev_data		*spidev;
 	struct spi_device		*spi;
 	unsigned			n_ioc, n;
 	struct spi_ioc_transfer		*ioc;
@@ -530,16 +503,17 @@ spidev_compat_ioc_message(struct file *filp, unsigned int cmd,
 	/* guard against device removal before, or while,
 	 * we issue this ioctl.
 	 */
-	spidev = filp->private_data;
-	spin_lock_irq(&spidev->spi_lock);
-	spi = spi_dev_get(spidev->spi);
-	spin_unlock_irq(&spidev->spi_lock);
+	spi = filp->private_data;
+	spi = spi_dev_get(spi);
 
 	if (spi == NULL)
 		return -ESHUTDOWN;
 
+	if (spi->dev.driver)
+		return -EBUSY;
+
 	/* SPI_IOC_MESSAGE needs the buffer locked "normally" */
-	mutex_lock(&spidev->buf_lock);
+	mutex_lock(&spi->buf_lock);
 
 	/* Check message and copy into scratch area */
 	ioc = spidev_get_ioc_message(cmd, u_ioc, &n_ioc);
@@ -557,11 +531,11 @@ spidev_compat_ioc_message(struct file *filp, unsigned int cmd,
 	}
 
 	/* translate to spi_message, execute */
-	retval = spidev_message(spidev, ioc, n_ioc);
+	retval = spidev_message(spi, ioc, n_ioc);
 	kfree(ioc);
 
 done:
-	mutex_unlock(&spidev->buf_lock);
+	mutex_unlock(&spi->buf_lock);
 	spi_dev_put(spi);
 	return retval;
 }
@@ -580,89 +554,108 @@ spidev_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 #define spidev_compat_ioctl NULL
 #endif /* CONFIG_COMPAT */
 
+static int spidev_devt_match(struct device *dev, void *data)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	dev_t devt = (dev_t) data;
+	int res = 0;
+
+	spin_lock_irq(&spi->spidev_lock);
+	if (spi->spidev_devt == devt)
+		res = 1;
+	spin_unlock_irq(&spi->spidev_lock);
+
+	return res;
+}
+
 static int spidev_open(struct inode *inode, struct file *filp)
 {
-	struct spidev_data	*spidev;
+	struct spi_device	*spi;
+	struct device		*dev;
 	int			status = -ENXIO;
 
-	mutex_lock(&device_list_lock);
+	dev = bus_find_device(&spi_bus_type, NULL, (void *) inode->i_rdev,
+			      spidev_devt_match);
 
-	list_for_each_entry(spidev, &device_list, device_entry) {
-		if (spidev->devt == inode->i_rdev) {
-			status = 0;
-			break;
-		}
-	}
-
-	if (status) {
+	if (!dev) {
 		pr_debug("spidev: nothing for minor %d\n", iminor(inode));
-		goto err_find_dev;
+		return status;
 	}
 
-	if (!spidev->tx_buffer) {
-		spidev->tx_buffer = kmalloc(bufsiz, GFP_KERNEL);
-		if (!spidev->tx_buffer) {
-			dev_dbg(&spidev->spi->dev, "open/ENOMEM\n");
+	spi = to_spi_device(dev);
+
+	mutex_lock(&spi->buf_lock);
+	spin_lock_irq(&spi->spidev_lock);
+	spi->spidev_users++;
+	spin_unlock_irq(&spi->spidev_lock);
+
+	if (!spi->tx_buffer) {
+		spi->tx_buffer = kmalloc(bufsiz, GFP_KERNEL);
+		if (!spi->tx_buffer) {
+			dev_dbg(&spi->dev, "open/ENOMEM\n");
 			status = -ENOMEM;
 			goto err_find_dev;
 		}
 	}
 
-	if (!spidev->rx_buffer) {
-		spidev->rx_buffer = kmalloc(bufsiz, GFP_KERNEL);
-		if (!spidev->rx_buffer) {
-			dev_dbg(&spidev->spi->dev, "open/ENOMEM\n");
+	if (!spi->rx_buffer) {
+		spi->rx_buffer = kmalloc(bufsiz, GFP_KERNEL);
+		if (!spi->rx_buffer) {
+			dev_dbg(&spi->dev, "open/ENOMEM\n");
 			status = -ENOMEM;
 			goto err_alloc_rx_buf;
 		}
 	}
 
-	spidev->users++;
-	filp->private_data = spidev;
+	mutex_unlock(&spi->buf_lock);
+
+	filp->private_data = spi;
 	nonseekable_open(inode, filp);
 
-	mutex_unlock(&device_list_lock);
 	return 0;
 
 err_alloc_rx_buf:
-	kfree(spidev->tx_buffer);
-	spidev->tx_buffer = NULL;
+	kfree(spi->tx_buffer);
+	spi->tx_buffer = NULL;
 err_find_dev:
-	mutex_unlock(&device_list_lock);
+	mutex_unlock(&spi->buf_lock);
+	spin_lock_irq(&spi->spidev_lock);
+	spi->spidev_users--;
+	spin_unlock_irq(&spi->spidev_lock);
 	return status;
 }
 
 static int spidev_release(struct inode *inode, struct file *filp)
 {
-	struct spidev_data	*spidev;
+	struct spi_device	*spi;
+	int do_free = 0;
 
-	mutex_lock(&device_list_lock);
-	spidev = filp->private_data;
+	spi = filp->private_data;
 	filp->private_data = NULL;
 
+	mutex_lock(&spi->buf_lock);
+	spin_lock_irq(&spi->spidev_lock);
+	spi->spidev_users--;
+
 	/* last close? */
-	spidev->users--;
-	if (!spidev->users) {
-		int		dofree;
+	if (!spi->spidev_users)
+		do_free = 1;
 
-		kfree(spidev->tx_buffer);
-		spidev->tx_buffer = NULL;
+	spin_unlock_irq(&spi->spidev_lock);
 
-		kfree(spidev->rx_buffer);
-		spidev->rx_buffer = NULL;
+	if (do_free) {
 
-		spin_lock_irq(&spidev->spi_lock);
-		if (spidev->spi)
-			spidev->speed_hz = spidev->spi->max_speed_hz;
+		kfree(spi->tx_buffer);
+		spi->tx_buffer = NULL;
 
-		/* ... after we unbound from the underlying device? */
-		dofree = (spidev->spi == NULL);
-		spin_unlock_irq(&spidev->spi_lock);
+		kfree(spi->rx_buffer);
+		spi->rx_buffer = NULL;
 
-		if (dofree)
-			kfree(spidev);
+		spi->spidev_speed_hz = spi->max_speed_hz;
 	}
-	mutex_unlock(&device_list_lock);
+	mutex_unlock(&spi->buf_lock);
+
+	spi_dev_put(spi);
 
 	return 0;
 }
@@ -691,152 +684,99 @@ static const struct file_operations spidev_fops = {
 
 static struct class *spidev_class;
 
-#ifdef CONFIG_OF
-static const struct of_device_id spidev_dt_ids[] = {
-	{ .compatible = "rohm,dh2228fv" },
-	{ .compatible = "lineartechnology,ltc2488" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, spidev_dt_ids);
-#endif
-
 /*-------------------------------------------------------------------------*/
 
-static int spidev_probe(struct spi_device *spi)
+int spi_attach_spidev(struct spi_device *spi)
 {
-	struct spidev_data	*spidev;
 	int			status;
 	unsigned long		minor;
 
-	/*
-	 * spidev should never be referenced in DT without a specific
-	 * compatible string, it is a Linux implementation thing
-	 * rather than a description of the hardware.
-	 */
-	if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) {
-		dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n");
-		WARN_ON(spi->dev.of_node &&
-			!of_match_device(spidev_dt_ids, &spi->dev));
-	}
-
-	/* Allocate driver data */
-	spidev = kzalloc(sizeof(*spidev), GFP_KERNEL);
-	if (!spidev)
-		return -ENOMEM;
-
 	/* Initialize the driver data */
-	spidev->spi = spi;
-	spin_lock_init(&spidev->spi_lock);
-	mutex_init(&spidev->buf_lock);
-
-	INIT_LIST_HEAD(&spidev->device_entry);
+	spin_lock_init(&spi->spidev_lock);
+	mutex_init(&spi->buf_lock);
 
 	/* If we can allocate a minor number, hook up this device.
 	 * Reusing minors is fine so long as udev or mdev is working.
 	 */
-	mutex_lock(&device_list_lock);
 	minor = find_first_zero_bit(minors, N_SPI_MINORS);
 	if (minor < N_SPI_MINORS) {
-		struct device *dev;
 
-		spidev->devt = MKDEV(SPIDEV_MAJOR, minor);
-		dev = device_create(spidev_class, &spi->dev, spidev->devt,
-				    spidev, "spidev%d.%d",
-				    spi->master->bus_num, spi->chip_select);
-		status = PTR_ERR_OR_ZERO(dev);
+		spi->spidev_devt = MKDEV(SPIDEV_MAJOR, minor);
+
+		spi->spidev = device_create(spidev_class, &spi->dev,
+					    spi->spidev_devt,
+					    spi, "spidev%d.%d",
+					    spi->master->bus_num,
+					    spi->chip_select);
+		status = PTR_ERR_OR_ZERO(spi->spidev);
+		if (status != 0) {
+			dev_err(&spi->dev, "Error creating spidev device!\n");
+			spi->spidev_devt = 0;
+			spi->spidev = NULL;
+		}
 	} else {
 		dev_dbg(&spi->dev, "no minor number available!\n");
 		status = -ENODEV;
 	}
 	if (status == 0) {
 		set_bit(minor, minors);
-		list_add(&spidev->device_entry, &device_list);
 	}
-	mutex_unlock(&device_list_lock);
-
-	spidev->speed_hz = spi->max_speed_hz;
-
-	if (status == 0)
-		spi_set_drvdata(spi, spidev);
-	else
-		kfree(spidev);
+	spi->spidev_speed_hz = spi->max_speed_hz;
 
 	return status;
 }
 
-static int spidev_remove(struct spi_device *spi)
+int spi_detach_spidev(struct spi_device *spi)
 {
-	struct spidev_data	*spidev = spi_get_drvdata(spi);
-
-	/* make sure ops on existing fds can abort cleanly */
-	spin_lock_irq(&spidev->spi_lock);
-	spidev->spi = NULL;
-	spin_unlock_irq(&spidev->spi_lock);
-
-	/* prevent new opens */
-	mutex_lock(&device_list_lock);
-	list_del(&spidev->device_entry);
-	device_destroy(spidev_class, spidev->devt);
-	clear_bit(MINOR(spidev->devt), minors);
-	if (spidev->users == 0)
-		kfree(spidev);
-	mutex_unlock(&device_list_lock);
+	dev_t	devt;
+	struct device *spidev;
+
+	/*
+	 * Make sure ops on existing fds can abort cleanly
+	 * and prevent new ones.
+	 */
+	spin_lock_irq(&spi->spidev_lock);
+	devt = spi->spidev_devt;
+	spidev = spi->spidev;
+	spi->spidev_devt = 0;
+	spi->spidev = NULL;
+	spin_unlock_irq(&spi->spidev_lock);
+
+	device_destroy(spidev_class, devt);
+	clear_bit(MINOR(devt), minors);
 
 	return 0;
 }
 
-static struct spi_driver spidev_spi_driver = {
-	.driver = {
-		.name =		"spidev",
-		.of_match_table = of_match_ptr(spidev_dt_ids),
-	},
-	.probe =	spidev_probe,
-	.remove =	spidev_remove,
-
-	/* NOTE:  suspend/resume methods are not necessary here.
-	 * We don't do anything except pass the requests to/from
-	 * the underlying controller.  The refrigerator handles
-	 * most issues; the controller driver handles the rest.
-	 */
-};
-
 /*-------------------------------------------------------------------------*/
 
-static int __init spidev_init(void)
+int __init spidev_init(void)
 {
 	int status;
 
 	/* Claim our 256 reserved device numbers.  Then register a class
-	 * that will key udev/mdev to add/remove /dev nodes.  Last, register
-	 * the driver which manages those device numbers.
+	 * that will key udev/mdev to add/remove /dev nodes.
 	 */
 	BUILD_BUG_ON(N_SPI_MINORS > 256);
-	status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
+	status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);
 	if (status < 0)
 		return status;
 
 	spidev_class = class_create(THIS_MODULE, "spidev");
 	if (IS_ERR(spidev_class)) {
-		unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
+		unregister_chrdev(SPIDEV_MAJOR, "spidev");
 		return PTR_ERR(spidev_class);
 	}
 
-	status = spi_register_driver(&spidev_spi_driver);
-	if (status < 0) {
-		class_destroy(spidev_class);
-		unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
-	}
 	return status;
 }
-module_init(spidev_init);
 
 static void __exit spidev_exit(void)
 {
-	spi_unregister_driver(&spidev_spi_driver);
 	class_destroy(spidev_class);
-	unregister_chrdev(SPIDEV_MAJOR, spidev_spi_driver.driver.name);
+	unregister_chrdev(SPIDEV_MAJOR, "spidev");
 }
-module_exit(spidev_exit);
+EXPORT_SYMBOL_GPL(spidev_exit);
 
 MODULE_AUTHOR("Andrea Paterniani, <a.paterniani@swapp-eng.it>");
 MODULE_DESCRIPTION("User mode SPI device interface");
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 1f03483..7154559 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -178,6 +178,20 @@ struct spi_device {
 	 *  - chipselect delays
 	 *  - ...
 	 */
+
+	/* spidev stuff */
+#if config_enabled(CONFIG_SPI_SPIDEV)
+	dev_t                   spidev_devt;
+	struct device *		spidev;
+	spinlock_t              spidev_lock;
+
+	/* TX/RX buffers are NULL unless this device is open (users > 0) */
+	struct mutex            buf_lock;
+	unsigned                spidev_users;
+	u8                      *tx_buffer;
+	u8                      *rx_buffer;
+	u32                     spidev_speed_hz;
+#endif /*CONFIG_SPI_SPIDEV*/
 };
 
 static inline struct spi_device *to_spi_device(struct device *dev)
-- 
2.8.1

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On Tue, Jul 19, 2016 at 12:35:40AM +0200, Michal Suchanek wrote:

>  config SPI_SPIDEV
> -	tristate "User mode SPI device driver support"
> +	bool "User mode SPI device driver support"

This is a step back, it would require spidev to be built in.

> -	spin_lock_irq(&spidev->spi_lock);
> -	spi = spi_dev_get(spidev->spi);
> -	spin_unlock_irq(&spidev->spi_lock);
> +	spi = filp->private_data;
> +	spi = spi_dev_get(spi);

All this refactoring to move spidev about should've been a separate
patch, it's hard to find the actual content in here.

> -	mutex_lock(&device_list_lock);
> +	dev = bus_find_device(&spi_bus_type, NULL, (void *) inode->i_rdev,
> +			      spidev_devt_match);

...

> -			dev_dbg(&spidev->spi->dev, "open/ENOMEM\n");
> +	spi = to_spi_device(dev);
> +
> +	mutex_lock(&spi->buf_lock);
> +	spin_lock_irq(&spi->spidev_lock);
> +	spi->spidev_users++;
> +	spin_unlock_irq(&spi->spidev_lock);

This is broken, it will unconditionally create a spidev for any chip
select regardless of if there's any actual hardware there and (even more
importantly) regardless if that hardware is actually a SPI device.  This
is not safe, especially given some of the ideas people seem to have for
userspaces.

I am getting completely fed up of saying this, it's not OK to just
expose pins to userspace when we have no idea what they are connected
to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

Hello,

On 19 July 2016 at 00:59, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 19, 2016 at 12:35:40AM +0200, Michal Suchanek wrote:
>
>>  config SPI_SPIDEV
>> -     tristate "User mode SPI device driver support"
>> +     bool "User mode SPI device driver support"
>
> This is a step back, it would require spidev to be built in.

That's a technical problem so it can be addressed.

>
>> -     spin_lock_irq(&spidev->spi_lock);
>> -     spi = spi_dev_get(spidev->spi);
>> -     spin_unlock_irq(&spidev->spi_lock);
>> +     spi = filp->private_data;
>> +     spi = spi_dev_get(spi);
>
> All this refactoring to move spidev about should've been a separate
> patch, it's hard to find the actual content in here.

And re-modularizing will probably need to move thing back, mostly.

Regarding this change

spidev uses device_create to allocate spidev character devices so
these are not part of some struct spidev_device but live separately
which gives needlessly more objects to manage

- the spi device
- the spidev character device
- a link object that points to both spi and spidev and is manually
  managed by the spidev driver

In order to reduce the amount of objects and management I appended
the link object at the end of struct spi_device.

Is there a better way?

>
>> -     mutex_lock(&device_list_lock);
>> +     dev = bus_find_device(&spi_bus_type, NULL, (void *) inode->i_rdev,
>> +                           spidev_devt_match);
>
> ...
>
>> -                     dev_dbg(&spidev->spi->dev, "open/ENOMEM\n");
>> +     spi = to_spi_device(dev);
>> +
>> +     mutex_lock(&spi->buf_lock);
>> +     spin_lock_irq(&spi->spidev_lock);
>> +     spi->spidev_users++;
>> +     spin_unlock_irq(&spi->spidev_lock);
>
> This is broken, it will unconditionally create a spidev for any chip
> select regardless of if there's any actual hardware there and (even more
> importantly) regardless if that hardware is actually a SPI device.  This
> is not safe, especially given some of the ideas people seem to have for
> userspaces.

This is as safe as it gets. For the device probe to happen somebody must
have configured the CS as spi slave device somewhere. What this patchset
accomplishes is creating an userspace interface for accessing SPI bus using
the CS regardless of the probe result (at least for devicetree nodes).

In particular it does not create a spidev device for CS lines which are unused.

Looking into SPI more this whole point is mostly moot, anyway. Most of the
time the unused CS lines will not be multiplexed to the SPI controller so
even if the SPI master tries to use them it does nothing. Also most of the
time all the CS lines can be exported as GPIO regardless of their use for SPI.
And set to any possible level using the GPIO userspace API.

>
> I am getting completely fed up of saying this, it's not OK to just
> expose pins to userspace when we have no idea what they are connected
> to.

I am getting fed up with this argument.

First, this patchset only exposes to userspace pins that are configured as SPI
slaves in devicetree.

It does so even in the cases when the driver for the device in question is not
available in the kernel (because it is not loaded or excluded completely) or
in the case you do not specify which SPI slave is connected to those pins.
This is the only change.

So in addition to saying 'load driver XY with parameters Z=BJ' this allows
saying 'there is possibly a SPI device and I may want to talk to it with an
userspace application'. With overlays this can be later amended to 'load
driver ZY with parameters X=XN' or whatever.

The fact that devicetree does not allow talking to a SPI device from userspace
without telling kernel what kind of protocol userspace is using is a regression
from using board files. The kernel has no business knowing that. When you
use a userspace driver it's userspace's job to manage protocols and devices.
The purpose of this patchset is for the userspace applications to continue doing
so with devicetree based systems without the need to fabricate fake hardware
description. And the hardware description will necessarily be fake in the case
when it is in fact userspace implementing and managing the drivers.

It's not to say that userspace drivers that duplicate kernel drivers cannot
examine the devicetree for configuration data. That's up to the userspace
driver writer to decide, however.

Thanks

Michal

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4047 bytes --]

On Tue, Jul 19, 2016 at 01:17:52PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 00:59, Mark Brown <broonie@kernel.org> wrote:

> Looking into SPI more this whole point is mostly moot, anyway. Most of the
> time the unused CS lines will not be multiplexed to the SPI controller so
> even if the SPI master tries to use them it does nothing. Also most of the
> time all the CS lines can be exported as GPIO regardless of their use for SPI.
> And set to any possible level using the GPIO userspace API.

Your system is not the entire world, Linux has to run on other systems
too.  There are actual devices out there with inflexible pinmuxing, we
can't just ignore them.

> In order to reduce the amount of objects and management I appended
> the link object at the end of struct spi_device.

> Is there a better way?

That seems mostly fine, the problem is one of code review - because the
patch does many logically distinct things in a single patch it is much
harder to review than if it were split out.  There's a lot of mechanical
refactoring which obscures the several more substantive changes that are
being made at the same time and since most of those substantive changes
are explicitly described at all it's hard to tell if they're intended or
doing what is expected.

> > I am getting completely fed up of saying this, it's not OK to just
> > expose pins to userspace when we have no idea what they are connected
> > to.

> I am getting fed up with this argument.

> First, this patchset only exposes to userspace pins that are configured as SPI
> slaves in devicetree.

Are you *absolutely* positive your changes won't do anything on non-DT
systems?  That definitely doesn't appear to be the case (and shouldn't
be, not all the world is DT), but now I look in more detail what it is
doing is exposing spidev for each explicitly described slave which is
substantially more safe and does address the main problem with
undescribed devices.

Please split this up so it can be reviewed, providing clear and specific
descriptions of each individual change (as covered in
SubmittingPatches).

This:

> -       status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
> +       status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);

also looks like a needless ABI change and the removal of the locking on
minor device allocation seems concerning and at least needs to be
covered in the changelog.

> The fact that devicetree does not allow talking to a SPI device from userspace
> without telling kernel what kind of protocol userspace is using is a regression
> from using board files. The kernel has no business knowing that. When you
> use a userspace driver it's userspace's job to manage protocols and devices.

The system as a whole, including both the kernel and userspace, has
every business knowing what the hardware is so it can understand how to
control it.  Userspace is no more able to discover what the magic
undescribed bits of hardware are automatically than the kernel is
without help from a hardware description, the DT isn't just something
for the kernel once you have userspace drivers.  

Directly enumerating spidev wasn't an especially great idea for board
files either, it was just an expedient hack that worked when the
hardware description was entirely in kernel and much easier to just go
and change but now we have an external ABI for hardware description and
we have to expect that people will use it as such.

> The purpose of this patchset is for the userspace applications to continue doing
> so with devicetree based systems without the need to fabricate fake hardware
> description. And the hardware description will necessarily be fake in the case
> when it is in fact userspace implementing and managing the drivers.

If userspace is managing to figure out how to control the device then
providing a description of the hardware is clearly within the bounds of
possibility and there is no need to fake anything.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

On 19 July 2016 at 14:44, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 19, 2016 at 01:17:52PM +0200, Michal Suchanek wrote:
>> On 19 July 2016 at 00:59, Mark Brown <broonie@kernel.org> wrote:
>
>> Looking into SPI more this whole point is mostly moot, anyway. Most of the
>> time the unused CS lines will not be multiplexed to the SPI controller so
>> even if the SPI master tries to use them it does nothing. Also most of the
>> time all the CS lines can be exported as GPIO regardless of their use for SPI.
>> And set to any possible level using the GPIO userspace API.
>
> Your system is not the entire world, Linux has to run on other systems
> too.  There are actual devices out there with inflexible pinmuxing, we
> can't just ignore them.

And they are not ignored.

>
>> In order to reduce the amount of objects and management I appended
>> the link object at the end of struct spi_device.
>
>> Is there a better way?
>
> That seems mostly fine, the problem is one of code review - because the
> patch does many logically distinct things in a single patch it is much
> harder to review than if it were split out.  There's a lot of mechanical
> refactoring which obscures the several more substantive changes that are
> being made at the same time and since most of those substantive changes
> are explicitly described at all it's hard to tell if they're intended or
> doing what is expected.
>
>> > I am getting completely fed up of saying this, it's not OK to just
>> > expose pins to userspace when we have no idea what they are connected
>> > to.
>
>> I am getting fed up with this argument.
>
>> First, this patchset only exposes to userspace pins that are configured as SPI
>> slaves in devicetree.
>
> Are you *absolutely* positive your changes won't do anything on non-DT
> systems?  That definitely doesn't appear to be the case (and shouldn't
> be, not all the world is DT), but now I look in more detail what it is
> doing is exposing spidev for each explicitly described slave which is
> substantially more safe and does address the main problem with
> undescribed devices.
>
> Please split this up so it can be reviewed, providing clear and specific
> descriptions of each individual change (as covered in
> SubmittingPatches).

OK, splitting this into multiple incremental patches should work better.

>
> This:
>
>> -       status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
>> +       status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);
>
> also looks like a needless ABI change

ABI change to what?

The spidev ABI should be the IOCTLs on /dev/spidev*. These are preserved.

What parts of the kernel or userspace should depend on spidev
devices belonging to class spi?

Changing this is deliberate so SPI devices can be enumerated walking
the bus without seeing the spidev devices. Or spidev devices can be
enumerated walking the spidev class. No need to maintain a list.

> and the removal of the locking on
> minor device allocation seems concerning and at least needs to be
> covered in the changelog.

This particular part should be covered by the SPI device list lock
because the attach/detach function should be only called with
the lock held.

It's certainly worth documenting as a separate change if it's still preserved
once spidev is again modularized.

>
>> The fact that devicetree does not allow talking to a SPI device from userspace
>> without telling kernel what kind of protocol userspace is using is a regression
>> from using board files. The kernel has no business knowing that. When you
>> use a userspace driver it's userspace's job to manage protocols and devices.
>
> The system as a whole, including both the kernel and userspace, has
> every business knowing what the hardware is so it can understand how to
> control it.  Userspace is no more able to discover what the magic
> undescribed bits of hardware are automatically than the kernel is
> without help from a hardware description, the DT isn't just something
> for the kernel once you have userspace drivers.
>
> Directly enumerating spidev wasn't an especially great idea for board
> files either, it was just an expedient hack that worked when the
> hardware description was entirely in kernel and much easier to just go
> and change but now we have an external ABI for hardware description and
> we have to expect that people will use it as such.
>
>> The purpose of this patchset is for the userspace applications to continue doing
>> so with devicetree based systems without the need to fabricate fake hardware
>> description. And the hardware description will necessarily be fake in the case
>> when it is in fact userspace implementing and managing the drivers.
>
> If userspace is managing to figure out how to control the device then
> providing a description of the hardware is clearly within the bounds of
> possibility and there is no need to fake anything.

However. maintaining the hardware description in multiple places is
redundant and error-prone. Since the userspace somehow managed to
figure it out on legacy kernels without devicetree it does not need
the information in devicetree. Users of such software will not want the
hardware description in devicetree and if forced to provide it will stub it out.

Also world is not all devicetree so userspace applications should be free
to consult devicetree information if present or use whatever other means
at their disposal to determine what hardware they are dealing with.
When portability is a concern consulting devicetree may be a secondary
source at best.

Thanks

Michal

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

On Tue, Jul 19, 2016 at 05:32:24PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 14:44, Mark Brown <broonie@kernel.org> wrote:

> >> -       status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
> >> +       status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);

> > also looks like a needless ABI change

> ABI change to what?

> The spidev ABI should be the IOCTLs on /dev/spidev*. These are preserved.

> What parts of the kernel or userspace should depend on spidev
> devices belonging to class spi?

It's a string that's exposed to userspace (via /proc/devices if nothing
else).

> > If userspace is managing to figure out how to control the device then
> > providing a description of the hardware is clearly within the bounds of
> > possibility and there is no need to fake anything.

> However. maintaining the hardware description in multiple places is
> redundant and error-prone. Since the userspace somehow managed to
> figure it out on legacy kernels without devicetree it does not need
> the information in devicetree. Users of such software will not want the
> hardware description in devicetree and if forced to provide it will stub it out.

Userspace is just as capable of looking at the DT as anything else, I'd
expect that where this is a viable way of deploying things people would
be writing udev/systemd magic to fire up userspace drivers automatically
when they see suitable hardware.  The only way this has ever worked in
the past has been with system specific hacks which lead to fragile,
non-portable userpaces that are hard to upgrade.  If we're trying to say
this is an interface that's there for all devices then we shouldn't be
defining it in a way that makes it difficult to automatically configure.

> Also world is not all devicetree so userspace applications should be free
> to consult devicetree information if present or use whatever other means
> at their disposal to determine what hardware they are dealing with.
> When portability is a concern consulting devicetree may be a secondary
> source at best.

ACPI has similar facilities (it's got a direct translation of DT in it
these days so most things with a DT binding also have an ACPI one for
free).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

On 19 July 2016 at 19:19, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 19, 2016 at 05:32:24PM +0200, Michal Suchanek wrote:
>> On 19 July 2016 at 14:44, Mark Brown <broonie@kernel.org> wrote:
>
>> >> -       status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
>> >> +       status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);
>
>> > also looks like a needless ABI change
>
>> ABI change to what?
>
>> The spidev ABI should be the IOCTLs on /dev/spidev*. These are preserved.
>
>> What parts of the kernel or userspace should depend on spidev
>> devices belonging to class spi?
>
> It's a string that's exposed to userspace (via /proc/devices if nothing
> else).

Yes, the change can be detected from userspace. The question is
more like 'Is there any sane reason anyone would rely on seeing
that exact string for something?'

BTW the classes are also exported through /sys so the spidev devices
would look differently there also.

>
>> > If userspace is managing to figure out how to control the device then
>> > providing a description of the hardware is clearly within the bounds of
>> > possibility and there is no need to fake anything.
>
>> However. maintaining the hardware description in multiple places is
>> redundant and error-prone. Since the userspace somehow managed to
>> figure it out on legacy kernels without devicetree it does not need
>> the information in devicetree. Users of such software will not want the
>> hardware description in devicetree and if forced to provide it will stub it out.
>
> Userspace is just as capable of looking at the DT as anything else, I'd
> expect that where this is a viable way of deploying things people would
> be writing udev/systemd magic to fire up userspace drivers automatically
> when they see suitable hardware.  The only way this has ever worked in
> the past has been with system specific hacks which lead to fragile,
> non-portable userpaces that are hard to upgrade.  If we're trying to say
> this is an interface that's there for all devices then we shouldn't be
> defining it in a way that makes it difficult to automatically configure.

Oh sure. Once a device-specific hack turns out to be something
desirable to maintain over long time and deploy on variety of devices we
have infrastructure to automate that.

That does not make device-specific hacks go away. Initially the user
will do only as much as it takes to get the device working right now
and that does not include writing devicetree description of the device
when it is used with userspace driver.

>
>> Also world is not all devicetree so userspace applications should be free
>> to consult devicetree information if present or use whatever other means
>> at their disposal to determine what hardware they are dealing with.
>> When portability is a concern consulting devicetree may be a secondary
>> source at best.
>
> ACPI has similar facilities (it's got a direct translation of DT in it
> these days so most things with a DT binding also have an ACPI one for
> free).

There is even a patch floating around for adding empty DT and some proxy
nodes for existing hardware so dynamically connected devices can be
configured with devicetree overlays on boards without devicetree.

So you can have devicetree description of the hardware. It's just sometimes
not useful.

Thanks

Michal

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On Tue, Jul 19, 2016 at 08:33:12PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 19:19, Mark Brown <broonie@kernel.org> wrote:

> > It's a string that's exposed to userspace (via /proc/devices if nothing
> > else).

> Yes, the change can be detected from userspace. The question is
> more like 'Is there any sane reason anyone would rely on seeing
> that exact string for something?'

People using it to create device files at runtime or detect board
variants would be the main ones.

> BTW the classes are also exported through /sys so the spidev devices
> would look differently there also.

Yup.

> That does not make device-specific hacks go away. Initially the user
> will do only as much as it takes to get the device working right now
> and that does not include writing devicetree description of the device
> when it is used with userspace driver.

Depends, especially if you see some long term use for your code it's
common to start with the enumeration part.  It's not there currently so
people can't do that but part of getting the good practice out there is
to make it the path of least resistance.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [lkp] [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Fengguang Wu]

[-- Attachment #1: Type: text/plain, Size: 3497 bytes --]

Hi,

[auto build test WARNING on v4.7-rc7]
[cannot apply to next-20160719]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Suchanek/Spidev-usability-patchset-update/20160720-055513
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

    include/linux/init.h:1: warning: no structured comments found
    kernel/sched/core.c:2081: warning: No description found for parameter 'cookie'
    kernel/sys.c:1: warning: no structured comments found
    drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
>> include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev_devt'
>> include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev'
>> include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev_lock'
>> include/linux/spi/spi.h:195: warning: No description found for parameter 'buf_lock'
>> include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev_users'
>> include/linux/spi/spi.h:195: warning: No description found for parameter 'tx_buffer'
>> include/linux/spi/spi.h:195: warning: No description found for parameter 'rx_buffer'
>> include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev_speed_hz'

git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 2797999799fc3b7f10f680b3e3fa34ca56e1385d
vim +/spidev_devt +195 include/linux/spi/spi.h

33e34dc6 David Brownell  2007-05-08  179  	 *  - ...
33e34dc6 David Brownell  2007-05-08  180  	 */
27979997 Michal Suchanek 2016-07-19  181  
27979997 Michal Suchanek 2016-07-19  182  	/* spidev stuff */
27979997 Michal Suchanek 2016-07-19  183  #if config_enabled(CONFIG_SPI_SPIDEV)
27979997 Michal Suchanek 2016-07-19  184  	dev_t                   spidev_devt;
27979997 Michal Suchanek 2016-07-19  185  	struct device *		spidev;
27979997 Michal Suchanek 2016-07-19  186  	spinlock_t              spidev_lock;
27979997 Michal Suchanek 2016-07-19  187  
27979997 Michal Suchanek 2016-07-19  188  	/* TX/RX buffers are NULL unless this device is open (users > 0) */
27979997 Michal Suchanek 2016-07-19  189  	struct mutex            buf_lock;
27979997 Michal Suchanek 2016-07-19  190  	unsigned                spidev_users;
27979997 Michal Suchanek 2016-07-19  191  	u8                      *tx_buffer;
27979997 Michal Suchanek 2016-07-19  192  	u8                      *rx_buffer;
27979997 Michal Suchanek 2016-07-19  193  	u32                     spidev_speed_hz;
27979997 Michal Suchanek 2016-07-19  194  #endif /*CONFIG_SPI_SPIDEV*/
8ae12a0d David Brownell  2006-01-08 @195  };
8ae12a0d David Brownell  2006-01-08  196  
8ae12a0d David Brownell  2006-01-08  197  static inline struct spi_device *to_spi_device(struct device *dev)
8ae12a0d David Brownell  2006-01-08  198  {
b885244e David Brownell  2006-01-08  199  	return dev ? container_of(dev, struct spi_device, dev) : NULL;
8ae12a0d David Brownell  2006-01-08  200  }
8ae12a0d David Brownell  2006-01-08  201  
8ae12a0d David Brownell  2006-01-08  202  /* most drivers won't need to care about device refcounting */
8ae12a0d David Brownell  2006-01-08  203  static inline struct spi_device *spi_dev_get(struct spi_device *spi)



---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6370 bytes --]

[PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

SPI slave devices are not created when looking up driver for the slave
fails. Create a device anyway so it can be used with spidev.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/spi/spi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f3ea768..f0de2ec 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1489,9 +1489,8 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc)
 	rc = of_modalias_node(nc, spi->modalias,
 				sizeof(spi->modalias));
 	if (rc < 0) {
-		dev_err(&master->dev, "cannot find modalias for %s\n",
+		dev_warn(&master->dev, "cannot find modalias for %s\n",
 			nc->full_name);
-		goto err_out;
 	}
 
 	/* Device address */
-- 
2.8.1

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:
> SPI slave devices are not created when looking up driver for the slave
> fails. Create a device anyway so it can be used with spidev.

>  	rc = of_modalias_node(nc, spi->modalias,
>  				sizeof(spi->modalias));
>  	if (rc < 0) {
> -		dev_err(&master->dev, "cannot find modalias for %s\n",
> +		dev_warn(&master->dev, "cannot find modalias for %s\n",

Nothing has change since you last sent this patch which converts
of_modailias_node() into something which looks up a driver so the
patch description still fails to describe what the patch is doing.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

Hello,

On 19 July 2016 at 01:02, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:
>> SPI slave devices are not created when looking up driver for the slave
>> fails. Create a device anyway so it can be used with spidev.
>
>>       rc = of_modalias_node(nc, spi->modalias,
>>                               sizeof(spi->modalias));
>>       if (rc < 0) {
>> -             dev_err(&master->dev, "cannot find modalias for %s\n",
>> +             dev_warn(&master->dev, "cannot find modalias for %s\n",
>
> Nothing has change since you last sent this patch which converts
> of_modailias_node() into something which looks up a driver so the
> patch description still fails to describe what the patch is doing.
>
> Please don't ignore review comments, people are generally making them
> for a reason and are likely to have the same concerns if issues remain
> unaddressed.  Having to repeat the same comments can get repetitive and
> make people question the value of time spent reviewing.  If you disagree
> with the review comments that's fine but you need to reply and discuss
> your concerns so that the reviewer can understand your decisions.

I have split the other part of the patch. Regarding the commit message
if you have suggestion for better wording please do share it.

>From my point of view the conceptual change described in the commit message
is that whenever SPI slave node is encountered in devicetree you get either
a device with active driver or a device with no driver whereas
previously you either
got a device with active driver or no device. So yes, it's about
allowing SPI slave
devices without a driver bound to them.

The technical implementation detail that can be seen in the patch is
ignoring the
return value of of_modalias_node.

Thanks

Michal

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]

On Tue, Jul 19, 2016 at 10:31:54AM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 01:02, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:

> >> SPI slave devices are not created when looking up driver for the slave
> >> fails. Create a device anyway so it can be used with spidev.

> > Nothing has change since you last sent this patch which converts
> > of_modailias_node() into something which looks up a driver so the
> > patch description still fails to describe what the patch is doing.

> I have split the other part of the patch. Regarding the commit message
> if you have suggestion for better wording please do share it.

As covered in SubmittingPatches your commit message should describe what
the change does and what the intended effect is.  If we were looking for
a device driver the code would be looking up a struct device_driver or
some other struct that contains one.  

> From my point of view the conceptual change described in the commit message
> is that whenever SPI slave node is encountered in devicetree you get either
> a device with active driver or a device with no driver whereas
> previously you either
> got a device with active driver or no device. So yes, it's about

This is not the case, it is perfectly possible to have a device with no
driver bound to it otherwise it would not be possible to use loadable
modules for drivers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 2/3] spi: of: allow instantiating slaves without a driver

[Michal Suchanek]

On 19 July 2016 at 12:52, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 19, 2016 at 10:31:54AM +0200, Michal Suchanek wrote:
>> On 19 July 2016 at 01:02, Mark Brown <broonie@kernel.org> wrote:
>> > On Tue, Jul 19, 2016 at 12:35:41AM +0200, Michal Suchanek wrote:
>
>> >> SPI slave devices are not created when looking up driver for the slave
>> >> fails. Create a device anyway so it can be used with spidev.
>
>> > Nothing has change since you last sent this patch which converts
>> > of_modailias_node() into something which looks up a driver so the
>> > patch description still fails to describe what the patch is doing.
>
>> I have split the other part of the patch. Regarding the commit message
>> if you have suggestion for better wording please do share it.
>
> As covered in SubmittingPatches your commit message should describe what
> the change does and what the intended effect is.  If we were looking for
> a device driver the code would be looking up a struct device_driver or
> some other struct that contains one.
>
>> From my point of view the conceptual change described in the commit message
>> is that whenever SPI slave node is encountered in devicetree you get either
>> a device with active driver or a device with no driver whereas
>> previously you either
>> got a device with active driver or no device. So yes, it's about
>
> This is not the case, it is perfectly possible to have a device with no
> driver bound to it otherwise it would not be possible to use loadable
> modules for drivers.

Ok, I missed this part. That makes the commit message indeed broken.

Thanks

Michal

[PATCH v3 3/3] spi: core: allow creating devices without spi-max-frequency

[Michal Suchanek]

Since the previous patch allows creating SPI slaves without specifying
what device the CS connects to make spi-max-frequency also optional.

The spi-max-frequency is device-specific and the driver can set it
anyway.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/spi/spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f0de2ec..08e65e4 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1554,11 +1554,11 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc)
 	/* Device speed */
 	rc = of_property_read_u32(nc, "spi-max-frequency", &value);
 	if (rc) {
-		dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
+		dev_warn(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
 			nc->full_name, rc);
-		goto err_out;
+	} else {
+		spi->max_speed_hz = value;
 	}
-	spi->max_speed_hz = value;
 
 	/* Store a pointer to the node in the device structure */
 	of_node_get(nc);
-- 
2.8.1