| Message ID | 1398705774-12361-1-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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?
On 29.04.2014, at 20:37, Mark Brown <broonie@kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
On Tue, Apr 29, 2014 at 11:31:49PM +0200, Martin Sperl wrote: > On 29.04.2014, at 20:37, Mark Brown <broonie@kernel.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
On 30.04.2014, at 20:14, Maxime Ripard <maxime.ripard@free-electrons.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 30, 2014 at 10:00:18PM +0200, Martin Sperl wrote: > > On 30.04.2014, at 20:14, Maxime Ripard <maxime.ripard@free-electrons.com> 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.
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.
On Wed, Apr 30, 2014 at 10:00:18PM +0200, Martin Sperl wrote: > > On 30.04.2014, at 20:14, Maxime Ripard <maxime.ripard@free-electrons.com> 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.
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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?
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
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; }
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(+)