[PATCH v2 0/5] fpga: don't use drvdata in common fpga code

[PATCH v2 0/5] fpga: don't use drvdata in common fpga code

[Alan Tull]

This patch set goes on top of v6 of the non-dt support patchset that has
been acked and I've asked Greg KH to send upstream.

This patchset changes the fpga_*_register functions to not set drvdata,
and changes parameters for register/unregister functions.  Also a
bug fix due to changes in the DT overlay code.

Setting drvdata is fine for DT based devices, that will have one
manager, bridge, or region device per platform device.  PCIe based
devices may have multiple FPGA mgr/bridge/regions under one pcie
device.  Without these changes, PCIe-based solutions have to create an
extra device for each child mgr/bridge/region to hold drvdata.

Also changing the fpga_*_register/unregister functions to take the
corresponding struct as the only parameter wherever they weren't already.

* int fpga_bridge_register(struct fpga_bridge *br)
* int fpga_mgr_register(struct fpga_manager *mgr)
* int fpga_region_register(struct fpga_region *region)
* void fpga_bridge_unregister(struct fpga_bridge *br)
* void fpga_mgr_unregister(struct fpga_manager *mgr)
* int fpga_region_unregister(struct fpga_region *region)

Other changes:

- Callers of fpga_(bridge|mgr)_register, are changed to alloc the
bridge/mgr struct and partly fill it, adding name, ops and priv.
- Caller uses devm for allocating the mgr/bridge structures.
- Change the FPGA Region DT notifier to return NOTIFY_STOP if
the overlay should be accepted.

I posted a branch to the linux-fpga repo on kernel.org which has both
the v6 non-DT support patchset and this v2 patchset for convenience.
Branch name is next-20171113-non-dt-support-v6+v2

Alan

Alan Tull (5):
  fpga: region: don't use drvdata in common fpga code
  fpga: manager: don't use drvdata in common fpga code
  fpga: bridge: don't use drvdata in common fpga code
  fpga: region: change fpga_region_register to have one param
  fpga: region: return NOTIFY_STOP if overlay shoud be accepted

 Documentation/fpga/fpga-mgr.txt     | 24 ++++++++++++-----
 Documentation/fpga/fpga-region.txt  |  3 +--
 drivers/fpga/altera-cvp.c           | 18 ++++++++++---
 drivers/fpga/altera-fpga2sdram.c    | 20 +++++++++++---
 drivers/fpga/altera-freeze-bridge.c | 18 ++++++++++---
 drivers/fpga/altera-hps2fpga.c      | 16 ++++++++---
 drivers/fpga/altera-pr-ip-core.c    | 17 ++++++++++--
 drivers/fpga/altera-ps-spi.c        | 18 ++++++++++---
 drivers/fpga/fpga-bridge.c          | 53 ++++++++++++++-----------------------
 drivers/fpga/fpga-mgr.c             | 49 +++++++++++++---------------------
 drivers/fpga/fpga-region.c          | 13 ++++++---
 drivers/fpga/ice40-spi.c            | 20 +++++++++++---
 drivers/fpga/of-fpga-region.c       |  9 ++++---
 drivers/fpga/socfpga-a10.c          | 16 ++++++++---
 drivers/fpga/socfpga.c              | 18 ++++++++++---
 drivers/fpga/ts73xx-fpga.c          | 18 ++++++++++---
 drivers/fpga/xilinx-pr-decoupler.c  | 15 ++++++++---
 drivers/fpga/xilinx-spi.c           | 18 ++++++++++---
 drivers/fpga/zynq-fpga.c            | 16 ++++++++---
 include/linux/fpga/fpga-bridge.h    |  7 ++---
 include/linux/fpga/fpga-mgr.h       |  8 +++---
 include/linux/fpga/fpga-region.h    |  4 ++-
 22 files changed, 270 insertions(+), 128 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] fpga: region: don't use drvdata in common fpga code

[Alan Tull]

Part of patchset that changes the following fpga_*_register
functions to not set drvdata:
* fpga_region_register.
* fpga_mgr_register
* fpga_bridge_register

The rationale is that setting drvdata is fine for DT based devices
that will have one manager, bridge, or region per platform device.
However PCIe based devices may have multiple FPGA mgr/bridge/regions
under one PCIe device.  Without these changes, the PCIe solution has
to create an extra device for each child mgr/bridge/region to hold
drvdata.

Signed-off-by: Alan Tull <atull@kernel.org>
Reported-by: Jiuyue Ma <majiuyue@huawei.com>
---
v2: No change to this patch in v2 of patchset
---
 drivers/fpga/fpga-region.c    | 1 -
 drivers/fpga/of-fpga-region.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index edab2a2..ebe1f87 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -178,7 +178,6 @@ int fpga_region_register(struct device *dev, struct fpga_region *region)
 	region->dev.parent = dev;
 	region->dev.of_node = dev->of_node;
 	region->dev.id = id;
-	dev_set_drvdata(dev, region);
 
 	ret = dev_set_name(&region->dev, "region%d", id);
 	if (ret)
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index c6b2119..3079ed8 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -431,6 +431,7 @@ static int of_fpga_region_probe(struct platform_device *pdev)
 		goto eprobe_mgr_put;
 
 	of_platform_populate(np, fpga_region_of_match, NULL, &region->dev);
+	dev_set_drvdata(dev, region);
 
 	dev_info(dev, "FPGA Region probed\n");
 
-- 
2.7.4

Re: [PATCH v2 1/5] fpga: region: don't use drvdata in common fpga code

[Moritz Fischer]

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

On Wed, Nov 15, 2017 at 02:51:48PM -0600, Alan Tull wrote:
> Part of patchset that changes the following fpga_*_register
> functions to not set drvdata:
> * fpga_region_register.
> * fpga_mgr_register
> * fpga_bridge_register
> 
> The rationale is that setting drvdata is fine for DT based devices
> that will have one manager, bridge, or region per platform device.
> However PCIe based devices may have multiple FPGA mgr/bridge/regions
> under one PCIe device.  Without these changes, the PCIe solution has
> to create an extra device for each child mgr/bridge/region to hold
> drvdata.
> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> Reported-by: Jiuyue Ma <majiuyue@huawei.com>
Acked-by: Moritz Fischer <mdf@kernel.org>
> ---
> v2: No change to this patch in v2 of patchset
> ---
>  drivers/fpga/fpga-region.c    | 1 -
>  drivers/fpga/of-fpga-region.c | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index edab2a2..ebe1f87 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -178,7 +178,6 @@ int fpga_region_register(struct device *dev, struct fpga_region *region)
>  	region->dev.parent = dev;
>  	region->dev.of_node = dev->of_node;
>  	region->dev.id = id;
> -	dev_set_drvdata(dev, region);
>  
>  	ret = dev_set_name(&region->dev, "region%d", id);
>  	if (ret)
> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
> index c6b2119..3079ed8 100644
> --- a/drivers/fpga/of-fpga-region.c
> +++ b/drivers/fpga/of-fpga-region.c
> @@ -431,6 +431,7 @@ static int of_fpga_region_probe(struct platform_device *pdev)
>  		goto eprobe_mgr_put;
>  
>  	of_platform_populate(np, fpga_region_of_match, NULL, &region->dev);
> +	dev_set_drvdata(dev, region);
>  
>  	dev_info(dev, "FPGA Region probed\n");
>  
> -- 
> 2.7.4
> 

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

