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

[Wu, Hao]

> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <atull@xxxxxxxxxx> wrote:
> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao.wu@xxxxxxxxx> 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