Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934487AbdD0DYH (ORCPT ); Wed, 26 Apr 2017 23:24:07 -0400 Received: from mx2.suse.de ([195.135.220.15]:42643 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751410AbdD0DYA (ORCPT ); Wed, 26 Apr 2017 23:24:00 -0400 Date: Thu, 27 Apr 2017 05:23:58 +0200 From: "Luis R. Rodriguez" To: "takahiro.akashi@linaro.org" , "Coelho, Luciano" , "gregkh@linuxfoundation.org" , "mcgrof@kernel.org" , "linux-kernel@vger.kernel.org" , "pjones@redhat.com" , "moritz.fischer@ettus.com" , "dhowells@redhat.com" , "pmladek@suse.com" , "Berg, Johannes" , "rjw@rjwysocki.net" , "yi1.li@linux.intel.com" , "kvalo@codeaurora.org" , "luto@kernel.org" , "arend.vanspriel@broadcom.com" , "rafal@milecki.pl" , "dwmw2@infradead.org" , "wagi@monom.org" , "atull@opensource.altera.com" , "Grumbach, Emmanuel" Subject: Re: [PATCH v6 2/5] firmware: add extensible driver data API Message-ID: <20170427032358.GJ28800@wotan.suse.de> References: <20170330032514.17173-1-mcgrof@kernel.org> <20170330032514.17173-3-mcgrof@kernel.org> <1491828162.31980.50.camel@intel.com> <20170411080149.GD15139@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170411080149.GD15139@linaro.org> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2517 Lines: 76 On Tue, Apr 11, 2017 at 05:01:51PM +0900, takahiro.akashi@linaro.org wrote: > On Mon, Apr 10, 2017 at 12:42:44PM +0000, Coelho, Luciano wrote: > > On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote: > > > +Driver data request parameters > > > +============================== > > > + > > > +Variations of types of driver data requests are specified by a driver data > > > +request parameter data structure. This data structure is expected to grow as > > > +new requirements grow. > > > > Again, not sure it's relevant to know that it can grow. For > > documentation purposes, the important is the *now*. > > There seem to be a couple of new features/parameters. > Why not list them now: > * optional > * keep > * API versioning We can document this on the driver header file and pull in the relevant doc. > I will add 'data(firmware) signing' here afterward. Great! > > > +/** > > > + * driver_data_request - synchronous request for a driver data file > > driver_data_request_sync Fixed. > > > + * @name: name of the driver data file > > > + * @params: driver data parameters, it provides all the requirements > > req_params Fixed. > > > +int driver_data_request_sync(const char *name, > > > + const struct driver_data_req_params *req_params, > > > + struct device *device) > > > +{ > > > + const struct firmware *driver_data; > > > + const struct driver_data_reqs *sync_reqs; > > > + struct driver_data_params params = { > > > + .req_params = *req_params, > > > + }; > > > + int ret; > > > + > > > + if (!device || !req_params || !name || name[0] == '\0') > > > + return -EINVAL; > > > + > > > + if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC) > > > + return -EINVAL; > > > > Why do you need to check this here? If the caller is calling _sync(), > > it's because that's what it needs. This mode value here seems > > redundant. > > > > OTOH, if you do have a reason for this value, then you could use > > driver_data_request_sync() in this if. > > I think two functions, driver_data_request_[a]sync(), can be > unified into one: > int driver_data_request(const char *name, > const struct driver_data_req_params *req_params, > struct device *device) > Indeed but I decided to draw the fine line on differences between sync/async, following similar core API trends in the kernel, like module request, etc. The implementation details can vary on setup for async so best also allow us to easily trace these uses on the actual driver calls. Luis