| Message ID | 20230711125726.3509391-14-amadeuszx.slawinski@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Define Intel PCI IDs and use them in drivers | expand |
On Tue, Jul 11, 2023 at 02:57:26PM +0200, Amadeusz Sławiński wrote: > Use PCI device IDs from pci_ids.h header. ... > switch (sst->dev_id) { > - case SST_MRFLD_PCI_ID: > + case PCI_DEVICE_ID_INTEL_ADSP_TNG: > case SST_BYT_ACPI_ID: > case SST_CHV_ACPI_ID: I think this needs a bit more, i.e. replacing the rest with respective PCI IDs. All three will be defined with SST prefix, which makes sense to me. ACPI here is a bit misleading, but correct. The ACPI specification assumes that respective part of the ID space covers 100% of PCI ID space. I have briefly checked the code and it seems that ID is used externally only for PCI case, so we may simply use the lower 16 bits of the ACPI _HID for the context. diff --git a/sound/soc/intel/atom/sst/sst.c b/sound/soc/intel/atom/sst/sst.c index a0d29510d2bc..b1f59604d825 100644 --- a/sound/soc/intel/atom/sst/sst.c +++ b/sound/soc/intel/atom/sst/sst.c @@ -222,7 +222,7 @@ static void sst_init_locks(struct intel_sst_drv *ctx) } int sst_alloc_drv_context(struct intel_sst_drv **ctx, - struct device *dev, unsigned int dev_id) + struct device *dev, unsigned short dev_id) { *ctx = devm_kzalloc(dev, sizeof(struct intel_sst_drv), GFP_KERNEL); if (!(*ctx)) diff --git a/sound/soc/intel/atom/sst/sst.h b/sound/soc/intel/atom/sst/sst.h index 4d37d39fd8f4..c04f033e1d5f 100644 --- a/sound/soc/intel/atom/sst/sst.h +++ b/sound/soc/intel/atom/sst/sst.h @@ -358,7 +358,7 @@ struct sst_fw_save { struct intel_sst_drv { int sst_state; int irq_num; - unsigned int dev_id; + unsigned short dev_id; void __iomem *ddr; void __iomem *shim; void __iomem *mailbox; @@ -523,7 +523,7 @@ int sst_register(struct device *); int sst_unregister(struct device *); int sst_alloc_drv_context(struct intel_sst_drv **ctx, - struct device *dev, unsigned int dev_id); + struct device *dev, unsigned short dev_id); int sst_context_init(struct intel_sst_drv *ctx); void sst_context_cleanup(struct intel_sst_drv *ctx); void sst_configure_runtime_pm(struct intel_sst_drv *ctx); ... > /* driver names */ > #define SST_DRV_NAME "intel_sst_driver" > -#define SST_MRFLD_PCI_ID 0x119A > #define SST_BYT_ACPI_ID 0x80860F28 > #define SST_CHV_ACPI_ID 0x808622A8 And kill all three!
On 7/11/2023 4:33 PM, Andy Shevchenko wrote: > On Tue, Jul 11, 2023 at 02:57:26PM +0200, Amadeusz Sławiński wrote: >> Use PCI device IDs from pci_ids.h header. > > ... > >> switch (sst->dev_id) { >> - case SST_MRFLD_PCI_ID: >> + case PCI_DEVICE_ID_INTEL_ADSP_TNG: >> case SST_BYT_ACPI_ID: >> case SST_CHV_ACPI_ID: > > I think this needs a bit more, i.e. replacing the rest with respective PCI IDs. > > All three will be defined with SST prefix, which makes sense to me. > > ACPI here is a bit misleading, but correct. The ACPI specification assumes that > respective part of the ID space covers 100% of PCI ID space. > > I have briefly checked the code and it seems that ID is used externally only > for PCI case, so we may simply use the lower 16 bits of the ACPI _HID for the > context. > Do I understand correctly that I should just do: #define PCI_DEVICE_ID_INTEL_SST_BYT 0x0F28 #define PCI_DEVICE_ID_INTEL_SST_CHV 0x22A8 and use those IDs instead? > diff --git a/sound/soc/intel/atom/sst/sst.c b/sound/soc/intel/atom/sst/sst.c > index a0d29510d2bc..b1f59604d825 100644 > --- a/sound/soc/intel/atom/sst/sst.c > +++ b/sound/soc/intel/atom/sst/sst.c > @@ -222,7 +222,7 @@ static void sst_init_locks(struct intel_sst_drv *ctx) > } > > int sst_alloc_drv_context(struct intel_sst_drv **ctx, > - struct device *dev, unsigned int dev_id) > + struct device *dev, unsigned short dev_id) > { > *ctx = devm_kzalloc(dev, sizeof(struct intel_sst_drv), GFP_KERNEL); > if (!(*ctx)) > diff --git a/sound/soc/intel/atom/sst/sst.h b/sound/soc/intel/atom/sst/sst.h > index 4d37d39fd8f4..c04f033e1d5f 100644 > --- a/sound/soc/intel/atom/sst/sst.h > +++ b/sound/soc/intel/atom/sst/sst.h > @@ -358,7 +358,7 @@ struct sst_fw_save { > struct intel_sst_drv { > int sst_state; > int irq_num; > - unsigned int dev_id; > + unsigned short dev_id; > void __iomem *ddr; > void __iomem *shim; > void __iomem *mailbox; > @@ -523,7 +523,7 @@ int sst_register(struct device *); > int sst_unregister(struct device *); > > int sst_alloc_drv_context(struct intel_sst_drv **ctx, > - struct device *dev, unsigned int dev_id); > + struct device *dev, unsigned short dev_id); > int sst_context_init(struct intel_sst_drv *ctx); > void sst_context_cleanup(struct intel_sst_drv *ctx); > void sst_configure_runtime_pm(struct intel_sst_drv *ctx); > > ... > >> /* driver names */ >> #define SST_DRV_NAME "intel_sst_driver" > >> -#define SST_MRFLD_PCI_ID 0x119A >> #define SST_BYT_ACPI_ID 0x80860F28 >> #define SST_CHV_ACPI_ID 0x808622A8 > > And kill all three! >
On Wed, Jul 12, 2023 at 02:19:47PM +0200, Amadeusz Sławiński wrote: > On 7/11/2023 4:33 PM, Andy Shevchenko wrote: > > On Tue, Jul 11, 2023 at 02:57:26PM +0200, Amadeusz Sławiński wrote: > > > Use PCI device IDs from pci_ids.h header. ... > > > switch (sst->dev_id) { > > > - case SST_MRFLD_PCI_ID: > > > + case PCI_DEVICE_ID_INTEL_ADSP_TNG: > > > case SST_BYT_ACPI_ID: > > > case SST_CHV_ACPI_ID: > > > > I think this needs a bit more, i.e. replacing the rest with respective PCI IDs. > > > > All three will be defined with SST prefix, which makes sense to me. > > > > ACPI here is a bit misleading, but correct. The ACPI specification assumes that > > respective part of the ID space covers 100% of PCI ID space. > > > > I have briefly checked the code and it seems that ID is used externally only > > for PCI case, so we may simply use the lower 16 bits of the ACPI _HID for the > > context. > > Do I understand correctly that I should just do: > #define PCI_DEVICE_ID_INTEL_SST_BYT 0x0F28 > #define PCI_DEVICE_ID_INTEL_SST_CHV 0x22A8 > and use those IDs instead? Correct!
diff --git a/sound/soc/intel/atom/sst/sst.c b/sound/soc/intel/atom/sst/sst.c index a0d29510d2bc..f582e0737778 100644 --- a/sound/soc/intel/atom/sst/sst.c +++ b/sound/soc/intel/atom/sst/sst.c @@ -16,6 +16,7 @@ #include <linux/interrupt.h> #include <linux/io.h> #include <linux/firmware.h> +#include <linux/pci.h> #include <linux/pm_runtime.h> #include <linux/pm_qos.h> #include <linux/async.h> @@ -174,7 +175,7 @@ int sst_driver_ops(struct intel_sst_drv *sst) { switch (sst->dev_id) { - case SST_MRFLD_PCI_ID: + case PCI_DEVICE_ID_INTEL_ADSP_TNG: case SST_BYT_ACPI_ID: case SST_CHV_ACPI_ID: sst->tstamp = SST_TIME_STAMP_MRFLD; diff --git a/sound/soc/intel/atom/sst/sst.h b/sound/soc/intel/atom/sst/sst.h index 4d37d39fd8f4..6670aaf9aca4 100644 --- a/sound/soc/intel/atom/sst/sst.h +++ b/sound/soc/intel/atom/sst/sst.h @@ -20,7 +20,6 @@ /* driver names */ #define SST_DRV_NAME "intel_sst_driver" -#define SST_MRFLD_PCI_ID 0x119A #define SST_BYT_ACPI_ID 0x80860F28 #define SST_CHV_ACPI_ID 0x808622A8 diff --git a/sound/soc/intel/atom/sst/sst_pci.c b/sound/soc/intel/atom/sst/sst_pci.c index 4058b4f80a0c..9098076d6c72 100644 --- a/sound/soc/intel/atom/sst/sst_pci.c +++ b/sound/soc/intel/atom/sst/sst_pci.c @@ -32,7 +32,7 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx) /* map registers */ /* DDR base */ - if (ctx->dev_id == SST_MRFLD_PCI_ID) { + if (ctx->dev_id == PCI_DEVICE_ID_INTEL_ADSP_TNG) { ctx->ddr_base = pci_resource_start(pci, 0); /* check that the relocated IMR base matches with FW Binary */ ddr_base = relocate_imr_addr_mrfld(ctx->ddr_base); @@ -173,7 +173,7 @@ static void intel_sst_remove(struct pci_dev *pci) /* PCI Routines */ static const struct pci_device_id intel_sst_ids[] = { - { PCI_VDEVICE(INTEL, SST_MRFLD_PCI_ID), 0}, + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADSP_TNG), 0}, { 0, } };
Use PCI device IDs from pci_ids.h header. Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/intel/atom/sst/sst.c | 3 ++- sound/soc/intel/atom/sst/sst.h | 1 - sound/soc/intel/atom/sst/sst_pci.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)