[v1,3/6] spi: pxa2xx: Remove no more needed PCI ID table

Message ID 20221017171243.57078-3-andriy.shevchenko@linux.intel.com
State New
Headers
Series [v1,1/6] spi: pxa2xx: Simplify with devm_platform_get_and_ioremap_resource() |

Commit Message

Andy Shevchenko Oct. 17, 2022, 5:12 p.m. UTC
  Since the PCI enumerated devices provide a property with SSP type,
there is no more necessity to bear the copy of the ID table here.
Remove it for good.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx.c | 117 +--------------------------------------
 1 file changed, 2 insertions(+), 115 deletions(-)
  

Comments

Mark Brown Oct. 17, 2022, 5:18 p.m. UTC | #1
On Mon, Oct 17, 2022 at 08:12:40PM +0300, Andy Shevchenko wrote:

> Since the PCI enumerated devices provide a property with SSP type,
> there is no more necessity to bear the copy of the ID table here.
> Remove it for good.

They do?  How?  Are you sure that this is true for all existing devices?
  
Andy Shevchenko Oct. 17, 2022, 5:35 p.m. UTC | #2
On Mon, Oct 17, 2022 at 06:18:38PM +0100, Mark Brown wrote:
> On Mon, Oct 17, 2022 at 08:12:40PM +0300, Andy Shevchenko wrote:
> 
> > Since the PCI enumerated devices provide a property with SSP type,
> > there is no more necessity to bear the copy of the ID table here.
> > Remove it for good.
> 
> They do?  How?  Are you sure that this is true for all existing devices?

Currently the board code assures that the property is always there for all
existing devices.
  
Mark Brown Oct. 17, 2022, 5:39 p.m. UTC | #3
On Mon, Oct 17, 2022 at 08:35:16PM +0300, Andy Shevchenko wrote:
> On Mon, Oct 17, 2022 at 06:18:38PM +0100, Mark Brown wrote:
> > On Mon, Oct 17, 2022 at 08:12:40PM +0300, Andy Shevchenko wrote:

> > > Since the PCI enumerated devices provide a property with SSP type,
> > > there is no more necessity to bear the copy of the ID table here.
> > > Remove it for good.

> > They do?  How?  Are you sure that this is true for all existing devices?

> Currently the board code assures that the property is always there for all
> existing devices.

Which board code is this?  The names of the new properties you're adding
is really not at all idiomatic for ACPI and this is pretty old code so
it's surprising that there's not existing systems that don't have this
in their BIOSs.
  
Andy Shevchenko Oct. 17, 2022, 5:41 p.m. UTC | #4
On Mon, Oct 17, 2022 at 06:39:19PM +0100, Mark Brown wrote:
> On Mon, Oct 17, 2022 at 08:35:16PM +0300, Andy Shevchenko wrote:
> > On Mon, Oct 17, 2022 at 06:18:38PM +0100, Mark Brown wrote:
> > > On Mon, Oct 17, 2022 at 08:12:40PM +0300, Andy Shevchenko wrote:
> 
> > > > Since the PCI enumerated devices provide a property with SSP type,
> > > > there is no more necessity to bear the copy of the ID table here.
> > > > Remove it for good.
> 
> > > They do?  How?  Are you sure that this is true for all existing devices?
> 
> > Currently the board code assures that the property is always there for all
> > existing devices.
> 
> Which board code is this?  The names of the new properties you're adding
> is really not at all idiomatic for ACPI and this is pretty old code so
> it's surprising that there's not existing systems that don't have this
> in their BIOSs.

drivers/mfd/intel-lpss-pci.c.
  
Mark Brown Oct. 17, 2022, 5:42 p.m. UTC | #5
On Mon, Oct 17, 2022 at 06:39:24PM +0100, Mark Brown wrote:
> On Mon, Oct 17, 2022 at 08:35:16PM +0300, Andy Shevchenko wrote:

> > Currently the board code assures that the property is always there for all
> > existing devices.

> Which board code is this?  The names of the new properties you're adding
> is really not at all idiomatic for ACPI and this is pretty old code so
> it's surprising that there's not existing systems that don't have this
> in their BIOSs.

