ASoC: snd-sof-pci - add model parameter and pass it to HDA codec driver
diff mbox series

Message ID 20191203161908.7496-1-perex@perex.cz
State New
Headers show
Series
  • ASoC: snd-sof-pci - add model parameter and pass it to HDA codec driver
Related show

Commit Message

Jaroslav Kysela Dec. 3, 2019, 4:19 p.m. UTC
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(-)

Comments

Pierre-Louis Bossart Dec. 3, 2019, 6:57 p.m. UTC | #1
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)
>
Takashi Iwai Dec. 3, 2019, 7:08 p.m. UTC | #2
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)
> >
>
Pierre-Louis Bossart Dec. 3, 2019, 7:47 p.m. UTC | #3
>>> 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?
Takashi Iwai Dec. 3, 2019, 9:45 p.m. UTC | #4
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

Patch
diff mbox series

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)