Re: [PATCH 1/1] PCI: Use FIELD_PREP() and remove *_SHIFT defines

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2962 bytes --]

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.