diff mbox

[v2,02/16] fpga: bridge: support getting bridge from device

Message ID 1492697401-11211-3-git-send-email-atull@kernel.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Alan Tull April 20, 2017, 2:09 p.m. UTC
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
---
 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(-)

Comments

Wu, Hao May 3, 2017, 11:58 a.m. UTC | #1
On Thu, Apr 20, 2017 at 09:09:47AM -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.
> 

Hi Alan

Thanks a lot for providing this patch set for non device tree support. :)
Actually I am reworking the Intel FPGA device drivers based on this patch
set, and I find some problems with the existing APIs including fpga bridge
and manager. My idea is to create all fpga bridges/regions/manager under
the same platform device (FME), it allows FME driver to establish the
relationship for the bridges/regions/managers it creates in an easy way.
But I found current fpga class API doesn't support this very well.
e.g fpga_bridge_get/get_to_list only accept parent device as the input
parameter, but it doesn't work if we have multiple bridges (and
regions/manager) under the same platform device. fpga_mgr has similar
issue, but fpga_region APIs work better, as they accept fpga_region as
parameter not the shared parent device.

Do you think if having multiple fpga-* under one parent device is in the
right direction? If yes, shall we provide some more APIs which accept
fpga_bridge (and same for fpga-mgr) as parameter instead of the parent
device just like fpga-region?

Thanks
Hao

--
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
Alan Tull May 3, 2017, 8:07 p.m. UTC | #2
On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
> On Thu, Apr 20, 2017 at 09:09:47AM -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.
>>
>
> Hi Alan
>
> Thanks a lot for providing this patch set for non device tree support. :)
> Actually I am reworking the Intel FPGA device drivers based on this patch
> set, and I find some problems with the existing APIs including fpga bridge
> and manager. My idea is to create all fpga bridges/regions/manager under
> the same platform device (FME), it allows FME driver to establish the
> relationship for the bridges/regions/managers it creates in an easy way.
> But I found current fpga class API doesn't support this very well.
> e.g fpga_bridge_get/get_to_list only accept parent device as the input
> parameter, but it doesn't work if we have multiple bridges (and
> regions/manager) under the same platform device. fpga_mgr has similar
> issue, but fpga_region APIs work better, as they accept fpga_region as
> parameter not the shared parent device.

That's good feedback.  I can post a couple patches that apply on top
of that patchset to add the APIs you need.

Probably what I'll do is add

struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);

And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the following:

struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
                                struct fpga_image_info *info);

int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
                               struct fpga_image_info *info,
                               struct list_head *bridge_list);

Working on it now.

>
> Do you think if having multiple fpga-* under one parent device is in the
> right direction?

That should be fine as long as it's coded with an eye on making things
reusable and seeing beyond the current project.  Just thinking of the
future and of what can be of general usefulness for others.  And there
will be others interested in reusing this.

Alan

> If yes, shall we provide some more APIs which accept
> fpga_bridge (and same for fpga-mgr) as parameter instead of the parent
> device just like fpga-region?
>
> Thanks
> Hao
>
--
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
Wu, Hao May 4, 2017, 9:20 a.m. UTC | #3
On Wed, May 03, 2017 at 03:07:33PM -0500, Alan Tull wrote:
> On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
> > On Thu, Apr 20, 2017 at 09:09:47AM -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.
> >>
> >
> > Hi Alan
> >
> > Thanks a lot for providing this patch set for non device tree support. :)
> > Actually I am reworking the Intel FPGA device drivers based on this patch
> > set, and I find some problems with the existing APIs including fpga bridge
> > and manager. My idea is to create all fpga bridges/regions/manager under
> > the same platform device (FME), it allows FME driver to establish the
> > relationship for the bridges/regions/managers it creates in an easy way.
> > But I found current fpga class API doesn't support this very well.
> > e.g fpga_bridge_get/get_to_list only accept parent device as the input
> > parameter, but it doesn't work if we have multiple bridges (and
> > regions/manager) under the same platform device. fpga_mgr has similar
> > issue, but fpga_region APIs work better, as they accept fpga_region as
> > parameter not the shared parent device.
> 
> That's good feedback.  I can post a couple patches that apply on top
> of that patchset to add the APIs you need.
> 
> Probably what I'll do is add
> 
> struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
> 
> And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the following:
> 
> struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>                                 struct fpga_image_info *info);
> 
> int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>                                struct fpga_image_info *info,
>                                struct list_head *bridge_list);
> 
> Working on it now.

Hi Alan

Thanks a lot! This sounds very good to me and I assume fpga_bridge_get_to_list
will accept struct fpga_bridge * as input parameter too. :)

> 
> >
> > Do you think if having multiple fpga-* under one parent device is in the
> > right direction?
> 
> That should be fine as long as it's coded with an eye on making things
> reusable and seeing beyond the current project.  Just thinking of the
> future and of what can be of general usefulness for others.  And there
> will be others interested in reusing this.
> 

Glad to hear that you agree with this. :)

I list some other APIs which have the similar issue, but may not related
to this patch directly.

void fpga_bridge_unregister(struct device *dev)
void fpga_mgr_unregister(struct device *dev)

They only accept the parent device, should we use struct fpga_bridge *and
struct fpga_manager * instead of the parent device in above 2 functions
too?

int fpga_bridge_register(struct device *dev, const char *name,
                         const struct fpga_bridge_ops *br_ops, void *priv)
int fpga_mgr_register(struct device *dev, const char *name,
		      const struct fpga_manager_ops *mops,
		      void *priv)

is it possible to return struct fpga_bridge/manager * in the register
functions? otherwise in driver we have to get the related pointer from
the drvdata of the parent device right after creation of each fpga-*.
The parent device only saves one fpga-* in drvdata at a time per current
API. If these APIs return the fpga-* pointer, then we don't need to care
about the what is saved in drvdata of the parent device.

Thanks
Hao

