Skip to contentSkip to conversation

Spidev usability patchset update

PATCH In review Michal Suchanek 3 participants 16 messages Jul 18, 2016 – Jul 30, 2016 SPI

lore
Share & tools
lore

Michal Suchanek

author
#0 lore raw

v3 cover Spidev usability patchset update

Hello,

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

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

Constructive comments on improving this patchset are welcome.

Thanks

Michal

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#1 lore raw

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

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

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

Signed-off-by Michal Suchanek <[email protected]>
---
 Documentation/spi/spidev |  40 ++---
 drivers/spi/Kconfig      |   2 +-
 drivers/spi/spi.c        |  19 ++-
 drivers/spi/spidev.c     | 370 ++++++++++++++++++++---------------------------
 include/linux/spi/spi.h  |  14 ++
 5 files changed, 197 insertions(+), 248 deletions(-)
Documentation/spi/spidev +11 -29
index 3d14035..dd8eac2 100644
@@ -21,23 +21,14 @@ they need to access kernel interfaces (such as IRQ handlers or other layers
 of the driver stack) that are not accessible to userspace.
 
 
-DEVICE CREATION, DRIVER BINDING
-===============================
-The simplest way to arrange to use this driver is to just list it in the
-spi_board_info for a device as the driver it should use:  the "modalias"
-entry is "spidev", matching the name of the driver exposing this API.
-Set up the other device characteristics (bits per word, SPI clocking,
-chipselect polarity, etc) as usual, so you won't always need to override
-them later.
-
-(Sysfs also supports userspace driven binding/unbinding of drivers to
-devices.  That mechanism might be supported here in the future.)
-
-When you do that, the sysfs node for the SPI device will include a child
-device node with a "dev" attribute that will be understood by udev or mdev.
-(Larger systems will have "udev".  Smaller ones may configure "mdev" into
-busybox; it's less featureful, but often enough.)  For a SPI device with
-chipselect C on bus B, you should see:
+DEVICE CREATION
+===============
+
+When spidev is included in the kernel, the sysfs node for the SPI device will
+include a child device node with a "dev" attribute that will be understood by
+udev or mdev.  (Larger systems will have "udev".  Smaller ones may configure
+"mdev" into busybox; it's less featureful, but often enough.)  For a SPI device
+with chipselect C on bus B, you should see:
 
     /dev/spidevB.C ... character special device, major number 153 with
 	a dynamically chosen minor device number.  This is the node

@@ -54,18 +45,9 @@ Do not try to manage the /dev character device special file nodes by hand.
 That's error prone, and you'd need to pay careful attention to system
 security issues; udev/mdev should already be configured securely.
 
-If you unbind the "spidev" driver from that device, those two "spidev" nodes
-(in sysfs and in /dev) should automatically be removed (respectively by the
-kernel and by udev/mdev).  You can unbind by removing the "spidev" driver
-module, which will affect all devices using this driver.  You can also unbind
-by having kernel code remove the SPI device, probably by removing the driver
-for its SPI controller (so its spi_master vanishes).
-
-Since this is a standard Linux device driver -- even though it just happens
-to expose a low level API to userspace -- it can be associated with any number
-of devices at a time.  Just provide one spi_board_info record for each such
-SPI device, and you'll get a /dev device node for each device.
-
+When a device driver is loaded for the SPI device operations on the spidev
+character device return EBUSY. You can unbind a driver from the device using
+the driver unbind file in sysfs.
 
 BASIC CHARACTER DEVICE API
 ==========================
drivers/spi/Kconfig +1 -1
index f2bbb16..3806e3c 100644
@@ -699,7 +699,7 @@ config SPI_ZYNQMP_GQSPI
 comment "SPI Protocol Masters"
 
 config SPI_SPIDEV
-	tristate "User mode SPI device driver support"
+	bool "User mode SPI device driver support"
 	help
 	  This supports user mode SPI protocol drivers.
drivers/spi/spi.c +16 -3
index 77e6e45..f3ea768 100644
@@ -41,6 +41,8 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/spi.h>
 
+#include "spidev.h"
+
 static void spidev_release(struct device *dev)
 {
 	struct spi_device	*spi = to_spi_device(dev);

@@ -544,11 +546,15 @@ int spi_add_device(struct spi_device *spi)
 
 	/* Device may be bound to an active driver when this returns */
 	status = device_add(&spi->dev);
-	if (status < 0)
+	if (status < 0) {
 		dev_err(dev, "can't add %s, status %d\n",
 				dev_name(&spi->dev), status);
-	else
-		dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev));
+		goto done;
+	}
+	dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev));
+
+	if (IS_ENABLED(CONFIG_SPI_SPIDEV))
+		spi_attach_spidev(spi);
 
 done:
 	mutex_unlock(&spi_add_lock);

