| Message ID | 20181120213644.19103-2-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC:Intel:Skylake: Enable HDaudio legacy fallback | expand |
On Tue, Nov 20, 2018 at 03:36:39PM -0600, Pierre-Louis Bossart wrote: > From: Takashi Iwai <tiwai@suse.de> > > It's with CNP, supposed to be equivalent with CNL entry. > May you consider to switch to PCI_DEVICE_DATA() first? > Signed-off-by: Takashi Iwai <tiwai@suse.de> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > sound/soc/intel/skylake/skl-messages.c | 8 ++++++++ > sound/soc/intel/skylake/skl.c | 3 +++ > 2 files changed, 11 insertions(+) > > diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c > index 8bfb8b0fa3d5..b0e6fb93eaf8 100644 > --- a/sound/soc/intel/skylake/skl-messages.c > +++ b/sound/soc/intel/skylake/skl-messages.c > @@ -247,6 +247,14 @@ static const struct skl_dsp_ops dsp_ops[] = { > .init_fw = cnl_sst_init_fw, > .cleanup = cnl_sst_dsp_cleanup > }, > + { > + .id = 0xa348, > + .num_cores = 4, > + .loader_ops = bxt_get_loader_ops, > + .init = cnl_sst_dsp_init, > + .init_fw = cnl_sst_init_fw, > + .cleanup = cnl_sst_dsp_cleanup > + }, > }; > > const struct skl_dsp_ops *skl_get_dsp_ops(int pci_id) > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c > index 3f0ac1312982..df36b8fe6d5e 100644 > --- a/sound/soc/intel/skylake/skl.c > +++ b/sound/soc/intel/skylake/skl.c > @@ -1121,6 +1121,9 @@ static const struct pci_device_id skl_ids[] = { > /* CNL */ > { PCI_DEVICE(0x8086, 0x9dc8), > .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, > + /* CFL */ > + { PCI_DEVICE(0x8086, 0xa348), > + .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, > { 0, } > }; > MODULE_DEVICE_TABLE(pci, skl_ids); > -- > 2.17.1 >
On 11/21/18 8:27 AM, Andy Shevchenko wrote: > On Tue, Nov 20, 2018 at 03:36:39PM -0600, Pierre-Louis Bossart wrote: >> From: Takashi Iwai <tiwai@suse.de> >> >> It's with CNP, supposed to be equivalent with CNL entry. >> > May you consider to switch to PCI_DEVICE_DATA() first? Is this really the recommended path? The macro generates PCI_DEVICE_ID_##vend##_##dev, and I don't have a turn key #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 I can use. In a number of cases we have multiple variants of the same hardware, and it starts being painful to use a 20-letter macro to differentiate between INTEL_AUDIO_CFL_Y and INTEL_AUDIO_CFL_H. The explicit code and a short comment are more readable really. git grep PCI_DEVICE_ID_INTEL gives me hundreds of definitions, some global, some local to specific drivers, doesn't seem like there is a well-agreed usage of this macro, is there? I don't mind making the change but I don't sense an strong argument for it? > >> Signed-off-by: Takashi Iwai <tiwai@suse.de> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> sound/soc/intel/skylake/skl-messages.c | 8 ++++++++ >> sound/soc/intel/skylake/skl.c | 3 +++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c >> index 8bfb8b0fa3d5..b0e6fb93eaf8 100644 >> --- a/sound/soc/intel/skylake/skl-messages.c >> +++ b/sound/soc/intel/skylake/skl-messages.c >> @@ -247,6 +247,14 @@ static const struct skl_dsp_ops dsp_ops[] = { >> .init_fw = cnl_sst_init_fw, >> .cleanup = cnl_sst_dsp_cleanup >> }, >> + { >> + .id = 0xa348, >> + .num_cores = 4, >> + .loader_ops = bxt_get_loader_ops, >> + .init = cnl_sst_dsp_init, >> + .init_fw = cnl_sst_init_fw, >> + .cleanup = cnl_sst_dsp_cleanup >> + }, >> }; >> >> const struct skl_dsp_ops *skl_get_dsp_ops(int pci_id) >> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c >> index 3f0ac1312982..df36b8fe6d5e 100644 >> --- a/sound/soc/intel/skylake/skl.c >> +++ b/sound/soc/intel/skylake/skl.c >> @@ -1121,6 +1121,9 @@ static const struct pci_device_id skl_ids[] = { >> /* CNL */ >> { PCI_DEVICE(0x8086, 0x9dc8), >> .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, >> + /* CFL */ >> + { PCI_DEVICE(0x8086, 0xa348), >> + .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, >> { 0, } >> }; >> MODULE_DEVICE_TABLE(pci, skl_ids); >> -- >> 2.17.1 >>
On Wed, Nov 21, 2018 at 11:16:50AM -0600, Pierre-Louis Bossart wrote: > On 11/21/18 8:27 AM, Andy Shevchenko wrote: > > May you consider to switch to PCI_DEVICE_DATA() first? > > Is this really the recommended path? > > The macro generates PCI_DEVICE_ID_##vend##_##dev, and I don't have a turn > key #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 I can use. In a number of > cases we have multiple variants of the same hardware, and it starts being > painful to use a 20-letter macro to differentiate between INTEL_AUDIO_CFL_Y > and INTEL_AUDIO_CFL_H. The explicit code and a short comment are more > readable really. > > git grep PCI_DEVICE_ID_INTEL gives me hundreds of definitions, some global, > some local to specific drivers, doesn't seem like there is a well-agreed > usage of this macro, is there? I don't mind making the change but I don't > sense an strong argument for it? Compare: /* CFL */ { PCI_DEVICE(0x8086, 0xa348), .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, to something like: #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 ... {PCI_DEVICE_DATA(INTEL, AUDIO_CFL, &snd_soc_acpi_intel_cnl_machines)}, Macro is recently introduced, that's why not many users of it. At least I'm planning to clean up dwc3-pci.c using it.
On Wed, 21 Nov 2018 18:38:41 +0100, Andy Shevchenko wrote: > > On Wed, Nov 21, 2018 at 11:16:50AM -0600, Pierre-Louis Bossart wrote: > > On 11/21/18 8:27 AM, Andy Shevchenko wrote: > > > > May you consider to switch to PCI_DEVICE_DATA() first? > > > > Is this really the recommended path? > > > > The macro generates PCI_DEVICE_ID_##vend##_##dev, and I don't have a turn > > key #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 I can use. In a number of > > cases we have multiple variants of the same hardware, and it starts being > > painful to use a 20-letter macro to differentiate between INTEL_AUDIO_CFL_Y > > and INTEL_AUDIO_CFL_H. The explicit code and a short comment are more > > readable really. > > > > git grep PCI_DEVICE_ID_INTEL gives me hundreds of definitions, some global, > > some local to specific drivers, doesn't seem like there is a well-agreed > > usage of this macro, is there? I don't mind making the change but I don't > > sense an strong argument for it? > > Compare: > > /* CFL */ > { PCI_DEVICE(0x8086, 0xa348), > .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, > > to something like: > > #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 > ... > > {PCI_DEVICE_DATA(INTEL, AUDIO_CFL, &snd_soc_acpi_intel_cnl_machines)}, The former gives a better grep-ability, though. I have no big preference over two, just want to mention that both have merits and demerits. thanks, Takashi
On Wed, Nov 21, 2018 at 11:17:41PM +0100, Takashi Iwai wrote: > On Wed, 21 Nov 2018 18:38:41 +0100, Andy Shevchenko wrote: > > Compare: > > > > /* CFL */ > > { PCI_DEVICE(0x8086, 0xa348), > > .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, > > > > to something like: > > > > #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 > > ... > > > > {PCI_DEVICE_DATA(INTEL, AUDIO_CFL, &snd_soc_acpi_intel_cnl_machines)}, > > The former gives a better grep-ability, though. > > I have no big preference over two, just want to mention that both have > merits and demerits. True. So, up to you, guys, what to choose.
On 11/22/18 3:56 AM, Andy Shevchenko wrote: > On Wed, Nov 21, 2018 at 11:17:41PM +0100, Takashi Iwai wrote: >> On Wed, 21 Nov 2018 18:38:41 +0100, Andy Shevchenko wrote: >>> Compare: >>> >>> /* CFL */ >>> { PCI_DEVICE(0x8086, 0xa348), >>> .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, >>> >>> to something like: >>> >>> #define PCI_DEVICE_ID_INTEL_AUDIO_CFL 0xa348 >>> ... >>> >>> {PCI_DEVICE_DATA(INTEL, AUDIO_CFL, &snd_soc_acpi_intel_cnl_machines)}, >> The former gives a better grep-ability, though. >> >> I have no big preference over two, just want to mention that both have >> merits and demerits. > True. > So, up to you, guys, what to choose. I am leaning to leave the PCI stuff alone for now. If we change the PCI definitions I'd like to do it on the SKL and legacy sides at the same time, to avoid multiple definitions or redundancies. However I like your suggestion for the macros on the other patch so will change the code. Thanks for the reviews! >
diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c index 8bfb8b0fa3d5..b0e6fb93eaf8 100644 --- a/sound/soc/intel/skylake/skl-messages.c +++ b/sound/soc/intel/skylake/skl-messages.c @@ -247,6 +247,14 @@ static const struct skl_dsp_ops dsp_ops[] = { .init_fw = cnl_sst_init_fw, .cleanup = cnl_sst_dsp_cleanup }, + { + .id = 0xa348, + .num_cores = 4, + .loader_ops = bxt_get_loader_ops, + .init = cnl_sst_dsp_init, + .init_fw = cnl_sst_init_fw, + .cleanup = cnl_sst_dsp_cleanup + }, }; const struct skl_dsp_ops *skl_get_dsp_ops(int pci_id) diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 3f0ac1312982..df36b8fe6d5e 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -1121,6 +1121,9 @@ static const struct pci_device_id skl_ids[] = { /* CNL */ { PCI_DEVICE(0x8086, 0x9dc8), .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, + /* CFL */ + { PCI_DEVICE(0x8086, 0xa348), + .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines}, { 0, } }; MODULE_DEVICE_TABLE(pci, skl_ids);