From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753728AbdBPAhN (ORCPT ); Wed, 15 Feb 2017 19:37:13 -0500 Received: from mga04.intel.com ([192.55.52.120]:42004 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753418AbdBPAhK (ORCPT ); Wed, 15 Feb 2017 19:37:10 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,167,1484035200"; d="scan'208";a="225844946" Date: Wed, 15 Feb 2017 16:36:53 -0800 (PST) From: matthew.gerlach@linux.intel.com X-X-Sender: mgerlach@mgerlach-VirtualBox To: Alan Tull cc: Moritz Fischer , Jason Gunthorpe , linux-kernel@vger.kernel.org, linux-fpga@vger.kernel.org Subject: Re: [RFC 1/8] fpga-mgr: add a single function for fpga loading methods In-Reply-To: <1487175261-7051-2-git-send-email-atull@kernel.org> Message-ID: References: <1487175261-7051-1-git-send-email-atull@kernel.org> <1487175261-7051-2-git-send-email-atull@kernel.org> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alan, On Wed, 15 Feb 2017, Alan Tull wrote: > Currently fpga-mgr.c has three methods for loading FPGA's depending on how > the FPGA image is presented: in a sg table, as a single buffer, or as a > firmware file. This commit adds these parameters to the fpga_image_info > stuct and adds a single function that can accept the image as any of these > three. This allows code to be written that could use any of the three > methods. > > Signed-off-by: Alan Tull > --- > drivers/fpga/fpga-mgr.c | 12 ++++++++++++ > drivers/fpga/fpga-region.c | 17 +++++++---------- > include/linux/fpga/fpga-mgr.h | 10 ++++++++++ > 3 files changed, 29 insertions(+), 10 deletions(-) > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index 86d2cb2..b7c719a 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c > @@ -309,6 +309,18 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr, > } > EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load); > > +int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info) > +{ > + if (info->firmware_name) > + return fpga_mgr_firmware_load(mgr, info, info->firmware_name); > + if (info->sgt) > + return fpga_mgr_buf_load_sg(mgr, info, info->sgt); > + if (info->buf && info->count) > + return fpga_mgr_buf_load(mgr, info, info->buf, info->count); > + return -EINVAL; > +} > +EXPORT_SYMBOL_GPL(fpga_mgr_load); > + I like this cleaner api. > static const char * const state_str[] = { > [FPGA_MGR_STATE_UNKNOWN] = "unknown", > [FPGA_MGR_STATE_POWER_OFF] = "power off", > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > index 3222fdb..24f4ed5 100644 > --- a/drivers/fpga/fpga-region.c > +++ b/drivers/fpga/fpga-region.c > @@ -223,14 +223,11 @@ static int fpga_region_get_bridges(struct fpga_region *region, > /** > * fpga_region_program_fpga - program FPGA > * @region: FPGA region > - * @firmware_name: name of FPGA image firmware file > * @overlay: device node of the overlay > - * Program an FPGA using information in the device tree. > - * Function assumes that there is a firmware-name property. > + * Program an FPGA using information in the region's fpga image info. > * Return 0 for success or negative error code. > */ > static int fpga_region_program_fpga(struct fpga_region *region, > - const char *firmware_name, > struct device_node *overlay) > { > struct fpga_manager *mgr; > @@ -260,7 +257,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, > goto err_put_br; > } > > - ret = fpga_mgr_firmware_load(mgr, region->info, firmware_name); > + ret = fpga_mgr_load(mgr, region->info); > if (ret) { > pr_err("failed to load fpga image\n"); > goto err_put_br; > @@ -351,7 +348,6 @@ static int child_regions_with_firmware(struct device_node *overlay) > static int fpga_region_notify_pre_apply(struct fpga_region *region, > struct of_overlay_notify_data *nd) > { > - const char *firmware_name = NULL; > struct fpga_image_info *info; > int ret; > > @@ -373,7 +369,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region, > if (of_property_read_bool(nd->overlay, "external-fpga-config")) > info->flags |= FPGA_MGR_EXTERNAL_CONFIG; > > - of_property_read_string(nd->overlay, "firmware-name", &firmware_name); > + of_property_read_string(nd->overlay, "firmware-name", > + &info->firmware_name); > > of_property_read_u32(nd->overlay, "region-unfreeze-timeout-us", > &info->enable_timeout_us); > @@ -382,7 +379,7 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region, > &info->disable_timeout_us); > > /* If FPGA was externally programmed, don't specify firmware */ > - if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && firmware_name) { > + if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && info->firmware_name) { > pr_err("error: specified firmware and external-fpga-config"); > return -EINVAL; > } > @@ -392,12 +389,12 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region, > return 0; > > /* If we got this far, we should be programming the FPGA */ > - if (!firmware_name) { > + if (!info->firmware_name) { > pr_err("should specify firmware-name or external-fpga-config\n"); > return -EINVAL; > } > > - return fpga_region_program_fpga(region, firmware_name, nd->overlay); > + return fpga_region_program_fpga(region, nd->overlay); > } > > /** > diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h > index 57beb5d..45df05a 100644 > --- a/include/linux/fpga/fpga-mgr.h > +++ b/include/linux/fpga/fpga-mgr.h > @@ -76,11 +76,19 @@ enum fpga_mgr_states { > * @flags: boolean flags as defined above > * @enable_timeout_us: maximum time to enable traffic through bridge (uSec) > * @disable_timeout_us: maximum time to disable traffic through bridge (uSec) > + * @firmware_name: name of FPGA image firmware file > + * @sgt: scatter/gather table containing FPGA image > + * @buf: contiguous buffer containing FPGA image > + * @count: size of buf > */ > struct fpga_image_info { > u32 flags; > u32 enable_timeout_us; > u32 disable_timeout_us; We really need to address your patch adds the config_complete_timout_us. It seems like it would conflict with this patch. > + const char *firmware_name; > + struct sg_table *sgt; > + const char *buf; > + size_t count; > }; > > /** > @@ -139,6 +147,8 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr, > struct fpga_image_info *info, > const char *image_name); > > +int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info); > + > struct fpga_manager *of_fpga_mgr_get(struct device_node *node); > > struct fpga_manager *fpga_mgr_get(struct device *dev); > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >