| Message ID | 20180920191759.17781-1-tpiepho@impinj.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | device tree spidev solution - driver_override for SPI | expand |
Hi Trent, On Thu, Sep 20, 2018 at 07:18:27PM +0000, Trent Piepho wrote: > This is an attempt to solve the problem of binding spidev to a device on > a device-tree system. > ... Tested-by: Kyle Roeschley <kyle.roeschley@ni.com> I tried this patch series on a Xilinx Zynq Z-7020 based system running kernel 4.14 with your patches (which applied cleanly). Our device tree previously listed spidev in its compatible strings. With just this change, I see the warnings prints as expected. When I update my compatible strings to something new and create a udev rule to write to driver_override, the warnings no longer show up. Thanks very much for addressing this issue!
On čtvrtek 20. září 2018 21:18:27 CEST, Trent Piepho wrote: > Trent Piepho (3): > spi: Add driver_override SPI device attribute > spi: Automatically attempt attach after driver_override alteration > spi: spidev: Fix OF tree warning logic Hello, I'm rebasing our internal kernels against 5.0, and I noticed that this patch series does not appear to be merged yet, and they are also not in any SPI subtree as far as I can see. I checked the mailing list, and I saw that the patches were reviewed by Geert and me. Are they blocked on something I missed? With kind regards, Jan
On Tue, 2019-03-05 at 12:15 +0100, Jan Kundrát wrote: > On čtvrtek 20. září 2018 21:18:27 CEST, Trent Piepho wrote: > > Trent Piepho (3): > > spi: Add driver_override SPI device attribute > > spi: Automatically attempt attach after driver_override alteration > > spi: spidev: Fix OF tree warning logic > > Hello, I'm rebasing our internal kernels against 5.0, and I noticed that > this patch series does not appear to be merged yet, and they are also not > in any SPI subtree as far as I can see. I checked the mailing list, and I > saw that the patches were reviewed by Geert and me. Are they blocked on > something I missed? The first and last are in, 5039563e7c25 and 605b3bec73cb. The second isn't. It's not strictly necessary to use it. But it does make the udev rules a lot smaller and I think makes the system easier to use. Mark quite rightly pointed out that the other driver_overrides didn't automatically attach and they should all work the same way. I made patches to convert them all to automatic, but then it fell off the radar and I never finished with it.
On úterý 5. března 2019 19:39:03 CET, Trent Piepho wrote: > On Tue, 2019-03-05 at 12:15 +0100, Jan Kundrát wrote: >> On čtvrtek 20. září 2018 21:18:27 CEST, Trent Piepho wrote: ... > > The first and last are in, 5039563e7c25 and 605b3bec73cb. The second > isn't. It's not strictly necessary to use it. But it does make the > udev rules a lot smaller and I think makes the system easier to use. Indeed :). Do you have an example on how this explicit attach from userspace looks like? I'm already setting ATTR{driver_override} from udev, but that's not enough without your second patch. Should I perhaps poke in sysfs, somehow? > Mark quite rightly pointed out that the other driver_overrides didn't > automatically attach and they should all work the same way. I made > patches to convert them all to automatic, but then it fell off the > radar and I never finished with it. If you ask me, that patch is rather simple and it enables a straightforward way of exporting devices to userspace. From my point of view, this is a desirable property. With kind regards, Jan
On Wed, 2019-03-06 at 14:01 +0100, Jan Kundrát wrote: > On úterý 5. března 2019 19:39:03 CET, Trent Piepho wrote: > > On Tue, 2019-03-05 at 12:15 +0100, Jan Kundrát wrote: > > > On čtvrtek 20. září 2018 21:18:27 CEST, Trent Piepho wrote: ... > > > > The first and last are in, 5039563e7c25 and 605b3bec73cb. The second > > isn't. It's not strictly necessary to use it. But it does make the > > udev rules a lot smaller and I think makes the system easier to use. > > Indeed :). Do you have an example on how this explicit attach from > userspace looks like? I'm already setting ATTR{driver_override} from udev, > but that's not enough without your second patch. Should I perhaps poke in > sysfs, somehow? What needs to happen is to write the device name to the spidev driver's bind attribute to manually attach. Because of driver_override, this will work, rather than fail as it does without. Unfortunately, "bind" is an attribute of the driver while udev is responding to a uevent for the device. So there's no handy ATTR{bind} to use. I don't recall the exact udev magic, but I did verify it can be done. I think it needed udev to run a command, something like: RUN+="echo %k > %S/%p/subsystem/drivers/spidev/bind" > > > Mark quite rightly pointed out that the other driver_overrides didn't > > automatically attach and they should all work the same way. I made > > patches to convert them all to automatic, but then it fell off the > > radar and I never finished with it. > > If you ask me, that patch is rather simple and it enables a straightforward > way of exporting devices to userspace. From my point of view, this is a > desirable property. Yes, I liked it too, that's why I did it that way. But it seems like all the driver_overrides should work the same way and it's not clear to my trying to change them would be accepted.
Hi Jan, On Wed, Mar 6, 2019 at 2:01 PM Jan Kundrát <jan.kundrat@cesnet.cz> wrote: > On úterý 5. března 2019 19:39:03 CET, Trent Piepho wrote: > > On Tue, 2019-03-05 at 12:15 +0100, Jan Kundrát wrote: > >> On čtvrtek 20. září 2018 21:18:27 CEST, Trent Piepho wrote: ... > > > > The first and last are in, 5039563e7c25 and 605b3bec73cb. The second > > isn't. It's not strictly necessary to use it. But it does make the > > udev rules a lot smaller and I think makes the system easier to use. > > Indeed :). Do you have an example on how this explicit attach from > userspace looks like? I'm already setting ATTR{driver_override} from udev, > but that's not enough without your second patch. Should I perhaps poke in > sysfs, somehow? > > > Mark quite rightly pointed out that the other driver_overrides didn't > > automatically attach and they should all work the same way. I made > > patches to convert them all to automatic, but then it fell off the > > radar and I never finished with it. > > If you ask me, that patch is rather simple and it enables a straightforward > way of exporting devices to userspace. From my point of view, this is a > desirable property. I don't have access to an appropriate system right now, but it should be something like: Override $dev to spidev: echo spidev > /sys/bus/spi/devices/$dev/driver_override Bind $dev to spidev: echo $dev > /sys/bus/spi/drivers/spidev/bind Good luck! Gr{oetje,eeting}s, Geert
> What needs to happen is to write the device name to the spidev driver's > bind attribute to manually attach. Because of driver_override, this > will work, rather than fail as it does without. > > Unfortunately, "bind" is an attribute of the driver while udev is > responding to a uevent for the device. So there's no handy ATTR{bind} > to use. > > I don't recall the exact udev magic, but I did verify it can be done. > I think it needed udev to run a command, something like: > RUN+="echo %k > %S/%p/subsystem/drivers/spidev/bind" Thanks. It turned out to be a bit more complicated because attributes set by ATTR{driver_override}+="spidev" only take effect after the rule has been processed, apparently. It's quite easy to hack around this. Here's a complete rule which works for me: # cannot use ATTR{driver_override}+="spidev" because it apparently only runs after the PROGRAM ACTION=="add|change", SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:my-device-from-device-tree", PROGRAM+="/bin/sh -c 'echo spidev > %S%p/driver_override && echo %k > %S%p/subsystem/drivers/spidev/bind'" Thanks for pointing me in the right direction, Trent and Geert. With kind regards, Jan
This is an attempt to solve the problem of binding spidev to a device on a device-tree system. Explicitly listing spidev in the "compatible" property of a device is not allowed, as it not type of the device but rather a Linux specific driver name. The current solution is to list every possible device spidev can be bound to, but this is highly unsatisfactory. 1. Needing to patch and compile the kernel is a significant barrier to many who could otherwise make use of embedded dev kits, such as RPi, that have SPI hardware. 2. Up-streaming a new spi device ID is a lengthy undertaking even to those who know how. This causes unneeded delay in upstream kernel support for a new device. 3. Maintaining a list of obscure and one-off device IDs in the spidev driver is largely a waste of effort to nearly every developer to use the driver. 4. It's not possible to bind a device that has a kernel SPI driver to spidev instead of its normal driver. This is quite useful to debug hardware problems, development of the kernel driver, and testing SPI bus timing, impedance, etc. It can also be used to access device features, like a one time programming step, that the device's kernel driver does not necessary need to support. This approach should solve these problems. Three existing Linux busses - AMBA, PCI, and platform - give device's a "driver_override" attribute, which can be used to bind them to a driver without requiring a match against the driver's device ID table. This can be used to bind spidev to a device without needing to add the device to some list inside the spidev driver. Udev rules can be written to make this automatic, example: ACTION=="add|change",, SUBSYSTEM=="spi", ENV{MODALIAS}=="spi:adrf6820", ATTR{driver_override}+="spidev" This will automatically bind a device that claims (likely via device-tree compatible value) to be an "adrf6820" to spidev. Additional rules could be imagined to bind specific spi bus chip selects, or any device on a certain bus, or via the device's node name in the device tree, to spidev. Changes from v1: - Fix leak on device release with override set - Simplifly spidev warn logic and remove dev_err - Split automatic attach attempt into another commit Trent Piepho (3): spi: Add driver_override SPI device attribute spi: Automatically attempt attach after driver_override alteration spi: spidev: Fix OF tree warning logic drivers/spi/spi.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ drivers/spi/spidev.c | 8 +++---- include/linux/spi/spi.h | 1 + 3 files changed, 62 insertions(+), 5 deletions(-)