Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755051AbcKNR1Y (ORCPT ); Mon, 14 Nov 2016 12:27:24 -0500 Received: from mail-qt0-f173.google.com ([209.85.216.173]:34551 "EHLO mail-qt0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933387AbcKNR1B (ORCPT ); Mon, 14 Nov 2016 12:27:01 -0500 MIME-Version: 1.0 In-Reply-To: References: <20161107001326.7395-1-moritz.fischer@ettus.com> <20161107001326.7395-2-moritz.fischer@ettus.com> From: Moritz Fischer Date: Mon, 14 Nov 2016 09:26:59 -0800 Message-ID: Subject: Re: [PATCH 1/4] fpga mgr: Introduce FPGA capabilities To: atull Cc: Linux Kernel Mailing List , moritz.fischer.private@gmail.com, Michal Simek , =?UTF-8?Q?S=C3=B6ren_Brinkmann?= , linux-arm-kernel , Julia Cartwright 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-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAEHRVP5021592 Content-Length: 2770 Lines: 79 Hi Alan, On Mon, Nov 14, 2016 at 6:06 AM, atull wrote: > On Mon, 7 Nov 2016, Moritz Fischer wrote: > >> Add FPGA capabilities as a way to express the capabilities >> of a given FPGA manager. >> >> Removes code duplication by comparing the low-level driver's >> capabilities at the framework level rather than having each driver >> check for supported operations in the write_init() callback. >> >> This allows for extending with additional capabilities, similar >> to the the dmaengine framework's implementation. >> >> Signed-off-by: Moritz Fischer >> Cc: Alan Tull >> Cc: Michal Simek >> Cc: Sören Brinkmann >> Cc: linux-kernel@vger.kernel.org >> Cc: linux-arm-kernel@lists.infradead.org >> --- >> >> Changes from RFC: >> * in the RFC the caps weren't actually stored into the struct fpga_mgr >> >> Note: >> >> If people disagree on the typedef being a 'false positive' I can fix >> that in a future rev of the patchset. >> >> Thanks, >> >> Moritz >> >> --- >> drivers/fpga/fpga-mgr.c | 15 ++++++++++++++ >> drivers/fpga/socfpga.c | 10 +++++----- >> drivers/fpga/zynq-fpga.c | 7 ++++++- >> include/linux/fpga/fpga-mgr.h | 46 ++++++++++++++++++++++++++++++++++++++++++- >> 4 files changed, 71 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c >> index 953dc91..ed57c17 100644 >> --- a/drivers/fpga/fpga-mgr.c >> +++ b/drivers/fpga/fpga-mgr.c >> @@ -49,6 +49,18 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf, >> struct device *dev = &mgr->dev; >> int ret; >> >> + if (flags & FPGA_MGR_PARTIAL_RECONFIG && >> + !fpga_mgr_has_cap(FPGA_MGR_CAP_PARTIAL_RECONF, mgr->caps)) { >> + dev_err(dev, "Partial reconfiguration not supported\n"); >> + return -ENOTSUPP; >> + } >> + >> + if (flags & FPGA_MGR_FULL_RECONFIG && >> + !fpga_mgr_has_cap(FPGA_MGR_CAP_FULL_RECONF, mgr->caps)) { >> + dev_err(dev, "Full reconfiguration not supported\n"); >> + return -ENOTSUPP; >> + } >> + > > Could you move the checks to their own function like > 'fpga_mgr_check_caps()' or something? I really like it if we can keep > the functions short, like a screen or so where it's practicle to do > so and I could see the number of caps growing here. Absolutely. Great suggestion. > The only counter argument I could think of is if a cap affects the sequence > in this function. Hmmm... Oh you mean the cap being there affecting the sequence in *this* function? I'd suggest we address that when we run into a cap that requires this. Cheers, Moritz