Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1731851 > unrolled thread
| Started by | Alan Tull <atull@kernel.org> |
|---|---|
| First post | 2017-09-13 22:50 +0200 |
| Last post | 2017-09-13 23:00 +0200 |
| Articles | 14 on this page of 34 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v4 00/18] Enable upper layers using FPGA region w/o device tree Alan Tull <atull@kernel.org> - 2017-09-13 22:50 +0200
[PATCH v4 10/18] fpga: region: separate out code that parses the overlay Alan Tull <atull@kernel.org> - 2017-09-13 22:50 +0200
[PATCH v4 18/18] fpga: add attribute groups Alan Tull <atull@kernel.org> - 2017-09-13 22:50 +0200
[PATCH v4 16/18] fpga: of-fpga-region: accept overlays that don't program FPGA Alan Tull <atull@kernel.org> - 2017-09-13 22:50 +0200
[PATCH v4 02/18] fpga: mgr: API change to replace fpga load functions with single function Alan Tull <atull@kernel.org> - 2017-09-13 22:50 +0200
[PATCH v4 17/18] fpga: clean up fpga Kconfig Alan Tull <atull@kernel.org> - 2017-09-13 22:50 +0200
Re: [PATCH v4 17/18] fpga: clean up fpga Kconfig matthew.gerlach@linux.intel.com - 2017-09-14 18:00 +0200
Re: [PATCH v4 17/18] fpga: clean up fpga Kconfig Alan Tull <atull@kernel.org> - 2017-09-14 22:50 +0200
[PATCH v4 06/18] fpga: region: get mgr early on Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
[PATCH v4 14/18] fpga: region: add fpga_region_class_find Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
[PATCH v4 05/18] fpga: region: remove unneeded of_node_get and put Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
[PATCH v4 13/18] fpga: region: add register/unregister functions Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
Re: [PATCH v4 13/18] fpga: region: add register/unregister functions Wu Hao <hao.wu@intel.com> - 2017-09-14 12:10 +0200
Re: [PATCH v4 13/18] fpga: region: add register/unregister functions Alan Tull <atull@kernel.org> - 2017-09-14 22:10 +0200
[PATCH v4 11/18] fpga: region: add fpga-region.h header Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
Re: [PATCH v4 11/18] fpga: region: add fpga-region.h header Wu Hao <hao.wu@intel.com> - 2017-09-14 12:00 +0200
Re: [PATCH v4 11/18] fpga: region: add fpga-region.h header Alan Tull <atull@kernel.org> - 2017-09-14 21:40 +0200
[PATCH v4 15/18] fpga: region: move device tree support to of-fpga-region.c Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
Re: [PATCH v4 15/18] fpga: region: move device tree support to of-fpga-region.c matthew.gerlach@linux.intel.com - 2017-09-14 18:00 +0200
[PATCH v4 03/18] fpga: mgr: separate getting/locking FPGA manager Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
[PATCH v4 09/18] fpga: region: use image info as parameter for programming region Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
[PATCH v4 01/18] fpga: bridge: support getting bridge from device Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device matthew.gerlach@linux.intel.com - 2017-09-14 01:40 +0200
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device Alan Tull <atull@kernel.org> - 2017-09-14 21:30 +0200
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device matthew.gerlach@linux.intel.com - 2017-09-15 00:30 +0200
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device Moritz Fischer <mdf@kernel.org> - 2017-09-15 01:00 +0200
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device Alan Tull <atull@kernel.org> - 2017-09-19 18:10 +0200
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device Moritz Fischer <mdf@kernel.org> - 2017-09-18 20:00 +0200
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device Alan Tull <atull@kernel.org> - 2017-09-18 23:00 +0200
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device Moritz Fischer <mdf@kernel.org> - 2017-09-19 01:00 +0200
Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device Alan Tull <atull@kernel.org> - 2017-09-19 17:40 +0200
[PATCH v4 12/18] fpga: region: rename some functions prior to moving Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
[PATCH v4 07/18] fpga: region: check for child regions before allocing image info Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
[PATCH v4 08/18] fpga: region: fix slow warning with more than one overlay Alan Tull <atull@kernel.org> - 2017-09-13 23:00 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Alan Tull <atull@kernel.org> |
|---|---|
| Date | 2017-09-13 23:00 +0200 |
| Subject | [PATCH v4 09/18] fpga: region: use image info as parameter for programming region |
| Message-ID | <upmVk-3P1-21@gated-at.bofh.it> |
| In reply to | #1731851 |
Use FPGA image info (region->info) when region code is
programming the FPGA to pass in multiple parameters.
This is a baby step in refactoring the FPGA region code to
separate out common FPGA region code from FPGA region
Device Tree overlay support.
Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
v3: change API to use the region->info, remove info param
remove check for region->info, not needed
v4: no change to this patch in this version of patchset
---
drivers/fpga/fpga-region.c | 16 +++++++++-------
include/linux/fpga/fpga-mgr.h | 4 ++++
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 35af952..eaacf50 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -223,14 +223,13 @@ static int fpga_region_get_bridges(struct fpga_region *region,
/**
* fpga_region_program_fpga - program FPGA
* @region: FPGA region
- * @overlay: device node of the overlay
- * Program an FPGA using information in the region's fpga image info.
+ * Program an FPGA using fpga image info (region->info).
* Return 0 for success or negative error code.
*/
-static int fpga_region_program_fpga(struct fpga_region *region,
- struct device_node *overlay)
+static int fpga_region_program_fpga(struct fpga_region *region)
{
struct device *dev = ®ion->dev;
+ struct fpga_image_info *info = region->info;
int ret;
region = fpga_region_get(region);
@@ -245,7 +244,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
goto err_put_region;
}
- ret = fpga_region_get_bridges(region, overlay);
+ ret = fpga_region_get_bridges(region, info->overlay);
if (ret) {
dev_err(dev, "failed to get FPGA bridges\n");
goto err_unlock_mgr;
@@ -257,7 +256,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
goto err_put_br;
}
- ret = fpga_mgr_load(region->mgr, region->info);
+ ret = fpga_mgr_load(region->mgr, info);
if (ret) {
dev_err(dev, "failed to load FPGA image\n");
goto err_put_br;
@@ -373,6 +372,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
if (!info)
return -ENOMEM;
+ info->overlay = nd->overlay;
+
/* Read FPGA region properties from the overlay */
if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
@@ -421,7 +422,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
}
region->info = info;
- ret = fpga_region_program_fpga(region, nd->overlay);
+
+ ret = fpga_region_program_fpga(region);
if (ret) {
fpga_image_info_free(info);
region->info = NULL;
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 5e2b36e..6977a6b02 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -89,6 +89,7 @@ enum fpga_mgr_states {
* @buf: contiguous buffer containing FPGA image
* @count: size of buf
* @dev: device that owns this
+ * @overlay: Device Tree overlay
*/
struct fpga_image_info {
u32 flags;
@@ -100,6 +101,9 @@ struct fpga_image_info {
const char *buf;
size_t count;
struct device *dev;
+#ifdef CONFIG_OF
+ struct device_node *overlay;
+#endif
};
/**
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Alan Tull <atull@kernel.org> |
|---|---|
| Date | 2017-09-13 23:00 +0200 |
| Subject | [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <upmVk-3P1-25@gated-at.bofh.it> |
| In reply to | #1731851 |
Add two functions for getting the FPGA bridge from the device
rather than device tree node. This is to enable writing code
that will support using FPGA bridges without device tree.
Rename one old function to make it clear that it is device
tree-ish. This leaves us with 3 functions for getting a bridge:
* fpga_bridge_get
Get the bridge given the device.
* fpga_bridges_get_to_list
Given the device, get the bridge and add it to a list.
* of_fpga_bridges_get_to_list
Renamed from priviously existing fpga_bridges_get_to_list.
Given the device node, get the bridge and add it to a list.
Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: use list_for_each_entry
static the bridge_list_lock
update copyright and author email
v3: no change to this patch in this version of patchset
v4: no change to this patch in this version of patchset
---
drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++--------
drivers/fpga/fpga-region.c | 11 ++--
include/linux/fpga/fpga-bridge.h | 7 ++-
3 files changed, 100 insertions(+), 28 deletions(-)
diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
index fcd2bd3..af6d97e 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -2,6 +2,7 @@
* FPGA Bridge Framework Driver
*
* Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved.
+ * Copyright (C) 2017 Intel Corporation
*
* This program is free software; you can redistribute it and/or modify it
* under the terms and conditions of the GNU General Public License,
@@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge)
}
EXPORT_SYMBOL_GPL(fpga_bridge_disable);
-/**
- * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
- *
- * @np: node pointer of a FPGA bridge
- * @info: fpga image specific information
- *
- * Return fpga_bridge struct if successful.
- * Return -EBUSY if someone already has a reference to the bridge.
- * Return -ENODEV if @np is not a FPGA Bridge.
- */
-struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
- struct fpga_image_info *info)
-
+struct fpga_bridge *__fpga_bridge_get(struct device *dev,
+ struct fpga_image_info *info)
{
- struct device *dev;
struct fpga_bridge *bridge;
int ret = -ENODEV;
- dev = class_find_device(fpga_bridge_class, NULL, np,
- fpga_bridge_of_node_match);
- if (!dev)
- goto err_dev;
-
bridge = to_fpga_bridge(dev);
if (!bridge)
goto err_dev;
@@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
put_device(dev);
return ERR_PTR(ret);
}
+
+/**
+ * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
+ *
+ * @np: node pointer of a FPGA bridge
+ * @info: fpga image specific information
+ *
+ * Return fpga_bridge struct if successful.
+ * Return -EBUSY if someone already has a reference to the bridge.
+ * Return -ENODEV if @np is not a FPGA Bridge.
+ */
+struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
+ struct fpga_image_info *info)
+{
+ struct device *dev;
+
+ dev = class_find_device(fpga_bridge_class, NULL, np,
+ fpga_bridge_of_node_match);
+ if (!dev)
+ return ERR_PTR(-ENODEV);
+
+ return __fpga_bridge_get(dev, info);
+}
EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
+static int fpga_bridge_dev_match(struct device *dev, const void *data)
+{
+ return dev->parent == data;
+}
+
+/**
+ * fpga_bridge_get - get an exclusive reference to a fpga bridge
+ * @dev: parent device that fpga bridge was registered with
+ *
+ * Given a device, get an exclusive reference to a fpga bridge.
+ *
+ * Return: fpga manager struct or IS_ERR() condition containing error code.
+ */
+struct fpga_bridge *fpga_bridge_get(struct device *dev,
+ struct fpga_image_info *info)
+{
+ struct device *bridge_dev;
+
+ bridge_dev = class_find_device(fpga_bridge_class, NULL, dev,
+ fpga_bridge_dev_match);
+ if (!bridge_dev)
+ return ERR_PTR(-ENODEV);
+
+ return __fpga_bridge_get(bridge_dev, info);
+}
+EXPORT_SYMBOL_GPL(fpga_bridge_get);
+
/**
* fpga_bridge_put - release a reference to a bridge
*
@@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)
EXPORT_SYMBOL_GPL(fpga_bridges_put);
/**
- * fpga_bridges_get_to_list - get a bridge, add it to a list
+ * of_fpga_bridge_get_to_list - get a bridge, add it to a list
*
* @np: node pointer of a FPGA bridge
* @info: fpga image specific information
@@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
*
* Return 0 for success, error code from of_fpga_bridge_get() othewise.
*/
-int fpga_bridge_get_to_list(struct device_node *np,
+int of_fpga_bridge_get_to_list(struct device_node *np,
+ struct fpga_image_info *info,
+ struct list_head *bridge_list)
+{
+ struct fpga_bridge *bridge;
+ unsigned long flags;
+
+ bridge = of_fpga_bridge_get(np, info);
+ if (IS_ERR(bridge))
+ return PTR_ERR(bridge);
+
+ spin_lock_irqsave(&bridge_list_lock, flags);
+ list_add(&bridge->node, bridge_list);
+ spin_unlock_irqrestore(&bridge_list_lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_fpga_bridge_get_to_list);
+
+/**
+ * fpga_bridge_get_to_list - given device, get a bridge, add it to a list
+ *
+ * @dev: FPGA bridge device
+ * @info: fpga image specific information
+ * @bridge_list: list of FPGA bridges
+ *
+ * Get an exclusive reference to the bridge and and it to the list.
+ *
+ * Return 0 for success, error code from fpga_bridge_get() othewise.
+ */
+int fpga_bridge_get_to_list(struct device *dev,
struct fpga_image_info *info,
struct list_head *bridge_list)
{
struct fpga_bridge *bridge;
unsigned long flags;
- bridge = of_fpga_bridge_get(np, info);
+ bridge = fpga_bridge_get(dev, info);
if (IS_ERR(bridge))
return PTR_ERR(bridge);
@@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void)
}
MODULE_DESCRIPTION("FPGA Bridge Driver");
-MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
MODULE_LICENSE("GPL v2");
subsys_initcall(fpga_bridge_dev_init);
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index d9ab7c7..91755562 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,
int i, ret;
/* If parent is a bridge, add to list */
- ret = fpga_bridge_get_to_list(region_np->parent, region->info,
- ®ion->bridge_list);
+ ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
+ ®ion->bridge_list);
+
+ /* -EBUSY means parent is a bridge that is under use. Give up. */
if (ret == -EBUSY)
return ret;
+ /* Zero return code means parent was a bridge and was added to list. */
if (!ret)
parent_br = region_np->parent;
@@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
continue;
/* If node is a bridge, get it and add to list */
- ret = fpga_bridge_get_to_list(br, region->info,
- ®ion->bridge_list);
+ ret = of_fpga_bridge_get_to_list(br, region->info,
+ ®ion->bridge_list);
/* If any of the bridges are in use, give up */
if (ret == -EBUSY) {
diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
index dba6e3c..9f6696b 100644
--- a/include/linux/fpga/fpga-bridge.h
+++ b/include/linux/fpga/fpga-bridge.h
@@ -42,6 +42,8 @@ struct fpga_bridge {
struct fpga_bridge *of_fpga_bridge_get(struct device_node *node,
struct fpga_image_info *info);
+struct fpga_bridge *fpga_bridge_get(struct device *dev,
+ struct fpga_image_info *info);
void fpga_bridge_put(struct fpga_bridge *bridge);
int fpga_bridge_enable(struct fpga_bridge *bridge);
int fpga_bridge_disable(struct fpga_bridge *bridge);
@@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);
int fpga_bridges_enable(struct list_head *bridge_list);
int fpga_bridges_disable(struct list_head *bridge_list);
void fpga_bridges_put(struct list_head *bridge_list);
-int fpga_bridge_get_to_list(struct device_node *np,
+int fpga_bridge_get_to_list(struct device *dev,
struct fpga_image_info *info,
struct list_head *bridge_list);
+int of_fpga_bridge_get_to_list(struct device_node *np,
+ struct fpga_image_info *info,
+ struct list_head *bridge_list);
int fpga_bridge_register(struct device *dev, const char *name,
const struct fpga_bridge_ops *br_ops, void *priv);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | matthew.gerlach@linux.intel.com |
|---|---|
| Date | 2017-09-14 01:40 +0200 |
| Subject | Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <uppqa-5wg-5@gated-at.bofh.it> |
| In reply to | #1731869 |
Hi Alan,
Two minor nits below.
Matthew Gerlach
On Wed, 13 Sep 2017, Alan Tull wrote:
> Add two functions for getting the FPGA bridge from the device
> rather than device tree node. This is to enable writing code
> that will support using FPGA bridges without device tree.
> Rename one old function to make it clear that it is device
> tree-ish. This leaves us with 3 functions for getting a bridge:
>
> * fpga_bridge_get
> Get the bridge given the device.
>
> * fpga_bridges_get_to_list
> Given the device, get the bridge and add it to a list.
>
> * of_fpga_bridges_get_to_list
> Renamed from priviously existing fpga_bridges_get_to_list.
> Given the device node, get the bridge and add it to a list.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> v2: use list_for_each_entry
> static the bridge_list_lock
> update copyright and author email
> v3: no change to this patch in this version of patchset
> v4: no change to this patch in this version of patchset
> ---
> drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++--------
> drivers/fpga/fpga-region.c | 11 ++--
> include/linux/fpga/fpga-bridge.h | 7 ++-
> 3 files changed, 100 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> index fcd2bd3..af6d97e 100644
> --- a/drivers/fpga/fpga-bridge.c
> +++ b/drivers/fpga/fpga-bridge.c
> @@ -2,6 +2,7 @@
> * FPGA Bridge Framework Driver
> *
> * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved.
> + * Copyright (C) 2017 Intel Corporation
> *
> * This program is free software; you can redistribute it and/or modify it
> * under the terms and conditions of the GNU General Public License,
> @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge)
> }
> EXPORT_SYMBOL_GPL(fpga_bridge_disable);
>
> -/**
> - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
> - *
> - * @np: node pointer of a FPGA bridge
> - * @info: fpga image specific information
> - *
> - * Return fpga_bridge struct if successful.
> - * Return -EBUSY if someone already has a reference to the bridge.
> - * Return -ENODEV if @np is not a FPGA Bridge.
> - */
> -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> - struct fpga_image_info *info)
> -
> +struct fpga_bridge *__fpga_bridge_get(struct device *dev,
> + struct fpga_image_info *info)
Should this be a static function?
I was recently told by mtd maintainers that function names prefixed with
__ should be avoided.
> {
> - struct device *dev;
> struct fpga_bridge *bridge;
> int ret = -ENODEV;
>
> - dev = class_find_device(fpga_bridge_class, NULL, np,
> - fpga_bridge_of_node_match);
> - if (!dev)
> - goto err_dev;
> -
> bridge = to_fpga_bridge(dev);
> if (!bridge)
> goto err_dev;
> @@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> put_device(dev);
> return ERR_PTR(ret);
> }
> +
> +/**
> + * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
> + *
> + * @np: node pointer of a FPGA bridge
> + * @info: fpga image specific information
> + *
> + * Return fpga_bridge struct if successful.
> + * Return -EBUSY if someone already has a reference to the bridge.
> + * Return -ENODEV if @np is not a FPGA Bridge.
> + */
> +struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> + struct fpga_image_info *info)
> +{
> + struct device *dev;
> +
> + dev = class_find_device(fpga_bridge_class, NULL, np,
> + fpga_bridge_of_node_match);
> + if (!dev)
> + return ERR_PTR(-ENODEV);
> +
> + return __fpga_bridge_get(dev, info);
> +}
> EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
>
> +static int fpga_bridge_dev_match(struct device *dev, const void *data)
> +{
> + return dev->parent == data;
> +}
> +
> +/**
> + * fpga_bridge_get - get an exclusive reference to a fpga bridge
> + * @dev: parent device that fpga bridge was registered with
> + *
> + * Given a device, get an exclusive reference to a fpga bridge.
> + *
> + * Return: fpga manager struct or IS_ERR() condition containing error code.
> + */
> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
> + struct fpga_image_info *info)
> +{
> + struct device *bridge_dev;
> +
> + bridge_dev = class_find_device(fpga_bridge_class, NULL, dev,
> + fpga_bridge_dev_match);
> + if (!bridge_dev)
> + return ERR_PTR(-ENODEV);
> +
> + return __fpga_bridge_get(bridge_dev, info);
> +}
> +EXPORT_SYMBOL_GPL(fpga_bridge_get);
> +
> /**
> * fpga_bridge_put - release a reference to a bridge
> *
> @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)
> EXPORT_SYMBOL_GPL(fpga_bridges_put);
>
> /**
> - * fpga_bridges_get_to_list - get a bridge, add it to a list
> + * of_fpga_bridge_get_to_list - get a bridge, add it to a list
> *
> * @np: node pointer of a FPGA bridge
> * @info: fpga image specific information
> @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
> *
> * Return 0 for success, error code from of_fpga_bridge_get() othewise.
> */
> -int fpga_bridge_get_to_list(struct device_node *np,
> +int of_fpga_bridge_get_to_list(struct device_node *np,
> + struct fpga_image_info *info,
> + struct list_head *bridge_list)
> +{
> + struct fpga_bridge *bridge;
> + unsigned long flags;
> +
> + bridge = of_fpga_bridge_get(np, info);
> + if (IS_ERR(bridge))
> + return PTR_ERR(bridge);
> +
> + spin_lock_irqsave(&bridge_list_lock, flags);
> + list_add(&bridge->node, bridge_list);
> + spin_unlock_irqrestore(&bridge_list_lock, flags);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_fpga_bridge_get_to_list);
> +
> +/**
> + * fpga_bridge_get_to_list - given device, get a bridge, add it to a list
> + *
> + * @dev: FPGA bridge device
> + * @info: fpga image specific information
> + * @bridge_list: list of FPGA bridges
> + *
> + * Get an exclusive reference to the bridge and and it to the list.
> + *
> + * Return 0 for success, error code from fpga_bridge_get() othewise.
> + */
> +int fpga_bridge_get_to_list(struct device *dev,
> struct fpga_image_info *info,
> struct list_head *bridge_list)
> {
> struct fpga_bridge *bridge;
> unsigned long flags;
>
> - bridge = of_fpga_bridge_get(np, info);
> + bridge = fpga_bridge_get(dev, info);
> if (IS_ERR(bridge))
> return PTR_ERR(bridge);
>
> @@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void)
> }
>
> MODULE_DESCRIPTION("FPGA Bridge Driver");
> -MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> +MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
> MODULE_LICENSE("GPL v2");
>
> subsys_initcall(fpga_bridge_dev_init);
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index d9ab7c7..91755562 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> int i, ret;
>
> /* If parent is a bridge, add to list */
> - ret = fpga_bridge_get_to_list(region_np->parent, region->info,
> - ®ion->bridge_list);
> + ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
> + ®ion->bridge_list);
> +
> + /* -EBUSY means parent is a bridge that is under use. Give up. */
> if (ret == -EBUSY)
> return ret;
>
> + /* Zero return code means parent was a bridge and was added to list. */
> if (!ret)
> parent_br = region_np->parent;
>
> @@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> continue;
>
> /* If node is a bridge, get it and add to list */
> - ret = fpga_bridge_get_to_list(br, region->info,
> - ®ion->bridge_list);
> + ret = of_fpga_bridge_get_to_list(br, region->info,
> + ®ion->bridge_list);
>
> /* If any of the bridges are in use, give up */
> if (ret == -EBUSY) {
> diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
> index dba6e3c..9f6696b 100644
> --- a/include/linux/fpga/fpga-bridge.h
> +++ b/include/linux/fpga/fpga-bridge.h
> @@ -42,6 +42,8 @@ struct fpga_bridge {
>
> struct fpga_bridge *of_fpga_bridge_get(struct device_node *node,
> struct fpga_image_info *info);
> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
> + struct fpga_image_info *info);
> void fpga_bridge_put(struct fpga_bridge *bridge);
> int fpga_bridge_enable(struct fpga_bridge *bridge);
> int fpga_bridge_disable(struct fpga_bridge *bridge);
> @@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);
> int fpga_bridges_enable(struct list_head *bridge_list);
> int fpga_bridges_disable(struct list_head *bridge_list);
> void fpga_bridges_put(struct list_head *bridge_list);
> -int fpga_bridge_get_to_list(struct device_node *np,
> +int fpga_bridge_get_to_list(struct device *dev,
> struct fpga_image_info *info,
> struct list_head *bridge_list);
> +int of_fpga_bridge_get_to_list(struct device_node *np,
> + struct fpga_image_info *info,
> + struct list_head *bridge_list);
>
> int fpga_bridge_register(struct device *dev, const char *name,
> const struct fpga_bridge_ops *br_ops, void *priv);
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
[toc] | [prev] | [next] | [standalone]
| From | Alan Tull <atull@kernel.org> |
|---|---|
| Date | 2017-09-14 21:30 +0200 |
| Subject | Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <upHZL-GM-1@gated-at.bofh.it> |
| In reply to | #1731985 |
On Wed, Sep 13, 2017 at 6:38 PM, <matthew.gerlach@linux.intel.com> wrote: Hi Matthew, > > Hi Alan, > > Two minor nits below. > > Matthew Gerlach > > On Wed, 13 Sep 2017, Alan Tull wrote: > >> Add two functions for getting the FPGA bridge from the device >> rather than device tree node. This is to enable writing code >> that will support using FPGA bridges without device tree. >> Rename one old function to make it clear that it is device >> tree-ish. This leaves us with 3 functions for getting a bridge: >> >> * fpga_bridge_get >> Get the bridge given the device. >> >> * fpga_bridges_get_to_list >> Given the device, get the bridge and add it to a list. >> >> * of_fpga_bridges_get_to_list >> Renamed from priviously existing fpga_bridges_get_to_list. >> Given the device node, get the bridge and add it to a list. >> >> Signed-off-by: Alan Tull <atull@kernel.org> >> --- >> v2: use list_for_each_entry >> static the bridge_list_lock >> update copyright and author email >> v3: no change to this patch in this version of patchset >> v4: no change to this patch in this version of patchset >> --- >> drivers/fpga/fpga-bridge.c | 110 >> +++++++++++++++++++++++++++++++-------- >> drivers/fpga/fpga-region.c | 11 ++-- >> include/linux/fpga/fpga-bridge.h | 7 ++- >> 3 files changed, 100 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c >> index fcd2bd3..af6d97e 100644 >> --- a/drivers/fpga/fpga-bridge.c >> +++ b/drivers/fpga/fpga-bridge.c >> @@ -2,6 +2,7 @@ >> * FPGA Bridge Framework Driver >> * >> * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. >> + * Copyright (C) 2017 Intel Corporation >> * >> * This program is free software; you can redistribute it and/or modify it >> * under the terms and conditions of the GNU General Public License, >> @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) >> } >> EXPORT_SYMBOL_GPL(fpga_bridge_disable); >> >> -/** >> - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge >> - * >> - * @np: node pointer of a FPGA bridge >> - * @info: fpga image specific information >> - * >> - * Return fpga_bridge struct if successful. >> - * Return -EBUSY if someone already has a reference to the bridge. >> - * Return -ENODEV if @np is not a FPGA Bridge. >> - */ >> -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, >> - struct fpga_image_info *info) >> - >> +struct fpga_bridge *__fpga_bridge_get(struct device *dev, >> + struct fpga_image_info *info) > > > Should this be a static function? You are right. Will fix in v5. > > I was recently told by mtd maintainers that function names prefixed with > __ should be avoided. I see functions named thusly around in the kernel. Can you point me to that thread or let me know what their thinking was about this? I am open for suggestions for a new function name. Alan
[toc] | [prev] | [next] | [standalone]
| From | matthew.gerlach@linux.intel.com |
|---|---|
| Date | 2017-09-15 00:30 +0200 |
| Subject | Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <upKNY-2pG-11@gated-at.bofh.it> |
| In reply to | #1732528 |
On Thu, 14 Sep 2017, Alan Tull wrote: > On Wed, Sep 13, 2017 at 6:38 PM, <matthew.gerlach@linux.intel.com> wrote: > > Hi Matthew, > >> >> Hi Alan, >> >> Two minor nits below. >> >> Matthew Gerlach >> >> On Wed, 13 Sep 2017, Alan Tull wrote: >> >>> Add two functions for getting the FPGA bridge from the device >>> rather than device tree node. This is to enable writing code >>> that will support using FPGA bridges without device tree. >>> Rename one old function to make it clear that it is device >>> tree-ish. This leaves us with 3 functions for getting a bridge: >>> >>> * fpga_bridge_get >>> Get the bridge given the device. >>> >>> * fpga_bridges_get_to_list >>> Given the device, get the bridge and add it to a list. >>> >>> * of_fpga_bridges_get_to_list >>> Renamed from priviously existing fpga_bridges_get_to_list. >>> Given the device node, get the bridge and add it to a list. >>> >>> Signed-off-by: Alan Tull <atull@kernel.org> >>> --- >>> v2: use list_for_each_entry >>> static the bridge_list_lock >>> update copyright and author email >>> v3: no change to this patch in this version of patchset >>> v4: no change to this patch in this version of patchset >>> --- >>> drivers/fpga/fpga-bridge.c | 110 >>> +++++++++++++++++++++++++++++++-------- >>> drivers/fpga/fpga-region.c | 11 ++-- >>> include/linux/fpga/fpga-bridge.h | 7 ++- >>> 3 files changed, 100 insertions(+), 28 deletions(-) >>> >>> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c >>> index fcd2bd3..af6d97e 100644 >>> --- a/drivers/fpga/fpga-bridge.c >>> +++ b/drivers/fpga/fpga-bridge.c >>> @@ -2,6 +2,7 @@ >>> * FPGA Bridge Framework Driver >>> * >>> * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. >>> + * Copyright (C) 2017 Intel Corporation >>> * >>> * This program is free software; you can redistribute it and/or modify it >>> * under the terms and conditions of the GNU General Public License, >>> @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) >>> } >>> EXPORT_SYMBOL_GPL(fpga_bridge_disable); >>> >>> -/** >>> - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge >>> - * >>> - * @np: node pointer of a FPGA bridge >>> - * @info: fpga image specific information >>> - * >>> - * Return fpga_bridge struct if successful. >>> - * Return -EBUSY if someone already has a reference to the bridge. >>> - * Return -ENODEV if @np is not a FPGA Bridge. >>> - */ >>> -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, >>> - struct fpga_image_info *info) >>> - >>> +struct fpga_bridge *__fpga_bridge_get(struct device *dev, >>> + struct fpga_image_info *info) >> >> >> Should this be a static function? > > You are right. Will fix in v5. > >> >> I was recently told by mtd maintainers that function names prefixed with >> __ should be avoided. > > I see functions named thusly around in the kernel. Can you point me > to that thread or let me know what their thinking was about this? I > am open for suggestions for a new function name. Marek Vasut just told me to "Avoid function names with __ prefix" in https://patchwork.kernel.org/patch/9883977/ I tend to agree with you that the __ prefix seems to be around, but this could be a case of "evolution" of coding style. Historically, __ seems to mean an internal version of an external function. If __ is to be avoided, we might have to rename to fpga_bridge_get_internal(). > > Alan > -- > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
[toc] | [prev] | [next] | [standalone]
| From | Moritz Fischer <mdf@kernel.org> |
|---|---|
| Date | 2017-09-15 01:00 +0200 |
| Subject | Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <upLgZ-2Ak-9@gated-at.bofh.it> |
| In reply to | #1732614 |
On Thu, Sep 14, 2017 at 03:29:09PM -0700, matthew.gerlach@linux.intel.com wrote: > > > On Thu, 14 Sep 2017, Alan Tull wrote: > > > On Wed, Sep 13, 2017 at 6:38 PM, <matthew.gerlach@linux.intel.com> wrote: > > > > Hi Matthew, > > > > > > > > Hi Alan, > > > > > > Two minor nits below. > > > > > > Matthew Gerlach > > > > > > On Wed, 13 Sep 2017, Alan Tull wrote: > > > > > > > Add two functions for getting the FPGA bridge from the device > > > > rather than device tree node. This is to enable writing code > > > > that will support using FPGA bridges without device tree. > > > > Rename one old function to make it clear that it is device > > > > tree-ish. This leaves us with 3 functions for getting a bridge: > > > > > > > > * fpga_bridge_get > > > > Get the bridge given the device. > > > > > > > > * fpga_bridges_get_to_list > > > > Given the device, get the bridge and add it to a list. > > > > > > > > * of_fpga_bridges_get_to_list > > > > Renamed from priviously existing fpga_bridges_get_to_list. > > > > Given the device node, get the bridge and add it to a list. > > > > > > > > Signed-off-by: Alan Tull <atull@kernel.org> > > > > --- > > > > v2: use list_for_each_entry > > > > static the bridge_list_lock > > > > update copyright and author email > > > > v3: no change to this patch in this version of patchset > > > > v4: no change to this patch in this version of patchset > > > > --- > > > > drivers/fpga/fpga-bridge.c | 110 > > > > +++++++++++++++++++++++++++++++-------- > > > > drivers/fpga/fpga-region.c | 11 ++-- > > > > include/linux/fpga/fpga-bridge.h | 7 ++- > > > > 3 files changed, 100 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c > > > > index fcd2bd3..af6d97e 100644 > > > > --- a/drivers/fpga/fpga-bridge.c > > > > +++ b/drivers/fpga/fpga-bridge.c > > > > @@ -2,6 +2,7 @@ > > > > * FPGA Bridge Framework Driver > > > > * > > > > * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. > > > > + * Copyright (C) 2017 Intel Corporation > > > > * > > > > * This program is free software; you can redistribute it and/or modify it > > > > * under the terms and conditions of the GNU General Public License, > > > > @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) > > > > } > > > > EXPORT_SYMBOL_GPL(fpga_bridge_disable); > > > > > > > > -/** > > > > - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge > > > > - * > > > > - * @np: node pointer of a FPGA bridge > > > > - * @info: fpga image specific information > > > > - * > > > > - * Return fpga_bridge struct if successful. > > > > - * Return -EBUSY if someone already has a reference to the bridge. > > > > - * Return -ENODEV if @np is not a FPGA Bridge. > > > > - */ > > > > -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, > > > > - struct fpga_image_info *info) > > > > - > > > > +struct fpga_bridge *__fpga_bridge_get(struct device *dev, > > > > + struct fpga_image_info *info) > > > > > > > > > Should this be a static function? > > > > You are right. Will fix in v5. > > > > > > > > I was recently told by mtd maintainers that function names prefixed with > > > __ should be avoided. > > > > I see functions named thusly around in the kernel. Can you point me > > to that thread or let me know what their thinking was about this? I > > am open for suggestions for a new function name. > > Marek Vasut just told me to "Avoid function names with __ prefix" in > > https://patchwork.kernel.org/patch/9883977/ > > I tend to agree with you that the __ prefix seems to be around, but > this could be a case of "evolution" of coding style. Historically, > __ seems to mean an internal version of an external function. If __ > is to be avoided, we might have to rename to fpga_bridge_get_internal(). Bear in mind that coding style can be subsystem specific. What applies to MTD doesn't necessarily have to apply here. > > > > > Alan > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > Thanks, Moritz PS: I'm travelling this week, will get around to reviews next week
[toc] | [prev] | [next] | [standalone]
| From | Alan Tull <atull@kernel.org> |
|---|---|
| Date | 2017-09-19 18:10 +0200 |
| Subject | Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <urtfY-7CA-29@gated-at.bofh.it> |
| In reply to | #1732633 |
On Thu, Sep 14, 2017 at 5:54 PM, Moritz Fischer <mdf@kernel.org> wrote: > On Thu, Sep 14, 2017 at 03:29:09PM -0700, matthew.gerlach@linux.intel.com wrote: >> >> >> On Thu, 14 Sep 2017, Alan Tull wrote: >> >> > On Wed, Sep 13, 2017 at 6:38 PM, <matthew.gerlach@linux.intel.com> wrote: >> > >> > Hi Matthew, >> > >> > > >> > > Hi Alan, >> > > >> > > Two minor nits below. >> > > >> > > Matthew Gerlach >> > > >> > > On Wed, 13 Sep 2017, Alan Tull wrote: >> > > >> > > > Add two functions for getting the FPGA bridge from the device >> > > > rather than device tree node. This is to enable writing code >> > > > that will support using FPGA bridges without device tree. >> > > > Rename one old function to make it clear that it is device >> > > > tree-ish. This leaves us with 3 functions for getting a bridge: >> > > > >> > > > * fpga_bridge_get >> > > > Get the bridge given the device. >> > > > >> > > > * fpga_bridges_get_to_list >> > > > Given the device, get the bridge and add it to a list. >> > > > >> > > > * of_fpga_bridges_get_to_list >> > > > Renamed from priviously existing fpga_bridges_get_to_list. >> > > > Given the device node, get the bridge and add it to a list. >> > > > >> > > > Signed-off-by: Alan Tull <atull@kernel.org> >> > > > --- >> > > > v2: use list_for_each_entry >> > > > static the bridge_list_lock >> > > > update copyright and author email >> > > > v3: no change to this patch in this version of patchset >> > > > v4: no change to this patch in this version of patchset >> > > > --- >> > > > drivers/fpga/fpga-bridge.c | 110 >> > > > +++++++++++++++++++++++++++++++-------- >> > > > drivers/fpga/fpga-region.c | 11 ++-- >> > > > include/linux/fpga/fpga-bridge.h | 7 ++- >> > > > 3 files changed, 100 insertions(+), 28 deletions(-) >> > > > >> > > > diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c >> > > > index fcd2bd3..af6d97e 100644 >> > > > --- a/drivers/fpga/fpga-bridge.c >> > > > +++ b/drivers/fpga/fpga-bridge.c >> > > > @@ -2,6 +2,7 @@ >> > > > * FPGA Bridge Framework Driver >> > > > * >> > > > * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. >> > > > + * Copyright (C) 2017 Intel Corporation >> > > > * >> > > > * This program is free software; you can redistribute it and/or modify it >> > > > * under the terms and conditions of the GNU General Public License, >> > > > @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) >> > > > } >> > > > EXPORT_SYMBOL_GPL(fpga_bridge_disable); >> > > > >> > > > -/** >> > > > - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge >> > > > - * >> > > > - * @np: node pointer of a FPGA bridge >> > > > - * @info: fpga image specific information >> > > > - * >> > > > - * Return fpga_bridge struct if successful. >> > > > - * Return -EBUSY if someone already has a reference to the bridge. >> > > > - * Return -ENODEV if @np is not a FPGA Bridge. >> > > > - */ >> > > > -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, >> > > > - struct fpga_image_info *info) >> > > > - >> > > > +struct fpga_bridge *__fpga_bridge_get(struct device *dev, >> > > > + struct fpga_image_info *info) >> > > >> > > >> > > Should this be a static function? >> > >> > You are right. Will fix in v5. >> > >> > > >> > > I was recently told by mtd maintainers that function names prefixed with >> > > __ should be avoided. >> > >> > I see functions named thusly around in the kernel. Can you point me >> > to that thread or let me know what their thinking was about this? I >> > am open for suggestions for a new function name. >> >> Marek Vasut just told me to "Avoid function names with __ prefix" in >> >> https://patchwork.kernel.org/patch/9883977/ >> >> I tend to agree with you that the __ prefix seems to be around, but >> this could be a case of "evolution" of coding style. Historically, >> __ seems to mean an internal version of an external function. If __ >> is to be avoided, we might have to rename to fpga_bridge_get_internal(). > > Bear in mind that coding style can be subsystem specific. What applies > to MTD doesn't necessarily have to apply here. Yes I agree. I'll keep the __ name unless someone gives us good explanation of the naming practice they are recommending. But yes, it should have been static. Alan
[toc] | [prev] | [next] | [standalone]
| From | Moritz Fischer <mdf@kernel.org> |
|---|---|
| Date | 2017-09-18 20:00 +0200 |
| Subject | Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <ur8uS-13C-19@gated-at.bofh.it> |
| In reply to | #1731869 |
[Multipart message — attachments visible in raw view] — view raw
On Wed, Sep 13, 2017 at 03:48:24PM -0500, Alan Tull wrote:
> Add two functions for getting the FPGA bridge from the device
> rather than device tree node. This is to enable writing code
> that will support using FPGA bridges without device tree.
> Rename one old function to make it clear that it is device
> tree-ish. This leaves us with 3 functions for getting a bridge:
>
> * fpga_bridge_get
> Get the bridge given the device.
>
> * fpga_bridges_get_to_list
> Given the device, get the bridge and add it to a list.
>
> * of_fpga_bridges_get_to_list
> Renamed from priviously existing fpga_bridges_get_to_list.
> Given the device node, get the bridge and add it to a list.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> v2: use list_for_each_entry
> static the bridge_list_lock
> update copyright and author email
> v3: no change to this patch in this version of patchset
> v4: no change to this patch in this version of patchset
> ---
> drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++--------
> drivers/fpga/fpga-region.c | 11 ++--
> include/linux/fpga/fpga-bridge.h | 7 ++-
> 3 files changed, 100 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> index fcd2bd3..af6d97e 100644
> --- a/drivers/fpga/fpga-bridge.c
> +++ b/drivers/fpga/fpga-bridge.c
> @@ -2,6 +2,7 @@
> * FPGA Bridge Framework Driver
> *
> * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved.
> + * Copyright (C) 2017 Intel Corporation
> *
> * This program is free software; you can redistribute it and/or modify it
> * under the terms and conditions of the GNU General Public License,
> @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge)
> }
> EXPORT_SYMBOL_GPL(fpga_bridge_disable);
>
> -/**
> - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
> - *
> - * @np: node pointer of a FPGA bridge
> - * @info: fpga image specific information
> - *
> - * Return fpga_bridge struct if successful.
> - * Return -EBUSY if someone already has a reference to the bridge.
> - * Return -ENODEV if @np is not a FPGA Bridge.
> - */
> -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> - struct fpga_image_info *info)
> -
> +struct fpga_bridge *__fpga_bridge_get(struct device *dev,
> + struct fpga_image_info *info)
> {
> - struct device *dev;
> struct fpga_bridge *bridge;
> int ret = -ENODEV;
>
> - dev = class_find_device(fpga_bridge_class, NULL, np,
> - fpga_bridge_of_node_match);
> - if (!dev)
> - goto err_dev;
> -
> bridge = to_fpga_bridge(dev);
> if (!bridge)
> goto err_dev;
> @@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> put_device(dev);
> return ERR_PTR(ret);
> }
> +
> +/**
> + * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
> + *
> + * @np: node pointer of a FPGA bridge
> + * @info: fpga image specific information
> + *
> + * Return fpga_bridge struct if successful.
> + * Return -EBUSY if someone already has a reference to the bridge.
> + * Return -ENODEV if @np is not a FPGA Bridge.
> + */
> +struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> + struct fpga_image_info *info)
> +{
> + struct device *dev;
> +
> + dev = class_find_device(fpga_bridge_class, NULL, np,
> + fpga_bridge_of_node_match);
> + if (!dev)
> + return ERR_PTR(-ENODEV);
> +
> + return __fpga_bridge_get(dev, info);
> +}
> EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
>
> +static int fpga_bridge_dev_match(struct device *dev, const void *data)
> +{
> + return dev->parent == data;
> +}
> +
> +/**
> + * fpga_bridge_get - get an exclusive reference to a fpga bridge
> + * @dev: parent device that fpga bridge was registered with
> + *
> + * Given a device, get an exclusive reference to a fpga bridge.
> + *
> + * Return: fpga manager struct or IS_ERR() condition containing error code.
> + */
> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
> + struct fpga_image_info *info)
> +{
> + struct device *bridge_dev;
> +
> + bridge_dev = class_find_device(fpga_bridge_class, NULL, dev,
> + fpga_bridge_dev_match);
> + if (!bridge_dev)
> + return ERR_PTR(-ENODEV);
> +
> + return __fpga_bridge_get(bridge_dev, info);
> +}
> +EXPORT_SYMBOL_GPL(fpga_bridge_get);
> +
> /**
> * fpga_bridge_put - release a reference to a bridge
> *
> @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)
> EXPORT_SYMBOL_GPL(fpga_bridges_put);
>
> /**
> - * fpga_bridges_get_to_list - get a bridge, add it to a list
> + * of_fpga_bridge_get_to_list - get a bridge, add it to a list
> *
> * @np: node pointer of a FPGA bridge
> * @info: fpga image specific information
> @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
> *
> * Return 0 for success, error code from of_fpga_bridge_get() othewise.
> */
> -int fpga_bridge_get_to_list(struct device_node *np,
> +int of_fpga_bridge_get_to_list(struct device_node *np,
> + struct fpga_image_info *info,
> + struct list_head *bridge_list)
> +{
> + struct fpga_bridge *bridge;
> + unsigned long flags;
> +
> + bridge = of_fpga_bridge_get(np, info);
> + if (IS_ERR(bridge))
> + return PTR_ERR(bridge);
> +
> + spin_lock_irqsave(&bridge_list_lock, flags);
> + list_add(&bridge->node, bridge_list);
> + spin_unlock_irqrestore(&bridge_list_lock, flags);
I know this is not new code, but I was wondering the other day:
Why are we using a single spinlock to protect all lists that are being
passed in as parameters here?
I have a patch that moves the spinlock into the region containing the
list that uses the bridges which (unless I misunderstand something
here), makes more sense:
diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
index 9651aa56244a..b03ec59448e2 100644
--- a/drivers/fpga/fpga-bridge.c
+++ b/drivers/fpga/fpga-bridge.c
@@ -214,6 +214,8 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
*
* Get an exclusive reference to the bridge and and it to the list.
*
+ * Must be called with list lock held.
+ *
* Return 0 for success, error code from of_fpga_bridge_get() othewise.
*/
int fpga_bridge_get_to_list(struct device_node *np,
@@ -221,15 +223,12 @@ int fpga_bridge_get_to_list(struct device_node *np,
struct list_head *bridge_list)
{
struct fpga_bridge *bridge;
- unsigned long flags;
bridge = of_fpga_bridge_get(np, info);
if (IS_ERR(bridge))
return PTR_ERR(bridge);
- spin_lock_irqsave(&bridge_list_lock, flags);
list_add(&bridge->node, bridge_list);
- spin_unlock_irqrestore(&bridge_list_lock, flags);
return 0;
}
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 3b6b2f4182a1..c5c958e0e601 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -37,6 +37,7 @@ struct fpga_region {
struct device dev;
struct mutex mutex; /* for exclusive reference to region */
struct list_head bridge_list;
+ spinlock_t bridge_list_lock; /* protects access to bridge list */
struct fpga_image_info *info;
};
@@ -180,11 +181,15 @@ static int fpga_region_get_bridges(struct fpga_region *region,
struct device *dev = ®ion->dev;
struct device_node *region_np = dev->of_node;
struct device_node *br, *np, *parent_br = NULL;
+ unsigned long flags;
int i, ret;
/* If parent is a bridge, add to list */
+ spin_lock_irqsave(®ion->bridge_list_lock, flags);
ret = fpga_bridge_get_to_list(region_np->parent, region->info,
®ion->bridge_list);
+ spin_unlock_irqrestore(®ion->bridge_list_lock, flags);
+
if (ret == -EBUSY)
return ret;
@@ -508,6 +513,7 @@ static int fpga_region_probe(struct platform_device *pdev)
mutex_init(®ion->mutex);
INIT_LIST_HEAD(®ion->bridge_list);
+ spin_lock_init(®ion->bridge_list_lock);
device_initialize(®ion->dev);
region->dev.class = fpga_region_class;
Am I missing something here? If not I"ll send out my patch separately.
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_fpga_bridge_get_to_list);
> +
> +/**
> + * fpga_bridge_get_to_list - given device, get a bridge, add it to a list
> + *
> + * @dev: FPGA bridge device
> + * @info: fpga image specific information
> + * @bridge_list: list of FPGA bridges
> + *
> + * Get an exclusive reference to the bridge and and it to the list.
> + *
> + * Return 0 for success, error code from fpga_bridge_get() othewise.
> + */
> +int fpga_bridge_get_to_list(struct device *dev,
> struct fpga_image_info *info,
> struct list_head *bridge_list)
> {
> struct fpga_bridge *bridge;
> unsigned long flags;
>
> - bridge = of_fpga_bridge_get(np, info);
> + bridge = fpga_bridge_get(dev, info);
> if (IS_ERR(bridge))
> return PTR_ERR(bridge);
>
> @@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void)
> }
>
> MODULE_DESCRIPTION("FPGA Bridge Driver");
> -MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> +MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
> MODULE_LICENSE("GPL v2");
>
> subsys_initcall(fpga_bridge_dev_init);
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index d9ab7c7..91755562 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> int i, ret;
>
> /* If parent is a bridge, add to list */
> - ret = fpga_bridge_get_to_list(region_np->parent, region->info,
> - ®ion->bridge_list);
> + ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
> + ®ion->bridge_list);
> +
> + /* -EBUSY means parent is a bridge that is under use. Give up. */
> if (ret == -EBUSY)
> return ret;
>
> + /* Zero return code means parent was a bridge and was added to list. */
> if (!ret)
> parent_br = region_np->parent;
>
> @@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> continue;
>
> /* If node is a bridge, get it and add to list */
> - ret = fpga_bridge_get_to_list(br, region->info,
> - ®ion->bridge_list);
> + ret = of_fpga_bridge_get_to_list(br, region->info,
> + ®ion->bridge_list);
>
> /* If any of the bridges are in use, give up */
> if (ret == -EBUSY) {
> diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
> index dba6e3c..9f6696b 100644
> --- a/include/linux/fpga/fpga-bridge.h
> +++ b/include/linux/fpga/fpga-bridge.h
> @@ -42,6 +42,8 @@ struct fpga_bridge {
>
> struct fpga_bridge *of_fpga_bridge_get(struct device_node *node,
> struct fpga_image_info *info);
> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
> + struct fpga_image_info *info);
> void fpga_bridge_put(struct fpga_bridge *bridge);
> int fpga_bridge_enable(struct fpga_bridge *bridge);
> int fpga_bridge_disable(struct fpga_bridge *bridge);
> @@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);
> int fpga_bridges_enable(struct list_head *bridge_list);
> int fpga_bridges_disable(struct list_head *bridge_list);
> void fpga_bridges_put(struct list_head *bridge_list);
> -int fpga_bridge_get_to_list(struct device_node *np,
> +int fpga_bridge_get_to_list(struct device *dev,
> struct fpga_image_info *info,
> struct list_head *bridge_list);
> +int of_fpga_bridge_get_to_list(struct device_node *np,
> + struct fpga_image_info *info,
> + struct list_head *bridge_list);
>
> int fpga_bridge_register(struct device *dev, const char *name,
> const struct fpga_bridge_ops *br_ops, void *priv);
> --
> 2.7.4
>
Thanks,
Moritz
[toc] | [prev] | [next] | [standalone]
| From | Alan Tull <atull@kernel.org> |
|---|---|
| Date | 2017-09-18 23:00 +0200 |
| Subject | Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <urbj5-32d-27@gated-at.bofh.it> |
| In reply to | #1734346 |
On Mon, Sep 18, 2017 at 12:59 PM, Moritz Fischer <mdf@kernel.org> wrote:
> On Wed, Sep 13, 2017 at 03:48:24PM -0500, Alan Tull wrote:
>> Add two functions for getting the FPGA bridge from the device
>> rather than device tree node. This is to enable writing code
>> that will support using FPGA bridges without device tree.
>> Rename one old function to make it clear that it is device
>> tree-ish. This leaves us with 3 functions for getting a bridge:
>>
>> * fpga_bridge_get
>> Get the bridge given the device.
>>
>> * fpga_bridges_get_to_list
>> Given the device, get the bridge and add it to a list.
>>
>> * of_fpga_bridges_get_to_list
>> Renamed from priviously existing fpga_bridges_get_to_list.
>> Given the device node, get the bridge and add it to a list.
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> ---
>> v2: use list_for_each_entry
>> static the bridge_list_lock
>> update copyright and author email
>> v3: no change to this patch in this version of patchset
>> v4: no change to this patch in this version of patchset
>> ---
>> drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++--------
>> drivers/fpga/fpga-region.c | 11 ++--
>> include/linux/fpga/fpga-bridge.h | 7 ++-
>> 3 files changed, 100 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
>> index fcd2bd3..af6d97e 100644
>> --- a/drivers/fpga/fpga-bridge.c
>> +++ b/drivers/fpga/fpga-bridge.c
>> @@ -2,6 +2,7 @@
>> * FPGA Bridge Framework Driver
>> *
>> * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved.
>> + * Copyright (C) 2017 Intel Corporation
>> *
>> * This program is free software; you can redistribute it and/or modify it
>> * under the terms and conditions of the GNU General Public License,
>> @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge)
>> }
>> EXPORT_SYMBOL_GPL(fpga_bridge_disable);
>>
>> -/**
>> - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
>> - *
>> - * @np: node pointer of a FPGA bridge
>> - * @info: fpga image specific information
>> - *
>> - * Return fpga_bridge struct if successful.
>> - * Return -EBUSY if someone already has a reference to the bridge.
>> - * Return -ENODEV if @np is not a FPGA Bridge.
>> - */
>> -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
>> - struct fpga_image_info *info)
>> -
>> +struct fpga_bridge *__fpga_bridge_get(struct device *dev,
>> + struct fpga_image_info *info)
>> {
>> - struct device *dev;
>> struct fpga_bridge *bridge;
>> int ret = -ENODEV;
>>
>> - dev = class_find_device(fpga_bridge_class, NULL, np,
>> - fpga_bridge_of_node_match);
>> - if (!dev)
>> - goto err_dev;
>> -
>> bridge = to_fpga_bridge(dev);
>> if (!bridge)
>> goto err_dev;
>> @@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
>> put_device(dev);
>> return ERR_PTR(ret);
>> }
>> +
>> +/**
>> + * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
>> + *
>> + * @np: node pointer of a FPGA bridge
>> + * @info: fpga image specific information
>> + *
>> + * Return fpga_bridge struct if successful.
>> + * Return -EBUSY if someone already has a reference to the bridge.
>> + * Return -ENODEV if @np is not a FPGA Bridge.
>> + */
>> +struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
>> + struct fpga_image_info *info)
>> +{
>> + struct device *dev;
>> +
>> + dev = class_find_device(fpga_bridge_class, NULL, np,
>> + fpga_bridge_of_node_match);
>> + if (!dev)
>> + return ERR_PTR(-ENODEV);
>> +
>> + return __fpga_bridge_get(dev, info);
>> +}
>> EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
>>
>> +static int fpga_bridge_dev_match(struct device *dev, const void *data)
>> +{
>> + return dev->parent == data;
>> +}
>> +
>> +/**
>> + * fpga_bridge_get - get an exclusive reference to a fpga bridge
>> + * @dev: parent device that fpga bridge was registered with
>> + *
>> + * Given a device, get an exclusive reference to a fpga bridge.
>> + *
>> + * Return: fpga manager struct or IS_ERR() condition containing error code.
>> + */
>> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
>> + struct fpga_image_info *info)
>> +{
>> + struct device *bridge_dev;
>> +
>> + bridge_dev = class_find_device(fpga_bridge_class, NULL, dev,
>> + fpga_bridge_dev_match);
>> + if (!bridge_dev)
>> + return ERR_PTR(-ENODEV);
>> +
>> + return __fpga_bridge_get(bridge_dev, info);
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_bridge_get);
>> +
>> /**
>> * fpga_bridge_put - release a reference to a bridge
>> *
>> @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)
>> EXPORT_SYMBOL_GPL(fpga_bridges_put);
>>
>> /**
>> - * fpga_bridges_get_to_list - get a bridge, add it to a list
>> + * of_fpga_bridge_get_to_list - get a bridge, add it to a list
>> *
>> * @np: node pointer of a FPGA bridge
>> * @info: fpga image specific information
>> @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
>> *
>> * Return 0 for success, error code from of_fpga_bridge_get() othewise.
>> */
>> -int fpga_bridge_get_to_list(struct device_node *np,
>> +int of_fpga_bridge_get_to_list(struct device_node *np,
>> + struct fpga_image_info *info,
>> + struct list_head *bridge_list)
>> +{
>> + struct fpga_bridge *bridge;
>> + unsigned long flags;
>> +
>> + bridge = of_fpga_bridge_get(np, info);
>> + if (IS_ERR(bridge))
>> + return PTR_ERR(bridge);
>> +
>> + spin_lock_irqsave(&bridge_list_lock, flags);
>> + list_add(&bridge->node, bridge_list);
>> + spin_unlock_irqrestore(&bridge_list_lock, flags);
>
> I know this is not new code, but I was wondering the other day:
>
> Why are we using a single spinlock to protect all lists that are being
> passed in as parameters here?
>
> I have a patch that moves the spinlock into the region containing the
> list that uses the bridges which (unless I misunderstand something
> here), makes more sense:
>
> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> index 9651aa56244a..b03ec59448e2 100644
> --- a/drivers/fpga/fpga-bridge.c
> +++ b/drivers/fpga/fpga-bridge.c
> @@ -214,6 +214,8 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
> *
> * Get an exclusive reference to the bridge and and it to the list.
> *
> + * Must be called with list lock held.
> + *
> * Return 0 for success, error code from of_fpga_bridge_get() othewise.
> */
> int fpga_bridge_get_to_list(struct device_node *np,
> @@ -221,15 +223,12 @@ int fpga_bridge_get_to_list(struct device_node *np,
> struct list_head *bridge_list)
> {
> struct fpga_bridge *bridge;
> - unsigned long flags;
>
> bridge = of_fpga_bridge_get(np, info);
> if (IS_ERR(bridge))
> return PTR_ERR(bridge);
>
> - spin_lock_irqsave(&bridge_list_lock, flags);
> list_add(&bridge->node, bridge_list);
> - spin_unlock_irqrestore(&bridge_list_lock, flags);
>
> return 0;
> }
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 3b6b2f4182a1..c5c958e0e601 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -37,6 +37,7 @@ struct fpga_region {
> struct device dev;
> struct mutex mutex; /* for exclusive reference to region */
> struct list_head bridge_list;
> + spinlock_t bridge_list_lock; /* protects access to bridge list */
> struct fpga_image_info *info;
> };
>
> @@ -180,11 +181,15 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> struct device *dev = ®ion->dev;
> struct device_node *region_np = dev->of_node;
> struct device_node *br, *np, *parent_br = NULL;
> + unsigned long flags;
> int i, ret;
>
> /* If parent is a bridge, add to list */
> + spin_lock_irqsave(®ion->bridge_list_lock, flags);
> ret = fpga_bridge_get_to_list(region_np->parent, region->info,
> ®ion->bridge_list);
> + spin_unlock_irqrestore(®ion->bridge_list_lock, flags);
> +
> if (ret == -EBUSY)
> return ret;
>
> @@ -508,6 +513,7 @@ static int fpga_region_probe(struct platform_device *pdev)
>
> mutex_init(®ion->mutex);
> INIT_LIST_HEAD(®ion->bridge_list);
> + spin_lock_init(®ion->bridge_list_lock);
>
> device_initialize(®ion->dev);
> region->dev.class = fpga_region_class;
>
> Am I missing something here? If not I"ll send out my patch separately.
Hi Moritz,
You are right! I'll look at your patch.
Alan
>
>
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_fpga_bridge_get_to_list);
>> +
>> +/**
>> + * fpga_bridge_get_to_list - given device, get a bridge, add it to a list
>> + *
>> + * @dev: FPGA bridge device
>> + * @info: fpga image specific information
>> + * @bridge_list: list of FPGA bridges
>> + *
>> + * Get an exclusive reference to the bridge and and it to the list.
>> + *
>> + * Return 0 for success, error code from fpga_bridge_get() othewise.
>> + */
>> +int fpga_bridge_get_to_list(struct device *dev,
>> struct fpga_image_info *info,
>> struct list_head *bridge_list)
>> {
>> struct fpga_bridge *bridge;
>> unsigned long flags;
>>
>> - bridge = of_fpga_bridge_get(np, info);
>> + bridge = fpga_bridge_get(dev, info);
>> if (IS_ERR(bridge))
>> return PTR_ERR(bridge);
>>
>> @@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void)
>> }
>>
>> MODULE_DESCRIPTION("FPGA Bridge Driver");
>> -MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
>> +MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
>> MODULE_LICENSE("GPL v2");
>>
>> subsys_initcall(fpga_bridge_dev_init);
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index d9ab7c7..91755562 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,
>> int i, ret;
>>
>> /* If parent is a bridge, add to list */
>> - ret = fpga_bridge_get_to_list(region_np->parent, region->info,
>> - ®ion->bridge_list);
>> + ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
>> + ®ion->bridge_list);
>> +
>> + /* -EBUSY means parent is a bridge that is under use. Give up. */
>> if (ret == -EBUSY)
>> return ret;
>>
>> + /* Zero return code means parent was a bridge and was added to list. */
>> if (!ret)
>> parent_br = region_np->parent;
>>
>> @@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
>> continue;
>>
>> /* If node is a bridge, get it and add to list */
>> - ret = fpga_bridge_get_to_list(br, region->info,
>> - ®ion->bridge_list);
>> + ret = of_fpga_bridge_get_to_list(br, region->info,
>> + ®ion->bridge_list);
>>
>> /* If any of the bridges are in use, give up */
>> if (ret == -EBUSY) {
>> diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
>> index dba6e3c..9f6696b 100644
>> --- a/include/linux/fpga/fpga-bridge.h
>> +++ b/include/linux/fpga/fpga-bridge.h
>> @@ -42,6 +42,8 @@ struct fpga_bridge {
>>
>> struct fpga_bridge *of_fpga_bridge_get(struct device_node *node,
>> struct fpga_image_info *info);
>> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
>> + struct fpga_image_info *info);
>> void fpga_bridge_put(struct fpga_bridge *bridge);
>> int fpga_bridge_enable(struct fpga_bridge *bridge);
>> int fpga_bridge_disable(struct fpga_bridge *bridge);
>> @@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);
>> int fpga_bridges_enable(struct list_head *bridge_list);
>> int fpga_bridges_disable(struct list_head *bridge_list);
>> void fpga_bridges_put(struct list_head *bridge_list);
>> -int fpga_bridge_get_to_list(struct device_node *np,
>> +int fpga_bridge_get_to_list(struct device *dev,
>> struct fpga_image_info *info,
>> struct list_head *bridge_list);
>> +int of_fpga_bridge_get_to_list(struct device_node *np,
>> + struct fpga_image_info *info,
>> + struct list_head *bridge_list);
>>
>> int fpga_bridge_register(struct device *dev, const char *name,
>> const struct fpga_bridge_ops *br_ops, void *priv);
>> --
>> 2.7.4
>>
> Thanks,
>
> Moritz
[toc] | [prev] | [next] | [standalone]
| From | Moritz Fischer <mdf@kernel.org> |
|---|---|
| Date | 2017-09-19 01:00 +0200 |
| Subject | Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <urdbb-4nr-9@gated-at.bofh.it> |
| In reply to | #1731869 |
[Multipart message — attachments visible in raw view] — view raw
On Wed, Sep 13, 2017 at 03:48:24PM -0500, Alan Tull wrote:
> Add two functions for getting the FPGA bridge from the device
> rather than device tree node. This is to enable writing code
> that will support using FPGA bridges without device tree.
> Rename one old function to make it clear that it is device
> tree-ish. This leaves us with 3 functions for getting a bridge:
>
> * fpga_bridge_get
> Get the bridge given the device.
>
> * fpga_bridges_get_to_list
> Given the device, get the bridge and add it to a list.
>
> * of_fpga_bridges_get_to_list
> Renamed from priviously existing fpga_bridges_get_to_list.
> Given the device node, get the bridge and add it to a list.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
> ---
> v2: use list_for_each_entry
> static the bridge_list_lock
> update copyright and author email
> v3: no change to this patch in this version of patchset
> v4: no change to this patch in this version of patchset
> ---
> drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++--------
> drivers/fpga/fpga-region.c | 11 ++--
> include/linux/fpga/fpga-bridge.h | 7 ++-
> 3 files changed, 100 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> index fcd2bd3..af6d97e 100644
> --- a/drivers/fpga/fpga-bridge.c
> +++ b/drivers/fpga/fpga-bridge.c
> @@ -2,6 +2,7 @@
> * FPGA Bridge Framework Driver
> *
> * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved.
> + * Copyright (C) 2017 Intel Corporation
> *
> * This program is free software; you can redistribute it and/or modify it
> * under the terms and conditions of the GNU General Public License,
> @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge)
> }
> EXPORT_SYMBOL_GPL(fpga_bridge_disable);
>
> -/**
> - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
> - *
> - * @np: node pointer of a FPGA bridge
> - * @info: fpga image specific information
> - *
> - * Return fpga_bridge struct if successful.
> - * Return -EBUSY if someone already has a reference to the bridge.
> - * Return -ENODEV if @np is not a FPGA Bridge.
> - */
> -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> - struct fpga_image_info *info)
> -
> +struct fpga_bridge *__fpga_bridge_get(struct device *dev,
> + struct fpga_image_info *info)
> {
> - struct device *dev;
> struct fpga_bridge *bridge;
> int ret = -ENODEV;
>
> - dev = class_find_device(fpga_bridge_class, NULL, np,
> - fpga_bridge_of_node_match);
> - if (!dev)
> - goto err_dev;
> -
> bridge = to_fpga_bridge(dev);
> if (!bridge)
> goto err_dev;
> @@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> put_device(dev);
> return ERR_PTR(ret);
> }
> +
> +/**
> + * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
> + *
> + * @np: node pointer of a FPGA bridge
> + * @info: fpga image specific information
> + *
> + * Return fpga_bridge struct if successful.
> + * Return -EBUSY if someone already has a reference to the bridge.
> + * Return -ENODEV if @np is not a FPGA Bridge.
> + */
> +struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
> + struct fpga_image_info *info)
> +{
> + struct device *dev;
> +
> + dev = class_find_device(fpga_bridge_class, NULL, np,
> + fpga_bridge_of_node_match);
> + if (!dev)
> + return ERR_PTR(-ENODEV);
> +
> + return __fpga_bridge_get(dev, info);
> +}
> EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
>
> +static int fpga_bridge_dev_match(struct device *dev, const void *data)
> +{
> + return dev->parent == data;
> +}
> +
> +/**
> + * fpga_bridge_get - get an exclusive reference to a fpga bridge
> + * @dev: parent device that fpga bridge was registered with
> + *
> + * Given a device, get an exclusive reference to a fpga bridge.
> + *
> + * Return: fpga manager struct or IS_ERR() condition containing error code.
> + */
> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
> + struct fpga_image_info *info)
> +{
> + struct device *bridge_dev;
> +
> + bridge_dev = class_find_device(fpga_bridge_class, NULL, dev,
> + fpga_bridge_dev_match);
> + if (!bridge_dev)
> + return ERR_PTR(-ENODEV);
> +
> + return __fpga_bridge_get(bridge_dev, info);
> +}
> +EXPORT_SYMBOL_GPL(fpga_bridge_get);
Do we really need two functions here? Can't we just do a:
if (dev->of_node)
dev = class_find_device(fpga_bridge_class, NULL, np,
fpga_bridge_of_node_match);
else
dev = class_find_device(fpga_bridge_class, NULL, dev,
fpga_bridge_dev_match);
instead? Maybe you could even move the check into the match function,
so you can reuse all the code here?
> +
> /**
> * fpga_bridge_put - release a reference to a bridge
> *
> @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)
> EXPORT_SYMBOL_GPL(fpga_bridges_put);
>
> /**
> - * fpga_bridges_get_to_list - get a bridge, add it to a list
> + * of_fpga_bridge_get_to_list - get a bridge, add it to a list
> *
> * @np: node pointer of a FPGA bridge
> * @info: fpga image specific information
> @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
> *
> * Return 0 for success, error code from of_fpga_bridge_get() othewise.
> */
> -int fpga_bridge_get_to_list(struct device_node *np,
> +int of_fpga_bridge_get_to_list(struct device_node *np,
> + struct fpga_image_info *info,
> + struct list_head *bridge_list)
> +{
> + struct fpga_bridge *bridge;
> + unsigned long flags;
> +
> + bridge = of_fpga_bridge_get(np, info);
> + if (IS_ERR(bridge))
> + return PTR_ERR(bridge);
> +
> + spin_lock_irqsave(&bridge_list_lock, flags);
> + list_add(&bridge->node, bridge_list);
> + spin_unlock_irqrestore(&bridge_list_lock, flags);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_fpga_bridge_get_to_list);
> +
> +/**
> + * fpga_bridge_get_to_list - given device, get a bridge, add it to a list
> + *
> + * @dev: FPGA bridge device
> + * @info: fpga image specific information
> + * @bridge_list: list of FPGA bridges
> + *
> + * Get an exclusive reference to the bridge and and it to the list.
> + *
> + * Return 0 for success, error code from fpga_bridge_get() othewise.
> + */
> +int fpga_bridge_get_to_list(struct device *dev,
> struct fpga_image_info *info,
> struct list_head *bridge_list)
> {
> struct fpga_bridge *bridge;
> unsigned long flags;
>
> - bridge = of_fpga_bridge_get(np, info);
> + bridge = fpga_bridge_get(dev, info);
> if (IS_ERR(bridge))
> return PTR_ERR(bridge);
>
> @@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void)
> }
>
> MODULE_DESCRIPTION("FPGA Bridge Driver");
> -MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> +MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
> MODULE_LICENSE("GPL v2");
>
> subsys_initcall(fpga_bridge_dev_init);
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index d9ab7c7..91755562 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> int i, ret;
>
> /* If parent is a bridge, add to list */
> - ret = fpga_bridge_get_to_list(region_np->parent, region->info,
> - ®ion->bridge_list);
> + ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
> + ®ion->bridge_list);
> +
> + /* -EBUSY means parent is a bridge that is under use. Give up. */
> if (ret == -EBUSY)
> return ret;
>
> + /* Zero return code means parent was a bridge and was added to list. */
> if (!ret)
> parent_br = region_np->parent;
>
> @@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> continue;
>
> /* If node is a bridge, get it and add to list */
> - ret = fpga_bridge_get_to_list(br, region->info,
> - ®ion->bridge_list);
> + ret = of_fpga_bridge_get_to_list(br, region->info,
> + ®ion->bridge_list);
>
> /* If any of the bridges are in use, give up */
> if (ret == -EBUSY) {
> diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
> index dba6e3c..9f6696b 100644
> --- a/include/linux/fpga/fpga-bridge.h
> +++ b/include/linux/fpga/fpga-bridge.h
> @@ -42,6 +42,8 @@ struct fpga_bridge {
>
> struct fpga_bridge *of_fpga_bridge_get(struct device_node *node,
> struct fpga_image_info *info);
> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
> + struct fpga_image_info *info);
> void fpga_bridge_put(struct fpga_bridge *bridge);
> int fpga_bridge_enable(struct fpga_bridge *bridge);
> int fpga_bridge_disable(struct fpga_bridge *bridge);
> @@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);
> int fpga_bridges_enable(struct list_head *bridge_list);
> int fpga_bridges_disable(struct list_head *bridge_list);
> void fpga_bridges_put(struct list_head *bridge_list);
> -int fpga_bridge_get_to_list(struct device_node *np,
> +int fpga_bridge_get_to_list(struct device *dev,
> struct fpga_image_info *info,
> struct list_head *bridge_list);
> +int of_fpga_bridge_get_to_list(struct device_node *np,
> + struct fpga_image_info *info,
> + struct list_head *bridge_list);
>
> int fpga_bridge_register(struct device *dev, const char *name,
> const struct fpga_bridge_ops *br_ops, void *priv);
> --
> 2.7.4
>
[toc] | [prev] | [next] | [standalone]
| From | Alan Tull <atull@kernel.org> |
|---|---|
| Date | 2017-09-19 17:40 +0200 |
| Subject | Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device |
| Message-ID | <ursMW-7dF-17@gated-at.bofh.it> |
| In reply to | #1734527 |
On Mon, Sep 18, 2017 at 5:53 PM, Moritz Fischer <mdf@kernel.org> wrote:
Hi Moritz,
> On Wed, Sep 13, 2017 at 03:48:24PM -0500, Alan Tull wrote:
>> Add two functions for getting the FPGA bridge from the device
>> rather than device tree node. This is to enable writing code
>> that will support using FPGA bridges without device tree.
>> Rename one old function to make it clear that it is device
>> tree-ish. This leaves us with 3 functions for getting a bridge:
>>
>> * fpga_bridge_get
>> Get the bridge given the device.
>>
>> * fpga_bridges_get_to_list
>> Given the device, get the bridge and add it to a list.
>>
>> * of_fpga_bridges_get_to_list
>> Renamed from priviously existing fpga_bridges_get_to_list.
>> Given the device node, get the bridge and add it to a list.
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> ---
>> v2: use list_for_each_entry
>> static the bridge_list_lock
>> update copyright and author email
>> v3: no change to this patch in this version of patchset
>> v4: no change to this patch in this version of patchset
>> ---
>> drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++--------
>> drivers/fpga/fpga-region.c | 11 ++--
>> include/linux/fpga/fpga-bridge.h | 7 ++-
>> 3 files changed, 100 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
>> index fcd2bd3..af6d97e 100644
>> --- a/drivers/fpga/fpga-bridge.c
>> +++ b/drivers/fpga/fpga-bridge.c
>> @@ -2,6 +2,7 @@
>> * FPGA Bridge Framework Driver
>> *
>> * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved.
>> + * Copyright (C) 2017 Intel Corporation
>> *
>> * This program is free software; you can redistribute it and/or modify it
>> * under the terms and conditions of the GNU General Public License,
>> @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge)
>> }
>> EXPORT_SYMBOL_GPL(fpga_bridge_disable);
>>
>> -/**
>> - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
>> - *
>> - * @np: node pointer of a FPGA bridge
>> - * @info: fpga image specific information
>> - *
>> - * Return fpga_bridge struct if successful.
>> - * Return -EBUSY if someone already has a reference to the bridge.
>> - * Return -ENODEV if @np is not a FPGA Bridge.
>> - */
>> -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
>> - struct fpga_image_info *info)
>> -
>> +struct fpga_bridge *__fpga_bridge_get(struct device *dev,
>> + struct fpga_image_info *info)
>> {
>> - struct device *dev;
>> struct fpga_bridge *bridge;
>> int ret = -ENODEV;
>>
>> - dev = class_find_device(fpga_bridge_class, NULL, np,
>> - fpga_bridge_of_node_match);
>> - if (!dev)
>> - goto err_dev;
>> -
>> bridge = to_fpga_bridge(dev);
>> if (!bridge)
>> goto err_dev;
>> @@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
>> put_device(dev);
>> return ERR_PTR(ret);
>> }
>> +
>> +/**
>> + * of_fpga_bridge_get - get an exclusive reference to a fpga bridge
>> + *
>> + * @np: node pointer of a FPGA bridge
>> + * @info: fpga image specific information
>> + *
>> + * Return fpga_bridge struct if successful.
>> + * Return -EBUSY if someone already has a reference to the bridge.
>> + * Return -ENODEV if @np is not a FPGA Bridge.
>> + */
>> +struct fpga_bridge *of_fpga_bridge_get(struct device_node *np,
>> + struct fpga_image_info *info)
>> +{
>> + struct device *dev;
>> +
>> + dev = class_find_device(fpga_bridge_class, NULL, np,
>> + fpga_bridge_of_node_match);
>> + if (!dev)
>> + return ERR_PTR(-ENODEV);
>> +
>> + return __fpga_bridge_get(dev, info);
>> +}
>> EXPORT_SYMBOL_GPL(of_fpga_bridge_get);
>>
>> +static int fpga_bridge_dev_match(struct device *dev, const void *data)
>> +{
>> + return dev->parent == data;
>> +}
>> +
>> +/**
>> + * fpga_bridge_get - get an exclusive reference to a fpga bridge
>> + * @dev: parent device that fpga bridge was registered with
>> + *
>> + * Given a device, get an exclusive reference to a fpga bridge.
>> + *
>> + * Return: fpga manager struct or IS_ERR() condition containing error code.
>> + */
>> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
>> + struct fpga_image_info *info)
>> +{
>> + struct device *bridge_dev;
>> +
>> + bridge_dev = class_find_device(fpga_bridge_class, NULL, dev,
>> + fpga_bridge_dev_match);
>> + if (!bridge_dev)
>> + return ERR_PTR(-ENODEV);
>> +
>> + return __fpga_bridge_get(bridge_dev, info);
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_bridge_get);
>
> Do we really need two functions here? Can't we just do a:
>
> if (dev->of_node)
If we have the device, we might as well find the bridge using the
device. But if we have the of node, but not the device, we need some
way to get to the device from the of node (can't use container_of).
Such as in the loop in fpga_region_get_bridges() which is going
through the device tree.
> dev = class_find_device(fpga_bridge_class, NULL, np,
> fpga_bridge_of_node_match);
> else
> dev = class_find_device(fpga_bridge_class, NULL, dev,
> fpga_bridge_dev_match);
>
> instead? Maybe you could even move the check into the match function,
> so you can reuse all the code here?
But I agree it would be nice to have one function and if we can make
it work, we should. We could have one function that takes both a
device node and a device. If the device is non-null, look it up, else
if the device node is non-node, look it up and return. If that
doesn't seem ugly and ungainly.
Alan
>
>> +
>> /**
>> * fpga_bridge_put - release a reference to a bridge
>> *
>> @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list)
>> EXPORT_SYMBOL_GPL(fpga_bridges_put);
>>
>> /**
>> - * fpga_bridges_get_to_list - get a bridge, add it to a list
>> + * of_fpga_bridge_get_to_list - get a bridge, add it to a list
>> *
>> * @np: node pointer of a FPGA bridge
>> * @info: fpga image specific information
>> @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put);
>> *
>> * Return 0 for success, error code from of_fpga_bridge_get() othewise.
>> */
>> -int fpga_bridge_get_to_list(struct device_node *np,
>> +int of_fpga_bridge_get_to_list(struct device_node *np,
>> + struct fpga_image_info *info,
>> + struct list_head *bridge_list)
>> +{
>> + struct fpga_bridge *bridge;
>> + unsigned long flags;
>> +
>> + bridge = of_fpga_bridge_get(np, info);
>> + if (IS_ERR(bridge))
>> + return PTR_ERR(bridge);
>> +
>> + spin_lock_irqsave(&bridge_list_lock, flags);
>> + list_add(&bridge->node, bridge_list);
>> + spin_unlock_irqrestore(&bridge_list_lock, flags);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_fpga_bridge_get_to_list);
>> +
>> +/**
>> + * fpga_bridge_get_to_list - given device, get a bridge, add it to a list
>> + *
>> + * @dev: FPGA bridge device
>> + * @info: fpga image specific information
>> + * @bridge_list: list of FPGA bridges
>> + *
>> + * Get an exclusive reference to the bridge and and it to the list.
>> + *
>> + * Return 0 for success, error code from fpga_bridge_get() othewise.
>> + */
>> +int fpga_bridge_get_to_list(struct device *dev,
>> struct fpga_image_info *info,
>> struct list_head *bridge_list)
>> {
>> struct fpga_bridge *bridge;
>> unsigned long flags;
>>
>> - bridge = of_fpga_bridge_get(np, info);
>> + bridge = fpga_bridge_get(dev, info);
>> if (IS_ERR(bridge))
>> return PTR_ERR(bridge);
>>
>> @@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void)
>> }
>>
>> MODULE_DESCRIPTION("FPGA Bridge Driver");
>> -MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
>> +MODULE_AUTHOR("Alan Tull <atull@kernel.org>");
>> MODULE_LICENSE("GPL v2");
>>
>> subsys_initcall(fpga_bridge_dev_init);
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index d9ab7c7..91755562 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -183,11 +183,14 @@ static int fpga_region_get_bridges(struct fpga_region *region,
>> int i, ret;
>>
>> /* If parent is a bridge, add to list */
>> - ret = fpga_bridge_get_to_list(region_np->parent, region->info,
>> - ®ion->bridge_list);
>> + ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
>> + ®ion->bridge_list);
>> +
>> + /* -EBUSY means parent is a bridge that is under use. Give up. */
>> if (ret == -EBUSY)
>> return ret;
>>
>> + /* Zero return code means parent was a bridge and was added to list. */
>> if (!ret)
>> parent_br = region_np->parent;
>>
>> @@ -207,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
>> continue;
>>
>> /* If node is a bridge, get it and add to list */
>> - ret = fpga_bridge_get_to_list(br, region->info,
>> - ®ion->bridge_list);
>> + ret = of_fpga_bridge_get_to_list(br, region->info,
>> + ®ion->bridge_list);
>>
>> /* If any of the bridges are in use, give up */
>> if (ret == -EBUSY) {
>> diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h
>> index dba6e3c..9f6696b 100644
>> --- a/include/linux/fpga/fpga-bridge.h
>> +++ b/include/linux/fpga/fpga-bridge.h
>> @@ -42,6 +42,8 @@ struct fpga_bridge {
>>
>> struct fpga_bridge *of_fpga_bridge_get(struct device_node *node,
>> struct fpga_image_info *info);
>> +struct fpga_bridge *fpga_bridge_get(struct device *dev,
>> + struct fpga_image_info *info);
>> void fpga_bridge_put(struct fpga_bridge *bridge);
>> int fpga_bridge_enable(struct fpga_bridge *bridge);
>> int fpga_bridge_disable(struct fpga_bridge *bridge);
>> @@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge);
>> int fpga_bridges_enable(struct list_head *bridge_list);
>> int fpga_bridges_disable(struct list_head *bridge_list);
>> void fpga_bridges_put(struct list_head *bridge_list);
>> -int fpga_bridge_get_to_list(struct device_node *np,
>> +int fpga_bridge_get_to_list(struct device *dev,
>> struct fpga_image_info *info,
>> struct list_head *bridge_list);
>> +int of_fpga_bridge_get_to_list(struct device_node *np,
>> + struct fpga_image_info *info,
>> + struct list_head *bridge_list);
>>
>> int fpga_bridge_register(struct device *dev, const char *name,
>> const struct fpga_bridge_ops *br_ops, void *priv);
>> --
>> 2.7.4
>>
[toc] | [prev] | [next] | [standalone]
| From | Alan Tull <atull@kernel.org> |
|---|---|
| Date | 2017-09-13 23:00 +0200 |
| Subject | [PATCH v4 12/18] fpga: region: rename some functions prior to moving |
| Message-ID | <upmVl-3P1-27@gated-at.bofh.it> |
| In reply to | #1731851 |
Rename some functions that will be moved to
of-fpga-region.c. Also change some parameters
and export a function to help with refactoring.
This is a step towards the larger goal of separating
device tree support from FPGA region common code.
* fpga_region_get_manager -> of_fpga_region_get_mgr
* add 'of_' prefix to the following:
* fpga_region_find
* fpga_region_get_bridges
* fpga_region_notify_pre_apply
* fpga_region_notify_post_remove),
* fpga_region_probe/remove
Parameter changes:
* of_fpga_region_find
change parameter to be the device node of the region.
* of_fpga_region_get_bridges
change second parameter to FPGA image info.
Export of_fpga_region_find as well.
Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
v3: no changes in patch for this version of patchset
v4: no change to this patch in this version of patchset
---
drivers/fpga/fpga-region.c | 60 ++++++++++++++++++++++++----------------------
1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 402d0b6..92ab216 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -42,12 +42,14 @@ static int fpga_region_of_node_match(struct device *dev, const void *data)
}
/**
- * fpga_region_find - find FPGA region
+ * of_fpga_region_find - find FPGA region
* @np: device node of FPGA Region
+ *
* Caller will need to put_device(®ion->dev) when done.
+ *
* Returns FPGA Region struct or NULL
*/
-static struct fpga_region *fpga_region_find(struct device_node *np)
+static struct fpga_region *of_fpga_region_find(struct device_node *np)
{
struct device *dev;
@@ -107,7 +109,7 @@ static void fpga_region_put(struct fpga_region *region)
}
/**
- * fpga_region_get_manager - get reference for FPGA manager
+ * of_fpga_region_get_mgr - get reference for FPGA manager
* @np: device node of FPGA region
*
* Get FPGA Manager from "fpga-mgr" property or from ancestor region.
@@ -116,7 +118,7 @@ static void fpga_region_put(struct fpga_region *region)
*
* Return: fpga manager struct or IS_ERR() condition containing error code.
*/
-static struct fpga_manager *fpga_region_get_manager(struct device_node *np)
+static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np)
{
struct device_node *mgr_node;
struct fpga_manager *mgr;
@@ -139,9 +141,9 @@ static struct fpga_manager *fpga_region_get_manager(struct device_node *np)
}
/**
- * fpga_region_get_bridges - create a list of bridges
+ * of_fpga_region_get_bridges - create a list of bridges
* @region: FPGA region
- * @overlay: device node of the overlay
+ * @info: FPGA image info
*
* Create a list of bridges including the parent bridge and the bridges
* specified by "fpga-bridges" property. Note that the
@@ -154,8 +156,8 @@ static struct fpga_manager *fpga_region_get_manager(struct device_node *np)
* Return 0 for success (even if there are no bridges specified)
* or -EBUSY if any of the bridges are in use.
*/
-static int fpga_region_get_bridges(struct fpga_region *region,
- struct device_node *overlay)
+static int of_fpga_region_get_bridges(struct fpga_region *region,
+ struct fpga_image_info *info)
{
struct device *dev = ®ion->dev;
struct device_node *region_np = dev->of_node;
@@ -163,7 +165,7 @@ static int fpga_region_get_bridges(struct fpga_region *region,
int i, ret;
/* If parent is a bridge, add to list */
- ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
+ ret = of_fpga_bridge_get_to_list(region_np->parent, info,
®ion->bridge_list);
/* -EBUSY means parent is a bridge that is under use. Give up. */
@@ -175,8 +177,8 @@ static int fpga_region_get_bridges(struct fpga_region *region,
parent_br = region_np->parent;
/* If overlay has a list of bridges, use it. */
- if (of_parse_phandle(overlay, "fpga-bridges", 0))
- np = overlay;
+ if (of_parse_phandle(info->overlay, "fpga-bridges", 0))
+ np = info->overlay;
else
np = region_np;
@@ -227,7 +229,7 @@ int fpga_region_program_fpga(struct fpga_region *region)
goto err_put_region;
}
- ret = fpga_region_get_bridges(region, info->overlay);
+ ret = of_fpga_region_get_bridges(region, info);
if (ret) {
dev_err(dev, "failed to get FPGA bridges\n");
goto err_unlock_mgr;
@@ -397,7 +399,7 @@ static struct fpga_image_info *of_fpga_region_parse_ov(
}
/**
- * fpga_region_notify_pre_apply - pre-apply overlay notification
+ * of_fpga_region_notify_pre_apply - pre-apply overlay notification
*
* @region: FPGA region that the overlay was applied to
* @nd: overlay notification data
@@ -410,8 +412,8 @@ static struct fpga_image_info *of_fpga_region_parse_ov(
*
* Returns 0 for success or negative error code for failure.
*/
-static int fpga_region_notify_pre_apply(struct fpga_region *region,
- struct of_overlay_notify_data *nd)
+static int of_fpga_region_notify_pre_apply(struct fpga_region *region,
+ struct of_overlay_notify_data *nd)
{
struct device *dev = ®ion->dev;
struct fpga_image_info *info;
@@ -441,7 +443,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
}
/**
- * fpga_region_notify_post_remove - post-remove overlay notification
+ * of_fpga_region_notify_post_remove - post-remove overlay notification
*
* @region: FPGA region that was targeted by the overlay that was removed
* @nd: overlay notification data
@@ -449,8 +451,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
* Called after an overlay has been removed if the overlay's target was a
* FPGA region.
*/
-static void fpga_region_notify_post_remove(struct fpga_region *region,
- struct of_overlay_notify_data *nd)
+static void of_fpga_region_notify_post_remove(struct fpga_region *region,
+ struct of_overlay_notify_data *nd)
{
fpga_bridges_disable(®ion->bridge_list);
fpga_bridges_put(®ion->bridge_list);
@@ -493,18 +495,18 @@ static int of_fpga_region_notify(struct notifier_block *nb,
return NOTIFY_OK;
}
- region = fpga_region_find(nd->target);
+ region = of_fpga_region_find(nd->target);
if (!region)
return NOTIFY_OK;
ret = 0;
switch (action) {
case OF_OVERLAY_PRE_APPLY:
- ret = fpga_region_notify_pre_apply(region, nd);
+ ret = of_fpga_region_notify_pre_apply(region, nd);
break;
case OF_OVERLAY_POST_REMOVE:
- fpga_region_notify_post_remove(region, nd);
+ of_fpga_region_notify_post_remove(region, nd);
break;
}
@@ -520,7 +522,7 @@ static struct notifier_block fpga_region_of_nb = {
.notifier_call = of_fpga_region_notify,
};
-static int fpga_region_probe(struct platform_device *pdev)
+static int of_fpga_region_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
@@ -528,7 +530,7 @@ static int fpga_region_probe(struct platform_device *pdev)
struct fpga_manager *mgr;
int id, ret = 0;
- mgr = fpga_region_get_manager(np);
+ mgr = of_fpga_region_get_mgr(np);
if (IS_ERR(mgr))
return -EPROBE_DEFER;
@@ -580,7 +582,7 @@ static int fpga_region_probe(struct platform_device *pdev)
return ret;
}
-static int fpga_region_remove(struct platform_device *pdev)
+static int of_fpga_region_remove(struct platform_device *pdev)
{
struct fpga_region *region = platform_get_drvdata(pdev);
@@ -590,9 +592,9 @@ static int fpga_region_remove(struct platform_device *pdev)
return 0;
}
-static struct platform_driver fpga_region_driver = {
- .probe = fpga_region_probe,
- .remove = fpga_region_remove,
+static struct platform_driver of_fpga_region_driver = {
+ .probe = of_fpga_region_probe,
+ .remove = of_fpga_region_remove,
.driver = {
.name = "fpga-region",
.of_match_table = of_match_ptr(fpga_region_of_match),
@@ -625,7 +627,7 @@ static int __init fpga_region_init(void)
if (ret)
goto err_class;
- ret = platform_driver_register(&fpga_region_driver);
+ ret = platform_driver_register(&of_fpga_region_driver);
if (ret)
goto err_plat;
@@ -641,7 +643,7 @@ static int __init fpga_region_init(void)
static void __exit fpga_region_exit(void)
{
- platform_driver_unregister(&fpga_region_driver);
+ platform_driver_unregister(&of_fpga_region_driver);
of_overlay_notifier_unregister(&fpga_region_of_nb);
class_destroy(fpga_region_class);
ida_destroy(&fpga_region_ida);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Alan Tull <atull@kernel.org> |
|---|---|
| Date | 2017-09-13 23:00 +0200 |
| Subject | [PATCH v4 07/18] fpga: region: check for child regions before allocing image info |
| Message-ID | <upmVl-3P1-29@gated-at.bofh.it> |
| In reply to | #1731851 |
During a device tree overlay pre-apply notification, the check
for child FPGA regions can happen slightly earlier. This saves
us from allocating the FPGA image info that just gets thrown
away.
This is a baby step in refactoring the FPGA region code to
separate out common FPGA region code from FPGA region
Device Tree overlay support.
Signed-off-by: Alan Tull <atull@kernel.org>
Acked-by: Moritz Fischer <mdf@kernel.org>
---
v2: split out from another patch
v3: s/dev/®ion->dev/ in one place
add Moritz' ack
v4: no change to this patch in this version of patchset
---
drivers/fpga/fpga-region.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index d78f444..afac543 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -355,15 +355,19 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
const char *firmware_name;
int ret;
- info = fpga_image_info_alloc(dev);
- if (!info)
- return -ENOMEM;
-
- /* Reject overlay if child FPGA Regions have firmware-name property */
+ /*
+ * Reject overlay if child FPGA Regions added in the overlay have
+ * firmware-name property (would mean that an FPGA region that has
+ * not been added to the live tree yet is doing FPGA programming).
+ */
ret = child_regions_with_firmware(nd->overlay);
if (ret)
return ret;
+ info = fpga_image_info_alloc(dev);
+ if (!info)
+ return -ENOMEM;
+
/* Read FPGA region properties from the overlay */
if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Alan Tull <atull@kernel.org> |
|---|---|
| Date | 2017-09-13 23:00 +0200 |
| Subject | [PATCH v4 08/18] fpga: region: fix slow warning with more than one overlay |
| Message-ID | <upmVl-3P1-31@gated-at.bofh.it> |
| In reply to | #1731851 |
When DT overlays are applied, each FPGA region keeps track of the fpga
image info as region->info. This pointer is assigned only if an
overlay causes the FPGA to be programmed. As it stands, this pointer
can be overwritten, causing a slow warning later when overlays are
removed.
This patch fixes this by changing the allowed behaviour. If a region
has received an overlay that programmed the FPGA, reject other
overlays that try to program the FPGA. To reprogram the FPGA, first
remove the overlay. This makes sense as removing the overlay also
removes the devices cleanly. Note that overlays that make DT changes
without reprogramming the FPGA are exempt from this restriction.
Signed-off-by: Alan Tull <atull@kernel.org>
---
v2: split out from another patch
v3: better explanation in header
v4: no change to this patch in this version of patchset
---
drivers/fpga/fpga-region.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index afac543..35af952 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -355,6 +355,11 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
const char *firmware_name;
int ret;
+ if (region->info) {
+ dev_err(dev, "Region already has overlay applied.\n");
+ return -EINVAL;
+ }
+
/*
* Reject overlay if child FPGA Regions added in the overlay have
* firmware-name property (would mean that an FPGA region that has
--
2.7.4
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web