| Date | Tue, 2 Feb 2021 18:04:41 +0000 | From | Christoph Hellwig <> | Subject | Re: [PATCH 02/14] cxl/mem: Map memory device registers |
| |
Any reason not to merge a bunch of patches? Both this one and the previous one are rather useless on their own, making review harder than necessary.
> + * cxl_mem_create() - Create a new &struct cxl_mem. > + * @pdev: The pci device associated with the new &struct cxl_mem. > + * @reg_lo: Lower 32b of the register locator > + * @reg_hi: Upper 32b of the register locator. > + * > + * Return: The new &struct cxl_mem on success, NULL on failure. > + * > + * Map the BAR for a CXL memory device. This BAR has the memory device's > + * registers for the device as specified in CXL specification. > + */
A lot of text with almost no value over just reading the function. What's that fetish with kerneldoc comments for trivial static functions?
> + reg_type = > + (reg_lo >> CXL_REGLOC_RBI_SHIFT) & CXL_REGLOC_RBI_MASK;
OTOH this screams for a helper that would make the code a lot more self documenting.
> + if (reg_type == CXL_REGLOC_RBI_MEMDEV) { > + rc = 0; > + cxlm = cxl_mem_create(pdev, reg_lo, reg_hi); > + if (!cxlm) > + rc = -ENODEV; > + break;
And given that we're going to grow more types eventually, why not start out with a switch here? Also why return the structure when nothing uses it?
|