From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932602AbdEDJ1K (ORCPT ); Thu, 4 May 2017 05:27:10 -0400 Received: from mga06.intel.com ([134.134.136.31]:40239 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754080AbdEDJZm (ORCPT ); Thu, 4 May 2017 05:25:42 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,287,1491289200"; d="scan'208";a="964283158" Date: Thu, 4 May 2017 17:20:49 +0800 From: Wu Hao To: Alan Tull Cc: Moritz Fischer , linux-kernel , linux-fpga@vger.kernel.org, matthew.gerlach@linux.intel.com Subject: Re: [PATCH v2 02/16] fpga: bridge: support getting bridge from device Message-ID: <20170504092049.GA3832@hao-dev> References: <1492697401-11211-1-git-send-email-atull@kernel.org> <1492697401-11211-3-git-send-email-atull@kernel.org> <20170503115831.GA30448@hao-dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 03, 2017 at 03:07:33PM -0500, Alan Tull wrote: > On Wed, May 3, 2017 at 6:58 AM, Wu Hao 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 > >