[PATCH] spi: Force the registration of the spidev devices

[PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

spidev device registration has always been a controversial subject since the
move to DT.

Obviously, a spidev node has nothing to do in the DT, and the position so far
has been to add the compatible of the devices to drive through spidev to the
list of the compatibles spidev can handle.

While this is nicer than the DT solution because of its accurate hardware
representation, it's still not perfect because you might not have access to the
DT, or you might be driving a completely generic device (such as a
microcontroller) that might be used for something else in a different
context/board.

Solve this by registering automatically spidev devices for all the unused chip
selects when a master registers itself against the spi core.

This also adds an i2cdev-like feeling, where you get all the spidev devices all
the time, without any modification.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/spi/spi.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d5d7d2235163..e6ca46e1e0fc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1384,6 +1384,52 @@ static void acpi_register_spi_devices(struct spi_master *master)
 static inline void acpi_register_spi_devices(struct spi_master *master) {}
 #endif /* CONFIG_ACPI */
 
+#ifdef CONFIG_SPI_SPIDEV
+static void spidev_register_devices(struct spi_master *master)
+{
+	struct spi_device *spi;
+	int i, status;
+
+	for (i = 0; i < master->num_chipselect; i++) {
+		spi = spi_alloc_device(master);
+		if (!spi) {
+			dev_err(&master->dev, "Couldn't allocate spidev device\n");
+			continue;
+		}
+
+		spi->chip_select = i;
+		strlcpy(spi->modalias, "spidev", sizeof(spi->modalias));
+
+		/*
+		 * This is far from perfect since an addition might be
+		 * done between here and the call to spi_add_device,
+		 * but we can't hold the lock and call spi_add_device
+		 * either, as it would trigger a deadlock.
+		 *
+		 * If such a race occurs, spi_add_device will still
+		 * catch it though, as it also checks for devices
+		 * being registered several times on the same chip
+		 * select.
+		*/
+		status = bus_for_each_dev(&spi_bus_type, NULL, spi,
+					  spi_dev_check);
+		if (status) {
+			dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
+			spi_dev_put(spi);
+			continue;
+		}
+
+		if (spi_add_device(spi)) {
+			dev_err(&master->dev, "Couldn't add spidev device\n");
+			spi_dev_put(spi);
+		}
+	}
+
+}
+#else
+static inline void spidev_register_devices(struct spi_master *master) {}
+#endif /* CONFIG_SPI_SPIDEV */
+
 static void spi_master_release(struct device *dev)
 {
 	struct spi_master *master;
@@ -1575,6 +1621,7 @@ int spi_register_master(struct spi_master *master)
 	/* Register devices from the device tree and ACPI */
 	of_register_spi_devices(master);
 	acpi_register_spi_devices(master);
+	spidev_register_devices(master);
 done:
 	return status;
 }
-- 
2.4.0

[PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

spidev device registration has always been a controversial subject since the
move to DT.

Obviously, a spidev node has nothing to do in the DT, and the position so far
has been to add the compatible of the devices to drive through spidev to the
list of the compatibles spidev can handle.

While this is nicer than the DT solution because of its accurate hardware
representation, it's still not perfect because you might not have access to the
DT, or you might be driving a completely generic device (such as a
microcontroller) that might be used for something else in a different
context/board.

Solve this by registering automatically spidev devices for all the unused chip
selects when a master registers itself against the spi core.

This also adds an i2cdev-like feeling, where you get all the spidev devices all
the time, without any modification.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/spi/spi.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d5d7d2235163..e6ca46e1e0fc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1384,6 +1384,52 @@ static void acpi_register_spi_devices(struct spi_master *master)
 static inline void acpi_register_spi_devices(struct spi_master *master) {}
 #endif /* CONFIG_ACPI */
 
+#ifdef CONFIG_SPI_SPIDEV
+static void spidev_register_devices(struct spi_master *master)
+{
+	struct spi_device *spi;
+	int i, status;
+
+	for (i = 0; i < master->num_chipselect; i++) {
+		spi = spi_alloc_device(master);
+		if (!spi) {
+			dev_err(&master->dev, "Couldn't allocate spidev device\n");
+			continue;
+		}
+
+		spi->chip_select = i;
+		strlcpy(spi->modalias, "spidev", sizeof(spi->modalias));
+
+		/*
+		 * This is far from perfect since an addition might be
+		 * done between here and the call to spi_add_device,
+		 * but we can't hold the lock and call spi_add_device
+		 * either, as it would trigger a deadlock.
+		 *
+		 * If such a race occurs, spi_add_device will still
+		 * catch it though, as it also checks for devices
+		 * being registered several times on the same chip
+		 * select.
+		*/
+		status = bus_for_each_dev(&spi_bus_type, NULL, spi,
+					  spi_dev_check);
+		if (status) {
+			dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
+			spi_dev_put(spi);
+			continue;
+		}
+
+		if (spi_add_device(spi)) {
+			dev_err(&master->dev, "Couldn't add spidev device\n");
+			spi_dev_put(spi);
+		}
+	}
+
+}
+#else
+static inline void spidev_register_devices(struct spi_master *master) {}
+#endif /* CONFIG_SPI_SPIDEV */
+
 static void spi_master_release(struct device *dev)
 {
 	struct spi_master *master;
@@ -1575,6 +1621,7 @@ int spi_register_master(struct spi_master *master)
 	/* Register devices from the device tree and ACPI */
 	of_register_spi_devices(master);
 	acpi_register_spi_devices(master);
+	spidev_register_devices(master);
 done:
 	return status;
 }
-- 
2.4.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: Add option to bind spidev to all chipselects

[Michal Suchanek]

Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
set.  Rename spidev devices to avoid sysfs conflict.

This allows dynamically loading SPI device overlays or communicating
with SPI devices configured by a kernel driver from userspace.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/spi/Kconfig     | 13 +++++++++
 drivers/spi/spi.c       | 74 ++++++++++++++++++++++++++++++++++---------------
 include/linux/spi/spi.h |  1 +
 3 files changed, 65 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 198f96b..b477828 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -651,6 +651,19 @@ config SPI_SPIDEV
 	  Note that this application programming interface is EXPERIMENTAL
 	  and hence SUBJECT TO CHANGE WITHOUT NOTICE while it stabilizes.
 
+config SPIDEV_SHADOW
+	depends on SPI_SPIDEV
+	bool "Allow spidev access to configured SPI devices (DANGEROUS)"
+	help
+	  This creates a spidev device node for every chipselect.
+
+	  It is possible to access even SPI devices which are in use by a
+	  kernel driver. This allows invoking features not in use by the kernel
+	  or checking device state from userspace when the kernel driver fails.
+
+	  Sending out-of-order messages to the device or reconfiguring the
+	  device might cause driver failure. DANGEROUS
+
 config SPI_TLE62X0
 	tristate "Infineon TLE62X0 (for power switching)"
 	depends on SYSFS
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e6ca46e..b48a0dc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -254,16 +254,23 @@ struct spi_device *spi_alloc_device(struct spi_master *master)
 }
 EXPORT_SYMBOL_GPL(spi_alloc_device);
 
-static void spi_dev_set_name(struct spi_device *spi)
+static void spi_dev_set_name(struct spi_device *spi, const char * alias)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
 
 	if (adev) {
-		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
+		if (alias)
+			dev_set_name(&spi->dev, "%s-%s", alias, acpi_dev_name(adev));
+		else
+			dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
 		return;
 	}
 
-	dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev),
+	if (alias)
+		dev_set_name(&spi->dev, "%s%u.%u", alias, spi->master->bus_num,
+		     spi->chip_select);
+	else
+		dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev),
 		     spi->chip_select);
 }
 
@@ -272,22 +279,25 @@ static int spi_dev_check(struct device *dev, void *data)
 	struct spi_device *spi = to_spi_device(dev);
 	struct spi_device *new_spi = data;
 
-	if (spi->master == new_spi->master &&
-	    spi->chip_select == new_spi->chip_select)
+	if (spi->master == new_spi->master
+	    && spi->chip_select == new_spi->chip_select
+#ifdef CONFIG_SPIDEV_SHADOW
+	    && !spi->shadow && !new_spi->shadow
+#endif
+	    )
 		return -EBUSY;
 	return 0;
 }
 
 /**
- * spi_add_device - Add spi_device allocated with spi_alloc_device
+ * spi_add_device_alias - Add spi_device allocated with spi_alloc_device
+ * possibly even when device for the CS exists.
  * @spi: spi_device to register
+ * @alias: string used as device name prefix or NULL
  *
- * Companion function to spi_alloc_device.  Devices allocated with
- * spi_alloc_device can be added onto the spi bus with this function.
- *
- * Returns 0 on success; negative errno on failure
+ * See spi_add_device
  */
-int spi_add_device(struct spi_device *spi)
+static inline int spi_add_device_alias(struct spi_device *spi, const char * alias)
 {
 	static DEFINE_MUTEX(spi_add_lock);
 	struct spi_master *master = spi->master;
@@ -303,7 +313,8 @@ int spi_add_device(struct spi_device *spi)
 	}
 
 	/* Set the bus ID string */
-	spi_dev_set_name(spi);
+	spi_dev_set_name(spi, alias);
+	spi->shadow = !!alias;
 
 	/* We need to make sure there's no other device with this
 	 * chipselect **BEFORE** we call setup(), else we'll trash
@@ -321,15 +332,17 @@ int spi_add_device(struct spi_device *spi)
 	if (master->cs_gpios)
 		spi->cs_gpio = master->cs_gpios[spi->chip_select];
 
-	/* Drivers may modify this initial i/o setup, but will
-	 * normally rely on the device being setup.  Devices
-	 * using SPI_CS_HIGH can't coexist well otherwise...
-	 */
-	status = spi_setup(spi);
-	if (status < 0) {
-		dev_err(dev, "can't setup %s, status %d\n",
-				dev_name(&spi->dev), status);
-		goto done;
+	if (!alias) {
+			/* Drivers may modify this initial i/o setup, but will
+			 * normally rely on the device being setup.  Devices
+			 * using SPI_CS_HIGH can't coexist well otherwise...
+			 */
+			status = spi_setup(spi);
+			if (status < 0) {
+					dev_err(dev, "can't setup %s, status %d\n",
+						dev_name(&spi->dev), status);
+					goto done;
+			}
 	}
 
 	/* Device may be bound to an active driver when this returns */
@@ -344,6 +357,20 @@ done:
 	mutex_unlock(&spi_add_lock);
 	return status;
 }
