[PATCH v1 00/10] spi: pxa2xx: Drop linux/spi/pxa2xx_spi.h

[PATCH v1 00/10] spi: pxa2xx: Drop linux/spi/pxa2xx_spi.h

[Andy Shevchenko]

As Arnd suggested we may drop linux/spi/pxa2xx_spi.h as most of
its content is being used solely internally to SPI subsystem
(PXA2xx drivers). Hence this refactoring series with the additional
win of getting rid of legacy documentation.

Cc: Arnd Bergmann <arnd@arndb.de>

Andy Shevchenko (10):
  spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()
  spi: pxa2xx: Keep PXA*_SSP types together
  spi: pxa2xx: Switch to use dev_err_probe()
  spi: pxa2xx: Extract pxa2xx_spi_init_ssp() helper
  spi: pxa2xx: Skip SSP initialization if it's done elsewhere
  spi: pxa2xx: Allow number of chip select pins to be read from property
  spi: pxa2xx: Provide num-cs for Sharp PDAs via device properties
  spi: pxa2xx: Move contents of linux/spi/pxa2xx_spi.h to a local one
  spi: pxa2xx: Remove outdated documentation
  spi: pxa2xx: Don't use "proxy" headers

 Documentation/spi/pxa2xx.rst   | 208 ---------------------------------
 arch/arm/mach-pxa/spitz.c      |  25 ++--
 drivers/spi/Kconfig            |   2 +-
 drivers/spi/spi-pxa2xx-dma.c   |  11 +-
 drivers/spi/spi-pxa2xx-pci.c   |  10 +-
 drivers/spi/spi-pxa2xx.c       | 118 +++++++++++--------
 drivers/spi/spi-pxa2xx.h       |  39 ++++++-
 include/linux/pxa2xx_ssp.h     |   2 +-
 include/linux/spi/pxa2xx_spi.h |  48 --------
 9 files changed, 140 insertions(+), 323 deletions(-)
 delete mode 100644 Documentation/spi/pxa2xx.rst
 delete mode 100644 include/linux/spi/pxa2xx_spi.h

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Andy Shevchenko]

Drop rather useless use of ACPI_PTR() and of_match_ptr().
It also removes the necessity to be dependent on ACPI and
of.h inclusion.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/Kconfig      | 2 +-
 drivers/spi/spi-pxa2xx.c | 9 +++------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index bc7021da2fe9..7cf86b034083 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -817,7 +817,7 @@ config SPI_PPC4xx
 
 config SPI_PXA2XX
 	tristate "PXA2xx SSP SPI master"
-	depends on ARCH_PXA || ARCH_MMP || PCI || ACPI || COMPILE_TEST
+	depends on ARCH_PXA || ARCH_MMP || PCI || COMPILE_TEST
 	select PXA_SSP if ARCH_PXA || ARCH_MMP
 	help
 	  This enables using a PXA2xx or Sodaville SSP port as a SPI master
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 6c2a14418972..2c39d3ff498e 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -19,7 +19,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
-#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
@@ -1730,7 +1729,6 @@ static const struct dev_pm_ops pxa2xx_spi_pm_ops = {
 	RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend, pxa2xx_spi_runtime_resume, NULL)
 };
 
-#ifdef CONFIG_ACPI
 static const struct acpi_device_id pxa2xx_spi_acpi_match[] = {
 	{ "80860F0E", LPSS_BYT_SSP },
 	{ "8086228E", LPSS_BSW_SSP },
@@ -1741,9 +1739,8 @@ static const struct acpi_device_id pxa2xx_spi_acpi_match[] = {
 	{}
 };
 MODULE_DEVICE_TABLE(acpi, pxa2xx_spi_acpi_match);
-#endif
 
-static const struct of_device_id pxa2xx_spi_of_match[] __maybe_unused = {
+static const struct of_device_id pxa2xx_spi_of_match[] = {
 	{ .compatible = "marvell,mmp2-ssp", .data = (void *)MMP2_SSP },
 	{}
 };
@@ -1753,8 +1750,8 @@ static struct platform_driver driver = {
 	.driver = {
 		.name	= "pxa2xx-spi",
 		.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),
+		.acpi_match_table = pxa2xx_spi_acpi_match,
+		.of_match_table = pxa2xx_spi_of_match,
 	},
 	.probe = pxa2xx_spi_probe,
 	.remove_new = pxa2xx_spi_remove,
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 02/10] spi: pxa2xx: Keep PXA*_SSP types together

[Andy Shevchenko]

Keep the PXA*_SSP types together in enum pxa_ssp_type
for better maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/pxa2xx_ssp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/pxa2xx_ssp.h b/include/linux/pxa2xx_ssp.h
index cd1973e6ac4b..844a2743ca94 100644
--- a/include/linux/pxa2xx_ssp.h
+++ b/include/linux/pxa2xx_ssp.h
@@ -217,9 +217,9 @@ enum pxa_ssp_type {
 	PXA27x_SSP,
 	PXA3xx_SSP,
 	PXA168_SSP,
-	MMP2_SSP,
 	PXA910_SSP,
 	CE4100_SSP,
+	MMP2_SSP,
 	MRFLD_SSP,
 	QUARK_X1000_SSP,
 	/* Keep LPSS types sorted with lpss_platforms[] */
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 03/10] spi: pxa2xx: Switch to use dev_err_probe()

[Andy Shevchenko]

Switch to use dev_err_probe() to simplify the error path and
unify a message template.

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

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 2c39d3ff498e..75d208087748 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1434,20 +1434,16 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 	platform_info = dev_get_platdata(dev);
 	if (!platform_info) {
 		platform_info = pxa2xx_spi_init_pdata(pdev);
-		if (IS_ERR(platform_info)) {
-			dev_err(&pdev->dev, "missing platform data\n");
-			return PTR_ERR(platform_info);
-		}
+		if (IS_ERR(platform_info))
+			return dev_err_probe(dev, PTR_ERR(platform_info), "missing platform data\n");
 	}
 
 	ssp = pxa_ssp_request(pdev->id, pdev->name);
 	if (!ssp)
 		ssp = &platform_info->ssp;
 
-	if (!ssp->mmio_base) {
-		dev_err(&pdev->dev, "failed to get SSP\n");
-		return -ENODEV;
-	}
+	if (!ssp->mmio_base)
+		return dev_err_probe(dev, -ENODEV, "failed to get SSP\n");
 
 	if (platform_info->is_target)
 		controller = devm_spi_alloc_target(dev, sizeof(*drv_data));
@@ -1455,8 +1451,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 		controller = devm_spi_alloc_host(dev, sizeof(*drv_data));
 
 	if (!controller) {
-		dev_err(&pdev->dev, "cannot alloc spi_controller\n");
-		status = -ENOMEM;
+		status = dev_err_probe(dev, -ENOMEM, "cannot alloc spi_controller\n");
 		goto out_error_controller_alloc;
 	}
 	drv_data = spi_controller_get_devdata(controller);
@@ -1510,7 +1505,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 	status = request_irq(ssp->irq, ssp_int, IRQF_SHARED, dev_name(dev),
 			drv_data);
 	if (status < 0) {
-		dev_err(&pdev->dev, "cannot get IRQ %d\n", ssp->irq);
+		dev_err_probe(dev, status, "cannot get IRQ %d\n", ssp->irq);
 		goto out_error_controller_alloc;
 	}
 
@@ -1626,7 +1621,7 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, drv_data);
 	status = spi_register_controller(controller);
 	if (status) {
-		dev_err(&pdev->dev, "problem registering SPI controller\n");
+		dev_err_probe(dev, status, "problem registering SPI controller\n");
 		goto out_error_pm_runtime_enabled;
 	}
 
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 04/10] spi: pxa2xx: Extract pxa2xx_spi_init_ssp() helper