> Alan
> 
> > If yes, shall we provide some more APIs which accept
> > fpga_bridge (and same for fpga-mgr) as parameter instead of the parent
> > device just like fpga-region?
> >
> > Thanks
> > Hao
> >
--
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
Alan Tull May 4, 2017, 9:31 p.m. UTC | #4
On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
> On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>> On Thu, Apr 20, 2017 at 09:09:47AM -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.
>>>
>>
>> Hi Alan
>>
>> Thanks a lot for providing this patch set for non device tree support. :)
>> Actually I am reworking the Intel FPGA device drivers based on this patch
>> set, and I find some problems with the existing APIs including fpga bridge
>> and manager. My idea is to create all fpga bridges/regions/manager under
>> the same platform device (FME), it allows FME driver to establish the
>> relationship for the bridges/regions/managers it creates in an easy way.
>> But I found current fpga class API doesn't support this very well.
>> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>> parameter, but it doesn't work if we have multiple bridges (and
>> regions/manager) under the same platform device. fpga_mgr has similar
>> issue, but fpga_region APIs work better, as they accept fpga_region as
>> parameter not the shared parent device.
>
> That's good feedback.  I can post a couple patches that apply on top
> of that patchset to add the APIs you need.
>
> Probably what I'll do is add
>
> struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>
> And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the following:
>
> struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>                                 struct fpga_image_info *info);
>
> int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>                                struct fpga_image_info *info,
>                                struct list_head *bridge_list);
>
> Working on it now.
>
>>
>> Do you think if having multiple fpga-* under one parent device is in the
>> right direction?
>
> That should be fine as long as it's coded with an eye on making things
> reusable and seeing beyond the current project.  Just thinking of the
> future and of what can be of general usefulness for others.  And there
> will be others interested in reusing this.
>
> Alan

Actually,  I don't think you will need the additional APIs we were
just discussing after all.  What you have is a multifunction device
(single piece of hardware, multi functions such as in drivers/mfd).
It will have child devices for the mgr, bridges, and regions.  When
registering the mgr and bridges you will need to allocate child
devices and use them to create the mgr and bridges.

Alan

>
>> If yes, shall we provide some more APIs which accept
>> fpga_bridge (and same for fpga-mgr) as parameter instead of the parent
>> device just like fpga-region?
>>
>> Thanks
>> Hao
>>
--
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
Wu, Hao May 8, 2017, 8:44 a.m. UTC | #5
> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:

> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:

> >> On Thu, Apr 20, 2017 at 09:09:47AM -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.

> >>>

> >>

> >> Hi Alan

> >>

> >> Thanks a lot for providing this patch set for non device tree support. :)

> >> Actually I am reworking the Intel FPGA device drivers based on this patch

> >> set, and I find some problems with the existing APIs including fpga bridge

> >> and manager. My idea is to create all fpga bridges/regions/manager under

> >> the same platform device (FME), it allows FME driver to establish the

> >> relationship for the bridges/regions/managers it creates in an easy way.

> >> But I found current fpga class API doesn't support this very well.

> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input

> >> parameter, but it doesn't work if we have multiple bridges (and

> >> regions/manager) under the same platform device. fpga_mgr has similar

> >> issue, but fpga_region APIs work better, as they accept fpga_region as

> >> parameter not the shared parent device.

> >

> > That's good feedback.  I can post a couple patches that apply on top

> > of that patchset to add the APIs you need.

> >

> > Probably what I'll do is add

> >

> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);

> >

> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the

> following:

> >

> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,

> >                                 struct fpga_image_info *info);

> >

> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,

> >                                struct fpga_image_info *info,

> >                                struct list_head *bridge_list);

> >

> > Working on it now.

> >

> >>

> >> Do you think if having multiple fpga-* under one parent device is in the

> >> right direction?

> >

> > That should be fine as long as it's coded with an eye on making things

> > reusable and seeing beyond the current project.  Just thinking of the

> > future and of what can be of general usefulness for others.  And there

> > will be others interested in reusing this.

> >

> > Alan

> 

> Actually,  I don't think you will need the additional APIs we were

> just discussing after all.  What you have is a multifunction device

> (single piece of hardware, multi functions such as in drivers/mfd).

> It will have child devices for the mgr, bridges, and regions.  When

> registering the mgr and bridges you will need to allocate child

> devices and use them to create the mgr and bridges.

> 

> Alan


Hi Alan

I tried to create child devices as the parent device for the mgr and
bridges in fme platform driver module. If only creates the device without
driver, it doesn't work as try_module_get(dev->parent->driver->owner)
always failed in mgr_get and bridge_get functions.

If it creates platform devices as child devices, and introduce new platform
device drivers for bridge and mgr, then it will be difficult to establish the
relationship for region/mgr/bridges (e.g when should region->mgr be
configured and cleared, as mgr is created/destroyed when mgr parent
device platform driver module is loaded/unload), and it maybe not really
necessary to introduce more different driver modules here.

But if it allows multiple fpga-* created under one device in one device
driver, it will be much easier to avoid above problems. So I asked if it
is possible to create multiple fpga-* under one parent device, I feel
this will not impact to current fpga drivers a lot, but provide more
flexibility for drivers to use fpga-region/bridge/manager to create
the topology in a device specific way, especially for non device
tree case. 

Thanks
Hao
Alan Tull May 8, 2017, 8:44 p.m. UTC | #6
On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>> >> On Thu, Apr 20, 2017 at 09:09:47AM -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.
>> >>>
>> >>
>> >> Hi Alan
>> >>
>> >> Thanks a lot for providing this patch set for non device tree support. :)
>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>> >> set, and I find some problems with the existing APIs including fpga bridge
>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>> >> the same platform device (FME), it allows FME driver to establish the
>> >> relationship for the bridges/regions/managers it creates in an easy way.
>> >> But I found current fpga class API doesn't support this very well.
>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>> >> parameter, but it doesn't work if we have multiple bridges (and
>> >> regions/manager) under the same platform device. fpga_mgr has similar
>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>> >> parameter not the shared parent device.
>> >
>> > That's good feedback.  I can post a couple patches that apply on top
>> > of that patchset to add the APIs you need.
>> >
>> > Probably what I'll do is add
>> >
>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>> >
>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>> following:
>> >
>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>> >                                 struct fpga_image_info *info);
>> >
>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>> >                                struct fpga_image_info *info,
>> >                                struct list_head *bridge_list);
>> >
>> > Working on it now.
>> >
>> >>
>> >> Do you think if having multiple fpga-* under one parent device is in the
>> >> right direction?
>> >
>> > That should be fine as long as it's coded with an eye on making things
>> > reusable and seeing beyond the current project.  Just thinking of the
>> > future and of what can be of general usefulness for others.  And there
>> > will be others interested in reusing this.
>> >
>> > Alan
>>
>> Actually,  I don't think you will need the additional APIs we were
>> just discussing after all.  What you have is a multifunction device
>> (single piece of hardware, multi functions such as in drivers/mfd).
>> It will have child devices for the mgr, bridges, and regions.  When
>> registering the mgr and bridges you will need to allocate child
>> devices and use them to create the mgr and bridges.
>>
>> Alan
>
> Hi Alan
>
> I tried to create child devices as the parent device for the mgr and
> bridges in fme platform driver module. If only creates the device without
> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
> always failed in mgr_get and bridge_get functions.