+
+/**
+ * spi_add_device - Add spi_device allocated with spi_alloc_device
+ * @spi: spi_device to register
+ *
+ * Companion function to spi_alloc_device.  Devices allocated with
+ * spi_alloc_device can be added onto the spi bus with this function.
+ *
+ * Returns 0 on success; negative errno on failure
+ */
+int spi_add_device(struct spi_device *spi)
+{
+	return spi_add_device_alias(spi, NULL);
+}
 EXPORT_SYMBOL_GPL(spi_add_device);
 
 /**
@@ -1400,6 +1427,7 @@ static void spidev_register_devices(struct spi_master *master)
 		spi->chip_select = i;
 		strlcpy(spi->modalias, "spidev", sizeof(spi->modalias));
 
+#ifndef CONFIG_SPIDEV_SHADOW
 		/*
 		 * This is far from perfect since an addition might be
 		 * done between here and the call to spi_add_device,
@@ -1418,8 +1446,8 @@ static void spidev_register_devices(struct spi_master *master)
 			spi_dev_put(spi);
 			continue;
 		}
-
-		if (spi_add_device(spi)) {
+#endif /* CONFIG_SPIDEV_SHADOW */
+		if (spi_add_device_alias(spi, "spidev")) {
 			dev_err(&master->dev, "Couldn't add spidev device\n");
 			spi_dev_put(spi);
 		}
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index d673072..8368714 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -97,6 +97,7 @@ struct spi_device {
 	void			*controller_data;
 	char			modalias[SPI_NAME_SIZE];
 	int			cs_gpio;	/* chip select gpio */
+	int			shadow; /* ignore when determining if CS is in use */
 
 	/*
 	 * likely need more hooks for more protocol options affecting how
-- 
2.1.4

[PATCH] spi: Add option to bind spidev to all chipselects

[Michal Suchanek]

Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
set.  Rename spidev devices to avoid sysfs conflict.

This allows dynamically loading SPI device overlays or communicating
with SPI devices configured by a kernel driver from userspace.

Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/Kconfig     | 13 +++++++++
 drivers/spi/spi.c       | 74 ++++++++++++++++++++++++++++++++++---------------
 include/linux/spi/spi.h |  1 +
 3 files changed, 65 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 198f96b..b477828 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -651,6 +651,19 @@ config SPI_SPIDEV
 	  Note that this application programming interface is EXPERIMENTAL
 	  and hence SUBJECT TO CHANGE WITHOUT NOTICE while it stabilizes.
 
+config SPIDEV_SHADOW
+	depends on SPI_SPIDEV
+	bool "Allow spidev access to configured SPI devices (DANGEROUS)"
+	help
+	  This creates a spidev device node for every chipselect.
+
+	  It is possible to access even SPI devices which are in use by a
+	  kernel driver. This allows invoking features not in use by the kernel
+	  or checking device state from userspace when the kernel driver fails.
+
+	  Sending out-of-order messages to the device or reconfiguring the
+	  device might cause driver failure. DANGEROUS
+
 config SPI_TLE62X0
 	tristate "Infineon TLE62X0 (for power switching)"
 	depends on SYSFS
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e6ca46e..b48a0dc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -254,16 +254,23 @@ struct spi_device *spi_alloc_device(struct spi_master *master)
 }
 EXPORT_SYMBOL_GPL(spi_alloc_device);
 
-static void spi_dev_set_name(struct spi_device *spi)
+static void spi_dev_set_name(struct spi_device *spi, const char * alias)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&spi->dev);
 
 	if (adev) {
-		dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
+		if (alias)
+			dev_set_name(&spi->dev, "%s-%s", alias, acpi_dev_name(adev));
+		else
+			dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
 		return;
 	}
 
-	dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev),
+	if (alias)
+		dev_set_name(&spi->dev, "%s%u.%u", alias, spi->master->bus_num,
+		     spi->chip_select);
+	else
+		dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev),
 		     spi->chip_select);
 }
 
@@ -272,22 +279,25 @@ static int spi_dev_check(struct device *dev, void *data)
 	struct spi_device *spi = to_spi_device(dev);
 	struct spi_device *new_spi = data;
 
-	if (spi->master == new_spi->master &&
-	    spi->chip_select == new_spi->chip_select)
+	if (spi->master == new_spi->master
+	    && spi->chip_select == new_spi->chip_select
+#ifdef CONFIG_SPIDEV_SHADOW
+	    && !spi->shadow && !new_spi->shadow
+#endif
+	    )
 		return -EBUSY;
 	return 0;
 }
 
 /**
- * spi_add_device - Add spi_device allocated with spi_alloc_device
+ * spi_add_device_alias - Add spi_device allocated with spi_alloc_device
+ * possibly even when device for the CS exists.
  * @spi: spi_device to register
+ * @alias: string used as device name prefix or NULL
  *
- * Companion function to spi_alloc_device.  Devices allocated with
- * spi_alloc_device can be added onto the spi bus with this function.
- *
- * Returns 0 on success; negative errno on failure
+ * See spi_add_device
  */
-int spi_add_device(struct spi_device *spi)
+static inline int spi_add_device_alias(struct spi_device *spi, const char * alias)
 {
 	static DEFINE_MUTEX(spi_add_lock);
 	struct spi_master *master = spi->master;
@@ -303,7 +313,8 @@ int spi_add_device(struct spi_device *spi)
 	}
 
 	/* Set the bus ID string */
-	spi_dev_set_name(spi);
+	spi_dev_set_name(spi, alias);
+	spi->shadow = !!alias;
 
 	/* We need to make sure there's no other device with this
 	 * chipselect **BEFORE** we call setup(), else we'll trash
@@ -321,15 +332,17 @@ int spi_add_device(struct spi_device *spi)
 	if (master->cs_gpios)
 		spi->cs_gpio = master->cs_gpios[spi->chip_select];
 
-	/* Drivers may modify this initial i/o setup, but will
-	 * normally rely on the device being setup.  Devices
-	 * using SPI_CS_HIGH can't coexist well otherwise...
-	 */
-	status = spi_setup(spi);
-	if (status < 0) {
-		dev_err(dev, "can't setup %s, status %d\n",
-				dev_name(&spi->dev), status);
-		goto done;
+	if (!alias) {
+			/* Drivers may modify this initial i/o setup, but will
+			 * normally rely on the device being setup.  Devices
+			 * using SPI_CS_HIGH can't coexist well otherwise...
+			 */
+			status = spi_setup(spi);
+			if (status < 0) {
+					dev_err(dev, "can't setup %s, status %d\n",
+						dev_name(&spi->dev), status);
+					goto done;
+			}
 	}
 
 	/* Device may be bound to an active driver when this returns */
@@ -344,6 +357,20 @@ done:
 	mutex_unlock(&spi_add_lock);
 	return status;
 }
+
+/**
+ * spi_add_device - Add spi_device allocated with spi_alloc_device
+ * @spi: spi_device to register
+ *
+ * Companion function to spi_alloc_device.  Devices allocated with
+ * spi_alloc_device can be added onto the spi bus with this function.
+ *
+ * Returns 0 on success; negative errno on failure
+ */
+int spi_add_device(struct spi_device *spi)
+{
+	return spi_add_device_alias(spi, NULL);
+}
 EXPORT_SYMBOL_GPL(spi_add_device);
 
 /**
@@ -1400,6 +1427,7 @@ static void spidev_register_devices(struct spi_master *master)
 		spi->chip_select = i;
 		strlcpy(spi->modalias, "spidev", sizeof(spi->modalias));
 
+#ifndef CONFIG_SPIDEV_SHADOW
 		/*
 		 * This is far from perfect since an addition might be
 		 * done between here and the call to spi_add_device,
@@ -1418,8 +1446,8 @@ static void spidev_register_devices(struct spi_master *master)
 			spi_dev_put(spi);
 			continue;
 		}
-
-		if (spi_add_device(spi)) {
+#endif /* CONFIG_SPIDEV_SHADOW */
+		if (spi_add_device_alias(spi, "spidev")) {
 			dev_err(&master->dev, "Couldn't add spidev device\n");
 			spi_dev_put(spi);
 		}
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index d673072..8368714 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -97,6 +97,7 @@ struct spi_device {
 	void			*controller_data;
 	char			modalias[SPI_NAME_SIZE];
 	int			cs_gpio;	/* chip select gpio */
+	int			shadow; /* ignore when determining if CS is in use */
 
 	/*
 	 * likely need more hooks for more protocol options affecting how
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Add option to bind spidev to all chipselects

[Maxime Ripard]

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

Hi,

On Wed, May 13, 2015 at 09:34:41AM -0000, Michal Suchanek wrote:
> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
> set.  Rename spidev devices to avoid sysfs conflict.
> 
> This allows dynamically loading SPI device overlays or communicating
> with SPI devices configured by a kernel driver from userspace.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

Output from checkpatch:
total: 2 errors, 4 warnings, 4 checks, 157 lines checked

...

I told you a few times already to run checkpatch before sending your
patches, apparently you make a point at ignoring me. Fine.

That being said, I'm not sure this is the right approach, or at least,
it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still
have the same issue with addition of new devices on previously unused
chip selects, and where we have an spidev device now.

What I think we should do is, when a new device is created, we just
lookup the modalias of the spi_device associated to it.

If that modalias is "spidev", then unregister the spidev device,
register the new device, you're done. If not, return an error.

On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good
idea. There's a good chance it will break the driver by doing stuff
behind its back, possibly in a way that will harm the whole kernel,
and it's something we usually try to avoid.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Add option to bind spidev to all chipselects

[Maxime Ripard]

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

Hi,

On Wed, May 13, 2015 at 09:34:41AM -0000, Michal Suchanek wrote:
> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
> set.  Rename spidev devices to avoid sysfs conflict.
> 
> This allows dynamically loading SPI device overlays or communicating
> with SPI devices configured by a kernel driver from userspace.
> 
> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Output from checkpatch:
total: 2 errors, 4 warnings, 4 checks, 157 lines checked

...

I told you a few times already to run checkpatch before sending your
patches, apparently you make a point at ignoring me. Fine.

That being said, I'm not sure this is the right approach, or at least,
it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still
have the same issue with addition of new devices on previously unused
chip selects, and where we have an spidev device now.

What I think we should do is, when a new device is created, we just
lookup the modalias of the spi_device associated to it.

If that modalias is "spidev", then unregister the spidev device,
register the new device, you're done. If not, return an error.

On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good
idea. There's a good chance it will break the driver by doing stuff
behind its back, possibly in a way that will harm the whole kernel,
and it's something we usually try to avoid.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Add option to bind spidev to all chipselects

[Michal Suchanek]

On 13 May 2015 at 12:16, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Wed, May 13, 2015 at 09:34:41AM -0000, Michal Suchanek wrote:
>> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
>> set.  Rename spidev devices to avoid sysfs conflict.
>>
>> This allows dynamically loading SPI device overlays or communicating
>> with SPI devices configured by a kernel driver from userspace.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>
> Output from checkpatch:
> total: 2 errors, 4 warnings, 4 checks, 157 lines checked
>
> ...
>
> I told you a few times already to run checkpatch before sending your
> patches, apparently you make a point at ignoring me. Fine.

That's a good idea to run, yes.

Sorry about that.

I also discovered an additional issue with unused variable when the
config option is set.

>
> That being said, I'm not sure this is the right approach, or at least,
> it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still
> have the same issue with addition of new devices on previously unused
> chip selects, and where we have an spidev device now.
>
> What I think we should do is, when a new device is created, we just
> lookup the modalias of the spi_device associated to it.
>
> If that modalias is "spidev", then unregister the spidev device,
> register the new device, you're done. If not, return an error.

Yes, that's what I intend to look into eventually. However, this patch
is still useful and allows both accessing unused bus with spidev and
dynamically loading overlays that would use the bus.

>
> On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good
> idea. There's a good chance it will break the driver by doing stuff
> behind its back, possibly in a way that will harm the whole kernel,
> and it's something we usually try to avoid.

What is the possibility to harm the whole kernel?

If the kernel crashes because the device misses a message this is
somewhat worrying.

You could see it as a developer option similar to SCSI error injection
and others that can introduce states that would normally occur only
rarely.

Thanks

Michal

Re: [PATCH] spi: Add option to bind spidev to all chipselects

[Michal Suchanek]

On 13 May 2015 at 12:16, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Wed, May 13, 2015 at 09:34:41AM -0000, Michal Suchanek wrote:
>> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
>> set.  Rename spidev devices to avoid sysfs conflict.
>>
>> This allows dynamically loading SPI device overlays or communicating
>> with SPI devices configured by a kernel driver from userspace.
>>
>> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Output from checkpatch:
> total: 2 errors, 4 warnings, 4 checks, 157 lines checked
>
> ...
>
> I told you a few times already to run checkpatch before sending your
> patches, apparently you make a point at ignoring me. Fine.

That's a good idea to run, yes.

Sorry about that.

I also discovered an additional issue with unused variable when the
config option is set.

>
> That being said, I'm not sure this is the right approach, or at least,
> it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still
> have the same issue with addition of new devices on previously unused
> chip selects, and where we have an spidev device now.
>
> What I think we should do is, when a new device is created, we just
> lookup the modalias of the spi_device associated to it.
>
> If that modalias is "spidev", then unregister the spidev device,
> register the new device, you're done. If not, return an error.

Yes, that's what I intend to look into eventually. However, this patch
is still useful and allows both accessing unused bus with spidev and
dynamically loading overlays that would use the bus.

>
> On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good
> idea. There's a good chance it will break the driver by doing stuff
> behind its back, possibly in a way that will harm the whole kernel,
> and it's something we usually try to avoid.

What is the possibility to harm the whole kernel?

If the kernel crashes because the device misses a message this is
somewhat worrying.

You could see it as a developer option similar to SCSI error injection
and others that can introduce states that would normally occur only
rarely.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Add option to bind spidev to all chipselects

[Mark Brown]

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

On Wed, May 13, 2015 at 09:34:41AM -0000, Michal Suchanek wrote:
> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
> set.  Rename spidev devices to avoid sysfs conflict.

To repeat my previous feedback I don't see any way in which this is
sane, having two drivers simultaneously controlling the same device is
an obviously terrible idea.

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

Re: [PATCH] spi: Add option to bind spidev to all chipselects

[Mark Brown]

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

On Wed, May 13, 2015 at 09:34:41AM -0000, Michal Suchanek wrote:
> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
> set.  Rename spidev devices to avoid sysfs conflict.

To repeat my previous feedback I don't see any way in which this is
sane, having two drivers simultaneously controlling the same device is
an obviously terrible idea.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:

> While this is nicer than the DT solution because of its accurate hardware
> representation, it's still not perfect because you might not have access to the
> DT, or you might be driving a completely generic device (such as a
> microcontroller) that might be used for something else in a different
> context/board.

Greg, you're copied on this because this seems to be a generic problem
that should perhaps be solved at a driver model level - having a way to
bind userspace access to devices that we don't otherwise have a driver
for.  The subsystem could specify the UIO driver to use when no other
driver is available.

> Solve this by registering automatically spidev devices for all the unused chip
> selects when a master registers itself against the spi core.

So, aside from the concern about this being generic the other thing here
is that we often have devices offering more chip selects than can
physically be used in a system but not doing anything to ensure that the
invalid ones can't be used.  It's unclear to me if that's OK or not, it
might be since it's root only I think but I need to think it through a
bit.

> This also adds an i2cdev-like feeling, where you get all the spidev devices all
> the time, without any modification.

I2C is a bit safer here here since it's a shared bus so you can't do
anything to devices not connected to the bus by mistake.

> +		/*
> +		 * This is far from perfect since an addition might be
> +		 * done between here and the call to spi_add_device,
> +		 * but we can't hold the lock and call spi_add_device
> +		 * either, as it would trigger a deadlock.
> +		 *
> +		 * If such a race occurs, spi_add_device will still
> +		 * catch it though, as it also checks for devices
> +		 * being registered several times on the same chip
> +		 * select.
> +		*/
> +		status = bus_for_each_dev(&spi_bus_type, NULL, spi,
> +					  spi_dev_check);
> +		if (status) {
> +			dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
> +			spi_dev_put(spi);
> +			continue;
> +		}

This still leaves us in the situation where if we do know the device
that is connected we have to explicitly bind it in spidev which is
apparently unreasonably difficult for people.  I'm also concerned about
the interactions with DT overlays here - what happens if a DT overlay
or other dynamic hardware instantiation comes along later and does bind
something to this chip select?  It seems like we should be able to
combine the two models, and the fact that we only create these devices
with a Kconfig option is a bit of an interesting thing here.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:

> While this is nicer than the DT solution because of its accurate hardware
> representation, it's still not perfect because you might not have access to the
> DT, or you might be driving a completely generic device (such as a
> microcontroller) that might be used for something else in a different
> context/board.

Greg, you're copied on this because this seems to be a generic problem
that should perhaps be solved at a driver model level - having a way to
bind userspace access to devices that we don't otherwise have a driver
for.  The subsystem could specify the UIO driver to use when no other
driver is available.

> Solve this by registering automatically spidev devices for all the unused chip
> selects when a master registers itself against the spi core.

So, aside from the concern about this being generic the other thing here
is that we often have devices offering more chip selects than can
physically be used in a system but not doing anything to ensure that the
invalid ones can't be used.  It's unclear to me if that's OK or not, it
might be since it's root only I think but I need to think it through a
bit.

> This also adds an i2cdev-like feeling, where you get all the spidev devices all
> the time, without any modification.

I2C is a bit safer here here since it's a shared bus so you can't do
anything to devices not connected to the bus by mistake.

> +		/*
> +		 * This is far from perfect since an addition might be
> +		 * done between here and the call to spi_add_device,
> +		 * but we can't hold the lock and call spi_add_device
> +		 * either, as it would trigger a deadlock.
> +		 *
> +		 * If such a race occurs, spi_add_device will still
> +		 * catch it though, as it also checks for devices
> +		 * being registered several times on the same chip
> +		 * select.
> +		*/
> +		status = bus_for_each_dev(&spi_bus_type, NULL, spi,
> +					  spi_dev_check);
> +		if (status) {
> +			dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
> +			spi_dev_put(spi);
> +			continue;
> +		}

This still leaves us in the situation where if we do know the device
that is connected we have to explicitly bind it in spidev which is
apparently unreasonably difficult for people.  I'm also concerned about
the interactions with DT overlays here - what happens if a DT overlay
or other dynamic hardware instantiation comes along later and does bind
something to this chip select?  It seems like we should be able to
combine the two models, and the fact that we only create these devices
with a Kconfig option is a bit of an interesting thing here.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Michal Suchanek]

On 13 May 2015 at 13:26, Mark Brown <broonie@kernel.org> wrote:
> On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
>
>> While this is nicer than the DT solution because of its accurate hardware
>> representation, it's still not perfect because you might not have access to the
>> DT, or you might be driving a completely generic device (such as a
>> microcontroller) that might be used for something else in a different
>> context/board.
>
> Greg, you're copied on this because this seems to be a generic problem
> that should perhaps be solved at a driver model level - having a way to
> bind userspace access to devices that we don't otherwise have a driver
> for.  The subsystem could specify the UIO driver to use when no other
> driver is available.
>
>> Solve this by registering automatically spidev devices for all the unused chip
>> selects when a master registers itself against the spi core.
>
> So, aside from the concern about this being generic the other thing here
> is that we often have devices offering more chip selects than can
> physically be used in a system but not doing anything to ensure that the
> invalid ones can't be used.  It's unclear to me if that's OK or not, it
> might be since it's root only I think but I need to think it through a
> bit.

Presumably you could add dt nodes for the chipselects and specify they
are disabled but it does not work for me currently.

This is mostly a cosmetic issue. The chipselects were always there but
now they are visible. Unlike the case when you explicitly bind spidev
using dt node the unusable chipselects are also bound.

>
>> This also adds an i2cdev-like feeling, where you get all the spidev devices all
>> the time, without any modification.
>
> I2C is a bit safer here here since it's a shared bus so you can't do
> anything to devices not connected to the bus by mistake.

This is quite safe too. Unless the device chipselect is activated the
device should ignore whatever you write on the bus - unless you get
the chipselect polarity wrong.

But you can get i2c address and lots of other things wrong too.

>
>> +             /*
>> +              * This is far from perfect since an addition might be
>> +              * done between here and the call to spi_add_device,
>> +              * but we can't hold the lock and call spi_add_device
>> +              * either, as it would trigger a deadlock.
>> +              *
>> +              * If such a race occurs, spi_add_device will still
>> +              * catch it though, as it also checks for devices
>> +              * being registered several times on the same chip
>> +              * select.
>> +             */
>> +             status = bus_for_each_dev(&spi_bus_type, NULL, spi,
>> +                                       spi_dev_check);
>> +             if (status) {
>> +                     dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
>> +                     spi_dev_put(spi);
>> +                     continue;
>> +             }
>
> This still leaves us in the situation where if we do know the device
> that is connected we have to explicitly bind it in spidev which is
> apparently unreasonably difficult for people.  I'm also concerned about
> the interactions with DT overlays here - what happens if a DT overlay
> or other dynamic hardware instantiation comes along later and does bind
> something to this chip select?  It seems like we should be able to
> combine the two models, and the fact that we only create these devices
> with a Kconfig option is a bit of an interesting thing here.

It does not bind anything because the chiselect is busy. That's why I
use a patch which disregards spidev when determining if a chiselect is
busy. That has the problem that you can access devices in use by
kernel using spidev then. Ideally spidev (as checked by module alias
or whatever) would be unbound when other driver requests the
chipselect and rebound when the driver quits.

Thanks

Michal

Re: [PATCH] spi: Force the registration of the spidev devices

[Michal Suchanek]

On 13 May 2015 at 13:26, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
>
>> While this is nicer than the DT solution because of its accurate hardware
>> representation, it's still not perfect because you might not have access to the
>> DT, or you might be driving a completely generic device (such as a
>> microcontroller) that might be used for something else in a different
>> context/board.
>
> Greg, you're copied on this because this seems to be a generic problem
> that should perhaps be solved at a driver model level - having a way to
> bind userspace access to devices that we don't otherwise have a driver
> for.  The subsystem could specify the UIO driver to use when no other
> driver is available.
>
>> Solve this by registering automatically spidev devices for all the unused chip
>> selects when a master registers itself against the spi core.
>
> So, aside from the concern about this being generic the other thing here
> is that we often have devices offering more chip selects than can
> physically be used in a system but not doing anything to ensure that the
> invalid ones can't be used.  It's unclear to me if that's OK or not, it
> might be since it's root only I think but I need to think it through a
> bit.

Presumably you could add dt nodes for the chipselects and specify they
are disabled but it does not work for me currently.

This is mostly a cosmetic issue. The chipselects were always there but
now they are visible. Unlike the case when you explicitly bind spidev
using dt node the unusable chipselects are also bound.

>
>> This also adds an i2cdev-like feeling, where you get all the spidev devices all
>> the time, without any modification.
>
> I2C is a bit safer here here since it's a shared bus so you can't do
> anything to devices not connected to the bus by mistake.

This is quite safe too. Unless the device chipselect is activated the
device should ignore whatever you write on the bus - unless you get
the chipselect polarity wrong.

But you can get i2c address and lots of other things wrong too.

>
>> +             /*
>> +              * This is far from perfect since an addition might be
>> +              * done between here and the call to spi_add_device,
>> +              * but we can't hold the lock and call spi_add_device
>> +              * either, as it would trigger a deadlock.
>> +              *
>> +              * If such a race occurs, spi_add_device will still
>> +              * catch it though, as it also checks for devices
>> +              * being registered several times on the same chip
>> +              * select.
>> +             */
>> +             status = bus_for_each_dev(&spi_bus_type, NULL, spi,
>> +                                       spi_dev_check);
>> +             if (status) {
>> +                     dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
>> +                     spi_dev_put(spi);
>> +                     continue;
>> +             }
>
> This still leaves us in the situation where if we do know the device
> that is connected we have to explicitly bind it in spidev which is
> apparently unreasonably difficult for people.  I'm also concerned about
> the interactions with DT overlays here - what happens if a DT overlay
> or other dynamic hardware instantiation comes along later and does bind
> something to this chip select?  It seems like we should be able to
> combine the two models, and the fact that we only create these devices
> with a Kconfig option is a bit of an interesting thing here.

It does not bind anything because the chiselect is busy. That's why I
use a patch which disregards spidev when determining if a chiselect is
busy. That has the problem that you can access devices in use by
kernel using spidev then. Ideally spidev (as checked by module alias
or whatever) would be unbound when other driver requests the
chipselect and rebound when the driver quits.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > Solve this by registering automatically spidev devices for all the unused chip
> > selects when a master registers itself against the spi core.
> 
> So, aside from the concern about this being generic the other thing here
> is that we often have devices offering more chip selects than can
> physically be used in a system but not doing anything to ensure that the
> invalid ones can't be used.  It's unclear to me if that's OK or not, it
> might be since it's root only I think but I need to think it through a
> bit.

I might plead guilty here... :)

I'd say we're also ok because if we delegate the device driving logic
to userspace, we should expect it to know what it does to first drive
the device properly, but also to open the right device for this.

What's the worst that could happen in such a case? The data are output
without any chipselect line being driven by the controller? Isn't that
supposed to be ignored by the devices?

> > This also adds an i2cdev-like feeling, where you get all the
> > spidev devices all the time, without any modification.
> 
> I2C is a bit safer here since it's a shared bus so you can't do
> anything to devices not connected to the bus by mistake.

I'm not sure to understand what you mean here. How is SPI different
from that aspect?

> > +		/*
> > +		 * This is far from perfect since an addition might be
> > +		 * done between here and the call to spi_add_device,
> > +		 * but we can't hold the lock and call spi_add_device
> > +		 * either, as it would trigger a deadlock.
> > +		 *
> > +		 * If such a race occurs, spi_add_device will still
> > +		 * catch it though, as it also checks for devices
> > +		 * being registered several times on the same chip
> > +		 * select.
> > +		*/
> > +		status = bus_for_each_dev(&spi_bus_type, NULL, spi,
> > +					  spi_dev_check);
> > +		if (status) {
> > +			dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
> > +			spi_dev_put(spi);
> > +			continue;
> > +		}
> 
> This still leaves us in the situation where if we do know the device
> that is connected we have to explicitly bind it in spidev which is
> apparently unreasonably difficult for people.

You can still do that, but the point is that you don't have to.

If you know what device you have, and want to use spidev on that
device, it will still work, since it will create an spidev device when
we will parse the DT.

That device will then be skipped by that logic, which is ok.

However, if you don't specify anything in your DT, and have no device
created because you don't really know what's on the other end, this
logic will create the spidev device so that you'll still get an spidev
node exported to the userspace.

> I'm also concerned about the interactions with DT overlays here -
> what happens if a DT overlay or other dynamic hardware instantiation
> comes along later and does bind something to this chip select?  It
> seems like we should be able to combine the two models, and the fact
> that we only create these devices with a Kconfig option is a bit of
> an interesting thing here.

I think the safe approach would be, just like I told in this thread,
to just check whether the modalias is spidev. If it is, destroy the
previous (spidev) device, create a new device as specified by the DT,
you're done.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > Solve this by registering automatically spidev devices for all the unused chip
> > selects when a master registers itself against the spi core.
> 
> So, aside from the concern about this being generic the other thing here
> is that we often have devices offering more chip selects than can
> physically be used in a system but not doing anything to ensure that the
> invalid ones can't be used.  It's unclear to me if that's OK or not, it
> might be since it's root only I think but I need to think it through a
> bit.

I might plead guilty here... :)

I'd say we're also ok because if we delegate the device driving logic
to userspace, we should expect it to know what it does to first drive
the device properly, but also to open the right device for this.

What's the worst that could happen in such a case? The data are output
without any chipselect line being driven by the controller? Isn't that
supposed to be ignored by the devices?

> > This also adds an i2cdev-like feeling, where you get all the
> > spidev devices all the time, without any modification.
> 
> I2C is a bit safer here since it's a shared bus so you can't do
> anything to devices not connected to the bus by mistake.

I'm not sure to understand what you mean here. How is SPI different
from that aspect?

> > +		/*
> > +		 * This is far from perfect since an addition might be
> > +		 * done between here and the call to spi_add_device,
> > +		 * but we can't hold the lock and call spi_add_device
> > +		 * either, as it would trigger a deadlock.
> > +		 *
> > +		 * If such a race occurs, spi_add_device will still
> > +		 * catch it though, as it also checks for devices
> > +		 * being registered several times on the same chip
> > +		 * select.
> > +		*/
> > +		status = bus_for_each_dev(&spi_bus_type, NULL, spi,
> > +					  spi_dev_check);
> > +		if (status) {
> > +			dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
> > +			spi_dev_put(spi);
> > +			continue;
> > +		}
> 
> This still leaves us in the situation where if we do know the device
> that is connected we have to explicitly bind it in spidev which is
> apparently unreasonably difficult for people.

You can still do that, but the point is that you don't have to.

If you know what device you have, and want to use spidev on that
device, it will still work, since it will create an spidev device when
we will parse the DT.

That device will then be skipped by that logic, which is ok.

However, if you don't specify anything in your DT, and have no device
created because you don't really know what's on the other end, this
logic will create the spidev device so that you'll still get an spidev
node exported to the userspace.

> I'm also concerned about the interactions with DT overlays here -
> what happens if a DT overlay or other dynamic hardware instantiation
> comes along later and does bind something to this chip select?  It
> seems like we should be able to combine the two models, and the fact
> that we only create these devices with a Kconfig option is a bit of
> an interesting thing here.

I think the safe approach would be, just like I told in this thread,
to just check whether the modalias is spidev. If it is, destroy the
previous (spidev) device, create a new device as specified by the DT,
you're done.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 02:51:02PM +0200, Maxime Ripard wrote:

> I'd say we're also ok because if we delegate the device driving logic
> to userspace, we should expect it to know what it does to first drive
> the device properly, but also to open the right device for this.

> What's the worst that could happen in such a case? The data are output
> without any chipselect line being driven by the controller? Isn't that
> supposed to be ignored by the devices?

I'm more worried about the chip select line being connected to the
"make the board catch fire" signal or whatever (more realistically
causing us to drive against some other external component) if the extra
chip selects weren't pinmuxed away.

> > > This also adds an i2cdev-like feeling, where you get all the
> > > spidev devices all the time, without any modification.

> > I2C is a bit safer here since it's a shared bus so you can't do
> > anything to devices not connected to the bus by mistake.

> I'm not sure to understand what you mean here. How is SPI different
> from that aspect?

Chip select signals.

> > This still leaves us in the situation where if we do know the device
> > that is connected we have to explicitly bind it in spidev which is
> > apparently unreasonably difficult for people.

> You can still do that, but the point is that you don't have to.

Right, but that's not what I'd expect to happen (and seems to make it
easier for people to not list things in the DT at all which doesn't seem
great).  If we're going to make it available by default I'd expect to be
able to use a userspace driver with anything that doesn't have a driver
bound rather than with devices that explicitly don't have any
identification.

> > I'm also concerned about the interactions with DT overlays here -
> > what happens if a DT overlay or other dynamic hardware instantiation
> > comes along later and does bind something to this chip select?  It
> > seems like we should be able to combine the two models, and the fact
> > that we only create these devices with a Kconfig option is a bit of
> > an interesting thing here.

> I think the safe approach would be, just like I told in this thread,
> to just check whether the modalias is spidev. If it is, destroy the
> previous (spidev) device, create a new device as specified by the DT,
> you're done.

Sure, but I don't see code for that here.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 02:51:02PM +0200, Maxime Ripard wrote:

> I'd say we're also ok because if we delegate the device driving logic
> to userspace, we should expect it to know what it does to first drive
> the device properly, but also to open the right device for this.

> What's the worst that could happen in such a case? The data are output
> without any chipselect line being driven by the controller? Isn't that
> supposed to be ignored by the devices?

I'm more worried about the chip select line being connected to the
"make the board catch fire" signal or whatever (more realistically
causing us to drive against some other external component) if the extra
chip selects weren't pinmuxed away.

> > > This also adds an i2cdev-like feeling, where you get all the
> > > spidev devices all the time, without any modification.

> > I2C is a bit safer here since it's a shared bus so you can't do
> > anything to devices not connected to the bus by mistake.

> I'm not sure to understand what you mean here. How is SPI different
> from that aspect?

Chip select signals.

> > This still leaves us in the situation where if we do know the device
> > that is connected we have to explicitly bind it in spidev which is
> > apparently unreasonably difficult for people.

> You can still do that, but the point is that you don't have to.

Right, but that's not what I'd expect to happen (and seems to make it
easier for people to not list things in the DT at all which doesn't seem
great).  If we're going to make it available by default I'd expect to be
able to use a userspace driver with anything that doesn't have a driver
bound rather than with devices that explicitly don't have any
identification.

> > I'm also concerned about the interactions with DT overlays here -
> > what happens if a DT overlay or other dynamic hardware instantiation
> > comes along later and does bind something to this chip select?  It
> > seems like we should be able to combine the two models, and the fact
> > that we only create these devices with a Kconfig option is a bit of
> > an interesting thing here.

> I think the safe approach would be, just like I told in this thread,
> to just check whether the modalias is spidev. If it is, destroy the
> previous (spidev) device, create a new device as specified by the DT,
> you're done.

Sure, but I don't see code for that here.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Michal Suchanek]

On 13 May 2015 at 16:36, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 13, 2015 at 02:51:02PM +0200, Maxime Ripard wrote:
>
>> I'd say we're also ok because if we delegate the device driving logic
>> to userspace, we should expect it to know what it does to first drive
>> the device properly, but also to open the right device for this.
>
>> What's the worst that could happen in such a case? The data are output
>> without any chipselect line being driven by the controller? Isn't that
>> supposed to be ignored by the devices?
>
> I'm more worried about the chip select line being connected to the
> "make the board catch fire" signal or whatever (more realistically
> causing us to drive against some other external component) if the extra
> chip selects weren't pinmuxed away.

They would not be pinmuxed away if

1) they are unused by anything else and cs happens to be the default
2) they are used by a device not in DT

right?

For the latter case we might want some way to disable unused
chipselects as well as for the cosmetic issue of multitude of useless
devices popping up.

But you know, unused i2c bus can be also connected to "make the board
catch fire" trace and nobody would notice until somebody has the great
idea to probe it. Incidentally, both i2c and spi cs are active-low
iirc.

>> > This still leaves us in the situation where if we do know the device
>> > that is connected we have to explicitly bind it in spidev which is
>> > apparently unreasonably difficult for people.
>
>> You can still do that, but the point is that you don't have to.
>
> Right, but that's not what I'd expect to happen (and seems to make it
> easier for people to not list things in the DT at all which doesn't seem
> great).  If we're going to make it available by default I'd expect to be
> able to use a userspace driver with anything that doesn't have a driver
> bound rather than with devices that explicitly don't have any
> identification.

Remember that spidev can and is used for boards which have spi
*CONNECTOR* to which you can connect some random gadget and use a
userspace program to communicate with it.

It is cool that you can load a dt overlay if the gadget happens to
have a kernel driver.

However, what identification do you write in the dt when there is no
need to use a kernel driver at all?

The option to create a node with 'spidev' compatible was rejected as broken.

The option to add new compatible to the spidev driver every time you
want to connect a new device was rejected as absurdly complex for end
users who have a board with system already preinstalled because it
requires adding the compatible in the spidev source and rebuilding the
kernel.

So this patch implements another option which is to bind spidev
*always*. The configuration of the port is then not recorded in the DT
and it's up to the user to visually check the port or apply other
relevant heuristic and run the correct userspace application.

Thanks

Michal

Re: [PATCH] spi: Force the registration of the spidev devices

[Michal Suchanek]

On 13 May 2015 at 16:36, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, May 13, 2015 at 02:51:02PM +0200, Maxime Ripard wrote:
>
>> I'd say we're also ok because if we delegate the device driving logic
>> to userspace, we should expect it to know what it does to first drive
>> the device properly, but also to open the right device for this.
>
>> What's the worst that could happen in such a case? The data are output
>> without any chipselect line being driven by the controller? Isn't that
>> supposed to be ignored by the devices?
>
> I'm more worried about the chip select line being connected to the
> "make the board catch fire" signal or whatever (more realistically
> causing us to drive against some other external component) if the extra
> chip selects weren't pinmuxed away.

They would not be pinmuxed away if

1) they are unused by anything else and cs happens to be the default
2) they are used by a device not in DT

right?

For the latter case we might want some way to disable unused
chipselects as well as for the cosmetic issue of multitude of useless
devices popping up.

But you know, unused i2c bus can be also connected to "make the board
catch fire" trace and nobody would notice until somebody has the great
idea to probe it. Incidentally, both i2c and spi cs are active-low
iirc.

>> > This still leaves us in the situation where if we do know the device
>> > that is connected we have to explicitly bind it in spidev which is
>> > apparently unreasonably difficult for people.
>
>> You can still do that, but the point is that you don't have to.
>
> Right, but that's not what I'd expect to happen (and seems to make it
> easier for people to not list things in the DT at all which doesn't seem
> great).  If we're going to make it available by default I'd expect to be
> able to use a userspace driver with anything that doesn't have a driver
> bound rather than with devices that explicitly don't have any
> identification.

Remember that spidev can and is used for boards which have spi
*CONNECTOR* to which you can connect some random gadget and use a
userspace program to communicate with it.

It is cool that you can load a dt overlay if the gadget happens to
have a kernel driver.

However, what identification do you write in the dt when there is no
need to use a kernel driver at all?

The option to create a node with 'spidev' compatible was rejected as broken.

The option to add new compatible to the spidev driver every time you
want to connect a new device was rejected as absurdly complex for end
users who have a board with system already preinstalled because it
requires adding the compatible in the spidev source and rebuilding the
kernel.

So this patch implements another option which is to bind spidev
*always*. The configuration of the port is then not recorded in the DT
and it's up to the user to visually check the port or apply other
relevant heuristic and run the correct userspace application.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> 
> > While this is nicer than the DT solution because of its accurate hardware
> > representation, it's still not perfect because you might not have access to the
> > DT, or you might be driving a completely generic device (such as a
> > microcontroller) that might be used for something else in a different
> > context/board.
> 
> Greg, you're copied on this because this seems to be a generic problem
> that should perhaps be solved at a driver model level - having a way to
> bind userspace access to devices that we don't otherwise have a driver
> for.  The subsystem could specify the UIO driver to use when no other
> driver is available.

That doesn't really work.  I've been talking to the ACPI people about
this, and the problem is "don't otherwise have a driver for" is an
impossible thing to prove, as you never know when a driver is going to
be loaded from userspace.

You can easily bind drivers to devices today from userspace, why not
just use the built-in functionality you have today if you "know" that
there is no driver for this hardware.

thanks,

greg k-h

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> 
> > While this is nicer than the DT solution because of its accurate hardware
> > representation, it's still not perfect because you might not have access to the
> > DT, or you might be driving a completely generic device (such as a
> > microcontroller) that might be used for something else in a different
> > context/board.
> 
> Greg, you're copied on this because this seems to be a generic problem
> that should perhaps be solved at a driver model level - having a way to
> bind userspace access to devices that we don't otherwise have a driver
> for.  The subsystem could specify the UIO driver to use when no other
> driver is available.

That doesn't really work.  I've been talking to the ACPI people about
this, and the problem is "don't otherwise have a driver for" is an
impossible thing to prove, as you never know when a driver is going to
be loaded from userspace.

You can easily bind drivers to devices today from userspace, why not
just use the built-in functionality you have today if you "know" that
there is no driver for this hardware.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Michal Suchanek]

On 13 May 2015 at 17:37, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
>> On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
>>
>> > While this is nicer than the DT solution because of its accurate hardware
>> > representation, it's still not perfect because you might not have access to the
>> > DT, or you might be driving a completely generic device (such as a
>> > microcontroller) that might be used for something else in a different
>> > context/board.
>>
>> Greg, you're copied on this because this seems to be a generic problem
>> that should perhaps be solved at a driver model level - having a way to
>> bind userspace access to devices that we don't otherwise have a driver
>> for.  The subsystem could specify the UIO driver to use when no other
>> driver is available.
>
> That doesn't really work.  I've been talking to the ACPI people about
> this, and the problem is "don't otherwise have a driver for" is an
> impossible thing to prove, as you never know when a driver is going to
> be loaded from userspace.

Yes, exactly. That's why binding spidev in addition to regular drivers
or have spidev bind always and get unbound when another driver tries
to bind the CS is preferable. The latter is pretty much specifying an
UIO driver to use when no other driver is available in the light of
the fact that 'is available' might change over time. You can prove
that a driver is not available right now.

>
> You can easily bind drivers to devices today from userspace, why not
> just use the built-in functionality you have today if you "know" that
> there is no driver for this hardware.
>

And what exactly do you bind the driver to when there is no device
created with the built-in functionality you have today?

Thanks

Michal

Re: [PATCH] spi: Force the registration of the spidev devices

[Michal Suchanek]

On 13 May 2015 at 17:37, Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
>> On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
>>
>> > While this is nicer than the DT solution because of its accurate hardware
>> > representation, it's still not perfect because you might not have access to the
>> > DT, or you might be driving a completely generic device (such as a
>> > microcontroller) that might be used for something else in a different
>> > context/board.
>>
>> Greg, you're copied on this because this seems to be a generic problem
>> that should perhaps be solved at a driver model level - having a way to
>> bind userspace access to devices that we don't otherwise have a driver
>> for.  The subsystem could specify the UIO driver to use when no other
>> driver is available.
>
> That doesn't really work.  I've been talking to the ACPI people about
> this, and the problem is "don't otherwise have a driver for" is an
> impossible thing to prove, as you never know when a driver is going to
> be loaded from userspace.

Yes, exactly. That's why binding spidev in addition to regular drivers
or have spidev bind always and get unbound when another driver tries
to bind the CS is preferable. The latter is pretty much specifying an
UIO driver to use when no other driver is available in the light of
the fact that 'is available' might change over time. You can prove
that a driver is not available right now.

>
> You can easily bind drivers to devices today from userspace, why not
> just use the built-in functionality you have today if you "know" that
> there is no driver for this hardware.
>

