Add spidev nodes for SPI controllers

Documentation/devicetree/bindings/trivial-devices.yaml properties: · line 297

Quoted 2 lines · Vishwaroop A

Documentation/devicetree/bindings/trivial-devices.yaml +2 -0

+            # NVIDIA Tegra SPIDEV Controller device
+          - nvidia,tegra-spidev

All the issues with having spidev nodes directly in the DT also apply if you add a prefix onto it. Whatever is attached to the SPI controller needs to be described, not just a placeholder like this.

On the Tegra Jetson boards we have a 40-pin expansion header similar to what is found on boards like Raspberry Pi and allows users to connect various cards to. By having a pseudo device we can interact with different SPI devices.

Yes by default nothing is connected and so there is nothing to talk to. However, this does enable us to do SPI loopback testing for example.

So I am wondering if it would be acceptable to having some generic dummy device-tree compatible string for this? I guess it does not need to be Tegra specific.

Quoted 11 lines · Jon Hunter

On the Tegra Jetson boards we have a 40-pin expansion header similar to what is found on boards like Raspberry Pi and allows users to connect various cards to. By having a pseudo device we can interact with different SPI devices.

Yes by default nothing is connected and so there is nothing to talk to. However, this does enable us to do SPI loopback testing for example.

So I am wondering if it would be acceptable to having some generic dummy device-tree compatible string for this? I guess it does not need to be Tegra specific.

I understand what he's trying to accomplish, it's the same thing as what everyone who wants to put a raw spidev compatible in their DT is trying to do. The way to do this would be something like a DT overlay that describes whatever is actually connected, or just customise the DT locally.

Quoted 5 lines · Mark Brown

I understand what he's trying to accomplish, it's the same thing as what everyone who wants to put a raw spidev compatible in their DT is trying to do. The way to do this would be something like a DT overlay that describes whatever is actually connected, or just customise the DT locally.

We could certainly use an overlay, but how do we handle the kernel side? My understanding is that per patch 3/3 we need to reference a compatible string the kernel is aware of. I guess we could use an existing one, but feels like a massive hack. It would be nice if there is something generic we can use for this like 'linux,spidev'.

I see that ACPI has something and it does print a warning that this should not be used in production systems.

Quoted 8 lines · Jon Hunter

Quoted 5 lines · Mark Brown

I understand what he's trying to accomplish, it's the same thing as what everyone who wants to put a raw spidev compatible in their DT is trying to do. The way to do this would be something like a DT overlay that describes whatever is actually connected, or just customise the DT locally.

Quoted 8 lines

We could certainly use an overlay, but how do we handle the kernel side? My understanding is that per patch 3/3 we need to reference a compatible string the kernel is aware of. I guess we could use an existing one, but feels like a massive hack. It would be nice if there is something generic we can use for this like 'linux,spidev'.

I see that ACPI has something and it does print a warning that this should not be used in production systems.

You can put 'spidev' in as the compatible and get the warning, we don't require specific compatibles if the Linux device ID is good enough. If you genuinely just have bare wires you're probably able to cope with the warning. If something is actually connected you should use the compatible for whatever that is, if spidev makes sense for it then that'd be OK to add to spidev.

Quoted 6 lines · Mark Brown

You can put 'spidev' in as the compatible and get the warning, we don't require specific compatibles if the Linux device ID is good enough. If you genuinely just have bare wires you're probably able to cope with the warning. If something is actually connected you should use the compatible for whatever that is, if spidev makes sense for it then that'd be OK to add to spidev.

We finally got back to this. Looks like just having 'spidev' as the compatible does not work. Apparently, it use to work and yes you would get the warning, but that no longer seems to be the case. I see a few others have been doing similar things and hacking their device-trees in different ways [0].

I completely agree that ideally we would have a proper compatible string for this because after all device-tree describes the hardware. One use-case that we use is external loop back for verifying SPI by simply connecting MOSI to the MISO. Would it be acceptable to have a compatible string for external loopback connections?

Quoted 8 lines · Jon Hunter

Quoted 6 lines · Mark Brown

You can put 'spidev' in as the compatible and get the warning, we don't require specific compatibles if the Linux device ID is good enough. If you genuinely just have bare wires you're probably able to cope with the warning. If something is actually connected you should use the compatible for whatever that is, if spidev makes sense for it then that'd be OK to add to spidev.

Quoted 5 lines

