[PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

Allow the same devices to be used in ACPI based systems if they provide
proper DT compatible string.

Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/spi/spidev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index e3c19f30f591..86a4c28b9e58 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -691,14 +691,12 @@ 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" },
 	{ .compatible = "lineartechnology,ltc2488" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, spidev_dt_ids);
-#endif
 
 /*-------------------------------------------------------------------------*/
 
@@ -788,7 +786,7 @@ static int spidev_remove(struct spi_device *spi)
 static struct spi_driver spidev_spi_driver = {
 	.driver = {
 		.name =		"spidev",
-		.of_match_table = of_match_ptr(spidev_dt_ids),
+		.of_match_table = spidev_dt_ids,
 	},
 	.probe =	spidev_probe,
 	.remove =	spidev_remove,
-- 
2.8.1

--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Mon, Jun 27, 2016 at 02:34:08PM +0300, Mika Westerberg wrote:

> Allow the same devices to be used in ACPI based systems if they provide
> proper DT compatible string.

Please allow a reasonable time for review, especially for an invasive
change like this which needs a bunch of research to figure out how
sensible the infrastructure to shove DT into ACPI is.

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

Re: [PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

On Mon, Jun 27, 2016 at 01:24:48PM +0100, Mark Brown wrote:
> On Mon, Jun 27, 2016 at 02:34:08PM +0300, Mika Westerberg wrote:
> 
> > Allow the same devices to be used in ACPI based systems if they provide
> > proper DT compatible string.
> 
> Please allow a reasonable time for review, especially for an invasive
> change like this which needs a bunch of research to figure out how
> sensible the infrastructure to shove DT into ACPI is.

Sure.

Can I ask your opinion regarding an alternative approach to this?

Instead of adding these "pseudo", Linux specific devices to DT or ACPI
to support raw SPI access, what if we amend spidev a bit:

  - Introduce CONFIG_SPI_SPIDEV_MASTER which is part of spidev.c
    and can be selected separately.

  - If that option is set we create /dev/spi-<bus_num> for each SPI
    master.

  - Introduce two new ioctls SPI_IOC_RD/WR_CS which allow setting
    of chip select.

The /dev/spi-0 then can be used analogous to /dev/i2c-0. You first need
to program wanted chip select using those new ioctls. Then you can use
the device as normal spidev (all existing file operations and ioctls
still work). You can pick another chip select as needed. If there is an
actual SPI device bound to a chip select, that cannot be used through
/dev/spi-0.
--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Tue, Jun 28, 2016 at 12:22:34PM +0300, Mika Westerberg wrote:
> On Mon, Jun 27, 2016 at 01:24:48PM +0100, Mark Brown wrote:

> > Please allow a reasonable time for review, especially for an invasive
> > change like this which needs a bunch of research to figure out how
> > sensible the infrastructure to shove DT into ACPI is.

> Sure.

The big thing I need to check out here is if this breaks the check for
directly listing spidev in DT so we end up with the same abstraction
failures that plague DT since people refuse to describe their hardware.

> The /dev/spi-0 then can be used analogous to /dev/i2c-0. You first need
> to program wanted chip select using those new ioctls. Then you can use
> the device as normal spidev (all existing file operations and ioctls
> still work). You can pick another chip select as needed. If there is an
> actual SPI device bound to a chip select, that cannot be used through
> /dev/spi-0.

The trouble with this is that unlike I2C using SPI devices involves
toggling signals that we don't definitively know are connected to the
bus.  It's therefore much less safe than the equivalent thing for DT.
If we went the extra step and required that a slave be listed in DT then
that is much less of a concern.

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

Re: [PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

On Tue, Jun 28, 2016 at 12:47:42PM +0100, Mark Brown wrote:
> On Tue, Jun 28, 2016 at 12:22:34PM +0300, Mika Westerberg wrote:
> > On Mon, Jun 27, 2016 at 01:24:48PM +0100, Mark Brown wrote:
> 
> > > Please allow a reasonable time for review, especially for an invasive
> > > change like this which needs a bunch of research to figure out how
> > > sensible the infrastructure to shove DT into ACPI is.
> 
> > Sure.
> 
> The big thing I need to check out here is if this breaks the check for
> directly listing spidev in DT so we end up with the same abstraction
> failures that plague DT since people refuse to describe their hardware.

You are talking about this check, right?

        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));
        } 

As far as I can tell it should work, with the patch applied, just as it worked
before. For ACPI there is no possibility to list the node directly in ASL code
as we always require DT compatible string in _DSD.

In other words we never match using name of the node.

> > The /dev/spi-0 then can be used analogous to /dev/i2c-0. You first need
> > to program wanted chip select using those new ioctls. Then you can use
> > the device as normal spidev (all existing file operations and ioctls
> > still work). You can pick another chip select as needed. If there is an
> > actual SPI device bound to a chip select, that cannot be used through
> > /dev/spi-0.
> 
> The trouble with this is that unlike I2C using SPI devices involves
> toggling signals that we don't definitively know are connected to the
> bus.  It's therefore much less safe than the equivalent thing for DT.
> If we went the extra step and required that a slave be listed in DT then
> that is much less of a concern.

OK, I see.
--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Tue, Jun 28, 2016 at 05:35:28PM +0300, Mika Westerberg wrote:
> On Tue, Jun 28, 2016 at 12:47:42PM +0100, Mark Brown wrote:

> > The big thing I need to check out here is if this breaks the check for
> > directly listing spidev in DT so we end up with the same abstraction
> > failures that plague DT since people refuse to describe their hardware.

> You are talking about this check, right?

>         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));
>         } 