Re: [PATCH v2 1/5] fpga: region: don't use drvdata in common fpga code

[Alan Tull]

On Mon, Dec 4, 2017 at 3:32 PM, Moritz Fischer <mdf@kernel.org> wrote:
> On Wed, Nov 15, 2017 at 02:51:48PM -0600, Alan Tull wrote:
>> Part of patchset that changes the following fpga_*_register
>> functions to not set drvdata:
>> * fpga_region_register.
>> * fpga_mgr_register
>> * fpga_bridge_register
>>
>> The rationale is that setting drvdata is fine for DT based devices
>> that will have one manager, bridge, or region per platform device.
>> However PCIe based devices may have multiple FPGA mgr/bridge/regions
>> under one PCIe device.  Without these changes, the PCIe solution has
>> to create an extra device for each child mgr/bridge/region to hold
>> drvdata.
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> Reported-by: Jiuyue Ma <majiuyue@huawei.com>
> Acked-by: Moritz Fischer <mdf@kernel.org>

Thanks!
Alan

[PATCH v2 2/5] fpga: manager: don't use drvdata in common fpga code

[Alan Tull]

Changes to the fpga manager code to not use drvdata in common
code.

Change fpga_mgr_register to not set or use drvdata.

Change the register/unregister function parameters to take the mgr
struct:
* int fpga_mgr_register(struct fpga_manager *mgr);
* void fpga_mgr_unregister(struct fpga_manager *mgr);

Change the drivers that call fpga_mgr_register to alloc the struct
fpga_manager (using devm_kzalloc) and partly fill it, adding name,
ops, parent device, and priv.

The rationale is that setting drvdata is fine for DT based devices
that will have one manager, bridge, or region per platform device.
However PCIe based devices may have multiple FPGA mgr/bridge/regions
under one pcie device.  Without these changes, the PCIe solution has
to create an extra device for each child mgr/bridge/region to hold
drvdata.

Signed-off-by: Alan Tull <atull@kernel.org>
Reported-by: Jiuyue Ma <majiuyue@huawei.com>
---
v2: change fpga_mgr_register to not need parent device param
---
 Documentation/fpga/fpga-mgr.txt  | 24 ++++++++++++++------
 drivers/fpga/altera-cvp.c        | 18 +++++++++++----
 drivers/fpga/altera-pr-ip-core.c | 17 ++++++++++++--
 drivers/fpga/altera-ps-spi.c     | 18 ++++++++++++---
 drivers/fpga/fpga-mgr.c          | 49 ++++++++++++++++------------------------
 drivers/fpga/ice40-spi.c         | 20 ++++++++++++----
 drivers/fpga/socfpga-a10.c       | 16 ++++++++++---
 drivers/fpga/socfpga.c           | 18 ++++++++++++---
 drivers/fpga/ts73xx-fpga.c       | 18 ++++++++++++---
 drivers/fpga/xilinx-spi.c        | 18 ++++++++++++---
 drivers/fpga/zynq-fpga.c         | 16 ++++++++++---
 include/linux/fpga/fpga-mgr.h    |  8 +++----
 12 files changed, 171 insertions(+), 69 deletions(-)

diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
index cc6413e..5b8b274 100644
--- a/Documentation/fpga/fpga-mgr.txt
+++ b/Documentation/fpga/fpga-mgr.txt
@@ -67,11 +67,9 @@ fpga_mgr_unlock when done programming the FPGA.
 To register or unregister the low level FPGA-specific driver:
 -------------------------------------------------------------
 
-	int fpga_mgr_register(struct device *dev, const char *name,
-			      const struct fpga_manager_ops *mops,
-			      void *priv);
+	int fpga_mgr_register(struct fpga_manager *mgr);
 
-	void fpga_mgr_unregister(struct device *dev);
+	void fpga_mgr_unregister(struct fpga_manager *mgr);
 
 Use of these two functions is described below in "How To Support a new FPGA
 device."
@@ -148,8 +146,13 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct socfpga_fpga_priv *priv;
+	struct fpga_manager *mgr;
 	int ret;
 
+	mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -157,13 +160,20 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
 	/* ... do ioremaps, get interrupts, etc. and save
 	   them in priv... */
 
-	return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
-				 &socfpga_fpga_ops, priv);
+	mgr->name = "Altera SOCFPGA FPGA Manager";
+	mgr->mops = &socfpga_fpga_ops;
+	mgr->priv = priv;
+	mgr->parent = dev;
+	platform_set_drvdata(pdev, mgr);
+
+	return fpga_mgr_register(mgr);
 }
 
 static int socfpga_fpga_remove(struct platform_device *pdev)
 {
-	fpga_mgr_unregister(&pdev->dev);
+	struct fpga_manager *mgr = platform_get_drvdata(pdev);
+
+	fpga_mgr_unregister(mgr);
 
 	return 0;
 }
diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index 00e73d2..384c99d 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -403,6 +403,7 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 			    const struct pci_device_id *dev_id)
 {
 	struct altera_cvp_conf *conf;
+	struct fpga_manager *mgr;
 	u16 cmd, val;
 	int ret;
 
@@ -421,6 +422,10 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 	if (!conf)
 		return -ENOMEM;
 
+	mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
 	/*
 	 * Enable memory BAR access. We cannot use pci_enable_device() here
 	 * because it will make the driver unusable with FPGA devices that
@@ -454,8 +459,13 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 	snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%s",
 		 ALTERA_CVP_MGR_NAME, pci_name(pdev));
 
-	ret = fpga_mgr_register(&pdev->dev, conf->mgr_name,
-				&altera_cvp_ops, conf);
+	mgr->name = conf->mgr_name;
+	mgr->mops = &altera_cvp_ops;
+	mgr->priv = conf;
+	mgr->parent = &pdev->dev;
+	pci_set_drvdata(pdev, mgr);
+
+	ret = fpga_mgr_register(mgr);
 	if (ret)
 		goto err_unmap;
 
@@ -463,7 +473,7 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 				 &driver_attr_chkcfg);
 	if (ret) {
 		dev_err(&pdev->dev, "Can't create sysfs chkcfg file\n");
-		fpga_mgr_unregister(&pdev->dev);
+		fpga_mgr_unregister(mgr);
 		goto err_unmap;
 	}
 
@@ -485,7 +495,7 @@ static void altera_cvp_remove(struct pci_dev *pdev)
 	u16 cmd;
 
 	driver_remove_file(&altera_cvp_driver.driver, &driver_attr_chkcfg);
-	fpga_mgr_unregister(&pdev->dev);
+	fpga_mgr_unregister(mgr);
 	pci_iounmap(pdev, conf->map);
 	pci_release_region(pdev, CVP_BAR);
 	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
index a7b31f9..a6da91b 100644
--- a/drivers/fpga/altera-pr-ip-core.c
+++ b/drivers/fpga/altera-pr-ip-core.c
@@ -187,8 +187,13 @@ static const struct fpga_manager_ops alt_pr_ops = {
 int alt_pr_register(struct device *dev, void __iomem *reg_base)
 {
 	struct alt_pr_priv *priv;
+	struct fpga_manager *mgr;
 	u32 val;
 
+	mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -201,15 +206,23 @@ int alt_pr_register(struct device *dev, void __iomem *reg_base)
 		(val & ALT_PR_CSR_STATUS_MSK) >> ALT_PR_CSR_STATUS_SFT,
 		(int)(val & ALT_PR_CSR_PR_START));
 
-	return fpga_mgr_register(dev, dev_name(dev), &alt_pr_ops, priv);
+	mgr->name = dev_name(dev);
+	mgr->mops = &alt_pr_ops;
+	mgr->priv = priv;
+	mgr->parent = dev;
+	dev_set_drvdata(dev, mgr);
+
+	return fpga_mgr_register(mgr);
 }
 EXPORT_SYMBOL_GPL(alt_pr_register);
 
 int alt_pr_unregister(struct device *dev)
 {
+	struct fpga_manager *mgr = dev_get_drvdata(dev);
+
 	dev_dbg(dev, "%s\n", __func__);
 
-	fpga_mgr_unregister(dev);
+	fpga_mgr_unregister(mgr);
 
 	return 0;
 }
diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
index 14f14ef..838d15a 100644
--- a/drivers/fpga/altera-ps-spi.c
+++ b/drivers/fpga/altera-ps-spi.c
@@ -238,6 +238,11 @@ static int altera_ps_probe(struct spi_device *spi)
 {
 	struct altera_ps_conf *conf;
 	const struct of_device_id *of_id;
+	struct fpga_manager *mgr;
+
+	mgr = devm_kzalloc(&spi->dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
 
 	conf = devm_kzalloc(&spi->dev, sizeof(*conf), GFP_KERNEL);
 	if (!conf)
@@ -273,13 +278,20 @@ static int altera_ps_probe(struct spi_device *spi)
 	snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s %s",
 		 dev_driver_string(&spi->dev), dev_name(&spi->dev));
 
-	return fpga_mgr_register(&spi->dev, conf->mgr_name,
-				 &altera_ps_ops, conf);
+	mgr->name = conf->mgr_name;
+	mgr->mops = &altera_ps_ops;
+	mgr->priv = conf;
+	mgr->parent = &spi->dev;
+	spi_set_drvdata(spi, mgr);
+
+	return fpga_mgr_register(mgr);
 }
 
 static int altera_ps_remove(struct spi_device *spi)
 {
-	fpga_mgr_unregister(&spi->dev);
+	struct fpga_manager *mgr = spi_get_drvdata(spi);
+
+	fpga_mgr_unregister(mgr);
 
 	return 0;
 }
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 223f240..1fd5494 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -518,48 +518,43 @@ EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
 
 /**
  * fpga_mgr_register - register a low level fpga manager driver
- * @dev:	fpga manager device from pdev
- * @name:	fpga manager name
- * @mops:	pointer to structure of fpga manager ops
- * @priv:	fpga manager private data
+ * @mgr:	fpga manager struct
+ *
+ * The following fields of mgr must be set: name, mops, and parent.
  *
  * Return: 0 on success, negative error code otherwise.
  */
-int fpga_mgr_register(struct device *dev, const char *name,
-		      const struct fpga_manager_ops *mops,
-		      void *priv)
+int fpga_mgr_register(struct fpga_manager *mgr)
 {
-	struct fpga_manager *mgr;
+	const struct fpga_manager_ops *mops;
+	struct device *parent = mgr->parent;
 	int id, ret;
 
+	if (!parent) {
+		pr_err("Attempt to register fpga manager without parent\n");
+		return -EINVAL;
+	}
+
+	mops = mgr->mops;
 	if (!mops || !mops->write_complete || !mops->state ||
 	    !mops->write_init || (!mops->write && !mops->write_sg) ||
 	    (mops->write && mops->write_sg)) {
-		dev_err(dev, "Attempt to register without fpga_manager_ops\n");
+		dev_err(parent, "Attempt to register without fpga_manager_ops\n");
 		return -EINVAL;
 	}
 
-	if (!name || !strlen(name)) {
-		dev_err(dev, "Attempt to register with no name!\n");
+	if (!mgr->name || !strlen(mgr->name)) {
+		dev_err(parent, "Attempt to register with no name!\n");
 		return -EINVAL;
 	}
 
-	mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
-	if (!mgr)
-		return -ENOMEM;
-
 	id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
 	if (id < 0) {
-		ret = id;
-		goto error_kfree;
+		return id;
 	}
 
 	mutex_init(&mgr->ref_mutex);
 
-	mgr->name = name;
-	mgr->mops = mops;
-	mgr->priv = priv;
-
 	/*
 	 * Initialize framework state by requesting low level driver read state
 	 * from device.  FPGA may be in reset mode or may have been programmed
@@ -570,10 +565,9 @@ int fpga_mgr_register(struct device *dev, const char *name,
 	device_initialize(&mgr->dev);
 	mgr->dev.class = fpga_mgr_class;
 	mgr->dev.groups = mops->groups;
-	mgr->dev.parent = dev;
-	mgr->dev.of_node = dev->of_node;
+	mgr->dev.parent = parent;
+	mgr->dev.of_node = parent->of_node;
 	mgr->dev.id = id;
-	dev_set_drvdata(dev, mgr);
 
 	ret = dev_set_name(&mgr->dev, "fpga%d", id);
 	if (ret)
@@ -589,8 +583,6 @@ int fpga_mgr_register(struct device *dev, const char *name,
 
 error_device:
 	ida_simple_remove(&fpga_mgr_ida, id);
-error_kfree:
-	kfree(mgr);
 
 	return ret;
 }
@@ -600,10 +592,8 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register);
  * fpga_mgr_unregister - unregister a low level fpga manager driver
  * @dev:	fpga manager device from pdev
  */
-void fpga_mgr_unregister(struct device *dev)
+void fpga_mgr_unregister(struct fpga_manager *mgr)
 {
-	struct fpga_manager *mgr = dev_get_drvdata(dev);
-
 	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
 
 	/*
@@ -622,7 +612,6 @@ static void fpga_mgr_dev_release(struct device *dev)
 	struct fpga_manager *mgr = to_fpga_manager(dev);
 
 	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
-	kfree(mgr);
 }
 
 static int __init fpga_mgr_class_init(void)
diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c
index 7fca820..cdf0711 100644
--- a/drivers/fpga/ice40-spi.c
+++ b/drivers/fpga/ice40-spi.c
@@ -133,8 +133,13 @@ static int ice40_fpga_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
 	struct ice40_fpga_priv *priv;
+	struct fpga_manager *mgr;
 	int ret;
 
+	mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
 	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -174,14 +179,21 @@ static int ice40_fpga_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	/* Register with the FPGA manager */
-	return fpga_mgr_register(dev, "Lattice iCE40 FPGA Manager",
-				 &ice40_fpga_ops, priv);
+	mgr->name = "Lattice iCE40 FPGA Manager";
+	mgr->mops = &ice40_fpga_ops;
+	mgr->priv = priv;
+	mgr->parent = dev;
+	spi_set_drvdata(spi, mgr);
+
+	return fpga_mgr_register(mgr);
 }
 
 static int ice40_fpga_remove(struct spi_device *spi)
 {
-	fpga_mgr_unregister(&spi->dev);
+	struct fpga_manager *mgr = spi_get_drvdata(spi);
+
+	fpga_mgr_unregister(mgr);
+
 	return 0;
 }
 
diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c
index f8770af..1c0c8c5 100644
--- a/drivers/fpga/socfpga-a10.c
+++ b/drivers/fpga/socfpga-a10.c
@@ -482,6 +482,7 @@ static int socfpga_a10_fpga_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct a10_fpga_priv *priv;
 	void __iomem *reg_base;
+	struct fpga_manager *mgr;
 	struct resource *res;
 	int ret;
 
@@ -519,8 +520,17 @@ static int socfpga_a10_fpga_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 
-	return fpga_mgr_register(dev, "SoCFPGA Arria10 FPGA Manager",
-				 &socfpga_a10_fpga_mgr_ops, priv);
+	mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
+	mgr->name = "SoCFPGA Arria10 FPGA Manager";
+	mgr->mops = &socfpga_a10_fpga_mgr_ops;
+	mgr->priv = priv;
+	mgr->parent = dev;
+	platform_set_drvdata(pdev, mgr);
+
+	return fpga_mgr_register(mgr);
 }
 
 static int socfpga_a10_fpga_remove(struct platform_device *pdev)
@@ -528,7 +538,7 @@ static int socfpga_a10_fpga_remove(struct platform_device *pdev)
 	struct fpga_manager *mgr = platform_get_drvdata(pdev);
 	struct a10_fpga_priv *priv = mgr->priv;
 
-	fpga_mgr_unregister(&pdev->dev);
+	fpga_mgr_unregister(mgr);
 	clk_disable_unprepare(priv->clk);
 
 	return 0;
diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
index b6672e6..b63cb53 100644
--- a/drivers/fpga/socfpga.c
+++ b/drivers/fpga/socfpga.c
@@ -555,9 +555,14 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct socfpga_fpga_priv *priv;
+	struct fpga_manager *mgr;
 	struct resource *res;
 	int ret;
 
+	mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -581,13 +586,20 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
-				 &socfpga_fpga_ops, priv);
+	mgr->name = "Altera SOCFPGA FPGA Manager";
+	mgr->mops = &socfpga_fpga_ops;
+	mgr->priv = priv;
+	mgr->parent = dev;
+	platform_set_drvdata(pdev, mgr);
+
+	return fpga_mgr_register(mgr);
 }
 
 static int socfpga_fpga_remove(struct platform_device *pdev)
 {
-	fpga_mgr_unregister(&pdev->dev);
+	struct fpga_manager *mgr = platform_get_drvdata(pdev);
+
+	fpga_mgr_unregister(mgr);
 
 	return 0;
 }