@@ -622,6 +628,10 @@ void spi_unregister_device(struct spi_device *spi)
 
 	if (spi->dev.of_node)
 		of_node_clear_flag(spi->dev.of_node, OF_POPULATED);
+
+	if (IS_ENABLED(CONFIG_SPI_SPIDEV))
+		spi_detach_spidev(spi);
+
 	device_unregister(&spi->dev);
 }
 EXPORT_SYMBOL_GPL(spi_unregister_device);

@@ -3128,6 +3138,9 @@ static int __init spi_init(void)
 	if (IS_ENABLED(CONFIG_OF_DYNAMIC))
 		WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));
 
+	if (IS_ENABLED(CONFIG_SPI_SPIDEV))
+		WARN_ON(spidev_init());
+
 	return 0;
 
 err2:
drivers/spi/spidev.c +155 -215

670 lines not on this page · raw message

include/linux/spi/spi.h +14 -0
index 1f03483..7154559 100644
@@ -178,6 +178,20 @@ struct spi_device {
 	 *  - chipselect delays
 	 *  - ...
 	 */
+
+	/* spidev stuff */
+#if config_enabled(CONFIG_SPI_SPIDEV)
+	dev_t                   spidev_devt;
+	struct device *		spidev;
+	spinlock_t              spidev_lock;
+
+	/* TX/RX buffers are NULL unless this device is open (users > 0) */
+	struct mutex            buf_lock;
+	unsigned                spidev_users;
+	u8                      *tx_buffer;
+	u8                      *rx_buffer;
+	u32                     spidev_speed_hz;
+#endif /*CONFIG_SPI_SPIDEV*/
 };
 
 static inline struct spi_device *to_spi_device(struct device *dev)
Signature · 1 line
2.8.1
maintainer
#2 lore raw

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

drivers/spi/Kconfig config SPI_ZYNQMP_GQSPI · line 702

Quoted 3 lines · Michal Suchanek

config SPI_SPIDEV

drivers/spi/Kconfig +1 -1
-	tristate "User mode SPI device driver support"
+	bool "User mode SPI device driver support"

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

drivers/spi/spidev.c spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 359

Quoted 5 lines
drivers/spi/spidev.c +2 -3
-	spin_lock_irq(&spidev->spi_lock);
-	spi = spi_dev_get(spidev->spi);
-	spin_unlock_irq(&spidev->spi_lock);
+	spi = filp->private_data;
+	spi = spi_dev_get(spi);

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

Quoted 3 lines
drivers/spi/spidev.c +2 -1
-	mutex_lock(&device_list_lock);
+	dev = bus_find_device(&spi_bus_type, NULL, (void *) inode->i_rdev,
+			      spidev_devt_match);

...

drivers/spi/spidev.c spidev_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 590

Quoted 7 lines
drivers/spi/spidev.c +6 -1
-			dev_dbg(&spidev->spi->dev, "open/ENOMEM\n");
+	spi = to_spi_device(dev);
+
+	mutex_lock(&spi->buf_lock);
+	spin_lock_irq(&spi->spidev_lock);
+	spi->spidev_users++;
+	spin_unlock_irq(&spi->spidev_lock);

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

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

author
#3 lore raw

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

Hello,

Quoted 7 lines · Mark Brown

drivers/spi/Kconfig config SPI_ZYNQMP_GQSPI · line 702

