On Fri, Jan 29, 2021 at 04:24:27PM -0800, Ben Widawsky wrote:
> #ifndef __CXL_H__
> #define __CXL_H__
>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/io.h>
> +
> +#define CXL_SET_FIELD(value, field) \
> + ({ \
> + WARN_ON(!FIELD_FIT(field##_MASK, value)); \
> + FIELD_PREP(field##_MASK, value); \
> + })
> +
> +#define CXL_GET_FIELD(word, field) FIELD_GET(field##_MASK, word)
This looks like some massive obsfucation. What is the intent
here?
> + /* Cap 0001h - CXL_CAP_CAP_ID_DEVICE_STATUS */
> + struct {
> + void __iomem *regs;
> + } status;
> +
> + /* Cap 0002h - CXL_CAP_CAP_ID_PRIMARY_MAILBOX */
> + struct {
> + void __iomem *regs;
> + size_t payload_size;
> + } mbox;
> +
> + /* Cap 4000h - CXL_CAP_CAP_ID_MEMDEV */
> + struct {
> + void __iomem *regs;
> + } mem;
This style looks massively obsfucated. For one the comments look like
absolute gibberish, but also what is the point of all these anonymous
structures?
> +#define cxl_reg(type) \
> + static inline void cxl_write_##type##_reg32(struct cxl_mem *cxlm, \
> + u32 reg, u32 value) \
> + { \
> + void __iomem *reg_addr = cxlm->type.regs; \
> + writel(value, reg_addr + reg); \
> + } \
> + static inline void cxl_write_##type##_reg64(struct cxl_mem *cxlm, \
> + u32 reg, u64 value) \
> + { \
> + void __iomem *reg_addr = cxlm->type.regs; \
> + writeq(value, reg_addr + reg); \
> + } \
What is the value add of all this obsfucation over the trivial
calls to the write*/read* functions, possible with a locally
declarate "void __iomem *" variable in the callers like in all
normall drivers? Except for making the life of the poor soul trying
to debug this code some time in the future really hard, of course.
> + /* 8.2.8.4.3 */
????
