LinuxLists.cc - [PATCH] spi: Force the registration of the spidev devices

[permalink] [raw]

Subject: [PATCH] spi: Add option to bind spidev to all chipselects

Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
set. Rename spidev devices to avoid sysfs conflict.

This allows dynamically loading SPI device overlays or communicating
with SPI devices configured by a kernel driver from userspace.

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/spi/Kconfig | 13 +++++++++
drivers/spi/spi.c | 74 ++++++++++++++++++++++++++++++++++---------------
include/linux/spi/spi.h | 1 +
3 files changed, 65 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 198f96b..b477828 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -651,6 +651,19 @@ config SPI_SPIDEV
Note that this application programming interface is EXPERIMENTAL
and hence SUBJECT TO CHANGE WITHOUT NOTICE while it stabilizes.

+config SPIDEV_SHADOW
+ depends on SPI_SPIDEV
+ bool "Allow spidev access to configured SPI devices (DANGEROUS)"
+ help
+ This creates a spidev device node for every chipselect.
+
+ It is possible to access even SPI devices which are in use by a
+ kernel driver. This allows invoking features not in use by the kernel
+ or checking device state from userspace when the kernel driver fails.
+
+ Sending out-of-order messages to the device or reconfiguring the
+ device might cause driver failure. DANGEROUS
+
config SPI_TLE62X0
tristate "Infineon TLE62X0 (for power switching)"
depends on SYSFS
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e6ca46e..b48a0dc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -254,16 +254,23 @@ struct spi_device *spi_alloc_device(struct spi_master *master)
}
EXPORT_SYMBOL_GPL(spi_alloc_device);

-static void spi_dev_set_name(struct spi_device *spi)
+static void spi_dev_set_name(struct spi_device *spi, const char * alias)
{
struct acpi_device *adev = ACPI_COMPANION(&spi->dev);

if (adev) {
- dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
+ if (alias)
+ dev_set_name(&spi->dev, "%s-%s", alias, acpi_dev_name(adev));
+ else
+ dev_set_name(&spi->dev, "spi-%s", acpi_dev_name(adev));
return;
}

- dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev),
+ if (alias)
+ dev_set_name(&spi->dev, "%s%u.%u", alias, spi->master->bus_num,
+ spi->chip_select);
+ else
+ dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->master->dev),
spi->chip_select);
}

@@ -272,22 +279,25 @@ static int spi_dev_check(struct device *dev, void *data)
struct spi_device *spi = to_spi_device(dev);
struct spi_device *new_spi = data;

- if (spi->master == new_spi->master &&
- spi->chip_select == new_spi->chip_select)
+ if (spi->master == new_spi->master
+ && spi->chip_select == new_spi->chip_select
+#ifdef CONFIG_SPIDEV_SHADOW
+ && !spi->shadow && !new_spi->shadow
+#endif
+ )
return -EBUSY;
return 0;
}

/**
- * spi_add_device - Add spi_device allocated with spi_alloc_device
+ * spi_add_device_alias - Add spi_device allocated with spi_alloc_device
+ * possibly even when device for the CS exists.
* @spi: spi_device to register
+ * @alias: string used as device name prefix or NULL
*
- * Companion function to spi_alloc_device. Devices allocated with
- * spi_alloc_device can be added onto the spi bus with this function.
- *
- * Returns 0 on success; negative errno on failure
+ * See spi_add_device
*/
-int spi_add_device(struct spi_device *spi)
+static inline int spi_add_device_alias(struct spi_device *spi, const char * alias)
{
static DEFINE_MUTEX(spi_add_lock);
struct spi_master *master = spi->master;
@@ -303,7 +313,8 @@ int spi_add_device(struct spi_device *spi)
}

/* Set the bus ID string */
- spi_dev_set_name(spi);
+ spi_dev_set_name(spi, alias);
+ spi->shadow = !!alias;

