Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751783AbdINUEf (ORCPT ); Thu, 14 Sep 2017 16:04:35 -0400 Received: from mail.kernel.org ([198.145.29.99]:43624 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751673AbdINUEa (ORCPT ); Thu, 14 Sep 2017 16:04:30 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CFEC221E91 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=atull@kernel.org X-Google-Smtp-Source: AOwi7QAkqS2+hOlP2eHe5TgToJBdQVe0LawnbrZwJQM489+CiwpZJjZ9tqtCl4AmahZQNNmx0+zmAC8IqR9Bzb02mz8= MIME-Version: 1.0 In-Reply-To: <20170914095616.GB10693@hao-dev> References: <20170913204841.2730-1-atull@kernel.org> <20170913204841.2730-14-atull@kernel.org> <20170914095616.GB10693@hao-dev> From: Alan Tull Date: Thu, 14 Sep 2017 15:03:48 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 13/18] fpga: region: add register/unregister functions To: Wu Hao Cc: Moritz Fischer , "linux-kernel@vger.kernel.org" , "linux-fpga@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7327 Lines: 219 On Thu, Sep 14, 2017 at 4:56 AM, Wu Hao wrote: > On Thu, Sep 14, 2017 at 04:48:36AM +0800, Alan Tull wrote: >> Another step in separating common code from device tree specific >> code for FPGA regions. >> >> * add FPGA region register/unregister functions. >> * add the register/unregister functions to the header >> * use devm_kzalloc to alloc the region. >> * add a method for getting bridges to the region struct >> * add priv to the region struct >> * use region->info in of_fpga_region_get_bridges >> >> Signed-off-by: Alan Tull >> --- >> v2: split out from another patch >> v3: use region->info, remove info param where applicable >> v4: no change to this patch in this version of patchset >> --- >> drivers/fpga/fpga-region.c | 106 ++++++++++++++++++++++++--------------- >> include/linux/fpga/fpga-region.h | 7 +++ >> 2 files changed, 72 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c >> index 92ab216..ce57383 100644 >> --- a/drivers/fpga/fpga-region.c >> +++ b/drivers/fpga/fpga-region.c >> @@ -143,7 +143,6 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np) >> /** >> * of_fpga_region_get_bridges - create a list of bridges >> * @region: FPGA region >> - * @info: FPGA image info >> * >> * Create a list of bridges including the parent bridge and the bridges >> * specified by "fpga-bridges" property. Note that the >> @@ -156,11 +155,11 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np) >> * Return 0 for success (even if there are no bridges specified) >> * or -EBUSY if any of the bridges are in use. >> */ >> -static int of_fpga_region_get_bridges(struct fpga_region *region, >> - struct fpga_image_info *info) >> +static int of_fpga_region_get_bridges(struct fpga_region *region) >> { >> struct device *dev = ®ion->dev; >> struct device_node *region_np = dev->of_node; >> + struct fpga_image_info *info = region->info; >> struct device_node *br, *np, *parent_br = NULL; >> int i, ret; >> >> @@ -192,7 +191,7 @@ static int of_fpga_region_get_bridges(struct fpga_region *region, >> continue; >> >> /* If node is a bridge, get it and add to list */ >> - ret = of_fpga_bridge_get_to_list(br, region->info, >> + ret = of_fpga_bridge_get_to_list(br, info, >> ®ion->bridge_list); >> >> /* If any of the bridges are in use, give up */ >> @@ -229,10 +228,16 @@ int fpga_region_program_fpga(struct fpga_region *region) >> goto err_put_region; >> } >> >> - ret = of_fpga_region_get_bridges(region, info); >> - if (ret) { >> - dev_err(dev, "failed to get FPGA bridges\n"); >> - goto err_unlock_mgr; >> + /* >> + * In some cases, we already have a list of bridges in the >> + * fpga region struct. Or we don't have any bridges. >> + */ >> + if (region->get_bridges) { >> + ret = region->get_bridges(region); >> + if (ret) { >> + dev_err(dev, "failed to get fpga region bridges\n"); >> + goto err_unlock_mgr; >> + } >> } >> >> ret = fpga_bridges_disable(®ion->bridge_list); >> @@ -259,7 +264,8 @@ int fpga_region_program_fpga(struct fpga_region *region) >> return 0; >> >> err_put_br: >> - fpga_bridges_put(®ion->bridge_list); >> + if (region->get_bridges) >> + fpga_bridges_put(®ion->bridge_list); >> err_unlock_mgr: >> fpga_mgr_unlock(region->mgr); >> err_put_region: >> @@ -522,39 +528,20 @@ static struct notifier_block fpga_region_of_nb = { >> .notifier_call = of_fpga_region_notify, >> }; >> >> -static int of_fpga_region_probe(struct platform_device *pdev) >> +int fpga_region_register(struct device *dev, struct fpga_region *region) >> { >> - struct device *dev = &pdev->dev; >> - struct device_node *np = dev->of_node; >> - struct fpga_region *region; >> - struct fpga_manager *mgr; >> int id, ret = 0; >> >> - mgr = of_fpga_region_get_mgr(np); >> - if (IS_ERR(mgr)) >> - return -EPROBE_DEFER; >> - >> - region = kzalloc(sizeof(*region), GFP_KERNEL); >> - if (!region) { >> - ret = -ENOMEM; >> - goto err_put_mgr; >> - } >> - >> - region->mgr = mgr; >> - >> id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL); >> - if (id < 0) { >> - ret = id; >> - goto err_kfree; >> - } >> + if (id < 0) >> + return id; >> >> mutex_init(®ion->mutex); >> INIT_LIST_HEAD(®ion->bridge_list); >> - >> device_initialize(®ion->dev); >> region->dev.class = fpga_region_class; >> region->dev.parent = dev; >> - region->dev.of_node = np; >> + region->dev.of_node = dev->of_node; >> region->dev.id = id; >> dev_set_drvdata(dev, region); >> >> @@ -566,19 +553,58 @@ static int of_fpga_region_probe(struct platform_device *pdev) >> if (ret) >> goto err_remove; >> >> + return 0; >> + >> +err_remove: >> + ida_simple_remove(&fpga_region_ida, id); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(fpga_region_register); >> + >> +int fpga_region_unregister(struct fpga_region *region) >> +{ >> + device_unregister(®ion->dev); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(fpga_region_unregister); >> + >> +static int of_fpga_region_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + struct fpga_region *region; >> + struct fpga_manager *mgr; >> + int ret; >> + >> + /* Find the FPGA mgr specified by region or parent region. */ >> + mgr = of_fpga_region_get_mgr(np); >> + if (IS_ERR(mgr)) >> + return -EPROBE_DEFER; >> + >> + region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL); >> + if (!region) { >> + ret = -ENOMEM; >> + goto eprobe_mgr_put; >> + } >> + >> + region->mgr = mgr; >> + >> + /* Specify how to get bridges for this type of region. */ >> + region->get_bridges = of_fpga_region_get_bridges; >> + >> + ret = fpga_region_register(dev, region); >> + if (ret) >> + goto eprobe_mgr_put; >> + >> of_platform_populate(np, fpga_region_of_match, NULL, ®ion->dev); >> >> dev_info(dev, "FPGA Region probed\n"); >> >> return 0; >> >> -err_remove: >> - ida_simple_remove(&fpga_region_ida, id); >> -err_kfree: >> - kfree(region); >> -err_put_mgr: >> +eprobe_mgr_put: >> fpga_mgr_put(mgr); >> - >> return ret; >> } >> >> @@ -586,8 +612,7 @@ static int of_fpga_region_remove(struct platform_device *pdev) >> { >> struct fpga_region *region = platform_get_drvdata(pdev); >> >> - device_unregister(®ion->dev); >> - fpga_mgr_put(region->mgr); >> + fpga_region_unregister(region); > > Hi Alan > > Do you think if we need to keep fpga_mgr_put(region->mgr) in this remove > function? :) Hi Hao, Thanks for catching that! I'll add it back in. Alan > > Thanks > Hao