I tried it and it wasn't hard.

Each mgr or bridge driver should be a separate file which registers
its driver using 'module_platform_driver".  That way the drivers are
registered with the kernel in a normal fashion.  The thing we want
here is to not bypass the kernel driver model.

You'll need to keep the platform_device pointers in private data somewhere.

For each child platform device, do a platform_device_alloc and
platform_device_add.

Then to get the manager, you can do

mgr = fpga_mgr_get(&priv->mgr_pdev->dev);

If this is in your probe function, you can use -EPROBE_DEFER if
platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
whatever you've created and return -EPROBE_DEFER to wait for the
drivers you need to be registered and ready for devices to be added.

>
> If it creates platform devices as child devices, and introduce new platform
> device drivers for bridge and mgr, then it will be difficult to establish the
> relationship for region/mgr/bridges (e.g when should region->mgr be
> configured and cleared, as mgr is created/destroyed when mgr parent
> device platform driver module is loaded/unload), and it maybe not really
> necessary to introduce more different driver modules here.

It should be pretty easy to create/destroy child devices as shown
above.  The kernel does this all the time.

>
> But if it allows multiple fpga-* created under one device in one device
> driver, it will be much easier to avoid above problems. So I asked if it
> is possible to create multiple fpga-* under one parent device,

I think it's fine for your FME to create child platform devices.  It's
similar to a mfd, but the mfd framework hides the platform devices
from the module that creates them, unfortunately.

> I feel
> this will not impact to current fpga drivers a lot, but provide more
> flexibility for drivers to use fpga-region/bridge/manager to create
> the topology in a device specific way, especially for non device
> tree case.
>

I would like to see most of this code as FME enumeration code + a mgr
driver + a bridge driver + a region driver.  If the FME and the
enumeration code can be separate files, so much the better for general
usability.

The enumeration code can build a set of regions by doing something like this:
1. figure out what type of mgr and bridges your hardware FME has.
2. do platform_device_alloc and platform_device_add to create the mgr
device, save a pointer to its platform_device in your FME driver's
private data.
2. For each port, create a region and a bridge device.  Save the
region's platform device or struct in a list in your FME driver's
priv.
3. then you can create the sub function devices.

> Thanks
> Hao
--
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
Moritz Fischer May 8, 2017, 8:52 p.m. UTC | #7
On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -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.
>>> >>>
>>> >>
>>> >> Hi Alan
>>> >>
>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>> >> the same platform device (FME), it allows FME driver to establish the
>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>> >> But I found current fpga class API doesn't support this very well.
>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>> >> parameter not the shared parent device.
>>> >
>>> > That's good feedback.  I can post a couple patches that apply on top
>>> > of that patchset to add the APIs you need.
>>> >
>>> > Probably what I'll do is add
>>> >
>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>> >
>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>> following:
>>> >
>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>> >                                 struct fpga_image_info *info);
>>> >
>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>> >                                struct fpga_image_info *info,
>>> >                                struct list_head *bridge_list);
>>> >
>>> > Working on it now.
>>> >
>>> >>
>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>> >> right direction?
>>> >
>>> > That should be fine as long as it's coded with an eye on making things
>>> > reusable and seeing beyond the current project.  Just thinking of the
>>> > future and of what can be of general usefulness for others.  And there
>>> > will be others interested in reusing this.
>>> >
>>> > Alan
>>>
>>> Actually,  I don't think you will need the additional APIs we were
>>> just discussing after all.  What you have is a multifunction device
>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>> It will have child devices for the mgr, bridges, and regions.  When
>>> registering the mgr and bridges you will need to allocate child
>>> devices and use them to create the mgr and bridges.
>>>
>>> Alan
>>
>> Hi Alan
>>
>> I tried to create child devices as the parent device for the mgr and
>> bridges in fme platform driver module. If only creates the device without
>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>> always failed in mgr_get and bridge_get functions.
>
> I tried it and it wasn't hard.
>
> Each mgr or bridge driver should be a separate file which registers
> its driver using 'module_platform_driver".  That way the drivers are
> registered with the kernel in a normal fashion.  The thing we want
> here is to not bypass the kernel driver model.
>
> You'll need to keep the platform_device pointers in private data somewhere.
>
> For each child platform device, do a platform_device_alloc and
> platform_device_add.
>
> Then to get the manager, you can do
>
> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>
> If this is in your probe function, you can use -EPROBE_DEFER if
> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
> whatever you've created and return -EPROBE_DEFER to wait for the
> drivers you need to be registered and ready for devices to be added.
>
>>
>> If it creates platform devices as child devices, and introduce new platform
>> device drivers for bridge and mgr, then it will be difficult to establish the
>> relationship for region/mgr/bridges (e.g when should region->mgr be
>> configured and cleared, as mgr is created/destroyed when mgr parent
>> device platform driver module is loaded/unload), and it maybe not really
>> necessary to introduce more different driver modules here.
>
> It should be pretty easy to create/destroy child devices as shown
> above.  The kernel does this all the time.
>
>>
>> But if it allows multiple fpga-* created under one device in one device
>> driver, it will be much easier to avoid above problems. So I asked if it
>> is possible to create multiple fpga-* under one parent device,
>
> I think it's fine for your FME to create child platform devices.  It's
> similar to a mfd, but the mfd framework hides the platform devices
> from the module that creates them, unfortunately.
>
>> I feel
>> this will not impact to current fpga drivers a lot, but provide more
>> flexibility for drivers to use fpga-region/bridge/manager to create
>> the topology in a device specific way, especially for non device
>> tree case.
>>
>
> I would like to see most of this code as FME enumeration code + a mgr
> driver + a bridge driver + a region driver.  If the FME and the
> enumeration code can be separate files, so much the better for general
> usability.
>
> The enumeration code can build a set of regions by doing something like this:
> 1. figure out what type of mgr and bridges your hardware FME has.
> 2. do platform_device_alloc and platform_device_add to create the mgr
> device, save a pointer to its platform_device in your FME driver's
> private data.
> 2. For each port, create a region and a bridge device.  Save the
> region's platform device or struct in a list in your FME driver's
> priv.
> 3. then you can create the sub function devices.

The above sounds like a poster-child application for MFD. If you do it
in a clever
way (i.e. write your platform drivers in a reusable way) you might be able to
just reuse them on your next generation.

Cheers,

Moritz
--
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
Alan Tull May 8, 2017, 9:02 p.m. UTC | #8
On Mon, May 8, 2017 at 3:52 PM, Moritz Fischer <mdf@kernel.org> wrote:
> On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
>> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -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.
>>>> >>>
>>>> >>
>>>> >> Hi Alan
>>>> >>
>>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>>> >> the same platform device (FME), it allows FME driver to establish the
>>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>>> >> But I found current fpga class API doesn't support this very well.
>>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>>> >> parameter not the shared parent device.
>>>> >
>>>> > That's good feedback.  I can post a couple patches that apply on top
>>>> > of that patchset to add the APIs you need.
>>>> >
>>>> > Probably what I'll do is add
>>>> >
>>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>>> >
>>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>>> following:
>>>> >
>>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>>> >                                 struct fpga_image_info *info);
>>>> >
>>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>>> >                                struct fpga_image_info *info,
>>>> >                                struct list_head *bridge_list);
>>>> >
>>>> > Working on it now.
>>>> >
>>>> >>
>>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>>> >> right direction?
>>>> >
>>>> > That should be fine as long as it's coded with an eye on making things
>>>> > reusable and seeing beyond the current project.  Just thinking of the
>>>> > future and of what can be of general usefulness for others.  And there
>>>> > will be others interested in reusing this.
>>>> >
>>>> > Alan
>>>>
>>>> Actually,  I don't think you will need the additional APIs we were
>>>> just discussing after all.  What you have is a multifunction device
>>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>>> It will have child devices for the mgr, bridges, and regions.  When
>>>> registering the mgr and bridges you will need to allocate child
>>>> devices and use them to create the mgr and bridges.
>>>>
>>>> Alan
>>>
>>> Hi Alan
>>>
>>> I tried to create child devices as the parent device for the mgr and
>>> bridges in fme platform driver module. If only creates the device without
>>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>>> always failed in mgr_get and bridge_get functions.
>>
>> I tried it and it wasn't hard.
>>
>> Each mgr or bridge driver should be a separate file which registers
>> its driver using 'module_platform_driver".  That way the drivers are
>> registered with the kernel in a normal fashion.  The thing we want
>> here is to not bypass the kernel driver model.
>>
>> You'll need to keep the platform_device pointers in private data somewhere.
>>
>> For each child platform device, do a platform_device_alloc and
>> platform_device_add.
>>
>> Then to get the manager, you can do
>>
>> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>>
>> If this is in your probe function, you can use -EPROBE_DEFER if
>> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
>> whatever you've created and return -EPROBE_DEFER to wait for the
>> drivers you need to be registered and ready for devices to be added.
>>
>>>
>>> If it creates platform devices as child devices, and introduce new platform
>>> device drivers for bridge and mgr, then it will be difficult to establish the
>>> relationship for region/mgr/bridges (e.g when should region->mgr be
>>> configured and cleared, as mgr is created/destroyed when mgr parent
>>> device platform driver module is loaded/unload), and it maybe not really
>>> necessary to introduce more different driver modules here.
>>
>> It should be pretty easy to create/destroy child devices as shown
>> above.  The kernel does this all the time.
>>
>>>
>>> But if it allows multiple fpga-* created under one device in one device
>>> driver, it will be much easier to avoid above problems. So I asked if it
>>> is possible to create multiple fpga-* under one parent device,
>>
>> I think it's fine for your FME to create child platform devices.  It's
>> similar to a mfd, but the mfd framework hides the platform devices
>> from the module that creates them, unfortunately.
>>
>>> I feel
>>> this will not impact to current fpga drivers a lot, but provide more
>>> flexibility for drivers to use fpga-region/bridge/manager to create
>>> the topology in a device specific way, especially for non device
>>> tree case.
>>>
>>
>> I would like to see most of this code as FME enumeration code + a mgr
>> driver + a bridge driver + a region driver.  If the FME and the
>> enumeration code can be separate files, so much the better for general
>> usability.
>>
>> The enumeration code can build a set of regions by doing something like this:
>> 1. figure out what type of mgr and bridges your hardware FME has.
>> 2. do platform_device_alloc and platform_device_add to create the mgr
>> device, save a pointer to its platform_device in your FME driver's
>> private data.
>> 2. For each port, create a region and a bridge device.  Save the
>> region's platform device or struct in a list in your FME driver's
>> priv.
>> 3. then you can create the sub function devices.
>
> The above sounds like a poster-child application for MFD. If you do it
> in a clever
> way (i.e. write your platform drivers in a reusable way) you might be able to
> just reuse them on your next generation.

Yes, I played with that with some test code.   I wrote some test code
that allocates dummy mgr and bridge devices using mfd_add_devices().
I didn't see any way of getting access to the devices after creating
them.  Maybe I'm missing something.  Neither the dev nor the
platform_device is saved in the cell struct.

Alan

>
> Cheers,
>
> Moritz
--
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
Moritz Fischer May 8, 2017, 9:11 p.m. UTC | #9
Hi Alan,