/* We need to make sure there's no other device with this
* chipselect **BEFORE** we call setup(), else we'll trash
@@ -321,15 +332,17 @@ int spi_add_device(struct spi_device *spi)
if (master->cs_gpios)
spi->cs_gpio = master->cs_gpios[spi->chip_select];

- /* Drivers may modify this initial i/o setup, but will
- * normally rely on the device being setup. Devices
- * using SPI_CS_HIGH can't coexist well otherwise...
- */
- status = spi_setup(spi);
- if (status < 0) {
- dev_err(dev, "can't setup %s, status %d\n",
- dev_name(&spi->dev), status);
- goto done;
+ if (!alias) {
+ /* Drivers may modify this initial i/o setup, but will
+ * normally rely on the device being setup. Devices
+ * using SPI_CS_HIGH can't coexist well otherwise...
+ */
+ status = spi_setup(spi);
+ if (status < 0) {
+ dev_err(dev, "can't setup %s, status %d\n",
+ dev_name(&spi->dev), status);
+ goto done;
+ }
}

/* Device may be bound to an active driver when this returns */
@@ -344,6 +357,20 @@ done:
mutex_unlock(&spi_add_lock);
return status;
}
+
+/**
+ * spi_add_device - Add spi_device allocated with spi_alloc_device
+ * @spi: spi_device to register
+ *
+ * Companion function to spi_alloc_device. Devices allocated with
+ * spi_alloc_device can be added onto the spi bus with this function.
+ *
+ * Returns 0 on success; negative errno on failure
+ */
+int spi_add_device(struct spi_device *spi)
+{
+ return spi_add_device_alias(spi, NULL);
+}
EXPORT_SYMBOL_GPL(spi_add_device);