[Andy Shevchenko]

Refactor pxa2xx_spi_init_pdata() by extracting a new
pxa2xx_spi_init_ssp() helper which makes code less
twisted. It will be easier to continue refactoring for
a new coming modification.

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

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 75d208087748..e7072727c25c 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1314,19 +1314,50 @@ static bool pxa2xx_spi_idma_filter(struct dma_chan *chan, void *param)
 	return param == chan->device->dev;
 }
 
+static int
+pxa2xx_spi_init_ssp(struct platform_device *pdev, struct ssp_device *ssp, enum pxa_ssp_type type)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int status;
+	u64 uid;
+
+	ssp->mmio_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(ssp->mmio_base))
+		return PTR_ERR(ssp->mmio_base);
+
+	ssp->phys_base = res->start;
+
+	ssp->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(ssp->clk))
+		return PTR_ERR(ssp->clk);
+
+	ssp->irq = platform_get_irq(pdev, 0);
+	if (ssp->irq < 0)
+		return ssp->irq;
+
+	ssp->type = type;
+	ssp->dev = dev;
+
+	status = acpi_dev_uid_to_integer(ACPI_COMPANION(dev), &uid);
+	if (status)
+		ssp->port_id = -1;
+	else
+		ssp->port_id = uid;
+
+	return 0;
+}
+
 static struct pxa2xx_spi_controller *
 pxa2xx_spi_init_pdata(struct platform_device *pdev)
 {
 	struct pxa2xx_spi_controller *pdata;
 	struct device *dev = &pdev->dev;
 	struct device *parent = dev->parent;
-	struct ssp_device *ssp;
-	struct resource *res;
 	enum pxa_ssp_type type = SSP_UNDEFINED;
 	const void *match;
 	bool is_lpss_priv;
 	int status;
-	u64 uid;
 
 	is_lpss_priv = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpss_priv");
 
@@ -1351,14 +1382,6 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 
-	ssp = &pdata->ssp;
-
-	ssp->mmio_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
-	if (IS_ERR(ssp->mmio_base))
-		return ERR_CAST(ssp->mmio_base);
-
-	ssp->phys_base = res->start;
-
 	/* Platforms with iDMA 64-bit */
 	if (is_lpss_priv) {
 		pdata->tx_param = parent;
@@ -1366,28 +1389,15 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 		pdata->dma_filter = pxa2xx_spi_idma_filter;
 	}
 
-	ssp->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(ssp->clk))
-		return ERR_CAST(ssp->clk);
-
-	ssp->irq = platform_get_irq(pdev, 0);
-	if (ssp->irq < 0)
-		return ERR_PTR(ssp->irq);
-
-	ssp->type = type;
-	ssp->dev = dev;
-
-	status = acpi_dev_uid_to_integer(ACPI_COMPANION(dev), &uid);
-	if (status)
-		ssp->port_id = -1;
-	else
-		ssp->port_id = uid;
-
 	pdata->is_target = device_property_read_bool(dev, "spi-slave");
 	pdata->num_chipselect = 1;
 	pdata->enable_dma = true;
 	pdata->dma_burst_size = 1;
 
+	status = pxa2xx_spi_init_ssp(pdev, &pdata->ssp, type);
+	if (status)
+		return ERR_PTR(status);
+
 	return pdata;
 }
 
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 05/10] spi: pxa2xx: Skip SSP initialization if it's done elsewhere

[Andy Shevchenko]

If SSP has been enumerated elsewhere, skip its initialization
in pxa2xx_spi_init_pdata().

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

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index e7072727c25c..b01a18c89b6b 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1355,6 +1355,7 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device *parent = dev->parent;
 	enum pxa_ssp_type type = SSP_UNDEFINED;
+	struct ssp_device *ssp = NULL;
 	const void *match;
 	bool is_lpss_priv;
 	int status;
@@ -1372,6 +1373,10 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 			return ERR_PTR(status);
 
 		type = (enum pxa_ssp_type)value;
+	} else {
+		ssp = pxa_ssp_request(pdev->id, pdev->name);
+		if (ssp)
+			type = ssp->type;
 	}
 
 	/* Validate the SSP type correctness */
@@ -1394,6 +1399,10 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 	pdata->enable_dma = true;
 	pdata->dma_burst_size = 1;
 
+	/* If SSP has been already enumerated, use it */
+	if (ssp)
+		return pdata;
+
 	status = pxa2xx_spi_init_ssp(pdev, &pdata->ssp, type);
 	if (status)
 		return ERR_PTR(status);
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 06/10] spi: pxa2xx: Allow number of chip select pins to be read from property

[Andy Shevchenko]

In some cases the number of the chip select pins might come from
the device property. Allow driver to use it.

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

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index b01a18c89b6b..f4435c39d096 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1358,6 +1358,7 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 	struct ssp_device *ssp = NULL;
 	const void *match;
 	bool is_lpss_priv;
+	u32 num_cs = 1;
 	int status;
 
 	is_lpss_priv = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpss_priv");
@@ -1394,8 +1395,11 @@ pxa2xx_spi_init_pdata(struct platform_device *pdev)
 		pdata->dma_filter = pxa2xx_spi_idma_filter;
 	}
 
+	/* Read number of chip select pins, if provided */
+	device_property_read_u32(dev, "num-cs", &num_cs);
+
+	pdata->num_chipselect = num_cs;
 	pdata->is_target = device_property_read_bool(dev, "spi-slave");
-	pdata->num_chipselect = 1;
 	pdata->enable_dma = true;
 	pdata->dma_burst_size = 1;
 
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 07/10] spi: pxa2xx: Provide num-cs for Sharp PDAs via device properties

[Andy Shevchenko]

Since driver can parse num-cs device property, replace platform data
with this new approach.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/arm/mach-pxa/spitz.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 318402ad685e..3c5f5a3cb480 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -18,10 +18,10 @@
 #include <linux/i2c.h>
 #include <linux/platform_data/i2c-pxa.h>
 #include <linux/platform_data/pca953x.h>
+#include <linux/property.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/ads7846.h>
 #include <linux/spi/corgi_lcd.h>
-#include <linux/spi/pxa2xx_spi.h>
 #include <linux/mtd/sharpsl.h>
 #include <linux/mtd/physmap.h>
 #include <linux/input-event-codes.h>