On Mon, May 8, 2017 at 2:02 PM, Alan Tull <atull@kernel.org> wrote:
> On Mon, May 8, 2017 at 3:52 PM, Moritz Fischer <mdf@kernel.org> wrote:
>> On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
>>> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -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.
>>>>> >>>
>>>>> >>
>>>>> >> Hi Alan
>>>>> >>
>>>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>>>> >> the same platform device (FME), it allows FME driver to establish the
>>>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>>>> >> But I found current fpga class API doesn't support this very well.
>>>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>>>> >> parameter not the shared parent device.
>>>>> >
>>>>> > That's good feedback.  I can post a couple patches that apply on top
>>>>> > of that patchset to add the APIs you need.
>>>>> >
>>>>> > Probably what I'll do is add
>>>>> >
>>>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>>>> >
>>>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>>>> following:
>>>>> >
>>>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>>>> >                                 struct fpga_image_info *info);
>>>>> >
>>>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>>>> >                                struct fpga_image_info *info,
>>>>> >                                struct list_head *bridge_list);
>>>>> >
>>>>> > Working on it now.
>>>>> >
>>>>> >>
>>>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>>>> >> right direction?
>>>>> >
>>>>> > That should be fine as long as it's coded with an eye on making things
>>>>> > reusable and seeing beyond the current project.  Just thinking of the
>>>>> > future and of what can be of general usefulness for others.  And there
>>>>> > will be others interested in reusing this.
>>>>> >
>>>>> > Alan
>>>>>
>>>>> Actually,  I don't think you will need the additional APIs we were
>>>>> just discussing after all.  What you have is a multifunction device
>>>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>>>> It will have child devices for the mgr, bridges, and regions.  When
>>>>> registering the mgr and bridges you will need to allocate child
>>>>> devices and use them to create the mgr and bridges.
>>>>>
>>>>> Alan
>>>>
>>>> Hi Alan
>>>>
>>>> I tried to create child devices as the parent device for the mgr and
>>>> bridges in fme platform driver module. If only creates the device without
>>>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>>>> always failed in mgr_get and bridge_get functions.
>>>
>>> I tried it and it wasn't hard.
>>>
>>> Each mgr or bridge driver should be a separate file which registers
>>> its driver using 'module_platform_driver".  That way the drivers are
>>> registered with the kernel in a normal fashion.  The thing we want
>>> here is to not bypass the kernel driver model.
>>>
>>> You'll need to keep the platform_device pointers in private data somewhere.
>>>
>>> For each child platform device, do a platform_device_alloc and
>>> platform_device_add.
>>>
>>> Then to get the manager, you can do
>>>
>>> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>>>
>>> If this is in your probe function, you can use -EPROBE_DEFER if
>>> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
>>> whatever you've created and return -EPROBE_DEFER to wait for the
>>> drivers you need to be registered and ready for devices to be added.
>>>
>>>>
>>>> If it creates platform devices as child devices, and introduce new platform
>>>> device drivers for bridge and mgr, then it will be difficult to establish the
>>>> relationship for region/mgr/bridges (e.g when should region->mgr be
>>>> configured and cleared, as mgr is created/destroyed when mgr parent
>>>> device platform driver module is loaded/unload), and it maybe not really
>>>> necessary to introduce more different driver modules here.
>>>
>>> It should be pretty easy to create/destroy child devices as shown
>>> above.  The kernel does this all the time.
>>>
>>>>
>>>> But if it allows multiple fpga-* created under one device in one device
>>>> driver, it will be much easier to avoid above problems. So I asked if it
>>>> is possible to create multiple fpga-* under one parent device,
>>>
>>> I think it's fine for your FME to create child platform devices.  It's
>>> similar to a mfd, but the mfd framework hides the platform devices
>>> from the module that creates them, unfortunately.
>>>
>>>> I feel
>>>> this will not impact to current fpga drivers a lot, but provide more
>>>> flexibility for drivers to use fpga-region/bridge/manager to create
>>>> the topology in a device specific way, especially for non device
>>>> tree case.
>>>>
>>>
>>> I would like to see most of this code as FME enumeration code + a mgr
>>> driver + a bridge driver + a region driver.  If the FME and the
>>> enumeration code can be separate files, so much the better for general
>>> usability.
>>>
>>> The enumeration code can build a set of regions by doing something like this:
>>> 1. figure out what type of mgr and bridges your hardware FME has.
>>> 2. do platform_device_alloc and platform_device_add to create the mgr
>>> device, save a pointer to its platform_device in your FME driver's
>>> private data.
>>> 2. For each port, create a region and a bridge device.  Save the
>>> region's platform device or struct in a list in your FME driver's
>>> priv.
>>> 3. then you can create the sub function devices.
>>
>> The above sounds like a poster-child application for MFD. If you do it
>> in a clever
>> way (i.e. write your platform drivers in a reusable way) you might be able to
>> just reuse them on your next generation.
>
> Yes, I played with that with some test code.   I wrote some test code
> that allocates dummy mgr and bridge devices using mfd_add_devices().
> I didn't see any way of getting access to the devices after creating
> them.  Maybe I'm missing something.  Neither the dev nor the
> platform_device is saved in the cell struct.

Currently working on some MFD stuff for an RTC, which device are you
trying to get to?

The parent from subdevice (fpga mgr?) you can get by something like:

foo_fpga_mgr_probe(struct platform_device *pdev)
{
    struct foo_parent *parent = dev_get_drvdata(dev->parent);
}

Or are you talking about the other way round (i.e. get access to children from
parent device) ? Which functionality would that achieve?

Cheers,

