On Thu, 2017-04-27 at 05:16 +0200, Luis R. Rodriguez wrote:
> > > +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.
>
> Because we allow one data structure to be used for the specified requirements
> and technically someone can make a mistake and use the wrong macros to set up
> the data structure. This ensures users don't async macros to set up the
> parameters and then use the sync call. Eventually the underlying
> firmware_class.c code does its own conditional checks on this as well.
If this could only happen in a programming error, maybe it's worth a
WARN() then, to make it easier to spot?
> > OTOH, if you do have a reason for this value, then you could use
> > driver_data_request_sync() in this if.
>
> You mean to allow *one* API call for all. Sure, that's possible, but I think
> its nicer to at least expose async/sync mechanisms on the caller side. The
> sync/async differences seem like a reasonable enough place to split the API.
I don't remember the details of this anymore, but doesn't the
driver_data_sync() function does exactly the same check? I meant that
you could do this:
if(WARN_ON_ONCE(!driver_data_request_sync()))
return -EINVAL;
And yes, I think either a single API call for all or not having the mode
in the struct would be cleaner. I think there are better ways to
prevent coding errors (since this should only happen if the user code is
implemented incorrectly).
--
Cheers,
Luca.
