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

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Thu, May 11, 2017 at 01:17:39PM -0500, Li, Yi wrote:
> On 5/2/2017 3:49 AM, 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 only
> >    o struct driver_data_params - stiches both of the the above together,
> > 				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.
> > 
> > A side consequences is get to replace all the old internal "firmware
> > behavior  options" flags with enums we properly document, remove the
> > blinding #ifdefs, and compartamentlize the userhelper fallback code
> > more appropriately unde CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
> > 
> > This commit should introduces no functional changes (TM).
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > ---
> >   drivers/base/firmware_class.c | 331 ++++++++++++++++++++++++++++++++----------
> >   include/linux/driver_data.h   |  82 +++++++++++
> >   2 files changed, 339 insertions(+), 74 deletions(-)
> >   create mode 100644 include/linux/driver_data.h
> > ...
> > diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h
> > new file mode 100644
> > index 000000000000..7ce3216a9a99
> > --- /dev/null
> > +++ b/include/linux/driver_data.h
> > @@ -0,0 +1,82 @@
> > +#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/.
> > + */
> > +
> > +/**
> > + * struct driver_data_async_cbs - callbacks for handling driver data requests
> > + * @found_cb: callback to be used when the driver data has been found. A
> > + *	callback is required. If the requested driver data is found it will
> > + *	passed on the callback, using the context set on @found_ctx.
> > + * @found_ctx: preferred context to be used as the second argument to
> > + * 	@found_cb.
> > + *
> > + * Used for specifying callbacks and contexts used for when asynchronous driver
> > + * data requests have completed. If no driver data is found the error will be
> > + * passed on the respective callback.
> > + */
> > +struct driver_data_async_cbs {
> > +	void (*found_cb)(const struct firmware *driver_data,
> > +			 void *context,
> > +			 int error);
> 
> Any reason why the call back function format are not consistent between
> async and sync mode.  In sync mode, the context is before struct firmware.
> in Async mode, it's reversed.
> 
> struct driver_data_sync_cbs {
>     int __must_check
>         (*found_cb)(void *context,
>                 const struct firmware *driver_data,
>                 int error);
> 

Indeed, its just to ensure if the user makes a mistake and uses a sync
callback for an async request we get a compile error, all without using
typdefs or anything nasty.

  Luis