We finally got back to this. Looks like just having 'spidev' as the compatible does not work. Apparently, it use to work and yes you would get the warning, but that no longer seems to be the case. I see a few others have been doing similar things and hacking their device-trees in different ways [0].

Huh, OK. I don't recall any deliberate SPI change for that.

Quoted 10 lines · Mark Brown

Quoted 8 lines · Jon Hunter

Quoted 6 lines · Mark Brown

You can put 'spidev' in as the compatible and get the warning, we don't require specific compatibles if the Linux device ID is good enough. If you genuinely just have bare wires you're probably able to cope with the warning. If something is actually connected you should use the compatible for whatever that is, if spidev makes sense for it then that'd be OK to add to spidev.

Quoted 7 lines

Quoted 5 lines

We finally got back to this. Looks like just having 'spidev' as the compatible does not work. Apparently, it use to work and yes you would get the warning, but that no longer seems to be the case. I see a few others have been doing similar things and hacking their device-trees in different ways [0].

Huh, OK. I don't recall any deliberate SPI change for that.

People in the discussion that Jon linked to indicated that it was this patch that caused the "regression":

commit 6840615f85f6046039ebc4989870ddb12892b7fc

Author: Mark Brown <[email protected]>
Date:   Thu Sep 23 18:00:23 2021 +0100

spi: spidev: Add SPI ID table

Currently autoloading for SPI devices does not use the DT ID table, it uses SPI modalises. Supporting OF modalises is going to be difficult if not impractical, an attempt was made but has been reverted, so ensure that module autoloading works for this driver by adding an id_table listing the SPI IDs for everything.

Fixes 96c8395e2166 ("spi: Revert modalias changes")

Signed-off-by Mark Brown <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Mark Brown <[email protected]>

If you say that the regression wasn't deliberate, maybe we should look at fixing this so that people don't have to work around stuff?

Mark, would another alternative be to add something like a sysfs export attribute? Something that you'd write a controller/chipselect pair to in order to create a spidev device? That has the benefit of removing this entirely from device tree where it doesn't belong, but still makes this option available to users that would otherwise have to resort to hacks.

Quoted 5 lines · Thierry Reding

Mark, would another alternative be to add something like a sysfs export attribute? Something that you'd write a controller/chipselect pair to in order to create a spidev device? That has the benefit of removing this entirely from device tree where it doesn't belong, but still makes this option available to users that would otherwise have to resort to hacks.

Possibly? I think I've lost track of what the use case is here, usually for the spidev stuff DT overlays seem like they're the right thing but perhaps this is different? If we are doing this at runtime sysfs seems like a reasonable way to trigger it, though you'd still need the DT to describe the controller and the chipselects that are available.

Quoted 5 lines · Mark Brown

Possibly? I think I've lost track of what the use case is here, usually for the spidev stuff DT overlays seem like they're the right thing but perhaps this is different? If we are doing this at runtime sysfs seems like a reasonable way to trigger it, though you'd still need the DT to describe the controller and the chipselects that are available.

Heh... it's exactly the opposite for me. I feel like I don't understand the need for spidev with a specific compatible string. If you've got a compatible string (or in other words you have a device with a very specific SPI chip connected), then why would you want to access it from userspace? Isn't a proper kernel driver the better option in most cases? That usually allows for better abstraction via some other subsystem. I suppose there are cases where there may not be a subsystem, or for other reasons it's more convenient to have direct access from userspace to avoid the roundtrip. Or maybe users could be concerned about safety?

In other cases users may just want to connect something completely custom or just have a way to poke whatever they connect. There may not be a proper driver for it. Or it could perhaps even be used temporarily as a way to write a userspace driver conveniently before porting it to Linux.

The way I imagine it, exporting would involve writing a chip-select to a specific SPI controller's "export" sysfs attribute to have a SPI device created for that particular chip-select and bind it to spidev.

Quoted 3 lines

The way I imagine it, exporting would involve writing a chip-select to a specific SPI controller's "export" sysfs attribute to have a SPI device created for that particular chip-select and bind it to spidev.

My general feeling with those is that if you're building for them you're probably either already modifiying your kernel or easily able to cope with doing so.

Quoted 7 lines · Mark Brown

Quoted 3 lines

The way I imagine it, exporting would involve writing a chip-select to a specific SPI controller's "export" sysfs attribute to have a SPI device created for that particular chip-select and bind it to spidev.

