[PATCH v1 1/6] spi: pxa2xx: Simplify with devm_platform_get_and_ioremap_resource()

[PATCH v1 1/6] spi: pxa2xx: Simplify with devm_platform_get_and_ioremap_resource()

[Andy Shevchenko]

Convert platform_get_resource(), devm_ioremap_resource() to a single
call to devm_platform_get_and_ioremap_resource(), as this is exactly
what this function does.

No functional changes.

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

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 2bf21c2e7a52..03ed6d4a14cd 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1482,8 +1482,7 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 
 	ssp = &pdata->ssp;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	ssp->mmio_base = devm_ioremap_resource(&pdev->dev, res);
+	ssp->mmio_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(ssp->mmio_base))
 		return ERR_CAST(ssp->mmio_base);
 
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/6] spi: pxa2xx: Respect Intel SSP type given by a property

[Andy Shevchenko]

Allow to set the Intel SSP type by reading the property.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx.c   | 6 ++++++
 include/linux/pxa2xx_ssp.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 03ed6d4a14cd..857732a54ca7 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1460,6 +1460,7 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 	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;
@@ -1468,9 +1469,14 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 	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);
+
 	match = device_get_match_data(&pdev->dev);
 	if (match)
 		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
diff --git a/include/linux/pxa2xx_ssp.h b/include/linux/pxa2xx_ssp.h
index a3fec2de512f..cd1973e6ac4b 100644
--- a/include/linux/pxa2xx_ssp.h
+++ b/include/linux/pxa2xx_ssp.h
@@ -229,6 +229,7 @@ enum pxa_ssp_type {
 	LPSS_SPT_SSP,
 	LPSS_BXT_SSP,
 	LPSS_CNL_SSP,
+	SSP_MAX
 };
 
 struct ssp_device {
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/6] spi: pxa2xx: Respect Intel SSP type given by a property

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 297 bytes --]

On Mon, Oct 17, 2022 at 08:12:39PM +0300, Andy Shevchenko wrote:

> Allow to set the Intel SSP type by reading the property.

> +	/* Always try to read property */
> +	device_property_read_u32(&pdev->dev, "intel,spi-pxa2xx-type", &value);
> +

What is the advantage of pushing this into firmware?

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/6] spi: pxa2xx: Respect Intel SSP type given by a property

[Andy Shevchenko]

On Mon, Oct 17, 2022 at 06:17:36PM +0100, Mark Brown wrote:
> On Mon, Oct 17, 2022 at 08:12:39PM +0300, Andy Shevchenko wrote:
> 
> > Allow to set the Intel SSP type by reading the property.
> 
> > +	/* Always try to read property */
> > +	device_property_read_u32(&pdev->dev, "intel,spi-pxa2xx-type", &value);
> 
> What is the advantage of pushing this into firmware?

To distinguish better what the controller generation is there.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 299 bytes --]

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?

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 776 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 967 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 573 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 626 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 832 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1201 bytes --]

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 547 bytes --]

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1365 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 491 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 587 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1526 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 686 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 4/6] spi: pxa2xx: Remove no more needed driver data

[Andy Shevchenko]

Since the ACPI enumerated devices provide a property with SSP type,
there is no more necessity to bear the copy of them in the ID table.
Drop the driver data in ACPI ID table.

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

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 1d36d055a9d6..31927493eeb0 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1323,12 +1323,12 @@ static void cleanup(struct spi_device *spi)
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id pxa2xx_spi_acpi_match[] = {
-	{ "INT33C0", LPSS_LPT_SSP },
-	{ "INT33C1", LPSS_LPT_SSP },
-	{ "INT3430", LPSS_LPT_SSP },
-	{ "INT3431", LPSS_LPT_SSP },
-	{ "80860F0E", LPSS_BYT_SSP },
-	{ "8086228E", LPSS_BSW_SSP },
+	{ "INT33C0" },
+	{ "INT33C1" },
+	{ "INT3430" },
+	{ "INT3431" },
+	{ "80860F0E" },
+	{ "8086228E" },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, pxa2xx_spi_acpi_match);
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 5/6] spi: pxa2xx: Move OF and ACPI ID tables closer to their user

[Andy Shevchenko]