/**
@@ -1400,6 +1427,7 @@ static void spidev_register_devices(struct spi_master *master)
spi->chip_select = i;
strlcpy(spi->modalias, "spidev", sizeof(spi->modalias));

+#ifndef CONFIG_SPIDEV_SHADOW
/*
* This is far from perfect since an addition might be
* done between here and the call to spi_add_device,
@@ -1418,8 +1446,8 @@ static void spidev_register_devices(struct spi_master *master)
spi_dev_put(spi);
continue;
}
-
- if (spi_add_device(spi)) {
+#endif /* CONFIG_SPIDEV_SHADOW */
+ if (spi_add_device_alias(spi, "spidev")) {
dev_err(&master->dev, "Couldn't add spidev device\n");
spi_dev_put(spi);
}
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index d673072..8368714 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -97,6 +97,7 @@ struct spi_device {
void *controller_data;
char modalias[SPI_NAME_SIZE];
int cs_gpio; /* chip select gpio */
+ int shadow; /* ignore when determining if CS is in use */

/*
* likely need more hooks for more protocol options affecting how
--
2.1.4

2015-05-13 10:20:06

[permalink] [raw]

Subject: Re: [PATCH] spi: Add option to bind spidev to all chipselects

Hi,

On Wed, May 13, 2015 at 09:34:41AM -0000, Michal Suchanek wrote:
> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
> set. Rename spidev devices to avoid sysfs conflict.
>
> This allows dynamically loading SPI device overlays or communicating
> with SPI devices configured by a kernel driver from userspace.
>
> Signed-off-by: Michal Suchanek <[email protected]>

Output from checkpatch:
total: 2 errors, 4 warnings, 4 checks, 157 lines checked

...

I told you a few times already to run checkpatch before sending your
patches, apparently you make a point at ignoring me. Fine.

That being said, I'm not sure this is the right approach, or at least,
it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still
have the same issue with addition of new devices on previously unused
chip selects, and where we have an spidev device now.

What I think we should do is, when a new device is created, we just
lookup the modalias of the spi_device associated to it.

If that modalias is "spidev", then unregister the spidev device,
register the new device, you're done. If not, return an error.

On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good
idea. There's a good chance it will break the driver by doing stuff
behind its back, possibly in a way that will harm the whole kernel,
and it's something we usually try to avoid.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (1.47 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-13 10:41:43

[permalink] [raw]

Subject: Re: [PATCH] spi: Add option to bind spidev to all chipselects

On 13 May 2015 at 12:16, Maxime Ripard <[email protected]> wrote:
> Hi,
>
> On Wed, May 13, 2015 at 09:34:41AM -0000, Michal Suchanek wrote:
>> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
>> set. Rename spidev devices to avoid sysfs conflict.
>>
>> This allows dynamically loading SPI device overlays or communicating
>> with SPI devices configured by a kernel driver from userspace.
>>
>> Signed-off-by: Michal Suchanek <[email protected]>
>
> Output from checkpatch:
> total: 2 errors, 4 warnings, 4 checks, 157 lines checked
>
> ...
>
> I told you a few times already to run checkpatch before sending your
> patches, apparently you make a point at ignoring me. Fine.

That's a good idea to run, yes.

Sorry about that.

I also discovered an additional issue with unused variable when the
config option is set.

>
> That being said, I'm not sure this is the right approach, or at least,
> it doesn't solve anything. If SPIDEV_SHADOW is not set, you will still
> have the same issue with addition of new devices on previously unused
> chip selects, and where we have an spidev device now.
>
> What I think we should do is, when a new device is created, we just
> lookup the modalias of the spi_device associated to it.
>
> If that modalias is "spidev", then unregister the spidev device,
> register the new device, you're done. If not, return an error.

Yes, that's what I intend to look into eventually. However, this patch
is still useful and allows both accessing unused bus with spidev and
dynamically loading overlays that would use the bus.

>
> On the SPIDEV_SHADOW stuff itself, I'm not sure this is such a good
> idea. There's a good chance it will break the driver by doing stuff
> behind its back, possibly in a way that will harm the whole kernel,
> and it's something we usually try to avoid.

What is the possibility to harm the whole kernel?

If the kernel crashes because the device misses a message this is
somewhat worrying.

You could see it as a developer option similar to SCSI error injection
and others that can introduce states that would normally occur only
rarely.

Thanks

Michal

2015-05-13 12:14:34

[permalink] [raw]

Subject: Re: [PATCH] spi: Add option to bind spidev to all chipselects

On Wed, May 13, 2015 at 09:34:41AM -0000, Michal Suchanek wrote:
> Bypass the check if CS is in use for spidev devices if CONFIG_SPIDEV_SHADOW is
> set. Rename spidev devices to avoid sysfs conflict.

To repeat my previous feedback I don't see any way in which this is
sane, having two drivers simultaneously controlling the same device is
an obviously terrible idea.

Attachments:

(No filename) (369.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-13 12:18:15

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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.

Attachments:

(No filename) (2.58 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-13 12:36:35

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

On 13 May 2015 at 13:26, Mark Brown <[email protected]> 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

2015-05-13 12:55:06

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (3.39 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-13 14:36:25

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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.

Attachments:

(No filename) (2.31 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-13 15:31:50

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

On 13 May 2015 at 16:36, Mark Brown <[email protected]> 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

2015-05-13 15:37:43

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

2015-05-13 15:53:03

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

On 13 May 2015 at 17:37, Greg Kroah-Hartman <[email protected]> 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

2015-05-13 17:13:23

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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.

Attachments:

(No filename) (516.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-13 17:20:32

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

2015-05-13 17:39:42

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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.

Attachments:

(No filename) (885.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-13 17:43:45

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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.

Attachments:

(No filename) (394.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-13 17:55:07

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (1.65 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-13 18:12:30

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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.

Attachments:

(No filename) (995.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-13 18:16:35

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

2015-05-13 18:17:39

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

2015-05-13 18:32:32

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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.

Attachments:

(No filename) (494.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-13 18:36:57

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

2015-05-13 18:52:06

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

Attachments:

(No filename) (473.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-13 19:10:10

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (3.07 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-13 19:10:52

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

On Wed, May 13, 2015 at 2:51 PM, Maxime Ripard
<[email protected]> 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 -- [email protected]

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

2015-05-13 19:20:10

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (703.00 B)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-13 19:23:34

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

On Wed, May 13, 2015 at 8:17 PM, Greg Kroah-Hartman
<[email protected]> 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 -- [email protected]

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

2015-05-13 19:30:08

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (2.35 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-13 19:45:08

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

On Wed, May 13, 2015 at 09:10:48PM +0200, Geert Uytterhoeven wrote:
> On Wed, May 13, 2015 at 2:51 PM, Maxime Ripard
> <[email protected]> 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

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (1.08 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-05-13 22:33:34

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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...

2015-05-14 14:34:57

[permalink] [raw]

Subject: Re: [PATCH] spi: Force the registration of the spidev devices

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.

Attachments:

(No filename) (1.03 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-15 08:10:16