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