There is no code that uses ID tables directly, except the
struct device_driver at the end of the file. Hence, move
tables closer to its user. It's always possible to access
them via pointer to a platform device.

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

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 31927493eeb0..76046612466d 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1321,25 +1321,6 @@ static void cleanup(struct spi_device *spi)
 	kfree(chip);
 }
 
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id pxa2xx_spi_acpi_match[] = {
-	{ "INT33C0" },
-	{ "INT33C1" },
-	{ "INT3430" },
-	{ "INT3431" },
-	{ "80860F0E" },
-	{ "8086228E" },
-	{ },
-};
-MODULE_DEVICE_TABLE(acpi, pxa2xx_spi_acpi_match);
-#endif
-
-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);
-
 static bool pxa2xx_spi_idma_filter(struct dma_chan *chan, void *param)
 {
 	return param == chan->device->dev;
@@ -1759,6 +1740,25 @@ static const struct dev_pm_ops pxa2xx_spi_pm_ops = {
 			   pxa2xx_spi_runtime_resume, NULL)
 };
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id pxa2xx_spi_acpi_match[] = {
+	{ "80860F0E" },
+	{ "8086228E" },
+	{ "INT33C0" },
+	{ "INT33C1" },
+	{ "INT3430" },
+	{ "INT3431" },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, pxa2xx_spi_acpi_match);
+#endif
+
+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);
+
 static struct platform_driver driver = {
 	.driver = {
 		.name	= "pxa2xx-spi",
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/6] spi: pxa2xx: Move OF and ACPI ID tables closer to their user

[Jonathan Cameron]

On Mon, 17 Oct 2022 20:12:42 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> There is no code that uses ID tables directly, except the
> struct device_driver at the end of the file. Hence, move
> tables closer to its user. It's always possible to access
> them via pointer to a platform device.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Seems sensible.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/spi/spi-pxa2xx.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index 31927493eeb0..76046612466d 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -1321,25 +1321,6 @@ static void cleanup(struct spi_device *spi)
>  	kfree(chip);
>  }
>  
> -#ifdef CONFIG_ACPI
> -static const struct acpi_device_id pxa2xx_spi_acpi_match[] = {
> -	{ "INT33C0" },
> -	{ "INT33C1" },
> -	{ "INT3430" },
> -	{ "INT3431" },
> -	{ "80860F0E" },
> -	{ "8086228E" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(acpi, pxa2xx_spi_acpi_match);
> -#endif
> -
> -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);
> -
>  static bool pxa2xx_spi_idma_filter(struct dma_chan *chan, void *param)
>  {
>  	return param == chan->device->dev;
> @@ -1759,6 +1740,25 @@ static const struct dev_pm_ops pxa2xx_spi_pm_ops = {
>  			   pxa2xx_spi_runtime_resume, NULL)
>  };
>  
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id pxa2xx_spi_acpi_match[] = {
> +	{ "80860F0E" },
> +	{ "8086228E" },
> +	{ "INT33C0" },
> +	{ "INT33C1" },
> +	{ "INT3430" },
> +	{ "INT3431" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, pxa2xx_spi_acpi_match);
> +#endif
> +
> +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);
> +
>  static struct platform_driver driver = {
>  	.driver = {
>  		.name	= "pxa2xx-spi",


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 6/6] spi: pxa2xx: Switch from PM ifdeffery to pm_ptr()

[Andy Shevchenko]

Cleaning up the driver to use pm_ptr() macro instead of ifdeffery
that makes it simpler and allows the compiler to remove those functions
if built without CONFIG_PM and CONFIG_PM_SLEEP support.

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

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 76046612466d..60cab241200b 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1680,7 +1680,6 @@ static int pxa2xx_spi_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int pxa2xx_spi_suspend(struct device *dev)
 {
 	struct driver_data *drv_data = dev_get_drvdata(dev);
@@ -1715,9 +1714,7 @@ static int pxa2xx_spi_resume(struct device *dev)
 	/* Start the queue running */
 	return spi_controller_resume(drv_data->controller);
 }
-#endif
 
-#ifdef CONFIG_PM
 static int pxa2xx_spi_runtime_suspend(struct device *dev)
 {
 	struct driver_data *drv_data = dev_get_drvdata(dev);
@@ -1732,12 +1729,10 @@ static int pxa2xx_spi_runtime_resume(struct device *dev)
 
 	return clk_prepare_enable(drv_data->ssp->clk);
 }
-#endif
 
 static const struct dev_pm_ops pxa2xx_spi_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_suspend, pxa2xx_spi_resume)
-	SET_RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend,
-			   pxa2xx_spi_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_suspend, pxa2xx_spi_resume)
+	RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend, pxa2xx_spi_runtime_resume, NULL)
 };
 
 #ifdef CONFIG_ACPI
@@ -1762,7 +1757,7 @@ MODULE_DEVICE_TABLE(of, pxa2xx_spi_of_match);
 static struct platform_driver driver = {
 	.driver = {
 		.name	= "pxa2xx-spi",
-		.pm	= &pxa2xx_spi_pm_ops,
+		.pm	= pm_ptr(&pxa2xx_spi_pm_ops),
 		.acpi_match_table = ACPI_PTR(pxa2xx_spi_acpi_match),
 		.of_match_table = of_match_ptr(pxa2xx_spi_of_match),
 	},
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 6/6] spi: pxa2xx: Switch from PM ifdeffery to pm_ptr()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 414 bytes --]

On Mon, Oct 17, 2022 at 08:12:43PM +0300, Andy Shevchenko wrote:

> Cleaning up the driver to use pm_ptr() macro instead of ifdeffery
> that makes it simpler and allows the compiler to remove those functions
> if built without CONFIG_PM and CONFIG_PM_SLEEP support.

