| Message ID | 1430405556-19166-3-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 30, 2015 at 08:22:36PM +0530, Vinod Koul wrote: > - /* Sunrise Point-LP */ > - { PCI_DEVICE(0x8086, 0x9d70), > - .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE }, > +static const struct pci_device_id azx_intel_adsp_ids[] = { > + /* Sunrise Point-LP */ > + { PCI_DEVICE(0x8086, 0x9d70), > + .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE }, > + { 0, } > +}; > +MODULE_DEVICE_TABLE(pci, azx_intel_adsp_ids); > + > static int __init azx_module_init(void) > { > int ret; > > ret = pci_register_driver(&azx_driver); > > + if (!hdac_adsp_enable) > + ret = pci_register_driver(&azx_intel_adsp_driver); This feels like the wrong idiom here. I'd expect us to do this by binding to the device but ignoring the DSP functionality, or if the device has only DSP functionality (it's not clear with the context I have but I think that's the case) just printing a message and not telling the sound core about it. Having the driver just fail to load seems like it might be a bit obscure. Though to be honest if it *is* going to register as a separate HDA device then it seems like userspace quirking for this is just as viable.
At Thu, 30 Apr 2015 21:33:55 +0100, Mark Brown wrote: > > On Thu, Apr 30, 2015 at 08:22:36PM +0530, Vinod Koul wrote: > > > - /* Sunrise Point-LP */ > > - { PCI_DEVICE(0x8086, 0x9d70), > > - .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE }, > > > +static const struct pci_device_id azx_intel_adsp_ids[] = { > > + /* Sunrise Point-LP */ > > + { PCI_DEVICE(0x8086, 0x9d70), > > + .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE }, > > + { 0, } > > +}; > > +MODULE_DEVICE_TABLE(pci, azx_intel_adsp_ids); > > + > > > static int __init azx_module_init(void) > > { > > int ret; > > > > ret = pci_register_driver(&azx_driver); > > > > + if (!hdac_adsp_enable) > > + ret = pci_register_driver(&azx_intel_adsp_driver); > > This feels like the wrong idiom here. I'd expect us to do this by > binding to the device but ignoring the DSP functionality, or if the > device has only DSP functionality (it's not clear with the context I > have but I think that's the case) just printing a message and not > telling the sound core about it. Having the driver just fail to load > seems like it might be a bit obscure. > > Though to be honest if it *is* going to register as a separate HDA > device then it seems like userspace quirking for this is just as viable. Ideally, it'd be best if the system can do everything automatically, e.g. loads the DSP driver at first, then falls back to the legacy driver if no DSP is found. But there seems no clean way to do this, so far. Thus the flag was introduced. BTW, the patch won't work as expected. The device list is created at the module build time (modpost), thus it must expose all devices, including SKL. The patch essentially excludes SKL PCI ID from the list. Another way to skip the unwanted driver is just to return -ENODEV from its probe callback when the given PCI is SKL and the flag is set. Then the driver core ignores this and continues to the next possible driver (i.e. the dsp one). The same should be applied to hda-soc-skl driver in vice versa. Takashi
On Thu, Apr 30, 2015 at 10:59:22PM +0200, Takashi Iwai wrote: > Ideally, it'd be best if the system can do everything automatically, > e.g. loads the DSP driver at first, then falls back to the legacy > driver if no DSP is found. But there seems no clean way to do this, > so far. Thus the flag was introduced. Eeew, that's horrible. I guess another DMI quirk table is going to be needed... > Another way to skip the unwanted driver is just to return -ENODEV from > its probe callback when the given PCI is SKL and the flag is set. > Then the driver core ignores this and continues to the next possible > driver (i.e. the dsp one). The same should be applied to hda-soc-skl > driver in vice versa. Right, that was one of my suggestions.
On Thu, Apr 30, 2015 at 10:23:30PM +0100, Mark Brown wrote: > On Thu, Apr 30, 2015 at 10:59:22PM +0200, Takashi Iwai wrote: > > > Ideally, it'd be best if the system can do everything automatically, > > e.g. loads the DSP driver at first, then falls back to the legacy > > driver if no DSP is found. But there seems no clean way to do this, > > so far. Thus the flag was introduced. > > Eeew, that's horrible. I guess another DMI quirk table is going to be > needed... > > > Another way to skip the unwanted driver is just to return -ENODEV from > > its probe callback when the given PCI is SKL and the flag is set. > > Then the driver core ignores this and continues to the next possible > > driver (i.e. the dsp one). The same should be applied to hda-soc-skl > > driver in vice versa. > > Right, that was one of my suggestions. okay that sounds okay to me. So the device probe checks the value of this flag and then either goes ahead with probe or returns -ENODEV Thanks
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 488dab208ddc..314a7beb2b29 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2023,9 +2023,6 @@ static const struct pci_device_id azx_ids[] = { /* Sunrise Point */ { PCI_DEVICE(0x8086, 0xa170), .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE }, - /* Sunrise Point-LP */ - { PCI_DEVICE(0x8086, 0x9d70), - .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE }, /* Haswell */ { PCI_DEVICE(0x8086, 0x0a0c), .driver_data = AZX_DRIVER_HDMI | AZX_DCAPS_INTEL_HASWELL }, @@ -2205,6 +2202,14 @@ static const struct pci_device_id azx_ids[] = { }; MODULE_DEVICE_TABLE(pci, azx_ids); +static const struct pci_device_id azx_intel_adsp_ids[] = { + /* Sunrise Point-LP */ + { PCI_DEVICE(0x8086, 0x9d70), + .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE }, + { 0, } +}; +MODULE_DEVICE_TABLE(pci, azx_intel_adsp_ids); + /* pci_driver definition */ static struct pci_driver azx_driver = { .name = KBUILD_MODNAME, @@ -2217,12 +2222,25 @@ static struct pci_driver azx_driver = { }, }; +static struct pci_driver azx_intel_adsp_driver = { + .name = KBUILD_MODNAME, + .id_table = azx_intel_adsp_ids, + .probe = azx_probe, + .remove = azx_remove, + .shutdown = azx_shutdown, + .driver = { + .pm = AZX_PM_OPS, + }, +}; + static int __init azx_module_init(void) { int ret; ret = pci_register_driver(&azx_driver); + if (!hdac_adsp_enable) + ret = pci_register_driver(&azx_intel_adsp_driver); return ret; } module_init(azx_module_init); @@ -2231,5 +2249,12 @@ static void __exit azx_module_exit(void) { pci_unregister_driver(&azx_driver); + /* Some Intel HDA controllers support aDSP this is enabled thru the + * ASoC HDA aDSP driver So we register for these devices only when + * aDSP is disabled by the hdac_adsp_enable flag + */ + if (!hdac_adsp_enable) + pci_unregister_driver(&azx_intel_adsp_driver); + } module_exit(azx_module_exit);
Sunrisepoint-LP HDA controller supports aDSP, so register selectively for this controller Signed-off-by: Vinod Koul <vinod.koul@intel.com> --- sound/pci/hda/hda_intel.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-)