diff --git a/drivers/fpga/ts73xx-fpga.c b/drivers/fpga/ts73xx-fpga.c
index f6a96b4..5b8150c 100644
--- a/drivers/fpga/ts73xx-fpga.c
+++ b/drivers/fpga/ts73xx-fpga.c
@@ -116,8 +116,13 @@ static int ts73xx_fpga_probe(struct platform_device *pdev)
 {
 	struct device *kdev = &pdev->dev;
 	struct ts73xx_fpga_priv *priv;
+	struct fpga_manager *mgr;
 	struct resource *res;
 
+	mgr = devm_kzalloc(kdev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
 	priv = devm_kzalloc(kdev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -131,13 +136,20 @@ static int ts73xx_fpga_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->io_base);
 	}
 
-	return fpga_mgr_register(kdev, "TS-73xx FPGA Manager",
-				 &ts73xx_fpga_ops, priv);
+	mgr->name = "TS-73xx FPGA Manager";
+	mgr->mops = &ts73xx_fpga_ops;
+	mgr->priv = priv;
+	mgr->parent = kdev;
+	platform_set_drvdata(pdev, mgr);
+
+	return fpga_mgr_register(mgr);
 }
 
 static int ts73xx_fpga_remove(struct platform_device *pdev)
 {
-	fpga_mgr_unregister(&pdev->dev);
+	struct fpga_manager *mgr = platform_get_drvdata(pdev);
+
+	fpga_mgr_unregister(mgr);
 
 	return 0;
 }
diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index 9b62a4c..3fe9d9e 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -143,6 +143,11 @@ static const struct fpga_manager_ops xilinx_spi_ops = {
 static int xilinx_spi_probe(struct spi_device *spi)
 {
 	struct xilinx_spi_conf *conf;
+	struct fpga_manager *mgr;
+
+	mgr = devm_kzalloc(&spi->dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
 
 	conf = devm_kzalloc(&spi->dev, sizeof(*conf), GFP_KERNEL);
 	if (!conf)
@@ -165,13 +170,20 @@ static int xilinx_spi_probe(struct spi_device *spi)
 		return PTR_ERR(conf->done);
 	}
 
-	return fpga_mgr_register(&spi->dev, "Xilinx Slave Serial FPGA Manager",
-				 &xilinx_spi_ops, conf);
+	mgr->name = "Xilinx Slave Serial FPGA Manager";
+	mgr->mops = &xilinx_spi_ops;
+	mgr->priv = conf;
+	mgr->parent = &spi->dev;
+	spi_set_drvdata(spi, mgr);
+
+	return fpga_mgr_register(mgr);
 }
 
 static int xilinx_spi_remove(struct spi_device *spi)
 {
-	fpga_mgr_unregister(&spi->dev);
+	struct fpga_manager *mgr = spi_get_drvdata(spi);
+
+	fpga_mgr_unregister(mgr);
 
 	return 0;
 }
diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..b4e60aee 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -558,9 +558,14 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct zynq_fpga_priv *priv;
+	struct fpga_manager *mgr;
 	struct resource *res;
 	int err;
 
+	mgr = devm_kzalloc(dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -613,8 +618,13 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 
 	clk_disable(priv->clk);
 
-	err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
-				&zynq_fpga_ops, priv);
+	mgr->name = "Xilinx Zynq FPGA Manager";
+	mgr->mops = &zynq_fpga_ops;
+	mgr->priv = priv;
+	mgr->parent = dev;
+	platform_set_drvdata(pdev, mgr);
+
+	err = fpga_mgr_register(mgr);
 	if (err) {
 		dev_err(dev, "unable to register FPGA manager\n");
 		clk_unprepare(priv->clk);
@@ -632,7 +642,7 @@ static int zynq_fpga_remove(struct platform_device *pdev)
 	mgr = platform_get_drvdata(pdev);
 	priv = mgr->priv;
 
-	fpga_mgr_unregister(&pdev->dev);
+	fpga_mgr_unregister(mgr);
 
 	clk_unprepare(priv->clk);
 
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..1ad48228 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -138,6 +138,7 @@ struct fpga_manager_ops {
 /**
  * struct fpga_manager - fpga manager structure
  * @name: name of low level fpga manager
+ * @parent: parent device
  * @dev: fpga manager device
  * @ref_mutex: only allows one reference to fpga manager
  * @state: state of fpga manager
@@ -146,6 +147,7 @@ struct fpga_manager_ops {
  */
 struct fpga_manager {
 	const char *name;
+	struct device *parent;
 	struct device dev;
 	struct mutex ref_mutex;
 	enum fpga_mgr_states state;
@@ -170,9 +172,7 @@ struct fpga_manager *fpga_mgr_get(struct device *dev);
 
 void fpga_mgr_put(struct fpga_manager *mgr);
 
-int fpga_mgr_register(struct device *dev, const char *name,
-		      const struct fpga_manager_ops *mops, void *priv);
-
-void fpga_mgr_unregister(struct device *dev);
+int fpga_mgr_register(struct fpga_manager *mgr);
+void fpga_mgr_unregister(struct fpga_manager *mgr);
 
 #endif /*_LINUX_FPGA_MGR_H */
-- 
2.7.4

[PATCH v2 3/5] fpga: bridge: don't use drvdata in common fpga code

[Alan Tull]

Changes to the fpga bridge code to not use drvdata in common code.

Change fpga_bridge_register to not set drvdata.

Change the register/unregister functions parameters to take the
bridge struct:
* int fpga_bridge_register(struct fpga_bridge *bridge);
* void fpga_bridge_unregister(struct fpga_bridge *bridge);

Change the drivers that call fpga_bridge_register to alloc the struct
fpga_bridge (using devm_kzalloc) and partly fill it, adding name,
ops, parent device, and priv.

The rationale is that setting drvdata is fine for DT based devices
that will have one manager, bridge, or region per platform device.
However PCIe based devices may have multiple FPGA mgr/bridge/regions
under one pcie device.  Without these changes, the PCIe solution has
to create an extra device for each child mgr/bridge/region to hold
drvdata.

Signed-off-by: Alan Tull <atull@kernel.org>
Reported-by: Jiuyue Ma <majiuyue@huawei.com>
---
v2: change fpga_bridge_register to not need parent device param
    fix undeclared - s/dev/&pdev->dev/
---
 drivers/fpga/altera-fpga2sdram.c    | 20 +++++++++++---
 drivers/fpga/altera-freeze-bridge.c | 18 ++++++++++---
 drivers/fpga/altera-hps2fpga.c      | 16 ++++++++---
 drivers/fpga/fpga-bridge.c          | 53 ++++++++++++++-----------------------
 drivers/fpga/xilinx-pr-decoupler.c  | 15 ++++++++---
 include/linux/fpga/fpga-bridge.h    |  7 ++---
 6 files changed, 80 insertions(+), 49 deletions(-)

diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
index d4eeb74..84effb0 100644
--- a/drivers/fpga/altera-fpga2sdram.c
+++ b/drivers/fpga/altera-fpga2sdram.c
@@ -106,10 +106,15 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct alt_fpga2sdram_data *priv;
+	struct fpga_bridge *br;
 	u32 enable;
 	struct regmap *sysmgr;
 	int ret = 0;
 
+	br = devm_kzalloc(dev, sizeof(*br), GFP_KERNEL);
+	if (!br)
+		return -ENOMEM;
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -131,8 +136,13 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
 	/* Get f2s bridge configuration saved in handoff register */
 	regmap_read(sysmgr, SYSMGR_ISWGRP_HANDOFF3, &priv->mask);
 
-	ret = fpga_bridge_register(dev, F2S_BRIDGE_NAME,
-				   &altera_fpga2sdram_br_ops, priv);
+	br->name = F2S_BRIDGE_NAME;
+	br->br_ops = &altera_fpga2sdram_br_ops;
+	br->priv = priv;
+	br->parent = dev;
+	platform_set_drvdata(pdev, br);
+
+	ret = fpga_bridge_register(br);
 	if (ret)
 		return ret;
 
@@ -146,7 +156,7 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
 				 (enable ? "enabling" : "disabling"));
 			ret = _alt_fpga2sdram_enable_set(priv, enable);
 			if (ret) {
-				fpga_bridge_unregister(&pdev->dev);
+				fpga_bridge_unregister(br);
 				return ret;
 			}
 		}
@@ -157,7 +167,9 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
 
 static int alt_fpga_bridge_remove(struct platform_device *pdev)
 {
-	fpga_bridge_unregister(&pdev->dev);
+	struct fpga_bridge *br = platform_get_drvdata(pdev);
+
+	fpga_bridge_unregister(br);
 
 	return 0;
 }
diff --git a/drivers/fpga/altera-freeze-bridge.c b/drivers/fpga/altera-freeze-bridge.c
index 6159cfcf..b49a8a3 100644
--- a/drivers/fpga/altera-freeze-bridge.c
+++ b/drivers/fpga/altera-freeze-bridge.c
@@ -219,6 +219,7 @@ static int altera_freeze_br_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = pdev->dev.of_node;
+	struct fpga_bridge *br;
 	void __iomem *base_addr;
 	struct altera_freeze_br_data *priv;
 	struct resource *res;
@@ -227,6 +228,10 @@ static int altera_freeze_br_probe(struct platform_device *pdev)
 	if (!np)
 		return -ENODEV;
 
+	br = devm_kzalloc(dev, sizeof(*br), GFP_KERNEL);
+	if (!br)
+		return -ENOMEM;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base_addr = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base_addr))
@@ -254,13 +259,20 @@ static int altera_freeze_br_probe(struct platform_device *pdev)
 
 	priv->base_addr = base_addr;
 
-	return fpga_bridge_register(dev, FREEZE_BRIDGE_NAME,
-				    &altera_freeze_br_br_ops, priv);
+	br->name = FREEZE_BRIDGE_NAME;
+	br->br_ops = &altera_freeze_br_br_ops;
+	br->priv = priv;
+	br->parent = dev;
+	platform_set_drvdata(pdev, br);
+
+	return fpga_bridge_register(br);
 }
 
 static int altera_freeze_br_remove(struct platform_device *pdev)
 {
-	fpga_bridge_unregister(&pdev->dev);
+	struct fpga_bridge *br = platform_get_drvdata(pdev);
+
+	fpga_bridge_unregister(br);
 
 	return 0;
 }
diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
index 406d2f1..7256b5c 100644
--- a/drivers/fpga/altera-hps2fpga.c
+++ b/drivers/fpga/altera-hps2fpga.c
@@ -139,6 +139,7 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct altera_hps2fpga_data *priv;
 	const struct of_device_id *of_id;
+	struct fpga_bridge *br;
 	u32 enable;
 	int ret;
 
@@ -150,6 +151,10 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
 
 	priv = (struct altera_hps2fpga_data *)of_id->data;
 
+	br = devm_kzalloc(dev, sizeof(*br), GFP_KERNEL);
+	if (!br)
+		return -ENOMEM;
+
 	priv->bridge_reset = of_reset_control_get_exclusive_by_index(dev->of_node,
 								     0);
 	if (IS_ERR(priv->bridge_reset)) {
@@ -190,8 +195,13 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = fpga_bridge_register(dev, priv->name, &altera_hps2fpga_br_ops,
-				   priv);
+	br->name = priv->name;
+	br->br_ops = &altera_hps2fpga_br_ops;
+	br->priv = priv;
+	br->parent = dev;
+	platform_set_drvdata(pdev, br);
+
+	ret = fpga_bridge_register(br);
 err:
 	if (ret)
 		clk_disable_unprepare(priv->clk);
@@ -204,7 +214,7 @@ static int alt_fpga_bridge_remove(struct platform_device *pdev)
 	struct fpga_bridge *bridge = platform_get_drvdata(pdev);
 	struct altera_hps2fpga_data *priv = bridge->priv;
 
-	fpga_bridge_unregister(&pdev->dev);
+	fpga_bridge_unregister(bridge);
 
 	clk_disable_unprepare(priv->clk);
 
diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
index e693c36..4c48dd6 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -331,48 +331,41 @@ ATTRIBUTE_GROUPS(fpga_bridge);
 
 /**
  * fpga_bridge_register - register a fpga bridge driver
- * @dev:	FPGA bridge device from pdev
- * @name:	FPGA bridge name
- * @br_ops:	pointer to structure of fpga bridge ops
- * @priv:	FPGA bridge private data
+ * @bridge:	FPGA bridge struct
+ *
+ * The following fields must be set in the bridge struct:
+ * name, br_ops, and parent.
  *
  * Return: 0 for success, error code otherwise.
  */
-int fpga_bridge_register(struct device *dev, const char *name,
-			 const struct fpga_bridge_ops *br_ops, void *priv)
+int fpga_bridge_register(struct fpga_bridge *bridge)
 {
-	struct fpga_bridge *bridge;
 	int id, ret = 0;
+	struct device *parent = bridge->parent;
 
-	if (!name || !strlen(name)) {
-		dev_err(dev, "Attempt to register with no name!\n");
+	if (!parent) {
+		pr_err("Attempt to register fpga bridge without parent\n");
 		return -EINVAL;
 	}
 
-	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
-	if (!bridge)
-		return -ENOMEM;
+	if (!bridge->name || !strlen(bridge->name)) {
+		dev_err(parent, "Attempt to register with no name!\n");
+		return -EINVAL;
+	}
 
 	id = ida_simple_get(&fpga_bridge_ida, 0, 0, GFP_KERNEL);
-	if (id < 0) {
-		ret = id;
-		goto error_kfree;
-	}
+	if (id < 0)
+		return id;
 
 	mutex_init(&bridge->mutex);
 	INIT_LIST_HEAD(&bridge->node);
 
-	bridge->name = name;
-	bridge->br_ops = br_ops;
-	bridge->priv = priv;
-
 	device_initialize(&bridge->dev);
-	bridge->dev.groups = br_ops->groups;
+	bridge->dev.groups = bridge->br_ops->groups;
 	bridge->dev.class = fpga_bridge_class;
-	bridge->dev.parent = dev;
-	bridge->dev.of_node = dev->of_node;
+	bridge->dev.parent = parent;
+	bridge->dev.of_node = parent->of_node;
 	bridge->dev.id = id;
-	dev_set_drvdata(dev, bridge);
 
 	ret = dev_set_name(&bridge->dev, "br%d", id);
 	if (ret)
@@ -382,17 +375,14 @@ int fpga_bridge_register(struct device *dev, const char *name,
 	if (ret)
 		goto error_device;
 
-	of_platform_populate(dev->of_node, NULL, NULL, dev);
+	of_platform_populate(parent->of_node, NULL, NULL, parent);
 
-	dev_info(bridge->dev.parent, "fpga bridge [%s] registered\n",
-		 bridge->name);
+	dev_info(parent, "fpga bridge [%s] registered\n", bridge->name);
 
 	return 0;
 
 error_device:
 	ida_simple_remove(&fpga_bridge_ida, id);
-error_kfree:
-	kfree(bridge);
 
 	return ret;
 }
@@ -402,10 +392,8 @@ EXPORT_SYMBOL_GPL(fpga_bridge_register);
  * fpga_bridge_unregister - unregister a fpga bridge driver
  * @dev: FPGA bridge device from pdev
  */
-void fpga_bridge_unregister(struct device *dev)
+void fpga_bridge_unregister(struct fpga_bridge *bridge)
 {
-	struct fpga_bridge *bridge = dev_get_drvdata(dev);
-
 	/*
 	 * If the low level driver provides a method for putting bridge into
 	 * a desired state upon unregister, do it.
@@ -422,7 +410,6 @@ static void fpga_bridge_dev_release(struct device *dev)
 	struct fpga_bridge *bridge = to_fpga_bridge(dev);
 
 	ida_simple_remove(&fpga_bridge_ida, bridge->dev.id);
-	kfree(bridge);
 }
 
 static int __init fpga_bridge_dev_init(void)
diff --git a/drivers/fpga/xilinx-pr-decoupler.c b/drivers/fpga/xilinx-pr-decoupler.c
index 0d77430..f434c76 100644
--- a/drivers/fpga/xilinx-pr-decoupler.c
+++ b/drivers/fpga/xilinx-pr-decoupler.c
@@ -94,9 +94,14 @@ MODULE_DEVICE_TABLE(of, xlnx_pr_decoupler_of_match);
 static int xlnx_pr_decoupler_probe(struct platform_device *pdev)
 {
 	struct xlnx_pr_decoupler_data *priv;
+	struct fpga_bridge *br;
 	int err;
 	struct resource *res;
 
+	br = devm_kzalloc(&pdev->dev, sizeof(*br), GFP_KERNEL);
+	if (!br)
+		return -ENOMEM;
+
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -120,9 +125,13 @@ static int xlnx_pr_decoupler_probe(struct platform_device *pdev)
 
 	clk_disable(priv->clk);
 
-	err = fpga_bridge_register(&pdev->dev, "Xilinx PR Decoupler",
-				   &xlnx_pr_decoupler_br_ops, priv);
+	br->name = "Xilinx PR Decoupler";
+	br->br_ops = &xlnx_pr_decoupler_br_ops;
+	br->priv = priv;
+	br->parent = &pdev->dev;
+	platform_set_drvdata(pdev, br);
 
+	err = fpga_bridge_register(br);
 	if (err) {
 		dev_err(&pdev->dev, "unable to register Xilinx PR Decoupler");
 		clk_unprepare(priv->clk);
@@ -137,7 +146,7 @@ static int xlnx_pr_decoupler_remove(struct platform_device *pdev)
 	struct fpga_bridge *bridge = platform_get_drvdata(pdev);
 	struct xlnx_pr_decoupler_data *p = bridge->priv;
 
-	fpga_bridge_unregister(&pdev->dev);
+	fpga_bridge_unregister(bridge);
 
 	clk_unprepare(p->clk);
 
diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
index 3694821..c46b7ac 100644
--- a/include/linux/fpga/fpga-bridge.h
+++ b/include/linux/fpga/fpga-bridge.h
@@ -26,6 +26,7 @@ struct fpga_bridge_ops {
  * struct fpga_bridge - FPGA bridge structure
  * @name: name of low level FPGA bridge
  * @dev: FPGA bridge device
+ * @parent: parent device
  * @mutex: enforces exclusive reference to bridge
  * @br_ops: pointer to struct of FPGA bridge ops
  * @info: fpga image specific information
@@ -35,6 +36,7 @@ struct fpga_bridge_ops {
 struct fpga_bridge {
 	const char *name;
 	struct device dev;
+	struct device *parent;
 	struct mutex mutex; /* for exclusive reference to bridge */
 	const struct fpga_bridge_ops *br_ops;
 	struct fpga_image_info *info;
@@ -62,8 +64,7 @@ int of_fpga_bridge_get_to_list(struct device_node *np,
 			       struct fpga_image_info *info,
 			       struct list_head *bridge_list);
 
-int fpga_bridge_register(struct device *dev, const char *name,
-			 const struct fpga_bridge_ops *br_ops, void *priv);
-void fpga_bridge_unregister(struct device *dev);
+int fpga_bridge_register(struct fpga_bridge *br);
+void fpga_bridge_unregister(struct fpga_bridge *br);
 
 #endif /* _LINUX_FPGA_BRIDGE_H */
-- 
2.7.4

[PATCH v2 4/5] fpga: region: change fpga_region_register to have one param

[Alan Tull]

Change fpga_region_register to only take one parameter:

  int fpga_region_register(struct fpga_region *region)

The parent dev is added to struct fpga_region.

This make it similar to fpga_bridge_register and fpga_mgr_register
which also just take their respective struct as a param.

Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: This patch added in this version of the patchset
---
 Documentation/fpga/fpga-region.txt |  3 +--
 drivers/fpga/fpga-region.c         | 12 +++++++++---
 drivers/fpga/of-fpga-region.c      |  3 ++-
 include/linux/fpga/fpga-region.h   |  4 +++-
 4 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/Documentation/fpga/fpga-region.txt b/Documentation/fpga/fpga-region.txt
index 139a02b..d38fa3b 100644
--- a/Documentation/fpga/fpga-region.txt
+++ b/Documentation/fpga/fpga-region.txt
@@ -42,8 +42,7 @@ The FPGA region API
 To register or unregister a region:
 -----------------------------------
 
-	int fpga_region_register(struct device *dev,
-				 struct fpga_region *region);
+	int fpga_region_register(struct fpga_region *region);
 	int fpga_region_unregister(struct fpga_region *region);
 
 An example of usage can be seen in the probe function of [3]
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index ebe1f87..84360fd 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -162,10 +162,16 @@ int fpga_region_program_fpga(struct fpga_region *region)
 }
 EXPORT_SYMBOL_GPL(fpga_region_program_fpga);
 
-int fpga_region_register(struct device *dev, struct fpga_region *region)
+int fpga_region_register(struct fpga_region *region)
 {
+	struct device *parent = region->parent;
 	int id, ret = 0;
 
+	if (!parent) {
+		pr_err("Attempt to register fpga region without parent\n");
+		return -EINVAL;
+	}
+
 	id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
 	if (id < 0)
 		return id;
@@ -175,8 +181,8 @@ int fpga_region_register(struct device *dev, struct fpga_region *region)
 	device_initialize(&region->dev);
 	region->dev.groups = region->groups;
 	region->dev.class = fpga_region_class;
-	region->dev.parent = dev;
-	region->dev.of_node = dev->of_node;
+	region->dev.parent = parent;
+	region->dev.of_node = parent->of_node;
 	region->dev.id = id;
 
 	ret = dev_set_name(&region->dev, "region%d", id);
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index 3079ed8..276ffe2 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -422,11 +422,12 @@ static int of_fpga_region_probe(struct platform_device *pdev)
 	}
 
 	region->mgr = mgr;
+	region->parent = dev;
 
 	/* Specify how to get bridges for this type of region. */
 	region->get_bridges = of_fpga_region_get_bridges;
 
-	ret = fpga_region_register(dev, region);
+	ret = fpga_region_register(region);
 	if (ret)
 		goto eprobe_mgr_put;
 
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index b652031..423c87e 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -8,6 +8,7 @@
 /**
  * struct fpga_region - FPGA Region structure
  * @dev: FPGA Region device
+ * @parent: parent device
  * @mutex: enforces exclusive reference to region
  * @bridge_list: list of FPGA bridges specified in region
  * @mgr: FPGA manager
@@ -18,6 +19,7 @@
  */
 struct fpga_region {
 	struct device dev;
+	struct device *parent;
 	struct mutex mutex; /* for exclusive reference to region */
 	struct list_head bridge_list;
 	struct fpga_manager *mgr;
@@ -34,7 +36,7 @@ struct fpga_region *fpga_region_class_find(
 	int (*match)(struct device *, const void *));
 
 int fpga_region_program_fpga(struct fpga_region *region);
-int fpga_region_register(struct device *dev, struct fpga_region *region);
+int fpga_region_register(struct fpga_region *region);
 int fpga_region_unregister(struct fpga_region *region);
 
 #endif /* _FPGA_REGION_H */
-- 
2.7.4

[PATCH v2 5/5] fpga: region: return NOTIFY_STOP if overlay shoud be accepted

[Alan Tull]

Recent changes to the Device Tree overlay notifier code have changed
how notifier return codes are interpreted, requiring a NOTIFY_STOP to
signal that the overlay should be accepted.  This commit makes the
appropriate change to the FPGA region's Device Tree overlay notifier.

Fixes: 24789c5ce5a3 ("of: overlay: detect cases where device tree may become corrupt")
Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: Added in v2 of this patchset.  Tested on linux-next/master.
---
 drivers/fpga/of-fpga-region.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index 276ffe2..a0c13cb 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -349,7 +349,8 @@ static void of_fpga_region_notify_post_remove(struct fpga_region *region,
  * This notifier handles programming a FPGA when a "firmware-name" property is
  * added to a fpga-region.
  *
- * Returns NOTIFY_OK or error if FPGA programming fails.
+ * Returns NOTIFY_OK if this notification isn't about this region or
+ * NOTIFY_STOP if the overaly is to be accepted or error if FPGA programming fails.
  */
 static int of_fpga_region_notify(struct notifier_block *nb,
 				 unsigned long action, void *arg)
@@ -395,7 +396,7 @@ static int of_fpga_region_notify(struct notifier_block *nb,
 	if (ret)
 		return notifier_from_errno(ret);
 
-	return NOTIFY_OK;
+	return NOTIFY_STOP;
 }
 
 static struct notifier_block fpga_region_of_nb = {
-- 
2.7.4

Re: [PATCH v2 5/5] fpga: region: return NOTIFY_STOP if overlay shoud be accepted

[Moritz Fischer]

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

On Wed, Nov 15, 2017 at 02:51:52PM -0600, Alan Tull wrote:
> Recent changes to the Device Tree overlay notifier code have changed
> how notifier return codes are interpreted, requiring a NOTIFY_STOP to
> signal that the overlay should be accepted.  This commit makes the
> appropriate change to the FPGA region's Device Tree overlay notifier.
> 
> Fixes: 24789c5ce5a3 ("of: overlay: detect cases where device tree may become corrupt")
> Signed-off-by: Alan Tull <atull@kernel.org>
Reviewed-by: Moritz Fischer <mdf@kernel.org>
> ---
> v2: Added in v2 of this patchset.  Tested on linux-next/master.
> ---
>  drivers/fpga/of-fpga-region.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
> index 276ffe2..a0c13cb 100644
> --- a/drivers/fpga/of-fpga-region.c
> +++ b/drivers/fpga/of-fpga-region.c
> @@ -349,7 +349,8 @@ static void of_fpga_region_notify_post_remove(struct fpga_region *region,
>   * This notifier handles programming a FPGA when a "firmware-name" property is
>   * added to a fpga-region.
>   *
> - * Returns NOTIFY_OK or error if FPGA programming fails.
> + * Returns NOTIFY_OK if this notification isn't about this region or
> + * NOTIFY_STOP if the overaly is to be accepted or error if FPGA programming fails.
>   */
>  static int of_fpga_region_notify(struct notifier_block *nb,
>  				 unsigned long action, void *arg)
> @@ -395,7 +396,7 @@ static int of_fpga_region_notify(struct notifier_block *nb,
>  	if (ret)
>  		return notifier_from_errno(ret);
>  
> -	return NOTIFY_OK;
> +	return NOTIFY_STOP;
>  }
>  
>  static struct notifier_block fpga_region_of_nb = {
> -- 
> 2.7.4
> 

Thanks,

Moritz

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

Re: [PATCH v2 5/5] fpga: region: return NOTIFY_STOP if overlay shoud be accepted

[Alan Tull]

On Thu, Dec 7, 2017 at 5:27 PM, Moritz Fischer <mdf@kernel.org> wrote:
> On Wed, Nov 15, 2017 at 02:51:52PM -0600, Alan Tull wrote:
>> Recent changes to the Device Tree overlay notifier code have changed
>> how notifier return codes are interpreted, requiring a NOTIFY_STOP to
>> signal that the overlay should be accepted.  This commit makes the
>> appropriate change to the FPGA region's Device Tree overlay notifier.
>>
>> Fixes: 24789c5ce5a3 ("of: overlay: detect cases where device tree may become corrupt")
>> Signed-off-by: Alan Tull <atull@kernel.org>
> Reviewed-by: Moritz Fischer <mdf@kernel.org>

Thanks!
Alan

Re: [PATCH v2 0/5] fpga: don't use drvdata in common fpga code

[Alan Tull]

On Wed, Nov 15, 2017 at 2:51 PM, Alan Tull <atull@kernel.org> wrote:

Gentle reminder, this patch set is pretty straightforward, could use
some reviewing.

Alan

> This patch set goes on top of v6 of the non-dt support patchset that has
> been acked and I've asked Greg KH to send upstream.
>
> This patchset changes the fpga_*_register functions to not set drvdata,
> and changes parameters for register/unregister functions.  Also a
> bug fix due to changes in the DT overlay code.
>
> Setting drvdata is fine for DT based devices, that will have one
> manager, bridge, or region device per platform device.  PCIe based
> devices may have multiple FPGA mgr/bridge/regions under one pcie
> device.  Without these changes, PCIe-based solutions have to create an
> extra device for each child mgr/bridge/region to hold drvdata.
>
> Also changing the fpga_*_register/unregister functions to take the
> corresponding struct as the only parameter wherever they weren't already.
>
> * int fpga_bridge_register(struct fpga_bridge *br)
> * int fpga_mgr_register(struct fpga_manager *mgr)
> * int fpga_region_register(struct fpga_region *region)
> * void fpga_bridge_unregister(struct fpga_bridge *br)
> * void fpga_mgr_unregister(struct fpga_manager *mgr)
> * int fpga_region_unregister(struct fpga_region *region)
>
> Other changes:
>
> - Callers of fpga_(bridge|mgr)_register, are changed to alloc the
> bridge/mgr struct and partly fill it, adding name, ops and priv.
> - Caller uses devm for allocating the mgr/bridge structures.
> - Change the FPGA Region DT notifier to return NOTIFY_STOP if
> the overlay should be accepted.
>
> I posted a branch to the linux-fpga repo on kernel.org which has both
> the v6 non-DT support patchset and this v2 patchset for convenience.
> Branch name is next-20171113-non-dt-support-v6+v2
>
> Alan
>
> Alan Tull (5):
>   fpga: region: don't use drvdata in common fpga code
>   fpga: manager: don't use drvdata in common fpga code
>   fpga: bridge: don't use drvdata in common fpga code
>   fpga: region: change fpga_region_register to have one param
>   fpga: region: return NOTIFY_STOP if overlay shoud be accepted
>
>  Documentation/fpga/fpga-mgr.txt     | 24 ++++++++++++-----
>  Documentation/fpga/fpga-region.txt  |  3 +--
>  drivers/fpga/altera-cvp.c           | 18 ++++++++++---
>  drivers/fpga/altera-fpga2sdram.c    | 20 +++++++++++---
>  drivers/fpga/altera-freeze-bridge.c | 18 ++++++++++---
>  drivers/fpga/altera-hps2fpga.c      | 16 ++++++++---
>  drivers/fpga/altera-pr-ip-core.c    | 17 ++++++++++--
>  drivers/fpga/altera-ps-spi.c        | 18 ++++++++++---
>  drivers/fpga/fpga-bridge.c          | 53 ++++++++++++++-----------------------
>  drivers/fpga/fpga-mgr.c             | 49 +++++++++++++---------------------
>  drivers/fpga/fpga-region.c          | 13 ++++++---
>  drivers/fpga/ice40-spi.c            | 20 +++++++++++---
>  drivers/fpga/of-fpga-region.c       |  9 ++++---
>  drivers/fpga/socfpga-a10.c          | 16 ++++++++---
>  drivers/fpga/socfpga.c              | 18 ++++++++++---
>  drivers/fpga/ts73xx-fpga.c          | 18 ++++++++++---
>  drivers/fpga/xilinx-pr-decoupler.c  | 15 ++++++++---
>  drivers/fpga/xilinx-spi.c           | 18 ++++++++++---
>  drivers/fpga/zynq-fpga.c            | 16 ++++++++---
>  include/linux/fpga/fpga-bridge.h    |  7 ++---
>  include/linux/fpga/fpga-mgr.h       |  8 +++---
>  include/linux/fpga/fpga-region.h    |  4 ++-
>  22 files changed, 270 insertions(+), 128 deletions(-)
>
> --
> 2.7.4
>