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