| Message ID | 20191203161908.7496-1-perex@perex.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: snd-sof-pci - add model parameter and pass it to HDA codec driver | expand |
On 12/3/19 10:19 AM, Jaroslav Kysela wrote: > It may be useful to pass the specific model to the generic HDA codec > routines like the legacy HDA driver (snd-hda-intel) allows. > The model name "sofbus" is tricky anyway. Humm, I must admit I have never looked at this for the legacy driver, and I am a bit confused on what this would be used for? The legacy driver uses codec->modelname but I can't figure out this part in hda_codec.c if (codec->bus->modelname) { codec->modelname = kstrdup(codec->bus->modelname, GFP_KERNEL); In theory there can be multiple codecs per bus (with different SDIs) so using the bus->modelname for the codec->modelname looks odd. Is there an example of this being used for my education? Also it'd make more sense to me to have this parameter in an intel-specific module, not the top level PCI one? > > Signed-off-by: Jaroslav Kysela <perex@perex.cz> > Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Cc: Mark Brown <broonie@kernel.org> > --- > include/sound/sof.h | 1 + > sound/soc/sof/intel/hda.c | 2 +- > sound/soc/sof/sof-pci-dev.c | 5 +++++ > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/include/sound/sof.h b/include/sound/sof.h > index 479101736ee0..a62686baa95d 100644 > --- a/include/sound/sof.h > +++ b/include/sound/sof.h > @@ -25,6 +25,7 @@ struct snd_sof_pdata { > const char *drv_name; > const char *name; > const char *platform; > + const char *modelname; > > struct device *dev; > > diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c > index 91bd88fddac7..ccb640bacc99 100644 > --- a/sound/soc/sof/intel/hda.c > +++ b/sound/soc/sof/intel/hda.c > @@ -271,7 +271,7 @@ static int hda_init(struct snd_sof_dev *sdev) > mutex_init(&hbus->prepare_mutex); > hbus->pci = pci; > hbus->mixer_assigned = -1; > - hbus->modelname = "sofbus"; > + hbus->modelname = sdev->pdata->modelname; > > /* initialise hdac bus */ > bus->addr = pci_resource_start(pci, 0); > diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c > index bbeffd932de7..c01ad85aad2a 100644 > --- a/sound/soc/sof/sof-pci-dev.c > +++ b/sound/soc/sof/sof-pci-dev.c > @@ -34,6 +34,10 @@ static int sof_pci_debug; > module_param_named(sof_pci_debug, sof_pci_debug, int, 0444); > MODULE_PARM_DESC(sof_pci_debug, "SOF PCI debug options (0x0 all off)"); > > +static char *model; > +module_param(model, charp, 0444); > +MODULE_PARM_DESC(model, "Use the given HDA board model."); > + > #define SOF_PCI_DISABLE_PM_RUNTIME BIT(0) > > #if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE) > @@ -326,6 +330,7 @@ static int sof_pci_probe(struct pci_dev *pci, > sof_pdata->desc = (struct sof_dev_desc *)pci_id->driver_data; > sof_pdata->dev = dev; > sof_pdata->platform = dev_name(dev); > + sof_pdata->modelname = model; > > /* alternate fw and tplg filenames ? */ > if (fw_path) >
On Tue, 03 Dec 2019 19:57:30 +0100, Pierre-Louis Bossart wrote: > > > > On 12/3/19 10:19 AM, Jaroslav Kysela wrote: > > It may be useful to pass the specific model to the generic HDA codec > > routines like the legacy HDA driver (snd-hda-intel) allows. > > The model name "sofbus" is tricky anyway. > > Humm, I must admit I have never looked at this for the legacy driver, > and I am a bit confused on what this would be used for? > The legacy driver uses codec->modelname but I can't figure out this > part in hda_codec.c > > if (codec->bus->modelname) { > codec->modelname = kstrdup(codec->bus->modelname, GFP_KERNEL); > > In theory there can be multiple codecs per bus (with different SDIs) > so using the bus->modelname for the codec->modelname looks odd. That's true. However, basically the model name is specific to the whole device, hence it's usually OK to pass to all codecs. The mismatched model name is just ignored (that's why the current code with model="sofbus" works). So you can think it a kind of quirk lookup with a system name given explicitly by user. > Is there an example of this being used for my education? You can find the list in Documentation/sound/hd-audio/models.rst and the usage in Documentation/sound/hd-audio/notes.rst. The documents are a bit outdated, though. Takashi > Also it'd make more sense to me to have this parameter in an > intel-specific module, not the top level PCI one? > > > > > Signed-off-by: Jaroslav Kysela <perex@perex.cz> > > Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > Cc: Mark Brown <broonie@kernel.org> > > --- > > include/sound/sof.h | 1 + > > sound/soc/sof/intel/hda.c | 2 +- > > sound/soc/sof/sof-pci-dev.c | 5 +++++ > > 3 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/include/sound/sof.h b/include/sound/sof.h > > index 479101736ee0..a62686baa95d 100644 > > --- a/include/sound/sof.h > > +++ b/include/sound/sof.h > > @@ -25,6 +25,7 @@ struct snd_sof_pdata { > > const char *drv_name; > > const char *name; > > const char *platform; > > + const char *modelname; > > struct device *dev; > > diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c > > index 91bd88fddac7..ccb640bacc99 100644 > > --- a/sound/soc/sof/intel/hda.c > > +++ b/sound/soc/sof/intel/hda.c > > @@ -271,7 +271,7 @@ static int hda_init(struct snd_sof_dev *sdev) > > mutex_init(&hbus->prepare_mutex); > > hbus->pci = pci; > > hbus->mixer_assigned = -1; > > - hbus->modelname = "sofbus"; > > + hbus->modelname = sdev->pdata->modelname; > > /* initialise hdac bus */ > > bus->addr = pci_resource_start(pci, 0); > > diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c > > index bbeffd932de7..c01ad85aad2a 100644 > > --- a/sound/soc/sof/sof-pci-dev.c > > +++ b/sound/soc/sof/sof-pci-dev.c > > @@ -34,6 +34,10 @@ static int sof_pci_debug; > > module_param_named(sof_pci_debug, sof_pci_debug, int, 0444); > > MODULE_PARM_DESC(sof_pci_debug, "SOF PCI debug options (0x0 all off)"); > > +static char *model; > > +module_param(model, charp, 0444); > > +MODULE_PARM_DESC(model, "Use the given HDA board model."); > > + > > #define SOF_PCI_DISABLE_PM_RUNTIME BIT(0) > > #if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE) > > @@ -326,6 +330,7 @@ static int sof_pci_probe(struct pci_dev *pci, > > sof_pdata->desc = (struct sof_dev_desc *)pci_id->driver_data; > > sof_pdata->dev = dev; > > sof_pdata->platform = dev_name(dev); > > + sof_pdata->modelname = model; > > /* alternate fw and tplg filenames ? */ > > if (fw_path) > > >
>>> It may be useful to pass the specific model to the generic HDA codec >>> routines like the legacy HDA driver (snd-hda-intel) allows. >>> The model name "sofbus" is tricky anyway. >> >> Humm, I must admit I have never looked at this for the legacy driver, >> and I am a bit confused on what this would be used for? >> The legacy driver uses codec->modelname but I can't figure out this >> part in hda_codec.c >> >> if (codec->bus->modelname) { >> codec->modelname = kstrdup(codec->bus->modelname, GFP_KERNEL); >> >> In theory there can be multiple codecs per bus (with different SDIs) >> so using the bus->modelname for the codec->modelname looks odd. > > That's true. However, basically the model name is specific to the > whole device, hence it's usually OK to pass to all codecs. The > mismatched model name is just ignored (that's why the current code > with model="sofbus" works). So you can think it a kind of quirk > lookup with a system name given explicitly by user. > >> Is there an example of this being used for my education? > > You can find the list in Documentation/sound/hd-audio/models.rst > and the usage in Documentation/sound/hd-audio/notes.rst. The > documents are a bit outdated, though. ah, ok, thanks for the pointers. So if you have a new machine that's not explicitly handled by quirks you can initially force existing tricks to be used, and in a second step the quirk is extended to handle that machine, yes?
On Tue, 03 Dec 2019 20:47:32 +0100, Pierre-Louis Bossart wrote: > > > >>> It may be useful to pass the specific model to the generic HDA codec > >>> routines like the legacy HDA driver (snd-hda-intel) allows. > >>> The model name "sofbus" is tricky anyway. > >> > >> Humm, I must admit I have never looked at this for the legacy driver, > >> and I am a bit confused on what this would be used for? > >> The legacy driver uses codec->modelname but I can't figure out this > >> part in hda_codec.c > >> > >> if (codec->bus->modelname) { > >> codec->modelname = kstrdup(codec->bus->modelname, GFP_KERNEL); > >> > >> In theory there can be multiple codecs per bus (with different SDIs) > >> so using the bus->modelname for the codec->modelname looks odd. > > > > That's true. However, basically the model name is specific to the > > whole device, hence it's usually OK to pass to all codecs. The > > mismatched model name is just ignored (that's why the current code > > with model="sofbus" works). So you can think it a kind of quirk > > lookup with a system name given explicitly by user. > > > >> Is there an example of this being used for my education? > > > > You can find the list in Documentation/sound/hd-audio/models.rst > > and the usage in Documentation/sound/hd-audio/notes.rst. The > > documents are a bit outdated, though. > > ah, ok, thanks for the pointers. > > So if you have a new machine that's not explicitly handled by quirks > you can initially force existing tricks to be used, and in a second > step the quirk is extended to handle that machine, yes? Yes, that's the usual step. Takashi
diff --git a/include/sound/sof.h b/include/sound/sof.h index 479101736ee0..a62686baa95d 100644 --- a/include/sound/sof.h +++ b/include/sound/sof.h @@ -25,6 +25,7 @@ struct snd_sof_pdata { const char *drv_name; const char *name; const char *platform; + const char *modelname; struct device *dev; diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 91bd88fddac7..ccb640bacc99 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -271,7 +271,7 @@ static int hda_init(struct snd_sof_dev *sdev) mutex_init(&hbus->prepare_mutex); hbus->pci = pci; hbus->mixer_assigned = -1; - hbus->modelname = "sofbus"; + hbus->modelname = sdev->pdata->modelname; /* initialise hdac bus */ bus->addr = pci_resource_start(pci, 0); diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c index bbeffd932de7..c01ad85aad2a 100644 --- a/sound/soc/sof/sof-pci-dev.c +++ b/sound/soc/sof/sof-pci-dev.c @@ -34,6 +34,10 @@ static int sof_pci_debug; module_param_named(sof_pci_debug, sof_pci_debug, int, 0444); MODULE_PARM_DESC(sof_pci_debug, "SOF PCI debug options (0x0 all off)"); +static char *model; +module_param(model, charp, 0444); +MODULE_PARM_DESC(model, "Use the given HDA board model."); + #define SOF_PCI_DISABLE_PM_RUNTIME BIT(0) #if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE) @@ -326,6 +330,7 @@ static int sof_pci_probe(struct pci_dev *pci, sof_pdata->desc = (struct sof_dev_desc *)pci_id->driver_data; sof_pdata->dev = dev; sof_pdata->platform = dev_name(dev); + sof_pdata->modelname = model; /* alternate fw and tplg filenames ? */ if (fw_path)
It may be useful to pass the specific model to the generic HDA codec routines like the legacy HDA driver (snd-hda-intel) allows. The model name "sofbus" is tricky anyway. Signed-off-by: Jaroslav Kysela <perex@perex.cz> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Cc: Mark Brown <broonie@kernel.org> --- include/sound/sof.h | 1 + sound/soc/sof/intel/hda.c | 2 +- sound/soc/sof/sof-pci-dev.c | 5 +++++ 3 files changed, 7 insertions(+), 1 deletion(-)