Incidentally stuff like this is part of why you should write cover
letters for your serieses explaining what they're up to and/or provide
much more detailed information on what's going on in your patch
descriptions.
  
Mark Brown Oct. 18, 2022, 11:42 a.m. UTC | #6
On Mon, Oct 17, 2022 at 08:41:24PM +0300, Andy Shevchenko wrote:
> On Mon, Oct 17, 2022 at 06:39:19PM +0100, Mark Brown wrote:

> > Which board code is this?  The names of the new properties you're adding
> > is really not at all idiomatic for ACPI and this is pretty old code so
> > it's surprising that there's not existing systems that don't have this
> > in their BIOSs.

> drivers/mfd/intel-lpss-pci.c.

OK, so this is another push for device properties for passing stuff
internally.  Please resubmit this series with descriptions of why this
is being done - I really can't tell what the benefit is here in concrete
terms, you say it somehow improves identification of which variant is in
use but don't articulate specifically why.

You should probably also restructure the code interpreting the device
IDs so that it's very clear that unknown values are handled well, this
would split things between multiple subsystems and right now the code is
a bit fragile.
  
Andy Shevchenko Oct. 19, 2022, 3:06 p.m. UTC | #7
On Tue, Oct 18, 2022 at 12:42:03PM +0100, Mark Brown wrote:
> On Mon, Oct 17, 2022 at 08:41:24PM +0300, Andy Shevchenko wrote:
> > On Mon, Oct 17, 2022 at 06:39:19PM +0100, Mark Brown wrote:

Thank you for your review!

> > > Which board code is this?  The names of the new properties you're adding
> > > is really not at all idiomatic for ACPI and this is pretty old code so
> > > it's surprising that there's not existing systems that don't have this
> > > in their BIOSs.
> 
> > drivers/mfd/intel-lpss-pci.c.
> 
> OK, so this is another push for device properties for passing stuff
> internally.  Please resubmit this series with descriptions of why this
> is being done - I really can't tell what the benefit is here in concrete
> terms, you say it somehow improves identification of which variant is in
> use but don't articulate specifically why.

I have sent a v2.

> You should probably also restructure the code interpreting the device
> IDs so that it's very clear that unknown values are handled well, this
> would split things between multiple subsystems and right now the code is
> a bit fragile.

I'm not sure how better to do this. Any example?
  
Mark Brown Oct. 19, 2022, 3:50 p.m. UTC | #8
On Wed, Oct 19, 2022 at 06:06:04PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 18, 2022 at 12:42:03PM +0100, Mark Brown wrote:

> > You should probably also restructure the code interpreting the device
> > IDs so that it's very clear that unknown values are handled well, this
> > would split things between multiple subsystems and right now the code is
> > a bit fragile.

> I'm not sure how better to do this. Any example?

For example a check that the ID is one we know about.  IIRC that bit of
context looked like a tree of if statements with no particular
validation.
  
Andy Shevchenko Oct. 20, 2022, 4:18 p.m. UTC | #9
On Wed, Oct 19, 2022 at 04:50:38PM +0100, Mark Brown wrote:
> On Wed, Oct 19, 2022 at 06:06:04PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 18, 2022 at 12:42:03PM +0100, Mark Brown wrote:
> 
> > > You should probably also restructure the code interpreting the device
> > > IDs so that it's very clear that unknown values are handled well, this
> > > would split things between multiple subsystems and right now the code is
> > > a bit fragile.
> 
> > I'm not sure how better to do this. Any example?
> 
> For example a check that the ID is one we know about.  IIRC that bit of
> context looked like a tree of if statements with no particular
> validation.

But isn't it guaranteed to be handled by device core, i.e. we won't get driver
even enumerated if ID is unknown to us.
  
Mark Brown Oct. 20, 2022, 4:25 p.m. UTC | #10
On Thu, Oct 20, 2022 at 07:18:23PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 19, 2022 at 04:50:38PM +0100, Mark Brown wrote:

> > For example a check that the ID is one we know about.  IIRC that bit of
> > context looked like a tree of if statements with no particular
> > validation.

> But isn't it guaranteed to be handled by device core, i.e. we won't get driver
> even enumerated if ID is unknown to us.

That's true currently since you're matching based on ACPI ID and then
have the lookup done with the ID information in the acpi_device_id table
but IIRC the patch was replacing that with some device property stuff.
  
Andy Shevchenko Oct. 20, 2022, 4:42 p.m. UTC | #11
On Thu, Oct 20, 2022 at 05:25:15PM +0100, Mark Brown wrote:
> On Thu, Oct 20, 2022 at 07:18:23PM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 19, 2022 at 04:50:38PM +0100, Mark Brown wrote:
> 
> > > For example a check that the ID is one we know about.  IIRC that bit of
> > > context looked like a tree of if statements with no particular
> > > validation.
> 
> > But isn't it guaranteed to be handled by device core, i.e. we won't get driver
> > even enumerated if ID is unknown to us.
> 
> That's true currently since you're matching based on ACPI ID and then
> have the lookup done with the ID information in the acpi_device_id table
> but IIRC the patch was replacing that with some device property stuff.

But that one also based on the IDs, it's not assigned without real IDs of
the devices on the certain platforms. I don't see how it's different in
this sense.
  
Mark Brown Oct. 20, 2022, 4:58 p.m. UTC | #12
On Thu, Oct 20, 2022 at 07:42:09PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 20, 2022 at 05:25:15PM +0100, Mark Brown wrote:

> > That's true currently since you're matching based on ACPI ID and then
> > have the lookup done with the ID information in the acpi_device_id table
> > but IIRC the patch was replacing that with some device property stuff.

> But that one also based on the IDs, it's not assigned without real IDs of
> the devices on the certain platforms. I don't see how it's different in
> this sense.

The driver won't even match and therefore load if it doesn't have a
lookup for the device with the current code, the type code comes from
the match.  If it has to go querying a device property then the driver
can load but end up with a device property it hasn't ever heard of and
end up misbehaving as a result.
  
Andy Shevchenko Oct. 20, 2022, 5:03 p.m. UTC | #13
On Thu, Oct 20, 2022 at 05:58:39PM +0100, Mark Brown wrote:
> On Thu, Oct 20, 2022 at 07:42:09PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 20, 2022 at 05:25:15PM +0100, Mark Brown wrote:
> 
> > > That's true currently since you're matching based on ACPI ID and then
> > > have the lookup done with the ID information in the acpi_device_id table
> > > but IIRC the patch was replacing that with some device property stuff.
> 
> > But that one also based on the IDs, it's not assigned without real IDs of
> > the devices on the certain platforms. I don't see how it's different in
> > this sense.
> 
> The driver won't even match and therefore load if it doesn't have a
> lookup for the device with the current code, the type code comes from
> the match.  If it has to go querying a device property then the driver
> can load but end up with a device property it hasn't ever heard of and
> end up misbehaving as a result.

That's how all MFD devices work nowadays, right? What's so special about
this driver? It's being used as a child by MFD. If what you are telling
is a real concern, we have to have a way to assure that all drivers that
are children of the MFDs should provide a match. IIRC there is no such
mechanism exists in the kernel these days.
  
Mark Brown Oct. 20, 2022, 5:26 p.m. UTC | #14
On Thu, Oct 20, 2022 at 08:03:37PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 20, 2022 at 05:58:39PM +0100, Mark Brown wrote:

> > The driver won't even match and therefore load if it doesn't have a
> > lookup for the device with the current code, the type code comes from
> > the match.  If it has to go querying a device property then the driver
> > can load but end up with a device property it hasn't ever heard of and
> > end up misbehaving as a result.

> That's how all MFD devices work nowadays, right? What's so special about
> this driver? It's being used as a child by MFD. If what you are telling
> is a real concern, we have to have a way to assure that all drivers that
> are children of the MFDs should provide a match. IIRC there is no such
> mechanism exists in the kernel these days.

