Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 26 Sep 2022 13:27:10 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Pali =?iso-8859-1?Q?Roh=E1r?= <pali@kernel.org>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Rob Herring <robh@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Sergio Paracuellos <sergio.paracuellos@gmail.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH 1/3] PCI: Add standard PCI Config Address macros
Message-ID: <20220926182710.GA1613197@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220924092404.31776-2-pali@kernel.org>
Precedence: bulk

On Sat, Sep 24, 2022 at 11:24:02AM +0200, Pali Roh?r wrote:
> Lot of PCI and PCIe controllers are using standard Config Address for PCI
> Configuration Mechanism #1 (as defined in PCI Local Bus Specification) or
> its extended version.
> 
> So introduce new macros PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() in
> include file drivers/pci/pci.h which can be suitable for PCI and PCIe
> controllers which uses this type of access to PCI config space.
> 
> Signed-off-by: Pali Roh?r <pali@kernel.org>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> +#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_REG_MASK	0xfc /* Limit aligned offset to a maximum of 256B */

Since all the above are used only in the macros below, I personally
don't think they're really necessary and I would find it easier to
read if they were just open-coded, e.g.,

  +#define PCI_CONF1_BUS(x)     (((x) & 0xff) << 16)
  +#define PCI_CONF1_DEV(x)	(((x) & 0x1f) << 11)
  +#define PCI_CONF1_FUNC(x)	(((x) & 0x07) <<  8)
  +#define PCI_CONF1_REG(x)     ( (x) & 0xfc)

> +#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_REG(x)	((x) & PCI_CONF1_REG_MASK)