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

From: Greg KH
Date: Mon Sep 18 2017 - 11:14:57 EST


On Thu, Sep 14, 2017 at 03:54:22PM -0700, Luis R. Rodriguez wrote:
> +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,
> +};

checkpatch.pl didn't complain about a lack of using BIT()?


> +
> +/**
> + * 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;

Agreed that this should not be "priv_reqs" but some other better name.

> + void *alloc_buf;
> + size_t alloc_buf_size;
> +};
> +
> +#define fw_req_param_sync(priv_params) \
> + (priv_params->mode == FW_API_SYNC)
> +#define fw_req_param_async(priv_params) \
> + (priv_params->mode == FW_API_ASYNC)
> +
> +#define fw_param_use_fallback(params) \
> + (!!((params)->priv_reqs & FW_PRIV_REQ_FALLBACK))
> +#define fw_param_uevent(params) \
> + (!!((params)->priv_reqs & FW_PRIV_REQ_FALLBACK_UEVENT))
> +#define fw_param_nocache(params) \
> + (!!((params)->priv_reqs & FW_PRIV_REQ_NO_CACHE))
> +#define fw_param_optional(params) \
> + (!!((params)->priv_reqs & FW_PRIV_REQ_OPTIONAL))

static inline functions to get proper typechecking?

> static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
> - void *buf, size_t size)
> + struct fw_priv_params *fw_priv_params)

Shouldn't the priv pointer hang off of 'struct firmware' in an opaque
type that can not be seen/accessed outside of this file?

That way you don't have to change the functions by adding new
parameters, what you did seems a lot more complex.

thanks,

greg k-h