Re: [PATCH 1/5] PCI: add ArrowLake-S PCI ID for Intel HDAudio subsystem.

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Aug 02, 2023 at 11:07:36AM -0500, Pierre-Louis Bossart wrote:
> On 8/2/23 10:57, Takashi Iwai wrote:
> > On Wed, 02 Aug 2023 17:52:26 +0200,
> > Bjorn Helgaas wrote:
> >> On Wed, Aug 02, 2023 at 10:01:01AM -0500, Pierre-Louis Bossart wrote:
> >>> Add part ID to common include file
> >>
> >> Please drop period at end of subject and add one at the end of the
> >> commit log.
> >>
> >> Also mention the drivers that will use this new #define; looks like
> >> hda_intel.c and ...
> >>
> >> Well, actually, I only see that one use, which means we probably
> >> shouldn't add this #define to pci_ids.h, per the comment at the top of
> >> the file.  If there's only one use, use the hex ID in the driver (or
> >> add a #define in the driver itself).
> > 
> > Judging from the previous patterns, the same ID could be required for
> > ASoC SOF driver, too, which isn't included in this patch set.  In
> > that case, it's worth to put to pci_ids.h.
> > (OTOH, it can be done at a later stage, too.)

When it becomes shared is the standard point at which we add to
pci_ids.h.

> I am not following. we just agreed a couple of weeks ago to record ALL
> Intel/HDaudio PCI IDs in the same pci_ids.h include file.

I'm not sure who "we" is here.  If it included me and I signed up to
it, I apologize for forgetting, and go ahead and add my:

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

I'm just pointing out the usual practice for pci_ids.h, as mentioned
in the file itself.

> ArrowLake-S is the first addition to first file after the work done by
> Cezary/Amadeusz. Yes it's required to be added since it'll be used in
> other parts later on. But even if there was ONE use of this PCI ID, why
> would we not add it for consistency to the global pci_ids.h file?
> Takashi's hda_intel.c file would look really bad if we have a mix of
> single-use PCIs and shared ones...

We already have a mix:

  static const struct pci_device_id azx_ids[] = {
        /* CPT */
        { PCI_DEVICE(0x8086, 0x1c20),
          .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
        /* PBG */
        { PCI_DEVICE(0x8086, 0x1d20),
          .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_PCH_NOPM },
        /* Panther Point */

I think the reason we don't add names used only once is because it
makes backporting things harder because it leads to more merge
conflicts in pci_ids.h.

Bjorn