Re: [PATCH] firmware: cleanup - group and document up private firmware parameters

[Luis R. Rodriguez]

On Fri, Sep 15, 2017 at 10:30:46AM +0200, Martin Fuzzey wrote:
> On 15/09/17 00:54, Luis R. Rodriguez wrote:
> > The above benefits makes the code much easier to understand and maintain.
> 
> Yes I agree it is much cleaner that way.
> 
> A couple of nitpicks below.
> 
> > +/**
> > + * enum fw_priv_reqs - private features only used internally
> > + *
> > + * @FW_PRIV_REQ_FALLBACK: specifies that the firmware request
> > + *	will use a fallback mechanism if the kernel's direct filesystem
> > + *	lookup failed to find the requested firmware. If the flag
> > + *	%FW_PRIV_REQ_FALLBACK is set but the flag
> > + *	%FW_PRIV_REQ_FALLBACK_UEVENT is not set, it means the caller
> > + *	is relying on a custom fallback mechanism for firmwarwe lookup as a
> > + *	fallback mechanism. The custom fallback mechanism is expected to find
> > + *	any found firmware using the exposed sysfs interface of the
> > + *	firmware_class.  Since the custom fallback mechanism is not compatible
> > + *	with the internal caching mechanism for firmware lookups at resume,
> > + *	caching will be disabled when the custom fallback mechanism is used.
> > + * @FW_PRIV_REQ_FALLBACK_UEVENT: indicates that the fallback mechanism
> > + *	this firmware request will rely on will be that of having the kernel
> > + *	issue a uevent to userspace. Userspace in turn is expected to be
> > + *	monitoring for uevents for the firmware_class and will use the
> > + *	exposted sysfs interface to upload the firmware for the caller.
> > + * @FW_PRIV_REQ_NO_CACHE: indicates that the firmware request
> > + *	should not set up and use the internal caching mechanism to assist
> > + *	drivers from fetching firmware at resume time after suspend.
> > + * @FW_PRIV_REQ_OPTIONAL: if set it is not a hard requirement by the
> > + *	caller that the file requested be present. An error will not be recorded
> > + *	if the file is not found.
> > + */
> > +enum fw_priv_reqs {
> > +	FW_PRIV_REQ_FALLBACK			= 1 << 0,
> > +	FW_PRIV_REQ_FALLBACK_UEVENT		= 1 << 1,
> > +	FW_PRIV_REQ_NO_CACHE			= 1 << 2,
> > +	FW_PRIV_REQ_OPTIONAL			= 1 << 3,
> > +};
> > +
> 
> Why REQ ?
> Looks more like a set of flags to me.
> Wouldn't FW_PRIV_FLAG_XXX be better?

Sure, its much better without anything so will just go with FW_PRIV_ as the
prefix.

> > +/**
> > + * struct fw_priv_params - private firmware parameters
> > + * @mode: mode of operation
> > + * @priv_reqs: private set of &enum fw_priv_reqs, private requirements for
> > + *	the firmware request
> > + * @alloc_buf: buffer area allocated by the caller so we can place the
> > + *	respective firmware
> > + * @alloc_buf_size: size of the @alloc_buf
> > + */
> > +struct fw_priv_params {
> > +	enum fw_api_mode mode;
> > +	u64 priv_reqs;
> 
> Not sure the priv_ prefix in the priv_reqs is necessary since the whole
> structure is private.
> I'd have named it just flags.

Went with priv_flags.

Thanks for the feedback!

  Luis