Re: [PATCH v6 1/5] firmware: add extensible driver data params

[Luca Coelho <luca@coelho.fi>]

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.