| Message ID | 20211015164047.44492-2-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: hda: New NHLT functions and cleanup | expand |
On 10/15/21 11:40 AM, Cezary Rojewski wrote: > From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > > ACPI is device independent, so printing warnings using device functions > is misleading. Replace dev_xxx() with pr_xxx() and remove now > unnecessary argument. the routines in sound/hda/intel-nhtl.c are called from a specific PCI device, why would you remove that information? This makes no sense to me. > > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > include/sound/intel-nhlt.h | 9 ++++----- > sound/hda/intel-dsp-config.c | 4 ++-- > sound/hda/intel-nhlt.c | 24 +++++++++++++----------- > sound/soc/intel/skylake/skl.c | 5 ++--- > sound/soc/sof/intel/hda.c | 4 ++-- > 5 files changed, 23 insertions(+), 23 deletions(-) > > diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h > index d0574805865f..4debab7c1996 100644 > --- a/include/sound/intel-nhlt.h > +++ b/include/sound/intel-nhlt.h > @@ -126,17 +126,17 @@ enum { > NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, > }; > > -struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); > +struct nhlt_acpi_table *intel_nhlt_init(void); > > void intel_nhlt_free(struct nhlt_acpi_table *addr); > > -int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt); > +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt); > > #else > > struct nhlt_acpi_table; > > -static inline struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) > +static inline struct nhlt_acpi_table *intel_nhlt_init(void) > { > return NULL; > } > @@ -145,8 +145,7 @@ static inline void intel_nhlt_free(struct nhlt_acpi_table *addr) > { > } > > -static inline int intel_nhlt_get_dmic_geo(struct device *dev, > - struct nhlt_acpi_table *nhlt) > +static inline int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) > { > return 0; > } > diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c > index b9ac9e9e45a4..60cc4735c6ec 100644 > --- a/sound/hda/intel-dsp-config.c > +++ b/sound/hda/intel-dsp-config.c > @@ -382,9 +382,9 @@ static int snd_intel_dsp_check_dmic(struct pci_dev *pci) > struct nhlt_acpi_table *nhlt; > int ret = 0; > > - nhlt = intel_nhlt_init(&pci->dev); > + nhlt = intel_nhlt_init(); > if (nhlt) { > - if (intel_nhlt_get_dmic_geo(&pci->dev, nhlt)) > + if (intel_nhlt_get_dmic_geo(nhlt)) > ret = 1; > intel_nhlt_free(nhlt); > } > diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c > index e2237239d922..195d9e193a6c 100644 > --- a/sound/hda/intel-nhlt.c > +++ b/sound/hda/intel-nhlt.c > @@ -1,10 +1,12 @@ > // SPDX-License-Identifier: GPL-2.0-only > // Copyright (c) 2015-2019 Intel Corporation > > +#define pr_fmt(fmt) "NHLT: " fmt > + > #include <linux/acpi.h> > #include <sound/intel-nhlt.h> > > -struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) > +struct nhlt_acpi_table *intel_nhlt_init(void) > { > struct nhlt_acpi_table *nhlt; > acpi_status status; > @@ -12,7 +14,7 @@ struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) > status = acpi_get_table(ACPI_SIG_NHLT, 0, > (struct acpi_table_header **)&nhlt); > if (ACPI_FAILURE(status)) { > - dev_warn(dev, "NHLT table not found\n"); > + pr_warn("NHLT table not found\n"); > return NULL; > } > > @@ -26,7 +28,7 @@ void intel_nhlt_free(struct nhlt_acpi_table *nhlt) > } > EXPORT_SYMBOL_GPL(intel_nhlt_free); > > -int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) > { > struct nhlt_endpoint *epnt; > struct nhlt_dmic_array_config *cfg; > @@ -40,7 +42,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > return 0; > > if (nhlt->header.length <= sizeof(struct acpi_table_header)) { > - dev_warn(dev, "Invalid DMIC description table\n"); > + pr_warn("Invalid DMIC description table\n"); > return 0; > } > > @@ -55,7 +57,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > > /* find max number of channels based on format_configuration */ > if (fmt_configs->fmt_count) { > - dev_dbg(dev, "%s: found %d format definitions\n", > + pr_debug("%s: found %d format definitions\n", > __func__, fmt_configs->fmt_count); > > for (i = 0; i < fmt_configs->fmt_count; i++) { > @@ -66,9 +68,9 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > if (fmt_ext->fmt.channels > max_ch) > max_ch = fmt_ext->fmt.channels; > } > - dev_dbg(dev, "%s: max channels found %d\n", __func__, max_ch); > + pr_debug("%s: max channels found %d\n", __func__, max_ch); > } else { > - dev_dbg(dev, "%s: No format information found\n", __func__); > + pr_debug("%s: No format information found\n", __func__); > } > > if (cfg->device_config.config_type != NHLT_CONFIG_TYPE_MIC_ARRAY) { > @@ -90,21 +92,21 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > dmic_geo = cfg_vendor->nb_mics; > break; > default: > - dev_warn(dev, "%s: undefined DMIC array_type 0x%0x\n", > + pr_warn("%s: undefined DMIC array_type 0x%0x\n", > __func__, cfg->array_type); > } > > if (dmic_geo > 0) { > - dev_dbg(dev, "%s: Array with %d dmics\n", __func__, dmic_geo); > + pr_debug("%s: Array with %d dmics\n", __func__, dmic_geo); > } > if (max_ch > dmic_geo) { > - dev_dbg(dev, "%s: max channels %d exceed dmic number %d\n", > + pr_debug("%s: max channels %d exceed dmic number %d\n", > __func__, max_ch, dmic_geo); > } > } > } > > - dev_dbg(dev, "%s: dmic number %d max_ch %d\n", > + pr_debug("%s: dmic number %d max_ch %d\n", > __func__, dmic_geo, max_ch); > > return dmic_geo; > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c > index 5b1a15e39912..4f122616b636 100644 > --- a/sound/soc/intel/skylake/skl.c > +++ b/sound/soc/intel/skylake/skl.c > @@ -517,8 +517,7 @@ static int skl_find_machine(struct skl_dev *skl, void *driver_data) > if (pdata) { > skl->use_tplg_pcm = pdata->use_tplg_pcm; > mach->mach_params.dmic_num = > - intel_nhlt_get_dmic_geo(&skl->pci->dev, > - skl->nhlt); > + intel_nhlt_get_dmic_geo(skl->nhlt); > } > > return 0; > @@ -1009,7 +1008,7 @@ static int skl_probe(struct pci_dev *pci, > > device_disable_async_suspend(bus->dev); > > - skl->nhlt = intel_nhlt_init(bus->dev); > + skl->nhlt = intel_nhlt_init(); > > if (skl->nhlt == NULL) { > #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) > diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c > index 883d78dd01b5..75c2ee91bf13 100644 > --- a/sound/soc/sof/intel/hda.c > +++ b/sound/soc/sof/intel/hda.c > @@ -651,9 +651,9 @@ static int check_nhlt_dmic(struct snd_sof_dev *sdev) > struct nhlt_acpi_table *nhlt; > int dmic_num; > > - nhlt = intel_nhlt_init(sdev->dev); > + nhlt = intel_nhlt_init(); > if (nhlt) { > - dmic_num = intel_nhlt_get_dmic_geo(sdev->dev, nhlt); > + dmic_num = intel_nhlt_get_dmic_geo(nhlt); > intel_nhlt_free(nhlt); > if (dmic_num >= 1 && dmic_num <= 4) > return dmic_num; >
On Fri, 15 Oct 2021 18:42:33 +0200, Pierre-Louis Bossart wrote: > > > > On 10/15/21 11:40 AM, Cezary Rojewski wrote: > > From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > > > > ACPI is device independent, so printing warnings using device functions > > is misleading. Replace dev_xxx() with pr_xxx() and remove now > > unnecessary argument. > > the routines in sound/hda/intel-nhtl.c are called from a specific PCI > device, why would you remove that information? > > This makes no sense to me. Right, otherwise this change would confuse user, too; they'll be clueless about who triggers it. It's OK to change to pr_*(), but then it should have more information that can be easily identified and understood what user should do. thanks, Takashi > > > > > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > > --- > > include/sound/intel-nhlt.h | 9 ++++----- > > sound/hda/intel-dsp-config.c | 4 ++-- > > sound/hda/intel-nhlt.c | 24 +++++++++++++----------- > > sound/soc/intel/skylake/skl.c | 5 ++--- > > sound/soc/sof/intel/hda.c | 4 ++-- > > 5 files changed, 23 insertions(+), 23 deletions(-) > > > > diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h > > index d0574805865f..4debab7c1996 100644 > > --- a/include/sound/intel-nhlt.h > > +++ b/include/sound/intel-nhlt.h > > @@ -126,17 +126,17 @@ enum { > > NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, > > }; > > > > -struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); > > +struct nhlt_acpi_table *intel_nhlt_init(void); > > > > void intel_nhlt_free(struct nhlt_acpi_table *addr); > > > > -int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt); > > +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt); > > > > #else > > > > struct nhlt_acpi_table; > > > > -static inline struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) > > +static inline struct nhlt_acpi_table *intel_nhlt_init(void) > > { > > return NULL; > > } > > @@ -145,8 +145,7 @@ static inline void intel_nhlt_free(struct nhlt_acpi_table *addr) > > { > > } > > > > -static inline int intel_nhlt_get_dmic_geo(struct device *dev, > > - struct nhlt_acpi_table *nhlt) > > +static inline int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) > > { > > return 0; > > } > > diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c > > index b9ac9e9e45a4..60cc4735c6ec 100644 > > --- a/sound/hda/intel-dsp-config.c > > +++ b/sound/hda/intel-dsp-config.c > > @@ -382,9 +382,9 @@ static int snd_intel_dsp_check_dmic(struct pci_dev *pci) > > struct nhlt_acpi_table *nhlt; > > int ret = 0; > > > > - nhlt = intel_nhlt_init(&pci->dev); > > + nhlt = intel_nhlt_init(); > > if (nhlt) { > > - if (intel_nhlt_get_dmic_geo(&pci->dev, nhlt)) > > + if (intel_nhlt_get_dmic_geo(nhlt)) > > ret = 1; > > intel_nhlt_free(nhlt); > > } > > diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c > > index e2237239d922..195d9e193a6c 100644 > > --- a/sound/hda/intel-nhlt.c > > +++ b/sound/hda/intel-nhlt.c > > @@ -1,10 +1,12 @@ > > // SPDX-License-Identifier: GPL-2.0-only > > // Copyright (c) 2015-2019 Intel Corporation > > > > +#define pr_fmt(fmt) "NHLT: " fmt > > + > > #include <linux/acpi.h> > > #include <sound/intel-nhlt.h> > > > > -struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) > > +struct nhlt_acpi_table *intel_nhlt_init(void) > > { > > struct nhlt_acpi_table *nhlt; > > acpi_status status; > > @@ -12,7 +14,7 @@ struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) > > status = acpi_get_table(ACPI_SIG_NHLT, 0, > > (struct acpi_table_header **)&nhlt); > > if (ACPI_FAILURE(status)) { > > - dev_warn(dev, "NHLT table not found\n"); > > + pr_warn("NHLT table not found\n"); > > return NULL; > > } > > > > @@ -26,7 +28,7 @@ void intel_nhlt_free(struct nhlt_acpi_table *nhlt) > > } > > EXPORT_SYMBOL_GPL(intel_nhlt_free); > > > > -int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > > +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) > > { > > struct nhlt_endpoint *epnt; > > struct nhlt_dmic_array_config *cfg; > > @@ -40,7 +42,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > > return 0; > > > > if (nhlt->header.length <= sizeof(struct acpi_table_header)) { > > - dev_warn(dev, "Invalid DMIC description table\n"); > > + pr_warn("Invalid DMIC description table\n"); > > return 0; > > } > > > > @@ -55,7 +57,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > > > > /* find max number of channels based on format_configuration */ > > if (fmt_configs->fmt_count) { > > - dev_dbg(dev, "%s: found %d format definitions\n", > > + pr_debug("%s: found %d format definitions\n", > > __func__, fmt_configs->fmt_count); > > > > for (i = 0; i < fmt_configs->fmt_count; i++) { > > @@ -66,9 +68,9 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > > if (fmt_ext->fmt.channels > max_ch) > > max_ch = fmt_ext->fmt.channels; > > } > > - dev_dbg(dev, "%s: max channels found %d\n", __func__, max_ch); > > + pr_debug("%s: max channels found %d\n", __func__, max_ch); > > } else { > > - dev_dbg(dev, "%s: No format information found\n", __func__); > > + pr_debug("%s: No format information found\n", __func__); > > } > > > > if (cfg->device_config.config_type != NHLT_CONFIG_TYPE_MIC_ARRAY) { > > @@ -90,21 +92,21 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > > dmic_geo = cfg_vendor->nb_mics; > > break; > > default: > > - dev_warn(dev, "%s: undefined DMIC array_type 0x%0x\n", > > + pr_warn("%s: undefined DMIC array_type 0x%0x\n", > > __func__, cfg->array_type); > > } > > > > if (dmic_geo > 0) { > > - dev_dbg(dev, "%s: Array with %d dmics\n", __func__, dmic_geo); > > + pr_debug("%s: Array with %d dmics\n", __func__, dmic_geo); > > } > > if (max_ch > dmic_geo) { > > - dev_dbg(dev, "%s: max channels %d exceed dmic number %d\n", > > + pr_debug("%s: max channels %d exceed dmic number %d\n", > > __func__, max_ch, dmic_geo); > > } > > } > > } > > > > - dev_dbg(dev, "%s: dmic number %d max_ch %d\n", > > + pr_debug("%s: dmic number %d max_ch %d\n", > > __func__, dmic_geo, max_ch); > > > > return dmic_geo; > > diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c > > index 5b1a15e39912..4f122616b636 100644 > > --- a/sound/soc/intel/skylake/skl.c > > +++ b/sound/soc/intel/skylake/skl.c > > @@ -517,8 +517,7 @@ static int skl_find_machine(struct skl_dev *skl, void *driver_data) > > if (pdata) { > > skl->use_tplg_pcm = pdata->use_tplg_pcm; > > mach->mach_params.dmic_num = > > - intel_nhlt_get_dmic_geo(&skl->pci->dev, > > - skl->nhlt); > > + intel_nhlt_get_dmic_geo(skl->nhlt); > > } > > > > return 0; > > @@ -1009,7 +1008,7 @@ static int skl_probe(struct pci_dev *pci, > > > > device_disable_async_suspend(bus->dev); > > > > - skl->nhlt = intel_nhlt_init(bus->dev); > > + skl->nhlt = intel_nhlt_init(); > > > > if (skl->nhlt == NULL) { > > #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) > > diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c > > index 883d78dd01b5..75c2ee91bf13 100644 > > --- a/sound/soc/sof/intel/hda.c > > +++ b/sound/soc/sof/intel/hda.c > > @@ -651,9 +651,9 @@ static int check_nhlt_dmic(struct snd_sof_dev *sdev) > > struct nhlt_acpi_table *nhlt; > > int dmic_num; > > > > - nhlt = intel_nhlt_init(sdev->dev); > > + nhlt = intel_nhlt_init(); > > if (nhlt) { > > - dmic_num = intel_nhlt_get_dmic_geo(sdev->dev, nhlt); > > + dmic_num = intel_nhlt_get_dmic_geo(nhlt); > > intel_nhlt_free(nhlt); > > if (dmic_num >= 1 && dmic_num <= 4) > > return dmic_num; > > >
On 2021-10-17 9:52 AM, Takashi Iwai wrote: > On Fri, 15 Oct 2021 18:42:33 +0200, > Pierre-Louis Bossart wrote: >> On 10/15/21 11:40 AM, Cezary Rojewski wrote: >>> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> >>> >>> ACPI is device independent, so printing warnings using device functions >>> is misleading. Replace dev_xxx() with pr_xxx() and remove now >>> unnecessary argument. >> >> the routines in sound/hda/intel-nhtl.c are called from a specific PCI >> device, why would you remove that information? >> >> This makes no sense to me. > > Right, otherwise this change would confuse user, too; they'll be > clueless about who triggers it. > > It's OK to change to pr_*(), but then it should have more information > that can be easily identified and understood what user should do. Isn't the answer as to 'who' used it obvious, though? NHLT is used for I2S and DMIC endpoints only, so the question is 'limited' in the first place. And then, you cannot have several Intel ADSP drivers running simultaneously. Also, logs found ACPI-table interface are device-less so this patch makes NHLT interface look more ACPI-generic alike. Regards, Czarek
On Mon, 18 Oct 2021 10:08:30 +0200, Cezary Rojewski wrote: > > On 2021-10-17 9:52 AM, Takashi Iwai wrote: > > On Fri, 15 Oct 2021 18:42:33 +0200, > > Pierre-Louis Bossart wrote: > >> On 10/15/21 11:40 AM, Cezary Rojewski wrote: > >>> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > >>> > >>> ACPI is device independent, so printing warnings using device functions > >>> is misleading. Replace dev_xxx() with pr_xxx() and remove now > >>> unnecessary argument. > >> > >> the routines in sound/hda/intel-nhtl.c are called from a specific PCI > >> device, why would you remove that information? > >> > >> This makes no sense to me. > > > > Right, otherwise this change would confuse user, too; they'll be > > clueless about who triggers it. > > > > It's OK to change to pr_*(), but then it should have more information > > that can be easily identified and understood what user should do. > > Isn't the answer as to 'who' used it obvious, though? NHLT is used for > I2S and DMIC endpoints only, so the question is 'limited' in the first > place. And then, you cannot have several Intel ADSP drivers running > simultaneously. Well, it's not about you or devs -- those must know which driver is relevant very well, of course. Instead, the problem is for *all* others who read the message. IOW, which user would know and think "hey, it's a NHLT thingy that must be ASoC xxx driver that spewed" only from the text snippet "NHLT table not found"? That's way too much expectation. Some more guidance is needed in the error message. The dev_*() variant gave at least the device names that can help guessing the relevant driver easily. > Also, logs found ACPI-table interface are device-less so this patch > makes NHLT interface look more ACPI-generic alike. The conversion itself is no problem, but the lost information is the problem. Takashi
On 2021-10-18 10:25 AM, Takashi Iwai wrote: > On Mon, 18 Oct 2021 10:08:30 +0200, > Cezary Rojewski wrote: >> >> On 2021-10-17 9:52 AM, Takashi Iwai wrote: >>> On Fri, 15 Oct 2021 18:42:33 +0200, >>> Pierre-Louis Bossart wrote: >>>> On 10/15/21 11:40 AM, Cezary Rojewski wrote: >>>>> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> >>>>> >>>>> ACPI is device independent, so printing warnings using device functions >>>>> is misleading. Replace dev_xxx() with pr_xxx() and remove now >>>>> unnecessary argument. >>>> >>>> the routines in sound/hda/intel-nhtl.c are called from a specific PCI >>>> device, why would you remove that information? >>>> >>>> This makes no sense to me. >>> >>> Right, otherwise this change would confuse user, too; they'll be >>> clueless about who triggers it. >>> >>> It's OK to change to pr_*(), but then it should have more information >>> that can be easily identified and understood what user should do. >> >> Isn't the answer as to 'who' used it obvious, though? NHLT is used for >> I2S and DMIC endpoints only, so the question is 'limited' in the first >> place. And then, you cannot have several Intel ADSP drivers running >> simultaneously. > > Well, it's not about you or devs -- those must know which driver is > relevant very well, of course. Instead, the problem is for *all* > others who read the message. > > IOW, which user would know and think "hey, it's a NHLT thingy that > must be ASoC xxx driver that spewed" only from the text snippet "NHLT > table not found"? That's way too much expectation. Some more > guidance is needed in the error message. The dev_*() variant gave at > least the device names that can help guessing the relevant driver > easily. > >> Also, logs found ACPI-table interface are device-less so this patch >> makes NHLT interface look more ACPI-generic alike. > > The conversion itself is no problem, but the lost information is the > problem. Well, most invocations that end in error will cause stack of error-logs (dev_xxx()) in the parent driver. NHLT reads: Non-HDAudio-Link-Table. The name itself points reader towards sound/hda already. I understand that not everyone will google "NHLT" and such but then there are ton of kernel messages which won't guide reader by the hand anyway. "NHLT table not found" is configuration problem, not a driver problem. Mixing BIOS/config problems with driver ones leads to incorrect conclusions what we want to avoid. Regards, Czarek
On Mon, 18 Oct 2021 11:07:15 +0200, Cezary Rojewski wrote: > > > > On 2021-10-18 10:25 AM, Takashi Iwai wrote: > > On Mon, 18 Oct 2021 10:08:30 +0200, > > Cezary Rojewski wrote: > >> > >> On 2021-10-17 9:52 AM, Takashi Iwai wrote: > >>> On Fri, 15 Oct 2021 18:42:33 +0200, > >>> Pierre-Louis Bossart wrote: > >>>> On 10/15/21 11:40 AM, Cezary Rojewski wrote: > >>>>> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > >>>>> > >>>>> ACPI is device independent, so printing warnings using device functions > >>>>> is misleading. Replace dev_xxx() with pr_xxx() and remove now > >>>>> unnecessary argument. > >>>> > >>>> the routines in sound/hda/intel-nhtl.c are called from a specific PCI > >>>> device, why would you remove that information? > >>>> > >>>> This makes no sense to me. > >>> > >>> Right, otherwise this change would confuse user, too; they'll be > >>> clueless about who triggers it. > >>> > >>> It's OK to change to pr_*(), but then it should have more information > >>> that can be easily identified and understood what user should do. > >> > >> Isn't the answer as to 'who' used it obvious, though? NHLT is used for > >> I2S and DMIC endpoints only, so the question is 'limited' in the first > >> place. And then, you cannot have several Intel ADSP drivers running > >> simultaneously. > > > > Well, it's not about you or devs -- those must know which driver is > > relevant very well, of course. Instead, the problem is for *all* > > others who read the message. > > > > IOW, which user would know and think "hey, it's a NHLT thingy that > > must be ASoC xxx driver that spewed" only from the text snippet "NHLT > > table not found"? That's way too much expectation. Some more > > guidance is needed in the error message. The dev_*() variant gave at > > least the device names that can help guessing the relevant driver > > easily. > > > >> Also, logs found ACPI-table interface are device-less so this patch > >> makes NHLT interface look more ACPI-generic alike. > > > > The conversion itself is no problem, but the lost information is the > > problem. > > Well, most invocations that end in error will cause stack of > error-logs (dev_xxx()) in the parent driver. NHLT reads: > Non-HDAudio-Link-Table. The name itself points reader towards > sound/hda already. But then how dropping the device information would improve things at all? It rather cuts off the link between that message and the later one. > I understand that not everyone will google "NHLT" > and such but then there are ton of kernel messages which won't guide > reader by the hand anyway. This can't be an excuse, we don't have to follow that anti pattern :) > "NHLT table not found" is configuration problem, not a driver > problem. Mixing BIOS/config problems with driver ones leads to > incorrect conclusions what we want to avoid. The configuration itself makes sense only if it's in actual use, i.e. the problem is always tied with the driver. So, if any, you can put more information around the message mentioning that it's the configuration problem and what to do for users, e.g. check your BIOS, etc. *That* would be the improvement. Takashi
On 2021-10-18 2:01 PM, Takashi Iwai wrote: > On Mon, 18 Oct 2021 11:07:15 +0200, > Cezary Rojewski wrote: ... >> Well, most invocations that end in error will cause stack of >> error-logs (dev_xxx()) in the parent driver. NHLT reads: >> Non-HDAudio-Link-Table. The name itself points reader towards >> sound/hda already. > > But then how dropping the device information would improve things at > all? It rather cuts off the link between that message and the later > one. > >> I understand that not everyone will google "NHLT" >> and such but then there are ton of kernel messages which won't guide >> reader by the hand anyway. > > This can't be an excuse, we don't have to follow that anti pattern :) > >> "NHLT table not found" is configuration problem, not a driver >> problem. Mixing BIOS/config problems with driver ones leads to >> incorrect conclusions what we want to avoid. > > The configuration itself makes sense only if it's in actual use, > i.e. the problem is always tied with the driver. > > So, if any, you can put more information around the message mentioning > that it's the configuration problem and what to do for users, > e.g. check your BIOS, etc. *That* would be the improvement. Ok, I'll think about better solution. For now I'll just drop this patch from the series and send v2. Thanks for the discussion! Czarek
diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h index d0574805865f..4debab7c1996 100644 --- a/include/sound/intel-nhlt.h +++ b/include/sound/intel-nhlt.h @@ -126,17 +126,17 @@ enum { NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, }; -struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); +struct nhlt_acpi_table *intel_nhlt_init(void); void intel_nhlt_free(struct nhlt_acpi_table *addr); -int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt); +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt); #else struct nhlt_acpi_table; -static inline struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +static inline struct nhlt_acpi_table *intel_nhlt_init(void) { return NULL; } @@ -145,8 +145,7 @@ static inline void intel_nhlt_free(struct nhlt_acpi_table *addr) { } -static inline int intel_nhlt_get_dmic_geo(struct device *dev, - struct nhlt_acpi_table *nhlt) +static inline int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) { return 0; } diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index b9ac9e9e45a4..60cc4735c6ec 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -382,9 +382,9 @@ static int snd_intel_dsp_check_dmic(struct pci_dev *pci) struct nhlt_acpi_table *nhlt; int ret = 0; - nhlt = intel_nhlt_init(&pci->dev); + nhlt = intel_nhlt_init(); if (nhlt) { - if (intel_nhlt_get_dmic_geo(&pci->dev, nhlt)) + if (intel_nhlt_get_dmic_geo(nhlt)) ret = 1; intel_nhlt_free(nhlt); } diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c index e2237239d922..195d9e193a6c 100644 --- a/sound/hda/intel-nhlt.c +++ b/sound/hda/intel-nhlt.c @@ -1,10 +1,12 @@ // SPDX-License-Identifier: GPL-2.0-only // Copyright (c) 2015-2019 Intel Corporation +#define pr_fmt(fmt) "NHLT: " fmt + #include <linux/acpi.h> #include <sound/intel-nhlt.h> -struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +struct nhlt_acpi_table *intel_nhlt_init(void) { struct nhlt_acpi_table *nhlt; acpi_status status; @@ -12,7 +14,7 @@ struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) status = acpi_get_table(ACPI_SIG_NHLT, 0, (struct acpi_table_header **)&nhlt); if (ACPI_FAILURE(status)) { - dev_warn(dev, "NHLT table not found\n"); + pr_warn("NHLT table not found\n"); return NULL; } @@ -26,7 +28,7 @@ void intel_nhlt_free(struct nhlt_acpi_table *nhlt) } EXPORT_SYMBOL_GPL(intel_nhlt_free); -int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) { struct nhlt_endpoint *epnt; struct nhlt_dmic_array_config *cfg; @@ -40,7 +42,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) return 0; if (nhlt->header.length <= sizeof(struct acpi_table_header)) { - dev_warn(dev, "Invalid DMIC description table\n"); + pr_warn("Invalid DMIC description table\n"); return 0; } @@ -55,7 +57,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) /* find max number of channels based on format_configuration */ if (fmt_configs->fmt_count) { - dev_dbg(dev, "%s: found %d format definitions\n", + pr_debug("%s: found %d format definitions\n", __func__, fmt_configs->fmt_count); for (i = 0; i < fmt_configs->fmt_count; i++) { @@ -66,9 +68,9 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) if (fmt_ext->fmt.channels > max_ch) max_ch = fmt_ext->fmt.channels; } - dev_dbg(dev, "%s: max channels found %d\n", __func__, max_ch); + pr_debug("%s: max channels found %d\n", __func__, max_ch); } else { - dev_dbg(dev, "%s: No format information found\n", __func__); + pr_debug("%s: No format information found\n", __func__); } if (cfg->device_config.config_type != NHLT_CONFIG_TYPE_MIC_ARRAY) { @@ -90,21 +92,21 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) dmic_geo = cfg_vendor->nb_mics; break; default: - dev_warn(dev, "%s: undefined DMIC array_type 0x%0x\n", + pr_warn("%s: undefined DMIC array_type 0x%0x\n", __func__, cfg->array_type); } if (dmic_geo > 0) { - dev_dbg(dev, "%s: Array with %d dmics\n", __func__, dmic_geo); + pr_debug("%s: Array with %d dmics\n", __func__, dmic_geo); } if (max_ch > dmic_geo) { - dev_dbg(dev, "%s: max channels %d exceed dmic number %d\n", + pr_debug("%s: max channels %d exceed dmic number %d\n", __func__, max_ch, dmic_geo); } } } - dev_dbg(dev, "%s: dmic number %d max_ch %d\n", + pr_debug("%s: dmic number %d max_ch %d\n", __func__, dmic_geo, max_ch); return dmic_geo; diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 5b1a15e39912..4f122616b636 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -517,8 +517,7 @@ static int skl_find_machine(struct skl_dev *skl, void *driver_data) if (pdata) { skl->use_tplg_pcm = pdata->use_tplg_pcm; mach->mach_params.dmic_num = - intel_nhlt_get_dmic_geo(&skl->pci->dev, - skl->nhlt); + intel_nhlt_get_dmic_geo(skl->nhlt); } return 0; @@ -1009,7 +1008,7 @@ static int skl_probe(struct pci_dev *pci, device_disable_async_suspend(bus->dev); - skl->nhlt = intel_nhlt_init(bus->dev); + skl->nhlt = intel_nhlt_init(); if (skl->nhlt == NULL) { #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 883d78dd01b5..75c2ee91bf13 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -651,9 +651,9 @@ static int check_nhlt_dmic(struct snd_sof_dev *sdev) struct nhlt_acpi_table *nhlt; int dmic_num; - nhlt = intel_nhlt_init(sdev->dev); + nhlt = intel_nhlt_init(); if (nhlt) { - dmic_num = intel_nhlt_get_dmic_geo(sdev->dev, nhlt); + dmic_num = intel_nhlt_get_dmic_geo(nhlt); intel_nhlt_free(nhlt); if (dmic_num >= 1 && dmic_num <= 4) return dmic_num;