Messages in this thread | | | Subject | Re: [PATCH] firmware: cleanup - group and document up private firmware parameters | From | Martin Fuzzey <> | Date | Fri, 15 Sep 2017 10:30:46 +0200 |
| |
Hi Luis,
On 15/09/17 00:54, Luis R. Rodriguez wrote: > The firmware API has a slew of private options available, which can > sometimes be hard to understand. When new functionality is introduced > we also tend to have modify a slew of internal helpers. > > Just stuff all common private requirements into its own data structure > and move features into properly defined flags which can then be carefully > documented. This: > > o reduces the amount of changes we have make as we advance functionality > o helps remove the #ifdef mess we had created for private features > > 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?
> +/** > + * 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.
Regards,
Martin
| |