From: Michal Suchanek <hramrach@gmail.com> To: Mark Brown <broonie@kernel.org> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Hans de Goede <hdegoede@redhat.com>, linux-spi <linux-spi@vger.kernel.org>, Martin Sperl <kernel@martin.sperl.org> Subject: Re: [PATCH] spi: Force the registration of the spidev devices Date: Wed, 13 May 2015 14:35:48 +0200 [thread overview] Message-ID: <CAOMqctTTwhjqLONixMyWiGuOHbmruKNp8QT8vw+y5hMp-wPtKQ@mail.gmail.com> (raw) In-Reply-To: <20150513112604.GI3066@sirena.org.uk> 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
WARNING: multiple messages have this Message-ID (diff)
From: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> To: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>, Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>, Linux Kernel Mailing List <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, linux-spi <linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> Subject: Re: [PATCH] spi: Force the registration of the spidev devices Date: Wed, 13 May 2015 14:35:48 +0200 [thread overview] Message-ID: <CAOMqctTTwhjqLONixMyWiGuOHbmruKNp8QT8vw+y5hMp-wPtKQ@mail.gmail.com> (raw) In-Reply-To: <20150513112604.GI3066-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> On 13 May 2015 at 13:26, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-05-13 12:36 UTC|newest] Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-05-12 20:33 [PATCH] spi: Force the registration of the spidev devices Maxime Ripard 2015-05-12 20:33 ` Maxime Ripard 2015-05-13 9:34 ` [PATCH] spi: Add option to bind spidev to all chipselects Michal Suchanek 2015-05-13 9:34 ` Michal Suchanek 2015-05-13 10:16 ` Maxime Ripard 2015-05-13 10:16 ` Maxime Ripard 2015-05-13 10:40 ` Michal Suchanek 2015-05-13 10:40 ` Michal Suchanek 2015-05-13 11:05 ` Mark Brown 2015-05-13 11:05 ` Mark Brown 2015-05-13 11:26 ` [PATCH] spi: Force the registration of the spidev devices Mark Brown 2015-05-13 11:26 ` Mark Brown 2015-05-13 12:35 ` Michal Suchanek [this message] 2015-05-13 12:35 ` Michal Suchanek 2015-05-13 12:51 ` Maxime Ripard 2015-05-13 12:51 ` Maxime Ripard 2015-05-13 14:36 ` Mark Brown 2015-05-13 14:36 ` Mark Brown 2015-05-13 15:31 ` Michal Suchanek 2015-05-13 15:31 ` Michal Suchanek 2015-05-13 17:43 ` Mark Brown 2015-05-13 17:43 ` Mark Brown 2015-05-13 19:09 ` Maxime Ripard 2015-05-13 19:10 ` Geert Uytterhoeven 2015-05-13 19:10 ` Geert Uytterhoeven 2015-05-13 19:41 ` Maxime Ripard 2015-05-13 19:41 ` Maxime Ripard 2015-05-13 15:37 ` Greg Kroah-Hartman 2015-05-13 15:37 ` Greg Kroah-Hartman 2015-05-13 15:52 ` Michal Suchanek 2015-05-13 15:52 ` Michal Suchanek 2015-05-13 17:13 ` Mark Brown 2015-05-13 17:13 ` Mark Brown 2015-05-13 17:20 ` Greg Kroah-Hartman 2015-05-13 17:20 ` Greg Kroah-Hartman 2015-05-13 17:39 ` Mark Brown 2015-05-13 17:39 ` Mark Brown 2015-05-13 18:16 ` Greg Kroah-Hartman 2015-05-13 18:16 ` Greg Kroah-Hartman 2015-05-13 18:32 ` Mark Brown 2015-05-13 18:36 ` Greg Kroah-Hartman 2015-05-13 18:36 ` Greg Kroah-Hartman 2015-05-13 18:51 ` Mark Brown 2015-05-13 18:51 ` Mark Brown 2015-05-13 19:17 ` Maxime Ripard 2015-05-13 19:17 ` Maxime Ripard 2015-05-13 17:50 ` Maxime Ripard 2015-05-13 17:50 ` Maxime Ripard 2015-05-13 18:12 ` Mark Brown 2015-05-13 18:17 ` Greg Kroah-Hartman 2015-05-13 18:17 ` Greg Kroah-Hartman 2015-05-13 19:23 ` Geert Uytterhoeven 2015-05-13 19:23 ` Geert Uytterhoeven 2015-05-13 19:26 ` Maxime Ripard 2015-05-13 19:26 ` Maxime Ripard 2015-05-13 22:33 ` Greg Kroah-Hartman 2015-05-13 22:33 ` Greg Kroah-Hartman 2015-05-14 14:34 ` Mark Brown 2015-05-14 14:34 ` Mark Brown 2015-05-15 8:09 ` Maxime Ripard 2015-07-15 6:27 ` Lucas De Marchi 2015-07-15 6:27 ` Lucas De Marchi -- strict thread matches above, loose matches on Subject: below -- 2014-04-28 17:22 Maxime Ripard 2014-04-28 17:22 ` Maxime Ripard 2014-04-29 18:37 ` Mark Brown 2014-04-29 18:37 ` Mark Brown [not found] ` <20140429183758.GH15125-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> 2014-04-29 21:31 ` Martin Sperl [not found] ` <24BF05CB-35FF-42E8-BE5C-A5E4E3D0C52A-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> 2014-04-30 18:14 ` Maxime Ripard 2014-04-30 20:00 ` Martin Sperl [not found] ` <DA3907EB-0C1B-42FB-B288-9E33F6E24E3E-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> 2014-04-30 22:19 ` Maxime Ripard 2014-05-01 1:21 ` Mark Brown 2014-04-30 18:06 ` Maxime Ripard 2014-04-30 18:06 ` Maxime Ripard 2014-05-01 1:18 ` Mark Brown 2014-05-01 1:18 ` Mark Brown 2014-05-01 22:36 ` Maxime Ripard 2014-05-01 22:36 ` Maxime Ripard 2014-05-01 23:28 ` Geert Uytterhoeven 2014-05-02 16:55 ` Mark Brown 2014-05-02 16:55 ` Mark Brown 2014-05-05 4:17 ` Maxime Ripard 2014-05-05 7:10 ` Geert Uytterhoeven 2014-05-05 13:57 ` Alexandre Belloni 2014-05-05 13:57 ` Alexandre Belloni 2014-05-05 14:22 ` Geert Uytterhoeven 2014-05-05 14:22 ` Geert Uytterhoeven 2014-05-05 19:16 ` Mark Brown 2014-05-02 17:40 ` Mark Brown 2014-05-02 17:40 ` Mark Brown 2014-05-05 4:21 ` Maxime Ripard 2014-05-05 19:17 ` Mark Brown 2014-05-05 19:17 ` Mark Brown 2014-05-08 2:22 ` Maxime Ripard 2014-05-08 2:22 ` Maxime Ripard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAOMqctTTwhjqLONixMyWiGuOHbmruKNp8QT8vw+y5hMp-wPtKQ@mail.gmail.com \ --to=hramrach@gmail.com \ --cc=broonie@kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=hdegoede@redhat.com \ --cc=kernel@martin.sperl.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-spi@vger.kernel.org \ --cc=maxime.ripard@free-electrons.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.