@@ -569,10 +569,6 @@ static struct spi_board_info spitz_spi_devices[] = {
 	},
 };
 
-static struct pxa2xx_spi_controller spitz_spi_info = {
-	.num_chipselect	= 3,
-};
-
 static struct gpiod_lookup_table spitz_spi_gpio_table = {
 	.dev_id = "spi2",
 	.table = {
@@ -583,10 +579,20 @@ static struct gpiod_lookup_table spitz_spi_gpio_table = {
 	},
 };
 
+static const struct property_entry spitz_spi_properties[] = {
+	PROPERTY_ENTRY_U32("num-cs", 3),
+	{ }
+};
+
+static const struct software_node spitz_spi_node = {
+	.properties = spitz_spi_properties,
+};
+
 static void __init spitz_spi_init(void)
 {
 	struct platform_device *pd;
 	int id = 2;
+	int err;
 
 	if (machine_is_akita())
 		gpiod_add_lookup_table(&akita_lcdcon_gpio_table);
@@ -601,8 +607,13 @@ static void __init spitz_spi_init(void)
 	if (pd == NULL) {
 		pr_err("pxa2xx-spi: failed to allocate device id %d\n", id);
 	} else {
-		pd->dev.platform_data = &spitz_spi_info;
-		platform_device_add(pd);
+		err = device_add_software_node(&pd->dev, &spitz_spi_node);
+		if (err) {
+			platform_device_put(pd);
+			pr_err("pxa2xx-spi: failed to add software node\n");
+		} else {
+			platform_device_add(pd);
+		}
 	}
 
 	spi_register_board_info(ARRAY_AND_SIZE(spitz_spi_devices));
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 08/10] spi: pxa2xx: Move contents of linux/spi/pxa2xx_spi.h to a local one

[Andy Shevchenko]

There is no user of the linux/spi/pxa2xx_spi.h. Move its contents
to the drivers/spi/spi-pxa2xx.h.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx-dma.c   |  1 -
 drivers/spi/spi-pxa2xx-pci.c   |  4 +--
 drivers/spi/spi-pxa2xx.c       |  1 -
 drivers/spi/spi-pxa2xx.h       | 36 ++++++++++++++++++++++++-
 include/linux/spi/pxa2xx_spi.h | 48 ----------------------------------
 5 files changed, 37 insertions(+), 53 deletions(-)
 delete mode 100644 include/linux/spi/pxa2xx_spi.h

diff --git a/drivers/spi/spi-pxa2xx-dma.c b/drivers/spi/spi-pxa2xx-dma.c
index be563f0dd03a..26416ced6505 100644
--- a/drivers/spi/spi-pxa2xx-dma.c
+++ b/drivers/spi/spi-pxa2xx-dma.c
@@ -12,7 +12,6 @@
 #include <linux/scatterlist.h>
 #include <linux/sizes.h>
 
-#include <linux/spi/pxa2xx_spi.h>
 #include <linux/spi/spi.h>
 
 #include "spi-pxa2xx.h"
diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index 861b21c63504..e11a613bc340 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -10,11 +10,11 @@
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 
-#include <linux/spi/pxa2xx_spi.h>
-
 #include <linux/dmaengine.h>
 #include <linux/platform_data/dma-dw.h>
 
+#include "spi-pxa2xx.h"
+
 #define PCI_DEVICE_ID_INTEL_QUARK_X1000		0x0935
 #define PCI_DEVICE_ID_INTEL_BYT			0x0f0e
 #define PCI_DEVICE_ID_INTEL_MRFLD		0x1194
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index f4435c39d096..e22d9d29c7e9 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -24,7 +24,6 @@
 #include <linux/property.h>
 #include <linux/slab.h>
 
-#include <linux/spi/pxa2xx_spi.h>
 #include <linux/spi/spi.h>
 
 #include "spi-pxa2xx.h"
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index 45cdbbc71c4b..08296729ea80 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -7,6 +7,7 @@
 #ifndef SPI_PXA2XX_H
 #define SPI_PXA2XX_H
 
+#include <linux/dmaengine.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/types.h>
@@ -15,7 +16,40 @@
 #include <linux/pxa2xx_ssp.h>
 
 struct gpio_desc;
-struct pxa2xx_spi_controller;
+
+/*
+ * The platform data for SSP controller devices
+ * (resides in device.platform_data).
+ */
+struct pxa2xx_spi_controller {
+	u8 num_chipselect;
+	u8 enable_dma;
+	u8 dma_burst_size;
+	bool is_target;
+
+	/* DMA engine specific config */
+	dma_filter_fn dma_filter;
+	void *tx_param;
+	void *rx_param;
+
+	/* For non-PXA arches */
+	struct ssp_device ssp;
+};
+
+/*
+ * The controller specific data for SPI target devices
+ * (resides in spi_board_info.controller_data),
+ * copied to spi_device.platform_data ... mostly for
+ * DMA tuning.
+ */
+struct pxa2xx_spi_chip {
+	u8 tx_threshold;
+	u8 tx_hi_threshold;
+	u8 rx_threshold;
+	u8 dma_burst_size;
+	u32 timeout;
+};
+
 struct spi_controller;
 struct spi_device;
 struct spi_transfer;
diff --git a/include/linux/spi/pxa2xx_spi.h b/include/linux/spi/pxa2xx_spi.h
deleted file mode 100644
index e5a4a045fb67..000000000000
--- a/include/linux/spi/pxa2xx_spi.h
+++ /dev/null
@@ -1,48 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * Copyright (C) 2005 Stephen Street / StreetFire Sound Labs
- */
-#ifndef __LINUX_SPI_PXA2XX_SPI_H
-#define __LINUX_SPI_PXA2XX_SPI_H
-
-#include <linux/dmaengine.h>
-#include <linux/types.h>
-
-#include <linux/pxa2xx_ssp.h>
-
-struct dma_chan;
-
-/*
- * The platform data for SSP controller devices
- * (resides in device.platform_data).
- */
-struct pxa2xx_spi_controller {
-	u8 num_chipselect;
-	u8 enable_dma;
-	u8 dma_burst_size;
-	bool is_target;
-
-	/* DMA engine specific config */
-	dma_filter_fn dma_filter;
-	void *tx_param;
-	void *rx_param;
-
-	/* For non-PXA arches */
-	struct ssp_device ssp;
-};
-
-/*
- * The controller specific data for SPI target devices
- * (resides in spi_board_info.controller_data),
- * copied to spi_device.platform_data ... mostly for
- * DMA tuning.
- */
-struct pxa2xx_spi_chip {
-	u8 tx_threshold;
-	u8 tx_hi_threshold;
-	u8 rx_threshold;
-	u8 dma_burst_size;
-	u32 timeout;
-};
-
-#endif	/* __LINUX_SPI_PXA2XX_SPI_H */
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 09/10] spi: pxa2xx: Remove outdated documentation

[Andy Shevchenko]