> As far as I can tell it should work, with the patch applied, just as it worked
> before. For ACPI there is no possibility to list the node directly in ASL code
> as we always require DT compatible string in _DSD.

Yes, but does of_node get set and does of_match_device() DTRT if it's a
DT into ACPI device?

> In other words we never match using name of the node.

If that's the case then it's definitely broken - we either always warn
or never warn, neither is good.

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

Re: [PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

On Tue, Jun 28, 2016 at 04:07:52PM +0100, Mark Brown wrote:
> On Tue, Jun 28, 2016 at 05:35:28PM +0300, Mika Westerberg wrote:
> > On Tue, Jun 28, 2016 at 12:47:42PM +0100, Mark Brown wrote:
> 
> > > The big thing I need to check out here is if this breaks the check for
> > > directly listing spidev in DT so we end up with the same abstraction
> > > failures that plague DT since people refuse to describe their hardware.
> 
> > You are talking about this check, right?
> 
> >         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));
> >         } 
> 
> > As far as I can tell it should work, with the patch applied, just as it worked
> > before. For ACPI there is no possibility to list the node directly in ASL code
> > as we always require DT compatible string in _DSD.
> 
> Yes, but does of_node get set and does of_match_device() DTRT if it's a
> DT into ACPI device?

of_node will be NULL and of_match_device() will not match (returns NULL
as well).

> > In other words we never match using name of the node.
> 
> If that's the case then it's definitely broken - we either always warn
> or never warn, neither is good.

For DT we warn if the device is used directly (by the node name) as
previously. For ACPI we never warn because it is not possible to use the
node name directly. We always require use of a proper compatible string.

I don't see how that is broken.
--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Tue, Jun 28, 2016 at 06:23:38PM +0300, Mika Westerberg wrote:
> On Tue, Jun 28, 2016 at 04:07:52PM +0100, Mark Brown wrote:

> > If that's the case then it's definitely broken - we either always warn
> > or never warn, neither is good.

> For DT we warn if the device is used directly (by the node name) as
> previously. For ACPI we never warn because it is not possible to use the
> node name directly. We always require use of a proper compatible string.

> I don't see how that is broken.

So you're saying that it's not possible for ACPI to use anything
*except* an explicitly listed compatible string to bind?  What we want
to avoid is any ACPI tables that explicitly list spidev since that's a
total abstraction failure.

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

