On Tue, 31 Oct 2023, Bjorn Helgaas wrote: > On Fri, Oct 27, 2023 at 11:38:11AM +0300, Ilpo Järvinen wrote: > > Instead of open-coded masking and shifting with PCI_CONF1_* bitfields, > > use GENMASK() and FIELD_PREP(), and then remove the *_SHIFT defines > > that are no longer needed. > > @@ -797,19 +799,15 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev) > > * Section 3.2.2.3.2, Figure 3-2, p. 50. > > */ > > > > -#define PCI_CONF1_BUS_SHIFT 16 /* Bus number */ > > -#define PCI_CONF1_DEV_SHIFT 11 /* Device number */ > > -#define PCI_CONF1_FUNC_SHIFT 8 /* Function number */ > > - > > -#define PCI_CONF1_BUS_MASK 0xff > > -#define PCI_CONF1_DEV_MASK 0x1f > > -#define PCI_CONF1_FUNC_MASK 0x7 > > +#define PCI_CONF1_BUS_MASK GENMASK(23, 16) > > +#define PCI_CONF1_DEV_MASK GENMASK(15, 11) > > +#define PCI_CONF1_FUNC_MASK GENMASK(10, 8) > > #define PCI_CONF1_REG_MASK 0xfc /* Limit aligned offset to a maximum of 256B */ > > > > #define PCI_CONF1_ENABLE BIT(31) > > -#define PCI_CONF1_BUS(x) (((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT) > > -#define PCI_CONF1_DEV(x) (((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT) > > -#define PCI_CONF1_FUNC(x) (((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT) > > +#define PCI_CONF1_BUS(x) FIELD_PREP(PCI_CONF1_BUS_MASK, (x)) > > +#define PCI_CONF1_DEV(x) FIELD_PREP(PCI_CONF1_DEV_MASK, (x)) > > +#define PCI_CONF1_FUNC(x) FIELD_PREP(PCI_CONF1_FUNC_MASK, (x)) > > #define PCI_CONF1_REG(x) ((x) & PCI_CONF1_REG_MASK) > > I love getting rid of the _SHIFT #defines. > > I'm a dinosaur and haven't been completely converted to the wonders of > GENMASK yet. Okay but would it convince even "a dinosaur" like you :-) if you imagine a Bit Location column in some spec which says: 23:16 GENMASK just happens to mystically repeat those two numbers: GENMASK(23, 16) Pretty magic, isn't it? > PCI_CONF1_ADDRESS is the only user of PCI_CONF1_BUS etc, > so I think this would be simpler overall: > > #define PCI_CONF1_BUS 0x00ff0000 > #define PCI_CONF1_DEV 0x0000f800 > #define PCI_CONF1_FUNC 0x00000700 > #define PCI_CONF1_REG 0x000000ff > > #define PCI_CONF1_ADDRESS(bus, dev, func, reg) \ > (FIELD_PREP(PCI_CONF1_BUS, (bus)) | \ > FIELD_PREP(PCI_CONF1_DEV, (dev)) | \ > FIELD_PREP(PCI_CONF1_FUNC, (func)) | \ > FIELD_PREP(PCI_CONF1_REG, (reg & ~0x3))) Yes, it makes sense to remove the extra layer. One additional thing, I just noticed lots of arch/ code is duplicating this calculation so perhaps this should also be moved outside of drivers/pci/ into include/linux/pci.h ? (Not in the same patch.) > The v6.7 merge window just opened, and I won't start merging v6.8 > material until v6.7-rc1 (probably Nov 12), so no hurry on this. Yes I knew I was sending it quite late because I tried to meet your request in getting it all done in the same merge window (which I obviously failed but it isn't the end of the world). -- i.
