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

[Wu Hao]

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

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