[PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[Mark Brown]

Since spidev is a detail of how Linux controls a device rather than a
description of the hardware in the system we should never have a node
described as "spidev" in DT, any SPI device could be a spidev so this
is just not a useful description.

In order to help prevent users from writing such device trees generate a
warning if spidev is instantiated as a DT node without an ID in the match
table.

Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spidev.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 23ad97807797..92c909eed6b5 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -703,6 +703,14 @@ static const struct file_operations spidev_fops = {
 
 static struct class *spidev_class;
 
+#ifdef CONFIG_OF
+static const struct of_device_id spidev_dt_ids[] = {
+	{ .compatible = "rohm,dh2228fv" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, spidev_dt_ids);
+#endif
+
 /*-------------------------------------------------------------------------*/
 
 static int spidev_probe(struct spi_device *spi)
@@ -711,6 +719,17 @@ static int spidev_probe(struct spi_device *spi)
 	int			status;
 	unsigned long		minor;
 
+	/*
+	 * spidev should never be referenced in DT without a specific
+	 * compatbile string, it is a Linux implementation thing
+	 * rather than a description of the hardware.
+	 */
+	if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) {
+		dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n");
+		WARN_ON(spi->dev.of_node &&
+			!of_match_device(spidev_dt_ids, &spi->dev));
+	}
+
 	/* Allocate driver data */
 	spidev = kzalloc(sizeof(*spidev), GFP_KERNEL);
 	if (!spidev)
@@ -777,13 +796,6 @@ static int spidev_remove(struct spi_device *spi)
 	return 0;
 }
 
-static const struct of_device_id spidev_dt_ids[] = {
-	{ .compatible = "rohm,dh2228fv" },
-	{},
-};
-
-MODULE_DEVICE_TABLE(of, spidev_dt_ids);
-
 static struct spi_driver spidev_spi_driver = {
 	.driver = {
 		.name =		"spidev",
-- 
2.1.4

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

Re: [PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[Martin Sperl]

> On 28.03.2015, at 00:42, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> Since spidev is a detail of how Linux controls a device rather than a
> description of the hardware in the system we should never have a node
> described as "spidev" in DT, any SPI device could be a spidev so this
> is just not a useful description.
> 
> In order to help prevent users from writing such device trees generate a
> warning if spidev is instantiated as a DT node without an ID in the match
> table.
> 
> Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

So what is now the “correct way” to create a spidev device via the
device-tree? 

There is no update in Documentation/spi/spidev how to achieve this in
a “correct” manner besides using board_info and the mentioned possible
sysfs approach, which is not implemented as an alternative.

If I still put spidev into the device-tree - as there is not other way
to do it as far as I known - then it produces an issue:
[  328.255338] spidev spi32765.1: buggy DT: spidev listed directly in DT
[  328.255399] ------------[ cut here ]------------
[  328.255443] WARNING: CPU: 0 PID: 2968 at drivers/spi/spidev.c:730 spidev_probe+0x198/0x1e4 [spidev]()
[  328.255457] Modules linked in: spidev(+) spi_bcm2835 bcm2835_wdt bcm2835_dma virt_dma
... lots more lines with stack-trace ...

So it is just a warning triggered by:
                 WARN_ON(spi->dev.of_node &&
                         !of_match_device(spidev_dt_ids, &spi->dev));

But still it is “irritating” as, it produces lots of messages in dmesg.

Note that on a raspberry pi the 2 SPI devices are by default configured 
as spidev and only overlays would change the settings to a different
alias. So this change will impact that default immediately.

Thus I wonder if it is wise to produce potential “support issues" for 
distributions triggered by this patch.

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

Re: [PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[Geert Uytterhoeven]

On Thu, Apr 23, 2015 at 9:21 AM, Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
>> On 28.03.2015, at 00:42, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>
>> Since spidev is a detail of how Linux controls a device rather than a
>> description of the hardware in the system we should never have a node
>> described as "spidev" in DT, any SPI device could be a spidev so this
>> is just not a useful description.
>>
>> In order to help prevent users from writing such device trees generate a
>> warning if spidev is instantiated as a DT node without an ID in the match
>> table.
>>
>> Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> So what is now the “correct way” to create a spidev device via the
> device-tree?

Add the compatible value for your device to the spidev_dt_ids[] array in
drivers/spi/spidev.c.

> Note that on a raspberry pi the 2 SPI devices are by default configured
> as spidev and only overlays would change the settings to a different
> alias. So this change will impact that default immediately.
>
> Thus I wonder if it is wise to produce potential “support issues" for
> distributions triggered by this patch.

So what you need is a way to handover from generic spidev to a device-specific
driver, cfr. what graphics drivers do when the device has been bound to by
vesafb or simplefb.

Could this be implemented in a generic way in the spi or DT code?

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[Geert Uytterhoeven]

On Thu, Apr 23, 2015 at 9:42 AM, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> Note that on a raspberry pi the 2 SPI devices are by default configured
>> as spidev and only overlays would change the settings to a different
>> alias. So this change will impact that default immediately.
>>
>> Thus I wonder if it is wise to produce potential “support issues" for
>> distributions triggered by this patch.
>
> So what you need is a way to handover from generic spidev to a device-specific
> driver, cfr. what graphics drivers do when the device has been bound to by
> vesafb or simplefb.
>
> Could this be implemented in a generic way in the spi or DT code?

I guess this has been suggested before: the spi core could provide spidev
access to all spi client devices which are not bound by a driver?

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

On Thu, Apr 23, 2015 at 09:45:16AM +0200, Geert Uytterhoeven wrote:

> I guess this has been suggested before: the spi core could provide spidev
> access to all spi client devices which are not bound by a driver?

I don't know if it's been suggested before, certainly nobody did the
work to make it happen.  I don't think I have a massive objection in
principal.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[Martin Sperl]

On 2015-04-23 12:36, Mark Brown wrote:
> On Thu, Apr 23, 2015 at 09:45:16AM +0200, Geert Uytterhoeven wrote:
>
>> I guess this has been suggested before: the spi core could provide spidev
>> access to all spi client devices which are not bound by a driver?
>
> I don't know if it's been suggested before, certainly nobody did the
> work to make it happen.  I don't think I have a massive objection in
> principal.
>

Maybe until that is fixed keep the warning message but remove the
WARN_ON(...), which is responsible for the 50+ lines in dmesg which
looks like a major issue because of the full stack-trace produced
and not just a "simple" warning...
--
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

Re: [PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On Thu, Apr 23, 2015 at 03:21:26PM +0200, Martin Sperl wrote:
> On 2015-04-23 12:36, Mark Brown wrote:
> >On Thu, Apr 23, 2015 at 09:45:16AM +0200, Geert Uytterhoeven wrote:

> >>I guess this has been suggested before: the spi core could provide spidev
> >>access to all spi client devices which are not bound by a driver?

> >I don't know if it's been suggested before, certainly nobody did the
> >work to make it happen.  I don't think I have a massive objection in
> >principal.

> Maybe until that is fixed keep the warning message but remove the
> WARN_ON(...), which is responsible for the 50+ lines in dmesg which
> looks like a major issue because of the full stack-trace produced
> and not just a "simple" warning...

No, the whole point is to be loud and obnoxious so nobody has any reason
to say that they didn't know there was a problem - we already have the
initial solution Geert pointed out of adding things to the ID table in
the driver and I fear a simple warning isn't loud enough for something
like this.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[Martin Sperl]

> On 23.04.2015, at 20:13, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> As *repeatedly* mentioned before please stop taking things off-list.  I
> really shouldn't need to remind you of this quite so often, I'm very
> tempted to start ignoring such messages.
> 
This was NOT my intention and I am really sorry for that.

>> I agree, that we need a long-term solution, but the way it is right now
>> in for-next/for-linux people think something is broken and will call it
>> a regression or maybe an API change (because of a change in behaviour on
>> the device-tree).
> 
> The entire point of this change is to make it look like things are
> broken because they are, in fact, broken.  We continue to support these
> systems, we just complain loudly about them.

Essentially you say: let us produce an error-message for some
situation where there is NO way around making it work without
an error message.

>> But even with that change proposed to automatically register spidev in
>> the absence of a dedicated driver, this would basically drive the
>> distributions for boards like the raspberry pi, beaglebone black or
>> similar to change:
>> compatiblity="spidev";
>> to:
>> compatiblity="spi,unknown_device";
> 
>> The distribution does not know what people will connect, so they can
>> not set the "right" value, so they have to use a dummy - and spidev
>> is the "typical" dummy that works right now.
> 
> The distributions should not be registering any devices at all, they've
> no idea if anything is there in the first place or if spidev is a
> suitable way of controlling it - it may even be that the pin
> configuration for SPI is totally unsuitable.  Users should be using
> device tree overlays to instantiate whatever hardware they have fitted
> to their system or there should be some other system which enumerates
> connected modules (in the rare cases where that's viable), it was the
> BeagleBone people who started the overlay work in the first place for
> precisely this reason.

OK, so how should someone deploy a RFM12B device correctly with
device-tree?

There exists no (official) kernel support, but several user-drivers that
make use of spidev.

If I would define the following in my device-tree (overlay) as per your
request:
	compatiblity=“hoperf,rfm12b”;

then (as of now) no spidev would get loaded and I can not use a
user-space driver that relies on spidev.

Similar for a max187 ADC.

Also note that the "beagle-bone people” still provide device-trees that
explicitly set up compatiblity=“spidev” - I just did a quick check:
/boot/dtbs/3.19.3-bone4/omap3-ha-lcd.dtb contains spidev.


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

Re: [PATCH] spi: spidev: Warn loudly if instantiated from DT as "spidev"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

On Thu, Apr 23, 2015 at 09:16:54PM +0200, Martin Sperl wrote:
> > On 23.04.2015, at 20:13, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > As *repeatedly* mentioned before please stop taking things off-list.  I
> > really shouldn't need to remind you of this quite so often, I'm very
> > tempted to start ignoring such messages.

> This was NOT my intention and I am really sorry for that.

Please also use the normal list, you're using the sourceforge list which
hasn't been in use for quite some time now.

> >> I agree, that we need a long-term solution, but the way it is right now
> >> in for-next/for-linux people think something is broken and will call it
> >> a regression or maybe an API change (because of a change in behaviour on
> >> the device-tree).

> > The entire point of this change is to make it look like things are
> > broken because they are, in fact, broken.  We continue to support these
> > systems, we just complain loudly about them.

> Essentially you say: let us produce an error-message for some
> situation where there is NO way around making it work without
> an error message.

Sure there is, add a device ID to spidev and use that in the DT as Geert
said.

> Also note that the "beagle-bone people” still provide device-trees that
> explicitly set up compatiblity=“spidev” - I just did a quick check:
> /boot/dtbs/3.19.3-bone4/omap3-ha-lcd.dtb contains spidev.

I suspect that's just some random DT they inherited from mainline rather
than something production, and honestly the fact that people keep on
doing this is exactly the reason we now complain loudly.  It's tedious
having to go through the "no, your DT should describe the hardware"
routine again and again.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]