Hi Moritz,

	Thanks for the review.
Please find my response inline.

-----Original Message-----
From: Moritz Fischer <mdf@kernel.org>
Sent: Saturday, January 16, 2021 8:28 AM
To: Nava kishore Manne <redacted>
Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal Simek
[off-list ref]; linux-fpga@vger.kernel.org;
devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org; git [off-list ref]; chinnikishore369@gmail.com
Subject: Re: [PATCH 2/2] fpga: Add support for Xilinx DFX AXI Shutdown
manager

Hi,

On Fri, Jan 15, 2021 at 07:04:31AM +0530, Nava kishore Manne wrote:

quoted

This patch adds support for Xilinx Dynamic Function eXchange(DFX) AXI
shutdown manager IP. It can be used to safely handling the AXI traffic
on a Reconfigurable Partition when it is undergoing dynamic
reconfiguration and there by preventing system deadlock that may occur
if AXI transactions are interrupted during reconfiguration.

PR-Decoupler and AXI shutdown manager are completely different IPs.
But both the IP registers are compatible and also both belong to the
same sub-system (fpga-bridge).So using same driver for both IP's.

I'm a bit confused, the whole goal here is to give the thing a different name?

Both the PR Decoupler and AXI Shutdown IP manager IP's are follows same register spec.
Most of the code is common so we thought of reusing  same driver for AXI shutdown manager as well.

quoted

Signed-off-by: Nava kishore Manne <redacted>
---
 drivers/fpga/xilinx-pr-decoupler.c | 35
++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/fpga/xilinx-pr-decoupler.c

b/drivers/fpga/xilinx-pr-decoupler.c
index 7d69af230567..c95f3d065ccb 100644

--- a/drivers/fpga/xilinx-pr-decoupler.c
+++ b/drivers/fpga/xilinx-pr-decoupler.c

@@ -19,10 +19,15 @@
 #define CTRL_OFFSET		0

 struct xlnx_pr_decoupler_data {
+	const struct xlnx_config_data *ipconfig;
 	void __iomem *io_base;
 	struct clk *clk;
 };

+struct xlnx_config_data {
+	char *name;
+};
+
 static inline void xlnx_pr_decoupler_write(struct xlnx_pr_decoupler_data

*d,

quoted

 					   u32 offset, u32 val)
 {

@@ -76,15 +81,28 @@ static const struct fpga_bridge_ops

xlnx_pr_decoupler_br_ops = {

quoted

 	.enable_show = xlnx_pr_decoupler_enable_show,  };

+static const struct xlnx_config_data decoupler_config = {
+	.name = "Xilinx PR Decoupler",
+};
+
+static const struct xlnx_config_data shutdown_config = {
+	.name = "Xilinx DFX AXI shutdown mgr", };

If it's just the strings, why not store them as is?

In order to differentiate the IP's at probe time we are using this name filed.

quoted

+
 static const struct of_device_id xlnx_pr_decoupler_of_match[] = {
-	{ .compatible = "xlnx,pr-decoupler-1.00", },
-	{ .compatible = "xlnx,pr-decoupler", },
+	{ .compatible = "xlnx,pr-decoupler-1.00", .data = &decoupler_config

},

quoted

+	{ .compatible = "xlnx,pr-decoupler", .data = &decoupler_config },
+	{ .compatible = "xlnx,dfx-axi-shutdown-manager-1.00",
+					.data = &shutdown_config },
+	{ .compatible = "xlnx,dfx-axi-shutdown-manager",
+					.data = &shutdown_config },
 	{},
 };
 MODULE_DEVICE_TABLE(of, xlnx_pr_decoupler_of_match);

 static int xlnx_pr_decoupler_probe(struct platform_device *pdev)  {
+	struct device_node *np = pdev->dev.of_node;
 	struct xlnx_pr_decoupler_data *priv;
 	struct fpga_bridge *br;
 	int err;

@@ -94,6 +112,14 @@ static int xlnx_pr_decoupler_probe(struct

platform_device *pdev)

quoted

 	if (!priv)
 		return -ENOMEM;

+	if (np) {
+		const struct of_device_id *match;
+
+		match = of_match_node(xlnx_pr_decoupler_of_match, np);
+		if (match && match->data)
+			priv->ipconfig = match->data;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	priv->io_base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(priv->io_base))

@@ -114,7 +140,7 @@ static int xlnx_pr_decoupler_probe(struct

platform_device *pdev)

 	clk_disable(priv->clk);

-	br = devm_fpga_bridge_create(&pdev->dev, "Xilinx PR Decoupler",
+	br = devm_fpga_bridge_create(&pdev->dev, priv->ipconfig->name,
 				     &xlnx_pr_decoupler_br_ops, priv);
 	if (!br) {
 		err = -ENOMEM;

@@ -125,7 +151,8 @@ static int xlnx_pr_decoupler_probe(struct

platform_device *pdev)

 	err = fpga_bridge_register(br);
 	if (err) {
-		dev_err(&pdev->dev, "unable to register Xilinx PR

Decoupler");

quoted

+		dev_err(&pdev->dev, "unable to register %s",
+			priv->ipconfig->name);
 		goto err_clk;
 	}

--
2.18.0

Regards,
Navakishore.

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