Quoted 3 lines · Michal Suchanek

config SPI_SPIDEV

drivers/spi/Kconfig +1 -1
-     tristate "User mode SPI device driver support"
+     bool "User mode SPI device driver support"

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

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

Quoted 9 lines

drivers/spi/spidev.c spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 359

Quoted 6 lines
drivers/spi/spidev.c +2 -3
-     spin_lock_irq(&spidev->spi_lock);
-     spi = spi_dev_get(spidev->spi);
-     spin_unlock_irq(&spidev->spi_lock);
+     spi = filp->private_data;
+     spi = spi_dev_get(spi);

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

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

Regarding this change

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

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

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

Is there a better way?

Quoted 15 lines
Quoted 4 lines
drivers/spi/spidev.c +2 -1
-     mutex_lock(&device_list_lock);
+     dev = bus_find_device(&spi_bus_type, NULL, (void *) inode->i_rdev,
+                           spidev_devt_match);

...

drivers/spi/spidev.c spidev_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) · line 590

Quoted 7 lines
drivers/spi/spidev.c +6 -1
-                     dev_dbg(&spidev->spi->dev, "open/ENOMEM\n");
+     spi = to_spi_device(dev);
+
+     mutex_lock(&spi->buf_lock);
+     spin_lock_irq(&spi->spidev_lock);
+     spi->spidev_users++;
+     spin_unlock_irq(&spi->spidev_lock);
Quoted 5 lines

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

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

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

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

Quoted 4 lines

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

I am getting fed up with this argument.

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

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

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

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

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

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#4 lore raw

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

Quoted 7 lines · Michal Suchanek

On 19 July 2016 at 00:59, Mark Brown <[email protected]> wrote:

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

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

Quoted 4 lines

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

Is there a better way?

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

Quoted 8 lines
Quoted 3 lines

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

I am getting fed up with this argument.

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

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

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

This:

drivers/spi/spidev.c static const struct file_operations spidev_fops = { · line 761

Quoted 2 lines
drivers/spi/spidev.c +1 -1
-       status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
+       status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);

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

Quoted 4 lines

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

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

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

Quoted 4 lines

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

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

author
#5 lore raw

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

Quoted 9 lines · Mark Brown
Quoted 7 lines · Michal Suchanek

On 19 July 2016 at 00:59, Mark Brown <[email protected]> wrote:

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

Quoted 3 lines

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

And they are not ignored.

Quoted 30 lines
Quoted 5 lines

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

Is there a better way?

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

Quoted 8 lines
Quoted 3 lines

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

I am getting fed up with this argument.

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

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

Quoted 3 lines

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

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

Quoted 7 lines

This:

drivers/spi/spidev.c static const struct file_operations spidev_fops = { · line 761

Quoted 2 lines
drivers/spi/spidev.c +1 -1
-       status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
+       status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);

also looks like a needless ABI change

ABI change to what?

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

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

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

Quoted 3 lines

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

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

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

Quoted 19 lines
Quoted 5 lines

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

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

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

Quoted 8 lines
Quoted 4 lines

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

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

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

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

Thanks

Michal

maintainer
#6 lore raw

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

Quoted 5 lines · Michal Suchanek
Quoted 2 lines · Mark Brown
Quoted 2 lines
drivers/spi/spidev.c +1 -1
-       status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
+       status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);
Quoted 8 lines
Quoted 1 line

also looks like a needless ABI change

ABI change to what?

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

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

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

Quoted 9 lines
Quoted 3 lines

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

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

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

Quoted 5 lines

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

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

author
#7 lore raw

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

Quoted 10 lines · Mark Brown
Quoted 8 lines · Michal Suchanek
Quoted 4 lines · Mark Brown

drivers/spi/spidev.c static const struct file_operations spidev_fops = { · line 761

Quoted 2 lines
drivers/spi/spidev.c +1 -1
-       status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
+       status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);

also looks like a needless ABI change

ABI change to what?

Quoted 7 lines
Quoted 4 lines

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

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

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

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

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

Quoted 11 lines
Quoted 10 lines
Quoted 4 lines

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

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

