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

["Luis R. Rodriguez" <mcgrof@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 <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.

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

Fixed, thanks for the review!

  Luis