From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1033471AbdD0CFV (ORCPT ); Wed, 26 Apr 2017 22:05:21 -0400 Received: from mx2.suse.de ([195.135.220.15]:38933 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1033451AbdD0CFM (ORCPT ); Wed, 26 Apr 2017 22:05:12 -0400 Date: Thu, 27 Apr 2017 04:05:09 +0200 From: "Luis R. Rodriguez" To: Luca Coelho Cc: "Luis R. Rodriguez" , gregkh@linuxfoundation.org, wagi@monom.org, dwmw2@infradead.org, rafal@milecki.pl, arend.vanspriel@broadcom.com, rjw@rjwysocki.net, yi1.li@linux.intel.com, atull@opensource.altera.com, moritz.fischer@ettus.com, pmladek@suse.com, johannes.berg@intel.com, emmanuel.grumbach@intel.com, kvalo@codeaurora.org, luto@kernel.org, takahiro.akashi@linaro.org, dhowells@redhat.com, pjones@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 1/5] firmware: add extensible driver data params Message-ID: <20170427020509.GH28800@wotan.suse.de> References: <20170330032514.17173-1-mcgrof@kernel.org> <20170330032514.17173-2-mcgrof@kernel.org> <1491463572.28535.44.camel@coelho.fi> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1491463572.28535.44.camel@coelho.fi> 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 On Thu, Apr 06, 2017 at 10:26:12AM +0300, Luca Coelho wrote: > On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote: > > Signed-off-by: Luis R. Rodriguez > > --- > > Looks fine with a few nitpicks. > > > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > > index 54fc4c42f126..f702566554e1 100644 > > --- a/drivers/base/firmware_class.c > > +++ b/drivers/base/firmware_class.c > > [...] > > > @@ -40,6 +41,77 @@ MODULE_AUTHOR("Manuel Estrada Sainz"); > > MODULE_DESCRIPTION("Multi purpose firmware loading support"); > > MODULE_LICENSE("GPL"); > > > > +struct driver_data_priv_params { > > + bool use_fallback; > > + bool use_fallback_uevent; > > + bool no_cache; > > + void *alloc_buf; > > + size_t alloc_buf_size; > > +}; > > + > > +struct driver_data_params { > > + const struct firmware *driver_data; > > + const struct driver_data_req_params req_params; > > + struct driver_data_priv_params priv_params; > > +}; > > + > > +/* > > + * These are kept to remain backward compatible with old behaviour. Do not > > + * modify them unless you know what you are doing. These are to be used only > > + * by the old API, so: > > + * > > + * Old sync APIs: > > + * o request_firmware(): __DATA_REQ_FIRMWARE() > > + * o request_firmware_direct(): __DATA_REQ_FIRMWARE_DIRECT() > > + * o request_firmware_into_buf(): __DATA_REQ_FIRMWARE_BUF() > > + * > > + * Old async API: > > + * o request_firmware_nowait(): __DATA_REQ_FIRMWARE_NOWAIT() > > + */ > > +#define __DATA_REQ_FIRMWARE() \ > > + .priv_params = { \ > > + .use_fallback = !!FW_OPT_FALLBACK, \ > > use_fallback is a boolean, so you don't need the double negation here. Actually FW_OPT_FALLBACK can end up being (1U << 2) and I rather really force just a true or false here, so will prefer to be pedantic and keep the double negation as FW_OPT_FALLBACK is a flag. > [...] > > > @@ -1332,12 +1433,15 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name, > > struct device *device, void *buf, size_t size) > > { > > int ret; > > + struct driver_data_params data_params = { > > + __DATA_REQ_FIRMWARE_BUF(buf, size), > > + }; > > > > __module_get(THIS_MODULE); > > - ret = _request_firmware(firmware_p, name, device, buf, size, > > - FW_OPT_UEVENT | FW_OPT_FALLBACK | > > - FW_OPT_NOCACHE); > > + ret = _request_firmware(firmware_p, name, &data_params, device); > > module_put(THIS_MODULE); > > + > > + > > Double empty-lines here. Fixed. > > return ret; > > } > > EXPORT_SYMBOL(request_firmware_into_buf); > > > > [...] > > > diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h > > new file mode 100644 > > index 000000000000..c3d3a4129838 > > --- /dev/null > > +++ b/include/linux/driver_data.h > > @@ -0,0 +1,88 @@ > > +#ifndef _LINUX_DRIVER_DATA_H > > +#define _LINUX_DRIVER_DATA_H > > + > > +#include > > +#include > > +#include > > +#include > > + > > +/* > > + * Driver Data internals > > + * > > + * Copyright (C) 2017 Luis R. Rodriguez > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms of copyleft-next (version 0.3.1 or later) as published > > + * at http://copyleft-next.org/. > > + */ > > + > > +/** > > + * enum driver_data_mode - driver data mode of operation > > + * > > + * DRIVER_DATA_SYNC: your call to request driver data is synchronous. We will > > + * look for the driver data file you have requested immediatley. > > + * DRIVER_DATA_ASYNC: your call to request driver data is asynchronous. We will > > It should be @DRIVER_DATA_SYNC and @DRIVER_DATA_ASYNC here. Fixed, thanks for the review! Luis