* [PATCH v6 0/3] fpga: Use standard class dev_release function
@ 2021-06-21 22:22 Russ Weight
2021-06-21 22:22 ` [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver Russ Weight
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Russ Weight @ 2021-06-21 22:22 UTC (permalink / raw)
To: mdf, linux-fpga
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
Russ Weight
The FPGA framework has a convention of using managed resource
functions to allow parent drivers to manage the data structures
allocated by the class drivers. They use an empty *_dev_release()
function to satisfy the class driver.
This is inconsistent with linux driver model.
These changes remove the managed resource functions and populate
the class dev_release callback functions. They also merge the
create and register functions into a single register function for
each of the fpga-mgr, fpga-region, and fpga-bridge class drivers.
For more context, refer to this email thread:
https://marc.info/?l=linux-fpga&m=162127412218557&w=2
I turned on the configs assocated with each of the modified files,
but I must have been missing some dependencies, because not all
of them compiled. I did a run-time test specifically with the
dfl-fme infrastructure. This would have exercised the region,
bridge, and fpga-mgr frameworks.
Changelog v5 -> v6:
- Moved FPGA manager/bridge/region optional parameters out of the ops
structure and back into the FPGA class driver structure.
- Changed fpga_*_register() function parameters to accept an info data
structure to provide flexibility in passing optional parameters.
- Added fpga_*_register_simple() functions to support current parameters
for users that don't require use of optional parameters.
Changelog v4 -> v5:
- Rebased on top of recently accepted patches.
- Removed compat_id from the fpga_mgr_register() parameter list
and added it to the fpga_manager_ops structure. This also required
dynamically allocating the dfl-fme-ops structure in order to add
the appropriate compat_id.
- Created the fpga_region_ops data structure which is optionally passed
to fpga_region_register(). compat_id, the get_bridges() pointer, and
the priv pointer are included in the fpga_region_ops structure.
Changelog v3 -> v4:
- Added the compat_id parameter to fpga_mgr_register() and
devm_fpga_mgr_register() to ensure that the compat_id is set before
the device_register() call.
- Added the compat_id parameter to fpga_region_register() to ensure
that the compat_id is set before the device_register() call.
- Modified the dfl_fpga_feature_devs_enumerate() function to restore
the fpga_region_register() call to the correct location.
Changelog v2 -> v3:
- Cleaned up comment headers for fpga_mgr_register(), fpga_bridge_register(),
and fpga_region_register().
- Fixed error return on ida_simple_get() failure for fpga_mgr_register(),
fpga_bridge_register(), and fpga_region_register().
- Fixed error return value for fpga_bridge_register(): ERR_PTR(ret) instead
of NULL.
Changelog v1 -> v2:
- Restored devm_fpga_mgr_register() functionality to the fpga-mgr
class driver, adapted for the combined create/register functionality.
- All previous callers of devm_fpga_mgr_register() will continue to call
devm_fpga_mgr_register().
- replaced unnecessary ternary operators in return statements with
standard if conditions.
Russ Weight (3):
fpga: mgr: Use standard dev_release for class driver
fpga: bridge: Use standard dev_release for class driver
fpga: region: Use standard dev_release for class driver
drivers/fpga/altera-cvp.c | 12 +-
drivers/fpga/altera-fpga2sdram.c | 12 +-
drivers/fpga/altera-freeze-bridge.c | 10 +-
drivers/fpga/altera-hps2fpga.c | 12 +-
drivers/fpga/altera-pr-ip-core.c | 9 +-
drivers/fpga/altera-ps-spi.c | 10 +-
drivers/fpga/dfl-fme-br.c | 10 +-
drivers/fpga/dfl-fme-mgr.c | 23 ++-
drivers/fpga/dfl-fme-region.c | 17 ++-
drivers/fpga/dfl.c | 12 +-
drivers/fpga/fpga-bridge.c | 133 ++++++-----------
drivers/fpga/fpga-mgr.c | 212 ++++++++++++----------------
drivers/fpga/fpga-region.c | 115 +++++----------
drivers/fpga/ice40-spi.c | 10 +-
drivers/fpga/machxo2-spi.c | 11 +-
drivers/fpga/of-fpga-region.c | 11 +-
drivers/fpga/socfpga-a10.c | 16 +--
drivers/fpga/socfpga.c | 10 +-
drivers/fpga/stratix10-soc.c | 16 +--
drivers/fpga/ts73xx-fpga.c | 10 +-
drivers/fpga/xilinx-pr-decoupler.c | 17 +--
drivers/fpga/xilinx-spi.c | 12 +-
drivers/fpga/zynq-fpga.c | 16 +--
drivers/fpga/zynqmp-fpga.c | 10 +-
include/linux/fpga/fpga-bridge.h | 33 +++--
include/linux/fpga/fpga-mgr.h | 62 +++++---
include/linux/fpga/fpga-region.h | 36 +++--
27 files changed, 377 insertions(+), 480 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver
2021-06-21 22:22 [PATCH v6 0/3] fpga: Use standard class dev_release function Russ Weight
@ 2021-06-21 22:22 ` Russ Weight
2021-06-22 7:32 ` Xu Yilun
2021-06-22 8:45 ` Xu Yilun
2021-06-21 22:22 ` [PATCH v6 2/3] fpga: bridge: " Russ Weight
2021-06-21 22:22 ` [PATCH v6 3/3] fpga: region: " Russ Weight
2 siblings, 2 replies; 14+ messages in thread
From: Russ Weight @ 2021-06-21 22:22 UTC (permalink / raw)
To: mdf, linux-fpga
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
Russ Weight
The FPGA manager class driver data structure is being treated as a
managed resource instead of using standard dev_release call-back
to release the class data structure. This change removes the
managed resource code for the freeing of the class data structure
and combines the create() and register() functions into a single
register() function.
The devm_fpga_mgr_register() function is retained.
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
v6:
- Moved FPGA manager optional parameters out of the ops structure and
back into the FPGA manager structure.
- Changed fpga_mgr_register()/devm_fpga_mgr_register() parameters to
accept an info data structure to provide flexibility in passing optional
parameters.
- Added fpga_mgr_register_simple()/devm_fpga_mgr_register_simple()
functions to support current parameters for users that don't require
the use of optional parameters.
v5:
- Rebased on top of recently accepted patches.
- Removed compat_id from the fpga_mgr_register() parameter list
and added it to the fpga_manager_ops structure. This also required
dynamically allocating the dfl-fme-ops structure in order to add
the appropriate compat_id.
v4:
- Added the compat_id parameter to fpga_mgr_register() and
devm_fpga_mgr_register() to ensure that the compat_id is set before
the device_register() call.
v3:
- Cleaned up comment header for fpga_mgr_register()
- Fix error return on ida_simple_get() failure
v2:
- Restored devm_fpga_mgr_register() functionality, adapted for the combined
create/register functionality.
- All previous callers of devm_fpga_mgr_register() will continue to call
devm_fpga_mgr_register().
- replaced unnecessary ternary operators in return statements with standard
if conditions.
---
drivers/fpga/altera-cvp.c | 12 +-
drivers/fpga/altera-pr-ip-core.c | 9 +-
drivers/fpga/altera-ps-spi.c | 10 +-
drivers/fpga/dfl-fme-mgr.c | 23 ++--
drivers/fpga/fpga-mgr.c | 212 +++++++++++++------------------
drivers/fpga/ice40-spi.c | 10 +-
drivers/fpga/machxo2-spi.c | 11 +-
drivers/fpga/socfpga-a10.c | 16 +--
drivers/fpga/socfpga.c | 10 +-
drivers/fpga/stratix10-soc.c | 16 +--
drivers/fpga/ts73xx-fpga.c | 10 +-
drivers/fpga/xilinx-spi.c | 12 +-
drivers/fpga/zynq-fpga.c | 16 +--
drivers/fpga/zynqmp-fpga.c | 10 +-
include/linux/fpga/fpga-mgr.h | 62 ++++++---
15 files changed, 204 insertions(+), 235 deletions(-)
diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index ccf4546eff29..ddf2ffe3f138 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -652,19 +652,15 @@ 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));
- mgr = devm_fpga_mgr_create(&pdev->dev, conf->mgr_name,
- &altera_cvp_ops, conf);
- if (!mgr) {
- ret = -ENOMEM;
+ mgr = fpga_mgr_register_simple(&pdev->dev, conf->mgr_name,
+ &altera_cvp_ops, conf);
+ if (IS_ERR(mgr)) {
+ ret = PTR_ERR(mgr);
goto err_unmap;
}
pci_set_drvdata(pdev, mgr);
- ret = fpga_mgr_register(mgr);
- if (ret)
- goto err_unmap;
-
return 0;
err_unmap:
diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
index dfdf21ed34c4..fa3e2697d285 100644
--- a/drivers/fpga/altera-pr-ip-core.c
+++ b/drivers/fpga/altera-pr-ip-core.c
@@ -191,11 +191,12 @@ 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));
- mgr = devm_fpga_mgr_create(dev, dev_name(dev), &alt_pr_ops, priv);
- if (!mgr)
- return -ENOMEM;
+ mgr = devm_fpga_mgr_register_simple(dev, dev_name(dev),
+ &alt_pr_ops, priv);
+ if (IS_ERR(mgr))
+ return PTR_ERR(mgr);
- return devm_fpga_mgr_register(dev, mgr);
+ return 0;
}
EXPORT_SYMBOL_GPL(alt_pr_register);
diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
index 23bfd4d1ad0f..990689cf8d67 100644
--- a/drivers/fpga/altera-ps-spi.c
+++ b/drivers/fpga/altera-ps-spi.c
@@ -302,12 +302,12 @@ 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));
- mgr = devm_fpga_mgr_create(&spi->dev, conf->mgr_name,
- &altera_ps_ops, conf);
- if (!mgr)
- return -ENOMEM;
+ mgr = devm_fpga_mgr_register_simple(&spi->dev, conf->mgr_name,
+ &altera_ps_ops, conf);
+ if (IS_ERR(mgr))
+ return PTR_ERR(mgr);
- return devm_fpga_mgr_register(&spi->dev, mgr);
+ return 0;
}
static const struct spi_device_id altera_ps_spi_ids[] = {
diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
index d5861d13b306..36add746ac42 100644
--- a/drivers/fpga/dfl-fme-mgr.c
+++ b/drivers/fpga/dfl-fme-mgr.c
@@ -282,7 +282,7 @@ static void fme_mgr_get_compat_id(void __iomem *fme_pr,
static int fme_mgr_probe(struct platform_device *pdev)
{
struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
- struct fpga_compat_id *compat_id;
+ struct fpga_manager_info info = { 0 };
struct device *dev = &pdev->dev;
struct fme_mgr_priv *priv;
struct fpga_manager *mgr;
@@ -302,20 +302,19 @@ static int fme_mgr_probe(struct platform_device *pdev)
return PTR_ERR(priv->ioaddr);
}
- compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
- if (!compat_id)
+ info.name = "DFL FME FPGA Manager";
+ info.mops = &fme_mgr_ops;
+ info.priv = priv;
+ info.compat_id = devm_kzalloc(dev, sizeof(*info.compat_id), GFP_KERNEL);
+ if (!info.compat_id)
return -ENOMEM;
- fme_mgr_get_compat_id(priv->ioaddr, compat_id);
+ fme_mgr_get_compat_id(priv->ioaddr, info.compat_id);
+ mgr = devm_fpga_mgr_register(dev, &info);
+ if (IS_ERR(mgr))
+ return PTR_ERR(mgr);
- mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
- &fme_mgr_ops, priv);
- if (!mgr)
- return -ENOMEM;
-
- mgr->compat_id = compat_id;
-
- return devm_fpga_mgr_register(dev, mgr);
+ return 0;
}
static struct platform_driver fme_mgr_driver = {
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index ecb4c3c795fa..fa2f1a95de82 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -550,21 +550,19 @@ void fpga_mgr_unlock(struct fpga_manager *mgr)
EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
/**
- * fpga_mgr_create - create and initialize an FPGA manager struct
+ * fpga_mgr_register - create and register an FPGA Manager device
* @parent: fpga manager device from pdev
- * @name: fpga manager name
- * @mops: pointer to structure of fpga manager ops
- * @priv: fpga manager private data
+ * @info: parameters for fpga manager
*
- * The caller of this function is responsible for freeing the struct with
- * fpga_mgr_free(). Using devm_fpga_mgr_create() instead is recommended.
+ * The caller of this function is responsible for calling fpga_mgr_unregister().
+ * Using devm_fpga_mgr_register instead is recommended.
*
- * Return: pointer to struct fpga_manager or NULL
+ * Return: pointer to struct fpga_manager pointer or ERR_PTR()
*/
-struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
- const struct fpga_manager_ops *mops,
- void *priv)
+struct fpga_manager *
+fpga_mgr_register(struct device *parent, struct fpga_manager_info *info)
{
+ const struct fpga_manager_ops *mops = info->mops;
struct fpga_manager *mgr;
int id, ret;
@@ -572,29 +570,31 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
!mops->write_init || (!mops->write && !mops->write_sg) ||
(mops->write && mops->write_sg)) {
dev_err(parent, "Attempt to register without fpga_manager_ops\n");
- return NULL;
+ return ERR_PTR(-EINVAL);
}
- if (!name || !strlen(name)) {
+ if (!info->name || !strlen(info->name)) {
dev_err(parent, "Attempt to register with no name!\n");
- return NULL;
+ return ERR_PTR(-EINVAL);
}
mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
if (!mgr)
- return NULL;
+ return ERR_PTR(-ENOMEM);
id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
- if (id < 0)
+ if (id < 0) {
+ ret = id;
goto error_kfree;
+ }
mutex_init(&mgr->ref_mutex);
- mgr->name = name;
- mgr->mops = mops;
- mgr->priv = priv;
+ mgr->name = info->name;
+ mgr->mops = info->mops;
+ mgr->priv = info->priv;
+ mgr->compat_id = info->compat_id;
- device_initialize(&mgr->dev);
mgr->dev.class = fpga_mgr_class;
mgr->dev.groups = mops->groups;
mgr->dev.parent = parent;
@@ -605,6 +605,19 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
if (ret)
goto error_device;
+ /*
+ * Initialize framework state by requesting low level driver read state
+ * from device. FPGA may be in reset mode or may have been programmed
+ * by bootloader or EEPROM.
+ */
+ mgr->state = mgr->mops->state(mgr);
+
+ ret = device_register(&mgr->dev);
+ if (ret) {
+ put_device(&mgr->dev);
+ return ERR_PTR(ret);
+ }
+
return mgr;
error_device:
@@ -612,98 +625,38 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
error_kfree:
kfree(mgr);
- return NULL;
-}
-EXPORT_SYMBOL_GPL(fpga_mgr_create);
-
-/**
- * fpga_mgr_free - free an FPGA manager created with fpga_mgr_create()
- * @mgr: fpga manager struct
- */
-void fpga_mgr_free(struct fpga_manager *mgr)
-{
- ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
- kfree(mgr);
-}
-EXPORT_SYMBOL_GPL(fpga_mgr_free);
-
-static void devm_fpga_mgr_release(struct device *dev, void *res)
-{
- struct fpga_mgr_devres *dr = res;
-
- fpga_mgr_free(dr->mgr);
+ return ERR_PTR(ret);
}
+EXPORT_SYMBOL_GPL(fpga_mgr_register);
/**
- * devm_fpga_mgr_create - create and initialize a managed FPGA manager struct
+ * fpga_mgr_register_simple - create and register an FPGA Manager device
* @parent: fpga manager device from pdev
* @name: fpga manager name
* @mops: pointer to structure of fpga manager ops
* @priv: fpga manager private data
*
- * This function is intended for use in an FPGA manager driver's probe function.
- * After the manager driver creates the manager struct with
- * devm_fpga_mgr_create(), it should register it with fpga_mgr_register(). The
- * manager driver's remove function should call fpga_mgr_unregister(). The
- * manager struct allocated with this function will be freed automatically on
- * driver detach. This includes the case of a probe function returning error
- * before calling fpga_mgr_register(), the struct will still get cleaned up.
- *
- * Return: pointer to struct fpga_manager or NULL
- */
-struct fpga_manager *devm_fpga_mgr_create(struct device *parent, const char *name,
- const struct fpga_manager_ops *mops,
- void *priv)
-{
- struct fpga_mgr_devres *dr;
-
- dr = devres_alloc(devm_fpga_mgr_release, sizeof(*dr), GFP_KERNEL);
- if (!dr)
- return NULL;
-
- dr->mgr = fpga_mgr_create(parent, name, mops, priv);
- if (!dr->mgr) {
- devres_free(dr);
- return NULL;
- }
-
- devres_add(parent, dr);
-
- return dr->mgr;
-}
-EXPORT_SYMBOL_GPL(devm_fpga_mgr_create);
-
-/**
- * fpga_mgr_register - register an FPGA manager
- * @mgr: fpga manager struct
+ * The caller of this function is responsible for calling fpga_mgr_unregister().
+ * Using devm_fpga_mgr_register_simple instead is recommended. This simple
+ * version of the register function should be sufficient for most users. The
+ * fpga_mgr_register function is available for users that need to pass
+ * additional, optional parameters.
*
- * Return: 0 on success, negative error code otherwise.
+ * Return: pointer to struct fpga_manager pointer or ERR_PTR()
*/
-int fpga_mgr_register(struct fpga_manager *mgr)
+struct fpga_manager *
+fpga_mgr_register_simple(struct device *parent, const char *name,
+ const struct fpga_manager_ops *mops, void *priv)
{
- int ret;
+ struct fpga_manager_info info = { 0 };
- /*
- * Initialize framework state by requesting low level driver read state
- * from device. FPGA may be in reset mode or may have been programmed
- * by bootloader or EEPROM.
- */
- mgr->state = mgr->mops->state(mgr);
+ info.name = name;
+ info.mops = mops;
+ info.priv = priv;
- ret = device_add(&mgr->dev);
- if (ret)
- goto error_device;
-
- dev_info(&mgr->dev, "%s registered\n", mgr->name);
-
- return 0;
-
-error_device:
- ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
-
- return ret;
+ return fpga_mgr_register(parent, &info);
}
-EXPORT_SYMBOL_GPL(fpga_mgr_register);
+EXPORT_SYMBOL_GPL(fpga_mgr_register_simple);
/**
* fpga_mgr_unregister - unregister an FPGA manager
@@ -726,14 +679,6 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
}
EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
-static int fpga_mgr_devres_match(struct device *dev, void *res,
- void *match_data)
-{
- struct fpga_mgr_devres *dr = res;
-
- return match_data == dr->mgr;
-}
-
static void devm_fpga_mgr_unregister(struct device *dev, void *res)
{
struct fpga_mgr_devres *dr = res;
@@ -743,44 +688,67 @@ static void devm_fpga_mgr_unregister(struct device *dev, void *res)
/**
* devm_fpga_mgr_register - resource managed variant of fpga_mgr_register()
- * @dev: managing device for this FPGA manager
- * @mgr: fpga manager struct
+ * @parent: fpga manager device from pdev
+ * @info: parameters for fpga manager
*
* This is the devres variant of fpga_mgr_register() for which the unregister
* function will be called automatically when the managing device is detached.
*/
-int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr)
+struct fpga_manager *
+devm_fpga_mgr_register(struct device *dev, struct fpga_manager_info *info)
{
struct fpga_mgr_devres *dr;
- int ret;
-
- /*
- * Make sure that the struct fpga_manager * that is passed in is
- * managed itself.
- */
- if (WARN_ON(!devres_find(dev, devm_fpga_mgr_release,
- fpga_mgr_devres_match, mgr)))
- return -EINVAL;
+ struct fpga_manager *mgr;
dr = devres_alloc(devm_fpga_mgr_unregister, sizeof(*dr), GFP_KERNEL);
if (!dr)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
- ret = fpga_mgr_register(mgr);
- if (ret) {
+ mgr = fpga_mgr_register(dev, info);
+ if (IS_ERR(mgr)) {
devres_free(dr);
- return ret;
+ return mgr;
}
dr->mgr = mgr;
devres_add(dev, dr);
- return 0;
+ return mgr;
}
EXPORT_SYMBOL_GPL(devm_fpga_mgr_register);
+/**
+ * devm_fpga_mgr_register_simple - resource managed variant of
+ * fpga_mgr_register_simple()
+ * @dev: fpga manager device from pdev
+ * @name: fpga manager name
+ * @mops: pointer to structure of fpga manager ops
+ * @priv: fpga manager private data
+ *
+ * This is the devres variant of fpga_mgr_register_simple() for which the
+ * unregister function will be called automatically when the managing
+ * device is detached.
+ */
+struct fpga_manager *
+devm_fpga_mgr_register_simple(struct device *parent, const char *name,
+ const struct fpga_manager_ops *mops, void *priv)
+{
+ struct fpga_manager_info info = { 0 };
+
+ info.name = name;
+ info.mops = mops;
+ info.priv = priv;
+
+ return devm_fpga_mgr_register(parent, &info);
+}
+EXPORT_SYMBOL_GPL(devm_fpga_mgr_register_simple);
+
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 69dec5af23c3..498f05cf425d 100644
--- a/drivers/fpga/ice40-spi.c
+++ b/drivers/fpga/ice40-spi.c
@@ -178,12 +178,12 @@ static int ice40_fpga_probe(struct spi_device *spi)
return ret;
}
- mgr = devm_fpga_mgr_create(dev, "Lattice iCE40 FPGA Manager",
- &ice40_fpga_ops, priv);
- if (!mgr)
- return -ENOMEM;
+ mgr = devm_fpga_mgr_register_simple(dev, "Lattice iCE40 FPGA Manager",
+ &ice40_fpga_ops, priv);
+ if (IS_ERR(mgr))
+ return PTR_ERR(mgr);
- return devm_fpga_mgr_register(dev, mgr);
+ return 0;
}
static const struct of_device_id ice40_fpga_of_match[] = {
diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
index 114a64d2b7a4..f0a15b724b87 100644
--- a/drivers/fpga/machxo2-spi.c
+++ b/drivers/fpga/machxo2-spi.c
@@ -366,12 +366,13 @@ static int machxo2_spi_probe(struct spi_device *spi)
return -EINVAL;
}
- mgr = devm_fpga_mgr_create(dev, "Lattice MachXO2 SPI FPGA Manager",
- &machxo2_ops, spi);
- if (!mgr)
- return -ENOMEM;
+ mgr = devm_fpga_mgr_register_simple(dev,
+ "Lattice MachXO2 SPI FPGA Manager",
+ &machxo2_ops, spi);
+ if (IS_ERR(mgr))
+ return PTR_ERR(mgr);
- return devm_fpga_mgr_register(dev, mgr);
+ return 0;
}
static const struct of_device_id of_match[] = {
diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c
index 573d88bdf730..5ffefaa3eb07 100644
--- a/drivers/fpga/socfpga-a10.c
+++ b/drivers/fpga/socfpga-a10.c
@@ -508,19 +508,15 @@ static int socfpga_a10_fpga_probe(struct platform_device *pdev)
return -EBUSY;
}
- mgr = devm_fpga_mgr_create(dev, "SoCFPGA Arria10 FPGA Manager",
- &socfpga_a10_fpga_mgr_ops, priv);
- if (!mgr)
- return -ENOMEM;
-
- platform_set_drvdata(pdev, mgr);
-
- ret = fpga_mgr_register(mgr);
- if (ret) {
+ mgr = fpga_mgr_register_simple(dev, "SoCFPGA Arria10 FPGA Manager",
+ &socfpga_a10_fpga_mgr_ops, priv);
+ if (IS_ERR(mgr)) {
clk_disable_unprepare(priv->clk);
- return ret;
+ return PTR_ERR(mgr);
}
+ platform_set_drvdata(pdev, mgr);
+
return 0;
}
diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
index 1f467173fc1f..9b4ca0ad7466 100644
--- a/drivers/fpga/socfpga.c
+++ b/drivers/fpga/socfpga.c
@@ -571,12 +571,12 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
if (ret)
return ret;
- mgr = devm_fpga_mgr_create(dev, "Altera SOCFPGA FPGA Manager",
- &socfpga_fpga_ops, priv);
- if (!mgr)
- return -ENOMEM;
+ mgr = devm_fpga_mgr_register_simple(dev, "Altera SOCFPGA FPGA Manager",
+ &socfpga_fpga_ops, priv);
+ if (IS_ERR(mgr))
+ return PTR_ERR(mgr);
- return devm_fpga_mgr_register(dev, mgr);
+ return 0;
}
#ifdef CONFIG_OF
diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index a2cea500f7cc..fb84d88d4ce9 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -425,18 +425,11 @@ static int s10_probe(struct platform_device *pdev)
init_completion(&priv->status_return_completion);
- mgr = fpga_mgr_create(dev, "Stratix10 SOC FPGA Manager",
- &s10_ops, priv);
- if (!mgr) {
- dev_err(dev, "unable to create FPGA manager\n");
- ret = -ENOMEM;
- goto probe_err;
- }
-
- ret = fpga_mgr_register(mgr);
- if (ret) {
+ mgr = fpga_mgr_register_simple(dev, "Stratix10 SOC FPGA Manager",
+ &s10_ops, priv);
+ if (IS_ERR(mgr)) {
dev_err(dev, "unable to register FPGA manager\n");
- fpga_mgr_free(mgr);
+ ret = PTR_ERR(mgr);
goto probe_err;
}
@@ -454,7 +447,6 @@ static int s10_remove(struct platform_device *pdev)
struct s10_priv *priv = mgr->priv;
fpga_mgr_unregister(mgr);
- fpga_mgr_free(mgr);
stratix10_svc_free_channel(priv->chan);
return 0;
diff --git a/drivers/fpga/ts73xx-fpga.c b/drivers/fpga/ts73xx-fpga.c
index 101f016c6ed8..2e11eea095ce 100644
--- a/drivers/fpga/ts73xx-fpga.c
+++ b/drivers/fpga/ts73xx-fpga.c
@@ -122,12 +122,12 @@ static int ts73xx_fpga_probe(struct platform_device *pdev)
if (IS_ERR(priv->io_base))
return PTR_ERR(priv->io_base);
- mgr = devm_fpga_mgr_create(kdev, "TS-73xx FPGA Manager",
- &ts73xx_fpga_ops, priv);
- if (!mgr)
- return -ENOMEM;
+ mgr = devm_fpga_mgr_register_simple(kdev, "TS-73xx FPGA Manager",
+ &ts73xx_fpga_ops, priv);
+ if (IS_ERR(mgr))
+ return PTR_ERR(mgr);
- return devm_fpga_mgr_register(kdev, mgr);
+ return 0;
}
static struct platform_driver ts73xx_fpga_driver = {
diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index fee4d0abf6bf..e78b82f9db79 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -247,13 +247,13 @@ static int xilinx_spi_probe(struct spi_device *spi)
return dev_err_probe(&spi->dev, PTR_ERR(conf->done),
"Failed to get DONE gpio\n");
- mgr = devm_fpga_mgr_create(&spi->dev,
- "Xilinx Slave Serial FPGA Manager",
- &xilinx_spi_ops, conf);
- if (!mgr)
- return -ENOMEM;
+ mgr = devm_fpga_mgr_register_simple(&spi->dev,
+ "Xilinx Slave Serial FPGA Manager",
+ &xilinx_spi_ops, conf);
+ if (IS_ERR(mgr))
+ return PTR_ERR(mgr);
- return devm_fpga_mgr_register(&spi->dev, mgr);
+ return 0;
}
static const struct of_device_id xlnx_spi_of_match[] = {
diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 9b75bd4f93d8..a3de365aadc6 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -609,20 +609,16 @@ static int zynq_fpga_probe(struct platform_device *pdev)
clk_disable(priv->clk);
- mgr = devm_fpga_mgr_create(dev, "Xilinx Zynq FPGA Manager",
- &zynq_fpga_ops, priv);
- if (!mgr)
- return -ENOMEM;
-
- platform_set_drvdata(pdev, mgr);
-
- err = fpga_mgr_register(mgr);
- if (err) {
+ mgr = fpga_mgr_register_simple(dev, "Xilinx Zynq FPGA Manager",
+ &zynq_fpga_ops, priv);
+ if (IS_ERR(mgr)) {
dev_err(dev, "unable to register FPGA manager\n");
clk_unprepare(priv->clk);
- return err;
+ return PTR_ERR(mgr);
}
+ platform_set_drvdata(pdev, mgr);
+
return 0;
}
diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
index 125743c9797f..d743a05cba95 100644
--- a/drivers/fpga/zynqmp-fpga.c
+++ b/drivers/fpga/zynqmp-fpga.c
@@ -102,12 +102,12 @@ static int zynqmp_fpga_probe(struct platform_device *pdev)
priv->dev = dev;
- mgr = devm_fpga_mgr_create(dev, "Xilinx ZynqMP FPGA Manager",
- &zynqmp_fpga_ops, priv);
- if (!mgr)
- return -ENOMEM;
+ mgr = devm_fpga_mgr_register_simple(dev, "Xilinx ZynqMP FPGA Manager",
+ &zynqmp_fpga_ops, priv);
+ if (IS_ERR(mgr))
+ return PTR_ERR(mgr);
- return devm_fpga_mgr_register(dev, mgr);
+ return 0;
}
static const struct of_device_id zynqmp_fpga_of_match[] = {
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 474c1f506307..a814407bc588 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -105,6 +105,36 @@ struct fpga_image_info {
#endif
};
+/**
+ * struct fpga_compat_id - id for compatibility check
+ *
+ * @id_h: high 64bit of the compat_id
+ * @id_l: low 64bit of the compat_id
+ */
+struct fpga_compat_id {
+ u64 id_h;
+ u64 id_l;
+};
+
+/**
+ * struct fpga_manager_info - collection of parameters for an FPGA Manager
+ * @name: fpga manager name
+ * @compat_id: FPGA manager id for compatibility check.
+ * @mops: pointer to structure of fpga manager ops
+ * @priv: fpga manager private data
+ *
+ * fpga_manager_info contains parameters for the register function. These
+ * are separated into an info structure because they some are optional
+ * others could be added to in the future. The info structure facilitates
+ * maintaining a stable API.
+ */
+struct fpga_manager_info {
+ const char *name;
+ struct fpga_compat_id *compat_id;
+ const struct fpga_manager_ops *mops;
+ void *priv;
+};
+
/**
* struct fpga_manager_ops - ops for low level fpga manager drivers
* @initial_header_size: Maximum number of bytes that should be passed into write_init
@@ -143,17 +173,6 @@ struct fpga_manager_ops {
#define FPGA_MGR_STATUS_IP_PROTOCOL_ERR BIT(3)
#define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR BIT(4)
-/**
- * struct fpga_compat_id - id for compatibility check
- *
- * @id_h: high 64bit of the compat_id
- * @id_l: low 64bit of the compat_id
- */
-struct fpga_compat_id {
- u64 id_h;
- u64 id_l;
-};
-
/**
* struct fpga_manager - fpga manager structure
* @name: name of low level fpga manager
@@ -191,17 +210,18 @@ struct fpga_manager *fpga_mgr_get(struct device *dev);
void fpga_mgr_put(struct fpga_manager *mgr);
-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);
+struct fpga_manager *
+fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
-int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr);
+struct fpga_manager *
+fpga_mgr_register_simple(struct device *dev, const char *name,
+ const struct fpga_manager_ops *mops, void *priv);
+void fpga_mgr_unregister(struct fpga_manager *mgr);
-struct fpga_manager *devm_fpga_mgr_create(struct device *dev, const char *name,
- const struct fpga_manager_ops *mops,
- void *priv);
+struct fpga_manager *
+devm_fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
+struct fpga_manager *
+devm_fpga_mgr_register_simple(struct device *dev, const char *name,
+ const struct fpga_manager_ops *mops, void *priv);
#endif /*_LINUX_FPGA_MGR_H */
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v6 2/3] fpga: bridge: Use standard dev_release for class driver
2021-06-21 22:22 [PATCH v6 0/3] fpga: Use standard class dev_release function Russ Weight
2021-06-21 22:22 ` [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver Russ Weight
@ 2021-06-21 22:22 ` Russ Weight
2021-06-22 8:02 ` Xu Yilun
2021-06-22 8:23 ` Xu Yilun
2021-06-21 22:22 ` [PATCH v6 3/3] fpga: region: " Russ Weight
2 siblings, 2 replies; 14+ messages in thread
From: Russ Weight @ 2021-06-21 22:22 UTC (permalink / raw)
To: mdf, linux-fpga
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
Russ Weight
The FPGA bridge class driver data structure is being treated as a
managed resource instead of using standard dev_release call-back
to release the class data structure. This change removes the
managed resource code and combines the create() and register()
functions into a single register() function.
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
v6:
- Changed fpga_bridge_register() parameters to accept an info data
structure to provide flexibility in passing optional parameters.
- Added fpga_bridge_register_simple() function to support current
parameters for users that don't require the use of optional
parameters.
v5:
- Rebased on top of recently accepted patches.
v4:
- Restore the previous format for the Return value in the comment header
for fpga_bridge_register()
v3:
- Cleaned up comment header for fpga_bridge_register()
- Fix error return values for fpga_bridge_register()
v2:
- No changes
drivers/fpga/altera-fpga2sdram.c | 12 +--
drivers/fpga/altera-freeze-bridge.c | 10 +--
drivers/fpga/altera-hps2fpga.c | 12 +--
drivers/fpga/dfl-fme-br.c | 10 +--
drivers/fpga/fpga-bridge.c | 133 +++++++++-------------------
drivers/fpga/xilinx-pr-decoupler.c | 17 ++--
include/linux/fpga/fpga-bridge.h | 33 +++++--
7 files changed, 91 insertions(+), 136 deletions(-)
diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
index a78e49c63c64..e165440ebbab 100644
--- a/drivers/fpga/altera-fpga2sdram.c
+++ b/drivers/fpga/altera-fpga2sdram.c
@@ -121,17 +121,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);
- br = devm_fpga_bridge_create(dev, F2S_BRIDGE_NAME,
- &altera_fpga2sdram_br_ops, priv);
- if (!br)
- return -ENOMEM;
+ br = fpga_bridge_register_simple(dev, F2S_BRIDGE_NAME,
+ &altera_fpga2sdram_br_ops, priv);
+ if (IS_ERR(br))
+ return PTR_ERR(mgr);
platform_set_drvdata(pdev, br);
- ret = fpga_bridge_register(br);
- if (ret)
- return ret;
-
dev_info(dev, "driver initialized with handoff %08x\n", priv->mask);
if (!of_property_read_u32(dev->of_node, "bridge-enable", &enable)) {
diff --git a/drivers/fpga/altera-freeze-bridge.c b/drivers/fpga/altera-freeze-bridge.c
index dd58c4aea92e..4e39b5475630 100644
--- a/drivers/fpga/altera-freeze-bridge.c
+++ b/drivers/fpga/altera-freeze-bridge.c
@@ -244,14 +244,14 @@ static int altera_freeze_br_probe(struct platform_device *pdev)
priv->base_addr = base_addr;
- br = devm_fpga_bridge_create(dev, FREEZE_BRIDGE_NAME,
- &altera_freeze_br_br_ops, priv);
- if (!br)
- return -ENOMEM;
+ br = fpga_bridge_register_simple(dev, FREEZE_BRIDGE_NAME,
+ &altera_freeze_br_br_ops, priv);
+ if (IS_ERR(br))
+ return PTR_ERR(br);
platform_set_drvdata(pdev, br);
- return fpga_bridge_register(br);
+ return 0;
}
static int altera_freeze_br_remove(struct platform_device *pdev)
diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
index 77b95f251821..a564eb29349c 100644
--- a/drivers/fpga/altera-hps2fpga.c
+++ b/drivers/fpga/altera-hps2fpga.c
@@ -180,19 +180,15 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
}
}
- br = devm_fpga_bridge_create(dev, priv->name,
- &altera_hps2fpga_br_ops, priv);
- if (!br) {
- ret = -ENOMEM;
+ br = fpga_bridge_register_simple(dev, priv->name,
+ &altera_hps2fpga_br_ops, priv);
+ if (IS_ERR(br)) {
+ ret = PTR_ERR(br);
goto err;
}
platform_set_drvdata(pdev, br);
- ret = fpga_bridge_register(br);
- if (ret)
- goto err;
-
return 0;
err:
diff --git a/drivers/fpga/dfl-fme-br.c b/drivers/fpga/dfl-fme-br.c
index 3ff9f3a687ce..0ad39e502142 100644
--- a/drivers/fpga/dfl-fme-br.c
+++ b/drivers/fpga/dfl-fme-br.c
@@ -68,14 +68,14 @@ static int fme_br_probe(struct platform_device *pdev)
priv->pdata = dev_get_platdata(dev);
- br = devm_fpga_bridge_create(dev, "DFL FPGA FME Bridge",
- &fme_bridge_ops, priv);
- if (!br)
- return -ENOMEM;
+ br = fpga_bridge_register_simple(dev, "DFL FPGA FME Bridge",
+ &fme_bridge_ops, priv);
+ if (IS_ERR(br))
+ return PTR_ERR(br);
platform_set_drvdata(pdev, br);
- return fpga_bridge_register(br);
+ return 0;
}
static int fme_br_remove(struct platform_device *pdev)
diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
index 798f55670646..1436acfc1e7d 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -312,55 +312,57 @@ static struct attribute *fpga_bridge_attrs[] = {
ATTRIBUTE_GROUPS(fpga_bridge);
/**
- * fpga_bridge_create - create and initialize a struct fpga_bridge
+ * fpga_bridge_register - create and register an FPGA Bridge device
* @parent: FPGA bridge device from pdev
- * @name: FPGA bridge name
- * @br_ops: pointer to structure of fpga bridge ops
- * @priv: FPGA bridge private data
+ * @info: parameters for FPGA Bridge
*
- * The caller of this function is responsible for freeing the bridge with
- * fpga_bridge_free(). Using devm_fpga_bridge_create() instead is recommended.
- *
- * Return: struct fpga_bridge or NULL
+ * Return: struct fpga_bridge pointer or ERR_PTR()
*/
-struct fpga_bridge *fpga_bridge_create(struct device *parent, const char *name,
- const struct fpga_bridge_ops *br_ops,
- void *priv)
+struct fpga_bridge *fpga_bridge_register(struct device *parent,
+ struct fpga_bridge_info *info)
{
struct fpga_bridge *bridge;
int id, ret;
- if (!name || !strlen(name)) {
+ if (!info->name || !strlen(info->name)) {
dev_err(parent, "Attempt to register with no name!\n");
- return NULL;
+ return ERR_PTR(-EINVAL);
}
bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
if (!bridge)
- return NULL;
+ return ERR_PTR(-ENOMEM);
id = ida_simple_get(&fpga_bridge_ida, 0, 0, GFP_KERNEL);
- if (id < 0)
+ if (id < 0) {
+ ret = id;
goto error_kfree;
+ }
mutex_init(&bridge->mutex);
INIT_LIST_HEAD(&bridge->node);
- bridge->name = name;
- bridge->br_ops = br_ops;
- bridge->priv = priv;
+ bridge->name = info->name;
+ bridge->br_ops = info->br_ops;
+ bridge->priv = info->priv;
- device_initialize(&bridge->dev);
- bridge->dev.groups = br_ops->groups;
+ bridge->dev.groups = info->br_ops->groups;
bridge->dev.class = fpga_bridge_class;
bridge->dev.parent = parent;
bridge->dev.of_node = parent->of_node;
bridge->dev.id = id;
+ of_platform_populate(bridge->dev.of_node, NULL, NULL, &bridge->dev);
ret = dev_set_name(&bridge->dev, "br%d", id);
if (ret)
goto error_device;
+ ret = device_register(&bridge->dev);
+ if (ret) {
+ put_device(&bridge->dev);
+ return ERR_PTR(ret);
+ }
+
return bridge;
error_device:
@@ -368,90 +370,37 @@ struct fpga_bridge *fpga_bridge_create(struct device *parent, const char *name,
error_kfree:
kfree(bridge);
- return NULL;
-}
-EXPORT_SYMBOL_GPL(fpga_bridge_create);
-
-/**
- * fpga_bridge_free - free an fpga bridge created by fpga_bridge_create()
- * @bridge: FPGA bridge struct
- */
-void fpga_bridge_free(struct fpga_bridge *bridge)
-{
- ida_simple_remove(&fpga_bridge_ida, bridge->dev.id);
- kfree(bridge);
-}
-EXPORT_SYMBOL_GPL(fpga_bridge_free);
-
-static void devm_fpga_bridge_release(struct device *dev, void *res)
-{
- struct fpga_bridge *bridge = *(struct fpga_bridge **)res;
-
- fpga_bridge_free(bridge);
+ return ERR_PTR(ret);
}
+EXPORT_SYMBOL_GPL(fpga_bridge_register);
/**
- * devm_fpga_bridge_create - create and init a managed struct fpga_bridge
+ * fpga_bridge_register_simple - create and register an FPGA Bridge device
* @parent: FPGA bridge device from pdev
* @name: FPGA bridge name
* @br_ops: pointer to structure of fpga bridge ops
* @priv: FPGA bridge private data
*
- * This function is intended for use in an FPGA bridge driver's probe function.
- * After the bridge driver creates the struct with devm_fpga_bridge_create(), it
- * should register the bridge with fpga_bridge_register(). The bridge driver's
- * remove function should call fpga_bridge_unregister(). The bridge struct
- * allocated with this function will be freed automatically on driver detach.
- * This includes the case of a probe function returning error before calling
- * fpga_bridge_register(), the struct will still get cleaned up.
- *
- * Return: struct fpga_bridge or NULL
- */
-struct fpga_bridge
-*devm_fpga_bridge_create(struct device *parent, const char *name,
- const struct fpga_bridge_ops *br_ops, void *priv)
-{
- struct fpga_bridge **ptr, *bridge;
-
- ptr = devres_alloc(devm_fpga_bridge_release, sizeof(*ptr), GFP_KERNEL);
- if (!ptr)
- return NULL;
-
- bridge = fpga_bridge_create(parent, name, br_ops, priv);
- if (!bridge) {
- devres_free(ptr);
- } else {
- *ptr = bridge;
- devres_add(parent, ptr);
- }
-
- return bridge;
-}
-EXPORT_SYMBOL_GPL(devm_fpga_bridge_create);
-
-/**
- * fpga_bridge_register - register an FPGA bridge
+ * This simple version of the register should be sufficient for most users.
+ * The fpga_mgr_register function is available for users that need to pass
+ * additional, optional parameters.
*
- * @bridge: FPGA bridge struct
- *
- * Return: 0 for success, error code otherwise.
+ * Return: struct fpga_bridge pointer or ERR_PTR()
*/
-int fpga_bridge_register(struct fpga_bridge *bridge)
+struct fpga_bridge *
+fpga_bridge_register_simple(struct device *parent, const char *name,
+ const struct fpga_bridge_ops *br_ops,
+ void *priv)
{
- struct device *dev = &bridge->dev;
- int ret;
-
- ret = device_add(dev);
- if (ret)
- return ret;
+ struct fpga_bridge_info info = { 0 };
- of_platform_populate(dev->of_node, NULL, NULL, dev);
+ info.name = name;
+ info.br_ops = br_ops;
+ info.priv = priv;
- dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name);
-
- return 0;
+ return fpga_bridge_register(parent, &info);
}
-EXPORT_SYMBOL_GPL(fpga_bridge_register);
+EXPORT_SYMBOL_GPL(fpga_bridge_register_simple);
/**
* fpga_bridge_unregister - unregister an FPGA bridge
@@ -475,6 +424,10 @@ EXPORT_SYMBOL_GPL(fpga_bridge_unregister);
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 ea2bde6e5bc4..24bc16a86091 100644
--- a/drivers/fpga/xilinx-pr-decoupler.c
+++ b/drivers/fpga/xilinx-pr-decoupler.c
@@ -138,22 +138,17 @@ static int xlnx_pr_decoupler_probe(struct platform_device *pdev)
clk_disable(priv->clk);
- br = devm_fpga_bridge_create(&pdev->dev, priv->ipconfig->name,
- &xlnx_pr_decoupler_br_ops, priv);
- if (!br) {
- err = -ENOMEM;
- goto err_clk;
- }
-
- platform_set_drvdata(pdev, br);
-
- err = fpga_bridge_register(br);
- if (err) {
+ br = fpga_bridge_register_simple(&pdev->dev, priv->ipconfig->name,
+ &xlnx_pr_decoupler_br_ops, priv);
+ if (IS_ERR(br)) {
+ err = PTR_ERR(br);
dev_err(&pdev->dev, "unable to register %s",
priv->ipconfig->name);
goto err_clk;
}
+ platform_set_drvdata(pdev, br);
+
return 0;
err_clk:
diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
index 6c3c28806ff1..e111698012f0 100644
--- a/include/linux/fpga/fpga-bridge.h
+++ b/include/linux/fpga/fpga-bridge.h
@@ -22,6 +22,23 @@ struct fpga_bridge_ops {
const struct attribute_group **groups;
};
+/**
+ * struct fpga_bridge_info - collection of parameters an FPGA Bridge
+ * @name: fpga bridge name
+ * @br_ops: pointer to structure of fpga bridge ops
+ * @priv: fpga bridge private data
+ *
+ * fpga_bridge_info contains parameters for the register function. These
+ * are separated into an info structure because they some are optional
+ * others could be added to in the future. The info structure facilitates
+ * maintaining a stable API.
+ */
+struct fpga_bridge_info {
+ const char *name;
+ const struct fpga_bridge_ops *br_ops;
+ void *priv;
+};
+
/**
* struct fpga_bridge - FPGA bridge structure
* @name: name of low level FPGA bridge
@@ -62,15 +79,13 @@ int of_fpga_bridge_get_to_list(struct device_node *np,
struct fpga_image_info *info,
struct list_head *bridge_list);
-struct fpga_bridge *fpga_bridge_create(struct device *dev, const char *name,
- const struct fpga_bridge_ops *br_ops,
- void *priv);
-void fpga_bridge_free(struct fpga_bridge *br);
-int fpga_bridge_register(struct fpga_bridge *br);
-void fpga_bridge_unregister(struct fpga_bridge *br);
+struct fpga_bridge *
+fpga_bridge_register(struct device *dev, struct fpga_bridge_info *info);
-struct fpga_bridge
-*devm_fpga_bridge_create(struct device *dev, const char *name,
- const struct fpga_bridge_ops *br_ops, void *priv);
+struct fpga_bridge *
+fpga_bridge_register_simple(struct device *dev, const char *name,
+ const struct fpga_bridge_ops *br_ops,
+ void *priv);
+void fpga_bridge_unregister(struct fpga_bridge *br);
#endif /* _LINUX_FPGA_BRIDGE_H */
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v6 3/3] fpga: region: Use standard dev_release for class driver
2021-06-21 22:22 [PATCH v6 0/3] fpga: Use standard class dev_release function Russ Weight
2021-06-21 22:22 ` [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver Russ Weight
2021-06-21 22:22 ` [PATCH v6 2/3] fpga: bridge: " Russ Weight
@ 2021-06-21 22:22 ` Russ Weight
2021-06-22 8:19 ` Xu Yilun
2 siblings, 1 reply; 14+ messages in thread
From: Russ Weight @ 2021-06-21 22:22 UTC (permalink / raw)
To: mdf, linux-fpga
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
Russ Weight
The FPGA region class driver data structure is being treated as a
managed resource instead of using standard dev_release call-back
to release the class data structure. This change removes the
managed resource code and combines the create() and register()
functions into a single register() function.
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
v6:
- Moved FPGA manager optional parameters out of the ops structure and
back into the FPGA Region structure.
- Changed fpga_region_register() parameters to accept an info data
structure to provide flexibility in passing optional parameters.
- Added fpga_region_register_simple() functions to support current
parameters for users that don't require the use of optional parameters.
v5:
- Rebased on top of recently accepted patches.
- Created the fpga_region_ops data structure which is optionally passed
to fpga_region_register(). compat_id, the get_bridges() pointer, and
the priv pointer are included in the fpga_region_ops structure.
v4:
- Added the compat_id parameter to fpga_region_register() to ensure
that the compat_id is set before the device_register() call.
- Modified the dfl_fpga_feature_devs_enumerate() function to restore
the fpga_region_register() call to the correct location.
v3:
- Cleaned up comment header for fpga_region_register()
- Fix fpga_region_register() error return on ida_simple_get() failure
v2:
- No changes
---
drivers/fpga/dfl-fme-region.c | 17 +++--
drivers/fpga/dfl.c | 12 ++--
drivers/fpga/fpga-region.c | 115 +++++++++++--------------------
drivers/fpga/of-fpga-region.c | 11 ++-
include/linux/fpga/fpga-region.h | 36 +++++++---
5 files changed, 82 insertions(+), 109 deletions(-)
diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
index 1eeb42af1012..36bf6865bbc3 100644
--- a/drivers/fpga/dfl-fme-region.c
+++ b/drivers/fpga/dfl-fme-region.c
@@ -30,6 +30,7 @@ static int fme_region_get_bridges(struct fpga_region *region)
static int fme_region_probe(struct platform_device *pdev)
{
struct dfl_fme_region_pdata *pdata = dev_get_platdata(&pdev->dev);
+ struct fpga_region_info info = { 0 };
struct device *dev = &pdev->dev;
struct fpga_region *region;
struct fpga_manager *mgr;
@@ -39,20 +40,18 @@ static int fme_region_probe(struct platform_device *pdev)
if (IS_ERR(mgr))
return -EPROBE_DEFER;
- region = devm_fpga_region_create(dev, mgr, fme_region_get_bridges);
- if (!region) {
- ret = -ENOMEM;
+ info.mgr = mgr;
+ info.compat_id = mgr->compat_id;
+ info.get_bridges = fme_region_get_bridges;
+ info.priv = pdata;
+ region = fpga_region_register(dev, &info);
+ if (IS_ERR(region)) {
+ ret = PTR_ERR(region);
goto eprobe_mgr_put;
}
- region->priv = pdata;
- region->compat_id = mgr->compat_id;
platform_set_drvdata(pdev, region);
- ret = fpga_region_register(region);
- if (ret)
- goto eprobe_mgr_put;
-
dev_dbg(dev, "DFL FME FPGA Region probed\n");
return 0;
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 511b20ff35a3..a4269d40586e 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -1400,19 +1400,15 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
if (!cdev)
return ERR_PTR(-ENOMEM);
- cdev->region = devm_fpga_region_create(info->dev, NULL, NULL);
- if (!cdev->region) {
- ret = -ENOMEM;
- goto free_cdev_exit;
- }
-
cdev->parent = info->dev;
mutex_init(&cdev->lock);
INIT_LIST_HEAD(&cdev->port_dev_list);
- ret = fpga_region_register(cdev->region);
- if (ret)
+ cdev->region = fpga_region_register_simple(info->dev, NULL, NULL);
+ if (IS_ERR(cdev->region)) {
+ ret = PTR_ERR(cdev->region);
goto free_cdev_exit;
+ }
/* create and init build info for enumeration */
binfo = devm_kzalloc(info->dev, sizeof(*binfo), GFP_KERNEL);
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index a4838715221f..0d831b85a98f 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -180,39 +180,36 @@ static struct attribute *fpga_region_attrs[] = {
ATTRIBUTE_GROUPS(fpga_region);
/**
- * fpga_region_create - alloc and init a struct fpga_region
+ * fpga_region_register - create and register an FPGA Region device
* @parent: device parent
- * @mgr: manager that programs this region
- * @get_bridges: optional function to get bridges to a list
- *
- * The caller of this function is responsible for freeing the resulting region
- * struct with fpga_region_free(). Using devm_fpga_region_create() instead is
- * recommended.
+ * @info: parameters for FPGA Region
*
- * Return: struct fpga_region or NULL
+ * Return: struct fpga_region or ERR_PTR()
*/
-struct fpga_region
-*fpga_region_create(struct device *parent,
- struct fpga_manager *mgr,
- int (*get_bridges)(struct fpga_region *))
+struct fpga_region *
+fpga_region_register(struct device *parent, struct fpga_region_info *info)
{
struct fpga_region *region;
int id, ret = 0;
region = kzalloc(sizeof(*region), GFP_KERNEL);
if (!region)
- return NULL;
+ return ERR_PTR(-ENOMEM);
id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
- if (id < 0)
+ if (id < 0) {
+ ret = id;
goto err_free;
+ }
+
+ region->mgr = info->mgr;
+ region->compat_id = info->compat_id;
+ region->priv = info->priv;
+ region->get_bridges = info->get_bridges;
- region->mgr = mgr;
- region->get_bridges = get_bridges;
mutex_init(®ion->mutex);
INIT_LIST_HEAD(®ion->bridge_list);
- device_initialize(®ion->dev);
region->dev.class = fpga_region_class;
region->dev.parent = parent;
region->dev.of_node = parent->of_node;
@@ -222,6 +219,12 @@ struct fpga_region
if (ret)
goto err_remove;
+ ret = device_register(®ion->dev);
+ if (ret) {
+ put_device(®ion->dev);
+ return ERR_PTR(ret);
+ }
+
return region;
err_remove:
@@ -229,78 +232,34 @@ struct fpga_region
err_free:
kfree(region);
- return NULL;
-}
-EXPORT_SYMBOL_GPL(fpga_region_create);
-
-/**
- * fpga_region_free - free an FPGA region created by fpga_region_create()
- * @region: FPGA region
- */
-void fpga_region_free(struct fpga_region *region)
-{
- ida_simple_remove(&fpga_region_ida, region->dev.id);
- kfree(region);
-}
-EXPORT_SYMBOL_GPL(fpga_region_free);
-
-static void devm_fpga_region_release(struct device *dev, void *res)
-{
- struct fpga_region *region = *(struct fpga_region **)res;
-
- fpga_region_free(region);
+ return ERR_PTR(ret);
}
+EXPORT_SYMBOL_GPL(fpga_region_register);
/**
- * devm_fpga_region_create - create and initialize a managed FPGA region struct
+ * fpga_region_register_simple - create and register an FPGA Region device
* @parent: device parent
* @mgr: manager that programs this region
* @get_bridges: optional function to get bridges to a list
*
- * This function is intended for use in an FPGA region driver's probe function.
- * After the region driver creates the region struct with
- * devm_fpga_region_create(), it should register it with fpga_region_register().
- * The region driver's remove function should call fpga_region_unregister().
- * The region struct allocated with this function will be freed automatically on
- * driver detach. This includes the case of a probe function returning error
- * before calling fpga_region_register(), the struct will still get cleaned up.
+ * This simple version of the register should be sufficient for most users.
+ * The fpga_bridge_register function is available for users that need to
+ * pass additional, optional parameters.
*
- * Return: struct fpga_region or NULL
+ * Return: struct fpga_region or ERR_PTR()
*/
-struct fpga_region
-*devm_fpga_region_create(struct device *parent,
- struct fpga_manager *mgr,
- int (*get_bridges)(struct fpga_region *))
+struct fpga_region *
+fpga_region_register_simple(struct device *parent, struct fpga_manager *mgr,
+ int (*get_bridges)(struct fpga_region *))
{
- struct fpga_region **ptr, *region;
-
- ptr = devres_alloc(devm_fpga_region_release, sizeof(*ptr), GFP_KERNEL);
- if (!ptr)
- return NULL;
+ struct fpga_region_info info = { 0 };
- region = fpga_region_create(parent, mgr, get_bridges);
- if (!region) {
- devres_free(ptr);
- } else {
- *ptr = region;
- devres_add(parent, ptr);
- }
+ info.mgr = mgr;
+ info.get_bridges = get_bridges;
- return region;
+ return fpga_region_register(parent, &info);
}
-EXPORT_SYMBOL_GPL(devm_fpga_region_create);
-
-/**
- * fpga_region_register - register an FPGA region
- * @region: FPGA region
- *
- * Return: 0 or -errno
- */
-int fpga_region_register(struct fpga_region *region)
-{
- return device_add(®ion->dev);
-}
-EXPORT_SYMBOL_GPL(fpga_region_register);
+EXPORT_SYMBOL_GPL(fpga_region_register_simple);
/**
* fpga_region_unregister - unregister an FPGA region
@@ -316,6 +275,10 @@ EXPORT_SYMBOL_GPL(fpga_region_unregister);
static void fpga_region_dev_release(struct device *dev)
{
+ struct fpga_region *region = to_fpga_region(dev);
+
+ ida_simple_remove(&fpga_region_ida, region->dev.id);
+ kfree(region);
}
/**
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index e3c25576b6b9..377d19585d8e 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -405,16 +405,13 @@ static int of_fpga_region_probe(struct platform_device *pdev)
if (IS_ERR(mgr))
return -EPROBE_DEFER;
- region = devm_fpga_region_create(dev, mgr, of_fpga_region_get_bridges);
- if (!region) {
- ret = -ENOMEM;
+ region = fpga_region_register_simple(dev, mgr,
+ of_fpga_region_get_bridges);
+ if (IS_ERR(region)) {
+ ret = PTR_ERR(region);
goto eprobe_mgr_put;
}
- ret = fpga_region_register(region);
- if (ret)
- goto eprobe_mgr_put;
-
of_platform_populate(np, fpga_region_of_match, NULL, ®ion->dev);
platform_set_drvdata(pdev, region);
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 27cb706275db..7b8f76d52c4e 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -7,6 +7,27 @@
#include <linux/fpga/fpga-mgr.h>
#include <linux/fpga/fpga-bridge.h>
+struct fpga_region;
+
+/**
+ * struct fpga_region_info - collection of parameters an FPGA Region
+ * @mgr: fpga region manager
+ * @compat_id: FPGA region id for compatibility check.
+ * @priv: fpga region private data
+ * @get_bridges: optional function to get bridges to a list
+ *
+ * fpga_region_info contains parameters for the register function. These
+ * are separated into an info structure because they some are optional
+ * others could be added to in the future. The info structure facilitates
+ * maintaining a stable API.
+ */
+struct fpga_region_info {
+ struct fpga_manager *mgr;
+ struct fpga_compat_id *compat_id;
+ void *priv;
+ int (*get_bridges)(struct fpga_region *region);
+};
+
/**
* struct fpga_region - FPGA Region structure
* @dev: FPGA Region device
@@ -37,15 +58,12 @@ struct fpga_region *fpga_region_class_find(
int fpga_region_program_fpga(struct fpga_region *region);
-struct fpga_region
-*fpga_region_create(struct device *dev, struct fpga_manager *mgr,
- int (*get_bridges)(struct fpga_region *));
-void fpga_region_free(struct fpga_region *region);
-int fpga_region_register(struct fpga_region *region);
-void fpga_region_unregister(struct fpga_region *region);
+struct fpga_region *
+fpga_region_register(struct device *dev, struct fpga_region_info *info);
-struct fpga_region
-*devm_fpga_region_create(struct device *dev, struct fpga_manager *mgr,
- int (*get_bridges)(struct fpga_region *));
+struct fpga_region *
+fpga_region_register_simple(struct device *dev, struct fpga_manager *mgr,
+ int (*get_bridges)(struct fpga_region *));
+void fpga_region_unregister(struct fpga_region *region);
#endif /* _FPGA_REGION_H */
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver
2021-06-21 22:22 ` [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver Russ Weight
@ 2021-06-22 7:32 ` Xu Yilun
2021-06-22 22:32 ` Russ Weight
2021-06-22 8:45 ` Xu Yilun
1 sibling, 1 reply; 14+ messages in thread
From: Xu Yilun @ 2021-06-22 7:32 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
Good to me overall, minor fixes see inline.
On Mon, Jun 21, 2021 at 03:22:47PM -0700, Russ Weight wrote:
> The FPGA manager class driver data structure is being treated as a
> managed resource instead of using standard dev_release call-back
> to release the class data structure. This change removes the
> managed resource code for the freeing of the class data structure
> and combines the create() and register() functions into a single
> register() function.
>
> The devm_fpga_mgr_register() function is retained.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v6:
> - Moved FPGA manager optional parameters out of the ops structure and
> back into the FPGA manager structure.
> - Changed fpga_mgr_register()/devm_fpga_mgr_register() parameters to
> accept an info data structure to provide flexibility in passing optional
> parameters.
> - Added fpga_mgr_register_simple()/devm_fpga_mgr_register_simple()
> functions to support current parameters for users that don't require
> the use of optional parameters.
These description should be added to the commit message if the idea is
to be accepted.
> v5:
> - Rebased on top of recently accepted patches.
> - Removed compat_id from the fpga_mgr_register() parameter list
> and added it to the fpga_manager_ops structure. This also required
> dynamically allocating the dfl-fme-ops structure in order to add
> the appropriate compat_id.
> v4:
> - Added the compat_id parameter to fpga_mgr_register() and
> devm_fpga_mgr_register() to ensure that the compat_id is set before
> the device_register() call.
> v3:
> - Cleaned up comment header for fpga_mgr_register()
> - Fix error return on ida_simple_get() failure
> v2:
> - Restored devm_fpga_mgr_register() functionality, adapted for the combined
> create/register functionality.
> - All previous callers of devm_fpga_mgr_register() will continue to call
> devm_fpga_mgr_register().
> - replaced unnecessary ternary operators in return statements with standard
> if conditions.
> ---
> drivers/fpga/altera-cvp.c | 12 +-
> drivers/fpga/altera-pr-ip-core.c | 9 +-
> drivers/fpga/altera-ps-spi.c | 10 +-
> drivers/fpga/dfl-fme-mgr.c | 23 ++--
> drivers/fpga/fpga-mgr.c | 212 +++++++++++++------------------
> drivers/fpga/ice40-spi.c | 10 +-
> drivers/fpga/machxo2-spi.c | 11 +-
> drivers/fpga/socfpga-a10.c | 16 +--
> drivers/fpga/socfpga.c | 10 +-
> drivers/fpga/stratix10-soc.c | 16 +--
> drivers/fpga/ts73xx-fpga.c | 10 +-
> drivers/fpga/xilinx-spi.c | 12 +-
> drivers/fpga/zynq-fpga.c | 16 +--
> drivers/fpga/zynqmp-fpga.c | 10 +-
> include/linux/fpga/fpga-mgr.h | 62 ++++++---
> 15 files changed, 204 insertions(+), 235 deletions(-)
>
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index ccf4546eff29..ddf2ffe3f138 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -652,19 +652,15 @@ 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));
>
> - mgr = devm_fpga_mgr_create(&pdev->dev, conf->mgr_name,
> - &altera_cvp_ops, conf);
> - if (!mgr) {
> - ret = -ENOMEM;
> + mgr = fpga_mgr_register_simple(&pdev->dev, conf->mgr_name,
> + &altera_cvp_ops, conf);
> + if (IS_ERR(mgr)) {
> + ret = PTR_ERR(mgr);
> goto err_unmap;
> }
>
> pci_set_drvdata(pdev, mgr);
>
> - ret = fpga_mgr_register(mgr);
> - if (ret)
> - goto err_unmap;
> -
> return 0;
>
> err_unmap:
> diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
> index dfdf21ed34c4..fa3e2697d285 100644
> --- a/drivers/fpga/altera-pr-ip-core.c
> +++ b/drivers/fpga/altera-pr-ip-core.c
> @@ -191,11 +191,12 @@ 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));
>
> - mgr = devm_fpga_mgr_create(dev, dev_name(dev), &alt_pr_ops, priv);
> - if (!mgr)
> - return -ENOMEM;
> + mgr = devm_fpga_mgr_register_simple(dev, dev_name(dev),
> + &alt_pr_ops, priv);
> + if (IS_ERR(mgr))
> + return PTR_ERR(mgr);
>
> - return devm_fpga_mgr_register(dev, mgr);
> + return 0;
I just remember there is a macro PTR_ERR_OR_ZERO() for this, or lkp robot
will make the suggestion.
> }
> EXPORT_SYMBOL_GPL(alt_pr_register);
>
> diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
> index 23bfd4d1ad0f..990689cf8d67 100644
> --- a/drivers/fpga/altera-ps-spi.c
> +++ b/drivers/fpga/altera-ps-spi.c
> @@ -302,12 +302,12 @@ 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));
>
> - mgr = devm_fpga_mgr_create(&spi->dev, conf->mgr_name,
> - &altera_ps_ops, conf);
> - if (!mgr)
> - return -ENOMEM;
> + mgr = devm_fpga_mgr_register_simple(&spi->dev, conf->mgr_name,
> + &altera_ps_ops, conf);
> + if (IS_ERR(mgr))
> + return PTR_ERR(mgr);
>
> - return devm_fpga_mgr_register(&spi->dev, mgr);
> + return 0;
PTR_ERR_OR_ZERO()
> }
>
> static const struct spi_device_id altera_ps_spi_ids[] = {
> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> index d5861d13b306..36add746ac42 100644
> --- a/drivers/fpga/dfl-fme-mgr.c
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -282,7 +282,7 @@ static void fme_mgr_get_compat_id(void __iomem *fme_pr,
> static int fme_mgr_probe(struct platform_device *pdev)
> {
> struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
> - struct fpga_compat_id *compat_id;
> + struct fpga_manager_info info = { 0 };
> struct device *dev = &pdev->dev;
> struct fme_mgr_priv *priv;
> struct fpga_manager *mgr;
> @@ -302,20 +302,19 @@ static int fme_mgr_probe(struct platform_device *pdev)
> return PTR_ERR(priv->ioaddr);
> }
>
> - compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
> - if (!compat_id)
> + info.name = "DFL FME FPGA Manager";
> + info.mops = &fme_mgr_ops;
> + info.priv = priv;
> + info.compat_id = devm_kzalloc(dev, sizeof(*info.compat_id), GFP_KERNEL);
> + if (!info.compat_id)
> return -ENOMEM;
>
> - fme_mgr_get_compat_id(priv->ioaddr, compat_id);
> + fme_mgr_get_compat_id(priv->ioaddr, info.compat_id);
> + mgr = devm_fpga_mgr_register(dev, &info);
> + if (IS_ERR(mgr))
> + return PTR_ERR(mgr);
>
> - mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
> - &fme_mgr_ops, priv);
> - if (!mgr)
> - return -ENOMEM;
> -
> - mgr->compat_id = compat_id;
> -
> - return devm_fpga_mgr_register(dev, mgr);
> + return 0;
PTR_ERR_OR_ZERO()
> }
>
> static struct platform_driver fme_mgr_driver = {
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index ecb4c3c795fa..fa2f1a95de82 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -550,21 +550,19 @@ void fpga_mgr_unlock(struct fpga_manager *mgr)
> EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
>
> /**
> - * fpga_mgr_create - create and initialize an FPGA manager struct
> + * fpga_mgr_register - create and register an FPGA Manager device
> * @parent: fpga manager device from pdev
> - * @name: fpga manager name
> - * @mops: pointer to structure of fpga manager ops
> - * @priv: fpga manager private data
> + * @info: parameters for fpga manager
> *
> - * The caller of this function is responsible for freeing the struct with
> - * fpga_mgr_free(). Using devm_fpga_mgr_create() instead is recommended.
> + * The caller of this function is responsible for calling fpga_mgr_unregister().
> + * Using devm_fpga_mgr_register instead is recommended.
devm_fpga_mgr_register()
> *
> - * Return: pointer to struct fpga_manager or NULL
> + * Return: pointer to struct fpga_manager pointer or ERR_PTR()
> */
> -struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
> - const struct fpga_manager_ops *mops,
> - void *priv)
> +struct fpga_manager *
> +fpga_mgr_register(struct device *parent, struct fpga_manager_info *info)
const struct fpga_manager_info *info
> {
> + const struct fpga_manager_ops *mops = info->mops;
> struct fpga_manager *mgr;
> int id, ret;
>
> @@ -572,29 +570,31 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
> !mops->write_init || (!mops->write && !mops->write_sg) ||
> (mops->write && mops->write_sg)) {
> dev_err(parent, "Attempt to register without fpga_manager_ops\n");
> - return NULL;
> + return ERR_PTR(-EINVAL);
> }
>
> - if (!name || !strlen(name)) {
> + if (!info->name || !strlen(info->name)) {
> dev_err(parent, "Attempt to register with no name!\n");
> - return NULL;
> + return ERR_PTR(-EINVAL);
> }
>
> mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> if (!mgr)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> - if (id < 0)
> + if (id < 0) {
> + ret = id;
> goto error_kfree;
> + }
>
> mutex_init(&mgr->ref_mutex);
>
> - mgr->name = name;
> - mgr->mops = mops;
> - mgr->priv = priv;
> + mgr->name = info->name;
> + mgr->mops = info->mops;
> + mgr->priv = info->priv;
> + mgr->compat_id = info->compat_id;
>
> - device_initialize(&mgr->dev);
> mgr->dev.class = fpga_mgr_class;
> mgr->dev.groups = mops->groups;
> mgr->dev.parent = parent;
> @@ -605,6 +605,19 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
> if (ret)
> goto error_device;
>
> + /*
> + * Initialize framework state by requesting low level driver read state
> + * from device. FPGA may be in reset mode or may have been programmed
> + * by bootloader or EEPROM.
> + */
> + mgr->state = mgr->mops->state(mgr);
> +
> + ret = device_register(&mgr->dev);
> + if (ret) {
> + put_device(&mgr->dev);
> + return ERR_PTR(ret);
> + }
> +
> return mgr;
>
> error_device:
> @@ -612,98 +625,38 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
> error_kfree:
> kfree(mgr);
>
> - return NULL;
> -}
> -EXPORT_SYMBOL_GPL(fpga_mgr_create);
> -
> -/**
> - * fpga_mgr_free - free an FPGA manager created with fpga_mgr_create()
> - * @mgr: fpga manager struct
> - */
> -void fpga_mgr_free(struct fpga_manager *mgr)
> -{
> - ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
> - kfree(mgr);
> -}
> -EXPORT_SYMBOL_GPL(fpga_mgr_free);
> -
> -static void devm_fpga_mgr_release(struct device *dev, void *res)
> -{
> - struct fpga_mgr_devres *dr = res;
> -
> - fpga_mgr_free(dr->mgr);
> + return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL_GPL(fpga_mgr_register);
>
> /**
> - * devm_fpga_mgr_create - create and initialize a managed FPGA manager struct
> + * fpga_mgr_register_simple - create and register an FPGA Manager device
> * @parent: fpga manager device from pdev
> * @name: fpga manager name
> * @mops: pointer to structure of fpga manager ops
> * @priv: fpga manager private data
> *
> - * This function is intended for use in an FPGA manager driver's probe function.
> - * After the manager driver creates the manager struct with
> - * devm_fpga_mgr_create(), it should register it with fpga_mgr_register(). The
> - * manager driver's remove function should call fpga_mgr_unregister(). The
> - * manager struct allocated with this function will be freed automatically on
> - * driver detach. This includes the case of a probe function returning error
> - * before calling fpga_mgr_register(), the struct will still get cleaned up.
> - *
> - * Return: pointer to struct fpga_manager or NULL
> - */
> -struct fpga_manager *devm_fpga_mgr_create(struct device *parent, const char *name,
> - const struct fpga_manager_ops *mops,
> - void *priv)
> -{
> - struct fpga_mgr_devres *dr;
> -
> - dr = devres_alloc(devm_fpga_mgr_release, sizeof(*dr), GFP_KERNEL);
> - if (!dr)
> - return NULL;
> -
> - dr->mgr = fpga_mgr_create(parent, name, mops, priv);
> - if (!dr->mgr) {
> - devres_free(dr);
> - return NULL;
> - }
> -
> - devres_add(parent, dr);
> -
> - return dr->mgr;
> -}
> -EXPORT_SYMBOL_GPL(devm_fpga_mgr_create);
> -
> -/**
> - * fpga_mgr_register - register an FPGA manager
> - * @mgr: fpga manager struct
> + * The caller of this function is responsible for calling fpga_mgr_unregister().
> + * Using devm_fpga_mgr_register_simple instead is recommended. This simple
devm_fpga_mgr_register_simple()
> + * version of the register function should be sufficient for most users. The
> + * fpga_mgr_register function is available for users that need to pass
> + * additional, optional parameters.
> *
> - * Return: 0 on success, negative error code otherwise.
> + * Return: pointer to struct fpga_manager pointer or ERR_PTR()
> */
> -int fpga_mgr_register(struct fpga_manager *mgr)
> +struct fpga_manager *
> +fpga_mgr_register_simple(struct device *parent, const char *name,
> + const struct fpga_manager_ops *mops, void *priv)
> {
> - int ret;
> + struct fpga_manager_info info = { 0 };
>
> - /*
> - * Initialize framework state by requesting low level driver read state
> - * from device. FPGA may be in reset mode or may have been programmed
> - * by bootloader or EEPROM.
> - */
> - mgr->state = mgr->mops->state(mgr);
> + info.name = name;
> + info.mops = mops;
> + info.priv = priv;
>
> - ret = device_add(&mgr->dev);
> - if (ret)
> - goto error_device;
> -
> - dev_info(&mgr->dev, "%s registered\n", mgr->name);
> -
> - return 0;
> -
> -error_device:
> - ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
> -
> - return ret;
> + return fpga_mgr_register(parent, &info);
> }
> -EXPORT_SYMBOL_GPL(fpga_mgr_register);
> +EXPORT_SYMBOL_GPL(fpga_mgr_register_simple);
>
> /**
> * fpga_mgr_unregister - unregister an FPGA manager
> @@ -726,14 +679,6 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
> }
> EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
>
> -static int fpga_mgr_devres_match(struct device *dev, void *res,
> - void *match_data)
> -{
> - struct fpga_mgr_devres *dr = res;
> -
> - return match_data == dr->mgr;
> -}
> -
> static void devm_fpga_mgr_unregister(struct device *dev, void *res)
> {
> struct fpga_mgr_devres *dr = res;
> @@ -743,44 +688,67 @@ static void devm_fpga_mgr_unregister(struct device *dev, void *res)
>
> /**
> * devm_fpga_mgr_register - resource managed variant of fpga_mgr_register()
> - * @dev: managing device for this FPGA manager
> - * @mgr: fpga manager struct
> + * @parent: fpga manager device from pdev
> + * @info: parameters for fpga manager
> *
> * This is the devres variant of fpga_mgr_register() for which the unregister
> * function will be called automatically when the managing device is detached.
> */
> -int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr)
> +struct fpga_manager *
> +devm_fpga_mgr_register(struct device *dev, struct fpga_manager_info *info)
const struct fpga_manager_info
> {
> struct fpga_mgr_devres *dr;
> - int ret;
> -
> - /*
> - * Make sure that the struct fpga_manager * that is passed in is
> - * managed itself.
> - */
> - if (WARN_ON(!devres_find(dev, devm_fpga_mgr_release,
> - fpga_mgr_devres_match, mgr)))
> - return -EINVAL;
> + struct fpga_manager *mgr;
>
> dr = devres_alloc(devm_fpga_mgr_unregister, sizeof(*dr), GFP_KERNEL);
> if (!dr)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> - ret = fpga_mgr_register(mgr);
> - if (ret) {
> + mgr = fpga_mgr_register(dev, info);
> + if (IS_ERR(mgr)) {
> devres_free(dr);
> - return ret;
> + return mgr;
> }
>
> dr->mgr = mgr;
> devres_add(dev, dr);
>
> - return 0;
> + return mgr;
> }
> EXPORT_SYMBOL_GPL(devm_fpga_mgr_register);
>
> +/**
> + * devm_fpga_mgr_register_simple - resource managed variant of
> + * fpga_mgr_register_simple()
> + * @dev: fpga manager device from pdev
@parent:
> + * @name: fpga manager name
> + * @mops: pointer to structure of fpga manager ops
> + * @priv: fpga manager private data
> + *
> + * This is the devres variant of fpga_mgr_register_simple() for which the
> + * unregister function will be called automatically when the managing
> + * device is detached.
> + */
> +struct fpga_manager *
> +devm_fpga_mgr_register_simple(struct device *parent, const char *name,
> + const struct fpga_manager_ops *mops, void *priv)
> +{
> + struct fpga_manager_info info = { 0 };
> +
> + info.name = name;
> + info.mops = mops;
> + info.priv = priv;
> +
> + return devm_fpga_mgr_register(parent, &info);
> +}
> +EXPORT_SYMBOL_GPL(devm_fpga_mgr_register_simple);
> +
> 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 69dec5af23c3..498f05cf425d 100644
> --- a/drivers/fpga/ice40-spi.c
> +++ b/drivers/fpga/ice40-spi.c
> @@ -178,12 +178,12 @@ static int ice40_fpga_probe(struct spi_device *spi)
> return ret;
> }
>
> - mgr = devm_fpga_mgr_create(dev, "Lattice iCE40 FPGA Manager",
> - &ice40_fpga_ops, priv);
> - if (!mgr)
> - return -ENOMEM;
> + mgr = devm_fpga_mgr_register_simple(dev, "Lattice iCE40 FPGA Manager",
> + &ice40_fpga_ops, priv);
> + if (IS_ERR(mgr))
> + return PTR_ERR(mgr);
>
> - return devm_fpga_mgr_register(dev, mgr);
> + return 0;
PTR_ERR_OR_ZERO()
> }
>
> static const struct of_device_id ice40_fpga_of_match[] = {
> diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
> index 114a64d2b7a4..f0a15b724b87 100644
> --- a/drivers/fpga/machxo2-spi.c
> +++ b/drivers/fpga/machxo2-spi.c
> @@ -366,12 +366,13 @@ static int machxo2_spi_probe(struct spi_device *spi)
> return -EINVAL;
> }
>
> - mgr = devm_fpga_mgr_create(dev, "Lattice MachXO2 SPI FPGA Manager",
> - &machxo2_ops, spi);
> - if (!mgr)
> - return -ENOMEM;
> + mgr = devm_fpga_mgr_register_simple(dev,
> + "Lattice MachXO2 SPI FPGA Manager",
> + &machxo2_ops, spi);
> + if (IS_ERR(mgr))
> + return PTR_ERR(mgr);
>
> - return devm_fpga_mgr_register(dev, mgr);
> + return 0;
PTR_ERR_OR_ZERO()
> }
>
> static const struct of_device_id of_match[] = {
> diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c
> index 573d88bdf730..5ffefaa3eb07 100644
> --- a/drivers/fpga/socfpga-a10.c
> +++ b/drivers/fpga/socfpga-a10.c
> @@ -508,19 +508,15 @@ static int socfpga_a10_fpga_probe(struct platform_device *pdev)
> return -EBUSY;
> }
>
> - mgr = devm_fpga_mgr_create(dev, "SoCFPGA Arria10 FPGA Manager",
> - &socfpga_a10_fpga_mgr_ops, priv);
> - if (!mgr)
> - return -ENOMEM;
> -
> - platform_set_drvdata(pdev, mgr);
> -
> - ret = fpga_mgr_register(mgr);
> - if (ret) {
> + mgr = fpga_mgr_register_simple(dev, "SoCFPGA Arria10 FPGA Manager",
> + &socfpga_a10_fpga_mgr_ops, priv);
> + if (IS_ERR(mgr)) {
> clk_disable_unprepare(priv->clk);
> - return ret;
> + return PTR_ERR(mgr);
> }
>
> + platform_set_drvdata(pdev, mgr);
> +
> return 0;
> }
>
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> index 1f467173fc1f..9b4ca0ad7466 100644
> --- a/drivers/fpga/socfpga.c
> +++ b/drivers/fpga/socfpga.c
> @@ -571,12 +571,12 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - mgr = devm_fpga_mgr_create(dev, "Altera SOCFPGA FPGA Manager",
> - &socfpga_fpga_ops, priv);
> - if (!mgr)
> - return -ENOMEM;
> + mgr = devm_fpga_mgr_register_simple(dev, "Altera SOCFPGA FPGA Manager",
> + &socfpga_fpga_ops, priv);
> + if (IS_ERR(mgr))
> + return PTR_ERR(mgr);
>
> - return devm_fpga_mgr_register(dev, mgr);
> + return 0;
PTR_ERR_OR_ZERO()
> }
>
> #ifdef CONFIG_OF
> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> index a2cea500f7cc..fb84d88d4ce9 100644
> --- a/drivers/fpga/stratix10-soc.c
> +++ b/drivers/fpga/stratix10-soc.c
> @@ -425,18 +425,11 @@ static int s10_probe(struct platform_device *pdev)
>
> init_completion(&priv->status_return_completion);
>
> - mgr = fpga_mgr_create(dev, "Stratix10 SOC FPGA Manager",
> - &s10_ops, priv);
> - if (!mgr) {
> - dev_err(dev, "unable to create FPGA manager\n");
> - ret = -ENOMEM;
> - goto probe_err;
> - }
> -
> - ret = fpga_mgr_register(mgr);
> - if (ret) {
> + mgr = fpga_mgr_register_simple(dev, "Stratix10 SOC FPGA Manager",
> + &s10_ops, priv);
> + if (IS_ERR(mgr)) {
> dev_err(dev, "unable to register FPGA manager\n");
> - fpga_mgr_free(mgr);
> + ret = PTR_ERR(mgr);
> goto probe_err;
> }
>
> @@ -454,7 +447,6 @@ static int s10_remove(struct platform_device *pdev)
> struct s10_priv *priv = mgr->priv;
>
> fpga_mgr_unregister(mgr);
> - fpga_mgr_free(mgr);
> stratix10_svc_free_channel(priv->chan);
>
> return 0;
> diff --git a/drivers/fpga/ts73xx-fpga.c b/drivers/fpga/ts73xx-fpga.c
> index 101f016c6ed8..2e11eea095ce 100644
> --- a/drivers/fpga/ts73xx-fpga.c
> +++ b/drivers/fpga/ts73xx-fpga.c
> @@ -122,12 +122,12 @@ static int ts73xx_fpga_probe(struct platform_device *pdev)
> if (IS_ERR(priv->io_base))
> return PTR_ERR(priv->io_base);
>
> - mgr = devm_fpga_mgr_create(kdev, "TS-73xx FPGA Manager",
> - &ts73xx_fpga_ops, priv);
> - if (!mgr)
> - return -ENOMEM;
> + mgr = devm_fpga_mgr_register_simple(kdev, "TS-73xx FPGA Manager",
> + &ts73xx_fpga_ops, priv);
> + if (IS_ERR(mgr))
> + return PTR_ERR(mgr);
>
> - return devm_fpga_mgr_register(kdev, mgr);
> + return 0;
PTR_ERR_OR_ZERO()
> }
>
> static struct platform_driver ts73xx_fpga_driver = {
> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
> index fee4d0abf6bf..e78b82f9db79 100644
> --- a/drivers/fpga/xilinx-spi.c
> +++ b/drivers/fpga/xilinx-spi.c
> @@ -247,13 +247,13 @@ static int xilinx_spi_probe(struct spi_device *spi)
> return dev_err_probe(&spi->dev, PTR_ERR(conf->done),
> "Failed to get DONE gpio\n");
>
> - mgr = devm_fpga_mgr_create(&spi->dev,
> - "Xilinx Slave Serial FPGA Manager",
> - &xilinx_spi_ops, conf);
> - if (!mgr)
> - return -ENOMEM;
> + mgr = devm_fpga_mgr_register_simple(&spi->dev,
> + "Xilinx Slave Serial FPGA Manager",
> + &xilinx_spi_ops, conf);
> + if (IS_ERR(mgr))
> + return PTR_ERR(mgr);
>
> - return devm_fpga_mgr_register(&spi->dev, mgr);
> + return 0;
> }
PTR_ERR_OR_ZERO()
>
> static const struct of_device_id xlnx_spi_of_match[] = {
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 9b75bd4f93d8..a3de365aadc6 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -609,20 +609,16 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>
> clk_disable(priv->clk);
>
> - mgr = devm_fpga_mgr_create(dev, "Xilinx Zynq FPGA Manager",
> - &zynq_fpga_ops, priv);
> - if (!mgr)
> - return -ENOMEM;
> -
> - platform_set_drvdata(pdev, mgr);
> -
> - err = fpga_mgr_register(mgr);
> - if (err) {
> + mgr = fpga_mgr_register_simple(dev, "Xilinx Zynq FPGA Manager",
> + &zynq_fpga_ops, priv);
> + if (IS_ERR(mgr)) {
> dev_err(dev, "unable to register FPGA manager\n");
> clk_unprepare(priv->clk);
> - return err;
> + return PTR_ERR(mgr);
> }
>
> + platform_set_drvdata(pdev, mgr);
> +
> return 0;
> }
>
> diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
> index 125743c9797f..d743a05cba95 100644
> --- a/drivers/fpga/zynqmp-fpga.c
> +++ b/drivers/fpga/zynqmp-fpga.c
> @@ -102,12 +102,12 @@ static int zynqmp_fpga_probe(struct platform_device *pdev)
>
> priv->dev = dev;
>
> - mgr = devm_fpga_mgr_create(dev, "Xilinx ZynqMP FPGA Manager",
> - &zynqmp_fpga_ops, priv);
> - if (!mgr)
> - return -ENOMEM;
> + mgr = devm_fpga_mgr_register_simple(dev, "Xilinx ZynqMP FPGA Manager",
> + &zynqmp_fpga_ops, priv);
> + if (IS_ERR(mgr))
> + return PTR_ERR(mgr);
>
> - return devm_fpga_mgr_register(dev, mgr);
> + return 0;
PTR_ERR_OR_ZERO()
> }
>
> static const struct of_device_id zynqmp_fpga_of_match[] = {
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 474c1f506307..a814407bc588 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -105,6 +105,36 @@ struct fpga_image_info {
> #endif
> };
>
> +/**
> + * struct fpga_compat_id - id for compatibility check
> + *
> + * @id_h: high 64bit of the compat_id
> + * @id_l: low 64bit of the compat_id
> + */
> +struct fpga_compat_id {
> + u64 id_h;
> + u64 id_l;
> +};
> +
> +/**
> + * struct fpga_manager_info - collection of parameters for an FPGA Manager
> + * @name: fpga manager name
> + * @compat_id: FPGA manager id for compatibility check.
> + * @mops: pointer to structure of fpga manager ops
> + * @priv: fpga manager private data
> + *
> + * fpga_manager_info contains parameters for the register function. These
> + * are separated into an info structure because they some are optional
> + * others could be added to in the future. The info structure facilitates
> + * maintaining a stable API.
> + */
> +struct fpga_manager_info {
> + const char *name;
> + struct fpga_compat_id *compat_id;
> + const struct fpga_manager_ops *mops;
> + void *priv;
> +};
> +
> /**
> * struct fpga_manager_ops - ops for low level fpga manager drivers
> * @initial_header_size: Maximum number of bytes that should be passed into write_init
> @@ -143,17 +173,6 @@ struct fpga_manager_ops {
> #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR BIT(3)
> #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR BIT(4)
>
> -/**
> - * struct fpga_compat_id - id for compatibility check
> - *
> - * @id_h: high 64bit of the compat_id
> - * @id_l: low 64bit of the compat_id
> - */
> -struct fpga_compat_id {
> - u64 id_h;
> - u64 id_l;
> -};
> -
> /**
> * struct fpga_manager - fpga manager structure
> * @name: name of low level fpga manager
> @@ -191,17 +210,18 @@ struct fpga_manager *fpga_mgr_get(struct device *dev);
>
> void fpga_mgr_put(struct fpga_manager *mgr);
>
> -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);
> +struct fpga_manager *
> +fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
const struct fpga_manager_info
>
> -int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr);
> +struct fpga_manager *
> +fpga_mgr_register_simple(struct device *dev, const char *name,
> + const struct fpga_manager_ops *mops, void *priv);
> +void fpga_mgr_unregister(struct fpga_manager *mgr);
>
> -struct fpga_manager *devm_fpga_mgr_create(struct device *dev, const char *name,
> - const struct fpga_manager_ops *mops,
> - void *priv);
> +struct fpga_manager *
> +devm_fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
const struct fpga_manager_info
> +struct fpga_manager *
> +devm_fpga_mgr_register_simple(struct device *dev, const char *name,
> + const struct fpga_manager_ops *mops, void *priv);
>
> #endif /*_LINUX_FPGA_MGR_H */
> --
> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/3] fpga: bridge: Use standard dev_release for class driver
2021-06-21 22:22 ` [PATCH v6 2/3] fpga: bridge: " Russ Weight
@ 2021-06-22 8:02 ` Xu Yilun
2021-06-22 23:04 ` Russ Weight
2021-06-22 8:23 ` Xu Yilun
1 sibling, 1 reply; 14+ messages in thread
From: Xu Yilun @ 2021-06-22 8:02 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
On Mon, Jun 21, 2021 at 03:22:48PM -0700, Russ Weight wrote:
> The FPGA bridge class driver data structure is being treated as a
> managed resource instead of using standard dev_release call-back
> to release the class data structure. This change removes the
> managed resource code and combines the create() and register()
> functions into a single register() function.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v6:
> - Changed fpga_bridge_register() parameters to accept an info data
> structure to provide flexibility in passing optional parameters.
> - Added fpga_bridge_register_simple() function to support current
> parameters for users that don't require the use of optional
> parameters.
> v5:
> - Rebased on top of recently accepted patches.
> v4:
> - Restore the previous format for the Return value in the comment header
> for fpga_bridge_register()
> v3:
> - Cleaned up comment header for fpga_bridge_register()
> - Fix error return values for fpga_bridge_register()
> v2:
> - No changes
> drivers/fpga/altera-fpga2sdram.c | 12 +--
> drivers/fpga/altera-freeze-bridge.c | 10 +--
> drivers/fpga/altera-hps2fpga.c | 12 +--
> drivers/fpga/dfl-fme-br.c | 10 +--
> drivers/fpga/fpga-bridge.c | 133 +++++++++-------------------
> drivers/fpga/xilinx-pr-decoupler.c | 17 ++--
> include/linux/fpga/fpga-bridge.h | 33 +++++--
> 7 files changed, 91 insertions(+), 136 deletions(-)
>
> diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
> index a78e49c63c64..e165440ebbab 100644
> --- a/drivers/fpga/altera-fpga2sdram.c
> +++ b/drivers/fpga/altera-fpga2sdram.c
> @@ -121,17 +121,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);
>
> - br = devm_fpga_bridge_create(dev, F2S_BRIDGE_NAME,
> - &altera_fpga2sdram_br_ops, priv);
> - if (!br)
> - return -ENOMEM;
> + br = fpga_bridge_register_simple(dev, F2S_BRIDGE_NAME,
> + &altera_fpga2sdram_br_ops, priv);
> + if (IS_ERR(br))
> + return PTR_ERR(mgr);
>
> platform_set_drvdata(pdev, br);
>
> - ret = fpga_bridge_register(br);
> - if (ret)
> - return ret;
> -
> dev_info(dev, "driver initialized with handoff %08x\n", priv->mask);
>
> if (!of_property_read_u32(dev->of_node, "bridge-enable", &enable)) {
> diff --git a/drivers/fpga/altera-freeze-bridge.c b/drivers/fpga/altera-freeze-bridge.c
> index dd58c4aea92e..4e39b5475630 100644
> --- a/drivers/fpga/altera-freeze-bridge.c
> +++ b/drivers/fpga/altera-freeze-bridge.c
> @@ -244,14 +244,14 @@ static int altera_freeze_br_probe(struct platform_device *pdev)
>
> priv->base_addr = base_addr;
>
> - br = devm_fpga_bridge_create(dev, FREEZE_BRIDGE_NAME,
> - &altera_freeze_br_br_ops, priv);
> - if (!br)
> - return -ENOMEM;
> + br = fpga_bridge_register_simple(dev, FREEZE_BRIDGE_NAME,
> + &altera_freeze_br_br_ops, priv);
> + if (IS_ERR(br))
> + return PTR_ERR(br);
>
> platform_set_drvdata(pdev, br);
>
> - return fpga_bridge_register(br);
> + return 0;
> }
>
> static int altera_freeze_br_remove(struct platform_device *pdev)
> diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
> index 77b95f251821..a564eb29349c 100644
> --- a/drivers/fpga/altera-hps2fpga.c
> +++ b/drivers/fpga/altera-hps2fpga.c
> @@ -180,19 +180,15 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
> }
> }
>
> - br = devm_fpga_bridge_create(dev, priv->name,
> - &altera_hps2fpga_br_ops, priv);
> - if (!br) {
> - ret = -ENOMEM;
> + br = fpga_bridge_register_simple(dev, priv->name,
> + &altera_hps2fpga_br_ops, priv);
> + if (IS_ERR(br)) {
> + ret = PTR_ERR(br);
> goto err;
> }
>
> platform_set_drvdata(pdev, br);
>
> - ret = fpga_bridge_register(br);
> - if (ret)
> - goto err;
> -
> return 0;
>
> err:
> diff --git a/drivers/fpga/dfl-fme-br.c b/drivers/fpga/dfl-fme-br.c
> index 3ff9f3a687ce..0ad39e502142 100644
> --- a/drivers/fpga/dfl-fme-br.c
> +++ b/drivers/fpga/dfl-fme-br.c
> @@ -68,14 +68,14 @@ static int fme_br_probe(struct platform_device *pdev)
>
> priv->pdata = dev_get_platdata(dev);
>
> - br = devm_fpga_bridge_create(dev, "DFL FPGA FME Bridge",
> - &fme_bridge_ops, priv);
> - if (!br)
> - return -ENOMEM;
> + br = fpga_bridge_register_simple(dev, "DFL FPGA FME Bridge",
> + &fme_bridge_ops, priv);
> + if (IS_ERR(br))
> + return PTR_ERR(br);
>
> platform_set_drvdata(pdev, br);
>
> - return fpga_bridge_register(br);
> + return 0;
> }
>
> static int fme_br_remove(struct platform_device *pdev)
> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> index 798f55670646..1436acfc1e7d 100644
> --- a/drivers/fpga/fpga-bridge.c
> +++ b/drivers/fpga/fpga-bridge.c
> @@ -312,55 +312,57 @@ static struct attribute *fpga_bridge_attrs[] = {
> ATTRIBUTE_GROUPS(fpga_bridge);
>
> /**
> - * fpga_bridge_create - create and initialize a struct fpga_bridge
> + * fpga_bridge_register - create and register an FPGA Bridge device
> * @parent: FPGA bridge device from pdev
> - * @name: FPGA bridge name
> - * @br_ops: pointer to structure of fpga bridge ops
> - * @priv: FPGA bridge private data
> + * @info: parameters for FPGA Bridge
> *
> - * The caller of this function is responsible for freeing the bridge with
> - * fpga_bridge_free(). Using devm_fpga_bridge_create() instead is recommended.
Mm.. The comments are removed. Do we need a devm version of
fpga_bridge_register? Or leave it in later patches?
> - *
> - * Return: struct fpga_bridge or NULL
> + * Return: struct fpga_bridge pointer or ERR_PTR()
> */
> -struct fpga_bridge *fpga_bridge_create(struct device *parent, const char *name,
> - const struct fpga_bridge_ops *br_ops,
> - void *priv)
> +struct fpga_bridge *fpga_bridge_register(struct device *parent,
> + struct fpga_bridge_info *info)
const struct fpga_bridge_info
> {
> struct fpga_bridge *bridge;
> int id, ret;
>
> - if (!name || !strlen(name)) {
> + if (!info->name || !strlen(info->name)) {
> dev_err(parent, "Attempt to register with no name!\n");
> - return NULL;
> + return ERR_PTR(-EINVAL);
> }
Is it necessary to add if(!info->br_ops) check, the field is mandatory for
fpga_bridge.
>
> bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> if (!bridge)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> id = ida_simple_get(&fpga_bridge_ida, 0, 0, GFP_KERNEL);
> - if (id < 0)
> + if (id < 0) {
> + ret = id;
> goto error_kfree;
> + }
>
> mutex_init(&bridge->mutex);
> INIT_LIST_HEAD(&bridge->node);
>
> - bridge->name = name;
> - bridge->br_ops = br_ops;
> - bridge->priv = priv;
> + bridge->name = info->name;
> + bridge->br_ops = info->br_ops;
> + bridge->priv = info->priv;
>
> - device_initialize(&bridge->dev);
> - bridge->dev.groups = br_ops->groups;
> + bridge->dev.groups = info->br_ops->groups;
> bridge->dev.class = fpga_bridge_class;
> bridge->dev.parent = parent;
> bridge->dev.of_node = parent->of_node;
> bridge->dev.id = id;
> + of_platform_populate(bridge->dev.of_node, NULL, NULL, &bridge->dev);
>
> ret = dev_set_name(&bridge->dev, "br%d", id);
> if (ret)
> goto error_device;
>
> + ret = device_register(&bridge->dev);
> + if (ret) {
> + put_device(&bridge->dev);
> + return ERR_PTR(ret);
> + }
> +
> return bridge;
>
> error_device:
> @@ -368,90 +370,37 @@ struct fpga_bridge *fpga_bridge_create(struct device *parent, const char *name,
> error_kfree:
> kfree(bridge);
>
> - return NULL;
> -}
> -EXPORT_SYMBOL_GPL(fpga_bridge_create);
> -
> -/**
> - * fpga_bridge_free - free an fpga bridge created by fpga_bridge_create()
> - * @bridge: FPGA bridge struct
> - */
> -void fpga_bridge_free(struct fpga_bridge *bridge)
> -{
> - ida_simple_remove(&fpga_bridge_ida, bridge->dev.id);
> - kfree(bridge);
> -}
> -EXPORT_SYMBOL_GPL(fpga_bridge_free);
> -
> -static void devm_fpga_bridge_release(struct device *dev, void *res)
> -{
> - struct fpga_bridge *bridge = *(struct fpga_bridge **)res;
> -
> - fpga_bridge_free(bridge);
> + return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL_GPL(fpga_bridge_register);
>
> /**
> - * devm_fpga_bridge_create - create and init a managed struct fpga_bridge
> + * fpga_bridge_register_simple - create and register an FPGA Bridge device
> * @parent: FPGA bridge device from pdev
> * @name: FPGA bridge name
> * @br_ops: pointer to structure of fpga bridge ops
> * @priv: FPGA bridge private data
> *
> - * This function is intended for use in an FPGA bridge driver's probe function.
> - * After the bridge driver creates the struct with devm_fpga_bridge_create(), it
> - * should register the bridge with fpga_bridge_register(). The bridge driver's
> - * remove function should call fpga_bridge_unregister(). The bridge struct
> - * allocated with this function will be freed automatically on driver detach.
> - * This includes the case of a probe function returning error before calling
> - * fpga_bridge_register(), the struct will still get cleaned up.
> - *
> - * Return: struct fpga_bridge or NULL
> - */
> -struct fpga_bridge
> -*devm_fpga_bridge_create(struct device *parent, const char *name,
> - const struct fpga_bridge_ops *br_ops, void *priv)
> -{
> - struct fpga_bridge **ptr, *bridge;
> -
> - ptr = devres_alloc(devm_fpga_bridge_release, sizeof(*ptr), GFP_KERNEL);
> - if (!ptr)
> - return NULL;
> -
> - bridge = fpga_bridge_create(parent, name, br_ops, priv);
> - if (!bridge) {
> - devres_free(ptr);
> - } else {
> - *ptr = bridge;
> - devres_add(parent, ptr);
> - }
> -
> - return bridge;
> -}
> -EXPORT_SYMBOL_GPL(devm_fpga_bridge_create);
> -
> -/**
> - * fpga_bridge_register - register an FPGA bridge
> + * This simple version of the register should be sufficient for most users.
> + * The fpga_mgr_register function is available for users that need to pass
fpga_bridge_register
> + * additional, optional parameters.
> *
> - * @bridge: FPGA bridge struct
> - *
> - * Return: 0 for success, error code otherwise.
> + * Return: struct fpga_bridge pointer or ERR_PTR()
> */
> -int fpga_bridge_register(struct fpga_bridge *bridge)
> +struct fpga_bridge *
> +fpga_bridge_register_simple(struct device *parent, const char *name,
> + const struct fpga_bridge_ops *br_ops,
> + void *priv)
> {
> - struct device *dev = &bridge->dev;
> - int ret;
> -
> - ret = device_add(dev);
> - if (ret)
> - return ret;
> + struct fpga_bridge_info info = { 0 };
>
> - of_platform_populate(dev->of_node, NULL, NULL, dev);
> + info.name = name;
> + info.br_ops = br_ops;
> + info.priv = priv;
>
> - dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name);
> -
> - return 0;
> + return fpga_bridge_register(parent, &info);
> }
> -EXPORT_SYMBOL_GPL(fpga_bridge_register);
> +EXPORT_SYMBOL_GPL(fpga_bridge_register_simple);
>
> /**
> * fpga_bridge_unregister - unregister an FPGA bridge
> @@ -475,6 +424,10 @@ EXPORT_SYMBOL_GPL(fpga_bridge_unregister);
>
> 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 ea2bde6e5bc4..24bc16a86091 100644
> --- a/drivers/fpga/xilinx-pr-decoupler.c
> +++ b/drivers/fpga/xilinx-pr-decoupler.c
> @@ -138,22 +138,17 @@ static int xlnx_pr_decoupler_probe(struct platform_device *pdev)
>
> clk_disable(priv->clk);
>
> - br = devm_fpga_bridge_create(&pdev->dev, priv->ipconfig->name,
> - &xlnx_pr_decoupler_br_ops, priv);
> - if (!br) {
> - err = -ENOMEM;
> - goto err_clk;
> - }
> -
> - platform_set_drvdata(pdev, br);
> -
> - err = fpga_bridge_register(br);
> - if (err) {
> + br = fpga_bridge_register_simple(&pdev->dev, priv->ipconfig->name,
> + &xlnx_pr_decoupler_br_ops, priv);
> + if (IS_ERR(br)) {
> + err = PTR_ERR(br);
> dev_err(&pdev->dev, "unable to register %s",
> priv->ipconfig->name);
> goto err_clk;
> }
>
> + platform_set_drvdata(pdev, br);
> +
> return 0;
>
> err_clk:
> diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
> index 6c3c28806ff1..e111698012f0 100644
> --- a/include/linux/fpga/fpga-bridge.h
> +++ b/include/linux/fpga/fpga-bridge.h
> @@ -22,6 +22,23 @@ struct fpga_bridge_ops {
> const struct attribute_group **groups;
> };
>
> +/**
> + * struct fpga_bridge_info - collection of parameters an FPGA Bridge
> + * @name: fpga bridge name
> + * @br_ops: pointer to structure of fpga bridge ops
> + * @priv: fpga bridge private data
> + *
> + * fpga_bridge_info contains parameters for the register function. These
> + * are separated into an info structure because they some are optional
> + * others could be added to in the future. The info structure facilitates
> + * maintaining a stable API.
> + */
> +struct fpga_bridge_info {
> + const char *name;
> + const struct fpga_bridge_ops *br_ops;
> + void *priv;
> +};
> +
> /**
> * struct fpga_bridge - FPGA bridge structure
> * @name: name of low level FPGA bridge
> @@ -62,15 +79,13 @@ int of_fpga_bridge_get_to_list(struct device_node *np,
> struct fpga_image_info *info,
> struct list_head *bridge_list);
>
> -struct fpga_bridge *fpga_bridge_create(struct device *dev, const char *name,
> - const struct fpga_bridge_ops *br_ops,
> - void *priv);
> -void fpga_bridge_free(struct fpga_bridge *br);
> -int fpga_bridge_register(struct fpga_bridge *br);
> -void fpga_bridge_unregister(struct fpga_bridge *br);
> +struct fpga_bridge *
> +fpga_bridge_register(struct device *dev, struct fpga_bridge_info *info);
struct device *parent, const struct fpga_bridge_info *info
>
> -struct fpga_bridge
> -*devm_fpga_bridge_create(struct device *dev, const char *name,
> - const struct fpga_bridge_ops *br_ops, void *priv);
> +struct fpga_bridge *
> +fpga_bridge_register_simple(struct device *dev, const char *name,
> + const struct fpga_bridge_ops *br_ops,
> + void *priv);
> +void fpga_bridge_unregister(struct fpga_bridge *br);
>
> #endif /* _LINUX_FPGA_BRIDGE_H */
> --
> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 3/3] fpga: region: Use standard dev_release for class driver
2021-06-21 22:22 ` [PATCH v6 3/3] fpga: region: " Russ Weight
@ 2021-06-22 8:19 ` Xu Yilun
2021-06-22 23:08 ` Russ Weight
0 siblings, 1 reply; 14+ messages in thread
From: Xu Yilun @ 2021-06-22 8:19 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
On Mon, Jun 21, 2021 at 03:22:49PM -0700, Russ Weight wrote:
> The FPGA region class driver data structure is being treated as a
> managed resource instead of using standard dev_release call-back
> to release the class data structure. This change removes the
> managed resource code and combines the create() and register()
> functions into a single register() function.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v6:
> - Moved FPGA manager optional parameters out of the ops structure and
> back into the FPGA Region structure.
> - Changed fpga_region_register() parameters to accept an info data
> structure to provide flexibility in passing optional parameters.
> - Added fpga_region_register_simple() functions to support current
> parameters for users that don't require the use of optional parameters.
Add the _simple() description in commit message.
> v5:
> - Rebased on top of recently accepted patches.
> - Created the fpga_region_ops data structure which is optionally passed
> to fpga_region_register(). compat_id, the get_bridges() pointer, and
> the priv pointer are included in the fpga_region_ops structure.
> v4:
> - Added the compat_id parameter to fpga_region_register() to ensure
> that the compat_id is set before the device_register() call.
> - Modified the dfl_fpga_feature_devs_enumerate() function to restore
> the fpga_region_register() call to the correct location.
> v3:
> - Cleaned up comment header for fpga_region_register()
> - Fix fpga_region_register() error return on ida_simple_get() failure
> v2:
> - No changes
> ---
> drivers/fpga/dfl-fme-region.c | 17 +++--
> drivers/fpga/dfl.c | 12 ++--
> drivers/fpga/fpga-region.c | 115 +++++++++++--------------------
> drivers/fpga/of-fpga-region.c | 11 ++-
> include/linux/fpga/fpga-region.h | 36 +++++++---
> 5 files changed, 82 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
> index 1eeb42af1012..36bf6865bbc3 100644
> --- a/drivers/fpga/dfl-fme-region.c
> +++ b/drivers/fpga/dfl-fme-region.c
> @@ -30,6 +30,7 @@ static int fme_region_get_bridges(struct fpga_region *region)
> static int fme_region_probe(struct platform_device *pdev)
> {
> struct dfl_fme_region_pdata *pdata = dev_get_platdata(&pdev->dev);
> + struct fpga_region_info info = { 0 };
> struct device *dev = &pdev->dev;
> struct fpga_region *region;
> struct fpga_manager *mgr;
> @@ -39,20 +40,18 @@ static int fme_region_probe(struct platform_device *pdev)
> if (IS_ERR(mgr))
> return -EPROBE_DEFER;
>
> - region = devm_fpga_region_create(dev, mgr, fme_region_get_bridges);
> - if (!region) {
> - ret = -ENOMEM;
> + info.mgr = mgr;
> + info.compat_id = mgr->compat_id;
> + info.get_bridges = fme_region_get_bridges;
> + info.priv = pdata;
> + region = fpga_region_register(dev, &info);
> + if (IS_ERR(region)) {
> + ret = PTR_ERR(region);
> goto eprobe_mgr_put;
> }
>
> - region->priv = pdata;
> - region->compat_id = mgr->compat_id;
> platform_set_drvdata(pdev, region);
>
> - ret = fpga_region_register(region);
> - if (ret)
> - goto eprobe_mgr_put;
> -
> dev_dbg(dev, "DFL FME FPGA Region probed\n");
>
> return 0;
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 511b20ff35a3..a4269d40586e 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -1400,19 +1400,15 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
> if (!cdev)
> return ERR_PTR(-ENOMEM);
>
> - cdev->region = devm_fpga_region_create(info->dev, NULL, NULL);
> - if (!cdev->region) {
> - ret = -ENOMEM;
> - goto free_cdev_exit;
> - }
> -
> cdev->parent = info->dev;
> mutex_init(&cdev->lock);
> INIT_LIST_HEAD(&cdev->port_dev_list);
>
> - ret = fpga_region_register(cdev->region);
> - if (ret)
> + cdev->region = fpga_region_register_simple(info->dev, NULL, NULL);
> + if (IS_ERR(cdev->region)) {
> + ret = PTR_ERR(cdev->region);
> goto free_cdev_exit;
> + }
>
> /* create and init build info for enumeration */
> binfo = devm_kzalloc(info->dev, sizeof(*binfo), GFP_KERNEL);
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index a4838715221f..0d831b85a98f 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -180,39 +180,36 @@ static struct attribute *fpga_region_attrs[] = {
> ATTRIBUTE_GROUPS(fpga_region);
>
> /**
> - * fpga_region_create - alloc and init a struct fpga_region
> + * fpga_region_register - create and register an FPGA Region device
> * @parent: device parent
> - * @mgr: manager that programs this region
> - * @get_bridges: optional function to get bridges to a list
> - *
> - * The caller of this function is responsible for freeing the resulting region
> - * struct with fpga_region_free(). Using devm_fpga_region_create() instead is
> - * recommended.
Same question, introduce a devm_fpga_region_register or leave it later?
> + * @info: parameters for FPGA Region
> *
> - * Return: struct fpga_region or NULL
> + * Return: struct fpga_region or ERR_PTR()
> */
> -struct fpga_region
> -*fpga_region_create(struct device *parent,
> - struct fpga_manager *mgr,
> - int (*get_bridges)(struct fpga_region *))
> +struct fpga_region *
> +fpga_region_register(struct device *parent, struct fpga_region_info *info)
const struct fpga_region_info
> {
> struct fpga_region *region;
> int id, ret = 0;
>
> region = kzalloc(sizeof(*region), GFP_KERNEL);
> if (!region)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
> - if (id < 0)
> + if (id < 0) {
> + ret = id;
> goto err_free;
> + }
> +
> + region->mgr = info->mgr;
> + region->compat_id = info->compat_id;
> + region->priv = info->priv;
> + region->get_bridges = info->get_bridges;
>
> - region->mgr = mgr;
> - region->get_bridges = get_bridges;
> mutex_init(®ion->mutex);
> INIT_LIST_HEAD(®ion->bridge_list);
>
> - device_initialize(®ion->dev);
> region->dev.class = fpga_region_class;
> region->dev.parent = parent;
> region->dev.of_node = parent->of_node;
> @@ -222,6 +219,12 @@ struct fpga_region
> if (ret)
> goto err_remove;
>
> + ret = device_register(®ion->dev);
> + if (ret) {
> + put_device(®ion->dev);
> + return ERR_PTR(ret);
> + }
> +
> return region;
>
> err_remove:
> @@ -229,78 +232,34 @@ struct fpga_region
> err_free:
> kfree(region);
>
> - return NULL;
> -}
> -EXPORT_SYMBOL_GPL(fpga_region_create);
> -
> -/**
> - * fpga_region_free - free an FPGA region created by fpga_region_create()
> - * @region: FPGA region
> - */
> -void fpga_region_free(struct fpga_region *region)
> -{
> - ida_simple_remove(&fpga_region_ida, region->dev.id);
> - kfree(region);
> -}
> -EXPORT_SYMBOL_GPL(fpga_region_free);
> -
> -static void devm_fpga_region_release(struct device *dev, void *res)
> -{
> - struct fpga_region *region = *(struct fpga_region **)res;
> -
> - fpga_region_free(region);
> + return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL_GPL(fpga_region_register);
>
> /**
> - * devm_fpga_region_create - create and initialize a managed FPGA region struct
> + * fpga_region_register_simple - create and register an FPGA Region device
> * @parent: device parent
> * @mgr: manager that programs this region
> * @get_bridges: optional function to get bridges to a list
> *
> - * This function is intended for use in an FPGA region driver's probe function.
> - * After the region driver creates the region struct with
> - * devm_fpga_region_create(), it should register it with fpga_region_register().
> - * The region driver's remove function should call fpga_region_unregister().
> - * The region struct allocated with this function will be freed automatically on
> - * driver detach. This includes the case of a probe function returning error
> - * before calling fpga_region_register(), the struct will still get cleaned up.
> + * This simple version of the register should be sufficient for most users.
> + * The fpga_bridge_register function is available for users that need to
fpga_region_register()
> + * pass additional, optional parameters.
> *
> - * Return: struct fpga_region or NULL
> + * Return: struct fpga_region or ERR_PTR()
> */
> -struct fpga_region
> -*devm_fpga_region_create(struct device *parent,
> - struct fpga_manager *mgr,
> - int (*get_bridges)(struct fpga_region *))
> +struct fpga_region *
> +fpga_region_register_simple(struct device *parent, struct fpga_manager *mgr,
> + int (*get_bridges)(struct fpga_region *))
> {
> - struct fpga_region **ptr, *region;
> -
> - ptr = devres_alloc(devm_fpga_region_release, sizeof(*ptr), GFP_KERNEL);
> - if (!ptr)
> - return NULL;
> + struct fpga_region_info info = { 0 };
>
> - region = fpga_region_create(parent, mgr, get_bridges);
> - if (!region) {
> - devres_free(ptr);
> - } else {
> - *ptr = region;
> - devres_add(parent, ptr);
> - }
> + info.mgr = mgr;
> + info.get_bridges = get_bridges;
>
> - return region;
> + return fpga_region_register(parent, &info);
> }
> -EXPORT_SYMBOL_GPL(devm_fpga_region_create);
> -
> -/**
> - * fpga_region_register - register an FPGA region
> - * @region: FPGA region
> - *
> - * Return: 0 or -errno
> - */
> -int fpga_region_register(struct fpga_region *region)
> -{
> - return device_add(®ion->dev);
> -}
> -EXPORT_SYMBOL_GPL(fpga_region_register);
> +EXPORT_SYMBOL_GPL(fpga_region_register_simple);
>
> /**
> * fpga_region_unregister - unregister an FPGA region
> @@ -316,6 +275,10 @@ EXPORT_SYMBOL_GPL(fpga_region_unregister);
>
> static void fpga_region_dev_release(struct device *dev)
> {
> + struct fpga_region *region = to_fpga_region(dev);
> +
> + ida_simple_remove(&fpga_region_ida, region->dev.id);
> + kfree(region);
> }
>
> /**
> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
> index e3c25576b6b9..377d19585d8e 100644
> --- a/drivers/fpga/of-fpga-region.c
> +++ b/drivers/fpga/of-fpga-region.c
> @@ -405,16 +405,13 @@ static int of_fpga_region_probe(struct platform_device *pdev)
> if (IS_ERR(mgr))
> return -EPROBE_DEFER;
>
> - region = devm_fpga_region_create(dev, mgr, of_fpga_region_get_bridges);
> - if (!region) {
> - ret = -ENOMEM;
> + region = fpga_region_register_simple(dev, mgr,
> + of_fpga_region_get_bridges);
> + if (IS_ERR(region)) {
> + ret = PTR_ERR(region);
> goto eprobe_mgr_put;
> }
>
> - ret = fpga_region_register(region);
> - if (ret)
> - goto eprobe_mgr_put;
> -
> of_platform_populate(np, fpga_region_of_match, NULL, ®ion->dev);
> platform_set_drvdata(pdev, region);
>
> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> index 27cb706275db..7b8f76d52c4e 100644
> --- a/include/linux/fpga/fpga-region.h
> +++ b/include/linux/fpga/fpga-region.h
> @@ -7,6 +7,27 @@
> #include <linux/fpga/fpga-mgr.h>
> #include <linux/fpga/fpga-bridge.h>
>
> +struct fpga_region;
> +
> +/**
> + * struct fpga_region_info - collection of parameters an FPGA Region
> + * @mgr: fpga region manager
> + * @compat_id: FPGA region id for compatibility check.
> + * @priv: fpga region private data
> + * @get_bridges: optional function to get bridges to a list
> + *
> + * fpga_region_info contains parameters for the register function. These
> + * are separated into an info structure because they some are optional
> + * others could be added to in the future. The info structure facilitates
> + * maintaining a stable API.
> + */
> +struct fpga_region_info {
> + struct fpga_manager *mgr;
> + struct fpga_compat_id *compat_id;
> + void *priv;
> + int (*get_bridges)(struct fpga_region *region);
> +};
> +
> /**
> * struct fpga_region - FPGA Region structure
> * @dev: FPGA Region device
> @@ -37,15 +58,12 @@ struct fpga_region *fpga_region_class_find(
>
> int fpga_region_program_fpga(struct fpga_region *region);
>
> -struct fpga_region
> -*fpga_region_create(struct device *dev, struct fpga_manager *mgr,
> - int (*get_bridges)(struct fpga_region *));
> -void fpga_region_free(struct fpga_region *region);
> -int fpga_region_register(struct fpga_region *region);
> -void fpga_region_unregister(struct fpga_region *region);
> +struct fpga_region *
> +fpga_region_register(struct device *dev, struct fpga_region_info *info);
struct device *parent, const struct fpga_region_info
>
> -struct fpga_region
> -*devm_fpga_region_create(struct device *dev, struct fpga_manager *mgr,
> - int (*get_bridges)(struct fpga_region *));
> +struct fpga_region *
> +fpga_region_register_simple(struct device *dev, struct fpga_manager *mgr,
struct device *parent
> + int (*get_bridges)(struct fpga_region *));
> +void fpga_region_unregister(struct fpga_region *region);
>
> #endif /* _FPGA_REGION_H */
> --
> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/3] fpga: bridge: Use standard dev_release for class driver
2021-06-21 22:22 ` [PATCH v6 2/3] fpga: bridge: " Russ Weight
2021-06-22 8:02 ` Xu Yilun
@ 2021-06-22 8:23 ` Xu Yilun
2021-06-22 23:05 ` Russ Weight
1 sibling, 1 reply; 14+ messages in thread
From: Xu Yilun @ 2021-06-22 8:23 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
2 more comments.
On Mon, Jun 21, 2021 at 03:22:48PM -0700, Russ Weight wrote:
> The FPGA bridge class driver data structure is being treated as a
> managed resource instead of using standard dev_release call-back
> to release the class data structure. This change removes the
> managed resource code and combines the create() and register()
> functions into a single register() function.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v6:
> - Changed fpga_bridge_register() parameters to accept an info data
> structure to provide flexibility in passing optional parameters.
> - Added fpga_bridge_register_simple() function to support current
> parameters for users that don't require the use of optional
> parameters.
Add the _simple() description in commit message if needed.
> +struct fpga_bridge *
> +fpga_bridge_register_simple(struct device *dev, const char *name,
struct device *parent,
> + const struct fpga_bridge_ops *br_ops,
> + void *priv);
> +void fpga_bridge_unregister(struct fpga_bridge *br);
>
> #endif /* _LINUX_FPGA_BRIDGE_H */
> --
> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver
2021-06-21 22:22 ` [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver Russ Weight
2021-06-22 7:32 ` Xu Yilun
@ 2021-06-22 8:45 ` Xu Yilun
2021-06-22 22:41 ` Russ Weight
1 sibling, 1 reply; 14+ messages in thread
From: Xu Yilun @ 2021-06-22 8:45 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
Some more comments.
On Mon, Jun 21, 2021 at 03:22:47PM -0700, Russ Weight wrote:
> +struct fpga_manager *
> +fpga_mgr_register(struct device *parent, struct fpga_manager_info *info)
> {
> + const struct fpga_manager_ops *mops = info->mops;
> struct fpga_manager *mgr;
> int id, ret;
>
> @@ -572,29 +570,31 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
Shall we add the check?
if (!mops || ...)
> !mops->write_init || (!mops->write && !mops->write_sg) ||
> (mops->write && mops->write_sg)) {
> dev_err(parent, "Attempt to register without fpga_manager_ops\n");
> - return NULL;
> + return ERR_PTR(-EINVAL);
> }
>
>
[...]
> +struct fpga_manager *
> +fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
struct device *parent, const struct fpga_manager_info
>
> -int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr);
> +struct fpga_manager *
> +fpga_mgr_register_simple(struct device *dev, const char *name,
struct device *parent,
> + const struct fpga_manager_ops *mops, void *priv);
> +void fpga_mgr_unregister(struct fpga_manager *mgr);
>
> -struct fpga_manager *devm_fpga_mgr_create(struct device *dev, const char *name,
> - const struct fpga_manager_ops *mops,
> - void *priv);
> +struct fpga_manager *
> +devm_fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
struct device *parent, const struct fpga_manager_info
> +struct fpga_manager *
> +devm_fpga_mgr_register_simple(struct device *dev, const char *name,
struct device *parent,
> + const struct fpga_manager_ops *mops, void *priv);
>
> #endif /*_LINUX_FPGA_MGR_H */
> --
> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver
2021-06-22 7:32 ` Xu Yilun
@ 2021-06-22 22:32 ` Russ Weight
0 siblings, 0 replies; 14+ messages in thread
From: Russ Weight @ 2021-06-22 22:32 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
On 6/22/21 12:32 AM, Xu Yilun wrote:
> Good to me overall, minor fixes see inline.
>
> On Mon, Jun 21, 2021 at 03:22:47PM -0700, Russ Weight wrote:
>> The FPGA manager class driver data structure is being treated as a
>> managed resource instead of using standard dev_release call-back
>> to release the class data structure. This change removes the
>> managed resource code for the freeing of the class data structure
>> and combines the create() and register() functions into a single
>> register() function.
>>
>> The devm_fpga_mgr_register() function is retained.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v6:
>> - Moved FPGA manager optional parameters out of the ops structure and
>> back into the FPGA manager structure.
>> - Changed fpga_mgr_register()/devm_fpga_mgr_register() parameters to
>> accept an info data structure to provide flexibility in passing optional
>> parameters.
>> - Added fpga_mgr_register_simple()/devm_fpga_mgr_register_simple()
>> functions to support current parameters for users that don't require
>> the use of optional parameters.
> These description should be added to the commit message if the idea is
> to be accepted.
I'll update the commit messages for the next version.
>
>> v5:
>> - Rebased on top of recently accepted patches.
>> - Removed compat_id from the fpga_mgr_register() parameter list
>> and added it to the fpga_manager_ops structure. This also required
>> dynamically allocating the dfl-fme-ops structure in order to add
>> the appropriate compat_id.
>> v4:
>> - Added the compat_id parameter to fpga_mgr_register() and
>> devm_fpga_mgr_register() to ensure that the compat_id is set before
>> the device_register() call.
>> v3:
>> - Cleaned up comment header for fpga_mgr_register()
>> - Fix error return on ida_simple_get() failure
>> v2:
>> - Restored devm_fpga_mgr_register() functionality, adapted for the combined
>> create/register functionality.
>> - All previous callers of devm_fpga_mgr_register() will continue to call
>> devm_fpga_mgr_register().
>> - replaced unnecessary ternary operators in return statements with standard
>> if conditions.
>> ---
>> drivers/fpga/altera-cvp.c | 12 +-
>> drivers/fpga/altera-pr-ip-core.c | 9 +-
>> drivers/fpga/altera-ps-spi.c | 10 +-
>> drivers/fpga/dfl-fme-mgr.c | 23 ++--
>> drivers/fpga/fpga-mgr.c | 212 +++++++++++++------------------
>> drivers/fpga/ice40-spi.c | 10 +-
>> drivers/fpga/machxo2-spi.c | 11 +-
>> drivers/fpga/socfpga-a10.c | 16 +--
>> drivers/fpga/socfpga.c | 10 +-
>> drivers/fpga/stratix10-soc.c | 16 +--
>> drivers/fpga/ts73xx-fpga.c | 10 +-
>> drivers/fpga/xilinx-spi.c | 12 +-
>> drivers/fpga/zynq-fpga.c | 16 +--
>> drivers/fpga/zynqmp-fpga.c | 10 +-
>> include/linux/fpga/fpga-mgr.h | 62 ++++++---
>> 15 files changed, 204 insertions(+), 235 deletions(-)
>>
>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
>> index ccf4546eff29..ddf2ffe3f138 100644
>> --- a/drivers/fpga/altera-cvp.c
>> +++ b/drivers/fpga/altera-cvp.c
>> @@ -652,19 +652,15 @@ 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));
>>
>> - mgr = devm_fpga_mgr_create(&pdev->dev, conf->mgr_name,
>> - &altera_cvp_ops, conf);
>> - if (!mgr) {
>> - ret = -ENOMEM;
>> + mgr = fpga_mgr_register_simple(&pdev->dev, conf->mgr_name,
>> + &altera_cvp_ops, conf);
>> + if (IS_ERR(mgr)) {
>> + ret = PTR_ERR(mgr);
>> goto err_unmap;
>> }
>>
>> pci_set_drvdata(pdev, mgr);
>>
>> - ret = fpga_mgr_register(mgr);
>> - if (ret)
>> - goto err_unmap;
>> -
>> return 0;
>>
>> err_unmap:
>> diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c
>> index dfdf21ed34c4..fa3e2697d285 100644
>> --- a/drivers/fpga/altera-pr-ip-core.c
>> +++ b/drivers/fpga/altera-pr-ip-core.c
>> @@ -191,11 +191,12 @@ 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));
>>
>> - mgr = devm_fpga_mgr_create(dev, dev_name(dev), &alt_pr_ops, priv);
>> - if (!mgr)
>> - return -ENOMEM;
>> + mgr = devm_fpga_mgr_register_simple(dev, dev_name(dev),
>> + &alt_pr_ops, priv);
>> + if (IS_ERR(mgr))
>> + return PTR_ERR(mgr);
>>
>> - return devm_fpga_mgr_register(dev, mgr);
>> + return 0;
> I just remember there is a macro PTR_ERR_OR_ZERO() for this, or lkp robot
> will make the suggestion.
Sure - I'll use PTR_ERR_OR_ZERO() where I can
>
>> }
>> EXPORT_SYMBOL_GPL(alt_pr_register);
>>
>> diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
>> index 23bfd4d1ad0f..990689cf8d67 100644
>> --- a/drivers/fpga/altera-ps-spi.c
>> +++ b/drivers/fpga/altera-ps-spi.c
>> @@ -302,12 +302,12 @@ 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));
>>
>> - mgr = devm_fpga_mgr_create(&spi->dev, conf->mgr_name,
>> - &altera_ps_ops, conf);
>> - if (!mgr)
>> - return -ENOMEM;
>> + mgr = devm_fpga_mgr_register_simple(&spi->dev, conf->mgr_name,
>> + &altera_ps_ops, conf);
>> + if (IS_ERR(mgr))
>> + return PTR_ERR(mgr);
>>
>> - return devm_fpga_mgr_register(&spi->dev, mgr);
>> + return 0;
> PTR_ERR_OR_ZERO()
>
>> }
>>
>> static const struct spi_device_id altera_ps_spi_ids[] = {
>> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
>> index d5861d13b306..36add746ac42 100644
>> --- a/drivers/fpga/dfl-fme-mgr.c
>> +++ b/drivers/fpga/dfl-fme-mgr.c
>> @@ -282,7 +282,7 @@ static void fme_mgr_get_compat_id(void __iomem *fme_pr,
>> static int fme_mgr_probe(struct platform_device *pdev)
>> {
>> struct dfl_fme_mgr_pdata *pdata = dev_get_platdata(&pdev->dev);
>> - struct fpga_compat_id *compat_id;
>> + struct fpga_manager_info info = { 0 };
>> struct device *dev = &pdev->dev;
>> struct fme_mgr_priv *priv;
>> struct fpga_manager *mgr;
>> @@ -302,20 +302,19 @@ static int fme_mgr_probe(struct platform_device *pdev)
>> return PTR_ERR(priv->ioaddr);
>> }
>>
>> - compat_id = devm_kzalloc(dev, sizeof(*compat_id), GFP_KERNEL);
>> - if (!compat_id)
>> + info.name = "DFL FME FPGA Manager";
>> + info.mops = &fme_mgr_ops;
>> + info.priv = priv;
>> + info.compat_id = devm_kzalloc(dev, sizeof(*info.compat_id), GFP_KERNEL);
>> + if (!info.compat_id)
>> return -ENOMEM;
>>
>> - fme_mgr_get_compat_id(priv->ioaddr, compat_id);
>> + fme_mgr_get_compat_id(priv->ioaddr, info.compat_id);
>> + mgr = devm_fpga_mgr_register(dev, &info);
>> + if (IS_ERR(mgr))
>> + return PTR_ERR(mgr);
>>
>> - mgr = devm_fpga_mgr_create(dev, "DFL FME FPGA Manager",
>> - &fme_mgr_ops, priv);
>> - if (!mgr)
>> - return -ENOMEM;
>> -
>> - mgr->compat_id = compat_id;
>> -
>> - return devm_fpga_mgr_register(dev, mgr);
>> + return 0;
> PTR_ERR_OR_ZERO()
>
>> }
>>
>> static struct platform_driver fme_mgr_driver = {
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index ecb4c3c795fa..fa2f1a95de82 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -550,21 +550,19 @@ void fpga_mgr_unlock(struct fpga_manager *mgr)
>> EXPORT_SYMBOL_GPL(fpga_mgr_unlock);
>>
>> /**
>> - * fpga_mgr_create - create and initialize an FPGA manager struct
>> + * fpga_mgr_register - create and register an FPGA Manager device
>> * @parent: fpga manager device from pdev
>> - * @name: fpga manager name
>> - * @mops: pointer to structure of fpga manager ops
>> - * @priv: fpga manager private data
>> + * @info: parameters for fpga manager
>> *
>> - * The caller of this function is responsible for freeing the struct with
>> - * fpga_mgr_free(). Using devm_fpga_mgr_create() instead is recommended.
>> + * The caller of this function is responsible for calling fpga_mgr_unregister().
>> + * Using devm_fpga_mgr_register instead is recommended.
> devm_fpga_mgr_register()
I'll look though the comment headers and make sure I'm using the ()'s consistently.
>
>> *
>> - * Return: pointer to struct fpga_manager or NULL
>> + * Return: pointer to struct fpga_manager pointer or ERR_PTR()
>> */
>> -struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
>> - const struct fpga_manager_ops *mops,
>> - void *priv)
>> +struct fpga_manager *
>> +fpga_mgr_register(struct device *parent, struct fpga_manager_info *info)
> const struct fpga_manager_info *info
Sure - I'll change the function prototypes and definitions to use const.
>
>> {
>> + const struct fpga_manager_ops *mops = info->mops;
>> struct fpga_manager *mgr;
>> int id, ret;
>>
>> @@ -572,29 +570,31 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
>> !mops->write_init || (!mops->write && !mops->write_sg) ||
>> (mops->write && mops->write_sg)) {
>> dev_err(parent, "Attempt to register without fpga_manager_ops\n");
>> - return NULL;
>> + return ERR_PTR(-EINVAL);
>> }
>>
>> - if (!name || !strlen(name)) {
>> + if (!info->name || !strlen(info->name)) {
>> dev_err(parent, "Attempt to register with no name!\n");
>> - return NULL;
>> + return ERR_PTR(-EINVAL);
>> }
>>
>> mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
>> if (!mgr)
>> - return NULL;
>> + return ERR_PTR(-ENOMEM);
>>
>> id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
>> - if (id < 0)
>> + if (id < 0) {
>> + ret = id;
>> goto error_kfree;
>> + }
>>
>> mutex_init(&mgr->ref_mutex);
>>
>> - mgr->name = name;
>> - mgr->mops = mops;
>> - mgr->priv = priv;
>> + mgr->name = info->name;
>> + mgr->mops = info->mops;
>> + mgr->priv = info->priv;
>> + mgr->compat_id = info->compat_id;
>>
>> - device_initialize(&mgr->dev);
>> mgr->dev.class = fpga_mgr_class;
>> mgr->dev.groups = mops->groups;
>> mgr->dev.parent = parent;
>> @@ -605,6 +605,19 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
>> if (ret)
>> goto error_device;
>>
>> + /*
>> + * Initialize framework state by requesting low level driver read state
>> + * from device. FPGA may be in reset mode or may have been programmed
>> + * by bootloader or EEPROM.
>> + */
>> + mgr->state = mgr->mops->state(mgr);
>> +
>> + ret = device_register(&mgr->dev);
>> + if (ret) {
>> + put_device(&mgr->dev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> return mgr;
>>
>> error_device:
>> @@ -612,98 +625,38 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
>> error_kfree:
>> kfree(mgr);
>>
>> - return NULL;
>> -}
>> -EXPORT_SYMBOL_GPL(fpga_mgr_create);
>> -
>> -/**
>> - * fpga_mgr_free - free an FPGA manager created with fpga_mgr_create()
>> - * @mgr: fpga manager struct
>> - */
>> -void fpga_mgr_free(struct fpga_manager *mgr)
>> -{
>> - ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
>> - kfree(mgr);
>> -}
>> -EXPORT_SYMBOL_GPL(fpga_mgr_free);
>> -
>> -static void devm_fpga_mgr_release(struct device *dev, void *res)
>> -{
>> - struct fpga_mgr_devres *dr = res;
>> -
>> - fpga_mgr_free(dr->mgr);
>> + return ERR_PTR(ret);
>> }
>> +EXPORT_SYMBOL_GPL(fpga_mgr_register);
>>
>> /**
>> - * devm_fpga_mgr_create - create and initialize a managed FPGA manager struct
>> + * fpga_mgr_register_simple - create and register an FPGA Manager device
>> * @parent: fpga manager device from pdev
>> * @name: fpga manager name
>> * @mops: pointer to structure of fpga manager ops
>> * @priv: fpga manager private data
>> *
>> - * This function is intended for use in an FPGA manager driver's probe function.
>> - * After the manager driver creates the manager struct with
>> - * devm_fpga_mgr_create(), it should register it with fpga_mgr_register(). The
>> - * manager driver's remove function should call fpga_mgr_unregister(). The
>> - * manager struct allocated with this function will be freed automatically on
>> - * driver detach. This includes the case of a probe function returning error
>> - * before calling fpga_mgr_register(), the struct will still get cleaned up.
>> - *
>> - * Return: pointer to struct fpga_manager or NULL
>> - */
>> -struct fpga_manager *devm_fpga_mgr_create(struct device *parent, const char *name,
>> - const struct fpga_manager_ops *mops,
>> - void *priv)
>> -{
>> - struct fpga_mgr_devres *dr;
>> -
>> - dr = devres_alloc(devm_fpga_mgr_release, sizeof(*dr), GFP_KERNEL);
>> - if (!dr)
>> - return NULL;
>> -
>> - dr->mgr = fpga_mgr_create(parent, name, mops, priv);
>> - if (!dr->mgr) {
>> - devres_free(dr);
>> - return NULL;
>> - }
>> -
>> - devres_add(parent, dr);
>> -
>> - return dr->mgr;
>> -}
>> -EXPORT_SYMBOL_GPL(devm_fpga_mgr_create);
>> -
>> -/**
>> - * fpga_mgr_register - register an FPGA manager
>> - * @mgr: fpga manager struct
>> + * The caller of this function is responsible for calling fpga_mgr_unregister().
>> + * Using devm_fpga_mgr_register_simple instead is recommended. This simple
> devm_fpga_mgr_register_simple()
>
>> + * version of the register function should be sufficient for most users. The
>> + * fpga_mgr_register function is available for users that need to pass
>> + * additional, optional parameters.
>> *
>> - * Return: 0 on success, negative error code otherwise.
>> + * Return: pointer to struct fpga_manager pointer or ERR_PTR()
>> */
>> -int fpga_mgr_register(struct fpga_manager *mgr)
>> +struct fpga_manager *
>> +fpga_mgr_register_simple(struct device *parent, const char *name,
>> + const struct fpga_manager_ops *mops, void *priv)
>> {
>> - int ret;
>> + struct fpga_manager_info info = { 0 };
>>
>> - /*
>> - * Initialize framework state by requesting low level driver read state
>> - * from device. FPGA may be in reset mode or may have been programmed
>> - * by bootloader or EEPROM.
>> - */
>> - mgr->state = mgr->mops->state(mgr);
>> + info.name = name;
>> + info.mops = mops;
>> + info.priv = priv;
>>
>> - ret = device_add(&mgr->dev);
>> - if (ret)
>> - goto error_device;
>> -
>> - dev_info(&mgr->dev, "%s registered\n", mgr->name);
>> -
>> - return 0;
>> -
>> -error_device:
>> - ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
>> -
>> - return ret;
>> + return fpga_mgr_register(parent, &info);
>> }
>> -EXPORT_SYMBOL_GPL(fpga_mgr_register);
>> +EXPORT_SYMBOL_GPL(fpga_mgr_register_simple);
>>
>> /**
>> * fpga_mgr_unregister - unregister an FPGA manager
>> @@ -726,14 +679,6 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
>> }
>> EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
>>
>> -static int fpga_mgr_devres_match(struct device *dev, void *res,
>> - void *match_data)
>> -{
>> - struct fpga_mgr_devres *dr = res;
>> -
>> - return match_data == dr->mgr;
>> -}
>> -
>> static void devm_fpga_mgr_unregister(struct device *dev, void *res)
>> {
>> struct fpga_mgr_devres *dr = res;
>> @@ -743,44 +688,67 @@ static void devm_fpga_mgr_unregister(struct device *dev, void *res)
>>
>> /**
>> * devm_fpga_mgr_register - resource managed variant of fpga_mgr_register()
>> - * @dev: managing device for this FPGA manager
>> - * @mgr: fpga manager struct
>> + * @parent: fpga manager device from pdev
>> + * @info: parameters for fpga manager
>> *
>> * This is the devres variant of fpga_mgr_register() for which the unregister
>> * function will be called automatically when the managing device is detached.
>> */
>> -int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr)
>> +struct fpga_manager *
>> +devm_fpga_mgr_register(struct device *dev, struct fpga_manager_info *info)
> const struct fpga_manager_info
>
>> {
>> struct fpga_mgr_devres *dr;
>> - int ret;
>> -
>> - /*
>> - * Make sure that the struct fpga_manager * that is passed in is
>> - * managed itself.
>> - */
>> - if (WARN_ON(!devres_find(dev, devm_fpga_mgr_release,
>> - fpga_mgr_devres_match, mgr)))
>> - return -EINVAL;
>> + struct fpga_manager *mgr;
>>
>> dr = devres_alloc(devm_fpga_mgr_unregister, sizeof(*dr), GFP_KERNEL);
>> if (!dr)
>> - return -ENOMEM;
>> + return ERR_PTR(-ENOMEM);
>>
>> - ret = fpga_mgr_register(mgr);
>> - if (ret) {
>> + mgr = fpga_mgr_register(dev, info);
>> + if (IS_ERR(mgr)) {
>> devres_free(dr);
>> - return ret;
>> + return mgr;
>> }
>>
>> dr->mgr = mgr;
>> devres_add(dev, dr);
>>
>> - return 0;
>> + return mgr;
>> }
>> EXPORT_SYMBOL_GPL(devm_fpga_mgr_register);
>>
>> +/**
>> + * devm_fpga_mgr_register_simple - resource managed variant of
>> + * fpga_mgr_register_simple()
>> + * @dev: fpga manager device from pdev
> @parent:
Thanks - I'll fix that.
- Russ
>
>> + * @name: fpga manager name
>> + * @mops: pointer to structure of fpga manager ops
>> + * @priv: fpga manager private data
>> + *
>> + * This is the devres variant of fpga_mgr_register_simple() for which the
>> + * unregister function will be called automatically when the managing
>> + * device is detached.
>> + */
>> +struct fpga_manager *
>> +devm_fpga_mgr_register_simple(struct device *parent, const char *name,
>> + const struct fpga_manager_ops *mops, void *priv)
>> +{
>> + struct fpga_manager_info info = { 0 };
>> +
>> + info.name = name;
>> + info.mops = mops;
>> + info.priv = priv;
>> +
>> + return devm_fpga_mgr_register(parent, &info);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_fpga_mgr_register_simple);
>> +
>> 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 69dec5af23c3..498f05cf425d 100644
>> --- a/drivers/fpga/ice40-spi.c
>> +++ b/drivers/fpga/ice40-spi.c
>> @@ -178,12 +178,12 @@ static int ice40_fpga_probe(struct spi_device *spi)
>> return ret;
>> }
>>
>> - mgr = devm_fpga_mgr_create(dev, "Lattice iCE40 FPGA Manager",
>> - &ice40_fpga_ops, priv);
>> - if (!mgr)
>> - return -ENOMEM;
>> + mgr = devm_fpga_mgr_register_simple(dev, "Lattice iCE40 FPGA Manager",
>> + &ice40_fpga_ops, priv);
>> + if (IS_ERR(mgr))
>> + return PTR_ERR(mgr);
>>
>> - return devm_fpga_mgr_register(dev, mgr);
>> + return 0;
> PTR_ERR_OR_ZERO()
>
>> }
>>
>> static const struct of_device_id ice40_fpga_of_match[] = {
>> diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c
>> index 114a64d2b7a4..f0a15b724b87 100644
>> --- a/drivers/fpga/machxo2-spi.c
>> +++ b/drivers/fpga/machxo2-spi.c
>> @@ -366,12 +366,13 @@ static int machxo2_spi_probe(struct spi_device *spi)
>> return -EINVAL;
>> }
>>
>> - mgr = devm_fpga_mgr_create(dev, "Lattice MachXO2 SPI FPGA Manager",
>> - &machxo2_ops, spi);
>> - if (!mgr)
>> - return -ENOMEM;
>> + mgr = devm_fpga_mgr_register_simple(dev,
>> + "Lattice MachXO2 SPI FPGA Manager",
>> + &machxo2_ops, spi);
>> + if (IS_ERR(mgr))
>> + return PTR_ERR(mgr);
>>
>> - return devm_fpga_mgr_register(dev, mgr);
>> + return 0;
> PTR_ERR_OR_ZERO()
>
>> }
>>
>> static const struct of_device_id of_match[] = {
>> diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c
>> index 573d88bdf730..5ffefaa3eb07 100644
>> --- a/drivers/fpga/socfpga-a10.c
>> +++ b/drivers/fpga/socfpga-a10.c
>> @@ -508,19 +508,15 @@ static int socfpga_a10_fpga_probe(struct platform_device *pdev)
>> return -EBUSY;
>> }
>>
>> - mgr = devm_fpga_mgr_create(dev, "SoCFPGA Arria10 FPGA Manager",
>> - &socfpga_a10_fpga_mgr_ops, priv);
>> - if (!mgr)
>> - return -ENOMEM;
>> -
>> - platform_set_drvdata(pdev, mgr);
>> -
>> - ret = fpga_mgr_register(mgr);
>> - if (ret) {
>> + mgr = fpga_mgr_register_simple(dev, "SoCFPGA Arria10 FPGA Manager",
>> + &socfpga_a10_fpga_mgr_ops, priv);
>> + if (IS_ERR(mgr)) {
>> clk_disable_unprepare(priv->clk);
>> - return ret;
>> + return PTR_ERR(mgr);
>> }
>>
>> + platform_set_drvdata(pdev, mgr);
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
>> index 1f467173fc1f..9b4ca0ad7466 100644
>> --- a/drivers/fpga/socfpga.c
>> +++ b/drivers/fpga/socfpga.c
>> @@ -571,12 +571,12 @@ static int socfpga_fpga_probe(struct platform_device *pdev)
>> if (ret)
>> return ret;
>>
>> - mgr = devm_fpga_mgr_create(dev, "Altera SOCFPGA FPGA Manager",
>> - &socfpga_fpga_ops, priv);
>> - if (!mgr)
>> - return -ENOMEM;
>> + mgr = devm_fpga_mgr_register_simple(dev, "Altera SOCFPGA FPGA Manager",
>> + &socfpga_fpga_ops, priv);
>> + if (IS_ERR(mgr))
>> + return PTR_ERR(mgr);
>>
>> - return devm_fpga_mgr_register(dev, mgr);
>> + return 0;
> PTR_ERR_OR_ZERO()
>
>> }
>>
>> #ifdef CONFIG_OF
>> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
>> index a2cea500f7cc..fb84d88d4ce9 100644
>> --- a/drivers/fpga/stratix10-soc.c
>> +++ b/drivers/fpga/stratix10-soc.c
>> @@ -425,18 +425,11 @@ static int s10_probe(struct platform_device *pdev)
>>
>> init_completion(&priv->status_return_completion);
>>
>> - mgr = fpga_mgr_create(dev, "Stratix10 SOC FPGA Manager",
>> - &s10_ops, priv);
>> - if (!mgr) {
>> - dev_err(dev, "unable to create FPGA manager\n");
>> - ret = -ENOMEM;
>> - goto probe_err;
>> - }
>> -
>> - ret = fpga_mgr_register(mgr);
>> - if (ret) {
>> + mgr = fpga_mgr_register_simple(dev, "Stratix10 SOC FPGA Manager",
>> + &s10_ops, priv);
>> + if (IS_ERR(mgr)) {
>> dev_err(dev, "unable to register FPGA manager\n");
>> - fpga_mgr_free(mgr);
>> + ret = PTR_ERR(mgr);
>> goto probe_err;
>> }
>>
>> @@ -454,7 +447,6 @@ static int s10_remove(struct platform_device *pdev)
>> struct s10_priv *priv = mgr->priv;
>>
>> fpga_mgr_unregister(mgr);
>> - fpga_mgr_free(mgr);
>> stratix10_svc_free_channel(priv->chan);
>>
>> return 0;
>> diff --git a/drivers/fpga/ts73xx-fpga.c b/drivers/fpga/ts73xx-fpga.c
>> index 101f016c6ed8..2e11eea095ce 100644
>> --- a/drivers/fpga/ts73xx-fpga.c
>> +++ b/drivers/fpga/ts73xx-fpga.c
>> @@ -122,12 +122,12 @@ static int ts73xx_fpga_probe(struct platform_device *pdev)
>> if (IS_ERR(priv->io_base))
>> return PTR_ERR(priv->io_base);
>>
>> - mgr = devm_fpga_mgr_create(kdev, "TS-73xx FPGA Manager",
>> - &ts73xx_fpga_ops, priv);
>> - if (!mgr)
>> - return -ENOMEM;
>> + mgr = devm_fpga_mgr_register_simple(kdev, "TS-73xx FPGA Manager",
>> + &ts73xx_fpga_ops, priv);
>> + if (IS_ERR(mgr))
>> + return PTR_ERR(mgr);
>>
>> - return devm_fpga_mgr_register(kdev, mgr);
>> + return 0;
> PTR_ERR_OR_ZERO()
>
>> }
>>
>> static struct platform_driver ts73xx_fpga_driver = {
>> diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
>> index fee4d0abf6bf..e78b82f9db79 100644
>> --- a/drivers/fpga/xilinx-spi.c
>> +++ b/drivers/fpga/xilinx-spi.c
>> @@ -247,13 +247,13 @@ static int xilinx_spi_probe(struct spi_device *spi)
>> return dev_err_probe(&spi->dev, PTR_ERR(conf->done),
>> "Failed to get DONE gpio\n");
>>
>> - mgr = devm_fpga_mgr_create(&spi->dev,
>> - "Xilinx Slave Serial FPGA Manager",
>> - &xilinx_spi_ops, conf);
>> - if (!mgr)
>> - return -ENOMEM;
>> + mgr = devm_fpga_mgr_register_simple(&spi->dev,
>> + "Xilinx Slave Serial FPGA Manager",
>> + &xilinx_spi_ops, conf);
>> + if (IS_ERR(mgr))
>> + return PTR_ERR(mgr);
>>
>> - return devm_fpga_mgr_register(&spi->dev, mgr);
>> + return 0;
>> }
> PTR_ERR_OR_ZERO()
>
>>
>> static const struct of_device_id xlnx_spi_of_match[] = {
>> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
>> index 9b75bd4f93d8..a3de365aadc6 100644
>> --- a/drivers/fpga/zynq-fpga.c
>> +++ b/drivers/fpga/zynq-fpga.c
>> @@ -609,20 +609,16 @@ static int zynq_fpga_probe(struct platform_device *pdev)
>>
>> clk_disable(priv->clk);
>>
>> - mgr = devm_fpga_mgr_create(dev, "Xilinx Zynq FPGA Manager",
>> - &zynq_fpga_ops, priv);
>> - if (!mgr)
>> - return -ENOMEM;
>> -
>> - platform_set_drvdata(pdev, mgr);
>> -
>> - err = fpga_mgr_register(mgr);
>> - if (err) {
>> + mgr = fpga_mgr_register_simple(dev, "Xilinx Zynq FPGA Manager",
>> + &zynq_fpga_ops, priv);
>> + if (IS_ERR(mgr)) {
>> dev_err(dev, "unable to register FPGA manager\n");
>> clk_unprepare(priv->clk);
>> - return err;
>> + return PTR_ERR(mgr);
>> }
>>
>> + platform_set_drvdata(pdev, mgr);
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
>> index 125743c9797f..d743a05cba95 100644
>> --- a/drivers/fpga/zynqmp-fpga.c
>> +++ b/drivers/fpga/zynqmp-fpga.c
>> @@ -102,12 +102,12 @@ static int zynqmp_fpga_probe(struct platform_device *pdev)
>>
>> priv->dev = dev;
>>
>> - mgr = devm_fpga_mgr_create(dev, "Xilinx ZynqMP FPGA Manager",
>> - &zynqmp_fpga_ops, priv);
>> - if (!mgr)
>> - return -ENOMEM;
>> + mgr = devm_fpga_mgr_register_simple(dev, "Xilinx ZynqMP FPGA Manager",
>> + &zynqmp_fpga_ops, priv);
>> + if (IS_ERR(mgr))
>> + return PTR_ERR(mgr);
>>
>> - return devm_fpga_mgr_register(dev, mgr);
>> + return 0;
> PTR_ERR_OR_ZERO()
>
>> }
>>
>> static const struct of_device_id zynqmp_fpga_of_match[] = {
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index 474c1f506307..a814407bc588 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -105,6 +105,36 @@ struct fpga_image_info {
>> #endif
>> };
>>
>> +/**
>> + * struct fpga_compat_id - id for compatibility check
>> + *
>> + * @id_h: high 64bit of the compat_id
>> + * @id_l: low 64bit of the compat_id
>> + */
>> +struct fpga_compat_id {
>> + u64 id_h;
>> + u64 id_l;
>> +};
>> +
>> +/**
>> + * struct fpga_manager_info - collection of parameters for an FPGA Manager
>> + * @name: fpga manager name
>> + * @compat_id: FPGA manager id for compatibility check.
>> + * @mops: pointer to structure of fpga manager ops
>> + * @priv: fpga manager private data
>> + *
>> + * fpga_manager_info contains parameters for the register function. These
>> + * are separated into an info structure because they some are optional
>> + * others could be added to in the future. The info structure facilitates
>> + * maintaining a stable API.
>> + */
>> +struct fpga_manager_info {
>> + const char *name;
>> + struct fpga_compat_id *compat_id;
>> + const struct fpga_manager_ops *mops;
>> + void *priv;
>> +};
>> +
>> /**
>> * struct fpga_manager_ops - ops for low level fpga manager drivers
>> * @initial_header_size: Maximum number of bytes that should be passed into write_init
>> @@ -143,17 +173,6 @@ struct fpga_manager_ops {
>> #define FPGA_MGR_STATUS_IP_PROTOCOL_ERR BIT(3)
>> #define FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR BIT(4)
>>
>> -/**
>> - * struct fpga_compat_id - id for compatibility check
>> - *
>> - * @id_h: high 64bit of the compat_id
>> - * @id_l: low 64bit of the compat_id
>> - */
>> -struct fpga_compat_id {
>> - u64 id_h;
>> - u64 id_l;
>> -};
>> -
>> /**
>> * struct fpga_manager - fpga manager structure
>> * @name: name of low level fpga manager
>> @@ -191,17 +210,18 @@ struct fpga_manager *fpga_mgr_get(struct device *dev);
>>
>> void fpga_mgr_put(struct fpga_manager *mgr);
>>
>> -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);
>> +struct fpga_manager *
>> +fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
> const struct fpga_manager_info
>
>>
>> -int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr);
>> +struct fpga_manager *
>> +fpga_mgr_register_simple(struct device *dev, const char *name,
>> + const struct fpga_manager_ops *mops, void *priv);
>> +void fpga_mgr_unregister(struct fpga_manager *mgr);
>>
>> -struct fpga_manager *devm_fpga_mgr_create(struct device *dev, const char *name,
>> - const struct fpga_manager_ops *mops,
>> - void *priv);
>> +struct fpga_manager *
>> +devm_fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
> const struct fpga_manager_info
>
>> +struct fpga_manager *
>> +devm_fpga_mgr_register_simple(struct device *dev, const char *name,
>> + const struct fpga_manager_ops *mops, void *priv);
>>
>> #endif /*_LINUX_FPGA_MGR_H */
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver
2021-06-22 8:45 ` Xu Yilun
@ 2021-06-22 22:41 ` Russ Weight
0 siblings, 0 replies; 14+ messages in thread
From: Russ Weight @ 2021-06-22 22:41 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
On 6/22/21 1:45 AM, Xu Yilun wrote:
> Some more comments.
>
> On Mon, Jun 21, 2021 at 03:22:47PM -0700, Russ Weight wrote:
>> +struct fpga_manager *
>> +fpga_mgr_register(struct device *parent, struct fpga_manager_info *info)
>> {
>> + const struct fpga_manager_ops *mops = info->mops;
>> struct fpga_manager *mgr;
>> int id, ret;
>>
>> @@ -572,29 +570,31 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
> Shall we add the check?
It's already there in the line before the context that is shown.
>
> if (!mops || ...)
>
>> !mops->write_init || (!mops->write && !mops->write_sg) ||
>> (mops->write && mops->write_sg)) {
>> dev_err(parent, "Attempt to register without fpga_manager_ops\n");
>> - return NULL;
>> + return ERR_PTR(-EINVAL);
>> }
>>
>>
> [...]
>
>> +struct fpga_manager *
>> +fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
> struct device *parent, const struct fpga_manager_info
Yes - I'll fix all of the prototypes.
Thanks,
- Russ
>
>>
>> -int devm_fpga_mgr_register(struct device *dev, struct fpga_manager *mgr);
>> +struct fpga_manager *
>> +fpga_mgr_register_simple(struct device *dev, const char *name,
> struct device *parent,
>
>> + const struct fpga_manager_ops *mops, void *priv);
>> +void fpga_mgr_unregister(struct fpga_manager *mgr);
>>
>> -struct fpga_manager *devm_fpga_mgr_create(struct device *dev, const char *name,
>> - const struct fpga_manager_ops *mops,
>> - void *priv);
>> +struct fpga_manager *
>> +devm_fpga_mgr_register(struct device *dev, struct fpga_manager_info *info);
>
> struct device *parent, const struct fpga_manager_info
>
>> +struct fpga_manager *
>> +devm_fpga_mgr_register_simple(struct device *dev, const char *name,
> struct device *parent,
>
>> + const struct fpga_manager_ops *mops, void *priv);
>>
>> #endif /*_LINUX_FPGA_MGR_H */
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/3] fpga: bridge: Use standard dev_release for class driver
2021-06-22 8:02 ` Xu Yilun
@ 2021-06-22 23:04 ` Russ Weight
0 siblings, 0 replies; 14+ messages in thread
From: Russ Weight @ 2021-06-22 23:04 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
On 6/22/21 1:02 AM, Xu Yilun wrote:
> On Mon, Jun 21, 2021 at 03:22:48PM -0700, Russ Weight wrote:
>> The FPGA bridge class driver data structure is being treated as a
>> managed resource instead of using standard dev_release call-back
>> to release the class data structure. This change removes the
>> managed resource code and combines the create() and register()
>> functions into a single register() function.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v6:
>> - Changed fpga_bridge_register() parameters to accept an info data
>> structure to provide flexibility in passing optional parameters.
>> - Added fpga_bridge_register_simple() function to support current
>> parameters for users that don't require the use of optional
>> parameters.
>> v5:
>> - Rebased on top of recently accepted patches.
>> v4:
>> - Restore the previous format for the Return value in the comment header
>> for fpga_bridge_register()
>> v3:
>> - Cleaned up comment header for fpga_bridge_register()
>> - Fix error return values for fpga_bridge_register()
>> v2:
>> - No changes
>> drivers/fpga/altera-fpga2sdram.c | 12 +--
>> drivers/fpga/altera-freeze-bridge.c | 10 +--
>> drivers/fpga/altera-hps2fpga.c | 12 +--
>> drivers/fpga/dfl-fme-br.c | 10 +--
>> drivers/fpga/fpga-bridge.c | 133 +++++++++-------------------
>> drivers/fpga/xilinx-pr-decoupler.c | 17 ++--
>> include/linux/fpga/fpga-bridge.h | 33 +++++--
>> 7 files changed, 91 insertions(+), 136 deletions(-)
>>
>> diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c
>> index a78e49c63c64..e165440ebbab 100644
>> --- a/drivers/fpga/altera-fpga2sdram.c
>> +++ b/drivers/fpga/altera-fpga2sdram.c
>> @@ -121,17 +121,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);
>>
>> - br = devm_fpga_bridge_create(dev, F2S_BRIDGE_NAME,
>> - &altera_fpga2sdram_br_ops, priv);
>> - if (!br)
>> - return -ENOMEM;
>> + br = fpga_bridge_register_simple(dev, F2S_BRIDGE_NAME,
>> + &altera_fpga2sdram_br_ops, priv);
>> + if (IS_ERR(br))
>> + return PTR_ERR(mgr);
>>
>> platform_set_drvdata(pdev, br);
>>
>> - ret = fpga_bridge_register(br);
>> - if (ret)
>> - return ret;
>> -
>> dev_info(dev, "driver initialized with handoff %08x\n", priv->mask);
>>
>> if (!of_property_read_u32(dev->of_node, "bridge-enable", &enable)) {
>> diff --git a/drivers/fpga/altera-freeze-bridge.c b/drivers/fpga/altera-freeze-bridge.c
>> index dd58c4aea92e..4e39b5475630 100644
>> --- a/drivers/fpga/altera-freeze-bridge.c
>> +++ b/drivers/fpga/altera-freeze-bridge.c
>> @@ -244,14 +244,14 @@ static int altera_freeze_br_probe(struct platform_device *pdev)
>>
>> priv->base_addr = base_addr;
>>
>> - br = devm_fpga_bridge_create(dev, FREEZE_BRIDGE_NAME,
>> - &altera_freeze_br_br_ops, priv);
>> - if (!br)
>> - return -ENOMEM;
>> + br = fpga_bridge_register_simple(dev, FREEZE_BRIDGE_NAME,
>> + &altera_freeze_br_br_ops, priv);
>> + if (IS_ERR(br))
>> + return PTR_ERR(br);
>>
>> platform_set_drvdata(pdev, br);
>>
>> - return fpga_bridge_register(br);
>> + return 0;
>> }
>>
>> static int altera_freeze_br_remove(struct platform_device *pdev)
>> diff --git a/drivers/fpga/altera-hps2fpga.c b/drivers/fpga/altera-hps2fpga.c
>> index 77b95f251821..a564eb29349c 100644
>> --- a/drivers/fpga/altera-hps2fpga.c
>> +++ b/drivers/fpga/altera-hps2fpga.c
>> @@ -180,19 +180,15 @@ static int alt_fpga_bridge_probe(struct platform_device *pdev)
>> }
>> }
>>
>> - br = devm_fpga_bridge_create(dev, priv->name,
>> - &altera_hps2fpga_br_ops, priv);
>> - if (!br) {
>> - ret = -ENOMEM;
>> + br = fpga_bridge_register_simple(dev, priv->name,
>> + &altera_hps2fpga_br_ops, priv);
>> + if (IS_ERR(br)) {
>> + ret = PTR_ERR(br);
>> goto err;
>> }
>>
>> platform_set_drvdata(pdev, br);
>>
>> - ret = fpga_bridge_register(br);
>> - if (ret)
>> - goto err;
>> -
>> return 0;
>>
>> err:
>> diff --git a/drivers/fpga/dfl-fme-br.c b/drivers/fpga/dfl-fme-br.c
>> index 3ff9f3a687ce..0ad39e502142 100644
>> --- a/drivers/fpga/dfl-fme-br.c
>> +++ b/drivers/fpga/dfl-fme-br.c
>> @@ -68,14 +68,14 @@ static int fme_br_probe(struct platform_device *pdev)
>>
>> priv->pdata = dev_get_platdata(dev);
>>
>> - br = devm_fpga_bridge_create(dev, "DFL FPGA FME Bridge",
>> - &fme_bridge_ops, priv);
>> - if (!br)
>> - return -ENOMEM;
>> + br = fpga_bridge_register_simple(dev, "DFL FPGA FME Bridge",
>> + &fme_bridge_ops, priv);
>> + if (IS_ERR(br))
>> + return PTR_ERR(br);
>>
>> platform_set_drvdata(pdev, br);
>>
>> - return fpga_bridge_register(br);
>> + return 0;
>> }
>>
>> static int fme_br_remove(struct platform_device *pdev)
>> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
>> index 798f55670646..1436acfc1e7d 100644
>> --- a/drivers/fpga/fpga-bridge.c
>> +++ b/drivers/fpga/fpga-bridge.c
>> @@ -312,55 +312,57 @@ static struct attribute *fpga_bridge_attrs[] = {
>> ATTRIBUTE_GROUPS(fpga_bridge);
>>
>> /**
>> - * fpga_bridge_create - create and initialize a struct fpga_bridge
>> + * fpga_bridge_register - create and register an FPGA Bridge device
>> * @parent: FPGA bridge device from pdev
>> - * @name: FPGA bridge name
>> - * @br_ops: pointer to structure of fpga bridge ops
>> - * @priv: FPGA bridge private data
>> + * @info: parameters for FPGA Bridge
>> *
>> - * The caller of this function is responsible for freeing the bridge with
>> - * fpga_bridge_free(). Using devm_fpga_bridge_create() instead is recommended.
> Mm.. The comments are removed. Do we need a devm version of
> fpga_bridge_register? Or leave it in later patches?
There never was a devm_fpga_bridge_register() function. All callers to
fpga_bridge_register() also call fpga_bridge_unregister(). If there is a desire to add
this functionality, I think it could be done in a separate patch set.
>
>> - *
>> - * Return: struct fpga_bridge or NULL
>> + * Return: struct fpga_bridge pointer or ERR_PTR()
>> */
>> -struct fpga_bridge *fpga_bridge_create(struct device *parent, const char *name,
>> - const struct fpga_bridge_ops *br_ops,
>> - void *priv)
>> +struct fpga_bridge *fpga_bridge_register(struct device *parent,
>> + struct fpga_bridge_info *info)
> const struct fpga_bridge_info
Yes, I'll add const to the functions prototypes and definitions.
>
>> {
>> struct fpga_bridge *bridge;
>> int id, ret;
>>
>> - if (!name || !strlen(name)) {
>> + if (!info->name || !strlen(info->name)) {
>> dev_err(parent, "Attempt to register with no name!\n");
>> - return NULL;
>> + return ERR_PTR(-EINVAL);
>> }
> Is it necessary to add if(!info->br_ops) check, the field is mandatory for
> fpga_bridge.
Yes, I'll add this.
>
>>
>> bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
>> if (!bridge)
>> - return NULL;
>> + return ERR_PTR(-ENOMEM);
>>
>> id = ida_simple_get(&fpga_bridge_ida, 0, 0, GFP_KERNEL);
>> - if (id < 0)
>> + if (id < 0) {
>> + ret = id;
>> goto error_kfree;
>> + }
>>
>> mutex_init(&bridge->mutex);
>> INIT_LIST_HEAD(&bridge->node);
>>
>> - bridge->name = name;
>> - bridge->br_ops = br_ops;
>> - bridge->priv = priv;
>> + bridge->name = info->name;
>> + bridge->br_ops = info->br_ops;
>> + bridge->priv = info->priv;
>>
>> - device_initialize(&bridge->dev);
>> - bridge->dev.groups = br_ops->groups;
>> + bridge->dev.groups = info->br_ops->groups;
>> bridge->dev.class = fpga_bridge_class;
>> bridge->dev.parent = parent;
>> bridge->dev.of_node = parent->of_node;
>> bridge->dev.id = id;
>> + of_platform_populate(bridge->dev.of_node, NULL, NULL, &bridge->dev);
>>
>> ret = dev_set_name(&bridge->dev, "br%d", id);
>> if (ret)
>> goto error_device;
>>
>> + ret = device_register(&bridge->dev);
>> + if (ret) {
>> + put_device(&bridge->dev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> return bridge;
>>
>> error_device:
>> @@ -368,90 +370,37 @@ struct fpga_bridge *fpga_bridge_create(struct device *parent, const char *name,
>> error_kfree:
>> kfree(bridge);
>>
>> - return NULL;
>> -}
>> -EXPORT_SYMBOL_GPL(fpga_bridge_create);
>> -
>> -/**
>> - * fpga_bridge_free - free an fpga bridge created by fpga_bridge_create()
>> - * @bridge: FPGA bridge struct
>> - */
>> -void fpga_bridge_free(struct fpga_bridge *bridge)
>> -{
>> - ida_simple_remove(&fpga_bridge_ida, bridge->dev.id);
>> - kfree(bridge);
>> -}
>> -EXPORT_SYMBOL_GPL(fpga_bridge_free);
>> -
>> -static void devm_fpga_bridge_release(struct device *dev, void *res)
>> -{
>> - struct fpga_bridge *bridge = *(struct fpga_bridge **)res;
>> -
>> - fpga_bridge_free(bridge);
>> + return ERR_PTR(ret);
>> }
>> +EXPORT_SYMBOL_GPL(fpga_bridge_register);
>>
>> /**
>> - * devm_fpga_bridge_create - create and init a managed struct fpga_bridge
>> + * fpga_bridge_register_simple - create and register an FPGA Bridge device
>> * @parent: FPGA bridge device from pdev
>> * @name: FPGA bridge name
>> * @br_ops: pointer to structure of fpga bridge ops
>> * @priv: FPGA bridge private data
>> *
>> - * This function is intended for use in an FPGA bridge driver's probe function.
>> - * After the bridge driver creates the struct with devm_fpga_bridge_create(), it
>> - * should register the bridge with fpga_bridge_register(). The bridge driver's
>> - * remove function should call fpga_bridge_unregister(). The bridge struct
>> - * allocated with this function will be freed automatically on driver detach.
>> - * This includes the case of a probe function returning error before calling
>> - * fpga_bridge_register(), the struct will still get cleaned up.
>> - *
>> - * Return: struct fpga_bridge or NULL
>> - */
>> -struct fpga_bridge
>> -*devm_fpga_bridge_create(struct device *parent, const char *name,
>> - const struct fpga_bridge_ops *br_ops, void *priv)
>> -{
>> - struct fpga_bridge **ptr, *bridge;
>> -
>> - ptr = devres_alloc(devm_fpga_bridge_release, sizeof(*ptr), GFP_KERNEL);
>> - if (!ptr)
>> - return NULL;
>> -
>> - bridge = fpga_bridge_create(parent, name, br_ops, priv);
>> - if (!bridge) {
>> - devres_free(ptr);
>> - } else {
>> - *ptr = bridge;
>> - devres_add(parent, ptr);
>> - }
>> -
>> - return bridge;
>> -}
>> -EXPORT_SYMBOL_GPL(devm_fpga_bridge_create);
>> -
>> -/**
>> - * fpga_bridge_register - register an FPGA bridge
>> + * This simple version of the register should be sufficient for most users.
>> + * The fpga_mgr_register function is available for users that need to pass
> fpga_bridge_register
Yes - I'll fix it.
>
>> + * additional, optional parameters.
>> *
>> - * @bridge: FPGA bridge struct
>> - *
>> - * Return: 0 for success, error code otherwise.
>> + * Return: struct fpga_bridge pointer or ERR_PTR()
>> */
>> -int fpga_bridge_register(struct fpga_bridge *bridge)
>> +struct fpga_bridge *
>> +fpga_bridge_register_simple(struct device *parent, const char *name,
>> + const struct fpga_bridge_ops *br_ops,
>> + void *priv)
>> {
>> - struct device *dev = &bridge->dev;
>> - int ret;
>> -
>> - ret = device_add(dev);
>> - if (ret)
>> - return ret;
>> + struct fpga_bridge_info info = { 0 };
>>
>> - of_platform_populate(dev->of_node, NULL, NULL, dev);
>> + info.name = name;
>> + info.br_ops = br_ops;
>> + info.priv = priv;
>>
>> - dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name);
>> -
>> - return 0;
>> + return fpga_bridge_register(parent, &info);
>> }
>> -EXPORT_SYMBOL_GPL(fpga_bridge_register);
>> +EXPORT_SYMBOL_GPL(fpga_bridge_register_simple);
>>
>> /**
>> * fpga_bridge_unregister - unregister an FPGA bridge
>> @@ -475,6 +424,10 @@ EXPORT_SYMBOL_GPL(fpga_bridge_unregister);
>>
>> 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 ea2bde6e5bc4..24bc16a86091 100644
>> --- a/drivers/fpga/xilinx-pr-decoupler.c
>> +++ b/drivers/fpga/xilinx-pr-decoupler.c
>> @@ -138,22 +138,17 @@ static int xlnx_pr_decoupler_probe(struct platform_device *pdev)
>>
>> clk_disable(priv->clk);
>>
>> - br = devm_fpga_bridge_create(&pdev->dev, priv->ipconfig->name,
>> - &xlnx_pr_decoupler_br_ops, priv);
>> - if (!br) {
>> - err = -ENOMEM;
>> - goto err_clk;
>> - }
>> -
>> - platform_set_drvdata(pdev, br);
>> -
>> - err = fpga_bridge_register(br);
>> - if (err) {
>> + br = fpga_bridge_register_simple(&pdev->dev, priv->ipconfig->name,
>> + &xlnx_pr_decoupler_br_ops, priv);
>> + if (IS_ERR(br)) {
>> + err = PTR_ERR(br);
>> dev_err(&pdev->dev, "unable to register %s",
>> priv->ipconfig->name);
>> goto err_clk;
>> }
>>
>> + platform_set_drvdata(pdev, br);
>> +
>> return 0;
>>
>> err_clk:
>> diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
>> index 6c3c28806ff1..e111698012f0 100644
>> --- a/include/linux/fpga/fpga-bridge.h
>> +++ b/include/linux/fpga/fpga-bridge.h
>> @@ -22,6 +22,23 @@ struct fpga_bridge_ops {
>> const struct attribute_group **groups;
>> };
>>
>> +/**
>> + * struct fpga_bridge_info - collection of parameters an FPGA Bridge
>> + * @name: fpga bridge name
>> + * @br_ops: pointer to structure of fpga bridge ops
>> + * @priv: fpga bridge private data
>> + *
>> + * fpga_bridge_info contains parameters for the register function. These
>> + * are separated into an info structure because they some are optional
>> + * others could be added to in the future. The info structure facilitates
>> + * maintaining a stable API.
>> + */
>> +struct fpga_bridge_info {
>> + const char *name;
>> + const struct fpga_bridge_ops *br_ops;
>> + void *priv;
>> +};
>> +
>> /**
>> * struct fpga_bridge - FPGA bridge structure
>> * @name: name of low level FPGA bridge
>> @@ -62,15 +79,13 @@ int of_fpga_bridge_get_to_list(struct device_node *np,
>> struct fpga_image_info *info,
>> struct list_head *bridge_list);
>>
>> -struct fpga_bridge *fpga_bridge_create(struct device *dev, const char *name,
>> - const struct fpga_bridge_ops *br_ops,
>> - void *priv);
>> -void fpga_bridge_free(struct fpga_bridge *br);
>> -int fpga_bridge_register(struct fpga_bridge *br);
>> -void fpga_bridge_unregister(struct fpga_bridge *br);
>> +struct fpga_bridge *
>> +fpga_bridge_register(struct device *dev, struct fpga_bridge_info *info);
> struct device *parent, const struct fpga_bridge_info *info
I'll make the change.
Thanks for the comments,
- Russ
>
>>
>> -struct fpga_bridge
>> -*devm_fpga_bridge_create(struct device *dev, const char *name,
>> - const struct fpga_bridge_ops *br_ops, void *priv);
>> +struct fpga_bridge *
>> +fpga_bridge_register_simple(struct device *dev, const char *name,
>> + const struct fpga_bridge_ops *br_ops,
>> + void *priv);
>> +void fpga_bridge_unregister(struct fpga_bridge *br);
>>
>> #endif /* _LINUX_FPGA_BRIDGE_H */
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 2/3] fpga: bridge: Use standard dev_release for class driver
2021-06-22 8:23 ` Xu Yilun
@ 2021-06-22 23:05 ` Russ Weight
0 siblings, 0 replies; 14+ messages in thread
From: Russ Weight @ 2021-06-22 23:05 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
On 6/22/21 1:23 AM, Xu Yilun wrote:
> 2 more comments.
>
> On Mon, Jun 21, 2021 at 03:22:48PM -0700, Russ Weight wrote:
>> The FPGA bridge class driver data structure is being treated as a
>> managed resource instead of using standard dev_release call-back
>> to release the class data structure. This change removes the
>> managed resource code and combines the create() and register()
>> functions into a single register() function.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v6:
>> - Changed fpga_bridge_register() parameters to accept an info data
>> structure to provide flexibility in passing optional parameters.
>> - Added fpga_bridge_register_simple() function to support current
>> parameters for users that don't require the use of optional
>> parameters.
> Add the _simple() description in commit message if needed.
>
>> +struct fpga_bridge *
>> +fpga_bridge_register_simple(struct device *dev, const char *name,
> struct device *parent,
Yes - I'll make these changes.
Thanks,
- Russ
>
>> + const struct fpga_bridge_ops *br_ops,
>> + void *priv);
>> +void fpga_bridge_unregister(struct fpga_bridge *br);
>>
>> #endif /* _LINUX_FPGA_BRIDGE_H */
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v6 3/3] fpga: region: Use standard dev_release for class driver
2021-06-22 8:19 ` Xu Yilun
@ 2021-06-22 23:08 ` Russ Weight
0 siblings, 0 replies; 14+ messages in thread
From: Russ Weight @ 2021-06-22 23:08 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, trix, lgoncalv, hao.wu, matthew.gerlach, richard.gong
On 6/22/21 1:19 AM, Xu Yilun wrote:
> On Mon, Jun 21, 2021 at 03:22:49PM -0700, Russ Weight wrote:
>> The FPGA region class driver data structure is being treated as a
>> managed resource instead of using standard dev_release call-back
>> to release the class data structure. This change removes the
>> managed resource code and combines the create() and register()
>> functions into a single register() function.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v6:
>> - Moved FPGA manager optional parameters out of the ops structure and
>> back into the FPGA Region structure.
>> - Changed fpga_region_register() parameters to accept an info data
>> structure to provide flexibility in passing optional parameters.
>> - Added fpga_region_register_simple() functions to support current
>> parameters for users that don't require the use of optional parameters.
> Add the _simple() description in commit message.
Yes
>
>> v5:
>> - Rebased on top of recently accepted patches.
>> - Created the fpga_region_ops data structure which is optionally passed
>> to fpga_region_register(). compat_id, the get_bridges() pointer, and
>> the priv pointer are included in the fpga_region_ops structure.
>> v4:
>> - Added the compat_id parameter to fpga_region_register() to ensure
>> that the compat_id is set before the device_register() call.
>> - Modified the dfl_fpga_feature_devs_enumerate() function to restore
>> the fpga_region_register() call to the correct location.
>> v3:
>> - Cleaned up comment header for fpga_region_register()
>> - Fix fpga_region_register() error return on ida_simple_get() failure
>> v2:
>> - No changes
>> ---
>> drivers/fpga/dfl-fme-region.c | 17 +++--
>> drivers/fpga/dfl.c | 12 ++--
>> drivers/fpga/fpga-region.c | 115 +++++++++++--------------------
>> drivers/fpga/of-fpga-region.c | 11 ++-
>> include/linux/fpga/fpga-region.h | 36 +++++++---
>> 5 files changed, 82 insertions(+), 109 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl-fme-region.c b/drivers/fpga/dfl-fme-region.c
>> index 1eeb42af1012..36bf6865bbc3 100644
>> --- a/drivers/fpga/dfl-fme-region.c
>> +++ b/drivers/fpga/dfl-fme-region.c
>> @@ -30,6 +30,7 @@ static int fme_region_get_bridges(struct fpga_region *region)
>> static int fme_region_probe(struct platform_device *pdev)
>> {
>> struct dfl_fme_region_pdata *pdata = dev_get_platdata(&pdev->dev);
>> + struct fpga_region_info info = { 0 };
>> struct device *dev = &pdev->dev;
>> struct fpga_region *region;
>> struct fpga_manager *mgr;
>> @@ -39,20 +40,18 @@ static int fme_region_probe(struct platform_device *pdev)
>> if (IS_ERR(mgr))
>> return -EPROBE_DEFER;
>>
>> - region = devm_fpga_region_create(dev, mgr, fme_region_get_bridges);
>> - if (!region) {
>> - ret = -ENOMEM;
>> + info.mgr = mgr;
>> + info.compat_id = mgr->compat_id;
>> + info.get_bridges = fme_region_get_bridges;
>> + info.priv = pdata;
>> + region = fpga_region_register(dev, &info);
>> + if (IS_ERR(region)) {
>> + ret = PTR_ERR(region);
>> goto eprobe_mgr_put;
>> }
>>
>> - region->priv = pdata;
>> - region->compat_id = mgr->compat_id;
>> platform_set_drvdata(pdev, region);
>>
>> - ret = fpga_region_register(region);
>> - if (ret)
>> - goto eprobe_mgr_put;
>> -
>> dev_dbg(dev, "DFL FME FPGA Region probed\n");
>>
>> return 0;
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index 511b20ff35a3..a4269d40586e 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -1400,19 +1400,15 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
>> if (!cdev)
>> return ERR_PTR(-ENOMEM);
>>
>> - cdev->region = devm_fpga_region_create(info->dev, NULL, NULL);
>> - if (!cdev->region) {
>> - ret = -ENOMEM;
>> - goto free_cdev_exit;
>> - }
>> -
>> cdev->parent = info->dev;
>> mutex_init(&cdev->lock);
>> INIT_LIST_HEAD(&cdev->port_dev_list);
>>
>> - ret = fpga_region_register(cdev->region);
>> - if (ret)
>> + cdev->region = fpga_region_register_simple(info->dev, NULL, NULL);
>> + if (IS_ERR(cdev->region)) {
>> + ret = PTR_ERR(cdev->region);
>> goto free_cdev_exit;
>> + }
>>
>> /* create and init build info for enumeration */
>> binfo = devm_kzalloc(info->dev, sizeof(*binfo), GFP_KERNEL);
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index a4838715221f..0d831b85a98f 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -180,39 +180,36 @@ static struct attribute *fpga_region_attrs[] = {
>> ATTRIBUTE_GROUPS(fpga_region);
>>
>> /**
>> - * fpga_region_create - alloc and init a struct fpga_region
>> + * fpga_region_register - create and register an FPGA Region device
>> * @parent: device parent
>> - * @mgr: manager that programs this region
>> - * @get_bridges: optional function to get bridges to a list
>> - *
>> - * The caller of this function is responsible for freeing the resulting region
>> - * struct with fpga_region_free(). Using devm_fpga_region_create() instead is
>> - * recommended.
> Same question, introduce a devm_fpga_region_register or leave it later?
I think it could be left for a different patch set. I would prefer not to introduce
additional/new functionality that might draw out the review process since these patches
are gating the security manager patch-set.
>
>> + * @info: parameters for FPGA Region
>> *
>> - * Return: struct fpga_region or NULL
>> + * Return: struct fpga_region or ERR_PTR()
>> */
>> -struct fpga_region
>> -*fpga_region_create(struct device *parent,
>> - struct fpga_manager *mgr,
>> - int (*get_bridges)(struct fpga_region *))
>> +struct fpga_region *
>> +fpga_region_register(struct device *parent, struct fpga_region_info *info)
> const struct fpga_region_info
Yes - I'll make the change.
>
>> {
>> struct fpga_region *region;
>> int id, ret = 0;
>>
>> region = kzalloc(sizeof(*region), GFP_KERNEL);
>> if (!region)
>> - return NULL;
>> + return ERR_PTR(-ENOMEM);
>>
>> id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL);
>> - if (id < 0)
>> + if (id < 0) {
>> + ret = id;
>> goto err_free;
>> + }
>> +
>> + region->mgr = info->mgr;
>> + region->compat_id = info->compat_id;
>> + region->priv = info->priv;
>> + region->get_bridges = info->get_bridges;
>>
>> - region->mgr = mgr;
>> - region->get_bridges = get_bridges;
>> mutex_init(®ion->mutex);
>> INIT_LIST_HEAD(®ion->bridge_list);
>>
>> - device_initialize(®ion->dev);
>> region->dev.class = fpga_region_class;
>> region->dev.parent = parent;
>> region->dev.of_node = parent->of_node;
>> @@ -222,6 +219,12 @@ struct fpga_region
>> if (ret)
>> goto err_remove;
>>
>> + ret = device_register(®ion->dev);
>> + if (ret) {
>> + put_device(®ion->dev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> return region;
>>
>> err_remove:
>> @@ -229,78 +232,34 @@ struct fpga_region
>> err_free:
>> kfree(region);
>>
>> - return NULL;
>> -}
>> -EXPORT_SYMBOL_GPL(fpga_region_create);
>> -
>> -/**
>> - * fpga_region_free - free an FPGA region created by fpga_region_create()
>> - * @region: FPGA region
>> - */
>> -void fpga_region_free(struct fpga_region *region)
>> -{
>> - ida_simple_remove(&fpga_region_ida, region->dev.id);
>> - kfree(region);
>> -}
>> -EXPORT_SYMBOL_GPL(fpga_region_free);
>> -
>> -static void devm_fpga_region_release(struct device *dev, void *res)
>> -{
>> - struct fpga_region *region = *(struct fpga_region **)res;
>> -
>> - fpga_region_free(region);
>> + return ERR_PTR(ret);
>> }
>> +EXPORT_SYMBOL_GPL(fpga_region_register);
>>
>> /**
>> - * devm_fpga_region_create - create and initialize a managed FPGA region struct
>> + * fpga_region_register_simple - create and register an FPGA Region device
>> * @parent: device parent
>> * @mgr: manager that programs this region
>> * @get_bridges: optional function to get bridges to a list
>> *
>> - * This function is intended for use in an FPGA region driver's probe function.
>> - * After the region driver creates the region struct with
>> - * devm_fpga_region_create(), it should register it with fpga_region_register().
>> - * The region driver's remove function should call fpga_region_unregister().
>> - * The region struct allocated with this function will be freed automatically on
>> - * driver detach. This includes the case of a probe function returning error
>> - * before calling fpga_region_register(), the struct will still get cleaned up.
>> + * This simple version of the register should be sufficient for most users.
>> + * The fpga_bridge_register function is available for users that need to
> fpga_region_register()
Yes - I'll fix it
>
>> + * pass additional, optional parameters.
>> *
>> - * Return: struct fpga_region or NULL
>> + * Return: struct fpga_region or ERR_PTR()
>> */
>> -struct fpga_region
>> -*devm_fpga_region_create(struct device *parent,
>> - struct fpga_manager *mgr,
>> - int (*get_bridges)(struct fpga_region *))
>> +struct fpga_region *
>> +fpga_region_register_simple(struct device *parent, struct fpga_manager *mgr,
>> + int (*get_bridges)(struct fpga_region *))
>> {
>> - struct fpga_region **ptr, *region;
>> -
>> - ptr = devres_alloc(devm_fpga_region_release, sizeof(*ptr), GFP_KERNEL);
>> - if (!ptr)
>> - return NULL;
>> + struct fpga_region_info info = { 0 };
>>
>> - region = fpga_region_create(parent, mgr, get_bridges);
>> - if (!region) {
>> - devres_free(ptr);
>> - } else {
>> - *ptr = region;
>> - devres_add(parent, ptr);
>> - }
>> + info.mgr = mgr;
>> + info.get_bridges = get_bridges;
>>
>> - return region;
>> + return fpga_region_register(parent, &info);
>> }
>> -EXPORT_SYMBOL_GPL(devm_fpga_region_create);
>> -
>> -/**
>> - * fpga_region_register - register an FPGA region
>> - * @region: FPGA region
>> - *
>> - * Return: 0 or -errno
>> - */
>> -int fpga_region_register(struct fpga_region *region)
>> -{
>> - return device_add(®ion->dev);
>> -}
>> -EXPORT_SYMBOL_GPL(fpga_region_register);
>> +EXPORT_SYMBOL_GPL(fpga_region_register_simple);
>>
>> /**
>> * fpga_region_unregister - unregister an FPGA region
>> @@ -316,6 +275,10 @@ EXPORT_SYMBOL_GPL(fpga_region_unregister);
>>
>> static void fpga_region_dev_release(struct device *dev)
>> {
>> + struct fpga_region *region = to_fpga_region(dev);
>> +
>> + ida_simple_remove(&fpga_region_ida, region->dev.id);
>> + kfree(region);
>> }
>>
>> /**
>> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
>> index e3c25576b6b9..377d19585d8e 100644
>> --- a/drivers/fpga/of-fpga-region.c
>> +++ b/drivers/fpga/of-fpga-region.c
>> @@ -405,16 +405,13 @@ static int of_fpga_region_probe(struct platform_device *pdev)
>> if (IS_ERR(mgr))
>> return -EPROBE_DEFER;
>>
>> - region = devm_fpga_region_create(dev, mgr, of_fpga_region_get_bridges);
>> - if (!region) {
>> - ret = -ENOMEM;
>> + region = fpga_region_register_simple(dev, mgr,
>> + of_fpga_region_get_bridges);
>> + if (IS_ERR(region)) {
>> + ret = PTR_ERR(region);
>> goto eprobe_mgr_put;
>> }
>>
>> - ret = fpga_region_register(region);
>> - if (ret)
>> - goto eprobe_mgr_put;
>> -
>> of_platform_populate(np, fpga_region_of_match, NULL, ®ion->dev);
>> platform_set_drvdata(pdev, region);
>>
>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>> index 27cb706275db..7b8f76d52c4e 100644
>> --- a/include/linux/fpga/fpga-region.h
>> +++ b/include/linux/fpga/fpga-region.h
>> @@ -7,6 +7,27 @@
>> #include <linux/fpga/fpga-mgr.h>
>> #include <linux/fpga/fpga-bridge.h>
>>
>> +struct fpga_region;
>> +
>> +/**
>> + * struct fpga_region_info - collection of parameters an FPGA Region
>> + * @mgr: fpga region manager
>> + * @compat_id: FPGA region id for compatibility check.
>> + * @priv: fpga region private data
>> + * @get_bridges: optional function to get bridges to a list
>> + *
>> + * fpga_region_info contains parameters for the register function. These
>> + * are separated into an info structure because they some are optional
>> + * others could be added to in the future. The info structure facilitates
>> + * maintaining a stable API.
>> + */
>> +struct fpga_region_info {
>> + struct fpga_manager *mgr;
>> + struct fpga_compat_id *compat_id;
>> + void *priv;
>> + int (*get_bridges)(struct fpga_region *region);
>> +};
>> +
>> /**
>> * struct fpga_region - FPGA Region structure
>> * @dev: FPGA Region device
>> @@ -37,15 +58,12 @@ struct fpga_region *fpga_region_class_find(
>>
>> int fpga_region_program_fpga(struct fpga_region *region);
>>
>> -struct fpga_region
>> -*fpga_region_create(struct device *dev, struct fpga_manager *mgr,
>> - int (*get_bridges)(struct fpga_region *));
>> -void fpga_region_free(struct fpga_region *region);
>> -int fpga_region_register(struct fpga_region *region);
>> -void fpga_region_unregister(struct fpga_region *region);
>> +struct fpga_region *
>> +fpga_region_register(struct device *dev, struct fpga_region_info *info);
> struct device *parent, const struct fpga_region_info
yes
>
>>
>> -struct fpga_region
>> -*devm_fpga_region_create(struct device *dev, struct fpga_manager *mgr,
>> - int (*get_bridges)(struct fpga_region *));
>> +struct fpga_region *
>> +fpga_region_register_simple(struct device *dev, struct fpga_manager *mgr,
> struct device *parent
yes
Thanks,
- Russ
>
>> + int (*get_bridges)(struct fpga_region *));
>> +void fpga_region_unregister(struct fpga_region *region);
>>
>> #endif /* _FPGA_REGION_H */
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-06-22 23:09 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-21 22:22 [PATCH v6 0/3] fpga: Use standard class dev_release function Russ Weight
2021-06-21 22:22 ` [PATCH v6 1/3] fpga: mgr: Use standard dev_release for class driver Russ Weight
2021-06-22 7:32 ` Xu Yilun
2021-06-22 22:32 ` Russ Weight
2021-06-22 8:45 ` Xu Yilun
2021-06-22 22:41 ` Russ Weight
2021-06-21 22:22 ` [PATCH v6 2/3] fpga: bridge: " Russ Weight
2021-06-22 8:02 ` Xu Yilun
2021-06-22 23:04 ` Russ Weight
2021-06-22 8:23 ` Xu Yilun
2021-06-22 23:05 ` Russ Weight
2021-06-21 22:22 ` [PATCH v6 3/3] fpga: region: " Russ Weight
2021-06-22 8:19 ` Xu Yilun
2021-06-22 23:08 ` Russ Weight
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).