Message-ID: <1491463572.28535.44.camel@coelho.fi>
From: Luca Coelho <luca@coelho.fi>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>, gregkh@linuxfoundation.org
Cc: 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
Date: Thu, 06 Apr 2017 10:26:12 +0300
In-Reply-To: <20170330032514.17173-2-mcgrof@kernel.org>
References: <20170330032514.17173-1-mcgrof@kernel.org>
         <20170330032514.17173-2-mcgrof@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [PATCH v6 1/5] firmware: add extensible driver data params
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4032
Lines: 134

On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
> As the firmware API evolves we keep extending functions with more arguments.
> Stop this nonsense by proving an extensible data structure which can be used
> to represent both user parameters and private internal parameters.
> 
> We introduce 3 data structures:
> 
>   o struct driver_data_req_params  - used for user specified parameters
>   o struct driver_data_priv_params - used for internal use
>   o struct driver_data_params - stiches both of the the above together,
> 				also only for internal use
> 
> This starts off by just making the existing APIs use the new data
> structures, it will make subsequent changes easier to review which will
> be adding new flexible APIs.
> 
> This commit should introduces no functional changes (TM).
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---

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.


[...]

> @@ -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.


>  	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 <linux/types.h>
> +#include <linux/compiler.h>
> +#include <linux/gfp.h>
> +#include <linux/device.h>
> +
> +/*
> + * Driver Data internals
> + *
> + * Copyright (C) 2017 Luis R. Rodriguez <mcgrof@kernel.org>
> + *
> + * 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.

--
Cheers,
Luca.