And what exactly do you bind the driver to when there is no device
created with the built-in functionality you have today?

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:

> You can easily bind drivers to devices today from userspace, why not
> just use the built-in functionality you have today if you "know" that
> there is no driver for this hardware.

So, that was my original suggestion too but people were complaining that
this wasn't a generally supported feature and requires specific support
of some kind in the bus (which nobody posted a patch for).  I have to
confess I didn't investigate too deeply myself.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:

> You can easily bind drivers to devices today from userspace, why not
> just use the built-in functionality you have today if you "know" that
> there is no driver for this hardware.

So, that was my original suggestion too but people were complaining that
this wasn't a generally supported feature and requires specific support
of some kind in the bus (which nobody posted a patch for).  I have to
confess I didn't investigate too deeply myself.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 06:13:00PM +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> 
> > You can easily bind drivers to devices today from userspace, why not
> > just use the built-in functionality you have today if you "know" that
> > there is no driver for this hardware.
> 
> So, that was my original suggestion too but people were complaining that
> this wasn't a generally supported feature and requires specific support
> of some kind in the bus (which nobody posted a patch for).

I don't understand the complaint, it's a very "generally supported
feature" and has been there for a very long time for any bus that wants
to use it.

greg k-h

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 06:13:00PM +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> 
> > You can easily bind drivers to devices today from userspace, why not
> > just use the built-in functionality you have today if you "know" that
> > there is no driver for this hardware.
> 
> So, that was my original suggestion too but people were complaining that
> this wasn't a generally supported feature and requires specific support
> of some kind in the bus (which nobody posted a patch for).

I don't understand the complaint, it's a very "generally supported
feature" and has been there for a very long time for any bus that wants
to use it.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 10:20:28AM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 06:13:00PM +0100, Mark Brown wrote:

> > So, that was my original suggestion too but people were complaining that
> > this wasn't a generally supported feature and requires specific support
> > of some kind in the bus (which nobody posted a patch for).

> I don't understand the complaint, it's a very "generally supported
> feature" and has been there for a very long time for any bus that wants
> to use it.

So it's not something that just works for all buses and someone would
need to write a patch (hint, hint - that's one of the big blockers
here)?  It is a bit surprising to me that this would be something that
the buses would need to cope with individually since for most things the
matching mechanics are taken care of in the driver core and we're
explicitly overriding them here.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 10:20:28AM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 06:13:00PM +0100, Mark Brown wrote:

> > So, that was my original suggestion too but people were complaining that
> > this wasn't a generally supported feature and requires specific support
> > of some kind in the bus (which nobody posted a patch for).

> I don't understand the complaint, it's a very "generally supported
> feature" and has been there for a very long time for any bus that wants
> to use it.

So it's not something that just works for all buses and someone would
need to write a patch (hint, hint - that's one of the big blockers
here)?  It is a bit surprising to me that this would be something that
the buses would need to cope with individually since for most things the
matching mechanics are taken care of in the driver core and we're
explicitly overriding them here.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 05:31:05PM +0200, Michal Suchanek wrote:

> But you know, unused i2c bus can be also connected to "make the board
> catch fire" trace and nobody would notice until somebody has the great
> idea to probe it. Incidentally, both i2c and spi cs are active-low
> iirc.

Someone would need to go and explicitly mark an I2C controller as
enabled on a board for that to happen.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 05:31:05PM +0200, Michal Suchanek wrote:

> But you know, unused i2c bus can be also connected to "make the board
> catch fire" trace and nobody would notice until somebody has the great
> idea to probe it. Incidentally, both i2c and spi cs are active-low
> iirc.

Someone would need to go and explicitly mark an I2C controller as
enabled on a board for that to happen.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

Hi Greg,

On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> > 
> > > While this is nicer than the DT solution because of its accurate hardware
> > > representation, it's still not perfect because you might not have access to the
> > > DT, or you might be driving a completely generic device (such as a
> > > microcontroller) that might be used for something else in a different
> > > context/board.
> > 
> > Greg, you're copied on this because this seems to be a generic problem
> > that should perhaps be solved at a driver model level - having a way to
> > bind userspace access to devices that we don't otherwise have a driver
> > for.  The subsystem could specify the UIO driver to use when no other
> > driver is available.
> 
> That doesn't really work.  I've been talking to the ACPI people about
> this, and the problem is "don't otherwise have a driver for" is an
> impossible thing to prove, as you never know when a driver is going to
> be loaded from userspace.
> 
> You can easily bind drivers to devices today from userspace, why not
> just use the built-in functionality you have today if you "know" that
> there is no driver for this hardware.

What we're really after here is that we want to have an spidev
instance when we don't even have a device.

And since SPI isn't really doing any kind of hotplug, the only
situation that might be problematic is if we have the DT overlays
registering new devices.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

Hi Greg,

On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> > 
> > > While this is nicer than the DT solution because of its accurate hardware
> > > representation, it's still not perfect because you might not have access to the
> > > DT, or you might be driving a completely generic device (such as a
> > > microcontroller) that might be used for something else in a different
> > > context/board.
> > 
> > Greg, you're copied on this because this seems to be a generic problem
> > that should perhaps be solved at a driver model level - having a way to
> > bind userspace access to devices that we don't otherwise have a driver
> > for.  The subsystem could specify the UIO driver to use when no other
> > driver is available.
> 
> That doesn't really work.  I've been talking to the ACPI people about
> this, and the problem is "don't otherwise have a driver for" is an
> impossible thing to prove, as you never know when a driver is going to
> be loaded from userspace.
> 
> You can easily bind drivers to devices today from userspace, why not
> just use the built-in functionality you have today if you "know" that
> there is no driver for this hardware.

What we're really after here is that we want to have an spidev
instance when we don't even have a device.

And since SPI isn't really doing any kind of hotplug, the only
situation that might be problematic is if we have the DT overlays
registering new devices.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
> On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:

> > That doesn't really work.  I've been talking to the ACPI people about
> > this, and the problem is "don't otherwise have a driver for" is an
> > impossible thing to prove, as you never know when a driver is going to
> > be loaded from userspace.

> > You can easily bind drivers to devices today from userspace, why not
> > just use the built-in functionality you have today if you "know" that
> > there is no driver for this hardware.

> What we're really after here is that we want to have an spidev
> instance when we don't even have a device.

We do?

> And since SPI isn't really doing any kind of hotplug, the only
> situation that might be problematic is if we have the DT overlays
> registering new devices.

Well, we still have the driver loaded from userspace in the future
situation as well - the hardware might be static but the software isn't.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 06:39:22PM +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 10:20:28AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, May 13, 2015 at 06:13:00PM +0100, Mark Brown wrote:
> 
> > > So, that was my original suggestion too but people were complaining that
> > > this wasn't a generally supported feature and requires specific support
> > > of some kind in the bus (which nobody posted a patch for).
> 
> > I don't understand the complaint, it's a very "generally supported
> > feature" and has been there for a very long time for any bus that wants
> > to use it.
> 
> So it's not something that just works for all buses and someone would
> need to write a patch (hint, hint - that's one of the big blockers
> here)?  It is a bit surprising to me that this would be something that
> the buses would need to cope with individually since for most things the
> matching mechanics are taken care of in the driver core and we're
> explicitly overriding them here.

It should "just work" for all busses, but if you want to add a "new_id"
sysfs file, you need to add the logic for that to your bus.  It's the
bind/unbind files in the driver directories.

greg k-h

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 06:39:22PM +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 10:20:28AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, May 13, 2015 at 06:13:00PM +0100, Mark Brown wrote:
> 
> > > So, that was my original suggestion too but people were complaining that
> > > this wasn't a generally supported feature and requires specific support
> > > of some kind in the bus (which nobody posted a patch for).
> 
> > I don't understand the complaint, it's a very "generally supported
> > feature" and has been there for a very long time for any bus that wants
> > to use it.
> 
> So it's not something that just works for all buses and someone would
> need to write a patch (hint, hint - that's one of the big blockers
> here)?  It is a bit surprising to me that this would be something that
> the buses would need to cope with individually since for most things the
> matching mechanics are taken care of in the driver core and we're
> explicitly overriding them here.

It should "just work" for all busses, but if you want to add a "new_id"
sysfs file, you need to add the logic for that to your bus.  It's the
bind/unbind files in the driver directories.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
> Hi Greg,
> 
> On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> > > 
> > > > While this is nicer than the DT solution because of its accurate hardware
> > > > representation, it's still not perfect because you might not have access to the
> > > > DT, or you might be driving a completely generic device (such as a
> > > > microcontroller) that might be used for something else in a different
> > > > context/board.
> > > 
> > > Greg, you're copied on this because this seems to be a generic problem
> > > that should perhaps be solved at a driver model level - having a way to
> > > bind userspace access to devices that we don't otherwise have a driver
> > > for.  The subsystem could specify the UIO driver to use when no other
> > > driver is available.
> > 
> > That doesn't really work.  I've been talking to the ACPI people about
> > this, and the problem is "don't otherwise have a driver for" is an
> > impossible thing to prove, as you never know when a driver is going to
> > be loaded from userspace.
> > 
> > You can easily bind drivers to devices today from userspace, why not
> > just use the built-in functionality you have today if you "know" that
> > there is no driver for this hardware.
> 
> What we're really after here is that we want to have an spidev
> instance when we don't even have a device.

That's crazy, just create a device, things do not work without one.

> And since SPI isn't really doing any kind of hotplug, the only
> situation that might be problematic is if we have the DT overlays
> registering new devices.

I have no idea what this all is, sorry, patches would be good if you
want to get your idea across.  Maybe it was sent early in this thread,
but I can't seem to find one...

greg k-h

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
> Hi Greg,
> 
> On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> > > 
> > > > While this is nicer than the DT solution because of its accurate hardware
> > > > representation, it's still not perfect because you might not have access to the
> > > > DT, or you might be driving a completely generic device (such as a
> > > > microcontroller) that might be used for something else in a different
> > > > context/board.
> > > 
> > > Greg, you're copied on this because this seems to be a generic problem
> > > that should perhaps be solved at a driver model level - having a way to
> > > bind userspace access to devices that we don't otherwise have a driver
> > > for.  The subsystem could specify the UIO driver to use when no other
> > > driver is available.
> > 
> > That doesn't really work.  I've been talking to the ACPI people about
> > this, and the problem is "don't otherwise have a driver for" is an
> > impossible thing to prove, as you never know when a driver is going to
> > be loaded from userspace.
> > 
> > You can easily bind drivers to devices today from userspace, why not
> > just use the built-in functionality you have today if you "know" that
> > there is no driver for this hardware.
> 
> What we're really after here is that we want to have an spidev
> instance when we don't even have a device.

That's crazy, just create a device, things do not work without one.

> And since SPI isn't really doing any kind of hotplug, the only
> situation that might be problematic is if we have the DT overlays
> registering new devices.

I have no idea what this all is, sorry, patches would be good if you
want to get your idea across.  Maybe it was sent early in this thread,
but I can't seem to find one...

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 11:16:31AM -0700, Greg Kroah-Hartman wrote:

> It should "just work" for all busses, but if you want to add a "new_id"
> sysfs file, you need to add the logic for that to your bus.  It's the
> bind/unbind files in the driver directories.

Oh, right.  For this to be useful here we'd need to implement a new_id
file, bind and unbind don't do anything helpful here.  I think I'd have
expected this to have a default implementation that non-enumerable buses
could pick up.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 07:32:11PM +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 11:16:31AM -0700, Greg Kroah-Hartman wrote:
> 
> > It should "just work" for all busses, but if you want to add a "new_id"
> > sysfs file, you need to add the logic for that to your bus.  It's the
> > bind/unbind files in the driver directories.
> 
> Oh, right.  For this to be useful here we'd need to implement a new_id
> file, bind and unbind don't do anything helpful here.  I think I'd have
> expected this to have a default implementation that non-enumerable buses
> could pick up.

No one has ever asked for that, so feel free to make a patch that adds
it :)

thanks,

greg k-h

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 07:32:11PM +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 11:16:31AM -0700, Greg Kroah-Hartman wrote:
> 
> > It should "just work" for all busses, but if you want to add a "new_id"
> > sysfs file, you need to add the logic for that to your bus.  It's the
> > bind/unbind files in the driver directories.
> 
> Oh, right.  For this to be useful here we'd need to implement a new_id
> file, bind and unbind don't do anything helpful here.  I think I'd have
> expected this to have a default implementation that non-enumerable buses
> could pick up.

No one has ever asked for that, so feel free to make a patch that adds
it :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 11:36:53AM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 07:32:11PM +0100, Mark Brown wrote:

> > Oh, right.  For this to be useful here we'd need to implement a new_id
> > file, bind and unbind don't do anything helpful here.  I think I'd have
> > expected this to have a default implementation that non-enumerable buses
> > could pick up.

> No one has ever asked for that, so feel free to make a patch that adds
> it :)

Maxime?  :P

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 11:36:53AM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 07:32:11PM +0100, Mark Brown wrote:

> > Oh, right.  For this to be useful here we'd need to implement a new_id
> > file, bind and unbind don't do anything helpful here.  I think I'd have
> > expected this to have a default implementation that non-enumerable buses
> > could pick up.

> No one has ever asked for that, so feel free to make a patch that adds
> it :)

Maxime?  :P

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 03:36:10PM +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 02:51:02PM +0200, Maxime Ripard wrote:
> 
> > I'd say we're also ok because if we delegate the device driving logic
> > to userspace, we should expect it to know what it does to first drive
> > the device properly, but also to open the right device for this.
> 
> > What's the worst that could happen in such a case? The data are output
> > without any chipselect line being driven by the controller? Isn't that
> > supposed to be ignored by the devices?
> 
> I'm more worried about the chip select line being connected to the
> "make the board catch fire" signal or whatever (more realistically
> causing us to drive against some other external component) if the extra
> chip selects weren't pinmuxed away.

It seems we've had this discussion at lot lately ;)

That indeed might be problematic....

> > > > This also adds an i2cdev-like feeling, where you get all the
> > > > spidev devices all the time, without any modification.
> 
> > > I2C is a bit safer here since it's a shared bus so you can't do
> > > anything to devices not connected to the bus by mistake.
> 
> > I'm not sure to understand what you mean here. How is SPI different
> > from that aspect?
> 
> Chip select signals.

Well, if it's not connected to the bus, it probably won't be connected
to the chip select either, will it?

> > > This still leaves us in the situation where if we do know the device
> > > that is connected we have to explicitly bind it in spidev which is
> > > apparently unreasonably difficult for people.
> 
> > You can still do that, but the point is that you don't have to.
> 
> Right, but that's not what I'd expect to happen (and seems to make it
> easier for people to not list things in the DT at all which doesn't seem
> great).  If we're going to make it available by default I'd expect to be
> able to use a userspace driver with anything that doesn't have a driver
> bound rather than with devices that explicitly don't have any
> identification.

The point is that if we don't have anything declared in the DT, we
won't even have a device. So we can't really expect that the device
will not be bound to a driver, because it won't even be there in the
first place.

> > > I'm also concerned about the interactions with DT overlays here -
> > > what happens if a DT overlay or other dynamic hardware instantiation
> > > comes along later and does bind something to this chip select?  It
> > > seems like we should be able to combine the two models, and the fact
> > > that we only create these devices with a Kconfig option is a bit of
> > > an interesting thing here.
> 
> > I think the safe approach would be, just like I told in this thread,
> > to just check whether the modalias is spidev. If it is, destroy the
> > previous (spidev) device, create a new device as specified by the DT,
> > you're done.
> 
> Sure, but I don't see code for that here.

No, of course. Remember that this code was written before the overlays
were posted.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Geert Uytterhoeven]

On Wed, May 13, 2015 at 2:51 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
>> > This also adds an i2cdev-like feeling, where you get all the
>> > spidev devices all the time, without any modification.
>>
>> I2C is a bit safer here since it's a shared bus so you can't do
>> anything to devices not connected to the bus by mistake.
>
> I'm not sure to understand what you mean here. How is SPI different
> from that aspect?

If you talk to a nonexistent i2c device, nothing happens, as it just sends
a message with a nonexistent address on the shared bus.

If you talk to a nonexistent spi device, hell may break loose if e.g. some
"smart" hardware engineer used the "unused" CS as a pull-up for the
_RESET line on an external device... It's a bit like banging random
"unused" GPIOs.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: Force the registration of the spidev devices

[Geert Uytterhoeven]

On Wed, May 13, 2015 at 2:51 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > This also adds an i2cdev-like feeling, where you get all the
>> > spidev devices all the time, without any modification.
>>
>> I2C is a bit safer here since it's a shared bus so you can't do
>> anything to devices not connected to the bus by mistake.
>
> I'm not sure to understand what you mean here. How is SPI different
> from that aspect?

If you talk to a nonexistent i2c device, nothing happens, as it just sends
a message with a nonexistent address on the shared bus.

If you talk to a nonexistent spi device, hell may break loose if e.g. some
"smart" hardware engineer used the "unused" CS as a pull-up for the
_RESET line on an external device... It's a bit like banging random
"unused" GPIOs.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 07:51:49PM +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 11:36:53AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, May 13, 2015 at 07:32:11PM +0100, Mark Brown wrote:
> 
> > > Oh, right.  For this to be useful here we'd need to implement a new_id
> > > file, bind and unbind don't do anything helpful here.  I think I'd have
> > > expected this to have a default implementation that non-enumerable buses
> > > could pick up.
> 
> > No one has ever asked for that, so feel free to make a patch that adds
> > it :)
> 
> Maxime?  :P

