Re: [PATCH 2/4] fpga: manager: change api, don't use drvdata

[Florian Fainelli]

On 04/27/2018 04:30 PM, Alan Tull wrote:
> On Fri, Apr 27, 2018 at 1:26 PM, Florian Fainelli  
> wrote:
>> On 04/26/2018 06:26 PM, Moritz Fischer wrote:
>>> From: Alan Tull 
>>>
>>> Change fpga_mgr_register to not set or use drvdata.  This supports
>>> the case where a PCIe device has more than one manager.
>>>
>>> Add fpga_mgr_create/free functions.  Change fpga_mgr_register and
>>> fpga_mgr_unregister functions to take the mgr struct as their only
>>> parameter.
>>>
>>>   struct fpga_manager *fpga_mgr_create(struct device *dev,
>>> const char *name,
>>> const struct fpga_manager_ops *mops,
>>> void *priv);
>>>   void fpga_mgr_free(struct fpga_manager *mgr);
>>>   int fpga_mgr_register(struct fpga_manager *mgr);
>>>   void fpga_mgr_unregister(struct fpga_manager *mgr);
>>>
>>> Update the drivers that call fpga_mgr_register with the new API.
>>
>> Apologies for chiming in so late, this commit does not make it clear
>> that fpga_mgr_unregister() now also free the 'mgr' argument by calling
>> fpga_mgr_free(), this is kind of detail, but an API should make that
>> clear IMHO.
> 
> If people follow the usage information, in
> Documentation/fpga/fpga-mgr.txt, they'll do the right thing.  But I
> can add a patch that clarifies the description of fpga_mgr_unregister
> in fpga-mgr.c that it "unregisters and frees" the manager.

Just mentioning that because not all APIs do this, take the network
devices: there is an unregister_netdev() and a free_netdev(). Either way
is fine with me as long as it is documented as such, I had to look at
the API implementation to figure out that, no, all the drivers were not
leaking their fpga_manager instance in their .remove() function :)
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] fpga: manager: change api, don't use drvdata

[Florian Fainelli]

On 04/26/2018 06:26 PM, Moritz Fischer wrote:
> From: Alan Tull 
> 
> Change fpga_mgr_register to not set or use drvdata.  This supports
> the case where a PCIe device has more than one manager.
> 
> Add fpga_mgr_create/free functions.  Change fpga_mgr_register and
> fpga_mgr_unregister functions to take the mgr struct as their only
> parameter.
> 
>   struct fpga_manager *fpga_mgr_create(struct device *dev,
> const char *name,
> const struct fpga_manager_ops *mops,
> void *priv);
>   void fpga_mgr_free(struct fpga_manager *mgr);
>   int fpga_mgr_register(struct fpga_manager *mgr);
>   void fpga_mgr_unregister(struct fpga_manager *mgr);
> 
> Update the drivers that call fpga_mgr_register with the new API.

Apologies for chiming in so late, this commit does not make it clear
that fpga_mgr_unregister() now also free the 'mgr' argument by calling
fpga_mgr_free(), this is kind of detail, but an API should make that
clear IMHO.

Thanks
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] fpga: manager: change api, don't use drvdata

[Moritz Fischer]

From: Alan Tull 

Change fpga_mgr_register to not set or use drvdata.  This supports
the case where a PCIe device has more than one manager.

Add fpga_mgr_create/free functions.  Change fpga_mgr_register and
fpga_mgr_unregister functions to take the mgr struct as their only
parameter.

  struct fpga_manager *fpga_mgr_create(struct device *dev,
const char *name,
const struct fpga_manager_ops *mops,
void *priv);
  void fpga_mgr_free(struct fpga_manager *mgr);
  int fpga_mgr_register(struct fpga_manager *mgr);
  void fpga_mgr_unregister(struct fpga_manager *mgr);

Update the drivers that call fpga_mgr_register with the new API.

Signed-off-by: Alan Tull 
[Moritz: Fixup whitespace issue]
Reported-by: Jiuyue Ma 
Signed-off-by: Moritz Fischer 
---
 Documentation/fpga/fpga-mgr.txt  | 35 ++
 drivers/fpga/altera-cvp.c| 19 ++--
 drivers/fpga/altera-pr-ip-core.c | 18 +++-
 drivers/fpga/altera-ps-spi.c | 20 ++--
 drivers/fpga/fpga-mgr.c  | 78 +---
 drivers/fpga/ice40-spi.c | 21 +++--
 drivers/fpga/machxo2-spi.c   | 20 ++--
 drivers/fpga/socfpga-a10.c   | 14 --
 drivers/fpga/socfpga.c   | 19 ++--
 drivers/fpga/ts73xx-fpga.c   | 20 ++--
 drivers/fpga/xilinx-spi.c| 20 ++--
 drivers/fpga/zynq-fpga.c | 14 --
 include/linux/fpga/fpga-mgr.h| 10 ++--
 13 files changed, 237 insertions(+), 71 deletions(-)

diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
index cc6413ed6fc9..86b6df66a905 100644
--- a/Documentation/fpga/fpga-mgr.txt
+++ b/Documentation/fpga/fpga-mgr.txt
@@ -63,17 +63,23 @@ The user should call fpga_mgr_lock and verify that it 
returns 0 before
 attempting to program the FPGA.  Likewise, the user should call
 fpga_mgr_unlock when done programming the FPGA.
 
+To alloc/free a FPGA manager struct:
+
+
+   struct fpga_manager *fpga_mgr_create(struct device *dev,
+const char *name,
+const struct fpga_manager_ops 
*mops,
+void *priv);
+   void fpga_mgr_free(struct fpga_manager *mgr);
 
 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
+Use of these functions is described below in "How To Support a new FPGA
 device."
 
 
@@ -148,6 +154,7 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
 {
struct device *dev = >dev;
struct socfpga_fpga_priv *priv;
+   struct fpga_manager *mgr;
int ret;
 
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -157,13 +164,25 @@ 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",
-_fpga_ops, priv);
+   mgr = fpga_mgr_create(dev, "Altera SOCFPGA FPGA Manager",
+ _fpga_ops, priv);
+   if (!mgr)
+   return -ENOMEM;
+
+   platform_set_drvdata(pdev, mgr);
+
+   ret = fpga_mgr_register(mgr);
+   if (ret)
+   fpga_mgr_free(mgr);
+
+   return ret;
 }
 
 static int socfpga_fpga_remove(struct platform_device *pdev)
 {
-   fpga_mgr_unregister(>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 77b04e4b3254..dd4edd8f22ce 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -401,6 +401,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;
 
@@ -452,16 +453,24 @@ 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(>dev, conf->mgr_name,
-   _cvp_ops, conf);
-   if (ret)
+   mgr = fpga_mgr_create(>dev, conf->mgr_name,
+ _cvp_ops, conf);
+   if (!mgr)
+