The documentation is referring to the legacy enumeration of the SPI
host controllers and target devices. It has nothing to do with the
modern way, which is the only supported in kernel right now. Hence,
remove outdated documentation file.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/spi/pxa2xx.rst | 208 -----------------------------------
 1 file changed, 208 deletions(-)
 delete mode 100644 Documentation/spi/pxa2xx.rst

diff --git a/Documentation/spi/pxa2xx.rst b/Documentation/spi/pxa2xx.rst
deleted file mode 100644
index 33d2ad95901e..000000000000
--- a/Documentation/spi/pxa2xx.rst
+++ /dev/null
@@ -1,208 +0,0 @@
-==============================
-PXA2xx SPI on SSP driver HOWTO
-==============================
-
-This a mini HOWTO on the pxa2xx_spi driver. The driver turns a PXA2xx
-synchronous serial port into an SPI host controller
-(see Documentation/spi/spi-summary.rst). The driver has the following features
-
-- Support for any PXA2xx and compatible SSP.
-- SSP PIO and SSP DMA data transfers.
-- External and Internal (SSPFRM) chip selects.
-- Per peripheral device (chip) configuration.
-- Full suspend, freeze, resume support.
-
-The driver is built around a &struct spi_message FIFO serviced by kernel
-thread. The kernel thread, spi_pump_messages(), drives message FIFO and
-is responsible for queuing SPI transactions and setting up and launching
-the DMA or interrupt driven transfers.
-
-Declaring PXA2xx host controllers
----------------------------------
-Typically, for a legacy platform, an SPI host controller is defined in the
-arch/.../mach-*/board-*.c as a "platform device". The host controller configuration
-is passed to the driver via a table found in include/linux/spi/pxa2xx_spi.h::
-
-  struct pxa2xx_spi_controller {
-	u8 num_chipselect;
-	u8 enable_dma;
-	...
-  };
-
-The "pxa2xx_spi_controller.num_chipselect" field is used to determine the number of
-peripheral devices (chips) attached to this SPI host controller.
-
-The "pxa2xx_spi_controller.enable_dma" field informs the driver that SSP DMA should
-be used. This caused the driver to acquire two DMA channels: Rx channel and
-Tx channel. The Rx channel has a higher DMA service priority than the Tx channel.
-See the "PXA2xx Developer Manual" section "DMA Controller".
-
-For the new platforms the description of the controller and peripheral devices
-comes from Device Tree or ACPI.
-
-NSSP HOST SAMPLE
-----------------
-Below is a sample configuration using the PXA255 NSSP for a legacy platform::
-
-  static struct resource pxa_spi_nssp_resources[] = {
-	[0] = {
-		.start	= __PREG(SSCR0_P(2)), /* Start address of NSSP */
-		.end	= __PREG(SSCR0_P(2)) + 0x2c, /* Range of registers */
-		.flags	= IORESOURCE_MEM,
-	},
-	[1] = {
-		.start	= IRQ_NSSP, /* NSSP IRQ */
-		.end	= IRQ_NSSP,
-		.flags	= IORESOURCE_IRQ,
-	},
-  };
-
-  static struct pxa2xx_spi_controller pxa_nssp_controller_info = {
-	.num_chipselect = 1, /* Matches the number of chips attached to NSSP */
-	.enable_dma = 1, /* Enables NSSP DMA */
-  };
-
-  static struct platform_device pxa_spi_nssp = {
-	.name = "pxa2xx-spi", /* MUST BE THIS VALUE, so device match driver */
-	.id = 2, /* Bus number, MUST MATCH SSP number 1..n */
-	.resource = pxa_spi_nssp_resources,
-	.num_resources = ARRAY_SIZE(pxa_spi_nssp_resources),
-	.dev = {
-		.platform_data = &pxa_nssp_controller_info, /* Passed to driver */
-	},
-  };
-
-  static struct platform_device *devices[] __initdata = {
-	&pxa_spi_nssp,
-  };
-
-  static void __init board_init(void)
-  {
-	(void)platform_add_device(devices, ARRAY_SIZE(devices));
-  }
-
-Declaring peripheral devices
-----------------------------
-Typically, for a legacy platform, each SPI peripheral device (chip) is defined in the
-arch/.../mach-*/board-*.c using the "spi_board_info" structure found in
-"linux/spi/spi.h". See "Documentation/spi/spi-summary.rst" for additional
-information.
-
-Each peripheral device (chip) attached to the PXA2xx must provide specific chip configuration
-information via the structure "pxa2xx_spi_chip" found in
-"include/linux/spi/pxa2xx_spi.h". The PXA2xx host controller driver will use
-the configuration whenever the driver communicates with the peripheral
-device. All fields are optional.
-
-::
-
-  struct pxa2xx_spi_chip {
-	u8 tx_threshold;
-	u8 rx_threshold;
-	u8 dma_burst_size;
-	u32 timeout;
-  };
-
-The "pxa2xx_spi_chip.tx_threshold" and "pxa2xx_spi_chip.rx_threshold" fields are
-used to configure the SSP hardware FIFO. These fields are critical to the
-performance of pxa2xx_spi driver and misconfiguration will result in rx
-FIFO overruns (especially in PIO mode transfers). Good default values are::
-
-	.tx_threshold = 8,
-	.rx_threshold = 8,
-
-The range is 1 to 16 where zero indicates "use default".
-
-The "pxa2xx_spi_chip.dma_burst_size" field is used to configure PXA2xx DMA
-engine and is related the "spi_device.bits_per_word" field.  Read and understand
-the PXA2xx "Developer Manual" sections on the DMA controller and SSP Controllers
-to determine the correct value. An SSP configured for byte-wide transfers would
-use a value of 8. The driver will determine a reasonable default if
-dma_burst_size == 0.
-
-The "pxa2xx_spi_chip.timeout" fields is used to efficiently handle
-trailing bytes in the SSP receiver FIFO. The correct value for this field is
-dependent on the SPI bus speed ("spi_board_info.max_speed_hz") and the specific
-peripheral device. Please note that the PXA2xx SSP 1 does not support trailing byte
-timeouts and must busy-wait any trailing bytes.
-
-NOTE: the SPI driver cannot control the chip select if SSPFRM is used, so the
-chipselect is dropped after each spi_transfer.  Most devices need chip select
-asserted around the complete message. Use SSPFRM as a GPIO (through a descriptor)
-to accommodate these chips.
-
-
-NSSP PERIPHERAL SAMPLE
-----------------------
-For a legacy platform or in some other cases, the pxa2xx_spi_chip structure
-is passed to the pxa2xx_spi driver in the "spi_board_info.controller_data"
-field. Below is a sample configuration using the PXA255 NSSP.
-
-::
-
-  static struct pxa2xx_spi_chip cs8415a_chip_info = {
-	.tx_threshold = 8, /* SSP hardware FIFO threshold */
-	.rx_threshold = 8, /* SSP hardware FIFO threshold */
-	.dma_burst_size = 8, /* Byte wide transfers used so 8 byte bursts */
-	.timeout = 235, /* See Intel documentation */
-  };
-
-  static struct pxa2xx_spi_chip cs8405a_chip_info = {
-	.tx_threshold = 8, /* SSP hardware FIFO threshold */
-	.rx_threshold = 8, /* SSP hardware FIFO threshold */
-	.dma_burst_size = 8, /* Byte wide transfers used so 8 byte bursts */
-	.timeout = 235, /* See Intel documentation */
-  };
-
-  static struct spi_board_info streetracer_spi_board_info[] __initdata = {
-	{
-		.modalias = "cs8415a", /* Name of spi_driver for this device */
-		.max_speed_hz = 3686400, /* Run SSP as fast a possible */
-		.bus_num = 2, /* Framework bus number */
-		.chip_select = 0, /* Framework chip select */
-		.platform_data = NULL; /* No spi_driver specific config */
-		.controller_data = &cs8415a_chip_info, /* Host controller config */
-		.irq = STREETRACER_APCI_IRQ, /* Peripheral device interrupt */
-	},
-	{
-		.modalias = "cs8405a", /* Name of spi_driver for this device */
-		.max_speed_hz = 3686400, /* Run SSP as fast a possible */
-		.bus_num = 2, /* Framework bus number */
-		.chip_select = 1, /* Framework chip select */
-		.controller_data = &cs8405a_chip_info, /* Host controller config */
-		.irq = STREETRACER_APCI_IRQ, /* Peripheral device interrupt */
-	},
-  };
-
-  static void __init streetracer_init(void)
-  {
-	spi_register_board_info(streetracer_spi_board_info,
-				ARRAY_SIZE(streetracer_spi_board_info));
-  }
-
-
-DMA and PIO I/O Support
------------------------
-The pxa2xx_spi driver supports both DMA and interrupt driven PIO message
-transfers.  The driver defaults to PIO mode and DMA transfers must be enabled
-by setting the "enable_dma" flag in the "pxa2xx_spi_controller" structure.
-For the newer platforms, that are known to support DMA, the driver will enable
-it automatically and try it first with a possible fallback to PIO. The DMA
-mode supports both coherent and stream based DMA mappings.
-
-The following logic is used to determine the type of I/O to be used on
-a per "spi_transfer" basis::
-
-  if spi_message.len > 65536 then
-	print "rate limited" warning
-	use PIO transfers
-
-  if enable_dma and the size is in the range [DMA burst size..65536] then
-	use streaming DMA mode
-
-  otherwise
-	use PIO transfer
-
-THANKS TO
----------
-David Brownell and others for mentoring the development of this driver.
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 10/10] spi: pxa2xx: Don't use "proxy" headers