I can try to give it a shot.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 07:51:49PM +0100, Mark Brown wrote:
> On Wed, May 13, 2015 at 11:36:53AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, May 13, 2015 at 07:32:11PM +0100, Mark Brown wrote:
> 
> > > Oh, right.  For this to be useful here we'd need to implement a new_id
> > > file, bind and unbind don't do anything helpful here.  I think I'd have
> > > expected this to have a default implementation that non-enumerable buses
> > > could pick up.
> 
> > No one has ever asked for that, so feel free to make a patch that adds
> > it :)
> 
> Maxime?  :P

I can try to give it a shot.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Geert Uytterhoeven]

On Wed, May 13, 2015 at 8:17 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
>> On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
>> > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
>> > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
>> > >
>> > > > While this is nicer than the DT solution because of its accurate hardware
>> > > > representation, it's still not perfect because you might not have access to the
>> > > > DT, or you might be driving a completely generic device (such as a
>> > > > microcontroller) that might be used for something else in a different
>> > > > context/board.
>> > >
>> > > Greg, you're copied on this because this seems to be a generic problem
>> > > that should perhaps be solved at a driver model level - having a way to
>> > > bind userspace access to devices that we don't otherwise have a driver
>> > > for.  The subsystem could specify the UIO driver to use when no other
>> > > driver is available.
>> >
>> > That doesn't really work.  I've been talking to the ACPI people about
>> > this, and the problem is "don't otherwise have a driver for" is an
>> > impossible thing to prove, as you never know when a driver is going to
>> > be loaded from userspace.
>> >
>> > You can easily bind drivers to devices today from userspace, why not
>> > just use the built-in functionality you have today if you "know" that
>> > there is no driver for this hardware.
>>
>> What we're really after here is that we want to have an spidev
>> instance when we don't even have a device.
>
> That's crazy, just create a device, things do not work without one.

One simple way that works today is to create a device tree overlay with a
device that's compatible with "spidev". That does violate DT policy, as it
doesn't describe the hardware. But it works. This can be done from userspace,
so we (the naggers about incorrect description of the hardware) will never
see the abusive dtso.

Alternatively, you can write some kernel code that does more or less the same
thing, i.e. create a device for "spidev", but without any DT involved.
If you add remove support, and make this controllable through sysfs, I think
this could be a viable solution. It would be similar to gpio export.
A DT overlay with the real device can still take over, if you instruct the
removal of the "spidev" device through sysfs first.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: Force the registration of the spidev devices

[Geert Uytterhoeven]

On Wed, May 13, 2015 at 8:17 PM, Greg Kroah-Hartman
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
>> On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
>> > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
>> > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
>> > >
>> > > > While this is nicer than the DT solution because of its accurate hardware
>> > > > representation, it's still not perfect because you might not have access to the
>> > > > DT, or you might be driving a completely generic device (such as a
>> > > > microcontroller) that might be used for something else in a different
>> > > > context/board.
>> > >
>> > > Greg, you're copied on this because this seems to be a generic problem
>> > > that should perhaps be solved at a driver model level - having a way to
>> > > bind userspace access to devices that we don't otherwise have a driver
>> > > for.  The subsystem could specify the UIO driver to use when no other
>> > > driver is available.
>> >
>> > That doesn't really work.  I've been talking to the ACPI people about
>> > this, and the problem is "don't otherwise have a driver for" is an
>> > impossible thing to prove, as you never know when a driver is going to
>> > be loaded from userspace.
>> >
>> > You can easily bind drivers to devices today from userspace, why not
>> > just use the built-in functionality you have today if you "know" that
>> > there is no driver for this hardware.
>>
>> What we're really after here is that we want to have an spidev
>> instance when we don't even have a device.
>
> That's crazy, just create a device, things do not work without one.

One simple way that works today is to create a device tree overlay with a
device that's compatible with "spidev". That does violate DT policy, as it
doesn't describe the hardware. But it works. This can be done from userspace,
so we (the naggers about incorrect description of the hardware) will never
see the abusive dtso.

Alternatively, you can write some kernel code that does more or less the same
thing, i.e. create a device for "spidev", but without any DT involved.
If you add remove support, and make this controllable through sysfs, I think
this could be a viable solution. It would be similar to gpio export.
A DT overlay with the real device can still take over, if you instruct the
removal of the "spidev" device through sysfs first.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
> > Hi Greg,
> > 
> > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> > > > 
> > > > > While this is nicer than the DT solution because of its accurate hardware
> > > > > representation, it's still not perfect because you might not have access to the
> > > > > DT, or you might be driving a completely generic device (such as a
> > > > > microcontroller) that might be used for something else in a different
> > > > > context/board.
> > > > 
> > > > Greg, you're copied on this because this seems to be a generic problem
> > > > that should perhaps be solved at a driver model level - having a way to
> > > > bind userspace access to devices that we don't otherwise have a driver
> > > > for.  The subsystem could specify the UIO driver to use when no other
> > > > driver is available.
> > > 
> > > That doesn't really work.  I've been talking to the ACPI people about
> > > this, and the problem is "don't otherwise have a driver for" is an
> > > impossible thing to prove, as you never know when a driver is going to
> > > be loaded from userspace.
> > > 
> > > You can easily bind drivers to devices today from userspace, why not
> > > just use the built-in functionality you have today if you "know" that
> > > there is no driver for this hardware.
> > 
> > What we're really after here is that we want to have an spidev
> > instance when we don't even have a device.
> 
> That's crazy, just create a device, things do not work without one.

Our use case is this one: we want to export spidev files so that "dev
boards" with a header that allows to plug virtually anything on it
(Raspberry Pi, Cubieboards, Xplained, and all the likes) without
having to change the kernel and / or device tree.

That would mean that if we plug something to that port, no device will
be created because the DT itself won't have that device declared in
the first place.

This patch is actually doing this: creating a new device for all the
chipselects that are not in use that will be bound to the spidev
driver.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
> > Hi Greg,
> > 
> > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> > > > 
> > > > > While this is nicer than the DT solution because of its accurate hardware
> > > > > representation, it's still not perfect because you might not have access to the
> > > > > DT, or you might be driving a completely generic device (such as a
> > > > > microcontroller) that might be used for something else in a different
> > > > > context/board.
> > > > 
> > > > Greg, you're copied on this because this seems to be a generic problem
> > > > that should perhaps be solved at a driver model level - having a way to
> > > > bind userspace access to devices that we don't otherwise have a driver
> > > > for.  The subsystem could specify the UIO driver to use when no other
> > > > driver is available.
> > > 
> > > That doesn't really work.  I've been talking to the ACPI people about
> > > this, and the problem is "don't otherwise have a driver for" is an
> > > impossible thing to prove, as you never know when a driver is going to
> > > be loaded from userspace.
> > > 
> > > You can easily bind drivers to devices today from userspace, why not
> > > just use the built-in functionality you have today if you "know" that
> > > there is no driver for this hardware.
> > 
> > What we're really after here is that we want to have an spidev
> > instance when we don't even have a device.
> 
> That's crazy, just create a device, things do not work without one.

Our use case is this one: we want to export spidev files so that "dev
boards" with a header that allows to plug virtually anything on it
(Raspberry Pi, Cubieboards, Xplained, and all the likes) without
having to change the kernel and / or device tree.

That would mean that if we plug something to that port, no device will
be created because the DT itself won't have that device declared in
the first place.

This patch is actually doing this: creating a new device for all the
chipselects that are not in use that will be bound to the spidev
driver.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 09:10:48PM +0200, Geert Uytterhoeven wrote:
> On Wed, May 13, 2015 at 2:51 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> >> > This also adds an i2cdev-like feeling, where you get all the
> >> > spidev devices all the time, without any modification.
> >>
> >> I2C is a bit safer here since it's a shared bus so you can't do
> >> anything to devices not connected to the bus by mistake.
> >
> > I'm not sure to understand what you mean here. How is SPI different
> > from that aspect?
> 
> If you talk to a nonexistent i2c device, nothing happens, as it just sends
> a message with a nonexistent address on the shared bus.
> 
> If you talk to a nonexistent spi device, hell may break loose if e.g. some
> "smart" hardware engineer used the "unused" CS as a pull-up for the
> _RESET line on an external device... It's a bit like banging random
> "unused" GPIOs.

Ah, right. I'm always surprised by how creative the hardware engineers
actually are :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 09:10:48PM +0200, Geert Uytterhoeven wrote:
> On Wed, May 13, 2015 at 2:51 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> > This also adds an i2cdev-like feeling, where you get all the
> >> > spidev devices all the time, without any modification.
> >>
> >> I2C is a bit safer here since it's a shared bus so you can't do
> >> anything to devices not connected to the bus by mistake.
> >
> > I'm not sure to understand what you mean here. How is SPI different
> > from that aspect?
> 
> If you talk to a nonexistent i2c device, nothing happens, as it just sends
> a message with a nonexistent address on the shared bus.
> 
> If you talk to a nonexistent spi device, hell may break loose if e.g. some
> "smart" hardware engineer used the "unused" CS as a pull-up for the
> _RESET line on an external device... It's a bit like banging random
> "unused" GPIOs.

Ah, right. I'm always surprised by how creative the hardware engineers
actually are :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote:
> On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
> > > Hi Greg,
> > > 
> > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> > > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> > > > > 
> > > > > > While this is nicer than the DT solution because of its accurate hardware
> > > > > > representation, it's still not perfect because you might not have access to the
> > > > > > DT, or you might be driving a completely generic device (such as a
> > > > > > microcontroller) that might be used for something else in a different
> > > > > > context/board.
> > > > > 
> > > > > Greg, you're copied on this because this seems to be a generic problem
> > > > > that should perhaps be solved at a driver model level - having a way to
> > > > > bind userspace access to devices that we don't otherwise have a driver
> > > > > for.  The subsystem could specify the UIO driver to use when no other
> > > > > driver is available.
> > > > 
> > > > That doesn't really work.  I've been talking to the ACPI people about
> > > > this, and the problem is "don't otherwise have a driver for" is an
> > > > impossible thing to prove, as you never know when a driver is going to
> > > > be loaded from userspace.
> > > > 
> > > > You can easily bind drivers to devices today from userspace, why not
> > > > just use the built-in functionality you have today if you "know" that
> > > > there is no driver for this hardware.
> > > 
> > > What we're really after here is that we want to have an spidev
> > > instance when we don't even have a device.
> > 
> > That's crazy, just create a device, things do not work without one.
> 
> Our use case is this one: we want to export spidev files so that "dev
> boards" with a header that allows to plug virtually anything on it
> (Raspberry Pi, Cubieboards, Xplained, and all the likes) without
> having to change the kernel and / or device tree.

You want to do that on a bus that is not self-describing or dynamic?
I too want a pony.  Please go kick the hardware engineer who designed
such a mess, we solved this problem 20+ years ago with "real" busses.

> That would mean that if we plug something to that port, no device will
> be created because the DT itself won't have that device declared in
> the first place.

Because you can't dynamically determine that something was plugged in,
of course.

> This patch is actually doing this: creating a new device for all the
> chipselects that are not in use that will be bound to the spidev
> driver.

I have yet to see a patch...

Re: [PATCH] spi: Force the registration of the spidev devices

[Greg Kroah-Hartman]

On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote:
> On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote:
> > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
> > > Hi Greg,
> > > 
> > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> > > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> > > > > 
> > > > > > While this is nicer than the DT solution because of its accurate hardware
> > > > > > representation, it's still not perfect because you might not have access to the
> > > > > > DT, or you might be driving a completely generic device (such as a
> > > > > > microcontroller) that might be used for something else in a different
> > > > > > context/board.
> > > > > 
> > > > > Greg, you're copied on this because this seems to be a generic problem
> > > > > that should perhaps be solved at a driver model level - having a way to
> > > > > bind userspace access to devices that we don't otherwise have a driver
> > > > > for.  The subsystem could specify the UIO driver to use when no other
> > > > > driver is available.
> > > > 
> > > > That doesn't really work.  I've been talking to the ACPI people about
> > > > this, and the problem is "don't otherwise have a driver for" is an
> > > > impossible thing to prove, as you never know when a driver is going to
> > > > be loaded from userspace.
> > > > 
> > > > You can easily bind drivers to devices today from userspace, why not
> > > > just use the built-in functionality you have today if you "know" that
> > > > there is no driver for this hardware.
> > > 
> > > What we're really after here is that we want to have an spidev
> > > instance when we don't even have a device.
> > 
> > That's crazy, just create a device, things do not work without one.
> 
> Our use case is this one: we want to export spidev files so that "dev
> boards" with a header that allows to plug virtually anything on it
> (Raspberry Pi, Cubieboards, Xplained, and all the likes) without
> having to change the kernel and / or device tree.

You want to do that on a bus that is not self-describing or dynamic?
I too want a pony.  Please go kick the hardware engineer who designed
such a mess, we solved this problem 20+ years ago with "real" busses.

> That would mean that if we plug something to that port, no device will
> be created because the DT itself won't have that device declared in
> the first place.

Because you can't dynamically determine that something was plugged in,
of course.

> This patch is actually doing this: creating a new device for all the
> chipselects that are not in use that will be bound to the spidev
> driver.

I have yet to see a patch...
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 03:33:31PM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote:

> > Our use case is this one: we want to export spidev files so that "dev
> > boards" with a header that allows to plug virtually anything on it
> > (Raspberry Pi, Cubieboards, Xplained, and all the likes) without
> > having to change the kernel and / or device tree.

> You want to do that on a bus that is not self-describing or dynamic?
> I too want a pony.  Please go kick the hardware engineer who designed
> such a mess, we solved this problem 20+ years ago with "real" busses.

The hardware engineers for some of these things did build enumeration in
but none of them with any sort of community have been able to make it
stick - the community just ends up ignoring the ID allocation.

> > This patch is actually doing this: creating a new device for all the
> > chipselects that are not in use that will be bound to the spidev
> > driver.

> I have yet to see a patch...

That was the first message in the thread.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, May 13, 2015 at 03:33:31PM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote:

> > Our use case is this one: we want to export spidev files so that "dev
> > boards" with a header that allows to plug virtually anything on it
> > (Raspberry Pi, Cubieboards, Xplained, and all the likes) without
> > having to change the kernel and / or device tree.

> You want to do that on a bus that is not self-describing or dynamic?
> I too want a pony.  Please go kick the hardware engineer who designed
> such a mess, we solved this problem 20+ years ago with "real" busses.

The hardware engineers for some of these things did build enumeration in
but none of them with any sort of community have been able to make it
stick - the community just ends up ignoring the ID allocation.

> > This patch is actually doing this: creating a new device for all the
> > chipselects that are not in use that will be bound to the spidev
> > driver.

> I have yet to see a patch...

That was the first message in the thread.

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

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, May 13, 2015 at 03:33:31PM -0700, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote:
> > On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote:
> > > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
> > > > Hi Greg,
> > > > 
> > > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
> > > > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > > > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
> > > > > > 
> > > > > > > While this is nicer than the DT solution because of its accurate hardware
> > > > > > > representation, it's still not perfect because you might not have access to the
> > > > > > > DT, or you might be driving a completely generic device (such as a
> > > > > > > microcontroller) that might be used for something else in a different
> > > > > > > context/board.
> > > > > > 
> > > > > > Greg, you're copied on this because this seems to be a generic problem
> > > > > > that should perhaps be solved at a driver model level - having a way to
> > > > > > bind userspace access to devices that we don't otherwise have a driver
> > > > > > for.  The subsystem could specify the UIO driver to use when no other
> > > > > > driver is available.
> > > > > 
> > > > > That doesn't really work.  I've been talking to the ACPI people about
> > > > > this, and the problem is "don't otherwise have a driver for" is an
> > > > > impossible thing to prove, as you never know when a driver is going to
> > > > > be loaded from userspace.
> > > > > 
> > > > > You can easily bind drivers to devices today from userspace, why not
> > > > > just use the built-in functionality you have today if you "know" that
> > > > > there is no driver for this hardware.
> > > > 
> > > > What we're really after here is that we want to have an spidev
> > > > instance when we don't even have a device.
> > > 
> > > That's crazy, just create a device, things do not work without one.
> > 
> > Our use case is this one: we want to export spidev files so that "dev
> > boards" with a header that allows to plug virtually anything on it
> > (Raspberry Pi, Cubieboards, Xplained, and all the likes) without
> > having to change the kernel and / or device tree.
> 
> You want to do that on a bus that is not self-describing or dynamic?
> I too want a pony.  Please go kick the hardware engineer who designed
> such a mess, we solved this problem 20+ years ago with "real" busses.

Well, we do have such ponies on some bus that don't have any kind of
enumeration. i2cdev allows to do just that already. That would seem
logical to have a similar behaviour for SPI.

> > That would mean that if we plug something to that port, no device will
> > be created because the DT itself won't have that device declared in
> > the first place.
> 
> Because you can't dynamically determine that something was plugged in,
> of course.

Well.. Yeah.

> 
> > This patch is actually doing this: creating a new device for all the
> > chipselects that are not in use that will be bound to the spidev
> > driver.
> 
> I have yet to see a patch...

You were in Cc of that patch, which is the first message in this thread.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Lucas De Marchi]

On Wed, May 13, 2015 at 7:33 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote:
>> On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote:
>> > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
>> > > Hi Greg,
>> > >
>> > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
>> > > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
>> > > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
>> > > > >
>> > > > > > While this is nicer than the DT solution because of its accurate hardware
>> > > > > > representation, it's still not perfect because you might not have access to the
>> > > > > > DT, or you might be driving a completely generic device (such as a
>> > > > > > microcontroller) that might be used for something else in a different
>> > > > > > context/board.
>> > > > >
>> > > > > Greg, you're copied on this because this seems to be a generic problem
>> > > > > that should perhaps be solved at a driver model level - having a way to
>> > > > > bind userspace access to devices that we don't otherwise have a driver
>> > > > > for.  The subsystem could specify the UIO driver to use when no other
>> > > > > driver is available.
>> > > >
>> > > > That doesn't really work.  I've been talking to the ACPI people about
>> > > > this, and the problem is "don't otherwise have a driver for" is an
>> > > > impossible thing to prove, as you never know when a driver is going to
>> > > > be loaded from userspace.
>> > > >
>> > > > You can easily bind drivers to devices today from userspace, why not
>> > > > just use the built-in functionality you have today if you "know" that
>> > > > there is no driver for this hardware.
>> > >
>> > > What we're really after here is that we want to have an spidev
>> > > instance when we don't even have a device.
>> >
>> > That's crazy, just create a device, things do not work without one.
>>
>> Our use case is this one: we want to export spidev files so that "dev
>> boards" with a header that allows to plug virtually anything on it
>> (Raspberry Pi, Cubieboards, Xplained, and all the likes) without
>> having to change the kernel and / or device tree.
>
> You want to do that on a bus that is not self-describing or dynamic?
> I too want a pony.  Please go kick the hardware engineer who designed
> such a mess, we solved this problem 20+ years ago with "real" busses.

Mind you we are talking about buses created more than 20+ years ago.
(Unfortunately) They are still used today for all kind of sensors.
Boards like RPi, beaglebone, minnowboard expose the pins so we can
actually talk to those sensors, plugging in anyone we'd like to.  For
some of them for example there are IIO drivers that we could use the
driver model to allow binding them. But spidev/i2c-dev allow userspace
to talk directly to them. And you don't know what *others* will plug
into that bus... might even be their own microntrollers with no
identification at all.

Without something like the patch in the first message, people need to
create DT overlays for platforms that support that. ACPI doesn't
support overlays (yet) so we need to keep awful external platform
drivers[1] just to make spidev to work.

-- 
Lucas De Marchi

[1] https://github.com/MinnowBoard/minnow-max-extras/blob/master/modules/low-speed-spidev/low-speed-spidev.c

Re: [PATCH] spi: Force the registration of the spidev devices

[Lucas De Marchi]

On Wed, May 13, 2015 at 7:33 PM, Greg Kroah-Hartman
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote:
>> On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote:
>> > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote:
>> > > Hi Greg,
>> > >
>> > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote:
>> > > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
>> > > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote:
>> > > > >
>> > > > > > While this is nicer than the DT solution because of its accurate hardware
>> > > > > > representation, it's still not perfect because you might not have access to the
>> > > > > > DT, or you might be driving a completely generic device (such as a
>> > > > > > microcontroller) that might be used for something else in a different
>> > > > > > context/board.
>> > > > >
>> > > > > Greg, you're copied on this because this seems to be a generic problem
>> > > > > that should perhaps be solved at a driver model level - having a way to
>> > > > > bind userspace access to devices that we don't otherwise have a driver
>> > > > > for.  The subsystem could specify the UIO driver to use when no other
>> > > > > driver is available.
>> > > >
>> > > > That doesn't really work.  I've been talking to the ACPI people about
>> > > > this, and the problem is "don't otherwise have a driver for" is an
>> > > > impossible thing to prove, as you never know when a driver is going to
>> > > > be loaded from userspace.
>> > > >
>> > > > You can easily bind drivers to devices today from userspace, why not
>> > > > just use the built-in functionality you have today if you "know" that
>> > > > there is no driver for this hardware.
>> > >
>> > > What we're really after here is that we want to have an spidev
>> > > instance when we don't even have a device.
>> >
>> > That's crazy, just create a device, things do not work without one.
>>
>> Our use case is this one: we want to export spidev files so that "dev
>> boards" with a header that allows to plug virtually anything on it
>> (Raspberry Pi, Cubieboards, Xplained, and all the likes) without
>> having to change the kernel and / or device tree.
>
> You want to do that on a bus that is not self-describing or dynamic?
> I too want a pony.  Please go kick the hardware engineer who designed
> such a mess, we solved this problem 20+ years ago with "real" busses.

Mind you we are talking about buses created more than 20+ years ago.
(Unfortunately) They are still used today for all kind of sensors.
Boards like RPi, beaglebone, minnowboard expose the pins so we can
actually talk to those sensors, plugging in anyone we'd like to.  For
some of them for example there are IIO drivers that we could use the
driver model to allow binding them. But spidev/i2c-dev allow userspace
to talk directly to them. And you don't know what *others* will plug
into that bus... might even be their own microntrollers with no
identification at all.

Without something like the patch in the first message, people need to
create DT overlays for platforms that support that. ACPI doesn't
support overlays (yet) so we need to keep awful external platform
drivers[1] just to make spidev to work.

-- 
Lucas De Marchi

[1] https://github.com/MinnowBoard/minnow-max-extras/blob/master/modules/low-speed-spidev/low-speed-spidev.c
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Mon, May 05, 2014 at 12:17:23PM -0700, Mark Brown wrote:
> On Sun, May 04, 2014 at 11:21:47PM -0500, Maxime Ripard wrote:
> > On Fri, May 02, 2014 at 10:40:48AM -0700, Mark Brown wrote:
> > > > i2c-dev works great in these cases, because you always have access to
> > > > all the bus, and all the devices, except if the device is already used
> > > > by someone. The patch I suggested is an attempt to mimic this.
> 
> > > It seems better to implement something like this at the device model
> > > level, provide a way to have a default UIO driver for anything on a
> > > given bus.  I don't see anything bus specific apart from saying what the
> > > default driver to use is and it avoids the icky code fiddling about with
> > > what devices are bound and the races that might be involved duplicated
> > > in individual buses.
> 
> > Hmmm, yes, that's probably a great long-term way of dealing with this,
> > but I don't see it happening soon.
> 
> Isn't the code in the patch that started this thread roughly what's
> needed, just done in a SPI specific way instead of a generic way?

Hmmm, I think I get your point now. Yes, we could do it. Let me find
some time to actually write something :)

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Mon, May 05, 2014 at 12:17:23PM -0700, Mark Brown wrote:
> On Sun, May 04, 2014 at 11:21:47PM -0500, Maxime Ripard wrote:
> > On Fri, May 02, 2014 at 10:40:48AM -0700, Mark Brown wrote:
> > > > i2c-dev works great in these cases, because you always have access to
> > > > all the bus, and all the devices, except if the device is already used
> > > > by someone. The patch I suggested is an attempt to mimic this.
> 
> > > It seems better to implement something like this at the device model
> > > level, provide a way to have a default UIO driver for anything on a
> > > given bus.  I don't see anything bus specific apart from saying what the
> > > default driver to use is and it avoids the icky code fiddling about with
> > > what devices are bound and the races that might be involved duplicated
> > > in individual buses.
> 
> > Hmmm, yes, that's probably a great long-term way of dealing with this,
> > but I don't see it happening soon.
> 
> Isn't the code in the patch that started this thread roughly what's
> needed, just done in a SPI specific way instead of a generic way?

Hmmm, I think I get your point now. Yes, we could do it. Let me find
some time to actually write something :)

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Sun, May 04, 2014 at 11:21:47PM -0500, Maxime Ripard wrote:
> On Fri, May 02, 2014 at 10:40:48AM -0700, Mark Brown wrote:
> > > i2c-dev works great in these cases, because you always have access to
> > > all the bus, and all the devices, except if the device is already used
> > > by someone. The patch I suggested is an attempt to mimic this.

> > It seems better to implement something like this at the device model
> > level, provide a way to have a default UIO driver for anything on a
> > given bus.  I don't see anything bus specific apart from saying what the
> > default driver to use is and it avoids the icky code fiddling about with
> > what devices are bound and the races that might be involved duplicated
> > in individual buses.

> Hmmm, yes, that's probably a great long-term way of dealing with this,
> but I don't see it happening soon.

Isn't the code in the patch that started this thread roughly what's
needed, just done in a SPI specific way instead of a generic way?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Sun, May 04, 2014 at 11:21:47PM -0500, Maxime Ripard wrote:
> On Fri, May 02, 2014 at 10:40:48AM -0700, Mark Brown wrote:
> > > i2c-dev works great in these cases, because you always have access to
> > > all the bus, and all the devices, except if the device is already used
> > > by someone. The patch I suggested is an attempt to mimic this.

> > It seems better to implement something like this at the device model
> > level, provide a way to have a default UIO driver for anything on a
> > given bus.  I don't see anything bus specific apart from saying what the
> > default driver to use is and it avoids the icky code fiddling about with
> > what devices are bound and the races that might be involved duplicated
> > in individual buses.

> Hmmm, yes, that's probably a great long-term way of dealing with this,
> but I don't see it happening soon.

Isn't the code in the patch that started this thread roughly what's
needed, just done in a SPI specific way instead of a generic way?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Mon, May 05, 2014 at 09:10:43AM +0200, Geert Uytterhoeven wrote:
> On Mon, May 5, 2014 at 6:17 AM, Maxime Ripard

> > Not really, because you can't declare a spidev device in the DT.

> Yes you can. I've done it before.

> See also "git grep -w spidev -- arch/arm/*/dts/".

It is technically possible but it is a bad idea that should not get past
review - it's putting a specific bit of Linux implementation detail
about how we want to to control the relevant device at this specific
time into the DT.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Geert Uytterhoeven]

Hi Alexandre,

On Mon, May 5, 2014 at 3:57 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 05/05/2014 at 09:10:43 +0200, Geert Uytterhoeven wrote :
>> On Mon, May 5, 2014 at 6:17 AM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Fri, May 02, 2014 at 01:28:26AM +0200, Geert Uytterhoeven wrote:
>> >> On Fri, May 2, 2014 at 12:36 AM, Maxime Ripard
>> >> <maxime.ripard@free-electrons.com> wrote:
>> >> > But it actually doesn't work in a case where you can't really predict
>> >> > what is on the other side of the bus. Either because, on the board
>> >> > you're using the pins are exposed and it's pretty much up to the user
>> >> > to know what to put on it. That could be handled by DT overlays
>> >> > though.
>> >> >
>> >> > What never works is where the device on the other side is so generic
>> >> > that you really can't tell what it does. Think of a microcontroller
>> >> > that would behave as a SPI slave. It's behaviour and what it does is
>> >> > pretty much dependant of what we flashed on it, and suddenly the
>> >> > compatible string is not the proper reprensentation anymore.
>> >>
>> >> So you will (hopefully soon) use overlay DT to change the DTS to match what's
>> >> connected?
>> >
>> > Not really, because you can't declare a spidev device in the DT.
>>
>> Yes you can. I've done it before.
>>
>> See also "git grep -w spidev -- arch/arm/*/dts/".
>
> I'm pretty sure that doesn't work as there is no compatible matching
> "spidev"

See the last line of spi_match_device():

        return strcmp(spi->modalias, drv->name) == 0;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: Force the registration of the spidev devices

[Geert Uytterhoeven]

Hi Alexandre,

On Mon, May 5, 2014 at 3:57 PM, Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On 05/05/2014 at 09:10:43 +0200, Geert Uytterhoeven wrote :
>> On Mon, May 5, 2014 at 6:17 AM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > On Fri, May 02, 2014 at 01:28:26AM +0200, Geert Uytterhoeven wrote:
>> >> On Fri, May 2, 2014 at 12:36 AM, Maxime Ripard
>> >> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> >> > But it actually doesn't work in a case where you can't really predict
>> >> > what is on the other side of the bus. Either because, on the board
>> >> > you're using the pins are exposed and it's pretty much up to the user
>> >> > to know what to put on it. That could be handled by DT overlays
>> >> > though.
>> >> >
>> >> > What never works is where the device on the other side is so generic
>> >> > that you really can't tell what it does. Think of a microcontroller
>> >> > that would behave as a SPI slave. It's behaviour and what it does is
>> >> > pretty much dependant of what we flashed on it, and suddenly the
>> >> > compatible string is not the proper reprensentation anymore.
>> >>
>> >> So you will (hopefully soon) use overlay DT to change the DTS to match what's
>> >> connected?
>> >
>> > Not really, because you can't declare a spidev device in the DT.
>>
>> Yes you can. I've done it before.
>>
>> See also "git grep -w spidev -- arch/arm/*/dts/".
>
> I'm pretty sure that doesn't work as there is no compatible matching
> "spidev"

See the last line of spi_match_device():

        return strcmp(spi->modalias, drv->name) == 0;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Alexandre Belloni]

On 05/05/2014 at 09:10:43 +0200, Geert Uytterhoeven wrote :
> Hi Maxime,
> 
> On Mon, May 5, 2014 at 6:17 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Fri, May 02, 2014 at 01:28:26AM +0200, Geert Uytterhoeven wrote:
> >> On Fri, May 2, 2014 at 12:36 AM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > But it actually doesn't work in a case where you can't really predict
> >> > what is on the other side of the bus. Either because, on the board
> >> > you're using the pins are exposed and it's pretty much up to the user
> >> > to know what to put on it. That could be handled by DT overlays
> >> > though.
> >> >
> >> > What never works is where the device on the other side is so generic
> >> > that you really can't tell what it does. Think of a microcontroller
> >> > that would behave as a SPI slave. It's behaviour and what it does is
> >> > pretty much dependant of what we flashed on it, and suddenly the
> >> > compatible string is not the proper reprensentation anymore.
> >>
> >> So you will (hopefully soon) use overlay DT to change the DTS to match what's
> >> connected?
> >
> > Not really, because you can't declare a spidev device in the DT.
> 
> Yes you can. I've done it before.
> 
> See also "git grep -w spidev -- arch/arm/*/dts/".
> 

I'm pretty sure that doesn't work as there is no compatible matching
"spidev"

My guess would be that you added it in spidev.c

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] spi: Force the registration of the spidev devices

[Alexandre Belloni]

On 05/05/2014 at 09:10:43 +0200, Geert Uytterhoeven wrote :
> Hi Maxime,
> 
> On Mon, May 5, 2014 at 6:17 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Fri, May 02, 2014 at 01:28:26AM +0200, Geert Uytterhoeven wrote:
> >> On Fri, May 2, 2014 at 12:36 AM, Maxime Ripard
> >> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> > But it actually doesn't work in a case where you can't really predict
> >> > what is on the other side of the bus. Either because, on the board
> >> > you're using the pins are exposed and it's pretty much up to the user
> >> > to know what to put on it. That could be handled by DT overlays
> >> > though.
> >> >
> >> > What never works is where the device on the other side is so generic
> >> > that you really can't tell what it does. Think of a microcontroller
> >> > that would behave as a SPI slave. It's behaviour and what it does is
> >> > pretty much dependant of what we flashed on it, and suddenly the
> >> > compatible string is not the proper reprensentation anymore.
> >>
> >> So you will (hopefully soon) use overlay DT to change the DTS to match what's
> >> connected?
> >
> > Not really, because you can't declare a spidev device in the DT.
> 
> Yes you can. I've done it before.
> 
> See also "git grep -w spidev -- arch/arm/*/dts/".
> 

I'm pretty sure that doesn't work as there is no compatible matching
"spidev"

My guess would be that you added it in spidev.c

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Geert Uytterhoeven]

Hi Maxime,

On Mon, May 5, 2014 at 6:17 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Fri, May 02, 2014 at 01:28:26AM +0200, Geert Uytterhoeven wrote:
>> On Fri, May 2, 2014 at 12:36 AM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > But it actually doesn't work in a case where you can't really predict
>> > what is on the other side of the bus. Either because, on the board
>> > you're using the pins are exposed and it's pretty much up to the user
>> > to know what to put on it. That could be handled by DT overlays
>> > though.
>> >
>> > What never works is where the device on the other side is so generic
>> > that you really can't tell what it does. Think of a microcontroller
>> > that would behave as a SPI slave. It's behaviour and what it does is
>> > pretty much dependant of what we flashed on it, and suddenly the
>> > compatible string is not the proper reprensentation anymore.
>>
>> So you will (hopefully soon) use overlay DT to change the DTS to match what's
>> connected?
>
> Not really, because you can't declare a spidev device in the DT.

Yes you can. I've done it before.

See also "git grep -w spidev -- arch/arm/*/dts/".

>> And then you want spidev to bind to it. Would it help if DT offered a feature
>> to add a compatible entry to a driver at runtime, cfr.
>> /sys/bus/pci/drivers/.../new_id on PCI?
>
> The thing is, in the core SPI bus, devices are only instiated (in the
> DT case), by parsing the DT. So, to bind a device to a new driver, it
> has to exist in the first place, and it won't exist, because there
> won't be any node in the DT for this device. But since DT is
> unsuitable for such a device, you go back to where we were initially.

So you add a node in the overlay DT? Or what am I missing?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Fri, May 02, 2014 at 10:40:48AM -0700, Mark Brown wrote:
> > i2c-dev works great in these cases, because you always have access to
> > all the bus, and all the devices, except if the device is already used
> > by someone. The patch I suggested is an attempt to mimic this.
> 
> It seems better to implement something like this at the device model
> level, provide a way to have a default UIO driver for anything on a
> given bus.  I don't see anything bus specific apart from saying what the
> default driver to use is and it avoids the icky code fiddling about with
> what devices are bound and the races that might be involved duplicated
> in individual buses.

Hmmm, yes, that's probably a great long-term way of dealing with this,
but I don't see it happening soon.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

Hi Geert

On Fri, May 02, 2014 at 01:28:26AM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Fri, May 2, 2014 at 12:36 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > But it actually doesn't work in a case where you can't really predict
> > what is on the other side of the bus. Either because, on the board
> > you're using the pins are exposed and it's pretty much up to the user
> > to know what to put on it. That could be handled by DT overlays
> > though.
> >
> > What never works is where the device on the other side is so generic
> > that you really can't tell what it does. Think of a microcontroller
> > that would behave as a SPI slave. It's behaviour and what it does is
> > pretty much dependant of what we flashed on it, and suddenly the
> > compatible string is not the proper reprensentation anymore.
> 
> So you will (hopefully soon) use overlay DT to change the DTS to match what's
> connected?

Not really, because you can't declare a spidev device in the DT.

> And then you want spidev to bind to it. Would it help if DT offered a feature
> to add a compatible entry to a driver at runtime, cfr.
> /sys/bus/pci/drivers/.../new_id on PCI?

The thing is, in the core SPI bus, devices are only instiated (in the
DT case), by parsing the DT. So, to bind a device to a new driver, it
has to exist in the first place, and it won't exist, because there
won't be any node in the DT for this device. But since DT is
unsuitable for such a device, you go back to where we were initially.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Thu, May 01, 2014 at 03:36:29PM -0700, Maxime Ripard wrote:

> But it actually doesn't work in a case where you can't really predict
> what is on the other side of the bus. Either because, on the board
> you're using the pins are exposed and it's pretty much up to the user
> to know what to put on it. That could be handled by DT overlays
> though.

Right, that's a completely unrelated issue.

> What never works is where the device on the other side is so generic
> that you really can't tell what it does. Think of a microcontroller
> that would behave as a SPI slave. It's behaviour and what it does is
> pretty much dependant of what we flashed on it, and suddenly the
> compatible string is not the proper reprensentation anymore.

