Re: [PATCH v6 2/5] firmware: add extensible driver data API

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Tue, Apr 11, 2017 at 05:01:51PM +0900, takahiro.akashi@linaro.org wrote:
> On Mon, Apr 10, 2017 at 12:42:44PM +0000, Coelho, Luciano wrote:
> > On Wed, 2017-03-29 at 20:25 -0700, Luis R. Rodriguez wrote:
> > > +Driver data request parameters
> > > +==============================
> > > +
> > > +Variations of types of driver data requests are specified by a driver data
> > > +request parameter data structure. This data structure is expected to grow as
> > > +new requirements grow.
> > 
> > Again, not sure it's relevant to know that it can grow.  For
> > documentation purposes, the important is the *now*.
> 
> There seem to be a couple of new features/parameters.
> Why not list them now:
>   * optional
>   * keep
>   * API versioning

We can document this on the driver header file and pull in the relevant doc.

> I will add 'data(firmware) signing' here afterward.

Great!

> > > +/**
> > > + * driver_data_request - synchronous request for a driver data file
> 
> driver_data_request_sync

Fixed.

> > > + * @name: name of the driver data file
> > > + * @params: driver data parameters, it provides all the requirements
> 
> req_params

Fixed.

> > > +int driver_data_request_sync(const char *name,
> > > +			     const struct driver_data_req_params *req_params,
> > > +			     struct device *device)
> > > +{
> > > +	const struct firmware *driver_data;
> > > +	const struct driver_data_reqs *sync_reqs;
> > > +	struct driver_data_params params = {
> > > +		.req_params = *req_params,
> > > +	};
> > > +	int ret;
> > > +
> > > +	if (!device || !req_params || !name || name[0] == '\0')
> > > +		return -EINVAL;
> > > +
> > > +	if (req_params->sync_reqs.mode != DRIVER_DATA_SYNC)
> > > +		return -EINVAL;
> > 
> > Why do you need to check this here? If the caller is calling _sync(),
> > it's because that's what it needs.  This mode value here seems
> > redundant.
> > 
> > OTOH, if you do have a reason for this value, then you could use
> > driver_data_request_sync() in this if.
> 
> I think two functions, driver_data_request_[a]sync(), can be
> unified into one:
> int driver_data_request(const char *name,
> 		        const struct driver_data_req_params *req_params,
> 		        struct device *device)
> 

Indeed but I decided to draw the fine line on differences between sync/async,
following similar core API trends in the kernel, like module request, etc.
The implementation details can vary on setup for async so best also allow us
to easily trace these uses on the actual driver calls.

  Luis