| Message ID | 20180103170242.5363-4-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3c22a73fb87366851dcf48d852357a6d808921cc |
| Headers | show |
On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote: > Same problem as with previous machine drivers, the codec dai > uses a hard-coded name of "i2c-ESSX8316:00" but ACPI provides > "i2c-ESSX8316:01" in some systems. Yeah, that's why using device instances is fragile... > > Fix by overriding the hard-coded value with the codec name derived > from the HID information The patch makes me think about introducing acpi_dev_get_dev_name() and utilize it here since I need something similar to have in one of GPIO drivers. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189261 > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel. > com> > --- > sound/soc/intel/boards/Kconfig | 1 + > sound/soc/intel/boards/bytcht_es8316.c | 26 > +++++++++++++++++++++++++- > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/intel/boards/Kconfig > b/sound/soc/intel/boards/Kconfig > index 89a73e3d9d2d..4af2393160bf 100644 > --- a/sound/soc/intel/boards/Kconfig > +++ b/sound/soc/intel/boards/Kconfig > @@ -153,6 +153,7 @@ config SND_SOC_INTEL_BYT_CHT_DA7213_MACH > config SND_SOC_INTEL_BYT_CHT_ES8316_MACH > tristate "Baytrail & Cherrytrail with ES8316 codec" > depends on X86_INTEL_LPSS && I2C && ACPI > + select SND_SOC_ACPI > select SND_SOC_ES8316 > help > This adds support for ASoC machine driver for Intel(R) > Baytrail & > diff --git a/sound/soc/intel/boards/bytcht_es8316.c > b/sound/soc/intel/boards/bytcht_es8316.c > index 8088396717e3..ae24f6205f05 100644 > --- a/sound/soc/intel/boards/bytcht_es8316.c > +++ b/sound/soc/intel/boards/bytcht_es8316.c > @@ -232,15 +232,39 @@ static struct snd_soc_card byt_cht_es8316_card = > { > .fully_routed = true, > }; > > +static char codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */ > + I would rather do use 4 + ACPI_ID_LEN + 3 /* or 1 + 2 */ + 1 and explain each part in the comment above. > static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) > { > - int ret = 0; > struct byt_cht_es8316_private *priv; > + struct snd_soc_acpi_mach *mach; > + const char *i2c_name = NULL; > + int dai_index = 0; > + int i; > + int ret = 0; > > priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC); > if (!priv) > return -ENOMEM; > > + mach = (&pdev->dev)->platform_data; > + /* fix index of codec dai */ > + for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) { > + if (!strcmp(byt_cht_es8316_dais[i].codec_name, > + "i2c-ESSX8316:00")) { > + dai_index = i; > + break; > + } Perhaps at some point you can do such in more generic (across Intel ASoCs) ? > + } > + > + /* fixup codec name based on HID */ > + i2c_name = snd_soc_acpi_find_name_from_hid(mach->id); > + if (i2c_name) { > + snprintf(codec_name, sizeof(codec_name), > + "%s%s", "i2c-", i2c_name); > + byt_cht_es8316_dais[dai_index].codec_name = > codec_name; > + } > + > /* register the soc card */ > byt_cht_es8316_card.dev = &pdev->dev; > snd_soc_card_set_drvdata(&byt_cht_es8316_card, priv);
On 1/4/18 8:57 AM, Andy Shevchenko wrote: > On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote: >> Same problem as with previous machine drivers, the codec dai >> uses a hard-coded name of "i2c-ESSX8316:00" but ACPI provides >> "i2c-ESSX8316:01" in some systems. > > Yeah, that's why using device instances is fragile... > >> >> Fix by overriding the hard-coded value with the codec name derived >> from the HID information > > The patch makes me think about introducing acpi_dev_get_dev_name() and > utilize it here since I need something similar to have in one of GPIO > drivers. we use this: snd_soc_acpi_find_name_from_hid(mach->id) When I started all this the recommendation was to do all this on the audio side of things, I have no objections to a move of the functionality in acpi. [snip] >> diff --git a/sound/soc/intel/boards/bytcht_es8316.c >> b/sound/soc/intel/boards/bytcht_es8316.c >> index 8088396717e3..ae24f6205f05 100644 >> --- a/sound/soc/intel/boards/bytcht_es8316.c >> +++ b/sound/soc/intel/boards/bytcht_es8316.c >> @@ -232,15 +232,39 @@ static struct snd_soc_card byt_cht_es8316_card = >> { >> .fully_routed = true, >> }; >> >> +static char codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */ >> + > > I would rather do use 4 + ACPI_ID_LEN + 3 /* or 1 + 2 */ + 1 and explain > each part in the comment above. yes we could do this. this is the same code as in other machine drivers, so we should do the replacement across all of them in one shot in a separate patch. > >> static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) >> { >> - int ret = 0; >> struct byt_cht_es8316_private *priv; >> + struct snd_soc_acpi_mach *mach; >> + const char *i2c_name = NULL; >> + int dai_index = 0; >> + int i; >> + int ret = 0; >> >> priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC); >> if (!priv) >> return -ENOMEM; >> >> + mach = (&pdev->dev)->platform_data; >> + /* fix index of codec dai */ >> + for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) { >> + if (!strcmp(byt_cht_es8316_dais[i].codec_name, >> + "i2c-ESSX8316:00")) { >> + dai_index = i; >> + break; >> + } > > Perhaps at some point you can do such in more generic (across Intel > ASoCs) ? Not sure what you are hinting at here? Did you mean changing the name of the array? Or adding a helper function to do this?
On Thu, 2018-01-04 at 09:24 -0600, Pierre-Louis Bossart wrote: > On 1/4/18 8:57 AM, Andy Shevchenko wrote: > > On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote: > > > > > > Fix by overriding the hard-coded value with the codec name derived > > > from the HID information > > > > The patch makes me think about introducing acpi_dev_get_dev_name() > > and > > utilize it here since I need something similar to have in one of > > GPIO > > drivers. > > we use this: snd_soc_acpi_find_name_from_hid(mach->id) > > When I started all this the recommendation was to do all this on the > audio side of things, I have no objections to a move of the > functionality in acpi. I almost done a patch. I will Cc you for the series. > > > +static char codec_name[16]; /* i2c-<HID>:00 with HID being 8 > > > chars */ > > > + > > > > I would rather do use 4 + ACPI_ID_LEN + 3 /* or 1 + 2 */ + 1 and > > explain > > each part in the comment above. > > yes we could do this. this is the same code as in other machine > drivers, > so we should do the replacement across all of them in one shot in a > separate patch. Whatever you prefer, it's minor. > > > + mach = (&pdev->dev)->platform_data; > > > + /* fix index of codec dai */ > > > + for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) { > > > + if (!strcmp(byt_cht_es8316_dais[i].codec_name, > > > + "i2c-ESSX8316:00")) { > > > + dai_index = i; > > > + break; > > > + } > > > > Perhaps at some point you can do such in more generic (across Intel > > ASoCs) ? > > Not sure what you are hinting at here? Did you mean changing the name > of > the array? Or adding a helper function to do this? Suggestion is to create a helper out of similar for-loops-fix-name in all current users.
On 1/4/18 10:00 AM, Andy Shevchenko wrote: > On Thu, 2018-01-04 at 09:24 -0600, Pierre-Louis Bossart wrote: >> On 1/4/18 8:57 AM, Andy Shevchenko wrote: >>> On Wed, 2018-01-03 at 11:02 -0600, Pierre-Louis Bossart wrote: >>>> >>>> Fix by overriding the hard-coded value with the codec name derived >>>> from the HID information >>> >>> The patch makes me think about introducing acpi_dev_get_dev_name() >>> and >>> utilize it here since I need something similar to have in one of >>> GPIO >>> drivers. >> >> we use this: snd_soc_acpi_find_name_from_hid(mach->id) >> >> When I started all this the recommendation was to do all this on the >> audio side of things, I have no objections to a move of the >> functionality in acpi. > > I almost done a patch. I will Cc you for the series. ok. The only concern I have is that this needs to be a somewhat contained change that doesn't depend on all sorts of other ACPI/GPIO changes. We will have to maintain a v4.14-based branch for a very very long time and the simpler we make it for people who back-port or cherry-pick the better. > >>>> +static char codec_name[16]; /* i2c-<HID>:00 with HID being 8 >>>> chars */ >>>> + >>> >>> I would rather do use 4 + ACPI_ID_LEN + 3 /* or 1 + 2 */ + 1 and >>> explain >>> each part in the comment above. >> >> yes we could do this. this is the same code as in other machine >> drivers, >> so we should do the replacement across all of them in one shot in a >> separate patch. > > Whatever you prefer, it's minor. > >>>> + mach = (&pdev->dev)->platform_data; >>>> + /* fix index of codec dai */ >>>> + for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) { >>>> + if (!strcmp(byt_cht_es8316_dais[i].codec_name, >>>> + "i2c-ESSX8316:00")) { >>>> + dai_index = i; >>>> + break; >>>> + } >>> >>> Perhaps at some point you can do such in more generic (across Intel >>> ASoCs) ? >> >> Not sure what you are hinting at here? Did you mean changing the name >> of >> the array? Or adding a helper function to do this? > > Suggestion is to create a helper out of similar for-loops-fix-name in > all current users. ok, I'll work on this. Thanks for the suggestion.
On Thu, 2018-01-04 at 11:06 -0600, Pierre-Louis Bossart wrote: > On 1/4/18 10:00 AM, Andy Shevchenko wrote: > > On Thu, 2018-01-04 at 09:24 -0600, Pierre-Louis Bossart wrote: > > > we use this: snd_soc_acpi_find_name_from_hid(mach->id) > > > > > > When I started all this the recommendation was to do all this on > > > the > > > audio side of things, I have no objections to a move of the > > > functionality in acpi. > > > > I almost done a patch. I will Cc you for the series. > > ok. The only concern I have is that this needs to be a somewhat > contained change that doesn't depend on all sorts of other ACPI/GPIO > changes. We will have to maintain a v4.14-based branch for a very > very > long time and the simpler we make it for people who back-port or > cherry-pick the better. Yes, that is how series has been prepared. You basically need two first patches out of three from it. It had been sent already.
diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig index 89a73e3d9d2d..4af2393160bf 100644 --- a/sound/soc/intel/boards/Kconfig +++ b/sound/soc/intel/boards/Kconfig @@ -153,6 +153,7 @@ config SND_SOC_INTEL_BYT_CHT_DA7213_MACH config SND_SOC_INTEL_BYT_CHT_ES8316_MACH tristate "Baytrail & Cherrytrail with ES8316 codec" depends on X86_INTEL_LPSS && I2C && ACPI + select SND_SOC_ACPI select SND_SOC_ES8316 help This adds support for ASoC machine driver for Intel(R) Baytrail & diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c index 8088396717e3..ae24f6205f05 100644 --- a/sound/soc/intel/boards/bytcht_es8316.c +++ b/sound/soc/intel/boards/bytcht_es8316.c @@ -232,15 +232,39 @@ static struct snd_soc_card byt_cht_es8316_card = { .fully_routed = true, }; +static char codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */ + static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) { - int ret = 0; struct byt_cht_es8316_private *priv; + struct snd_soc_acpi_mach *mach; + const char *i2c_name = NULL; + int dai_index = 0; + int i; + int ret = 0; priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC); if (!priv) return -ENOMEM; + mach = (&pdev->dev)->platform_data; + /* fix index of codec dai */ + for (i = 0; i < ARRAY_SIZE(byt_cht_es8316_dais); i++) { + if (!strcmp(byt_cht_es8316_dais[i].codec_name, + "i2c-ESSX8316:00")) { + dai_index = i; + break; + } + } + + /* fixup codec name based on HID */ + i2c_name = snd_soc_acpi_find_name_from_hid(mach->id); + if (i2c_name) { + snprintf(codec_name, sizeof(codec_name), + "%s%s", "i2c-", i2c_name); + byt_cht_es8316_dais[dai_index].codec_name = codec_name; + } + /* register the soc card */ byt_cht_es8316_card.dev = &pdev->dev; snd_soc_card_set_drvdata(&byt_cht_es8316_card, priv);
Same problem as with previous machine drivers, the codec dai uses a hard-coded name of "i2c-ESSX8316:00" but ACPI provides "i2c-ESSX8316:01" in some systems. Fix by overriding the hard-coded value with the codec name derived from the HID information Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189261 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/soc/intel/boards/Kconfig | 1 + sound/soc/intel/boards/bytcht_es8316.c | 26 +++++++++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-)