My general feeling with those is that if you're building for them you're probably either already modifiying your kernel or easily able to cope with doing so.

That's definitely what we do today, modify the kernel directly to achieve what we need. I am trying to avoid carrying too many out of tree patches for stuff like this and have something in the kernel that works by default. This is even more important for 3rd party Linux distros that will not accept non-upstream code.

Our devkits, very much like Raspberry PI, allow users to connect various hardware for development and so having an easy way to connect a SPI device is useful. For any production systems, users will definitely want a proper device and device-tree bindings. So I am just trying to explore what would be acceptable. If it is acceptable to have a sysfs interface for creating a SPI device at runtime, then we can look into that.

Quoted 9 lines · Jon Hunter

Quoted 7 lines · Mark Brown

Quoted 3 lines

The way I imagine it, exporting would involve writing a chip-select to a specific SPI controller's "export" sysfs attribute to have a SPI device created for that particular chip-select and bind it to spidev.

My general feeling with those is that if you're building for them you're probably either already modifiying your kernel or easily able to cope with doing so.

Quoted 5 lines

That's definitely what we do today, modify the kernel directly to achieve what we need. I am trying to avoid carrying too many out of tree patches for stuff like this and have something in the kernel that works by default. This is even more important for 3rd party Linux distros that will not accept non-upstream code.

Overlays should work well for that case too!

Quoted 7 lines · Mark Brown

Quoted 5 lines · Jon Hunter

That's definitely what we do today, modify the kernel directly to achieve what we need. I am trying to avoid carrying too many out of tree patches for stuff like this and have something in the kernel that works by default. This is even more important for 3rd party Linux distros that will not accept non-upstream code.

Quoted 1 line

Overlays should work well for that case too!

If you mean device-tree overlays, I don't see how that will work today. Unless we are to use one of the existing compatible strings, but that feels wrong because we are not using any of those devices and like I mentioned, just using 'spidev' alone in device-tree does not work with the latest kernels.

Quoted 10 lines · Jon Hunter

Quoted 9 lines · Mark Brown

Quoted 5 lines · Jon Hunter

That's definitely what we do today, modify the kernel directly to achieve what we need. I am trying to avoid carrying too many out of tree patches for stuff like this and have something in the kernel that works by default. This is even more important for 3rd party Linux distros that will not accept non-upstream code.

Overlays should work well for that case too!

Quoted 5 lines

If you mean device-tree overlays, I don't see how that will work today. Unless we are to use one of the existing compatible strings, but that feels wrong because we are not using any of those devices and like I mentioned, just using 'spidev' alone in device-tree does not work with the latest kernels.

Why would you need to use a compatible string for anything? I'd expect the overlay to add the entire new device, compatible and all.

Quoted 2 lines · Mark Brown

Why would you need to use a compatible string for anything? I'd expect the overlay to add the entire new device, compatible and all.

We need a compatible string for the SPI device in device-tree. Sorry, but I am not sure how we can add a SPI device without one.

Can you confirm what you mean by 'overlay'? To me an overlay, is purely a device-tree overlay and even if we have a device-tree overlay, we are still missing the kernel driver part that matches the compatible string.

Quoted 9 lines · Mark Brown

Quoted 5 lines · Jon Hunter

We finally got back to this. Looks like just having 'spidev' as the compatible does not work. Apparently, it use to work and yes you would get the warning, but that no longer seems to be the case. I see a few others have been doing similar things and hacking their device-trees in different ways [0].

Huh, OK. I don't recall any deliberate SPI change for that.

People in the discussion that Jon linked to indicated that it was this patch that caused the "regression":

If you say that the regression wasn't deliberate, maybe we should look at fixing this so that people don't have to work around stuff?

Add "tegra-spidev" name string to load spidev driver which allows user-space programs to access and communicate with SPI devices connected to the system.

Signed-off-by Vishwaroop A <[email protected]>

---
 drivers/spi/spidev.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/spi/spidev.c +1 -0

index 653f82984216..1d2c01bd76a9 100644
@@ -711,6 +711,7 @@ static const struct spi_device_id spidev_spi_ids[] = {
 	{ .name = "spi-authenta" },
 	{ .name = "em3581" },
 	{ .name = "si3210" },
+	{ .name = "tegra-spidev" },
 	{},
 };
 MODULE_DEVICE_TABLE(spi, spidev_spi_ids);