| Message ID | 20210701013733.75483-3-russell.h.weight@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fpga: Use standard class dev_release function | expand |
On Wed, Jun 30, 2021 at 06:37:32PM -0700, Russ Weight wrote: > The FPGA bridge class driver data structure is being treated as a > managed resource instead of using the standard dev_release call-back > function to release the class data structure. This change removes > the managed resource code and combines the create() and register() > functions into a single register() or register_simple() function. > > The register() function accepts an info data structure to provide > flexibility in passing optional parameters. The register_simple() > function supports the current parameter list for users that don't > require the use of optional parameters. Please update the commit message, others are good to me. Thanks, Yilun > > Signed-off-by: Russ Weight <russell.h.weight@intel.com> > Reviewed-by: Xu Yilun <yilun.xu@intel.com> > --- > v9: > - Cleaned up documentation for the FPGA Bridge register functions > - Renamed fpga_bridge_register() to fpga_bridge_register_full() > - Renamed fpga_bridge_register_simple() to fpga_bridge_register() > v8: > - Added reviewed-by tag. > - Updated Documentation/driver-api/fpga/fpga-bridge.rst documentation. > v7: > - Update the commit message to describe the new parameters for the > fpga_bridge_register() function and to mention the > fpga_bridge_register_simple() function. > - Fix function prototypes in header file to rename dev to parent. > - Some cleanup of comments. > - Update function defintions/prototypes to apply const to the new info > parameter. > - Verify that info->br_ops is non-null in register functions. > 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 > --- > Documentation/driver-api/fpga/fpga-bridge.rst | 8 +- > 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 | 137 ++++++------------ > drivers/fpga/xilinx-pr-decoupler.c | 17 +-- > include/linux/fpga/fpga-bridge.h | 33 +++-- > 8 files changed, 101 insertions(+), 138 deletions(-) > > diff --git a/Documentation/driver-api/fpga/fpga-bridge.rst b/Documentation/driver-api/fpga/fpga-bridge.rst > index 8d650b4e2ce6..38e3bac8e877 100644 > --- a/Documentation/driver-api/fpga/fpga-bridge.rst > +++ b/Documentation/driver-api/fpga/fpga-bridge.rst > @@ -6,8 +6,10 @@ API to implement a new FPGA bridge > > * struct fpga_bridge - The FPGA Bridge structure > * struct fpga_bridge_ops - Low level Bridge driver ops > -* devm_fpga_bridge_create() - Allocate and init a bridge struct > -* fpga_bridge_register() - Register a bridge > +* fpga_bridge_register_full() - Create and register a bridge using the > + fpga_bridge_info structure to provide the full flexibility of options > +* fpga_bridge_register() - Create and register a bridge using standard > + arguments > * fpga_bridge_unregister() - Unregister a bridge > > .. kernel-doc:: include/linux/fpga/fpga-bridge.h > @@ -17,7 +19,7 @@ API to implement a new FPGA bridge > :functions: fpga_bridge_ops > > .. kernel-doc:: drivers/fpga/fpga-bridge.c > - :functions: devm_fpga_bridge_create > + :functions: fpga_bridge_register_full > > .. kernel-doc:: drivers/fpga/fpga-bridge.c > :functions: fpga_bridge_register > diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c > index a78e49c63c64..66063507116b 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(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..bfbfa43cd05b 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(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..aa758426c22b 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(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..808d1f4d76df 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(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..e6d98be8531b 100644 > --- a/drivers/fpga/fpga-bridge.c > +++ b/drivers/fpga/fpga-bridge.c > @@ -312,55 +312,63 @@ static struct attribute *fpga_bridge_attrs[] = { > ATTRIBUTE_GROUPS(fpga_bridge); > > /** > - * fpga_bridge_create - create and initialize a struct fpga_bridge > + * fpga_bridge_register_full - 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_full(struct device *parent, > + const struct fpga_bridge_info *info) > { > struct fpga_bridge *bridge; > int id, ret; > > - if (!name || !strlen(name)) { > + if (!info->br_ops) { > + dev_err(parent, "Attempt to register without fpga_bridge_ops\n"); > + return ERR_PTR(-EINVAL); > + } > + > + 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,88 +376,35 @@ 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_full); > > /** > - * devm_fpga_bridge_create - create and init a managed 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 > * > - * 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. > + * This simple version of the register should be sufficient for most users. > + * The fpga_bridge_register_full() function is available for users that need to pass > + * additional, optional parameters. > * > - * Return: struct fpga_bridge or NULL > + * Return: struct fpga_bridge pointer or ERR_PTR() > */ > -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 * > +fpga_bridge_register(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); > - } > + struct fpga_bridge_info info = { 0 }; > > - return bridge; > -} > -EXPORT_SYMBOL_GPL(devm_fpga_bridge_create); > + info.name = name; > + info.br_ops = br_ops; > + info.priv = priv; > > -/** > - * fpga_bridge_register - register an FPGA bridge > - * > - * @bridge: FPGA bridge struct > - * > - * Return: 0 for success, error code otherwise. > - */ > -int fpga_bridge_register(struct fpga_bridge *bridge) > -{ > - struct device *dev = &bridge->dev; > - int ret; > - > - ret = device_add(dev); > - if (ret) > - return ret; > - > - of_platform_populate(dev->of_node, NULL, NULL, dev); > - > - dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name); > - > - return 0; > + return fpga_bridge_register_full(parent, &info); > } > EXPORT_SYMBOL_GPL(fpga_bridge_register); > > @@ -475,6 +430,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..c004e52b9464 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(&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..f0a4d99d9c8e 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_full(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(struct device *parent, 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
On Wed, Jun 30, 2021 at 06:37:32PM -0700, Russ Weight wrote: > The FPGA bridge class driver data structure is being treated as a > managed resource instead of using the standard dev_release call-back > function to release the class data structure. This change removes > the managed resource code and combines the create() and register() > functions into a single register() or register_simple() function. > > The register() function accepts an info data structure to provide > flexibility in passing optional parameters. The register_simple() > function supports the current parameter list for users that don't > require the use of optional parameters. > > Signed-off-by: Russ Weight <russell.h.weight@intel.com> > Reviewed-by: Xu Yilun <yilun.xu@intel.com> > --- > v9: > - Cleaned up documentation for the FPGA Bridge register functions > - Renamed fpga_bridge_register() to fpga_bridge_register_full() > - Renamed fpga_bridge_register_simple() to fpga_bridge_register() Now we come back to the fpga_XXX_register for normal cases and fpga_XXX_register_full for extra parameters. But the fpga_bridge_register_full has no use case yet, should we implement it when it is really needed? Thanks, Yilun > v8: > - Added reviewed-by tag. > - Updated Documentation/driver-api/fpga/fpga-bridge.rst documentation. > v7: > - Update the commit message to describe the new parameters for the > fpga_bridge_register() function and to mention the > fpga_bridge_register_simple() function. > - Fix function prototypes in header file to rename dev to parent. > - Some cleanup of comments. > - Update function defintions/prototypes to apply const to the new info > parameter. > - Verify that info->br_ops is non-null in register functions. > 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 > --- > Documentation/driver-api/fpga/fpga-bridge.rst | 8 +- > 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 | 137 ++++++------------ > drivers/fpga/xilinx-pr-decoupler.c | 17 +-- > include/linux/fpga/fpga-bridge.h | 33 +++-- > 8 files changed, 101 insertions(+), 138 deletions(-) > > diff --git a/Documentation/driver-api/fpga/fpga-bridge.rst b/Documentation/driver-api/fpga/fpga-bridge.rst > index 8d650b4e2ce6..38e3bac8e877 100644 > --- a/Documentation/driver-api/fpga/fpga-bridge.rst > +++ b/Documentation/driver-api/fpga/fpga-bridge.rst > @@ -6,8 +6,10 @@ API to implement a new FPGA bridge > > * struct fpga_bridge - The FPGA Bridge structure > * struct fpga_bridge_ops - Low level Bridge driver ops > -* devm_fpga_bridge_create() - Allocate and init a bridge struct > -* fpga_bridge_register() - Register a bridge > +* fpga_bridge_register_full() - Create and register a bridge using the > + fpga_bridge_info structure to provide the full flexibility of options > +* fpga_bridge_register() - Create and register a bridge using standard > + arguments > * fpga_bridge_unregister() - Unregister a bridge > > .. kernel-doc:: include/linux/fpga/fpga-bridge.h > @@ -17,7 +19,7 @@ API to implement a new FPGA bridge > :functions: fpga_bridge_ops > > .. kernel-doc:: drivers/fpga/fpga-bridge.c > - :functions: devm_fpga_bridge_create > + :functions: fpga_bridge_register_full > > .. kernel-doc:: drivers/fpga/fpga-bridge.c > :functions: fpga_bridge_register > diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c > index a78e49c63c64..66063507116b 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(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..bfbfa43cd05b 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(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..aa758426c22b 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(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..808d1f4d76df 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(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..e6d98be8531b 100644 > --- a/drivers/fpga/fpga-bridge.c > +++ b/drivers/fpga/fpga-bridge.c > @@ -312,55 +312,63 @@ static struct attribute *fpga_bridge_attrs[] = { > ATTRIBUTE_GROUPS(fpga_bridge); > > /** > - * fpga_bridge_create - create and initialize a struct fpga_bridge > + * fpga_bridge_register_full - 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_full(struct device *parent, > + const struct fpga_bridge_info *info) > { > struct fpga_bridge *bridge; > int id, ret; > > - if (!name || !strlen(name)) { > + if (!info->br_ops) { > + dev_err(parent, "Attempt to register without fpga_bridge_ops\n"); > + return ERR_PTR(-EINVAL); > + } > + > + 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,88 +376,35 @@ 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_full); > > /** > - * devm_fpga_bridge_create - create and init a managed 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 > * > - * 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. > + * This simple version of the register should be sufficient for most users. > + * The fpga_bridge_register_full() function is available for users that need to pass > + * additional, optional parameters. > * > - * Return: struct fpga_bridge or NULL > + * Return: struct fpga_bridge pointer or ERR_PTR() > */ > -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 * > +fpga_bridge_register(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); > - } > + struct fpga_bridge_info info = { 0 }; > > - return bridge; > -} > -EXPORT_SYMBOL_GPL(devm_fpga_bridge_create); > + info.name = name; > + info.br_ops = br_ops; > + info.priv = priv; > > -/** > - * fpga_bridge_register - register an FPGA bridge > - * > - * @bridge: FPGA bridge struct > - * > - * Return: 0 for success, error code otherwise. > - */ > -int fpga_bridge_register(struct fpga_bridge *bridge) > -{ > - struct device *dev = &bridge->dev; > - int ret; > - > - ret = device_add(dev); > - if (ret) > - return ret; > - > - of_platform_populate(dev->of_node, NULL, NULL, dev); > - > - dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name); > - > - return 0; > + return fpga_bridge_register_full(parent, &info); > } > EXPORT_SYMBOL_GPL(fpga_bridge_register); > > @@ -475,6 +430,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..c004e52b9464 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(&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..f0a4d99d9c8e 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_full(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(struct device *parent, 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
Russ, Similar concern with 2 register functions as in fpga-mgr. But here it does not seen that any instance uses fpga_bridge_register_full directly. So it really isn't needed. However if fpga-mgr changes to having a single register(... *info), fpga-bridge should likewise. Peeking ahead, fpga-region looks similar. my preference for api stability is for have mgr,bridge and region to have the *register_full(... *info) as the only *register() On 6/30/21 6:37 PM, Russ Weight wrote: > The FPGA bridge class driver data structure is being treated as a > managed resource instead of using the standard dev_release call-back > function to release the class data structure. This change removes > the managed resource code and combines the create() and register() > functions into a single register() or register_simple() function. > > The register() function accepts an info data structure to provide > flexibility in passing optional parameters. The register_simple() > function supports the current parameter list for users that don't > require the use of optional parameters. > > Signed-off-by: Russ Weight <russell.h.weight@intel.com> > Reviewed-by: Xu Yilun <yilun.xu@intel.com> > --- > v9: > - Cleaned up documentation for the FPGA Bridge register functions > - Renamed fpga_bridge_register() to fpga_bridge_register_full() > - Renamed fpga_bridge_register_simple() to fpga_bridge_register() > v8: > - Added reviewed-by tag. > - Updated Documentation/driver-api/fpga/fpga-bridge.rst documentation. > v7: > - Update the commit message to describe the new parameters for the > fpga_bridge_register() function and to mention the > fpga_bridge_register_simple() function. > - Fix function prototypes in header file to rename dev to parent. > - Some cleanup of comments. > - Update function defintions/prototypes to apply const to the new info > parameter. > - Verify that info->br_ops is non-null in register functions. > 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 > --- > Documentation/driver-api/fpga/fpga-bridge.rst | 8 +- > 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 | 137 ++++++------------ > drivers/fpga/xilinx-pr-decoupler.c | 17 +-- > include/linux/fpga/fpga-bridge.h | 33 +++-- > 8 files changed, 101 insertions(+), 138 deletions(-) > > diff --git a/Documentation/driver-api/fpga/fpga-bridge.rst b/Documentation/driver-api/fpga/fpga-bridge.rst > index 8d650b4e2ce6..38e3bac8e877 100644 > --- a/Documentation/driver-api/fpga/fpga-bridge.rst > +++ b/Documentation/driver-api/fpga/fpga-bridge.rst > @@ -6,8 +6,10 @@ API to implement a new FPGA bridge > > * struct fpga_bridge - The FPGA Bridge structure > * struct fpga_bridge_ops - Low level Bridge driver ops > -* devm_fpga_bridge_create() - Allocate and init a bridge struct > -* fpga_bridge_register() - Register a bridge > +* fpga_bridge_register_full() - Create and register a bridge using the > + fpga_bridge_info structure to provide the full flexibility of options fpga_bridge_info (and fpga_mgr_info earlier) could be added to the kernel-doc:: list lower down > +* fpga_bridge_register() - Create and register a bridge using standard > + arguments > * fpga_bridge_unregister() - Unregister a bridge > > .. kernel-doc:: include/linux/fpga/fpga-bridge.h > @@ -17,7 +19,7 @@ API to implement a new FPGA bridge > :functions: fpga_bridge_ops > > .. kernel-doc:: drivers/fpga/fpga-bridge.c > - :functions: devm_fpga_bridge_create > + :functions: fpga_bridge_register_full > > .. kernel-doc:: drivers/fpga/fpga-bridge.c > :functions: fpga_bridge_register > diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c > index a78e49c63c64..66063507116b 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(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..bfbfa43cd05b 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(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..aa758426c22b 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(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..808d1f4d76df 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(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..e6d98be8531b 100644 > --- a/drivers/fpga/fpga-bridge.c > +++ b/drivers/fpga/fpga-bridge.c > @@ -312,55 +312,63 @@ static struct attribute *fpga_bridge_attrs[] = { > ATTRIBUTE_GROUPS(fpga_bridge); > > /** > - * fpga_bridge_create - create and initialize a struct fpga_bridge > + * fpga_bridge_register_full - 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_full(struct device *parent, > + const struct fpga_bridge_info *info) > { > struct fpga_bridge *bridge; > int id, ret; > > - if (!name || !strlen(name)) { > + if (!info->br_ops) { > + dev_err(parent, "Attempt to register without fpga_bridge_ops\n"); > + return ERR_PTR(-EINVAL); > + } > + > + 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,88 +376,35 @@ 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_full); > > /** > - * devm_fpga_bridge_create - create and init a managed 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 > * > - * 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. > + * This simple version of the register should be sufficient for most users. > + * The fpga_bridge_register_full() function is available for users that need to pass > + * additional, optional parameters. > * > - * Return: struct fpga_bridge or NULL > + * Return: struct fpga_bridge pointer or ERR_PTR() > */ > -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 * > +fpga_bridge_register(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); > - } > + struct fpga_bridge_info info = { 0 }; > > - return bridge; > -} > -EXPORT_SYMBOL_GPL(devm_fpga_bridge_create); > + info.name = name; > + info.br_ops = br_ops; > + info.priv = priv; > > -/** > - * fpga_bridge_register - register an FPGA bridge > - * > - * @bridge: FPGA bridge struct > - * > - * Return: 0 for success, error code otherwise. > - */ > -int fpga_bridge_register(struct fpga_bridge *bridge) > -{ > - struct device *dev = &bridge->dev; > - int ret; > - > - ret = device_add(dev); > - if (ret) > - return ret; > - > - of_platform_populate(dev->of_node, NULL, NULL, dev); > - > - dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name); > - > - return 0; > + return fpga_bridge_register_full(parent, &info); > } > EXPORT_SYMBOL_GPL(fpga_bridge_register); > > @@ -475,6 +430,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..c004e52b9464 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(&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..f0a4d99d9c8e 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; This groups seems to be unused. If it is, maybe it should go into info. Tom > }; > > +/** > + * 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_full(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(struct device *parent, 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 */
On 7/6/21 1:08 PM, Tom Rix wrote: > Russ, > > Similar concern with 2 register functions as in fpga-mgr. > > But here it does not seen that any instance uses fpga_bridge_register_full directly. > > So it really isn't needed. Sure - the fpga_bridge_register_full() is really not needed. I can delete that. > > However if fpga-mgr changes to having a single register(... *info), fpga-bridge should likewise. > > Peeking ahead, fpga-region looks similar. > > my preference for api stability is for have mgr,bridge and region to have the *register_full(... *info) as the only *register() > > > On 6/30/21 6:37 PM, Russ Weight wrote: >> The FPGA bridge class driver data structure is being treated as a >> managed resource instead of using the standard dev_release call-back >> function to release the class data structure. This change removes >> the managed resource code and combines the create() and register() >> functions into a single register() or register_simple() function. >> >> The register() function accepts an info data structure to provide >> flexibility in passing optional parameters. The register_simple() >> function supports the current parameter list for users that don't >> require the use of optional parameters. >> >> Signed-off-by: Russ Weight <russell.h.weight@intel.com> >> Reviewed-by: Xu Yilun <yilun.xu@intel.com> >> --- >> v9: >>   - Cleaned up documentation for the FPGA Bridge register functions >>   - Renamed fpga_bridge_register() to fpga_bridge_register_full() >>   - Renamed fpga_bridge_register_simple() to fpga_bridge_register() >> v8: >>   - Added reviewed-by tag. >>   - Updated Documentation/driver-api/fpga/fpga-bridge.rst documentation. >> v7: >>   - Update the commit message to describe the new parameters for the >>     fpga_bridge_register() function and to mention the >>     fpga_bridge_register_simple() function. >>   - Fix function prototypes in header file to rename dev to parent. >>   - Some cleanup of comments. >>   - Update function defintions/prototypes to apply const to the new info >>     parameter. >>   - Verify that info->br_ops is non-null in register functions. >> 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 >> --- >>  Documentation/driver-api/fpga/fpga-bridge.rst |  8 +- >>  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                   | 137 ++++++------------ >>  drivers/fpga/xilinx-pr-decoupler.c           | 17 +-- >>  include/linux/fpga/fpga-bridge.h             | 33 +++-- >>  8 files changed, 101 insertions(+), 138 deletions(-) >> >> diff --git a/Documentation/driver-api/fpga/fpga-bridge.rst b/Documentation/driver-api/fpga/fpga-bridge.rst >> index 8d650b4e2ce6..38e3bac8e877 100644 >> --- a/Documentation/driver-api/fpga/fpga-bridge.rst >> +++ b/Documentation/driver-api/fpga/fpga-bridge.rst >> @@ -6,8 +6,10 @@ API to implement a new FPGA bridge >>   * struct fpga_bridge - The FPGA Bridge structure >>  * struct fpga_bridge_ops - Low level Bridge driver ops >> -* devm_fpga_bridge_create() - Allocate and init a bridge struct >> -* fpga_bridge_register() - Register a bridge >> +* fpga_bridge_register_full() - Create and register a bridge using the >> + fpga_bridge_info structure to provide the full flexibility of options > fpga_bridge_info (and fpga_mgr_info earlier) could be added to the kernel-doc:: list lower down I wasn't sure if fpga_bridge_info needed to be document here since it is a temporary data structure that is used only for passing argments. The data that is passed ends up in the fpga_bridge data structure. I can add this documentation if you think it is needed. If we delete the fpga_bridge_register_full() function, then this temporary structure goes away for the fpga_bridge. >> +* fpga_bridge_register() - Create and register a bridge using standard >> + arguments >>  * fpga_bridge_unregister() - Unregister a bridge >>   .. kernel-doc:: include/linux/fpga/fpga-bridge.h >> @@ -17,7 +19,7 @@ API to implement a new FPGA bridge >>     :functions: fpga_bridge_ops >>   .. kernel-doc:: drivers/fpga/fpga-bridge.c >> -  :functions: devm_fpga_bridge_create >> +  :functions: fpga_bridge_register_full >>   .. kernel-doc:: drivers/fpga/fpga-bridge.c >>     :functions: fpga_bridge_register >> diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c >> index a78e49c63c64..66063507116b 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(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..bfbfa43cd05b 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(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..aa758426c22b 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(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..808d1f4d76df 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(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..e6d98be8531b 100644 >> --- a/drivers/fpga/fpga-bridge.c >> +++ b/drivers/fpga/fpga-bridge.c >> @@ -312,55 +312,63 @@ static struct attribute *fpga_bridge_attrs[] = { >>  ATTRIBUTE_GROUPS(fpga_bridge); >>   /** >> - * fpga_bridge_create - create and initialize a struct fpga_bridge >> + * fpga_bridge_register_full - 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_full(struct device *parent, >> +             const struct fpga_bridge_info *info) >>  { >>      struct fpga_bridge *bridge; >>      int id, ret; >>  -   if (!name || !strlen(name)) { >> +   if (!info->br_ops) { >> +       dev_err(parent, "Attempt to register without fpga_bridge_ops\n"); >> +       return ERR_PTR(-EINVAL); >> +   } >> + >> +   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,88 +376,35 @@ 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_full); >>   /** >> - * devm_fpga_bridge_create - create and init a managed 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 >>   * >> - * 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. >> + * This simple version of the register should be sufficient for most users. >> + * The fpga_bridge_register_full() function is available for users that need to pass >> + * additional, optional parameters. >>   * >> - * Return: struct fpga_bridge or NULL >> + * Return: struct fpga_bridge pointer or ERR_PTR() >>   */ >> -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 * >> +fpga_bridge_register(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); >> -   } >> +   struct fpga_bridge_info info = { 0 }; >>  -   return bridge; >> -} >> -EXPORT_SYMBOL_GPL(devm_fpga_bridge_create); >> +   info.name = name; >> +   info.br_ops = br_ops; >> +   info.priv = priv; >>  -/** >> - * fpga_bridge_register - register an FPGA bridge >> - * >> - * @bridge: FPGA bridge struct >> - * >> - * Return: 0 for success, error code otherwise. >> - */ >> -int fpga_bridge_register(struct fpga_bridge *bridge) >> -{ >> -   struct device *dev = &bridge->dev; >> -   int ret; >> - >> -   ret = device_add(dev); >> -   if (ret) >> -       return ret; >> - >> -   of_platform_populate(dev->of_node, NULL, NULL, dev); >> - >> -   dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name); >> - >> -   return 0; >> +   return fpga_bridge_register_full(parent, &info); >>  } >>  EXPORT_SYMBOL_GPL(fpga_bridge_register); >>  @@ -475,6 +430,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..c004e52b9464 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(&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..f0a4d99d9c8e 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; > > This groups seems to be unused. > > If it is, maybe it should go into info. Maybe - but that could be done as a separate patch, right? It is outside the scope of what I'm trying to do. - Russ > > Tom > >>  }; >>  +/** >> + * 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_full(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(struct device *parent, 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 */ >
On 6/30/21 10:39 PM, Xu Yilun wrote: > On Wed, Jun 30, 2021 at 06:37:32PM -0700, Russ Weight wrote: >> The FPGA bridge class driver data structure is being treated as a >> managed resource instead of using the standard dev_release call-back >> function to release the class data structure. This change removes >> the managed resource code and combines the create() and register() >> functions into a single register() or register_simple() function. >> >> The register() function accepts an info data structure to provide >> flexibility in passing optional parameters. The register_simple() >> function supports the current parameter list for users that don't >> require the use of optional parameters. > Please update the commit message, others are good to me. Will do. - Russ > > Thanks, > Yilun > >> Signed-off-by: Russ Weight <russell.h.weight@intel.com> >> Reviewed-by: Xu Yilun <yilun.xu@intel.com> >> --- >> v9: >> - Cleaned up documentation for the FPGA Bridge register functions >> - Renamed fpga_bridge_register() to fpga_bridge_register_full() >> - Renamed fpga_bridge_register_simple() to fpga_bridge_register() >> v8: >> - Added reviewed-by tag. >> - Updated Documentation/driver-api/fpga/fpga-bridge.rst documentation. >> v7: >> - Update the commit message to describe the new parameters for the >> fpga_bridge_register() function and to mention the >> fpga_bridge_register_simple() function. >> - Fix function prototypes in header file to rename dev to parent. >> - Some cleanup of comments. >> - Update function defintions/prototypes to apply const to the new info >> parameter. >> - Verify that info->br_ops is non-null in register functions. >> 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 >> --- >> Documentation/driver-api/fpga/fpga-bridge.rst | 8 +- >> 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 | 137 ++++++------------ >> drivers/fpga/xilinx-pr-decoupler.c | 17 +-- >> include/linux/fpga/fpga-bridge.h | 33 +++-- >> 8 files changed, 101 insertions(+), 138 deletions(-) >> >> diff --git a/Documentation/driver-api/fpga/fpga-bridge.rst b/Documentation/driver-api/fpga/fpga-bridge.rst >> index 8d650b4e2ce6..38e3bac8e877 100644 >> --- a/Documentation/driver-api/fpga/fpga-bridge.rst >> +++ b/Documentation/driver-api/fpga/fpga-bridge.rst >> @@ -6,8 +6,10 @@ API to implement a new FPGA bridge >> >> * struct fpga_bridge - The FPGA Bridge structure >> * struct fpga_bridge_ops - Low level Bridge driver ops >> -* devm_fpga_bridge_create() - Allocate and init a bridge struct >> -* fpga_bridge_register() - Register a bridge >> +* fpga_bridge_register_full() - Create and register a bridge using the >> + fpga_bridge_info structure to provide the full flexibility of options >> +* fpga_bridge_register() - Create and register a bridge using standard >> + arguments >> * fpga_bridge_unregister() - Unregister a bridge >> >> .. kernel-doc:: include/linux/fpga/fpga-bridge.h >> @@ -17,7 +19,7 @@ API to implement a new FPGA bridge >> :functions: fpga_bridge_ops >> >> .. kernel-doc:: drivers/fpga/fpga-bridge.c >> - :functions: devm_fpga_bridge_create >> + :functions: fpga_bridge_register_full >> >> .. kernel-doc:: drivers/fpga/fpga-bridge.c >> :functions: fpga_bridge_register >> diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c >> index a78e49c63c64..66063507116b 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(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..bfbfa43cd05b 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(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..aa758426c22b 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(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..808d1f4d76df 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(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..e6d98be8531b 100644 >> --- a/drivers/fpga/fpga-bridge.c >> +++ b/drivers/fpga/fpga-bridge.c >> @@ -312,55 +312,63 @@ static struct attribute *fpga_bridge_attrs[] = { >> ATTRIBUTE_GROUPS(fpga_bridge); >> >> /** >> - * fpga_bridge_create - create and initialize a struct fpga_bridge >> + * fpga_bridge_register_full - 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_full(struct device *parent, >> + const struct fpga_bridge_info *info) >> { >> struct fpga_bridge *bridge; >> int id, ret; >> >> - if (!name || !strlen(name)) { >> + if (!info->br_ops) { >> + dev_err(parent, "Attempt to register without fpga_bridge_ops\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + 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,88 +376,35 @@ 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_full); >> >> /** >> - * devm_fpga_bridge_create - create and init a managed 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 >> * >> - * 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. >> + * This simple version of the register should be sufficient for most users. >> + * The fpga_bridge_register_full() function is available for users that need to pass >> + * additional, optional parameters. >> * >> - * Return: struct fpga_bridge or NULL >> + * Return: struct fpga_bridge pointer or ERR_PTR() >> */ >> -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 * >> +fpga_bridge_register(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); >> - } >> + struct fpga_bridge_info info = { 0 }; >> >> - return bridge; >> -} >> -EXPORT_SYMBOL_GPL(devm_fpga_bridge_create); >> + info.name = name; >> + info.br_ops = br_ops; >> + info.priv = priv; >> >> -/** >> - * fpga_bridge_register - register an FPGA bridge >> - * >> - * @bridge: FPGA bridge struct >> - * >> - * Return: 0 for success, error code otherwise. >> - */ >> -int fpga_bridge_register(struct fpga_bridge *bridge) >> -{ >> - struct device *dev = &bridge->dev; >> - int ret; >> - >> - ret = device_add(dev); >> - if (ret) >> - return ret; >> - >> - of_platform_populate(dev->of_node, NULL, NULL, dev); >> - >> - dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name); >> - >> - return 0; >> + return fpga_bridge_register_full(parent, &info); >> } >> EXPORT_SYMBOL_GPL(fpga_bridge_register); >> >> @@ -475,6 +430,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..c004e52b9464 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(&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..f0a4d99d9c8e 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_full(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(struct device *parent, 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
On 6/30/21 11:26 PM, Xu Yilun wrote: > On Wed, Jun 30, 2021 at 06:37:32PM -0700, Russ Weight wrote: >> The FPGA bridge class driver data structure is being treated as a >> managed resource instead of using the standard dev_release call-back >> function to release the class data structure. This change removes >> the managed resource code and combines the create() and register() >> functions into a single register() or register_simple() function. >> >> The register() function accepts an info data structure to provide >> flexibility in passing optional parameters. The register_simple() >> function supports the current parameter list for users that don't >> require the use of optional parameters. >> >> Signed-off-by: Russ Weight <russell.h.weight@intel.com> >> Reviewed-by: Xu Yilun <yilun.xu@intel.com> >> --- >> v9: >> - Cleaned up documentation for the FPGA Bridge register functions >> - Renamed fpga_bridge_register() to fpga_bridge_register_full() >> - Renamed fpga_bridge_register_simple() to fpga_bridge_register() > Now we come back to the fpga_XXX_register for normal cases and > fpga_XXX_register_full for extra parameters. > > But the fpga_bridge_register_full has no use case yet, should we > implement it when it is really needed? I was just trying to maintain consistency throughout the framework. I can remove the unused fpga_bridge_register_full() function. - Russ > > Thanks, > Yilun > >> v8: >> - Added reviewed-by tag. >> - Updated Documentation/driver-api/fpga/fpga-bridge.rst documentation. >> v7: >> - Update the commit message to describe the new parameters for the >> fpga_bridge_register() function and to mention the >> fpga_bridge_register_simple() function. >> - Fix function prototypes in header file to rename dev to parent. >> - Some cleanup of comments. >> - Update function defintions/prototypes to apply const to the new info >> parameter. >> - Verify that info->br_ops is non-null in register functions. >> 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 >> --- >> Documentation/driver-api/fpga/fpga-bridge.rst | 8 +- >> 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 | 137 ++++++------------ >> drivers/fpga/xilinx-pr-decoupler.c | 17 +-- >> include/linux/fpga/fpga-bridge.h | 33 +++-- >> 8 files changed, 101 insertions(+), 138 deletions(-) >> >> diff --git a/Documentation/driver-api/fpga/fpga-bridge.rst b/Documentation/driver-api/fpga/fpga-bridge.rst >> index 8d650b4e2ce6..38e3bac8e877 100644 >> --- a/Documentation/driver-api/fpga/fpga-bridge.rst >> +++ b/Documentation/driver-api/fpga/fpga-bridge.rst >> @@ -6,8 +6,10 @@ API to implement a new FPGA bridge >> >> * struct fpga_bridge - The FPGA Bridge structure >> * struct fpga_bridge_ops - Low level Bridge driver ops >> -* devm_fpga_bridge_create() - Allocate and init a bridge struct >> -* fpga_bridge_register() - Register a bridge >> +* fpga_bridge_register_full() - Create and register a bridge using the >> + fpga_bridge_info structure to provide the full flexibility of options >> +* fpga_bridge_register() - Create and register a bridge using standard >> + arguments >> * fpga_bridge_unregister() - Unregister a bridge >> >> .. kernel-doc:: include/linux/fpga/fpga-bridge.h >> @@ -17,7 +19,7 @@ API to implement a new FPGA bridge >> :functions: fpga_bridge_ops >> >> .. kernel-doc:: drivers/fpga/fpga-bridge.c >> - :functions: devm_fpga_bridge_create >> + :functions: fpga_bridge_register_full >> >> .. kernel-doc:: drivers/fpga/fpga-bridge.c >> :functions: fpga_bridge_register >> diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c >> index a78e49c63c64..66063507116b 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(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..bfbfa43cd05b 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(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..aa758426c22b 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(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..808d1f4d76df 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(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..e6d98be8531b 100644 >> --- a/drivers/fpga/fpga-bridge.c >> +++ b/drivers/fpga/fpga-bridge.c >> @@ -312,55 +312,63 @@ static struct attribute *fpga_bridge_attrs[] = { >> ATTRIBUTE_GROUPS(fpga_bridge); >> >> /** >> - * fpga_bridge_create - create and initialize a struct fpga_bridge >> + * fpga_bridge_register_full - 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_full(struct device *parent, >> + const struct fpga_bridge_info *info) >> { >> struct fpga_bridge *bridge; >> int id, ret; >> >> - if (!name || !strlen(name)) { >> + if (!info->br_ops) { >> + dev_err(parent, "Attempt to register without fpga_bridge_ops\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + 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,88 +376,35 @@ 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_full); >> >> /** >> - * devm_fpga_bridge_create - create and init a managed 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 >> * >> - * 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. >> + * This simple version of the register should be sufficient for most users. >> + * The fpga_bridge_register_full() function is available for users that need to pass >> + * additional, optional parameters. >> * >> - * Return: struct fpga_bridge or NULL >> + * Return: struct fpga_bridge pointer or ERR_PTR() >> */ >> -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 * >> +fpga_bridge_register(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); >> - } >> + struct fpga_bridge_info info = { 0 }; >> >> - return bridge; >> -} >> -EXPORT_SYMBOL_GPL(devm_fpga_bridge_create); >> + info.name = name; >> + info.br_ops = br_ops; >> + info.priv = priv; >> >> -/** >> - * fpga_bridge_register - register an FPGA bridge >> - * >> - * @bridge: FPGA bridge struct >> - * >> - * Return: 0 for success, error code otherwise. >> - */ >> -int fpga_bridge_register(struct fpga_bridge *bridge) >> -{ >> - struct device *dev = &bridge->dev; >> - int ret; >> - >> - ret = device_add(dev); >> - if (ret) >> - return ret; >> - >> - of_platform_populate(dev->of_node, NULL, NULL, dev); >> - >> - dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name); >> - >> - return 0; >> + return fpga_bridge_register_full(parent, &info); >> } >> EXPORT_SYMBOL_GPL(fpga_bridge_register); >> >> @@ -475,6 +430,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..c004e52b9464 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(&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..f0a4d99d9c8e 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_full(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(struct device *parent, 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
diff --git a/Documentation/driver-api/fpga/fpga-bridge.rst b/Documentation/driver-api/fpga/fpga-bridge.rst index 8d650b4e2ce6..38e3bac8e877 100644 --- a/Documentation/driver-api/fpga/fpga-bridge.rst +++ b/Documentation/driver-api/fpga/fpga-bridge.rst @@ -6,8 +6,10 @@ API to implement a new FPGA bridge * struct fpga_bridge - The FPGA Bridge structure * struct fpga_bridge_ops - Low level Bridge driver ops -* devm_fpga_bridge_create() - Allocate and init a bridge struct -* fpga_bridge_register() - Register a bridge +* fpga_bridge_register_full() - Create and register a bridge using the + fpga_bridge_info structure to provide the full flexibility of options +* fpga_bridge_register() - Create and register a bridge using standard + arguments * fpga_bridge_unregister() - Unregister a bridge .. kernel-doc:: include/linux/fpga/fpga-bridge.h @@ -17,7 +19,7 @@ API to implement a new FPGA bridge :functions: fpga_bridge_ops .. kernel-doc:: drivers/fpga/fpga-bridge.c - :functions: devm_fpga_bridge_create + :functions: fpga_bridge_register_full .. kernel-doc:: drivers/fpga/fpga-bridge.c :functions: fpga_bridge_register diff --git a/drivers/fpga/altera-fpga2sdram.c b/drivers/fpga/altera-fpga2sdram.c index a78e49c63c64..66063507116b 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(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..bfbfa43cd05b 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(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..aa758426c22b 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(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..808d1f4d76df 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(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..e6d98be8531b 100644 --- a/drivers/fpga/fpga-bridge.c +++ b/drivers/fpga/fpga-bridge.c @@ -312,55 +312,63 @@ static struct attribute *fpga_bridge_attrs[] = { ATTRIBUTE_GROUPS(fpga_bridge); /** - * fpga_bridge_create - create and initialize a struct fpga_bridge + * fpga_bridge_register_full - 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_full(struct device *parent, + const struct fpga_bridge_info *info) { struct fpga_bridge *bridge; int id, ret; - if (!name || !strlen(name)) { + if (!info->br_ops) { + dev_err(parent, "Attempt to register without fpga_bridge_ops\n"); + return ERR_PTR(-EINVAL); + } + + 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,88 +376,35 @@ 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_full); /** - * devm_fpga_bridge_create - create and init a managed 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 * - * 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. + * This simple version of the register should be sufficient for most users. + * The fpga_bridge_register_full() function is available for users that need to pass + * additional, optional parameters. * - * Return: struct fpga_bridge or NULL + * Return: struct fpga_bridge pointer or ERR_PTR() */ -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 * +fpga_bridge_register(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); - } + struct fpga_bridge_info info = { 0 }; - return bridge; -} -EXPORT_SYMBOL_GPL(devm_fpga_bridge_create); + info.name = name; + info.br_ops = br_ops; + info.priv = priv; -/** - * fpga_bridge_register - register an FPGA bridge - * - * @bridge: FPGA bridge struct - * - * Return: 0 for success, error code otherwise. - */ -int fpga_bridge_register(struct fpga_bridge *bridge) -{ - struct device *dev = &bridge->dev; - int ret; - - ret = device_add(dev); - if (ret) - return ret; - - of_platform_populate(dev->of_node, NULL, NULL, dev); - - dev_info(dev->parent, "fpga bridge [%s] registered\n", bridge->name); - - return 0; + return fpga_bridge_register_full(parent, &info); } EXPORT_SYMBOL_GPL(fpga_bridge_register); @@ -475,6 +430,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..c004e52b9464 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(&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..f0a4d99d9c8e 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_full(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(struct device *parent, 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 */