[PATCH 0/6] FPGA Manager Patches for 4.17

[PATCH 0/6] FPGA Manager Patches for 4.17

[Moritz Fischer]

Hi Greg,

can you please take the following patches for 4.17.

Alan's changes are mostly refactoring in preparation for non-dt based
FPGA regions.

Anatolij's changes fix a small issue with altera-ps-spi FPGAs,
where the FPGA is being reset unnessesarily on driver load.

All patches have been reviewed on the list.

I'm sending the patches this time instead of Alan so I can get more
familiar with the process.

If you prefer to pull instead there's also a signed tag:

  git://git.kernel.org/pub/scm/linux/kernel/git/mdf/linux.git tags/fpga-for-greg-4.17

Thanks,

Moritz

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: fpga-region: comment on fpga_region_program_fpga locking

Anatolij Gustschin (1):
  fpga-manager: altera-ps-spi: preserve nCONFIG state

 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        | 20 +++++++++++++----
 drivers/fpga/fpga-bridge.c          | 43 ++++++++++++++-----------------------
 drivers/fpga/fpga-mgr.c             | 39 ++++++++++++---------------------
 drivers/fpga/fpga-region.c          | 14 ++++++++++--
 drivers/fpga/ice40-spi.c            | 20 +++++++++++++----
 drivers/fpga/of-fpga-region.c       |  4 +++-
 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, 262 insertions(+), 114 deletions(-)

-- 
2.16.2

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

[Moritz Fischer]

From: Alan Tull <atull@kernel.org>

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 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 edab2a2e03ef..ebe1f872810d 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 119ff75522f1..35e7e8c4a0cb 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -438,6 +438,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.16.2

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

[Greg KH]

On Thu, Mar 29, 2018 at 08:36:53AM -0700, Moritz Fischer wrote:
> From: Alan Tull <atull@kernel.org>
> 
> Part of patchset that changes the following fpga_*_register
> functions to not set drvdata:
> * fpga_region_register.
> * fpga_mgr_register
> * fpga_bridge_register

