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
