| Message ID | 1431462804-30467-1-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: > While this is nicer than the DT solution because of its accurate hardware > representation, it's still not perfect because you might not have access to the > DT, or you might be driving a completely generic device (such as a > microcontroller) that might be used for something else in a different > context/board. Greg, you're copied on this because this seems to be a generic problem that should perhaps be solved at a driver model level - having a way to bind userspace access to devices that we don't otherwise have a driver for. The subsystem could specify the UIO driver to use when no other driver is available. > Solve this by registering automatically spidev devices for all the unused chip > selects when a master registers itself against the spi core. So, aside from the concern about this being generic the other thing here is that we often have devices offering more chip selects than can physically be used in a system but not doing anything to ensure that the invalid ones can't be used. It's unclear to me if that's OK or not, it might be since it's root only I think but I need to think it through a bit. > This also adds an i2cdev-like feeling, where you get all the spidev devices all > the time, without any modification. I2C is a bit safer here here since it's a shared bus so you can't do anything to devices not connected to the bus by mistake. > + /* > + * This is far from perfect since an addition might be > + * done between here and the call to spi_add_device, > + * but we can't hold the lock and call spi_add_device > + * either, as it would trigger a deadlock. > + * > + * If such a race occurs, spi_add_device will still > + * catch it though, as it also checks for devices > + * being registered several times on the same chip > + * select. > + */ > + status = bus_for_each_dev(&spi_bus_type, NULL, spi, > + spi_dev_check); > + if (status) { > + dev_dbg(&master->dev, "Chipselect already in use.. Skipping."); > + spi_dev_put(spi); > + continue; > + } This still leaves us in the situation where if we do know the device that is connected we have to explicitly bind it in spidev which is apparently unreasonably difficult for people. I'm also concerned about the interactions with DT overlays here - what happens if a DT overlay or other dynamic hardware instantiation comes along later and does bind something to this chip select? It seems like we should be able to combine the two models, and the fact that we only create these devices with a Kconfig option is a bit of an interesting thing here.
On 13 May 2015 at 13:26, Mark Brown <broonie@kernel.org> wrote: > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: > >> While this is nicer than the DT solution because of its accurate hardware >> representation, it's still not perfect because you might not have access to the >> DT, or you might be driving a completely generic device (such as a >> microcontroller) that might be used for something else in a different >> context/board. > > Greg, you're copied on this because this seems to be a generic problem > that should perhaps be solved at a driver model level - having a way to > bind userspace access to devices that we don't otherwise have a driver > for. The subsystem could specify the UIO driver to use when no other > driver is available. > >> Solve this by registering automatically spidev devices for all the unused chip >> selects when a master registers itself against the spi core. > > So, aside from the concern about this being generic the other thing here > is that we often have devices offering more chip selects than can > physically be used in a system but not doing anything to ensure that the > invalid ones can't be used. It's unclear to me if that's OK or not, it > might be since it's root only I think but I need to think it through a > bit. Presumably you could add dt nodes for the chipselects and specify they are disabled but it does not work for me currently. This is mostly a cosmetic issue. The chipselects were always there but now they are visible. Unlike the case when you explicitly bind spidev using dt node the unusable chipselects are also bound. > >> This also adds an i2cdev-like feeling, where you get all the spidev devices all >> the time, without any modification. > > I2C is a bit safer here here since it's a shared bus so you can't do > anything to devices not connected to the bus by mistake. This is quite safe too. Unless the device chipselect is activated the device should ignore whatever you write on the bus - unless you get the chipselect polarity wrong. But you can get i2c address and lots of other things wrong too. > >> + /* >> + * This is far from perfect since an addition might be >> + * done between here and the call to spi_add_device, >> + * but we can't hold the lock and call spi_add_device >> + * either, as it would trigger a deadlock. >> + * >> + * If such a race occurs, spi_add_device will still >> + * catch it though, as it also checks for devices >> + * being registered several times on the same chip >> + * select. >> + */ >> + status = bus_for_each_dev(&spi_bus_type, NULL, spi, >> + spi_dev_check); >> + if (status) { >> + dev_dbg(&master->dev, "Chipselect already in use.. Skipping."); >> + spi_dev_put(spi); >> + continue; >> + } > > This still leaves us in the situation where if we do know the device > that is connected we have to explicitly bind it in spidev which is > apparently unreasonably difficult for people. I'm also concerned about > the interactions with DT overlays here - what happens if a DT overlay > or other dynamic hardware instantiation comes along later and does bind > something to this chip select? It seems like we should be able to > combine the two models, and the fact that we only create these devices > with a Kconfig option is a bit of an interesting thing here. It does not bind anything because the chiselect is busy. That's why I use a patch which disregards spidev when determining if a chiselect is busy. That has the problem that you can access devices in use by kernel using spidev then. Ideally spidev (as checked by module alias or whatever) would be unbound when other driver requests the chipselect and rebound when the driver quits. Thanks Michal -- 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, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: > > Solve this by registering automatically spidev devices for all the unused chip > > selects when a master registers itself against the spi core. > > So, aside from the concern about this being generic the other thing here > is that we often have devices offering more chip selects than can > physically be used in a system but not doing anything to ensure that the > invalid ones can't be used. It's unclear to me if that's OK or not, it > might be since it's root only I think but I need to think it through a > bit. I might plead guilty here... :) I'd say we're also ok because if we delegate the device driving logic to userspace, we should expect it to know what it does to first drive the device properly, but also to open the right device for this. What's the worst that could happen in such a case? The data are output without any chipselect line being driven by the controller? Isn't that supposed to be ignored by the devices? > > This also adds an i2cdev-like feeling, where you get all the > > spidev devices all the time, without any modification. > > I2C is a bit safer here since it's a shared bus so you can't do > anything to devices not connected to the bus by mistake. I'm not sure to understand what you mean here. How is SPI different from that aspect? > > + /* > > + * This is far from perfect since an addition might be > > + * done between here and the call to spi_add_device, > > + * but we can't hold the lock and call spi_add_device > > + * either, as it would trigger a deadlock. > > + * > > + * If such a race occurs, spi_add_device will still > > + * catch it though, as it also checks for devices > > + * being registered several times on the same chip > > + * select. > > + */ > > + status = bus_for_each_dev(&spi_bus_type, NULL, spi, > > + spi_dev_check); > > + if (status) { > > + dev_dbg(&master->dev, "Chipselect already in use.. Skipping."); > > + spi_dev_put(spi); > > + continue; > > + } > > This still leaves us in the situation where if we do know the device > that is connected we have to explicitly bind it in spidev which is > apparently unreasonably difficult for people. You can still do that, but the point is that you don't have to. If you know what device you have, and want to use spidev on that device, it will still work, since it will create an spidev device when we will parse the DT. That device will then be skipped by that logic, which is ok. However, if you don't specify anything in your DT, and have no device created because you don't really know what's on the other end, this logic will create the spidev device so that you'll still get an spidev node exported to the userspace. > I'm also concerned about the interactions with DT overlays here - > what happens if a DT overlay or other dynamic hardware instantiation > comes along later and does bind something to this chip select? It > seems like we should be able to combine the two models, and the fact > that we only create these devices with a Kconfig option is a bit of > an interesting thing here. I think the safe approach would be, just like I told in this thread, to just check whether the modalias is spidev. If it is, destroy the previous (spidev) device, create a new device as specified by the DT, you're done. Maxime
On Wed, May 13, 2015 at 02:51:02PM +0200, Maxime Ripard wrote: > I'd say we're also ok because if we delegate the device driving logic > to userspace, we should expect it to know what it does to first drive > the device properly, but also to open the right device for this. > What's the worst that could happen in such a case? The data are output > without any chipselect line being driven by the controller? Isn't that > supposed to be ignored by the devices? I'm more worried about the chip select line being connected to the "make the board catch fire" signal or whatever (more realistically causing us to drive against some other external component) if the extra chip selects weren't pinmuxed away. > > > This also adds an i2cdev-like feeling, where you get all the > > > spidev devices all the time, without any modification. > > I2C is a bit safer here since it's a shared bus so you can't do > > anything to devices not connected to the bus by mistake. > I'm not sure to understand what you mean here. How is SPI different > from that aspect? Chip select signals. > > This still leaves us in the situation where if we do know the device > > that is connected we have to explicitly bind it in spidev which is > > apparently unreasonably difficult for people. > You can still do that, but the point is that you don't have to. Right, but that's not what I'd expect to happen (and seems to make it easier for people to not list things in the DT at all which doesn't seem great). If we're going to make it available by default I'd expect to be able to use a userspace driver with anything that doesn't have a driver bound rather than with devices that explicitly don't have any identification. > > I'm also concerned about the interactions with DT overlays here - > > what happens if a DT overlay or other dynamic hardware instantiation > > comes along later and does bind something to this chip select? It > > seems like we should be able to combine the two models, and the fact > > that we only create these devices with a Kconfig option is a bit of > > an interesting thing here. > I think the safe approach would be, just like I told in this thread, > to just check whether the modalias is spidev. If it is, destroy the > previous (spidev) device, create a new device as specified by the DT, > you're done. Sure, but I don't see code for that here.
On 13 May 2015 at 16:36, Mark Brown <broonie@kernel.org> wrote: > On Wed, May 13, 2015 at 02:51:02PM +0200, Maxime Ripard wrote: > >> I'd say we're also ok because if we delegate the device driving logic >> to userspace, we should expect it to know what it does to first drive >> the device properly, but also to open the right device for this. > >> What's the worst that could happen in such a case? The data are output >> without any chipselect line being driven by the controller? Isn't that >> supposed to be ignored by the devices? > > I'm more worried about the chip select line being connected to the > "make the board catch fire" signal or whatever (more realistically > causing us to drive against some other external component) if the extra > chip selects weren't pinmuxed away. They would not be pinmuxed away if 1) they are unused by anything else and cs happens to be the default 2) they are used by a device not in DT right? For the latter case we might want some way to disable unused chipselects as well as for the cosmetic issue of multitude of useless devices popping up. But you know, unused i2c bus can be also connected to "make the board catch fire" trace and nobody would notice until somebody has the great idea to probe it. Incidentally, both i2c and spi cs are active-low iirc. >> > This still leaves us in the situation where if we do know the device >> > that is connected we have to explicitly bind it in spidev which is >> > apparently unreasonably difficult for people. > >> You can still do that, but the point is that you don't have to. > > Right, but that's not what I'd expect to happen (and seems to make it > easier for people to not list things in the DT at all which doesn't seem > great). If we're going to make it available by default I'd expect to be > able to use a userspace driver with anything that doesn't have a driver > bound rather than with devices that explicitly don't have any > identification. Remember that spidev can and is used for boards which have spi *CONNECTOR* to which you can connect some random gadget and use a userspace program to communicate with it. It is cool that you can load a dt overlay if the gadget happens to have a kernel driver. However, what identification do you write in the dt when there is no need to use a kernel driver at all? The option to create a node with 'spidev' compatible was rejected as broken. The option to add new compatible to the spidev driver every time you want to connect a new device was rejected as absurdly complex for end users who have a board with system already preinstalled because it requires adding the compatible in the spidev source and rebuilding the kernel. So this patch implements another option which is to bind spidev *always*. The configuration of the port is then not recorded in the DT and it's up to the user to visually check the port or apply other relevant heuristic and run the correct userspace application. Thanks Michal -- 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, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: > > > While this is nicer than the DT solution because of its accurate hardware > > representation, it's still not perfect because you might not have access to the > > DT, or you might be driving a completely generic device (such as a > > microcontroller) that might be used for something else in a different > > context/board. > > Greg, you're copied on this because this seems to be a generic problem > that should perhaps be solved at a driver model level - having a way to > bind userspace access to devices that we don't otherwise have a driver > for. The subsystem could specify the UIO driver to use when no other > driver is available. That doesn't really work. I've been talking to the ACPI people about this, and the problem is "don't otherwise have a driver for" is an impossible thing to prove, as you never know when a driver is going to be loaded from userspace. You can easily bind drivers to devices today from userspace, why not just use the built-in functionality you have today if you "know" that there is no driver for this hardware. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13 May 2015 at 17:37, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: >> On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: >> >> > While this is nicer than the DT solution because of its accurate hardware >> > representation, it's still not perfect because you might not have access to the >> > DT, or you might be driving a completely generic device (such as a >> > microcontroller) that might be used for something else in a different >> > context/board. >> >> Greg, you're copied on this because this seems to be a generic problem >> that should perhaps be solved at a driver model level - having a way to >> bind userspace access to devices that we don't otherwise have a driver >> for. The subsystem could specify the UIO driver to use when no other >> driver is available. > > That doesn't really work. I've been talking to the ACPI people about > this, and the problem is "don't otherwise have a driver for" is an > impossible thing to prove, as you never know when a driver is going to > be loaded from userspace. Yes, exactly. That's why binding spidev in addition to regular drivers or have spidev bind always and get unbound when another driver tries to bind the CS is preferable. The latter is pretty much specifying an UIO driver to use when no other driver is available in the light of the fact that 'is available' might change over time. You can prove that a driver is not available right now. > > You can easily bind drivers to devices today from userspace, why not > just use the built-in functionality you have today if you "know" that > there is no driver for this hardware. > And what exactly do you bind the driver to when there is no device created with the built-in functionality you have today? Thanks Michal -- 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, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: > You can easily bind drivers to devices today from userspace, why not > just use the built-in functionality you have today if you "know" that > there is no driver for this hardware. So, that was my original suggestion too but people were complaining that this wasn't a generally supported feature and requires specific support of some kind in the bus (which nobody posted a patch for). I have to confess I didn't investigate too deeply myself.
On Wed, May 13, 2015 at 06:13:00PM +0100, Mark Brown wrote: > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: > > > You can easily bind drivers to devices today from userspace, why not > > just use the built-in functionality you have today if you "know" that > > there is no driver for this hardware. > > So, that was my original suggestion too but people were complaining that > this wasn't a generally supported feature and requires specific support > of some kind in the bus (which nobody posted a patch for). I don't understand the complaint, it's a very "generally supported feature" and has been there for a very long time for any bus that wants to use it. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 13, 2015 at 10:20:28AM -0700, Greg Kroah-Hartman wrote: > On Wed, May 13, 2015 at 06:13:00PM +0100, Mark Brown wrote: > > So, that was my original suggestion too but people were complaining that > > this wasn't a generally supported feature and requires specific support > > of some kind in the bus (which nobody posted a patch for). > I don't understand the complaint, it's a very "generally supported > feature" and has been there for a very long time for any bus that wants > to use it. So it's not something that just works for all buses and someone would need to write a patch (hint, hint - that's one of the big blockers here)? It is a bit surprising to me that this would be something that the buses would need to cope with individually since for most things the matching mechanics are taken care of in the driver core and we're explicitly overriding them here.
On Wed, May 13, 2015 at 05:31:05PM +0200, Michal Suchanek wrote: > But you know, unused i2c bus can be also connected to "make the board > catch fire" trace and nobody would notice until somebody has the great > idea to probe it. Incidentally, both i2c and spi cs are active-low > iirc. Someone would need to go and explicitly mark an I2C controller as enabled on a board for that to happen.
Hi Greg, On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: > > > > > While this is nicer than the DT solution because of its accurate hardware > > > representation, it's still not perfect because you might not have access to the > > > DT, or you might be driving a completely generic device (such as a > > > microcontroller) that might be used for something else in a different > > > context/board. > > > > Greg, you're copied on this because this seems to be a generic problem > > that should perhaps be solved at a driver model level - having a way to > > bind userspace access to devices that we don't otherwise have a driver > > for. The subsystem could specify the UIO driver to use when no other > > driver is available. > > That doesn't really work. I've been talking to the ACPI people about > this, and the problem is "don't otherwise have a driver for" is an > impossible thing to prove, as you never know when a driver is going to > be loaded from userspace. > > You can easily bind drivers to devices today from userspace, why not > just use the built-in functionality you have today if you "know" that > there is no driver for this hardware. What we're really after here is that we want to have an spidev instance when we don't even have a device. And since SPI isn't really doing any kind of hotplug, the only situation that might be problematic is if we have the DT overlays registering new devices. Maxime
On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote: > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: > > That doesn't really work. I've been talking to the ACPI people about > > this, and the problem is "don't otherwise have a driver for" is an > > impossible thing to prove, as you never know when a driver is going to > > be loaded from userspace. > > You can easily bind drivers to devices today from userspace, why not > > just use the built-in functionality you have today if you "know" that > > there is no driver for this hardware. > What we're really after here is that we want to have an spidev > instance when we don't even have a device. We do? > And since SPI isn't really doing any kind of hotplug, the only > situation that might be problematic is if we have the DT overlays > registering new devices. Well, we still have the driver loaded from userspace in the future situation as well - the hardware might be static but the software isn't.
On Wed, May 13, 2015 at 06:39:22PM +0100, Mark Brown wrote: > On Wed, May 13, 2015 at 10:20:28AM -0700, Greg Kroah-Hartman wrote: > > On Wed, May 13, 2015 at 06:13:00PM +0100, Mark Brown wrote: > > > > So, that was my original suggestion too but people were complaining that > > > this wasn't a generally supported feature and requires specific support > > > of some kind in the bus (which nobody posted a patch for). > > > I don't understand the complaint, it's a very "generally supported > > feature" and has been there for a very long time for any bus that wants > > to use it. > > So it's not something that just works for all buses and someone would > need to write a patch (hint, hint - that's one of the big blockers > here)? It is a bit surprising to me that this would be something that > the buses would need to cope with individually since for most things the > matching mechanics are taken care of in the driver core and we're > explicitly overriding them here. It should "just work" for all busses, but if you want to add a "new_id" sysfs file, you need to add the logic for that to your bus. It's the bind/unbind files in the driver directories. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote: > Hi Greg, > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: > > > > > > > While this is nicer than the DT solution because of its accurate hardware > > > > representation, it's still not perfect because you might not have access to the > > > > DT, or you might be driving a completely generic device (such as a > > > > microcontroller) that might be used for something else in a different > > > > context/board. > > > > > > Greg, you're copied on this because this seems to be a generic problem > > > that should perhaps be solved at a driver model level - having a way to > > > bind userspace access to devices that we don't otherwise have a driver > > > for. The subsystem could specify the UIO driver to use when no other > > > driver is available. > > > > That doesn't really work. I've been talking to the ACPI people about > > this, and the problem is "don't otherwise have a driver for" is an > > impossible thing to prove, as you never know when a driver is going to > > be loaded from userspace. > > > > You can easily bind drivers to devices today from userspace, why not > > just use the built-in functionality you have today if you "know" that > > there is no driver for this hardware. > > What we're really after here is that we want to have an spidev > instance when we don't even have a device. That's crazy, just create a device, things do not work without one. > And since SPI isn't really doing any kind of hotplug, the only > situation that might be problematic is if we have the DT overlays > registering new devices. I have no idea what this all is, sorry, patches would be good if you want to get your idea across. Maybe it was sent early in this thread, but I can't seem to find one... greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 13, 2015 at 11:16:31AM -0700, Greg Kroah-Hartman wrote: > It should "just work" for all busses, but if you want to add a "new_id" > sysfs file, you need to add the logic for that to your bus. It's the > bind/unbind files in the driver directories. Oh, right. For this to be useful here we'd need to implement a new_id file, bind and unbind don't do anything helpful here. I think I'd have expected this to have a default implementation that non-enumerable buses could pick up.
On Wed, May 13, 2015 at 07:32:11PM +0100, Mark Brown wrote: > On Wed, May 13, 2015 at 11:16:31AM -0700, Greg Kroah-Hartman wrote: > > > It should "just work" for all busses, but if you want to add a "new_id" > > sysfs file, you need to add the logic for that to your bus. It's the > > bind/unbind files in the driver directories. > > Oh, right. For this to be useful here we'd need to implement a new_id > file, bind and unbind don't do anything helpful here. I think I'd have > expected this to have a default implementation that non-enumerable buses > could pick up. No one has ever asked for that, so feel free to make a patch that adds it :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 13, 2015 at 11:36:53AM -0700, Greg Kroah-Hartman wrote: > On Wed, May 13, 2015 at 07:32:11PM +0100, Mark Brown wrote: > > Oh, right. For this to be useful here we'd need to implement a new_id > > file, bind and unbind don't do anything helpful here. I think I'd have > > expected this to have a default implementation that non-enumerable buses > > could pick up. > No one has ever asked for that, so feel free to make a patch that adds > it :) Maxime? :P
On Wed, May 13, 2015 at 03:36:10PM +0100, Mark Brown wrote: > On Wed, May 13, 2015 at 02:51:02PM +0200, Maxime Ripard wrote: > > > I'd say we're also ok because if we delegate the device driving logic > > to userspace, we should expect it to know what it does to first drive > > the device properly, but also to open the right device for this. > > > What's the worst that could happen in such a case? The data are output > > without any chipselect line being driven by the controller? Isn't that > > supposed to be ignored by the devices? > > I'm more worried about the chip select line being connected to the > "make the board catch fire" signal or whatever (more realistically > causing us to drive against some other external component) if the extra > chip selects weren't pinmuxed away. It seems we've had this discussion at lot lately ;) That indeed might be problematic.... > > > > This also adds an i2cdev-like feeling, where you get all the > > > > spidev devices all the time, without any modification. > > > > I2C is a bit safer here since it's a shared bus so you can't do > > > anything to devices not connected to the bus by mistake. > > > I'm not sure to understand what you mean here. How is SPI different > > from that aspect? > > Chip select signals. Well, if it's not connected to the bus, it probably won't be connected to the chip select either, will it? > > > This still leaves us in the situation where if we do know the device > > > that is connected we have to explicitly bind it in spidev which is > > > apparently unreasonably difficult for people. > > > You can still do that, but the point is that you don't have to. > > Right, but that's not what I'd expect to happen (and seems to make it > easier for people to not list things in the DT at all which doesn't seem > great). If we're going to make it available by default I'd expect to be > able to use a userspace driver with anything that doesn't have a driver > bound rather than with devices that explicitly don't have any > identification. The point is that if we don't have anything declared in the DT, we won't even have a device. So we can't really expect that the device will not be bound to a driver, because it won't even be there in the first place. > > > I'm also concerned about the interactions with DT overlays here - > > > what happens if a DT overlay or other dynamic hardware instantiation > > > comes along later and does bind something to this chip select? It > > > seems like we should be able to combine the two models, and the fact > > > that we only create these devices with a Kconfig option is a bit of > > > an interesting thing here. > > > I think the safe approach would be, just like I told in this thread, > > to just check whether the modalias is spidev. If it is, destroy the > > previous (spidev) device, create a new device as specified by the DT, > > you're done. > > Sure, but I don't see code for that here. No, of course. Remember that this code was written before the overlays were posted. Maxime
On Wed, May 13, 2015 at 2:51 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: >> > This also adds an i2cdev-like feeling, where you get all the >> > spidev devices all the time, without any modification. >> >> I2C is a bit safer here since it's a shared bus so you can't do >> anything to devices not connected to the bus by mistake. > > I'm not sure to understand what you mean here. How is SPI different > from that aspect? If you talk to a nonexistent i2c device, nothing happens, as it just sends a message with a nonexistent address on the shared bus. If you talk to a nonexistent spi device, hell may break loose if e.g. some "smart" hardware engineer used the "unused" CS as a pull-up for the _RESET line on an external device... It's a bit like banging random "unused" GPIOs. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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, May 13, 2015 at 07:51:49PM +0100, Mark Brown wrote: > On Wed, May 13, 2015 at 11:36:53AM -0700, Greg Kroah-Hartman wrote: > > On Wed, May 13, 2015 at 07:32:11PM +0100, Mark Brown wrote: > > > > Oh, right. For this to be useful here we'd need to implement a new_id > > > file, bind and unbind don't do anything helpful here. I think I'd have > > > expected this to have a default implementation that non-enumerable buses > > > could pick up. > > > No one has ever asked for that, so feel free to make a patch that adds > > it :) > > Maxime? :P I can try to give it a shot. Maxime
On Wed, May 13, 2015 at 8:17 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote: >> On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: >> > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: >> > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: >> > > >> > > > While this is nicer than the DT solution because of its accurate hardware >> > > > representation, it's still not perfect because you might not have access to the >> > > > DT, or you might be driving a completely generic device (such as a >> > > > microcontroller) that might be used for something else in a different >> > > > context/board. >> > > >> > > Greg, you're copied on this because this seems to be a generic problem >> > > that should perhaps be solved at a driver model level - having a way to >> > > bind userspace access to devices that we don't otherwise have a driver >> > > for. The subsystem could specify the UIO driver to use when no other >> > > driver is available. >> > >> > That doesn't really work. I've been talking to the ACPI people about >> > this, and the problem is "don't otherwise have a driver for" is an >> > impossible thing to prove, as you never know when a driver is going to >> > be loaded from userspace. >> > >> > You can easily bind drivers to devices today from userspace, why not >> > just use the built-in functionality you have today if you "know" that >> > there is no driver for this hardware. >> >> What we're really after here is that we want to have an spidev >> instance when we don't even have a device. > > That's crazy, just create a device, things do not work without one. One simple way that works today is to create a device tree overlay with a device that's compatible with "spidev". That does violate DT policy, as it doesn't describe the hardware. But it works. This can be done from userspace, so we (the naggers about incorrect description of the hardware) will never see the abusive dtso. Alternatively, you can write some kernel code that does more or less the same thing, i.e. create a device for "spidev", but without any DT involved. If you add remove support, and make this controllable through sysfs, I think this could be a viable solution. It would be similar to gpio export. A DT overlay with the real device can still take over, if you instruct the removal of the "spidev" device through sysfs first. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote: > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote: > > Hi Greg, > > > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: > > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: > > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: > > > > > > > > > While this is nicer than the DT solution because of its accurate hardware > > > > > representation, it's still not perfect because you might not have access to the > > > > > DT, or you might be driving a completely generic device (such as a > > > > > microcontroller) that might be used for something else in a different > > > > > context/board. > > > > > > > > Greg, you're copied on this because this seems to be a generic problem > > > > that should perhaps be solved at a driver model level - having a way to > > > > bind userspace access to devices that we don't otherwise have a driver > > > > for. The subsystem could specify the UIO driver to use when no other > > > > driver is available. > > > > > > That doesn't really work. I've been talking to the ACPI people about > > > this, and the problem is "don't otherwise have a driver for" is an > > > impossible thing to prove, as you never know when a driver is going to > > > be loaded from userspace. > > > > > > You can easily bind drivers to devices today from userspace, why not > > > just use the built-in functionality you have today if you "know" that > > > there is no driver for this hardware. > > > > What we're really after here is that we want to have an spidev > > instance when we don't even have a device. > > That's crazy, just create a device, things do not work without one. Our use case is this one: we want to export spidev files so that "dev boards" with a header that allows to plug virtually anything on it (Raspberry Pi, Cubieboards, Xplained, and all the likes) without having to change the kernel and / or device tree. That would mean that if we plug something to that port, no device will be created because the DT itself won't have that device declared in the first place. This patch is actually doing this: creating a new device for all the chipselects that are not in use that will be bound to the spidev driver. Maxime
On Wed, May 13, 2015 at 09:10:48PM +0200, Geert Uytterhoeven wrote: > On Wed, May 13, 2015 at 2:51 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > >> > This also adds an i2cdev-like feeling, where you get all the > >> > spidev devices all the time, without any modification. > >> > >> I2C is a bit safer here since it's a shared bus so you can't do > >> anything to devices not connected to the bus by mistake. > > > > I'm not sure to understand what you mean here. How is SPI different > > from that aspect? > > If you talk to a nonexistent i2c device, nothing happens, as it just sends > a message with a nonexistent address on the shared bus. > > If you talk to a nonexistent spi device, hell may break loose if e.g. some > "smart" hardware engineer used the "unused" CS as a pull-up for the > _RESET line on an external device... It's a bit like banging random > "unused" GPIOs. Ah, right. I'm always surprised by how creative the hardware engineers actually are :) Maxime
On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote: > On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote: > > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote: > > > Hi Greg, > > > > > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: > > > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: > > > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: > > > > > > > > > > > While this is nicer than the DT solution because of its accurate hardware > > > > > > representation, it's still not perfect because you might not have access to the > > > > > > DT, or you might be driving a completely generic device (such as a > > > > > > microcontroller) that might be used for something else in a different > > > > > > context/board. > > > > > > > > > > Greg, you're copied on this because this seems to be a generic problem > > > > > that should perhaps be solved at a driver model level - having a way to > > > > > bind userspace access to devices that we don't otherwise have a driver > > > > > for. The subsystem could specify the UIO driver to use when no other > > > > > driver is available. > > > > > > > > That doesn't really work. I've been talking to the ACPI people about > > > > this, and the problem is "don't otherwise have a driver for" is an > > > > impossible thing to prove, as you never know when a driver is going to > > > > be loaded from userspace. > > > > > > > > You can easily bind drivers to devices today from userspace, why not > > > > just use the built-in functionality you have today if you "know" that > > > > there is no driver for this hardware. > > > > > > What we're really after here is that we want to have an spidev > > > instance when we don't even have a device. > > > > That's crazy, just create a device, things do not work without one. > > Our use case is this one: we want to export spidev files so that "dev > boards" with a header that allows to plug virtually anything on it > (Raspberry Pi, Cubieboards, Xplained, and all the likes) without > having to change the kernel and / or device tree. You want to do that on a bus that is not self-describing or dynamic? I too want a pony. Please go kick the hardware engineer who designed such a mess, we solved this problem 20+ years ago with "real" busses. > That would mean that if we plug something to that port, no device will > be created because the DT itself won't have that device declared in > the first place. Because you can't dynamically determine that something was plugged in, of course. > This patch is actually doing this: creating a new device for all the > chipselects that are not in use that will be bound to the spidev > driver. I have yet to see a patch... -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 13, 2015 at 03:33:31PM -0700, Greg Kroah-Hartman wrote: > On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote: > > Our use case is this one: we want to export spidev files so that "dev > > boards" with a header that allows to plug virtually anything on it > > (Raspberry Pi, Cubieboards, Xplained, and all the likes) without > > having to change the kernel and / or device tree. > You want to do that on a bus that is not self-describing or dynamic? > I too want a pony. Please go kick the hardware engineer who designed > such a mess, we solved this problem 20+ years ago with "real" busses. The hardware engineers for some of these things did build enumeration in but none of them with any sort of community have been able to make it stick - the community just ends up ignoring the ID allocation. > > This patch is actually doing this: creating a new device for all the > > chipselects that are not in use that will be bound to the spidev > > driver. > I have yet to see a patch... That was the first message in the thread.
On Wed, May 13, 2015 at 03:33:31PM -0700, Greg Kroah-Hartman wrote: > On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote: > > On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote: > > > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote: > > > > Hi Greg, > > > > > > > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: > > > > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: > > > > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: > > > > > > > > > > > > > While this is nicer than the DT solution because of its accurate hardware > > > > > > > representation, it's still not perfect because you might not have access to the > > > > > > > DT, or you might be driving a completely generic device (such as a > > > > > > > microcontroller) that might be used for something else in a different > > > > > > > context/board. > > > > > > > > > > > > Greg, you're copied on this because this seems to be a generic problem > > > > > > that should perhaps be solved at a driver model level - having a way to > > > > > > bind userspace access to devices that we don't otherwise have a driver > > > > > > for. The subsystem could specify the UIO driver to use when no other > > > > > > driver is available. > > > > > > > > > > That doesn't really work. I've been talking to the ACPI people about > > > > > this, and the problem is "don't otherwise have a driver for" is an > > > > > impossible thing to prove, as you never know when a driver is going to > > > > > be loaded from userspace. > > > > > > > > > > You can easily bind drivers to devices today from userspace, why not > > > > > just use the built-in functionality you have today if you "know" that > > > > > there is no driver for this hardware. > > > > > > > > What we're really after here is that we want to have an spidev > > > > instance when we don't even have a device. > > > > > > That's crazy, just create a device, things do not work without one. > > > > Our use case is this one: we want to export spidev files so that "dev > > boards" with a header that allows to plug virtually anything on it > > (Raspberry Pi, Cubieboards, Xplained, and all the likes) without > > having to change the kernel and / or device tree. > > You want to do that on a bus that is not self-describing or dynamic? > I too want a pony. Please go kick the hardware engineer who designed > such a mess, we solved this problem 20+ years ago with "real" busses. Well, we do have such ponies on some bus that don't have any kind of enumeration. i2cdev allows to do just that already. That would seem logical to have a similar behaviour for SPI. > > That would mean that if we plug something to that port, no device will > > be created because the DT itself won't have that device declared in > > the first place. > > Because you can't dynamically determine that something was plugged in, > of course. Well.. Yeah. > > > This patch is actually doing this: creating a new device for all the > > chipselects that are not in use that will be bound to the spidev > > driver. > > I have yet to see a patch... You were in Cc of that patch, which is the first message in this thread. Maxime
On Wed, May 13, 2015 at 7:33 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, May 13, 2015 at 09:26:40PM +0200, Maxime Ripard wrote: >> On Wed, May 13, 2015 at 11:17:36AM -0700, Greg Kroah-Hartman wrote: >> > On Wed, May 13, 2015 at 07:50:34PM +0200, Maxime Ripard wrote: >> > > Hi Greg, >> > > >> > > On Wed, May 13, 2015 at 08:37:40AM -0700, Greg Kroah-Hartman wrote: >> > > > On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote: >> > > > > On Tue, May 12, 2015 at 10:33:24PM +0200, Maxime Ripard wrote: >> > > > > >> > > > > > While this is nicer than the DT solution because of its accurate hardware >> > > > > > representation, it's still not perfect because you might not have access to the >> > > > > > DT, or you might be driving a completely generic device (such as a >> > > > > > microcontroller) that might be used for something else in a different >> > > > > > context/board. >> > > > > >> > > > > Greg, you're copied on this because this seems to be a generic problem >> > > > > that should perhaps be solved at a driver model level - having a way to >> > > > > bind userspace access to devices that we don't otherwise have a driver >> > > > > for. The subsystem could specify the UIO driver to use when no other >> > > > > driver is available. >> > > > >> > > > That doesn't really work. I've been talking to the ACPI people about >> > > > this, and the problem is "don't otherwise have a driver for" is an >> > > > impossible thing to prove, as you never know when a driver is going to >> > > > be loaded from userspace. >> > > > >> > > > You can easily bind drivers to devices today from userspace, why not >> > > > just use the built-in functionality you have today if you "know" that >> > > > there is no driver for this hardware. >> > > >> > > What we're really after here is that we want to have an spidev >> > > instance when we don't even have a device. >> > >> > That's crazy, just create a device, things do not work without one. >> >> Our use case is this one: we want to export spidev files so that "dev >> boards" with a header that allows to plug virtually anything on it >> (Raspberry Pi, Cubieboards, Xplained, and all the likes) without >> having to change the kernel and / or device tree. > > You want to do that on a bus that is not self-describing or dynamic? > I too want a pony. Please go kick the hardware engineer who designed > such a mess, we solved this problem 20+ years ago with "real" busses. Mind you we are talking about buses created more than 20+ years ago. (Unfortunately) They are still used today for all kind of sensors. Boards like RPi, beaglebone, minnowboard expose the pins so we can actually talk to those sensors, plugging in anyone we'd like to. For some of them for example there are IIO drivers that we could use the driver model to allow binding them. But spidev/i2c-dev allow userspace to talk directly to them. And you don't know what *others* will plug into that bus... might even be their own microntrollers with no identification at all. Without something like the patch in the first message, people need to create DT overlays for platforms that support that. ACPI doesn't support overlays (yet) so we need to keep awful external platform drivers[1] just to make spidev to work.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index d5d7d2235163..e6ca46e1e0fc 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1384,6 +1384,52 @@ static void acpi_register_spi_devices(struct spi_master *master) static inline void acpi_register_spi_devices(struct spi_master *master) {} #endif /* CONFIG_ACPI */ +#ifdef CONFIG_SPI_SPIDEV +static void spidev_register_devices(struct spi_master *master) +{ + struct spi_device *spi; + int i, status; + + for (i = 0; i < master->num_chipselect; i++) { + spi = spi_alloc_device(master); + if (!spi) { + dev_err(&master->dev, "Couldn't allocate spidev device\n"); + continue; + } + + spi->chip_select = i; + strlcpy(spi->modalias, "spidev", sizeof(spi->modalias)); + + /* + * This is far from perfect since an addition might be + * done between here and the call to spi_add_device, + * but we can't hold the lock and call spi_add_device + * either, as it would trigger a deadlock. + * + * If such a race occurs, spi_add_device will still + * catch it though, as it also checks for devices + * being registered several times on the same chip + * select. + */ + status = bus_for_each_dev(&spi_bus_type, NULL, spi, + spi_dev_check); + if (status) { + dev_dbg(&master->dev, "Chipselect already in use.. Skipping."); + spi_dev_put(spi); + continue; + } + + if (spi_add_device(spi)) { + dev_err(&master->dev, "Couldn't add spidev device\n"); + spi_dev_put(spi); + } + } + +} +#else +static inline void spidev_register_devices(struct spi_master *master) {} +#endif /* CONFIG_SPI_SPIDEV */ + static void spi_master_release(struct device *dev) { struct spi_master *master; @@ -1575,6 +1621,7 @@ int spi_register_master(struct spi_master *master) /* Register devices from the device tree and ACPI */ of_register_spi_devices(master); acpi_register_spi_devices(master); + spidev_register_devices(master); done: return status; }
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(+)