That sounds like another situation for overlays, really the thing that's
compatible here is going to be the whole assembly rather than just the
device in isolation (or you're in a similar situation to the one you're
in with FPGAs where you have a driver implementing an application like a
loadeer

> i2c-dev works great in these cases, because you always have access to
> all the bus, and all the devices, except if the device is already used
> by someone. The patch I suggested is an attempt to mimic this.

It seems better to implement something like this at the device model
level, provide a way to have a default UIO driver for anything on a
given bus.  I don't see anything bus specific apart from saying what the
default driver to use is and it avoids the icky code fiddling about with
what devices are bound and the races that might be involved duplicated
in individual buses.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Thu, May 01, 2014 at 03:36:29PM -0700, Maxime Ripard wrote:

> But it actually doesn't work in a case where you can't really predict
> what is on the other side of the bus. Either because, on the board
> you're using the pins are exposed and it's pretty much up to the user
> to know what to put on it. That could be handled by DT overlays
> though.

Right, that's a completely unrelated issue.

> What never works is where the device on the other side is so generic
> that you really can't tell what it does. Think of a microcontroller
> that would behave as a SPI slave. It's behaviour and what it does is
> pretty much dependant of what we flashed on it, and suddenly the
> compatible string is not the proper reprensentation anymore.

That sounds like another situation for overlays, really the thing that's
compatible here is going to be the whole assembly rather than just the
device in isolation (or you're in a similar situation to the one you're
in with FPGAs where you have a driver implementing an application like a
loadeer

> i2c-dev works great in these cases, because you always have access to
> all the bus, and all the devices, except if the device is already used
> by someone. The patch I suggested is an attempt to mimic this.

It seems better to implement something like this at the device model
level, provide a way to have a default UIO driver for anything on a
given bus.  I don't see anything bus specific apart from saying what the
default driver to use is and it avoids the icky code fiddling about with
what devices are bound and the races that might be involved duplicated
in individual buses.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Fri, May 02, 2014 at 01:28:26AM +0200, Geert Uytterhoeven wrote:

> And then you want spidev to bind to it. Would it help if DT offered a feature
> to add a compatible entry to a driver at runtime, cfr.
> /sys/bus/pci/drivers/.../new_id on PCI?

Yes, that's what I'd been under the impression that the bind file did -
allowed modification of the list of devices to bind.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Fri, May 02, 2014 at 01:28:26AM +0200, Geert Uytterhoeven wrote:

> And then you want spidev to bind to it. Would it help if DT offered a feature
> to add a compatible entry to a driver at runtime, cfr.
> /sys/bus/pci/drivers/.../new_id on PCI?

Yes, that's what I'd been under the impression that the bind file did -
allowed modification of the list of devices to bind.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Geert Uytterhoeven]

Hi Maxime,

On Fri, May 2, 2014 at 12:36 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> But it actually doesn't work in a case where you can't really predict
> what is on the other side of the bus. Either because, on the board
> you're using the pins are exposed and it's pretty much up to the user
> to know what to put on it. That could be handled by DT overlays
> though.
>
> What never works is where the device on the other side is so generic
> that you really can't tell what it does. Think of a microcontroller
> that would behave as a SPI slave. It's behaviour and what it does is
> pretty much dependant of what we flashed on it, and suddenly the
> compatible string is not the proper reprensentation anymore.

So you will (hopefully soon) use overlay DT to change the DTS to match what's
connected?

And then you want spidev to bind to it. Would it help if DT offered a feature
to add a compatible entry to a driver at runtime, cfr.
/sys/bus/pci/drivers/.../new_id on PCI?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, Apr 30, 2014 at 06:18:11PM -0700, Mark Brown wrote:
> On Wed, Apr 30, 2014 at 11:06:09AM -0700, Maxime Ripard wrote:
> > On Tue, Apr 29, 2014 at 11:37:58AM -0700, Mark Brown wrote:
> 
> > > Why can we not handle this by using sysfs to bind spidev to the
> > > device?
> 
> > I just tried it, and apparently, you can't really use this, since spi
> > devices are created from the device tree (or ACPI) whenever the master
> > registers.
> 
> Can you be more specific as to what the issue is here?  If we actually
> have a specific kernel driver for a device I would strongly expect that
> we would want to use it and not spidev, if we don't have one I don't see
> the issue.
>
> > It doesn't really work either for a device that would be bound to a
> > driver, that you unbind, and then try to bind to spidev instead. It
> > looks like the device is released whenever you unbind it, so you can't
> > really use it afterwards.
> 
> I guess this is the issue...  what exactly is the use case here?  I
> would only expect spidev to be used if there is no in kernel driver for
> a device.

The issue and use case is this: we don't have an upstreamable way to
use spidev.

You suggested a while back to add the compatibles of devices we would
want to drive with spidev in the spidev compatible list. It's fine for
devices where we should have a driver, but don't, or devices that will
never ever be handled by the kernel.

But it actually doesn't work in a case where you can't really predict
what is on the other side of the bus. Either because, on the board
you're using the pins are exposed and it's pretty much up to the user
to know what to put on it. That could be handled by DT overlays
though.

What never works is where the device on the other side is so generic
that you really can't tell what it does. Think of a microcontroller
that would behave as a SPI slave. It's behaviour and what it does is
pretty much dependant of what we flashed on it, and suddenly the
compatible string is not the proper reprensentation anymore.

i2c-dev works great in these cases, because you always have access to
all the bus, and all the devices, except if the device is already used
by someone. The patch I suggested is an attempt to mimic this.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, Apr 30, 2014 at 06:18:11PM -0700, Mark Brown wrote:
> On Wed, Apr 30, 2014 at 11:06:09AM -0700, Maxime Ripard wrote:
> > On Tue, Apr 29, 2014 at 11:37:58AM -0700, Mark Brown wrote:
> 
> > > Why can we not handle this by using sysfs to bind spidev to the
> > > device?
> 
> > I just tried it, and apparently, you can't really use this, since spi
> > devices are created from the device tree (or ACPI) whenever the master
> > registers.
> 
> Can you be more specific as to what the issue is here?  If we actually
> have a specific kernel driver for a device I would strongly expect that
> we would want to use it and not spidev, if we don't have one I don't see
> the issue.
>
> > It doesn't really work either for a device that would be bound to a
> > driver, that you unbind, and then try to bind to spidev instead. It
> > looks like the device is released whenever you unbind it, so you can't
> > really use it afterwards.
> 
> I guess this is the issue...  what exactly is the use case here?  I
> would only expect spidev to be used if there is no in kernel driver for
> a device.

The issue and use case is this: we don't have an upstreamable way to
use spidev.

You suggested a while back to add the compatibles of devices we would
want to drive with spidev in the spidev compatible list. It's fine for
devices where we should have a driver, but don't, or devices that will
never ever be handled by the kernel.

But it actually doesn't work in a case where you can't really predict
what is on the other side of the bus. Either because, on the board
you're using the pins are exposed and it's pretty much up to the user
to know what to put on it. That could be handled by DT overlays
though.

What never works is where the device on the other side is so generic
that you really can't tell what it does. Think of a microcontroller
that would behave as a SPI slave. It's behaviour and what it does is
pretty much dependant of what we flashed on it, and suddenly the
compatible string is not the proper reprensentation anymore.

i2c-dev works great in these cases, because you always have access to
all the bus, and all the devices, except if the device is already used
by someone. The patch I suggested is an attempt to mimic this.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, Apr 30, 2014 at 10:00:18PM +0200, Martin Sperl wrote:
> 
> On 30.04.2014, at 20:14, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> > I'd really be in favor of using the DT overlays for this. All this is
> > nice, but very fragile. You can't really handle any weird
> > configurations that you might encounter, like weird interrupt
> > controllers that might be requiring more informations, or you don't
> > set any interrupt parents, and this parameter list will basically be
> > an ever-growing list of parameters to deal with all kind of crazy
> > corner-cases, and I'm not sure it's something we want.

> Problem is getting DT overlays included in the kernel it seems...

We should fix that and solve this problem for all kinds of devices (and
some other similar use cases like the FPGAs).

> Following your argument we should not have this interface either
> but use DT for these GPIOs as well - similar argument for I2C...

> Still we have them and they are heavily used by those dev-boards,
> so why not something along the same lines also for SPI?

Those are legacy things, their presence doesn't mean they're a good
idea.  If there's problems with the userspace tooling for using overlays
when they get merged then that's the layer to fix things.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, Apr 30, 2014 at 11:06:09AM -0700, Maxime Ripard wrote:
> On Tue, Apr 29, 2014 at 11:37:58AM -0700, Mark Brown wrote:

> > Why can we not handle this by using sysfs to bind spidev to the
> > device?

> I just tried it, and apparently, you can't really use this, since spi
> devices are created from the device tree (or ACPI) whenever the master
> registers.

Can you be more specific as to what the issue is here?  If we actually
have a specific kernel driver for a device I would strongly expect that
we would want to use it and not spidev, if we don't have one I don't see
the issue.

> It doesn't really work either for a device that would be bound to a
> driver, that you unbind, and then try to bind to spidev instead. It
> looks like the device is released whenever you unbind it, so you can't
> really use it afterwards.

I guess this is the issue...  what exactly is the use case here?  I
would only expect spidev to be used if there is no in kernel driver for
a device.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Wed, Apr 30, 2014 at 11:06:09AM -0700, Maxime Ripard wrote:
> On Tue, Apr 29, 2014 at 11:37:58AM -0700, Mark Brown wrote:

> > Why can we not handle this by using sysfs to bind spidev to the
> > device?

> I just tried it, and apparently, you can't really use this, since spi
> devices are created from the device tree (or ACPI) whenever the master
> registers.

Can you be more specific as to what the issue is here?  If we actually
have a specific kernel driver for a device I would strongly expect that
we would want to use it and not spidev, if we don't have one I don't see
the issue.

> It doesn't really work either for a device that would be bound to a
> driver, that you unbind, and then try to bind to spidev instead. It
> looks like the device is released whenever you unbind it, so you can't
> really use it afterwards.

I guess this is the issue...  what exactly is the use case here?  I
would only expect spidev to be used if there is no in kernel driver for
a device.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Wed, Apr 30, 2014 at 10:00:18PM +0200, Martin Sperl wrote:
> 
> On 30.04.2014, at 20:14, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > I'd really be in favor of using the DT overlays for this. All this is
> > nice, but very fragile. You can't really handle any weird
> > configurations that you might encounter, like weird interrupt
> > controllers that might be requiring more informations, or you don't
> > set any interrupt parents, and this parameter list will basically be
> > an ever-growing list of parameters to deal with all kind of crazy
> > corner-cases, and I'm not sure it's something we want.
> > 
> Problem is getting DT overlays included in the kernel it seems...
> 
> I agree arguments will tend to increase, but the "core" stuff would be
> not changing all that often, as there are only so many parameters of
> spi_device that can get set/configured anyway.
> 
> Everything else is either a driver specific parameter that the driver
> supports itself or we have to resort to some voodoo magic blob 
> kind of thing.
> 
> But for the most part we want to configure simple devices for those
> boards: ADCs, LCD-Displays, Network Controller,...

Except that for every single one of these devices, you'd have
different requirements, since platform data are driver specific.

> These typically do not have that many parameters of their own that 
> have to be set up during probing/initialization (mostly interrupt flags,
> maybe some extra GPIO, maybe an attached clock speed).

Except that even if the properties are similar, you'd have to maintain
the map to your common definition to the actual structures.

> The other stuff gets typically set up via other interfaces 
> (netlink et.al) anyway. So I do not see that many parameters that 
> would realistically get set.
> 
> Also note that this should allow people to do simple stuff quickly 
> - like spidev - not complex stuff.

Yeah, but you were talking about LCD displays, that are just crazy
complex most of the time.

> So from the support-perspective of an end-users it is easier to say:
> * echo "cs=1:...." > /sys/bus/spi_root/spi0/register
> * if it does not work: 
>   echo "cs=1" > /sys/bus/spi_root/spi0/register
> * and run again slightly modified: 
>   echo "cs=1:...." > /sys/bus/spi_root/spi0/register

And the devil is in these "...."

> than:
> * install dt compiler with 
> * create that file (dt) and modify the settings
> * run the compiler
> * copy the compiled DT-overlay to /lib/firmware
> * echo "..." > /sys/devices/bone_capemgr.*/slots
> * reboot if it does not work...
> * run steps 2 to 5 again and see if it works...

I'm not sure that "kernel development is hard" has ever been a good
argument.

> SPIDEV is for people wanting to do rapid development.

Yep. It can be used for that. But what you suggest goes far beyond
spidev alone.

> The register part + kernel-driver is the next step up in complexity, 
> where spidev is not fast enough for some reason, or does not provide
> for all the needs (shared access,...).
> 
> The complex clock example of yours is something that would fall in the 
> DT-overlay case - these things are most likely too complicated for 
> the simple user and requires extensive knowledge of the design.
> Those people tend to know how to build a kernel and compile DTs.
> 
> Different tools for different level of complexities.

So, you're saying that, to the end-users you're concerned about,
saying "yeaaah, you know, sometimes you can use this spi register
thing, sometimes you can use the DT overlays, but you know, only in
cases where it is complicated." is actually easier than being
consitent and always requiring to use the overlays?

> > You listed only ARM boards, that are not supposed to be non-DT. RPi is
> > using DT fine, just like the beaglebones. I don't really see why we
> > should care about !DT cases.
> I listed those ARM boards because I have most experience with them.
> 
> I do not know how those INTEL Galileo boards work, but I doubt 
> they run DT - ACPI more likely.
> But people still want to do rapid development on those using existing
> arduino shields, so the DT-overlay requirement would cut them off.

Then ACPI should probably provide informations on whatever shield is
plugged in there.

> Same for future ARM64 devices where there seems to be a push by 
> vendors to move to ACPI for those systems - not DT.

Both ACPI and DT will be used for ARM64.

> Other examples are the WIFI router, most of which also have SPI 
> interfaces and people have done all sorts of things with them - not 
> all run ARM either - at least all those WIFI routers I have seen
> so far do not run DT-kernels...

I'm not sure I get your point here. Most likely, the router won't have
a kernel that have your tool anyway. So you'd have to recompile
it. Why not just change the board file (if it's a board file) before
recompiling?

> So there is more to it than just ARM and we should be able to handle
> the "easy" cases for all systems independent from how we configure 
> the rest of the system.

But there is not much more than ACPI + DT + board files.

> Also the proposed setting is along the same lines as GPIO management
> via /sys/class/gpio.

Not at all. Individual GPIOs are not single devices. The GPIOs
controllers are. And you can't bind or load those controllers driver
from /sys/class/gpios

> Following your argument we should not have this interface either
> but use DT for these GPIOs as well - similar argument for I2C...

Hmmm, you can't do it for i2c. Or you're no longer talking about
"loading any driver on any device through sysfs", and only about
i2c-dev. In this case, Take a look at the patch that started the
discussion, and Mark's comments.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Martin Sperl]

On 30.04.2014, at 20:14, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> I'd really be in favor of using the DT overlays for this. All this is
> nice, but very fragile. You can't really handle any weird
> configurations that you might encounter, like weird interrupt
> controllers that might be requiring more informations, or you don't
> set any interrupt parents, and this parameter list will basically be
> an ever-growing list of parameters to deal with all kind of crazy
> corner-cases, and I'm not sure it's something we want.
> 
Problem is getting DT overlays included in the kernel it seems...

I agree arguments will tend to increase, but the "core" stuff would be
not changing all that often, as there are only so many parameters of
spi_device that can get set/configured anyway.

Everything else is either a driver specific parameter that the driver
supports itself or we have to resort to some voodoo magic blob 
kind of thing.

But for the most part we want to configure simple devices for those
boards: ADCs, LCD-Displays, Network Controller,...

These typically do not have that many parameters of their own that 
have to be set up during probing/initialization (mostly interrupt flags,
maybe some extra GPIO, maybe an attached clock speed).

The other stuff gets typically set up via other interfaces 
(netlink et.al) anyway. So I do not see that many parameters that 
would realistically get set.

Also note that this should allow people to do simple stuff quickly 
- like spidev - not complex stuff.

So from the support-perspective of an end-users it is easier to say:
* echo "cs=1:...." > /sys/bus/spi_root/spi0/register
* if it does not work: 
  echo "cs=1" > /sys/bus/spi_root/spi0/register
* and run again slightly modified: 
  echo "cs=1:...." > /sys/bus/spi_root/spi0/register

than:
* install dt compiler with 
* create that file (dt) and modify the settings
* run the compiler
* copy the compiled DT-overlay to /lib/firmware
* echo "..." > /sys/devices/bone_capemgr.*/slots
* reboot if it does not work...
* run steps 2 to 5 again and see if it works...

SPIDEV is for people wanting to do rapid development.

The register part + kernel-driver is the next step up in complexity, 
where spidev is not fast enough for some reason, or does not provide
for all the needs (shared access,...).

The complex clock example of yours is something that would fall in the 
DT-overlay case - these things are most likely too complicated for 
the simple user and requires extensive knowledge of the design.
Those people tend to know how to build a kernel and compile DTs.

Different tools for different level of complexities.

> You listed only ARM boards, that are not supposed to be non-DT. RPi is
> using DT fine, just like the beaglebones. I don't really see why we
> should care about !DT cases.
I listed those ARM boards because I have most experience with them.

I do not know how those INTEL Galileo boards work, but I doubt 
they run DT - ACPI more likely.
But people still want to do rapid development on those using existing
arduino shields, so the DT-overlay requirement would cut them off.

Same for future ARM64 devices where there seems to be a push by 
vendors to move to ACPI for those systems - not DT.

Other examples are the WIFI router, most of which also have SPI 
interfaces and people have done all sorts of things with them - not 
all run ARM either - at least all those WIFI routers I have seen
so far do not run DT-kernels...

So there is more to it than just ARM and we should be able to handle
the "easy" cases for all systems independent from how we configure 
the rest of the system.

Also the proposed setting is along the same lines as GPIO management
via /sys/class/gpio.

Following your argument we should not have this interface either
but use DT for these GPIOs as well - similar argument for I2C...

Still we have them and they are heavily used by those dev-boards,
so why not something along the same lines also for SPI?

Ciao,
	Martin--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

On Tue, Apr 29, 2014 at 11:31:49PM +0200, Martin Sperl wrote:
> On 29.04.2014, at 20:37, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> > On Mon, Apr 28, 2014 at 10:22:54AM -0700, Maxime Ripard wrote:
> > 
> >> spidev device registration has always been a controversial subject since the
> >> move to DT.
> > 
> > Why can we not handle this by using sysfs to bind spidev to the device?
> 
> 
> 
> I think the question is actually more generic than just "spidev" related.
> 
> A solution should also help all those users of development boards like 
> raspberry pi, beagle board,... where people want to add spi devices without
> having to know too much about the fine details of the kernel configuration
> /kernel compiling/creating a new device tree or similar.... 
> They just want their device to work with the existing kernel 
> with minimal effort!
> 
> For just this purpose I got an out of tree module called spi-config that 
> - as of now - allows to define the binding of spi devices (not solely 
> focused on spidev) as per module argument while loading the driver.
> 
> This could easily get extended to work also via sysfs.
> 
> The problem here is mostly around the "passing" of driver specific platform 
> data, which this module currently handles in a sort of "binary blob" way, 
> which is suboptimal (as it is very architecture specific),
> but at least it works...
> 
> Some examples of what is there:
> 
> Bind spidev to spi0.1 with a speed of 2MHz and SPI mode 3:
> modprobe spi-config bus=0:cs=1:modalias=spidev:speed=2000000:mode=3
> 
> Bind an mcp2515 CAN-bus controller to spi0.0 with 10MHz speed and 
> IRQ triggered via GPIO25:
> modprobe spi-config bus=0:cs=0:modalias=mcp2515:speed=10000000:gpioirq=25:\
> mode=0:pd=20:pdu32-0=16000000:pdu32-4=0x2002
> 
> The extra "pd...=" args are the ones describing platform data that is needed
> by the mcp251x driver to set up the device correctly. 
> In the above example we define:
> * pd=20: the platform data size is 20 bytes (zero filled)
> * pdu32-0: a u32 at offset 0 with a value of 16MHz Quarz speed
> * pdu32-4: a u32 at offset 4 the interrupt flags used by the driver
>            (=IRQF_TRIGGER_FALLING|IRQF_ONESHOT)
> 
> The above mcp2515 device now configured via sysfs could look like this:
> echo "cs=0:modalias=mcp2515:speed=10000000:gpioirq=25:mode=0:\
> pd=20:pdu32-0=16000000:pdu32-4=0x2002" > /sys/class/spi_master/spi0/register
> 
> Unregistering the device:
> echo "cs=0" > /sys/class/spi_master/spi0/unregister
> 
> As said: the trickiest part is "platform data", all the "spi-parameters"
> are well-defined and are easily parseable.
> 
> Options that come to my mind to increase "readability" are:
> * "const char *extra_parameters" 
>   in spi_device containing all "unhandled" parameters left for the driver
>   to parse during probing.
> * "int (*config)(struct spi_device *,const char *key,const char *value)"
>   in spi_driver that sets the corresponding parameters
>   (creating platform data if it is not already set/allocated) 
>   and which is called prior to calling spi_driver->probe(spi)
> 
> So with those "readability" options the sysfs interface could 
> look like this:
> echo "cs=0:modalias=mcp2515:speed=10000000:gpioirq=25:mode=0:\
> can_oscillator_hz=16000000:interrupt_flags=0x2002" \
> > /sys/class/spi_master/spi0/register
> 
> When using the second option the framework could fall back to the
> voodoo-"pd...=" approach that I have show above to avoid having to touch 
> all spi drivers immediately to make use of this interface.
> 
> The biggest pitfall I see is that if a device driver expects platform
> data but none is provided, then loading the driver could crash the kernel.
> But that is something that probably should get fixed in the driver 
> itself and not in the framework.
> 
> A totally different approach could be the use of device-tree overlays, 
> which is also not included in the kernel, but would allow for similar
> config schemes.
> But that would leave systems not using device tree without this option.
> 
> So it seems as if we need to find an approach that is acceptable...

I'd really be in favor of using the DT overlays for this. All this is
nice, but very fragile. You can't really handle any weird
configurations that you might encounter, like weird interrupt
controllers that might be requiring more informations, or you don't
set any interrupt parents, and this parameter list will basically be
an ever-growing list of parameters to deal with all kind of crazy
corner-cases, and I'm not sure it's something we want.

You listed only ARM boards, that are not supposed to be non-DT. RPi is
using DT fine, just like the beaglebones. I don't really see why we
should care about !DT cases.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

Hi Mark

On Tue, Apr 29, 2014 at 11:37:58AM -0700, Mark Brown wrote:
> On Mon, Apr 28, 2014 at 10:22:54AM -0700, Maxime Ripard wrote:
> 
> > spidev device registration has always been a controversial subject
> > since the move to DT.
> 
> Why can we not handle this by using sysfs to bind spidev to the
> device?

I just tried it, and apparently, you can't really use this, since spi
devices are created from the device tree (or ACPI) whenever the master
registers.

As such, you still need a DT node for every spidev device you might
want to use.

It doesn't really work either for a device that would be bound to a
driver, that you unbind, and then try to bind to spidev instead. It
looks like the device is released whenever you unbind it, so you can't
really use it afterwards.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

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

Hi Mark

On Tue, Apr 29, 2014 at 11:37:58AM -0700, Mark Brown wrote:
> On Mon, Apr 28, 2014 at 10:22:54AM -0700, Maxime Ripard wrote:
> 
> > spidev device registration has always been a controversial subject
> > since the move to DT.
> 
> Why can we not handle this by using sysfs to bind spidev to the
> device?

I just tried it, and apparently, you can't really use this, since spi
devices are created from the device tree (or ACPI) whenever the master
registers.

As such, you still need a DT node for every spidev device you might
want to use.

It doesn't really work either for a device that would be bound to a
driver, that you unbind, and then try to bind to spidev instead. It
looks like the device is released whenever you unbind it, so you can't
really use it afterwards.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Martin Sperl]

On 29.04.2014, at 20:37, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Mon, Apr 28, 2014 at 10:22:54AM -0700, Maxime Ripard wrote:
> 
>> spidev device registration has always been a controversial subject since the
>> move to DT.
> 
> Why can we not handle this by using sysfs to bind spidev to the device?



I think the question is actually more generic than just "spidev" related.

A solution should also help all those users of development boards like 
raspberry pi, beagle board,... where people want to add spi devices without
having to know too much about the fine details of the kernel configuration
/kernel compiling/creating a new device tree or similar.... 
They just want their device to work with the existing kernel 
with minimal effort!

For just this purpose I got an out of tree module called spi-config that 
- as of now - allows to define the binding of spi devices (not solely 
focused on spidev) as per module argument while loading the driver.

This could easily get extended to work also via sysfs.

The problem here is mostly around the "passing" of driver specific platform 
data, which this module currently handles in a sort of "binary blob" way, 
which is suboptimal (as it is very architecture specific),
but at least it works...

Some examples of what is there:

Bind spidev to spi0.1 with a speed of 2MHz and SPI mode 3:
modprobe spi-config bus=0:cs=1:modalias=spidev:speed=2000000:mode=3

Bind an mcp2515 CAN-bus controller to spi0.0 with 10MHz speed and 
IRQ triggered via GPIO25:
modprobe spi-config bus=0:cs=0:modalias=mcp2515:speed=10000000:gpioirq=25:\
mode=0:pd=20:pdu32-0=16000000:pdu32-4=0x2002

The extra "pd...=" args are the ones describing platform data that is needed
by the mcp251x driver to set up the device correctly. 
In the above example we define:
* pd=20: the platform data size is 20 bytes (zero filled)
* pdu32-0: a u32 at offset 0 with a value of 16MHz Quarz speed
* pdu32-4: a u32 at offset 4 the interrupt flags used by the driver
           (=IRQF_TRIGGER_FALLING|IRQF_ONESHOT)

The above mcp2515 device now configured via sysfs could look like this:
echo "cs=0:modalias=mcp2515:speed=10000000:gpioirq=25:mode=0:\
pd=20:pdu32-0=16000000:pdu32-4=0x2002" > /sys/class/spi_master/spi0/register

Unregistering the device:
echo "cs=0" > /sys/class/spi_master/spi0/unregister

As said: the trickiest part is "platform data", all the "spi-parameters"
are well-defined and are easily parseable.

Options that come to my mind to increase "readability" are:
* "const char *extra_parameters" 
  in spi_device containing all "unhandled" parameters left for the driver
  to parse during probing.
* "int (*config)(struct spi_device *,const char *key,const char *value)"
  in spi_driver that sets the corresponding parameters
  (creating platform data if it is not already set/allocated) 
  and which is called prior to calling spi_driver->probe(spi)

So with those "readability" options the sysfs interface could 
look like this:
echo "cs=0:modalias=mcp2515:speed=10000000:gpioirq=25:mode=0:\
can_oscillator_hz=16000000:interrupt_flags=0x2002" \
> /sys/class/spi_master/spi0/register

When using the second option the framework could fall back to the
voodoo-"pd...=" approach that I have show above to avoid having to touch 
all spi drivers immediately to make use of this interface.

The biggest pitfall I see is that if a device driver expects platform
data but none is provided, then loading the driver could crash the kernel.
But that is something that probably should get fixed in the driver 
itself and not in the framework.

A totally different approach could be the use of device-tree overlays, 
which is also not included in the kernel, but would allow for similar
config schemes.
But that would leave systems not using device tree without this option.

So it seems as if we need to find an approach that is acceptable...

Ciao,
	Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Mon, Apr 28, 2014 at 10:22:54AM -0700, Maxime Ripard wrote:

> spidev device registration has always been a controversial subject since the
> move to DT.

Why can we not handle this by using sysfs to bind spidev to the device?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: Force the registration of the spidev devices

[Mark Brown]

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

On Mon, Apr 28, 2014 at 10:22:54AM -0700, Maxime Ripard wrote:

> spidev device registration has always been a controversial subject since the
> move to DT.

Why can we not handle this by using sysfs to bind spidev to the device?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

spidev device registration has always been a controversial subject since the
move to DT.

Obviously, a spidev node has nothing to do in the DT, and the position so far
has been to add the compatible of the devices to drive through spidev to the
list of the compatibles spidev can handle.

While this is nicer than the DT solution because of its accurate hardware
representation, it's still not perfect because you might not have access to the
DT, or you might be driving a completely generic device (such as a
microcontroller) that might be used for something else in a different
context/board.

Solve this by registering automatically spidev devices for all the unused chip
selects when a master registers itself against the spi core.

This also adds an i2cdev-like feeling, where you get all the spidev devices all
the time, without any modification.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/spi/spi.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4eb9bf02996c..e832a5e20e8d 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1402,6 +1402,52 @@ static void acpi_register_spi_devices(struct spi_master *master)
 static inline void acpi_register_spi_devices(struct spi_master *master) {}
 #endif /* CONFIG_ACPI */
 
+#ifdef CONFIG_SPI_SPIDEV
+static void spidev_register_devices(struct spi_master *master)
+{
+	struct spi_device *spi;
+	int i, status;
+
+	for (i = 0; i < master->num_chipselect; i++) {
+		spi = spi_alloc_device(master);
+		if (!spi) {
+			dev_err(&master->dev, "Couldn't allocate spidev device\n");
+			continue;
+		}
+
+		spi->chip_select = i;
+		strlcpy(spi->modalias, "spidev", sizeof(spi->modalias));
+
+		/*
+		 * This is far from perfect since an addition might be
+		 * done between here and the call to spi_add_device,
+		 * but we can't hold the lock and call spi_add_device
+		 * either, as it would trigger a deadlock.
+		 *
+		 * If such a race occurs, spi_add_device will still
+		 * catch it though, as it also checks for devices
+		 * being registered several times on the same chip
+		 * select.
+		*/
+		status = bus_for_each_dev(&spi_bus_type, NULL, spi,
+					  spi_dev_check);
+		if (status) {
+			dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
+			spi_dev_put(spi);
+			continue;
+		}
+
+		if (spi_add_device(spi)) {
+			dev_err(&master->dev, "Couldn't add spidev device\n");
+			spi_dev_put(spi);
+		}
+	}
+
+}
+#else
+static inline void spidev_register_devices(struct spi_master *master) {}
+#endif /* CONFIG_SPI_SPIDEV */
+
 static void spi_master_release(struct device *dev)
 {
 	struct spi_master *master;
@@ -1591,6 +1637,7 @@ int spi_register_master(struct spi_master *master)
 	/* Register devices from the device tree and ACPI */
 	of_register_spi_devices(master);
 	acpi_register_spi_devices(master);
+	spidev_register_devices(master);
 done:
 	return status;
 }
-- 
1.9.1

[PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard]

spidev device registration has always been a controversial subject since the
move to DT.

Obviously, a spidev node has nothing to do in the DT, and the position so far
has been to add the compatible of the devices to drive through spidev to the
list of the compatibles spidev can handle.

While this is nicer than the DT solution because of its accurate hardware
representation, it's still not perfect because you might not have access to the
DT, or you might be driving a completely generic device (such as a
microcontroller) that might be used for something else in a different
context/board.

Solve this by registering automatically spidev devices for all the unused chip
selects when a master registers itself against the spi core.

This also adds an i2cdev-like feeling, where you get all the spidev devices all
the time, without any modification.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/spi/spi.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4eb9bf02996c..e832a5e20e8d 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1402,6 +1402,52 @@ static void acpi_register_spi_devices(struct spi_master *master)
 static inline void acpi_register_spi_devices(struct spi_master *master) {}
 #endif /* CONFIG_ACPI */
 
+#ifdef CONFIG_SPI_SPIDEV
+static void spidev_register_devices(struct spi_master *master)
+{
+	struct spi_device *spi;
+	int i, status;
+
+	for (i = 0; i < master->num_chipselect; i++) {
+		spi = spi_alloc_device(master);
+		if (!spi) {
+			dev_err(&master->dev, "Couldn't allocate spidev device\n");
+			continue;
+		}
+
+		spi->chip_select = i;
+		strlcpy(spi->modalias, "spidev", sizeof(spi->modalias));
+
+		/*
+		 * This is far from perfect since an addition might be
+		 * done between here and the call to spi_add_device,
+		 * but we can't hold the lock and call spi_add_device
+		 * either, as it would trigger a deadlock.
+		 *
+		 * If such a race occurs, spi_add_device will still
+		 * catch it though, as it also checks for devices
+		 * being registered several times on the same chip
+		 * select.
+		*/
+		status = bus_for_each_dev(&spi_bus_type, NULL, spi,
+					  spi_dev_check);
+		if (status) {
+			dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
+			spi_dev_put(spi);
+			continue;
+		}
+
+		if (spi_add_device(spi)) {
+			dev_err(&master->dev, "Couldn't add spidev device\n");
+			spi_dev_put(spi);
+		}
+	}
+
+}
+#else
+static inline void spidev_register_devices(struct spi_master *master) {}
+#endif /* CONFIG_SPI_SPIDEV */
+
 static void spi_master_release(struct device *dev)
 {
 	struct spi_master *master;
@@ -1591,6 +1637,7 @@ int spi_register_master(struct spi_master *master)
 	/* Register devices from the device tree and ACPI */
 	of_register_spi_devices(master);
 	acpi_register_spi_devices(master);
+	spidev_register_devices(master);
 done:
 	return status;
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html