Most of the MFDs don't actually have multiple options for a given child
driver, and it's common where there are multiple options to either bind
with a different name representing the different child device or have
something that looks like a switch statement for the IDs which will
error out if it hits an ID that's not one the driver knows about (like
spi-pxa2xx-pci does with lpss_spi_setup()).
  
Andy Shevchenko Oct. 20, 2022, 5:41 p.m. UTC | #15
On Thu, Oct 20, 2022 at 06:26:01PM +0100, Mark Brown wrote:
> On Thu, Oct 20, 2022 at 08:03:37PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 20, 2022 at 05:58:39PM +0100, Mark Brown wrote:
> 
> > > The driver won't even match and therefore load if it doesn't have a
> > > lookup for the device with the current code, the type code comes from
> > > the match.  If it has to go querying a device property then the driver
> > > can load but end up with a device property it hasn't ever heard of and
> > > end up misbehaving as a result.
> 
> > That's how all MFD devices work nowadays, right? What's so special about
> > this driver? It's being used as a child by MFD. If what you are telling
> > is a real concern, we have to have a way to assure that all drivers that
> > are children of the MFDs should provide a match. IIRC there is no such
> > mechanism exists in the kernel these days.
> 
> Most of the MFDs don't actually have multiple options for a given child
> driver, and it's common where there are multiple options to either bind
> with a different name representing the different child device or have
> something that looks like a switch statement for the IDs which will
> error out if it hits an ID that's not one the driver knows about (like
> spi-pxa2xx-pci does with lpss_spi_setup()).

Okay, would it work for you if we check the named resource and only if it's
found take a property? In such case we can guarantee (AFAICS) that the 3rd
parties (like unknown firmware) won't mess up with the driver.
  
Mark Brown Oct. 20, 2022, 5:45 p.m. UTC | #16
On Thu, Oct 20, 2022 at 08:41:43PM +0300, Andy Shevchenko wrote:

> Okay, would it work for you if we check the named resource and only if it's
> found take a property? In such case we can guarantee (AFAICS) that the 3rd
> parties (like unknown firmware) won't mess up with the driver.

Not sure I quite get what you're proposing here but I *think* so,
assuming you mean checking the values if the property is present (and
error out if the property isn't there at all and you're instantiating
via a MFD rather than direct PCI/DT binding I guess)?
  
Andy Shevchenko Oct. 20, 2022, 5:55 p.m. UTC | #17
On Thu, Oct 20, 2022 at 06:45:19PM +0100, Mark Brown wrote:
> On Thu, Oct 20, 2022 at 08:41:43PM +0300, Andy Shevchenko wrote:
> 
> > Okay, would it work for you if we check the named resource and only if it's
> > found take a property? In such case we can guarantee (AFAICS) that the 3rd
> > parties (like unknown firmware) won't mess up with the driver.
> 
> Not sure I quite get what you're proposing here but I *think* so,
> assuming you mean checking the values if the property is present (and
> error out if the property isn't there at all and you're instantiating
> via a MFD rather than direct PCI/DT binding I guess)?


When we instantiate via MFD, we (semi-)manually create resources for each of
the children. These resources may or may not have a dedicated names. Those
names can be given _only_ inside the source code in the kernel, so it means
it is _explicit_ telling, that we are know where the device in question comes
from.


In the code it will be like


	if (resource_with_name_present()) {
		ret = device_property_...
		if (ret)
			return ERROR "No mandatory property provided";
	}

Like you said, checking property only when we have resource present _by name_
and bail out if there is none.
  
Mark Brown Oct. 20, 2022, 6:07 p.m. UTC | #18
On Thu, Oct 20, 2022 at 08:55:02PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 20, 2022 at 06:45:19PM +0100, Mark Brown wrote:

> > Not sure I quite get what you're proposing here but I *think* so,
> > assuming you mean checking the values if the property is present (and
> > error out if the property isn't there at all and you're instantiating
> > via a MFD rather than direct PCI/DT binding I guess)?