Moritz
--
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
Alan Tull May 8, 2017, 9:20 p.m. UTC | #10
On Mon, May 8, 2017 at 4:11 PM, Moritz Fischer <mdf@kernel.org> wrote:
> Hi Alan,
>
> On Mon, May 8, 2017 at 2:02 PM, Alan Tull <atull@kernel.org> wrote:
>> On Mon, May 8, 2017 at 3:52 PM, Moritz Fischer <mdf@kernel.org> wrote:
>>> On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
>>>> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>>>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>>>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>>>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -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.
>>>>>> >>>
>>>>>> >>
>>>>>> >> Hi Alan
>>>>>> >>
>>>>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>>>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>>>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>>>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>>>>> >> the same platform device (FME), it allows FME driver to establish the
>>>>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>>>>> >> But I found current fpga class API doesn't support this very well.
>>>>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>>>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>>>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>>>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>>>>> >> parameter not the shared parent device.
>>>>>> >
>>>>>> > That's good feedback.  I can post a couple patches that apply on top
>>>>>> > of that patchset to add the APIs you need.
>>>>>> >
>>>>>> > Probably what I'll do is add
>>>>>> >
>>>>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>>>>> >
>>>>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>>>>> following:
>>>>>> >
>>>>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>>>>> >                                 struct fpga_image_info *info);
>>>>>> >
>>>>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>>>>> >                                struct fpga_image_info *info,
>>>>>> >                                struct list_head *bridge_list);
>>>>>> >
>>>>>> > Working on it now.
>>>>>> >
>>>>>> >>
>>>>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>>>>> >> right direction?
>>>>>> >
>>>>>> > That should be fine as long as it's coded with an eye on making things
>>>>>> > reusable and seeing beyond the current project.  Just thinking of the
>>>>>> > future and of what can be of general usefulness for others.  And there
>>>>>> > will be others interested in reusing this.
>>>>>> >
>>>>>> > Alan
>>>>>>
>>>>>> Actually,  I don't think you will need the additional APIs we were
>>>>>> just discussing after all.  What you have is a multifunction device
>>>>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>>>>> It will have child devices for the mgr, bridges, and regions.  When
>>>>>> registering the mgr and bridges you will need to allocate child
>>>>>> devices and use them to create the mgr and bridges.
>>>>>>
>>>>>> Alan
>>>>>
>>>>> Hi Alan
>>>>>
>>>>> I tried to create child devices as the parent device for the mgr and
>>>>> bridges in fme platform driver module. If only creates the device without
>>>>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>>>>> always failed in mgr_get and bridge_get functions.
>>>>
>>>> I tried it and it wasn't hard.
>>>>
>>>> Each mgr or bridge driver should be a separate file which registers
>>>> its driver using 'module_platform_driver".  That way the drivers are
>>>> registered with the kernel in a normal fashion.  The thing we want
>>>> here is to not bypass the kernel driver model.
>>>>
>>>> You'll need to keep the platform_device pointers in private data somewhere.
>>>>
>>>> For each child platform device, do a platform_device_alloc and
>>>> platform_device_add.
>>>>
>>>> Then to get the manager, you can do
>>>>
>>>> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>>>>
>>>> If this is in your probe function, you can use -EPROBE_DEFER if
>>>> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
>>>> whatever you've created and return -EPROBE_DEFER to wait for the
>>>> drivers you need to be registered and ready for devices to be added.
>>>>
>>>>>
>>>>> If it creates platform devices as child devices, and introduce new platform
>>>>> device drivers for bridge and mgr, then it will be difficult to establish the
>>>>> relationship for region/mgr/bridges (e.g when should region->mgr be
>>>>> configured and cleared, as mgr is created/destroyed when mgr parent
>>>>> device platform driver module is loaded/unload), and it maybe not really
>>>>> necessary to introduce more different driver modules here.
>>>>
>>>> It should be pretty easy to create/destroy child devices as shown
>>>> above.  The kernel does this all the time.
>>>>
>>>>>
>>>>> But if it allows multiple fpga-* created under one device in one device
>>>>> driver, it will be much easier to avoid above problems. So I asked if it
>>>>> is possible to create multiple fpga-* under one parent device,
>>>>
>>>> I think it's fine for your FME to create child platform devices.  It's
>>>> similar to a mfd, but the mfd framework hides the platform devices
>>>> from the module that creates them, unfortunately.
>>>>
>>>>> I feel
>>>>> this will not impact to current fpga drivers a lot, but provide more
>>>>> flexibility for drivers to use fpga-region/bridge/manager to create
>>>>> the topology in a device specific way, especially for non device
>>>>> tree case.
>>>>>
>>>>
>>>> I would like to see most of this code as FME enumeration code + a mgr
>>>> driver + a bridge driver + a region driver.  If the FME and the
>>>> enumeration code can be separate files, so much the better for general
>>>> usability.
>>>>
>>>> The enumeration code can build a set of regions by doing something like this:
>>>> 1. figure out what type of mgr and bridges your hardware FME has.
>>>> 2. do platform_device_alloc and platform_device_add to create the mgr
>>>> device, save a pointer to its platform_device in your FME driver's
>>>> private data.
>>>> 2. For each port, create a region and a bridge device.  Save the
>>>> region's platform device or struct in a list in your FME driver's
>>>> priv.
>>>> 3. then you can create the sub function devices.
>>>
>>> The above sounds like a poster-child application for MFD. If you do it
>>> in a clever
>>> way (i.e. write your platform drivers in a reusable way) you might be able to
>>> just reuse them on your next generation.
>>
>> Yes, I played with that with some test code.   I wrote some test code
>> that allocates dummy mgr and bridge devices using mfd_add_devices().
>> I didn't see any way of getting access to the devices after creating
>> them.  Maybe I'm missing something.  Neither the dev nor the
>> platform_device is saved in the cell struct.
>
> Currently working on some MFD stuff for an RTC, which device are you
> trying to get to?
>
> The parent from subdevice (fpga mgr?) you can get by something like:
>
> foo_fpga_mgr_probe(struct platform_device *pdev)
> {
>     struct foo_parent *parent = dev_get_drvdata(dev->parent);
> }
>
> Or are you talking about the other way round (i.e. get access to children from
> parent device) ?

That's it.

> Which functionality would that achieve?

Suppose someone (non-DT case) is enumerating some hardware in the FPGA
that includes a mgr and some bridges.  So they create the mgr device.
Then for each bridge and they want to create a bridge device and a
region device and let the region know what mgr and bridge to use.  The
main enumerating device keeps track of all these regions so if stuff
gets unloaded, it can destroy it all properly.

Alan

>
> Cheers,
>
> Moritz
--
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
Moritz Fischer May 8, 2017, 9:55 p.m. UTC | #11
Hi Alan,