Re: [PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

On Tue, Jun 28, 2016 at 05:18:22PM +0100, Mark Brown wrote:
> On Tue, Jun 28, 2016 at 06:23:38PM +0300, Mika Westerberg wrote:
> > On Tue, Jun 28, 2016 at 04:07:52PM +0100, Mark Brown wrote:
> 
> > > If that's the case then it's definitely broken - we either always warn
> > > or never warn, neither is good.
> 
> > For DT we warn if the device is used directly (by the node name) as
> > previously. For ACPI we never warn because it is not possible to use the
> > node name directly. We always require use of a proper compatible string.
> 
> > I don't see how that is broken.
> 
> So you're saying that it's not possible for ACPI to use anything
> *except* an explicitly listed compatible string to bind?  What we want
> to avoid is any ACPI tables that explicitly list spidev since that's a
> total abstraction failure.

That's exactly what I'm saying. We never match using the node name
(which is always 32-bit "string" like "SPI0"). For PRP0001 match to
succeeed you need to have "compatible" property.

The ASL code looks like:

        Device (SPID) {
                Name (_HID, "PRP0001")

                Name (_CRS, ResourceTempate () {
                        SpiSerialBus (1, PolarityLow, FourWireMode, 0x08,
                            ControllerInitiated, 0x007A1200, ClockPolarityLow,
                            ClockPhaseSecond, "\\_SB.SPI1",
                            0x00, ResourceConsumer)
                })

                Name (_DSD, Package () {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package () {
                        Package () {"compatible", "rohm,dh2228fv"},
                    }
                })
        }

Without the "compatible" property you get a warning from ACPI core:

  \_SB.SPI1.SPID requires 'compatible' property

and it will not match anything.
--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Wed, Jun 29, 2016 at 12:13:04PM +0300, Mika Westerberg wrote:
> On Tue, Jun 28, 2016 at 05:18:22PM +0100, Mark Brown wrote:

> > So you're saying that it's not possible for ACPI to use anything
> > *except* an explicitly listed compatible string to bind?  What we want
> > to avoid is any ACPI tables that explicitly list spidev since that's a
> > total abstraction failure.

> That's exactly what I'm saying. We never match using the node name
> (which is always 32-bit "string" like "SPI0"). For PRP0001 match to
> succeeed you need to have "compatible" property.

> Without the "compatible" property you get a warning from ACPI core:

>   \_SB.SPI1.SPID requires 'compatible' property

> and it will not match anything.

But can that compatible string be spidev and be matched via the paths DT
uses while still ensuring that enough of the OF matching code is enabled
to cause the check to warn?  That's the problem.

Life would be a lot eaiser if Intel would use DT where it's appropriate,
or if the OF in ACPI stuff were more transparent and needed fewer
special cases.

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

Re: [PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

On Wed, Jun 29, 2016 at 05:21:27PM +0100, Mark Brown wrote:
> On Wed, Jun 29, 2016 at 12:13:04PM +0300, Mika Westerberg wrote:
> > On Tue, Jun 28, 2016 at 05:18:22PM +0100, Mark Brown wrote:
> 
> > > So you're saying that it's not possible for ACPI to use anything
> > > *except* an explicitly listed compatible string to bind?  What we want
> > > to avoid is any ACPI tables that explicitly list spidev since that's a
> > > total abstraction failure.
> 
> > That's exactly what I'm saying. We never match using the node name
> > (which is always 32-bit "string" like "SPI0"). For PRP0001 match to
> > succeeed you need to have "compatible" property.
> 
> > Without the "compatible" property you get a warning from ACPI core:
> 
> >   \_SB.SPI1.SPID requires 'compatible' property
> 
> > and it will not match anything.
> 
> But can that compatible string be spidev and be matched via the paths DT
> uses while still ensuring that enough of the OF matching code is enabled
> to cause the check to warn?  That's the problem.

No it can't. The match is done in ACPI code not in DT code. See
drivers/acpi/bus.c::acpi_of_match_device().

> Life would be a lot eaiser if Intel would use DT where it's appropriate,
> or if the OF in ACPI stuff were more transparent and needed fewer
> special cases.

I don't get this. We added PRP0001 and properties because then we can
reuse DT bindings in ACPI and thus there is no need to reinvent all
these things for ACPI.

This is not a special case at all. I'm merely enabling the same thing to
work in ACPI systems (sans the node name match).
--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Wed, Jun 29, 2016 at 07:34:20PM +0300, Mika Westerberg wrote:
> On Wed, Jun 29, 2016 at 05:21:27PM +0100, Mark Brown wrote:

> > But can that compatible string be spidev and be matched via the paths DT
> > uses while still ensuring that enough of the OF matching code is enabled
> > to cause the check to warn?  That's the problem.

> No it can't. The match is done in ACPI code not in DT code. See
> drivers/acpi/bus.c::acpi_of_match_device().

And we're *sure* that's going to be maintained?  People do use the
fallback matching that DT does, I don't trust the ACPI maintainers not
to do the same thing.

> > Life would be a lot eaiser if Intel would use DT where it's appropriate,
> > or if the OF in ACPI stuff were more transparent and needed fewer
> > special cases.

> I don't get this. We added PRP0001 and properties because then we can
> reuse DT bindings in ACPI and thus there is no need to reinvent all
> these things for ACPI.

Right, but rather than just define a translation from ACPI to DT and use
the DT code paths in their entirety for the relevant nodes what's
happening is that there's a shim layer between ACPI and DT which sort of
emulates bits of the DT interfaces but not quite and can lead to
surprises.

> This is not a special case at all. I'm merely enabling the same thing to
> work in ACPI systems (sans the node name match).

The force DT into ACPI layer that exists in the ACPI code is the special
case here.

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

Re: [PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

On Wed, Jun 29, 2016 at 07:31:01PM +0100, Mark Brown wrote:
> On Wed, Jun 29, 2016 at 07:34:20PM +0300, Mika Westerberg wrote:
> > On Wed, Jun 29, 2016 at 05:21:27PM +0100, Mark Brown wrote:
> 
> > > But can that compatible string be spidev and be matched via the paths DT
> > > uses while still ensuring that enough of the OF matching code is enabled
> > > to cause the check to warn?  That's the problem.
> 
> > No it can't. The match is done in ACPI code not in DT code. See
> > drivers/acpi/bus.c::acpi_of_match_device().
> 
> And we're *sure* that's going to be maintained?  People do use the
> fallback matching that DT does, I don't trust the ACPI maintainers not
> to do the same thing.

That fallback matching does not even work in ACPI like I told you
already. We have no plans to do anything like that either. That's the
reason why we complain if there is PRP0001 without compatible string.

> > > Life would be a lot eaiser if Intel would use DT where it's appropriate,
> > > or if the OF in ACPI stuff were more transparent and needed fewer
> > > special cases.
> 
> > I don't get this. We added PRP0001 and properties because then we can
> > reuse DT bindings in ACPI and thus there is no need to reinvent all
> > these things for ACPI.
> 
> Right, but rather than just define a translation from ACPI to DT and use
> the DT code paths in their entirety for the relevant nodes what's
> happening is that there's a shim layer between ACPI and DT which sort of
> emulates bits of the DT interfaces but not quite and can lead to
> surprises.

Maybe but currently it works fine. And if problems are found they will
be fixed as usual.
--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Wed, Jun 29, 2016 at 09:51:55PM +0300, Mika Westerberg wrote:
> On Wed, Jun 29, 2016 at 07:31:01PM +0100, Mark Brown wrote:

> > And we're *sure* that's going to be maintained?  People do use the
> > fallback matching that DT does, I don't trust the ACPI maintainers not
> > to do the same thing.

> That fallback matching does not even work in ACPI like I told you
> already. We have no plans to do anything like that either. That's the
> reason why we complain if there is PRP0001 without compatible string.

No, you're completely missing the point here.  The problem is someone
using something like linux,spidev as a compatible string and going into
fallback matching on SPI bus IDs rather than compatible strings.  I just
don't have confidence that someone isn't going to try to add that
fallback path given that it's used for DT.

> > Right, but rather than just define a translation from ACPI to DT and use
> > the DT code paths in their entirety for the relevant nodes what's
> > happening is that there's a shim layer between ACPI and DT which sort of
> > emulates bits of the DT interfaces but not quite and can lead to
> > surprises.

> Maybe but currently it works fine. And if problems are found they will
> be fixed as usual.

I'm worried that the reason it's working fine may be that there is very
little usage, and that there may be more gotchas like we're already
seeing lurking about.

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

Re: [PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

On Wed, Jun 29, 2016 at 08:20:59PM +0100, Mark Brown wrote:
> On Wed, Jun 29, 2016 at 09:51:55PM +0300, Mika Westerberg wrote:
> > On Wed, Jun 29, 2016 at 07:31:01PM +0100, Mark Brown wrote:
> 
> > > And we're *sure* that's going to be maintained?  People do use the
> > > fallback matching that DT does, I don't trust the ACPI maintainers not
> > > to do the same thing.
> 
> > That fallback matching does not even work in ACPI like I told you
> > already. We have no plans to do anything like that either. That's the
> > reason why we complain if there is PRP0001 without compatible string.
> 
> No, you're completely missing the point here.  The problem is someone
> using something like linux,spidev as a compatible string and going into
> fallback matching on SPI bus IDs rather than compatible strings.  I just
> don't have confidence that someone isn't going to try to add that
> fallback path given that it's used for DT.

If someone is using "linux,spidev" as compatible string it will not
match anything and we are not going to add any kind of fallback to the
ACPI core for that.

Of course we cannot predict what happens in the future but is it really
preventing merging of this patch?

What I'm simply trying to achieve is to use the existing two DT
compatible strings from ACPI. I've tested that it works fine and
provided you all the evidence that it does not break the check for
misusing DT.

> > > Right, but rather than just define a translation from ACPI to DT and use
> > > the DT code paths in their entirety for the relevant nodes what's
> > > happening is that there's a shim layer between ACPI and DT which sort of
> > > emulates bits of the DT interfaces but not quite and can lead to
> > > surprises.
> 
> > Maybe but currently it works fine. And if problems are found they will
> > be fixed as usual.
> 
> I'm worried that the reason it's working fine may be that there is very
> little usage, and that there may be more gotchas like we're already
> seeing lurking about.

With more usage I'm sure we can improve things. After all the end goal
is to let single driver to work on both DT and ACPI systems with minimal
changes. If you think the current implementation is flawed, nothing
prevents you from proposing better alternative :-)
--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Thu, Jun 30, 2016 at 03:49:31PM +0300, Mika Westerberg wrote:

> If someone is using "linux,spidev" as compatible string it will not
> match anything and we are not going to add any kind of fallback to the
> ACPI core for that.

> Of course we cannot predict what happens in the future but is it really
> preventing merging of this patch?

Yes.  People are *way* too happy to abuse spidev in board descriptions,
especially people working in the areas these boards seem to target.
There seems to be little engagement with the abstractions involved or
interest in keeping things working well long term in more general
applications, it's all about quick hacks now.

Given the push for compatibility with DT and reuse of the existing
software stack I just don't have any confidence that the fallback path
isn't going to be implemented, and relying on anyone noticing that these
warnings are broken when it does get added (or that spidev is affected
at all) just doesn't seem wise to me.

> > I'm worried that the reason it's working fine may be that there is very
> > little usage, and that there may be more gotchas like we're already
> > seeing lurking about.

> With more usage I'm sure we can improve things. After all the end goal
> is to let single driver to work on both DT and ACPI systems with minimal
> changes. If you think the current implementation is flawed, nothing
> prevents you from proposing better alternative :-)

I've been suggesting that you should just use DT!

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

Re: [PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

On Thu, Jun 30, 2016 at 03:49:31PM +0300, Mika Westerberg wrote:
> On Wed, Jun 29, 2016 at 08:20:59PM +0100, Mark Brown wrote:
> > On Wed, Jun 29, 2016 at 09:51:55PM +0300, Mika Westerberg wrote:
> > > On Wed, Jun 29, 2016 at 07:31:01PM +0100, Mark Brown wrote:
> > 
> > > > And we're *sure* that's going to be maintained?  People do use the
> > > > fallback matching that DT does, I don't trust the ACPI maintainers not
> > > > to do the same thing.
> > 
> > > That fallback matching does not even work in ACPI like I told you
> > > already. We have no plans to do anything like that either. That's the
> > > reason why we complain if there is PRP0001 without compatible string.
> > 
> > No, you're completely missing the point here.  The problem is someone
> > using something like linux,spidev as a compatible string and going into
> > fallback matching on SPI bus IDs rather than compatible strings.  I just
> > don't have confidence that someone isn't going to try to add that
> > fallback path given that it's used for DT.
> 
> If someone is using "linux,spidev" as compatible string it will not
> match anything and we are not going to add any kind of fallback to the
> ACPI core for that.
> 
> Of course we cannot predict what happens in the future but is it really
> preventing merging of this patch?
> 
> What I'm simply trying to achieve is to use the existing two DT
> compatible strings from ACPI. I've tested that it works fine and
> provided you all the evidence that it does not break the check for
> misusing DT.

Actually there seems to be alternative to this.

Windows seems to have a similar spidev raw interface in their MITT test
suite here:

https://msdn.microsoft.com/fi-fi/windows/hardware/drivers/spb/spi-tests-in-mitt

It exposes three ACPI SPI devices with ACPI IDs of SPT0001, SPT0002 and
SPT0003. I'm thinking that instead of using the existing DT compatible
strings we could use these ACPI IDs in the driver.
--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Fri, Jul 01, 2016 at 01:19:12PM +0300, Mika Westerberg wrote:

> Windows seems to have a similar spidev raw interface in their MITT test
> suite here:

> https://msdn.microsoft.com/fi-fi/windows/hardware/drivers/spb/spi-tests-in-mitt

> It exposes three ACPI SPI devices with ACPI IDs of SPT0001, SPT0002 and
> SPT0003. I'm thinking that instead of using the existing DT compatible
> strings we could use these ACPI IDs in the driver.

Ugh :(  But yes, if there's existing ACPI IDs for this functionality
clearly we should support those.  Probably with an equivalent warning
about how they're only for non-production systems.

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

Re: [PATCH RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mika Westerberg]

On Fri, Jul 01, 2016 at 04:50:35PM +0200, Mark Brown wrote:
> On Fri, Jul 01, 2016 at 01:19:12PM +0300, Mika Westerberg wrote:
> 
> > Windows seems to have a similar spidev raw interface in their MITT test
> > suite here:
> 
> > https://msdn.microsoft.com/fi-fi/windows/hardware/drivers/spb/spi-tests-in-mitt
> 
> > It exposes three ACPI SPI devices with ACPI IDs of SPT0001, SPT0002 and
> > SPT0003. I'm thinking that instead of using the existing DT compatible
> > strings we could use these ACPI IDs in the driver.
> 
> Ugh :(  But yes, if there's existing ACPI IDs for this functionality
> clearly we should support those.  Probably with an equivalent warning
> about how they're only for non-production systems.

OK, good :)

Let me explain some background why I'm doing this. Maybe it brings
better alternatives.

There are these boards for Makers and IoT stuff which basically have pin
header where you can connect different low speed peripherals, like
sensors and so on. The main point is that you don't always know
beforehand what devices will be connected to the board. Now, it seems
that IoT/Maker folks solved this in userspace so that they are not using
existing drivers provided by Linux kernel but instead they are using raw
access to buses like I2C and SPI, and provide their own "drivers" for
those peripherals.

For I2C it is easy because we have i2c-dev and it does not require any
kind of firmware support. For SPI we need to be more careful because of
bus signals like chip selects. Instead of writing out-of-tree board files
to provide proper configuration for SPI (spidev) we can at least try to
take advantage of the boot firmware, like ACPI which already has
a way to describe devices connected to SPI bus.

Now, since spidev is just Linux software abstraction for raw access to
the SPI bus I don't think it is good idea to allocate special ACPI ID
just for that - it does not describe hardware. So instead I'm thinking
we could re-use those Windows ACPI IDs in the driver.

With this we can either stick these devices with the boot firmware
shipping with boards or alternatively provide overlays which can be
loaded to the existing firmware as needed. Users of these boards can
then take mainline Linux and use whatever existing IoT userspace
components.

I'm going to prepare a new patch adding these ACPI IDs to the spidev
early next week.
--
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 RESEND] spi: spidev: Allow matching DT compatible strings from ACPI

[Mark Brown]

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

On Sat, Jul 02, 2016 at 12:01:14PM +0300, Mika Westerberg wrote:

> There are these boards for Makers and IoT stuff which basically have pin
> header where you can connect different low speed peripherals, like
> sensors and so on. The main point is that you don't always know
> beforehand what devices will be connected to the board. Now, it seems

Sure, that's where most of this crap comes from.

> that IoT/Maker folks solved this in userspace so that they are not using
> existing drivers provided by Linux kernel but instead they are using raw
> access to buses like I2C and SPI, and provide their own "drivers" for
> those peripherals.

...which is of course a terrible solution which results in a huge amount
of wheel reinvention and effort duplication.  The best practice here is
to use overlays, there's people doing active work on standardizing
connectors for DT at the minute which will help a lot there but
engagement from the maker community in general seems depressingly poor
even though some of this stuff ends up getting productized.
Unfortunately you're going to have to redo all the overlay work for ACPI
to use it on x86.

What would be really nice would be if the schematic capture tools were
able to output DT overlays automatically, that'd make it really simple
to design an add on board/circuit and get the software support up with
no bother.

> Now, since spidev is just Linux software abstraction for raw access to
> the SPI bus I don't think it is good idea to allocate special ACPI ID
> just for that - it does not describe hardware. So instead I'm thinking
> we could re-use those Windows ACPI IDs in the driver.

Yes, exactly - spidev should never appear directly in ACPI or DT since
our ideas about the best way to control the hardware may change.

> With this we can either stick these devices with the boot firmware
> shipping with boards or alternatively provide overlays which can be
> loaded to the existing firmware as needed. Users of these boards can
> then take mainline Linux and use whatever existing IoT userspace
> components.

It really needs to be overlays, not everyone using these boards is doing
the roll your own driver stuff - they also get used by people developing
more substantial add on modules for example, or people prototyping more
real systems who want normal software support.

> I'm going to prepare a new patch adding these ACPI IDs to the spidev
> early next week.

Please also ensure that we complain loudly when we use them like we do
for spidev.  We shouldn't be encouraging people to develop software like
this.

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