> When we instantiate via MFD, we (semi-)manually create resources for each of
> the children. These resources may or may not have a dedicated names. Those
> names can be given _only_ inside the source code in the kernel, so it means
> it is _explicit_ telling, that we are know where the device in question comes
> from.

> 	if (resource_with_name_present()) {
> 		ret = device_property_...

> Like you said, checking property only when we have resource present _by name_
> and bail out if there is none.

Remember that device_property backs onto fwnode so properties can come
from _DSD properties too since fwnode will query any source of
properties (and further remember that things will be going through
multiple trees so even with stuff purely in the kernel things could get
out of sync).  I think the code would have to also check that it was a
MFD child at least, you couldn't get _DSD on a child node so that should
be fine.
  
Andy Shevchenko Oct. 20, 2022, 6:19 p.m. UTC | #19
On Thu, Oct 20, 2022 at 07:07:10PM +0100, Mark Brown wrote:
> On Thu, Oct 20, 2022 at 08:55:02PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 20, 2022 at 06:45:19PM +0100, Mark Brown wrote:
> 
> > > Not sure I quite get what you're proposing here but I *think* so,
> > > assuming you mean checking the values if the property is present (and
> > > error out if the property isn't there at all and you're instantiating
> > > via a MFD rather than direct PCI/DT binding I guess)?
> 
> > When we instantiate via MFD, we (semi-)manually create resources for each of
> > the children. These resources may or may not have a dedicated names. Those
> > names can be given _only_ inside the source code in the kernel, so it means
> > it is _explicit_ telling, that we are know where the device in question comes
> > from.
> 
> > 	if (resource_with_name_present()) {
> > 		ret = device_property_...
> 
> > Like you said, checking property only when we have resource present _by name_
> > and bail out if there is none.
> 
> Remember that device_property backs onto fwnode so properties can come
> from _DSD properties too since fwnode will query any source of
> properties (and further remember that things will be going through
> multiple trees so even with stuff purely in the kernel things could get
> out of sync).

> I think the code would have to also check that it was a
> MFD child at least,

That's exactly what I'm talking about when said "named resource check".

> you couldn't get _DSD on a child node so that should
> be fine.

So, I guess we settled down this. I'll prepare v4 with the discussed changes.
  
Mark Brown Oct. 21, 2022, 10:42 a.m. UTC | #20
On Thu, Oct 20, 2022 at 09:19:06PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 20, 2022 at 07:07:10PM +0100, Mark Brown wrote:

> > Remember that device_property backs onto fwnode so properties can come
> > from _DSD properties too since fwnode will query any source of

> > I think the code would have to also check that it was a
> > MFD child at least,

> That's exactly what I'm talking about when said "named resource check".

Like I say a property can come from any firmware interface.
  
Andy Shevchenko Oct. 21, 2022, 10:51 a.m. UTC | #21
On Fri, Oct 21, 2022 at 11:42:31AM +0100, Mark Brown wrote:
> On Thu, Oct 20, 2022 at 09:19:06PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 20, 2022 at 07:07:10PM +0100, Mark Brown wrote:
> 
> > > Remember that device_property backs onto fwnode so properties can come
> > > from _DSD properties too since fwnode will query any source of
> 
> > > I think the code would have to also check that it was a
> > > MFD child at least,
> 
> > That's exactly what I'm talking about when said "named resource check".
> 
> Like I say a property can come from any firmware interface.

But I'm talking about resource (not a property) as IO memory. It doesn't come
via firmware at all. Have you had a chance to look into the v4?
  
Mark Brown Oct. 21, 2022, 10:59 a.m. UTC | #22
On Fri, Oct 21, 2022 at 01:51:47PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 21, 2022 at 11:42:31AM +0100, Mark Brown wrote:

> > > That's exactly what I'm talking about when said "named resource check".

> > Like I say a property can come from any firmware interface.

> But I'm talking about resource (not a property) as IO memory. It doesn't come
> via firmware at all. Have you had a chance to look into the v4?

On DT based systems resources can be named by the firmware, I don't know
if that's possible with ACPI but as the name suggests the driver gets
used on PXA systems too.
  
Andy Shevchenko Oct. 21, 2022, 11:15 a.m. UTC | #23
On Fri, Oct 21, 2022 at 11:59:20AM +0100, Mark Brown wrote:
> On Fri, Oct 21, 2022 at 01:51:47PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 21, 2022 at 11:42:31AM +0100, Mark Brown wrote:
> 
> > > > That's exactly what I'm talking about when said "named resource check".
> 
> > > Like I say a property can come from any firmware interface.
> 
> > But I'm talking about resource (not a property) as IO memory. It doesn't come
> > via firmware at all. Have you had a chance to look into the v4?
> 
> On DT based systems resources can be named by the firmware, I don't know
> if that's possible with ACPI but as the name suggests the driver gets
> used on PXA systems too.

And how is it related to DT if the enumeration happens via platform driver
code? As for PXA this is all comes via board files:

$ git grep -n -w '"pxa2xx-spi"'
Documentation/spi/pxa2xx.rst:66:        .name = "pxa2xx-spi", /* MUST BE THIS VALUE, so device match driver */
arch/arm/mach-pxa/devices.c:1082:       pd = platform_device_alloc("pxa2xx-spi", id);
arch/arm/mach-pxa/icontrol.c:127:       .name          = "pxa2xx-spi",
arch/arm/mach-pxa/icontrol.c:135:       .name          = "pxa2xx-spi",
drivers/mfd/intel-lpss.c:123:   .name = "pxa2xx-spi",
drivers/spi/spi-pxa2xx-pci.c:298:       pi.name = "pxa2xx-spi";
drivers/spi/spi-pxa2xx.c:1765:          .name   = "pxa2xx-spi",

In the current code and after my patch series the priority is that
the driver data from the spi-pxa2xx.c is the first. So, if compatible
(which is by fact the only "marvell,mmp2-ssp") has named resources
that exactly the same as LPSS for MFD, nothing will change the driver
behaviour.

For the ACPI there is no names for the resources so far.
  
Mark Brown Oct. 21, 2022, 12:28 p.m. UTC | #24
On Fri, Oct 21, 2022 at 02:15:59PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 21, 2022 at 11:59:20AM +0100, Mark Brown wrote:

> > On DT based systems resources can be named by the firmware, I don't know
> > if that's possible with ACPI but as the name suggests the driver gets
> > used on PXA systems too.

> And how is it related to DT if the enumeration happens via platform driver
> code? As for PXA this is all comes via board files:

...

> In the current code and after my patch series the priority is that
> the driver data from the spi-pxa2xx.c is the first. So, if compatible
> (which is by fact the only "marvell,mmp2-ssp") has named resources
> that exactly the same as LPSS for MFD, nothing will change the driver
> behaviour.

> For the ACPI there is no names for the resources so far.

It's not so much does this work now as will this clearly work in future
when someone changes something, and will any changes that are concerning
be likely to set off alarm bells.  I'm sure it works fine now.

BTW the new property isn't added to the binding document, though this
case is a bit iffy given that the intent is apparently that it not be
added to the document since this is basically working around the issues
with ACPI not being terribly descriptive, the property is very much not
idiomatic for DT.  Having it in the binding document might actually end
up being an issue - from that point of view it'd be good if we had a
namespace for things that should never appear in DT that didn't look
like a DT namespace.
  
Andy Shevchenko Oct. 21, 2022, 12:46 p.m. UTC | #25
On Fri, Oct 21, 2022 at 01:28:52PM +0100, Mark Brown wrote:
> On Fri, Oct 21, 2022 at 02:15:59PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 21, 2022 at 11:59:20AM +0100, Mark Brown wrote:
> 
> > > On DT based systems resources can be named by the firmware, I don't know
> > > if that's possible with ACPI but as the name suggests the driver gets
> > > used on PXA systems too.
> 
> > And how is it related to DT if the enumeration happens via platform driver
> > code? As for PXA this is all comes via board files:

...

> > In the current code and after my patch series the priority is that
> > the driver data from the spi-pxa2xx.c is the first. So, if compatible
> > (which is by fact the only "marvell,mmp2-ssp") has named resources
> > that exactly the same as LPSS for MFD, nothing will change the driver
> > behaviour.
> 
> > For the ACPI there is no names for the resources so far.
> 
> It's not so much does this work now as will this clearly work in future
> when someone changes something, and will any changes that are concerning
> be likely to set off alarm bells.  I'm sure it works fine now.

But how? If in the future ones added a board file -- it will be visible
immediately via `git grep` and hence can be fixed.

In case it's a DT enumerated, we are the ones who will see it in the compatible
list in the spi-pxa2xx.c and there the type is a requirement. So, can you point
out what did I miss?

> BTW the new property isn't added to the binding document, though this
> case is a bit iffy given that the intent is apparently that it not be
> added to the document since this is basically working around the issues
> with ACPI not being terribly descriptive, the property is very much not
> idiomatic for DT.  Having it in the binding document might actually end
> up being an issue - from that point of view it'd be good if we had a
> namespace for things that should never appear in DT that didn't look
> like a DT namespace.

Again, if it appears in the DT, there is no way we noticed the behavioral
change due to priorities of the data we try in the driver. Any change
there will be immediately questioned by reviewers, right?
  

Patch

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 857732a54ca7..1d36d055a9d6 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -20,7 +20,6 @@ 
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/of.h>
-#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
@@ -1335,121 +1334,17 @@  static const struct acpi_device_id pxa2xx_spi_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, pxa2xx_spi_acpi_match);
 #endif
 
-/*
- * PCI IDs of compound devices that integrate both host controller and private
- * integrated DMA engine. Please note these are not used in module
- * autoloading and probing in this module but matching the LPSS SSP type.
- */
-static const struct pci_device_id pxa2xx_spi_pci_compound_match[] = {
-	/* SPT-LP */
-	{ PCI_VDEVICE(INTEL, 0x9d29), LPSS_SPT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x9d2a), LPSS_SPT_SSP },
-	/* SPT-H */
-	{ PCI_VDEVICE(INTEL, 0xa129), LPSS_SPT_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa12a), LPSS_SPT_SSP },
-	/* KBL-H */
-	{ PCI_VDEVICE(INTEL, 0xa2a9), LPSS_SPT_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa2aa), LPSS_SPT_SSP },
-	/* CML-V */
-	{ PCI_VDEVICE(INTEL, 0xa3a9), LPSS_SPT_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa3aa), LPSS_SPT_SSP },
-	/* BXT A-Step */
-	{ PCI_VDEVICE(INTEL, 0x0ac2), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x0ac4), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x0ac6), LPSS_BXT_SSP },
-	/* BXT B-Step */
-	{ PCI_VDEVICE(INTEL, 0x1ac2), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x1ac4), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x1ac6), LPSS_BXT_SSP },
-	/* GLK */
-	{ PCI_VDEVICE(INTEL, 0x31c2), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x31c4), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x31c6), LPSS_BXT_SSP },
-	/* ICL-LP */
-	{ PCI_VDEVICE(INTEL, 0x34aa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x34ab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x34fb), LPSS_CNL_SSP },
-	/* EHL */
-	{ PCI_VDEVICE(INTEL, 0x4b2a), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x4b2b), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x4b37), LPSS_BXT_SSP },
-	/* JSL */
-	{ PCI_VDEVICE(INTEL, 0x4daa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x4dab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x4dfb), LPSS_CNL_SSP },
-	/* TGL-H */
-	{ PCI_VDEVICE(INTEL, 0x43aa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x43ab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x43fb), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x43fd), LPSS_CNL_SSP },
-	/* ADL-P */
-	{ PCI_VDEVICE(INTEL, 0x51aa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x51ab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x51fb), LPSS_CNL_SSP },
-	/* ADL-M */
-	{ PCI_VDEVICE(INTEL, 0x54aa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x54ab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x54fb), LPSS_CNL_SSP },
-	/* APL */
-	{ PCI_VDEVICE(INTEL, 0x5ac2), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x5ac4), LPSS_BXT_SSP },
-	{ PCI_VDEVICE(INTEL, 0x5ac6), LPSS_BXT_SSP },
-	/* RPL-S */
-	{ PCI_VDEVICE(INTEL, 0x7a2a), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x7a2b), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x7a79), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x7a7b), LPSS_CNL_SSP },
-	/* ADL-S */
-	{ PCI_VDEVICE(INTEL, 0x7aaa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x7aab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x7af9), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x7afb), LPSS_CNL_SSP },
-	/* MTL-P */
-	{ PCI_VDEVICE(INTEL, 0x7e27), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x7e30), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x7e46), LPSS_CNL_SSP },
-	/* CNL-LP */
-	{ PCI_VDEVICE(INTEL, 0x9daa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x9dab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x9dfb), LPSS_CNL_SSP },
-	/* CNL-H */
-	{ PCI_VDEVICE(INTEL, 0xa32a), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa32b), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa37b), LPSS_CNL_SSP },
-	/* CML-LP */
-	{ PCI_VDEVICE(INTEL, 0x02aa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x02ab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x02fb), LPSS_CNL_SSP },
-	/* CML-H */
-	{ PCI_VDEVICE(INTEL, 0x06aa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x06ab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0x06fb), LPSS_CNL_SSP },
-	/* TGL-LP */
-	{ PCI_VDEVICE(INTEL, 0xa0aa), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa0ab), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa0de), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa0df), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa0fb), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa0fd), LPSS_CNL_SSP },
-	{ PCI_VDEVICE(INTEL, 0xa0fe), LPSS_CNL_SSP },
-	{ },
-};
-
 static const struct of_device_id pxa2xx_spi_of_match[] = {
 	{ .compatible = "marvell,mmp2-ssp", .data = (void *)MMP2_SSP },
 	{},
 };
 MODULE_DEVICE_TABLE(of, pxa2xx_spi_of_match);
 
