| Message ID | 20221017171243.57078-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/6] spi: pxa2xx: Simplify with devm_platform_get_and_ioremap_resource() | expand |
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?
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.
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.
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.
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.
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.
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?
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.
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.
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.
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.
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.
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.
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()).
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.
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)?
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.
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.
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.
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.
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?
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.
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.
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.
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?
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))
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(-)