[Andy Shevchenko]

Update header inclusions to follow IWYU (Include What You Use)
principle.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi-pxa2xx-dma.c | 10 ++++++++--
 drivers/spi/spi-pxa2xx-pci.c |  6 ++++++
 drivers/spi/spi-pxa2xx.c     | 10 +++++++---
 drivers/spi/spi-pxa2xx.h     |  3 +--
 4 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx-dma.c b/drivers/spi/spi-pxa2xx-dma.c
index 26416ced6505..751cb0f77b62 100644
--- a/drivers/spi/spi-pxa2xx-dma.c
+++ b/drivers/spi/spi-pxa2xx-dma.c
@@ -6,16 +6,22 @@
  * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
  */
 
-#include <linux/device.h>
+#include <linux/atomic.h>
+#include <linux/dev_printk.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
+#include <linux/errno.h>
+#include <linux/irqreturn.h>
 #include <linux/scatterlist.h>
-#include <linux/sizes.h>
+#include <linux/string.h>
+#include <linux/types.h>
 
 #include <linux/spi/spi.h>
 
 #include "spi-pxa2xx.h"
 
+struct device;
+
 static void pxa2xx_spi_dma_transfer_complete(struct driver_data *drv_data,
 					     bool error)
 {
diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index e11a613bc340..6d2efdb0e95f 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -6,9 +6,15 @@
  * Copyright (C) 2016, 2021 Intel Corporation
  */
 #include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/sprintf.h>
+#include <linux/string.h>
+#include <linux/types.h>
 
 #include <linux/dmaengine.h>
 #include <linux/platform_data/dma-dw.h>
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index e22d9d29c7e9..86d0f1064a45 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -5,24 +5,28 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/atomic.h>
 #include <linux/bitops.h>
+#include <linux/bug.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/dmaengine.h>
 #include <linux/err.h>
-#include <linux/errno.h>
 #include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/ioport.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/math64.h>
+#include <linux/minmax.h>
 #include <linux/mod_devicetable.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include <linux/types.h>
 
 #include <linux/spi/spi.h>
 
diff --git a/drivers/spi/spi-pxa2xx.h b/drivers/spi/spi-pxa2xx.h
index 08296729ea80..f1097c96c50f 100644
--- a/drivers/spi/spi-pxa2xx.h
+++ b/drivers/spi/spi-pxa2xx.h
@@ -8,8 +8,7 @@
 #define SPI_PXA2XX_H
 
 #include <linux/dmaengine.h>
-#include <linux/interrupt.h>
-#include <linux/io.h>
+#include <linux/irqreturn.h>
 #include <linux/types.h>
 #include <linux/sizes.h>
 
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Mark Brown]

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

On Tue, Mar 26, 2024 at 08:07:51PM +0200, Andy Shevchenko wrote:
> Drop rather useless use of ACPI_PTR() and of_match_ptr().
> It also removes the necessity to be dependent on ACPI and
> of.h inclusion.

I think the ACPI dependency there is as much about hiding the device on
irrelevant platforms as anything else, might be better replaced with an
x86 dependency though.

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

Re: [PATCH v1 07/10] spi: pxa2xx: Provide num-cs for Sharp PDAs via device properties

[Mark Brown]

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

On Tue, Mar 26, 2024 at 08:07:57PM +0200, Andy Shevchenko wrote:

> Since driver can parse num-cs device property, replace platform data
> with this new approach.

But why?

> -static struct pxa2xx_spi_controller spitz_spi_info = {
> -	.num_chipselect	= 3,
> -};

> +static const struct property_entry spitz_spi_properties[] = {
> +	PROPERTY_ENTRY_U32("num-cs", 3),
> +	{ }
> +};

This is just platform data with less validation AFAICT.

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

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 06:16:28PM +0000, Mark Brown wrote:
> On Tue, Mar 26, 2024 at 08:07:51PM +0200, Andy Shevchenko wrote:
> > Drop rather useless use of ACPI_PTR() and of_match_ptr().
> > It also removes the necessity to be dependent on ACPI and
> > of.h inclusion.
> 
> I think the ACPI dependency there is as much about hiding the device on
> irrelevant platforms as anything else, might be better replaced with an
> x86 dependency though.

The whole idea behind ACPI_PTR() (which seems following the of_match_ptr()
implementation) looks premature. Now we have a lot of patches from DT people to
remove of_match_ptr(), i.o.w. make the ID visible even on non-OF platforms.

Having the list of supported IDs is not bad thing anyway as it might help
to google for a device elsewhere, for example.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Mark Brown]

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

On Tue, Mar 26, 2024 at 08:22:59PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2024 at 06:16:28PM +0000, Mark Brown wrote:
> > On Tue, Mar 26, 2024 at 08:07:51PM +0200, Andy Shevchenko wrote:

> > > Drop rather useless use of ACPI_PTR() and of_match_ptr().
> > > It also removes the necessity to be dependent on ACPI and
> > > of.h inclusion.

> > I think the ACPI dependency there is as much about hiding the device on
> > irrelevant platforms as anything else, might be better replaced with an
> > x86 dependency though.

> The whole idea behind ACPI_PTR() (which seems following the of_match_ptr()
> implementation) looks premature. Now we have a lot of patches from DT people to
> remove of_match_ptr(), i.o.w. make the ID visible even on non-OF platforms.

> Having the list of supported IDs is not bad thing anyway as it might help
> to google for a device elsewhere, for example.

That's nice but I'm not sure what that has to do with the dependency on
ACPI?

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

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 06:25:16PM +0000, Mark Brown wrote:
> On Tue, Mar 26, 2024 at 08:22:59PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 26, 2024 at 06:16:28PM +0000, Mark Brown wrote:
> > > On Tue, Mar 26, 2024 at 08:07:51PM +0200, Andy Shevchenko wrote:
> 
> > > > Drop rather useless use of ACPI_PTR() and of_match_ptr().
> > > > It also removes the necessity to be dependent on ACPI and
> > > > of.h inclusion.
> 
> > > I think the ACPI dependency there is as much about hiding the device on
> > > irrelevant platforms as anything else, might be better replaced with an
> > > x86 dependency though.
> 
> > The whole idea behind ACPI_PTR() (which seems following the of_match_ptr()
> > implementation) looks premature. Now we have a lot of patches from DT people to
> > remove of_match_ptr(), i.o.w. make the ID visible even on non-OF platforms.
> 
> > Having the list of supported IDs is not bad thing anyway as it might help
> > to google for a device elsewhere, for example.
> 
> That's nice but I'm not sure what that has to do with the dependency on
> ACPI?

ACPI_PTR() makes ID no-op only if ACPI is defined. That also satisfies
the ugly ifdeffery that is removed by this patch as well. If there is
no dependency we will have compiler warning for defined but not used
variable.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Mark Brown]

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

On Tue, Mar 26, 2024 at 08:44:11PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2024 at 06:25:16PM +0000, Mark Brown wrote:
> > On Tue, Mar 26, 2024 at 08:22:59PM +0200, Andy Shevchenko wrote:
> > > On Tue, Mar 26, 2024 at 06:16:28PM +0000, Mark Brown wrote:

> > > > I think the ACPI dependency there is as much about hiding the device on
> > > > irrelevant platforms as anything else, might be better replaced with an
> > > > x86 dependency though.

...

> > That's nice but I'm not sure what that has to do with the dependency on
> > ACPI?

> ACPI_PTR() makes ID no-op only if ACPI is defined. That also satisfies
> the ugly ifdeffery that is removed by this patch as well. If there is
> no dependency we will have compiler warning for defined but not used
> variable.

Again I don't think that's the main purpose of the dependency here.

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

Re: [PATCH v1 07/10] spi: pxa2xx: Provide num-cs for Sharp PDAs via device properties

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 06:21:48PM +0000, Mark Brown wrote:
> On Tue, Mar 26, 2024 at 08:07:57PM +0200, Andy Shevchenko wrote:
> 
> > Since driver can parse num-cs device property, replace platform data
> > with this new approach.
> 
> But why?

To be able to hide the header's contents from public.
Should I update the commit message?

...

> > +static const struct property_entry spitz_spi_properties[] = {
> > +	PROPERTY_ENTRY_U32("num-cs", 3),
> > +	{ }
> > +};
> 
> This is just platform data with less validation AFAICT.

I'm not sure what validation you are expecting here. It should be done via
DT schema ideally when the platform gets converted to DT. This change is
an interim to that (at least it makes kernel side better). After the platform
code may be gone completely or converted. If the latter happens, we got
the validation back.

In any case it's not worse than plain DT property handling in the kernel.
The validation in that case is done elsewhere. Since the property is defined
in board files the assumed validation is done during development/review
stages. But OTOH for the legacy code we need not to touch the property
provider more than once. We are _not_ expecting this to be spread.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 06:49:58PM +0000, Mark Brown wrote:
> On Tue, Mar 26, 2024 at 08:44:11PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 26, 2024 at 06:25:16PM +0000, Mark Brown wrote:
> > > On Tue, Mar 26, 2024 at 08:22:59PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Mar 26, 2024 at 06:16:28PM +0000, Mark Brown wrote:
> 
> > > > > I think the ACPI dependency there is as much about hiding the device on
> > > > > irrelevant platforms as anything else, might be better replaced with an
> > > > > x86 dependency though.

...

> > > That's nice but I'm not sure what that has to do with the dependency on
> > > ACPI?
> 
> > ACPI_PTR() makes ID no-op only if ACPI is defined. That also satisfies
> > the ugly ifdeffery that is removed by this patch as well. If there is
> > no dependency we will have compiler warning for defined but not used
> > variable.
> 
> Again I don't think that's the main purpose of the dependency here.

Oh, oh, my bad I missed acpi_dev_uid_to_integer() call.
Okay, with that in mind it's functional dependency for the ACPI-based
platforms. Do you want to keep it untouched?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Mark Brown]

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

On Tue, Mar 26, 2024 at 08:52:53PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2024 at 06:49:58PM +0000, Mark Brown wrote:

> > > > > > I think the ACPI dependency there is as much about hiding the device on
> > > > > > irrelevant platforms as anything else, might be better replaced with an
> > > > > > x86 dependency though.

> Oh, oh, my bad I missed acpi_dev_uid_to_integer() call.
> Okay, with that in mind it's functional dependency for the ACPI-based
> platforms. Do you want to keep it untouched?

That's not actually what I was thinking of (please read what I wrote
above, like I say I was thining about hiding things) but surely if that
was a reason to keep the dependency it'd need to be an actual ACPI
dependency rather than an ||?

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

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 07:10:09PM +0000, Mark Brown wrote:
> On Tue, Mar 26, 2024 at 08:52:53PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 26, 2024 at 06:49:58PM +0000, Mark Brown wrote:
> 
> > > > > > > I think the ACPI dependency there is as much about hiding the device on
> > > > > > > irrelevant platforms as anything else, might be better replaced with an
> > > > > > > x86 dependency though.
> 
> > Oh, oh, my bad I missed acpi_dev_uid_to_integer() call.
> > Okay, with that in mind it's functional dependency for the ACPI-based
> > platforms. Do you want to keep it untouched?
> 
> That's not actually what I was thinking of (please read what I wrote
> above, like I say I was thining about hiding things) but surely if that
> was a reason to keep the dependency it'd need to be an actual ACPI
> dependency rather than an ||?

For my knowledge there is none of the ACPI-based platform where CONFIG_ACPI
needs to be 'n' while having the real device (as per ACPI ID table) to be on.
That's why I answered purely from the compilation point of view.

Personally I see that dependency more confusing than hinting about anything.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 09:20:05PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2024 at 07:10:09PM +0000, Mark Brown wrote:
> > On Tue, Mar 26, 2024 at 08:52:53PM +0200, Andy Shevchenko wrote:
> > > On Tue, Mar 26, 2024 at 06:49:58PM +0000, Mark Brown wrote:
> > 
> > > > > > > > I think the ACPI dependency there is as much about hiding the device on
> > > > > > > > irrelevant platforms as anything else, might be better replaced with an
> > > > > > > > x86 dependency though.
> > 
> > > Oh, oh, my bad I missed acpi_dev_uid_to_integer() call.
> > > Okay, with that in mind it's functional dependency for the ACPI-based
> > > platforms. Do you want to keep it untouched?
> > 
> > That's not actually what I was thinking of (please read what I wrote
> > above, like I say I was thining about hiding things) but surely if that
> > was a reason to keep the dependency it'd need to be an actual ACPI
> > dependency rather than an ||?
> 
> For my knowledge there is none of the ACPI-based platform where CONFIG_ACPI
> needs to be 'n' while having the real device (as per ACPI ID table) to be on.

to be on --> in a sense of "to be present".

> That's why I answered purely from the compilation point of view.
> 
> Personally I see that dependency more confusing than hinting about anything.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Mark Brown]

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

On Tue, Mar 26, 2024 at 09:20:05PM +0200, Andy Shevchenko wrote:

> For my knowledge there is none of the ACPI-based platform where CONFIG_ACPI
> needs to be 'n' while having the real device (as per ACPI ID table) to be on.
> That's why I answered purely from the compilation point of view.

I don't understand the relevance of that, and frankly can't make much
sense of it.

> Personally I see that dependency more confusing than hinting about anything.

When you don't have a dependency in Kconfig then people get offered the
device even if it is impossible for it to be useful on their platform.
The purpose of any || COMPILE_TEST dependency is to improve the
usability of Kconfig.

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

Re: [PATCH v1 07/10] spi: pxa2xx: Provide num-cs for Sharp PDAs via device properties

[Mark Brown]

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

On Tue, Mar 26, 2024 at 08:50:04PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2024 at 06:21:48PM +0000, Mark Brown wrote:
> > On Tue, Mar 26, 2024 at 08:07:57PM +0200, Andy Shevchenko wrote:

> > > Since driver can parse num-cs device property, replace platform data
> > > with this new approach.

> > But why?

> To be able to hide the header's contents from public.
> Should I update the commit message?

That would definitely help, but it's hard to see what the actual benefit
is here.  It's removing platform data without doing the more difficult
bit where the platform gets converted to DT.

> > > +static const struct property_entry spitz_spi_properties[] = {
> > > +	PROPERTY_ENTRY_U32("num-cs", 3),
> > > +	{ }
> > > +};

> > This is just platform data with less validation AFAICT.

> I'm not sure what validation you are expecting here. It should be done via

Well, the problem with swnode is that there's no validation to expect -
it's an inherent problem with swnode.

> DT schema ideally when the platform gets converted to DT. This change is
> an interim to that (at least it makes kernel side better). After the platform
> code may be gone completely or converted. If the latter happens, we got
> the validation back.

It is not clear to me that this makes the kernel side better, it just
seems to be rewriting the platform data for the sake of it.  If it was
converting to DT there'd be some stuff from it being DT but this keeps
everything as in kernel as board files, just in a more complex form.

> In any case it's not worse than plain DT property handling in the kernel.
> The validation in that case is done elsewhere. Since the property is defined
> in board files the assumed validation is done during development/review
> stages. But OTOH for the legacy code we need not to touch the property
> provider more than once. We are _not_ expecting this to be spread.

I'm guessing you're just checking this by inspection though...

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

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 07:32:47PM +0000, Mark Brown wrote:
> On Tue, Mar 26, 2024 at 09:20:05PM +0200, Andy Shevchenko wrote:
> 
> > For my knowledge there is none of the ACPI-based platform where CONFIG_ACPI
> > needs to be 'n' while having the real device (as per ACPI ID table) to be on.
> > That's why I answered purely from the compilation point of view.
> 
> I don't understand the relevance of that, and frankly can't make much
> sense of it.

It's relevant to the ID table presence at run-time. But it seems I wrongly got
your point (see below).

> > Personally I see that dependency more confusing than hinting about anything.
> 
> When you don't have a dependency in Kconfig then people get offered the
> device even if it is impossible for it to be useful on their platform.

There is currently a dependency among others PCI || ACPI || COMPILE_TEST

From the point of view of the real platforms it means that if there is
a PCI compiled we support PCI devices that use this "platform" driver.
Similar with ACPI.

What you want is to hide this in the menuconfig for the irrelevant platforms
which have PCI _or_ ACPI enabled, correct?

But if we add x86 dependency to that, it will drop the support for non-x86
ACPI-based platforms with this device. I have no clue what are those.

Yes, we may try to have ((PCI || ACPI) && X86) at the end, but I believe
this will have a good regression effect.


> The purpose of any || COMPILE_TEST dependency is to improve the
> usability of Kconfig.

Right, when I see FOO || COMPILE_TEST I interpret FOO as *functional*
dependency, meaning that without FOO the certain driver makes no sense
from functional point of view.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Mark Brown]

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

On Tue, Mar 26, 2024 at 10:04:43PM +0200, Andy Shevchenko wrote:

> There is currently a dependency among others PCI || ACPI || COMPILE_TEST

> From the point of view of the real platforms it means that if there is
> a PCI compiled we support PCI devices that use this "platform" driver.
> Similar with ACPI.

> What you want is to hide this in the menuconfig for the irrelevant platforms
> which have PCI _or_ ACPI enabled, correct?

> But if we add x86 dependency to that, it will drop the support for non-x86
> ACPI-based platforms with this device. I have no clue what are those.

Given the history I would be surprised to learn of any platforms that
have used this other than PXA, MMP or x86.  It's possible Intel shipped
the IP directly exposed on a PCI card of some kind but it'd be pretty
surprising given the way it tends to get used and the idiom for PCI
cards, Marvell having done that would be even more surprising.  The x86
uses I'm aware of are integrated into the chipset rather than something
meaingfully separate to the SoC.

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

Re: [PATCH v1 07/10] spi: pxa2xx: Provide num-cs for Sharp PDAs via device properties

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 08:02:57PM +0000, Mark Brown wrote:
> On Tue, Mar 26, 2024 at 08:50:04PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 26, 2024 at 06:21:48PM +0000, Mark Brown wrote:
> > > On Tue, Mar 26, 2024 at 08:07:57PM +0200, Andy Shevchenko wrote:
> 
> > > > Since driver can parse num-cs device property, replace platform data
> > > > with this new approach.
> 
> > > But why?
> 
> > To be able to hide the header's contents from public.
> > Should I update the commit message?
> 
> That would definitely help, but it's hard to see what the actual benefit
> is here.  It's removing platform data without doing the more difficult
> bit where the platform gets converted to DT.

Will do in v2.

> > > > +static const struct property_entry spitz_spi_properties[] = {
> > > > +	PROPERTY_ENTRY_U32("num-cs", 3),
> > > > +	{ }
> > > > +};
> 
> > > This is just platform data with less validation AFAICT.
> 
> > I'm not sure what validation you are expecting here. It should be done via
> 
> Well, the problem with swnode is that there's no validation to expect -
> it's an inherent problem with swnode.

I do not object this.

> > DT schema ideally when the platform gets converted to DT. This change is
> > an interim to that (at least it makes kernel side better). After the platform
> > code may be gone completely or converted. If the latter happens, we got
> > the validation back.
> 
> It is not clear to me that this makes the kernel side better, it just
> seems to be rewriting the platform data for the sake of it.  If it was
> converting to DT there'd be some stuff from it being DT but this keeps
> everything as in kernel as board files, just in a more complex form.

Not really. The benefits with swnode conversion are the following:

- reducing custom APIs / data types between _shared_ (in a sense of
  supporting zillion different platforms) driver and a certain board
  file

- as an effect of the above, reducing kernel code base, and as the result
  make maintenance easier and bug-free for that parts

- preparing a driver to be ready for any old board file conversion to DT
  as it reduces that churn (you won't need to touch the driver code)

- ...anything else I forgot to mention...

> > In any case it's not worse than plain DT property handling in the kernel.
> > The validation in that case is done elsewhere. Since the property is defined
> > in board files the assumed validation is done during development/review
> > stages. But OTOH for the legacy code we need not to touch the property
> > provider more than once. We are _not_ expecting this to be spread.
> 
> I'm guessing you're just checking this by inspection though...

Yes, we seems do not have any tool to perform a such against software nodes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 01/10] spi: pxa2xx: Drop ACPI_PTR() and of_match_ptr()

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 08:12:09PM +0000, Mark Brown wrote:
> On Tue, Mar 26, 2024 at 10:04:43PM +0200, Andy Shevchenko wrote:
> 
> > There is currently a dependency among others PCI || ACPI || COMPILE_TEST
> 
> > From the point of view of the real platforms it means that if there is
> > a PCI compiled we support PCI devices that use this "platform" driver.
> > Similar with ACPI.
> 
> > What you want is to hide this in the menuconfig for the irrelevant platforms
> > which have PCI _or_ ACPI enabled, correct?
> 
> > But if we add x86 dependency to that, it will drop the support for non-x86
> > ACPI-based platforms with this device. I have no clue what are those.
> 
> Given the history I would be surprised to learn of any platforms that
> have used this other than PXA, MMP or x86.  It's possible Intel shipped
> the IP directly exposed on a PCI card of some kind but it'd be pretty
> surprising given the way it tends to get used and the idiom for PCI
> cards, Marvell having done that would be even more surprising.  The x86
> uses I'm aware of are integrated into the chipset rather than something
> meaingfully separate to the SoC.

Okay, do you want to have an experimental (that we might revert in the future
if any regression being reported) patch to narrow this dependencies as I
described above?

I can prepend the v2 with a such.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 07/10] spi: pxa2xx: Provide num-cs for Sharp PDAs via device properties

[Mark Brown]

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

On Tue, Mar 26, 2024 at 10:12:12PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2024 at 08:02:57PM +0000, Mark Brown wrote:

> > It is not clear to me that this makes the kernel side better, it just
> > seems to be rewriting the platform data for the sake of it.  If it was
> > converting to DT there'd be some stuff from it being DT but this keeps
> > everything as in kernel as board files, just in a more complex form.

> Not really. The benefits with swnode conversion are the following:

> - reducing custom APIs / data types between _shared_ (in a sense of
>   supporting zillion different platforms) driver and a certain board
>   file

> - as an effect of the above, reducing kernel code base, and as the result
>   make maintenance easier and bug-free for that parts

I'm more worried about the possibility of breaking things with swnode
support than I am for board files - with board files you've got a good
chance of failing to compile if things get messed up, with swnode you
can typo a property or whatever and silently fail.  

> - preparing a driver to be ready for any old board file conversion to DT
>   as it reduces that churn (you won't need to touch the driver code)

The driver appears to already have DT support (there's a compatible for
MMP2 in there)?

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

Re: (subset) [PATCH v1 00/10] spi: pxa2xx: Drop linux/spi/pxa2xx_spi.h

[Mark Brown]

On Tue, 26 Mar 2024 20:07:50 +0200, Andy Shevchenko wrote:
> As Arnd suggested we may drop linux/spi/pxa2xx_spi.h as most of
> its content is being used solely internally to SPI subsystem
> (PXA2xx drivers). Hence this refactoring series with the additional
> win of getting rid of legacy documentation.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> 
> [...]

Applied to

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

Thanks!

[02/10] spi: pxa2xx: Keep PXA*_SSP types together
        commit: dad983d8812975b53db83f02ae6b0ad15f018a9e
[03/10] spi: pxa2xx: Switch to use dev_err_probe()
        commit: d5449432f794e75cd4f5e46bc33bfe6ce20b657d

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

Re: [PATCH v1 07/10] spi: pxa2xx: Provide num-cs for Sharp PDAs via device properties

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 08:26:11PM +0000, Mark Brown wrote:
> On Tue, Mar 26, 2024 at 10:12:12PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 26, 2024 at 08:02:57PM +0000, Mark Brown wrote:
> 
> > > It is not clear to me that this makes the kernel side better, it just
> > > seems to be rewriting the platform data for the sake of it.  If it was
> > > converting to DT there'd be some stuff from it being DT but this keeps
> > > everything as in kernel as board files, just in a more complex form.
> 
> > Not really. The benefits with swnode conversion are the following:
> 
> > - reducing custom APIs / data types between _shared_ (in a sense of
> >   supporting zillion different platforms) driver and a certain board
> >   file
> 
> > - as an effect of the above, reducing kernel code base, and as the result
> >   make maintenance easier and bug-free for that parts
> 
> I'm more worried about the possibility of breaking things with swnode
> support than I am for board files - with board files you've got a good
> chance of failing to compile if things get messed up, with swnode you
> can typo a property or whatever and silently fail.

I understand that, but here it's consolidated in a single series
and not supposed to be modified in the future, only dropping or
properly converting.

Btw, you may say the same about the all patches that converts to
GPIO lookup tables (one typo in the not-so-often used GPIO line
device ID name), but I don't remember that kind of conversions
got much of objection.

> > - preparing a driver to be ready for any old board file conversion to DT
> >   as it reduces that churn (you won't need to touch the driver code)
> 
> The driver appears to already have DT support (there's a compatible for
> MMP2 in there)?

The MMP2 is using default number of chip select pins.
Also note that my reply is generic (I used 'a driver' form).

-- 
With Best Regards,
Andy Shevchenko