That's not what this specific patch does.  Please do not write generic
changelog text that is identical for 3 patches that does different
things in each one :(

thanks,

greg k-h

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

[Alan Tull]

On Thu, Mar 29, 2018 at 12:01 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

Hi Greg,

> On Thu, Mar 29, 2018 at 08:36:53AM -0700, Moritz Fischer wrote:
>> From: Alan Tull <atull@kernel.org>
>>
>> Part of patchset that changes the following fpga_*_register
>> functions to not set drvdata:
>> * fpga_region_register.
>> * fpga_mgr_register
>> * fpga_bridge_register
>
> That's not what this specific patch does.  Please do not write generic
> changelog text that is identical for 3 patches that does different
> things in each one :(

Thanks for the guidance, it's always helpful.  I'll clean up.

Alan

>
> thanks,
>
> greg k-h

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

[Moritz Fischer]

From: Alan Tull <atull@kernel.org>

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 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          | 39 ++++++++++++++-------------------------
 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, 166 insertions(+), 64 deletions(-)

diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
index cc6413ed6fc9..3cea6b57c156 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->parent = dev;
+	mgr->name = "Altera SOCFPGA FPGA Manager";
+	mgr->mops = &socfpga_fpga_ops;
+	mgr->priv = priv;
+	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 00e73d28077c..f02a1a88609a 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->parent = &pdev->dev;
+	mgr->name = conf->mgr_name;
+	mgr->mops = &altera_cvp_ops;
+	mgr->priv = conf;
+	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 a7b31f9797ce..9d60cdc39c55 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->parent = dev;
+	mgr->name = dev_name(dev);
+	mgr->mops = &alt_pr_ops;
+	mgr->priv = priv;
+	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 14f14efdf0d5..b061408bf0ae 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->parent = &spi->dev;
+	mgr->name = conf->mgr_name;
+	mgr->mops = &altera_ps_ops;
+	mgr->priv = conf;
+	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 9939d2cbc9a6..245e7a6efb6a 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -516,20 +516,24 @@ 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;
+	struct device *dev = mgr->parent;
+	const struct fpga_manager_ops *mops = mgr->mops;
+	const char *name = mgr->name;
 	int id, ret;
 
+	if (!dev) {
+		pr_err("Attempt to register fpga manager without parent\n");
+		return -EINVAL;
+	}
+
 	if (!mops || !mops->write_complete || !mops->state ||
 	    !mops->write_init || (!mops->write && !mops->write_sg) ||
 	    (mops->write && mops->write_sg)) {
@@ -542,22 +546,13 @@ int fpga_mgr_register(struct device *dev, const char *name,
 		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
@@ -571,7 +566,6 @@ int fpga_mgr_register(struct device *dev, const char *name,
 	mgr->dev.parent = dev;
 	mgr->dev.of_node = dev->of_node;
 	mgr->dev.id = id;
-	dev_set_drvdata(dev, mgr);
 
 	ret = dev_set_name(&mgr->dev, "fpga%d", id);
 	if (ret)
@@ -587,8 +581,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;
 }
@@ -598,10 +590,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);
 
 	/*
@@ -620,7 +610,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 7fca82023062..912746735c71 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->parent = dev;
+	mgr->name = "Lattice iCE40 FPGA Manager";
+	mgr->mops = &ice40_fpga_ops;
+	mgr->priv = priv;
+	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 a46e343a5b72..16f642a5e40e 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;
 	}
 
-	ret = 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->parent = dev;
+	mgr->name = "SoCFPGA Arria10 FPGA Manager";
+	mgr->mops = &socfpga_a10_fpga_mgr_ops;
+	mgr->priv = priv;
+	platform_set_drvdata(pdev, mgr);
+
+	ret = fpga_mgr_register(mgr);
 	if (ret) {
 		clk_disable_unprepare(priv->clk);
 		return ret;
@@ -534,7 +544,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 b6672e66cda6..2e764f5393fb 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->parent = dev;
+	mgr->name = "Altera SOCFPGA FPGA Manager";
+	mgr->mops = &socfpga_fpga_ops;
+	mgr->priv = priv;
+	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 f6a96b42e2ca..33dd92626ea1 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->parent = kdev;
+	mgr->name = "TS-73xx FPGA Manager";
+	mgr->mops = &ts73xx_fpga_ops;
+	mgr->priv = priv;
+	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 9b62a4c2a3df..4775480ab3bd 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->parent = &spi->dev;
+	mgr->name = "Xilinx Slave Serial FPGA Manager";
+	mgr->mops = &xilinx_spi_ops;
+	mgr->priv = conf;
+	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 70b15b303471..2f9da8b2a354 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->parent = dev;
+	mgr->name = "Xilinx Zynq FPGA Manager";
+	mgr->mops = &zynq_fpga_ops;
+	mgr->priv = priv;
+	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 3c6de23aabdf..1ad482286b40 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.16.2

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

[Greg KH]

On Thu, Mar 29, 2018 at 08:36:54AM -0700, Moritz Fischer wrote:
> From: Alan Tull <atull@kernel.org>
> 
> 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>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  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          | 39 ++++++++++++++-------------------------
>  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, 166 insertions(+), 64 deletions(-)
> 
> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
> index cc6413ed6fc9..3cea6b57c156 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;

This feels odd to have to do for every driver.  Why can't the fpga core
handle this all for you?


> +
>  	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);

Why can't this be:
	fpga_mgr_create(...)
that returns the correct structure?  That way each driver always gets
the proper things set (you don't have to remember and audit each driver
to ensure they do it all correctly), and all is good?

That should make this a lot simpler to use, and change.

thanks,

greg k-h

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

[Alan Tull]

On Thu, Mar 29, 2018 at 12:03 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

Hi Greg,

> On Thu, Mar 29, 2018 at 08:36:54AM -0700, Moritz Fischer wrote:
>> From: Alan Tull <atull@kernel.org>
>>
>> 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>
>> Signed-off-by: Moritz Fischer <mdf@kernel.org>
>> ---
>>  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          | 39 ++++++++++++++-------------------------
>>  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, 166 insertions(+), 64 deletions(-)
>>
>> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
>> index cc6413ed6fc9..3cea6b57c156 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;
>
> This feels odd to have to do for every driver.  Why can't the fpga core
> handle this all for you?
>
>
>> +
>>       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);
>
> Why can't this be:
>         fpga_mgr_create(...)
> that returns the correct structure?  That way each driver always gets
> the proper things set (you don't have to remember and audit each driver
> to ensure they do it all correctly), and all is good?
>
> That should make this a lot simpler to use, and change.

Are you suggesting something like this?

-       return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
-                                &socfpga_fpga_ops, priv);
+       mgr = fpga_mgr_create(dev, "Altera SOCFPGA FPGA Manager",
+                               &socfpga_fpga_ops, priv);
+
+       platform_set_drvdata(pdev, mgr);
+
+       return fpga_mgr_register(mgr);

That would be straightforward and if there are more fields to fill in
in the future, we can still do that before calling fpga_mgr_register.

Alan

>
> thanks,
>
> greg k-h

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

[Greg KH]

On Thu, Mar 29, 2018 at 01:26:39PM -0500, Alan Tull wrote:
> On Thu, Mar 29, 2018 at 12:03 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> Hi Greg,
> 
> > On Thu, Mar 29, 2018 at 08:36:54AM -0700, Moritz Fischer wrote:
> >> From: Alan Tull <atull@kernel.org>
> >>
> >> 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>
> >> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> >> ---
> >>  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          | 39 ++++++++++++++-------------------------
> >>  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, 166 insertions(+), 64 deletions(-)
> >>
> >> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
> >> index cc6413ed6fc9..3cea6b57c156 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;
> >
> > This feels odd to have to do for every driver.  Why can't the fpga core
> > handle this all for you?
> >
> >
> >> +
> >>       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);
> >
> > Why can't this be:
> >         fpga_mgr_create(...)
> > that returns the correct structure?  That way each driver always gets
> > the proper things set (you don't have to remember and audit each driver
> > to ensure they do it all correctly), and all is good?
> >
> > That should make this a lot simpler to use, and change.
> 
> Are you suggesting something like this?
> 
> -       return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> -                                &socfpga_fpga_ops, priv);
> +       mgr = fpga_mgr_create(dev, "Altera SOCFPGA FPGA Manager",
> +                               &socfpga_fpga_ops, priv);
> +
> +       platform_set_drvdata(pdev, mgr);
> +
> +       return fpga_mgr_register(mgr);
> 
> That would be straightforward and if there are more fields to fill in
> in the future, we can still do that before calling fpga_mgr_register.

Or you can add another parameter to the fpga_mgr_create() call :)

Yes, that looks a lot better, thanks.

greg k-h

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

[Moritz Fischer]

From: Alan Tull <atull@kernel.org>

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/altera-fpga2sdram.c    | 20 +++++++++++++----
 drivers/fpga/altera-freeze-bridge.c | 18 +++++++++++++---
 drivers/fpga/altera-hps2fpga.c      | 16 +++++++++++---
 drivers/fpga/fpga-bridge.c          | 43 ++++++++++++++-----------------------
 drivers/fpga/xilinx-pr-decoupler.c  | 15 ++++++++++---
 include/linux/fpga/fpga-bridge.h    |  7 +++---
 6 files changed, 76 insertions(+), 43 deletions(-)

diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
index d4eeb74388da..a4593c0f5e42 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->parent = dev;
+	br->name = F2S_BRIDGE_NAME;
+	br->br_ops = &altera_fpga2sdram_br_ops;
+	br->priv = priv;
+	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 6159cfcf78a2..498bc6c1d533 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->parent = dev;
+	br->name = FREEZE_BRIDGE_NAME;
+	br->br_ops = &altera_freeze_br_br_ops;
+	br->priv = priv;
+	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 406d2f10741f..2a695f53962d 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->parent = dev;
+	br->name = priv->name;
+	br->br_ops = &altera_hps2fpga_br_ops;
+	br->priv = priv;
+	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 31bd2c59c305..ac40e3ae8921 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -329,48 +329,42 @@ 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;
+	struct device *dev = bridge->parent;
+	const char *name = bridge->name;
 	int id, ret = 0;
 
+	if (!dev) {
+		pr_err("Attempt to register fpga bridge without parent\n");
+		return -EINVAL;
+	}
+
 	if (!name || !strlen(name)) {
 		dev_err(dev, "Attempt to register with no name!\n");
 		return -EINVAL;
 	}
 
-	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
-	if (!bridge)
-		return -ENOMEM;
-
 	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.id = id;
-	dev_set_drvdata(dev, bridge);
 
 	ret = dev_set_name(&bridge->dev, "br%d", id);
 	if (ret)
@@ -389,8 +383,6 @@ int fpga_bridge_register(struct device *dev, const char *name,
 
 error_device:
 	ida_simple_remove(&fpga_bridge_ida, id);
-error_kfree:
-	kfree(bridge);
 
 	return ret;
 }
@@ -400,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.
@@ -420,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 0d7743089414..629337bd3768 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->parent = &pdev->dev;
+	br->name = "Xilinx PR Decoupler";
+	br->br_ops = &xlnx_pr_decoupler_br_ops;
+	br->priv = priv;
+	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 3694821a6d2d..c46b7ac0a4bd 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.16.2

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

[Greg KH]

On Thu, Mar 29, 2018 at 08:36:55AM -0700, Moritz Fischer wrote:
> From: Alan Tull <atull@kernel.org>
> 
> 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>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  drivers/fpga/altera-fpga2sdram.c    | 20 +++++++++++++----
>  drivers/fpga/altera-freeze-bridge.c | 18 +++++++++++++---
>  drivers/fpga/altera-hps2fpga.c      | 16 +++++++++++---
>  drivers/fpga/fpga-bridge.c          | 43 ++++++++++++++-----------------------
>  drivers/fpga/xilinx-pr-decoupler.c  | 15 ++++++++++---
>  include/linux/fpga/fpga-bridge.h    |  7 +++---
>  6 files changed, 76 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
> index d4eeb74388da..a4593c0f5e42 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->parent = dev;
> +	br->name = F2S_BRIDGE_NAME;
> +	br->br_ops = &altera_fpga2sdram_br_ops;
> +	br->priv = priv;
> +	platform_set_drvdata(pdev, br);

Same question here, why not fpga_bridge_create(...)?

thanks,

greg k-h

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

[Moritz Fischer]

From: Alan Tull <atull@kernel.org>

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.

The one caller of fpga_region_register is changed to alloc the
fpga_region struct, fill it in, and pass it to the register
function.

Signed-off-by: Alan Tull <atull@kernel.org>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 Documentation/fpga/fpga-region.txt | 3 +--
 drivers/fpga/fpga-region.c         | 8 +++++++-
 drivers/fpga/of-fpga-region.c      | 3 ++-
 include/linux/fpga/fpga-region.h   | 4 +++-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/fpga/fpga-region.txt b/Documentation/fpga/fpga-region.txt
index 139a02ba1ff6..d38fa3b4154a 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 ebe1f872810d..660a91b9e246 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 *dev = region->parent;
 	int id, ret = 0;
 
+	if (!dev) {
+		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;
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index 35e7e8c4a0cb..a7b38aafeaa7 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -428,12 +428,13 @@ static int of_fpga_region_probe(struct platform_device *pdev)
 		goto eprobe_mgr_put;
 	}
 
+	region->parent = dev;
 	region->mgr = mgr;
 
 	/* 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 b6520318ab9c..423c87e3e29a 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.16.2

Re: [PATCH 4/6] fpga: region: change fpga_region_register to have one param

[Greg KH]

On Thu, Mar 29, 2018 at 08:36:56AM -0700, Moritz Fischer wrote:
> From: Alan Tull <atull@kernel.org>
> 
> 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.
> 
> The one caller of fpga_region_register is changed to alloc the
> fpga_region struct, fill it in, and pass it to the register
> function.
> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  Documentation/fpga/fpga-region.txt | 3 +--
>  drivers/fpga/fpga-region.c         | 8 +++++++-
>  drivers/fpga/of-fpga-region.c      | 3 ++-
>  include/linux/fpga/fpga-region.h   | 4 +++-
>  4 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/fpga/fpga-region.txt b/Documentation/fpga/fpga-region.txt
> index 139a02ba1ff6..d38fa3b4154a 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 ebe1f872810d..660a91b9e246 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 *dev = region->parent;
>  	int id, ret = 0;
>  
> +	if (!dev) {
> +		pr_err("Attempt to register fpga region without parent\n");
> +		return -EINVAL;
> +	}

Are you sure you don't want a virtual device?  That is what will happen
if you do not have a parent, right?  Or do you always want to have
"real" devices?



> +
>  	id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
>  	if (id < 0)
>  		return id;
> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
> index 35e7e8c4a0cb..a7b38aafeaa7 100644
> --- a/drivers/fpga/of-fpga-region.c
> +++ b/drivers/fpga/of-fpga-region.c
> @@ -428,12 +428,13 @@ static int of_fpga_region_probe(struct platform_device *pdev)
>  		goto eprobe_mgr_put;
>  	}
>  
> +	region->parent = dev;
>  	region->mgr = mgr;
>  
>  	/* 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 b6520318ab9c..423c87e3e29a 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;

Why doesn't your dev parent pointer point to this, why do you need to
have a separate pointer?  That feels really wrong.  Pass in the parent
pointer when you create the struct device, otherwise it will be
registered incorrectly anyway.  Then you always have the correct
pointer, no need to keep a "spare" copy.

thanks,

greg k-h

Re: [PATCH 4/6] fpga: region: change fpga_region_register to have one param

[Alan Tull]

On Thu, Mar 29, 2018 at 12:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

Hi Greg,

>> -int fpga_region_register(struct device *dev, struct fpga_region *region)
>> +int fpga_region_register(struct fpga_region *region)
>>  {
>> +     struct device *dev = region->parent;
>>       int id, ret = 0;
>>
>> +     if (!dev) {
>> +             pr_err("Attempt to register fpga region without parent\n");
>> +             return -EINVAL;
>> +     }
>
> Are you sure you don't want a virtual device?  That is what will happen
> if you do not have a parent, right?  Or do you always want to have
> "real" devices?

I don't want to restrict this to "real" devices, so yes, I'll be
removing this check.

>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>> index b6520318ab9c..423c87e3e29a 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;
>
> Why doesn't your dev parent pointer point to this, why do you need to
> have a separate pointer?  That feels really wrong.  Pass in the parent
> pointer when you create the struct device, otherwise it will be
> registered incorrectly anyway.  Then you always have the correct
> pointer, no need to keep a "spare" copy.

I'll add a fpga_mgr_create function and let it set the parent.  No
need to save it.

Thanks again for the review,
Alan

>
> thanks,
>
> greg k-h

Re: [PATCH 4/6] fpga: region: change fpga_region_register to have one param

[Moritz Fischer]

On Thu, Mar 29, 2018 at 03:42:51PM -0500, Alan Tull wrote:
> On Thu, Mar 29, 2018 at 12:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> Hi Greg,
> 
> >> -int fpga_region_register(struct device *dev, struct fpga_region *region)
> >> +int fpga_region_register(struct fpga_region *region)
> >>  {
> >> +     struct device *dev = region->parent;
> >>       int id, ret = 0;
> >>
> >> +     if (!dev) {
> >> +             pr_err("Attempt to register fpga region without parent\n");
> >> +             return -EINVAL;
> >> +     }
> >
> > Are you sure you don't want a virtual device?  That is what will happen
> > if you do not have a parent, right?  Or do you always want to have
> > "real" devices?
> 
> I don't want to restrict this to "real" devices, so yes, I'll be
> removing this check.
> 
> >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> >> index b6520318ab9c..423c87e3e29a 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;
> >
> > Why doesn't your dev parent pointer point to this, why do you need to
> > have a separate pointer?  That feels really wrong.  Pass in the parent
> > pointer when you create the struct device, otherwise it will be
> > registered incorrectly anyway.  Then you always have the correct
> > pointer, no need to keep a "spare" copy.
> 
> I'll add a fpga_mgr_create function and let it set the parent.  No
> need to save it.

I think we had discussed this in the first round of the patchset.
How about fpga_mgr_alloc(...) and fpga_mgr_register(...) as suggested
back then?

Thanks for the review,

Moritz

Re: [PATCH 4/6] fpga: region: change fpga_region_register to have one param

[Alan Tull]

On Thu, Mar 29, 2018 at 4:39 PM, Moritz Fischer <mdf@kernel.org> wrote:
> On Thu, Mar 29, 2018 at 03:42:51PM -0500, Alan Tull wrote:
>> On Thu, Mar 29, 2018 at 12:06 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> Hi Greg,
>>
>> >> -int fpga_region_register(struct device *dev, struct fpga_region *region)
>> >> +int fpga_region_register(struct fpga_region *region)
>> >>  {
>> >> +     struct device *dev = region->parent;
>> >>       int id, ret = 0;
>> >>
>> >> +     if (!dev) {
>> >> +             pr_err("Attempt to register fpga region without parent\n");
>> >> +             return -EINVAL;
>> >> +     }
>> >
>> > Are you sure you don't want a virtual device?  That is what will happen
>> > if you do not have a parent, right?  Or do you always want to have
>> > "real" devices?
>>
>> I don't want to restrict this to "real" devices, so yes, I'll be
>> removing this check.
>>
>> >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>> >> index b6520318ab9c..423c87e3e29a 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;
>> >
>> > Why doesn't your dev parent pointer point to this, why do you need to
>> > have a separate pointer?  That feels really wrong.  Pass in the parent
>> > pointer when you create the struct device, otherwise it will be
>> > registered incorrectly anyway.  Then you always have the correct
>> > pointer, no need to keep a "spare" copy.
>>
>> I'll add a fpga_mgr_create function and let it set the parent.  No
>> need to save it.
>
> I think we had discussed this in the first round of the patchset.

Yup! :)

> How about fpga_mgr_alloc(...) and fpga_mgr_register(...) as suggested
> back then?

I'm cool with either name.  The alloc/create function will be doing
more than alloc.  It's going to fill in some struct members, allocate
an id #, and init the dev.  There's precedent for similar functions
named either way, even 'alloc' functions that do plenty of
initialization.

Alan

>
> Thanks for the review,
>
> Moritz

[PATCH 5/6] fpga: fpga-region: comment on fpga_region_program_fpga locking

[Moritz Fischer]

From: Alan Tull <atull@kernel.org>

Add a comment to the header of fpga_region_program_fpga()
regarding locking of the bridges.

Signed-off-by: Alan Tull <atull@kernel.org>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/fpga-region.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 660a91b9e246..3af960e24558 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -95,6 +95,11 @@ static void fpga_region_put(struct fpga_region *region)
  * fpga_region_program_fpga - program FPGA
  * @region: FPGA region
  * Program an FPGA using fpga image info (region->info).
+ * If the region has a get_bridges function, the exclusive reference for the
+ * bridges will be held if programming succeeds.  This is intended to prevent
+ * reprogramming the region until the caller considers it safe to do so.
+ * The caller will need to call fpga_bridges_put() before attempting to
+ * reprogram the region.
  * Return 0 for success or negative error code.
  */
 int fpga_region_program_fpga(struct fpga_region *region)
-- 
2.16.2

[PATCH 6/6] fpga-manager: altera-ps-spi: preserve nCONFIG state

[Moritz Fischer]

From: Anatolij Gustschin <agust@denx.de>

If the driver module is loaded when FPGA is configured, the FPGA
is reset because nconfig is pulled low (low-active gpio inited
with GPIOD_OUT_HIGH activates the signal which means setting its
value to low). Init nconfig with GPIOD_OUT_LOW to prevent this.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
Signed-off-by: Alan Tull <atull@kernel.org>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/altera-ps-spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
index b061408bf0ae..250d9cf9bb1b 100644
--- a/drivers/fpga/altera-ps-spi.c
+++ b/drivers/fpga/altera-ps-spi.c
@@ -254,7 +254,7 @@ static int altera_ps_probe(struct spi_device *spi)
 
 	conf->data = of_id->data;
 	conf->spi = spi;
-	conf->config = devm_gpiod_get(&spi->dev, "nconfig", GPIOD_OUT_HIGH);
+	conf->config = devm_gpiod_get(&spi->dev, "nconfig", GPIOD_OUT_LOW);
 	if (IS_ERR(conf->config)) {
 		dev_err(&spi->dev, "Failed to get config gpio: %ld\n",
 			PTR_ERR(conf->config));
-- 
2.16.2

Re: [PATCH 0/6] FPGA Manager Patches for 4.17

[Greg KH]

On Thu, Mar 29, 2018 at 08:36:52AM -0700, Moritz Fischer wrote:
> Hi Greg,
> 
> can you please take the following patches for 4.17.
> 
> Alan's changes are mostly refactoring in preparation for non-dt based
> FPGA regions.
> 
> Anatolij's changes fix a small issue with altera-ps-spi FPGAs,
> where the FPGA is being reset unnessesarily on driver load.
> 
> All patches have been reviewed on the list.
> 
> I'm sending the patches this time instead of Alan so I can get more
> familiar with the process.

Wow this is late.  If 4.16-final happens this Sunday, the most this
would possibly be in linux-next is for 1 day.  That's not ok.

If any of these are serious bug fixes, I'll consider them for 4.17-rc2,
so please send them after 4.17-rc1 is out.  And also send the new
features/cleanups then as well, as a separate series so I know to keep
them apart.

Next time please try to submit stuff sooner, there is no reason you
should not send them to me whenever they are ready, do not sit on them
at all if possible.

thanks,

greg k-h