-#ifdef CONFIG_PCI
-
 static bool pxa2xx_spi_idma_filter(struct dma_chan *chan, void *param)
 {
 	return param == chan->device->dev;
 }
 
-#endif /* CONFIG_PCI */
-
 static struct pxa2xx_spi_controller *
 pxa2xx_spi_init_pdata(struct platform_device *pdev)
 {
@@ -1458,17 +1353,12 @@  pxa2xx_spi_init_pdata(struct platform_device *pdev)
 	struct device *parent = dev->parent;
 	struct ssp_device *ssp;
 	struct resource *res;
-	struct pci_dev *pcidev = dev_is_pci(parent) ? to_pci_dev(parent) : NULL;
-	const struct pci_device_id *pcidev_id = NULL;
 	u32 value = SSP_UNDEFINED;
 	enum pxa_ssp_type type;
 	const void *match;
 	int status;
 	u64 uid;
 
-	if (pcidev)
-		pcidev_id = pci_match_id(pxa2xx_spi_pci_compound_match, pcidev);
-
 	/* Always try to read property */
 	device_property_read_u32(&pdev->dev, "intel,spi-pxa2xx-type", &value);
 
@@ -1477,8 +1367,6 @@  pxa2xx_spi_init_pdata(struct platform_device *pdev)
 		type = (enum pxa_ssp_type)match;
 	else if (value > SSP_UNDEFINED && value < SSP_MAX)
 		type = (enum pxa_ssp_type)value;
-	else if (pcidev_id)
-		type = (enum pxa_ssp_type)pcidev_id->driver_data;
 	else
 		return ERR_PTR(-EINVAL);
 
@@ -1494,13 +1382,12 @@  pxa2xx_spi_init_pdata(struct platform_device *pdev)
 
 	ssp->phys_base = res->start;
 
-#ifdef CONFIG_PCI
-	if (pcidev_id) {
+	/* Platforms with iDMA 64-bit */
+	if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpss_priv")) {
 		pdata->tx_param = parent;
 		pdata->rx_param = parent;
 		pdata->dma_filter = pxa2xx_spi_idma_filter;
 	}
-#endif
 
 	ssp->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(ssp->clk))