On Mon, May 8, 2017 at 2:20 PM, Alan Tull <atull@kernel.org> wrote:
> On Mon, May 8, 2017 at 4:11 PM, Moritz Fischer <mdf@kernel.org> wrote:
>> Hi Alan,
>>
>> On Mon, May 8, 2017 at 2:02 PM, Alan Tull <atull@kernel.org> wrote:
>>> On Mon, May 8, 2017 at 3:52 PM, Moritz Fischer <mdf@kernel.org> wrote:
>>>> On Mon, May 8, 2017 at 1:44 PM, Alan Tull <atull@kernel.org> wrote:
>>>>> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
>>>>>>> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
>>>>>>> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
>>>>>>> >> On Thu, Apr 20, 2017 at 09:09:47AM -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.
>>>>>>> >>>
>>>>>>> >>
>>>>>>> >> Hi Alan
>>>>>>> >>
>>>>>>> >> Thanks a lot for providing this patch set for non device tree support. :)
>>>>>>> >> Actually I am reworking the Intel FPGA device drivers based on this patch
>>>>>>> >> set, and I find some problems with the existing APIs including fpga bridge
>>>>>>> >> and manager. My idea is to create all fpga bridges/regions/manager under
>>>>>>> >> the same platform device (FME), it allows FME driver to establish the
>>>>>>> >> relationship for the bridges/regions/managers it creates in an easy way.
>>>>>>> >> But I found current fpga class API doesn't support this very well.
>>>>>>> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
>>>>>>> >> parameter, but it doesn't work if we have multiple bridges (and
>>>>>>> >> regions/manager) under the same platform device. fpga_mgr has similar
>>>>>>> >> issue, but fpga_region APIs work better, as they accept fpga_region as
>>>>>>> >> parameter not the shared parent device.
>>>>>>> >
>>>>>>> > That's good feedback.  I can post a couple patches that apply on top
>>>>>>> > of that patchset to add the APIs you need.
>>>>>>> >
>>>>>>> > Probably what I'll do is add
>>>>>>> >
>>>>>>> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
>>>>>>> >
>>>>>>> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
>>>>>>> following:
>>>>>>> >
>>>>>>> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
>>>>>>> >                                 struct fpga_image_info *info);
>>>>>>> >
>>>>>>> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
>>>>>>> >                                struct fpga_image_info *info,
>>>>>>> >                                struct list_head *bridge_list);
>>>>>>> >
>>>>>>> > Working on it now.
>>>>>>> >
>>>>>>> >>
>>>>>>> >> Do you think if having multiple fpga-* under one parent device is in the
>>>>>>> >> right direction?
>>>>>>> >
>>>>>>> > That should be fine as long as it's coded with an eye on making things
>>>>>>> > reusable and seeing beyond the current project.  Just thinking of the
>>>>>>> > future and of what can be of general usefulness for others.  And there
>>>>>>> > will be others interested in reusing this.
>>>>>>> >
>>>>>>> > Alan
>>>>>>>
>>>>>>> Actually,  I don't think you will need the additional APIs we were
>>>>>>> just discussing after all.  What you have is a multifunction device
>>>>>>> (single piece of hardware, multi functions such as in drivers/mfd).
>>>>>>> It will have child devices for the mgr, bridges, and regions.  When
>>>>>>> registering the mgr and bridges you will need to allocate child
>>>>>>> devices and use them to create the mgr and bridges.
>>>>>>>
>>>>>>> Alan
>>>>>>
>>>>>> Hi Alan
>>>>>>
>>>>>> I tried to create child devices as the parent device for the mgr and
>>>>>> bridges in fme platform driver module. If only creates the device without
>>>>>> driver, it doesn't work as try_module_get(dev->parent->driver->owner)
>>>>>> always failed in mgr_get and bridge_get functions.
>>>>>
>>>>> I tried it and it wasn't hard.
>>>>>
>>>>> Each mgr or bridge driver should be a separate file which registers
>>>>> its driver using 'module_platform_driver".  That way the drivers are
>>>>> registered with the kernel in a normal fashion.  The thing we want
>>>>> here is to not bypass the kernel driver model.
>>>>>
>>>>> You'll need to keep the platform_device pointers in private data somewhere.
>>>>>
>>>>> For each child platform device, do a platform_device_alloc and
>>>>> platform_device_add.
>>>>>
>>>>> Then to get the manager, you can do
>>>>>
>>>>> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
>>>>>
>>>>> If this is in your probe function, you can use -EPROBE_DEFER if
>>>>> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
>>>>> whatever you've created and return -EPROBE_DEFER to wait for the
>>>>> drivers you need to be registered and ready for devices to be added.
>>>>>
>>>>>>
>>>>>> If it creates platform devices as child devices, and introduce new platform
>>>>>> device drivers for bridge and mgr, then it will be difficult to establish the
>>>>>> relationship for region/mgr/bridges (e.g when should region->mgr be
>>>>>> configured and cleared, as mgr is created/destroyed when mgr parent
>>>>>> device platform driver module is loaded/unload), and it maybe not really
>>>>>> necessary to introduce more different driver modules here.
>>>>>
>>>>> It should be pretty easy to create/destroy child devices as shown
>>>>> above.  The kernel does this all the time.
>>>>>
>>>>>>
>>>>>> But if it allows multiple fpga-* created under one device in one device
>>>>>> driver, it will be much easier to avoid above problems. So I asked if it
>>>>>> is possible to create multiple fpga-* under one parent device,
>>>>>
>>>>> I think it's fine for your FME to create child platform devices.  It's
>>>>> similar to a mfd, but the mfd framework hides the platform devices
>>>>> from the module that creates them, unfortunately.
>>>>>
>>>>>> I feel
>>>>>> this will not impact to current fpga drivers a lot, but provide more
>>>>>> flexibility for drivers to use fpga-region/bridge/manager to create
>>>>>> the topology in a device specific way, especially for non device
>>>>>> tree case.
>>>>>>
>>>>>
>>>>> I would like to see most of this code as FME enumeration code + a mgr
>>>>> driver + a bridge driver + a region driver.  If the FME and the
>>>>> enumeration code can be separate files, so much the better for general
>>>>> usability.
>>>>>
>>>>> The enumeration code can build a set of regions by doing something like this:
>>>>> 1. figure out what type of mgr and bridges your hardware FME has.
>>>>> 2. do platform_device_alloc and platform_device_add to create the mgr
>>>>> device, save a pointer to its platform_device in your FME driver's
>>>>> private data.
>>>>> 2. For each port, create a region and a bridge device.  Save the
>>>>> region's platform device or struct in a list in your FME driver's
>>>>> priv.
>>>>> 3. then you can create the sub function devices.
>>>>
>>>> The above sounds like a poster-child application for MFD. If you do it
>>>> in a clever
>>>> way (i.e. write your platform drivers in a reusable way) you might be able to
>>>> just reuse them on your next generation.
>>>
>>> Yes, I played with that with some test code.   I wrote some test code
>>> that allocates dummy mgr and bridge devices using mfd_add_devices().
>>> I didn't see any way of getting access to the devices after creating
>>> them.  Maybe I'm missing something.  Neither the dev nor the
>>> platform_device is saved in the cell struct.
>>
>> Currently working on some MFD stuff for an RTC, which device are you
>> trying to get to?
>>
>> The parent from subdevice (fpga mgr?) you can get by something like:
>>
>> foo_fpga_mgr_probe(struct platform_device *pdev)
>> {
>>     struct foo_parent *parent = dev_get_drvdata(dev->parent);
>> }
>>
>> Or are you talking about the other way round (i.e. get access to children from
>> parent device) ?
>
> That's it.
>
>> Which functionality would that achieve?
>
> Suppose someone (non-DT case) is enumerating some hardware in the FPGA
> that includes a mgr and some bridges.  So they create the mgr device.
> Then for each bridge and they want to create a bridge device and a
> region device and let the region know what mgr and bridge to use.  The
> main enumerating device keeps track of all these regions so if stuff
> gets unloaded, it can destroy it all properly.

Ok that might be tougher :( Don't have a solution for that up my sleeve ;-)

Cheers,
Moritz
--
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
Wu, Hao May 9, 2017, 7:16 a.m. UTC | #12
On Mon, May 08, 2017 at 03:44:12PM -0500, Alan Tull wrote:
> On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao.wu@intel.com> wrote:
> >> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@kernel.org> wrote:
> >> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@intel.com> wrote:
> >> >> On Thu, Apr 20, 2017 at 09:09:47AM -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.
> >> >>>
> >> >>
> >> >> Hi Alan
> >> >>
> >> >> Thanks a lot for providing this patch set for non device tree support. :)
> >> >> Actually I am reworking the Intel FPGA device drivers based on this patch
> >> >> set, and I find some problems with the existing APIs including fpga bridge
> >> >> and manager. My idea is to create all fpga bridges/regions/manager under
> >> >> the same platform device (FME), it allows FME driver to establish the
> >> >> relationship for the bridges/regions/managers it creates in an easy way.
> >> >> But I found current fpga class API doesn't support this very well.
> >> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input
> >> >> parameter, but it doesn't work if we have multiple bridges (and
> >> >> regions/manager) under the same platform device. fpga_mgr has similar
> >> >> issue, but fpga_region APIs work better, as they accept fpga_region as
> >> >> parameter not the shared parent device.
> >> >
> >> > That's good feedback.  I can post a couple patches that apply on top
> >> > of that patchset to add the APIs you need.
> >> >
> >> > Probably what I'll do is add
> >> >
> >> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr);
> >> >
> >> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the
> >> following:
> >> >
> >> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br,
> >> >                                 struct fpga_image_info *info);
> >> >
> >> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br,
> >> >                                struct fpga_image_info *info,
> >> >                                struct list_head *bridge_list);
> >> >
> >> > Working on it now.
> >> >
> >> >>
> >> >> Do you think if having multiple fpga-* under one parent device is in the
> >> >> right direction?
> >> >
> >> > That should be fine as long as it's coded with an eye on making things
> >> > reusable and seeing beyond the current project.  Just thinking of the
> >> > future and of what can be of general usefulness for others.  And there
> >> > will be others interested in reusing this.
> >> >
> >> > Alan
> >>
> >> Actually,  I don't think you will need the additional APIs we were
> >> just discussing after all.  What you have is a multifunction device
> >> (single piece of hardware, multi functions such as in drivers/mfd).
> >> It will have child devices for the mgr, bridges, and regions.  When
> >> registering the mgr and bridges you will need to allocate child
> >> devices and use them to create the mgr and bridges.
> >>
> >> Alan
> >
> > Hi Alan
> >
> > I tried to create child devices as the parent device for the mgr and
> > bridges in fme platform driver module. If only creates the device without
> > driver, it doesn't work as try_module_get(dev->parent->driver->owner)
> > always failed in mgr_get and bridge_get functions.
> 
> I tried it and it wasn't hard.
> 
> Each mgr or bridge driver should be a separate file which registers
> its driver using 'module_platform_driver".  That way the drivers are
> registered with the kernel in a normal fashion.  The thing we want
> here is to not bypass the kernel driver model.
> 
> You'll need to keep the platform_device pointers in private data somewhere.
> 
> For each child platform device, do a platform_device_alloc and
> platform_device_add.
> 
> Then to get the manager, you can do
> 
> mgr = fpga_mgr_get(&priv->mgr_pdev->dev);
> 
> If this is in your probe function, you can use -EPROBE_DEFER if
> platform_device_alloc or fpga_mgr_get fail.  Then you could destroy
> whatever you've created and return -EPROBE_DEFER to wait for the
> drivers you need to be registered and ready for devices to be added.
> 
> >
> > If it creates platform devices as child devices, and introduce new platform
> > device drivers for bridge and mgr, then it will be difficult to establish the
> > relationship for region/mgr/bridges (e.g when should region->mgr be
> > configured and cleared, as mgr is created/destroyed when mgr parent
> > device platform driver module is loaded/unload), and it maybe not really
> > necessary to introduce more different driver modules here.
> 
> It should be pretty easy to create/destroy child devices as shown
> above.  The kernel does this all the time.
> 
> >
> > But if it allows multiple fpga-* created under one device in one device
> > driver, it will be much easier to avoid above problems. So I asked if it
> > is possible to create multiple fpga-* under one parent device,
> 
> I think it's fine for your FME to create child platform devices.  It's
> similar to a mfd, but the mfd framework hides the platform devices
> from the module that creates them, unfortunately.
> 
> > I feel
> > this will not impact to current fpga drivers a lot, but provide more
> > flexibility for drivers to use fpga-region/bridge/manager to create
> > the topology in a device specific way, especially for non device
> > tree case.
> >
> 
> I would like to see most of this code as FME enumeration code + a mgr
> driver + a bridge driver + a region driver.  If the FME and the
> enumeration code can be separate files, so much the better for general
> usability.
> 
> The enumeration code can build a set of regions by doing something like this:
> 1. figure out what type of mgr and bridges your hardware FME has.
> 2. do platform_device_alloc and platform_device_add to create the mgr
> device, save a pointer to its platform_device in your FME driver's
> private data.
> 2. For each port, create a region and a bridge device.  Save the
> region's platform device or struct in a list in your FME driver's
> priv.
> 3. then you can create the sub function devices.

Thanks Alan for the suggestion.

I will follow this direction to rework the FME driver. In above step 2,
I will put fpga-mgr/bridge platform device info into the platform data
of the fpga-region platform device, then fpga-region knows which mgr
and bridge to use.

Thanks
Hao
--
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
diff mbox

Patch

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 3b6b2f4..655940f 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,
-				      &region->bridge_list);
+	ret = of_fpga_bridge_get_to_list(region_np->parent, region->info,
+					 &region->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,
-					      &region->bridge_list);
+		ret = of_fpga_bridge_get_to_list(br, region->info,
+						 &region->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);