Quoted 8 lines

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

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

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

Quoted 10 lines
Quoted 6 lines

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

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

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

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

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#8 lore raw

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

Quoted 8 lines · Michal Suchanek
Quoted 2 lines · Mark Brown

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

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

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

Quoted 2 lines

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

Yup.

Quoted 4 lines

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

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

#9 lore raw

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

In reply to Michal Suchanek

Hi,

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

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

All warnings (new ones prefixed by >>):

include/linux/init.h:1: warning: no structured comments found kernel/sched/core.c:2081: warning: No description found for parameter 'cookie' kernel/sys.c:1: warning: no structured comments found drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found

Quoted 8 lines
Quoted 8 lines

include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev_devt' include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev' include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev_lock' include/linux/spi/spi.h:195: warning: No description found for parameter 'buf_lock' include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev_users' include/linux/spi/spi.h:195: warning: No description found for parameter 'tx_buffer' include/linux/spi/spi.h:195: warning: No description found for parameter 'rx_buffer' include/linux/spi/spi.h:195: warning: No description found for parameter 'spidev_speed_hz'

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

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

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

author
#10 lore raw

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

In reply to Michal Suchanek

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

Signed-off-by Michal Suchanek <[email protected]>
---
 drivers/spi/spi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/spi/spi.c +1 -2
index f3ea768..f0de2ec 100644
@@ -1489,9 +1489,8 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc)
 	rc = of_modalias_node(nc, spi->modalias,
 				sizeof(spi->modalias));
 	if (rc < 0) {
-		dev_err(&master->dev, "cannot find modalias for %s\n",
+		dev_warn(&master->dev, "cannot find modalias for %s\n",
 			nc->full_name);
-		goto err_out;
 	}
 
 	/* Device address */
Signature · 1 line
2.8.1
maintainer
#11 lore raw

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

drivers/spi/spi.c of_register_spi_device(struct spi_master *master, struct device_node *nc) · line 1492

Quoted 8 lines · Michal Suchanek

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

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

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

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

author
#12 lore raw

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

Hello,

Quoted 14 lines · Mark Brown

drivers/spi/spi.c of_register_spi_device(struct spi_master *master, struct device_node *nc) · line 1492

Quoted 8 lines · Michal Suchanek

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

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

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

Quoted 6 lines

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

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

Quoted 1 line

From my point of view the conceptual change described in the commit message

is that whenever SPI slave node is encountered in devicetree you get either a device with active driver or a device with no driver whereas previously you either
got a device with active driver or no device. So yes, it's about allowing SPI slave
devices without a driver bound to them.

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

Thanks

Michal

maintainer
#13 lore raw

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

Quoted 6 lines · Michal Suchanek
Quoted 4 lines · Mark Brown
Quoted 2 lines · Michal Suchanek

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

Quoted 6 lines
Quoted 3 lines

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

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

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

Quoted 5 lines

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

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

author
#14 lore raw

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

Quoted 25 lines · Mark Brown
Quoted 6 lines · Michal Suchanek
Quoted 4 lines · Mark Brown
Quoted 2 lines · Michal Suchanek

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

Quoted 6 lines
Quoted 3 lines

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

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

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

Quoted 5 lines

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

Quoted 3 lines

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

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

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#15 lore raw

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

In reply to Michal Suchanek

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

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

Signed-off-by Michal Suchanek <[email protected]>
---
 drivers/spi/spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/spi/spi.c +3 -3
index f0de2ec..08e65e4 100644
@@ -1554,11 +1554,11 @@ of_register_spi_device(struct spi_master *master, struct device_node *nc)
 	/* Device speed */
 	rc = of_property_read_u32(nc, "spi-max-frequency", &value);
 	if (rc) {
-		dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
+		dev_warn(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n",
 			nc->full_name, rc);
-		goto err_out;
+	} else {
+		spi->max_speed_hz = value;
 	}
-	spi->max_speed_hz = value;
 
 	/* Store a pointer to the node in the device structure */
 	of_node_get(nc);
Signature · 1 line
2.8.1