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

[Alan Tull]

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

>
>> 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
>>