Are you sure this works cleanly and doesn't suffer from similar problems
to of_match_ptr() when PM is disabled, leaving some unreferenced statics?

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 6/6] spi: pxa2xx: Switch from PM ifdeffery to pm_ptr()

[Andy Shevchenko]

On Mon, Oct 17, 2022 at 06:19:53PM +0100, Mark Brown wrote:
> On Mon, Oct 17, 2022 at 08:12:43PM +0300, Andy Shevchenko wrote:
> 
> > Cleaning up the driver to use pm_ptr() macro instead of ifdeffery
> > that makes it simpler and allows the compiler to remove those functions
> > if built without CONFIG_PM and CONFIG_PM_SLEEP support.
> 
> Are you sure this works cleanly and doesn't suffer from similar problems
> to of_match_ptr() when PM is disabled, leaving some unreferenced statics?

Yes, this is the trick with PTR_IF() behind it, which is not used by OF code.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 6/6] spi: pxa2xx: Switch from PM ifdeffery to pm_ptr()

[Jonathan Cameron]

On Mon, 17 Oct 2022 20:12:43 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Cleaning up the driver to use pm_ptr() macro instead of ifdeffery
> that makes it simpler and allows the compiler to remove those functions
> if built without CONFIG_PM and CONFIG_PM_SLEEP support.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
FWIW I like these - so drive by review.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I think you could change the handling of !pm_runtime_suspended()
to use pm_runtime_force_suspend() and equivalent for resume path.
I haven't checked that closely though - just looks like a typical
usecase for those functions that are hardened against some of
the corner cases that can occur in interactions between different
forms of pm.


> ---
>  drivers/spi/spi-pxa2xx.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index 76046612466d..60cab241200b 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -1680,7 +1680,6 @@ static int pxa2xx_spi_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int pxa2xx_spi_suspend(struct device *dev)
>  {
>  	struct driver_data *drv_data = dev_get_drvdata(dev);
> @@ -1715,9 +1714,7 @@ static int pxa2xx_spi_resume(struct device *dev)
>  	/* Start the queue running */
>  	return spi_controller_resume(drv_data->controller);
>  }
> -#endif
>  
> -#ifdef CONFIG_PM
>  static int pxa2xx_spi_runtime_suspend(struct device *dev)
>  {
>  	struct driver_data *drv_data = dev_get_drvdata(dev);
> @@ -1732,12 +1729,10 @@ static int pxa2xx_spi_runtime_resume(struct device *dev)
>  
>  	return clk_prepare_enable(drv_data->ssp->clk);
>  }
> -#endif
>  
>  static const struct dev_pm_ops pxa2xx_spi_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_suspend, pxa2xx_spi_resume)
> -	SET_RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend,
> -			   pxa2xx_spi_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_suspend, pxa2xx_spi_resume)
> +	RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend, pxa2xx_spi_runtime_resume, NULL)
>  };
>  
>  #ifdef CONFIG_ACPI
> @@ -1762,7 +1757,7 @@ MODULE_DEVICE_TABLE(of, pxa2xx_spi_of_match);
>  static struct platform_driver driver = {
>  	.driver = {
>  		.name	= "pxa2xx-spi",
> -		.pm	= &pxa2xx_spi_pm_ops,
> +		.pm	= pm_ptr(&pxa2xx_spi_pm_ops),
>  		.acpi_match_table = ACPI_PTR(pxa2xx_spi_acpi_match),
>  		.of_match_table = of_match_ptr(pxa2xx_spi_of_match),
>  	},


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 6/6] spi: pxa2xx: Switch from PM ifdeffery to pm_ptr()

[Andy Shevchenko]

On Tue, Oct 18, 2022 at 10:14:52AM +0100, Jonathan Cameron wrote:
> On Mon, 17 Oct 2022 20:12:43 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Cleaning up the driver to use pm_ptr() macro instead of ifdeffery
> > that makes it simpler and allows the compiler to remove those functions
> > if built without CONFIG_PM and CONFIG_PM_SLEEP support.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> FWIW I like these - so drive by review.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> I think you could change the handling of !pm_runtime_suspended()
> to use pm_runtime_force_suspend() and equivalent for resume path.
> I haven't checked that closely though - just looks like a typical
> usecase for those functions that are hardened against some of
> the corner cases that can occur in interactions between different
> forms of pm.

Thanks for an advice. Wouldn't it be matter of a separate change?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (subset) [PATCH v1 1/6] spi: pxa2xx: Simplify with devm_platform_get_and_ioremap_resource()

[Mark Brown]

On Mon, 17 Oct 2022 20:12:38 +0300, Andy Shevchenko wrote:
> Convert platform_get_resource(), devm_ioremap_resource() to a single
> call to devm_platform_get_and_ioremap_resource(), as this is exactly
> what this function does.
> 
> No functional changes.
> 
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/6] spi: pxa2xx: Simplify with devm_platform_get_and_ioremap_resource()
      commit: e3b7fca31185813297bb995d7b21a6305bb62c84

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel