Re: [PATCH] ASoC: Intel: catpt: remove unneeded semicolon
On 2021-02-01 10:03 PM, Cezary Rojewski wrote:
On 2021-02-01 9:01 AM, Yang Li wrote:
Eliminate the following coccicheck warning:
./sound/soc/intel/catpt/pcm.c:355:2-3: Unneeded semicolon
Reported-by: Abaci Robot
Signed-off-by: Yang Li
---
sound/soc/intel/catpt/pcm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/catpt/pcm.c b/sound/soc/intel/catpt/pcm.c
index e5d54bb..88a0879 100644
--- a/sound/soc/intel/catpt/pcm.c
+++ b/sound/soc/intel/catpt/pcm.c
@@ -352,7 +352,7 @@ static int catpt_dai_apply_usettings(struct
snd_soc_dai *dai,
break;
default:
return 0;
- };
+ }
list_for_each_entry(pos, &component->card->snd_card->controls,
list) {
if (pos->private_data == component &&
Hello Yang,
Your patch is much appreciated.
I noticed that more mistakes such as this have been made in the code.
Could you please also update switch-statements in other parts of catpt
(from what I've found, pcm.c has 2 occurrences while loader.c has 1)?
I see now. Patch [1] provided the fixes already but optimization of mine
[2] done later, overridden part of it. In that case, there's nothing
else to be done.
Acked-by: Cezary Rojewski
Regards,
Czarek
[1]: https://lore.kernel.org/r/20201101171943.2305030-1-t...@redhat.com
[2]:
https://lore.kernel.org/r/2020111612.8530-4-cezary.rojew...@intel.com
[PATCH] Revert "dmaengine: dw: Enable runtime PM"
This reverts commit 842067940a3e3fc008a60fee388e000219b32632.
For some solutions e.g. sound/soc/intel/catpt, DW DMA is part of a
compound device (in that very example, domains: ADSP, SSP0, SSP1, DMA0
and DMA1 are part of a single entity) rather than being a standalone
one. Driver for said device may enlist DMA to transfer data during
suspend or resume sequences.
Manipulating RPM explicitly in dw's DMA request and release channel
functions causes suspend() to also invoke resume() for the exact same
device. Similar situation occurs for resume() sequence. Effectively
renders device dysfunctional after first suspend() attempt. Revert the
change to address the problem.
Cc: Andy Shevchenko
Signed-off-by: Cezary Rojewski
---
drivers/dma/dw/core.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 19a23767533a..7ab83fe601ed 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -982,11 +982,8 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
dev_vdbg(chan2dev(chan), "%s\n", __func__);
- pm_runtime_get_sync(dw->dma.dev);
-
/* ASSERT: channel is idle */
if (dma_readl(dw, CH_EN) & dwc->mask) {
- pm_runtime_put_sync_suspend(dw->dma.dev);
dev_dbg(chan2dev(chan), "DMA channel not idle?\n");
return -EIO;
}
@@ -1003,7 +1000,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
* We need controller-specific data to set up slave transfers.
*/
if (chan->private && !dw_dma_filter(chan, chan->private)) {
- pm_runtime_put_sync_suspend(dw->dma.dev);
dev_warn(chan2dev(chan), "Wrong controller-specific data\n");
return -EINVAL;
}
@@ -1047,8 +1043,6 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
if (!dw->in_use)
do_dw_dma_off(dw);
- pm_runtime_put_sync_suspend(dw->dma.dev);
-
dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
}
--
2.17.1
Re: [PATCH] Revert "dmaengine: dw: Enable runtime PM"
On 2021-02-03 6:06 PM, Andy Shevchenko wrote: On Wed, Feb 3, 2021 at 5:53 PM Cezary Rojewski wrote: This reverts commit 842067940a3e3fc008a60fee388e000219b32632. For some solutions e.g. sound/soc/intel/catpt, DW DMA is part of a compound device (in that very example, domains: ADSP, SSP0, SSP1, DMA0 and DMA1 are part of a single entity) rather than being a standalone one. Driver for said device may enlist DMA to transfer data during suspend or resume sequences. Manipulating RPM explicitly in dw's DMA request and release channel functions causes suspend() to also invoke resume() for the exact same device. Similar situation occurs for resume() sequence. Effectively renders device dysfunctional after first suspend() attempt. Revert the change to address the problem. I kinda had the mixed feelings about this, thanks for the report. Acked-by: Andy Shevchenko Fixes tag? Noted, sent v2 with updated tag area. Thanks, Czarek
[PATCH v2] Revert "dmaengine: dw: Enable runtime PM"
This reverts commit 842067940a3e3fc008a60fee388e000219b32632.
For some solutions e.g. sound/soc/intel/catpt, DW DMA is part of a
compound device (in that very example, domains: ADSP, SSP0, SSP1, DMA0
and DMA1 are part of a single entity) rather than being a standalone
one. Driver for said device may enlist DMA to transfer data during
suspend or resume sequences.
Manipulating RPM explicitly in dw's DMA request and release channel
functions causes suspend() to also invoke resume() for the exact same
device. Similar situation occurs for resume() sequence. Effectively
renders device dysfunctional after first suspend() attempt. Revert the
change to address the problem.
Fixes: 842067940a3e ("dmaengine: dw: Enable runtime PM")
Cc: Andy Shevchenko
Signed-off-by: Cezary Rojewski
Acked-by: Andy Shevchenko
---
Changes v2:
- enriched tag area with fixes tag
drivers/dma/dw/core.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 19a23767533a..7ab83fe601ed 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -982,11 +982,8 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
dev_vdbg(chan2dev(chan), "%s\n", __func__);
- pm_runtime_get_sync(dw->dma.dev);
-
/* ASSERT: channel is idle */
if (dma_readl(dw, CH_EN) & dwc->mask) {
- pm_runtime_put_sync_suspend(dw->dma.dev);
dev_dbg(chan2dev(chan), "DMA channel not idle?\n");
return -EIO;
}
@@ -1003,7 +1000,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
* We need controller-specific data to set up slave transfers.
*/
if (chan->private && !dw_dma_filter(chan, chan->private)) {
- pm_runtime_put_sync_suspend(dw->dma.dev);
dev_warn(chan2dev(chan), "Wrong controller-specific data\n");
return -EINVAL;
}
@@ -1047,8 +1043,6 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
if (!dw->in_use)
do_dw_dma_off(dw);
- pm_runtime_put_sync_suspend(dw->dma.dev);
-
dev_vdbg(chan2dev(chan), "%s: done\n", __func__);
}
--
2.17.1
Re: [PATCH] Revert "dmaengine: dw: Enable runtime PM"
On 2021-02-03 6:08 PM, Andy Shevchenko wrote: On Wed, Feb 3, 2021 at 7:06 PM Andy Shevchenko wrote: On Wed, Feb 3, 2021 at 5:53 PM Cezary Rojewski wrote: This reverts commit 842067940a3e3fc008a60fee388e000219b32632. For some solutions e.g. sound/soc/intel/catpt, DW DMA is part of a compound device (in that very example, domains: ADSP, SSP0, SSP1, DMA0 and DMA1 are part of a single entity) rather than being a standalone one. Driver for said device may enlist DMA to transfer data during suspend or resume sequences. Manipulating RPM explicitly in dw's DMA request and release channel functions causes suspend() to also invoke resume() for the exact same device. Similar situation occurs for resume() sequence. Effectively renders device dysfunctional after first suspend() attempt. Revert the change to address the problem. I kinda had the mixed feelings about this, thanks for the report. Side note: the better solution in general seems to have a specific power domain for the ASoC multi-function devices (if ever you move to use auxiliary bus, it may be done easier I think). This is an area I haven't touched yet. Will definitely check it out. Thanks for the recommendations, Andy. Much appreciated. Regards, Czarek
Re: [PATCH] ASoC: Intel: catpt: remove unneeded semicolon
On 2021-02-01 9:01 AM, Yang Li wrote:
Eliminate the following coccicheck warning:
./sound/soc/intel/catpt/pcm.c:355:2-3: Unneeded semicolon
Reported-by: Abaci Robot
Signed-off-by: Yang Li
---
sound/soc/intel/catpt/pcm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/catpt/pcm.c b/sound/soc/intel/catpt/pcm.c
index e5d54bb..88a0879 100644
--- a/sound/soc/intel/catpt/pcm.c
+++ b/sound/soc/intel/catpt/pcm.c
@@ -352,7 +352,7 @@ static int catpt_dai_apply_usettings(struct snd_soc_dai
*dai,
break;
default:
return 0;
- };
+ }
list_for_each_entry(pos, &component->card->snd_card->controls, list) {
if (pos->private_data == component &&
Hello Yang,
Your patch is much appreciated.
I noticed that more mistakes such as this have been made in the code.
Could you please also update switch-statements in other parts of catpt
(from what I've found, pcm.c has 2 occurrences while loader.c has 1)?
Regards,
Czarek
Re: No sound cards detected on Kabylake laptops after upgrade to kernel 5.8
On 2021-03-11 6:50 AM, Chris Chiu wrote: On Tue, Mar 9, 2021 at 11:29 PM Cezary Rojewski wrote: ... Topologies for most common skylake driver configurations: - skl/kbl with i2s rt286 - apl/glk with i2s rt298 - with hda dsp can be found in alsa-topology-conf [2]. Standard, official tool called 'alsatplg' is capable of compiling these into binary form which, after being transferred to /lib/firmware/ may be consumed by the driver during runtime. I have no problem with providing precompiled binaries to linux-firmware, if that's what community wants. ... I think the guild [1] is too complicated for normal users to fix the problem. Given it's not only the internal microphone being affected, it's no sound devices being created at all so no audio functions can work after kernel 5.8. Is there any potential problem to built-in the " with hda dsp" precompiled binary in linux-firmware? In general, linux-firmware is not the place to put driver-specific configuration files. It'd best to have standard UCM/topology files being build and honored during disto image creation. In regard to the guide, thanks for checking it out. What do you think could be improved so that normal user has easier time with it? Feedback is much appreciated. Regards, Czarek
Re: No sound cards detected on Kabylake laptops after upgrade to kernel 5.8
On 2021-03-09 1:19 PM, Chris Chiu wrote: Hi Guys, We have received reports that on some Kabylake laptops (Acer Swift SF314-54/55 and Lenovo Yoga C930...etc), all sound cards no longer be detected after upgrade to kernel later than 5.8. These laptops have one thing in common, all of them have Realtek audio codec and connect the internal microphone to DMIC of the Intel SST controller either [8086:9d71] or [8086:9dc8]. Please refer to https://bugzilla.kernel.org/show_bug.cgi?id=201251#c246 and https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1915117. From the dmesg from kernel 5.8, the sound related parts only show as follows but the expected snd_hda_codec_realtek and the snd_soc_skl are not even loaded then. [ 13.357495] snd_hda_intel :00:1f.3: DSP detected with PCI class/subclass/prog-if info 0x040100 [ 13.357500] snd_hda_intel :00:1f.3: Digital mics found on Skylake+ platform, using SST driver Building the kernel with the CONFIG_SND_SOC_INTEL_KBL removed can load the snd_hda_codec_realtek successfully and the pulseaudio and alsa-utils can detect the sound cards again. The result of bisecting between kernel 5.4 and 5.8 also get similar result, reverting the commit "ALSA: hda: Allow SST driver on SKL and KBL platforms with DMIC" can fix the issue. I tried to generate the required firmware for snd_soc_skl but it did not help. Please refer to what I did in https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1915117/comments/14 and https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1915117/comments/18. Since the skl_hda_dsp_generic-tplg.bin and dfw_sst.bin are not in the linux-firmware. The Intel SST support for Skylake family is not yet complete. Can we simply revert the "ALSA: hda: Allow SST driver on SKL and KBL platforms with DMIC" in the current stage and wait for SOF support for Skylake family? Or please suggest a better solution for this. Thanks Chris Hello Chris, Guide: "Linux: HDA+DMIC with skylake driver" [1] should help understanding history behind the problem as well as fixing it. Upstream skylake driver - snd_soc_skl - is intended to support HDA DSP + DMIC configuration via means of snd_soc_skl_hda_dsp machine board driver. You _may_ switch to legacy HDAudio driver - snd_hda_intel - losing DMIC support in the process. To remove any confusion - for Skylake and Kabylake platforms, snd_soc_skl is your option. Now, due to above, I doubt any skylake-related topology has ever been upstreamed to linux-firmware as a) most boards are I2S-based, these are used by our clients which we support via separate channel b) hda dsp+dmic support on linux for missing until early 2020. Topologies for most common skylake driver configurations: - skl/kbl with i2s rt286 - apl/glk with i2s rt298 - with hda dsp can be found in alsa-topology-conf [2]. Standard, official tool called 'alsatplg' is capable of compiling these into binary form which, after being transferred to /lib/firmware/ may be consumed by the driver during runtime. I have no problem with providing precompiled binaries to linux-firmware, if that's what community wants. Regards, Czarek [1]: https://gist.github.com/crojewsk/4e6382bfb0dbfaaf60513174211f29cb [2]: https://github.com/alsa-project/alsa-topology-conf/tree/master/topology
[PATCH 1/8] ASoC: Intel: Skylake: Remove superfluous chip initialization
commit 2ef81057d80456870b97890dd79c8f56a85b1242 upstream.
Skylake driver does the controller init operation twice:
- first during probe (only to stop it just before scheduling probe_work)
- and during said probe_work where the actual correct sequence is
executed
To properly complete boot sequence when iDisp codec is present, bus
initialization has to be called only after _i915_init() finishes.
With additional _reset_list preceding _i915_init(), iDisp codec never
gets the chance to enumerate on the link. Remove the superfluous
initialization to address the issue.
Signed-off-by: Cezary Rojewski
Reviewed-by: Pierre-Louis Bossart
Link: https://lore.kernel.org/r/20200305145314.32579-2-cezary.rojew...@intel.com
Signed-off-by: Mark Brown
Cc: # 5.4.x
---
sound/soc/intel/skylake/skl.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 141dbbf975ac..861c07417fed 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -807,6 +807,9 @@ static void skl_probe_work(struct work_struct *work)
return;
}
+ skl_init_pci(skl);
+ skl_dum_set(bus);
+
err = skl_init_chip(bus, true);
if (err < 0) {
dev_err(bus->dev, "Init chip failed with err: %d\n", err);
@@ -922,8 +925,6 @@ static int skl_first_init(struct hdac_bus *bus)
return -ENXIO;
}
- snd_hdac_bus_reset_link(bus, true);
-
snd_hdac_bus_parse_capabilities(bus);
/* check if PPCAP exists */
@@ -971,11 +972,7 @@ static int skl_first_init(struct hdac_bus *bus)
if (err < 0)
return err;
- /* initialize chip */
- skl_init_pci(skl);
- skl_dum_set(bus);
-
- return skl_init_chip(bus, true);
+ return 0;
}
static int skl_probe(struct pci_dev *pci,
@@ -1080,8 +1077,6 @@ static int skl_probe(struct pci_dev *pci,
if (bus->mlcap)
snd_hdac_ext_bus_get_ml_capabilities(bus);
- snd_hdac_bus_stop_chip(bus);
-
/* create device for soc dmic */
err = skl_dmic_device_register(skl);
if (err < 0) {
--
2.17.1
[PATCH 0/8] ASoC: Intel: Skylake: Fix HDAudio and DMIC for v5.4
First six of the backport address numerous problems troubling HDAudio configuration users for Skylake driver. Upstream series: "ASoC: Intel: Skylake: Fix HDaudio and Dmic" [1] provides the explanation and reasoning behind it. These have been initialy pushed into v5.7-rc1 via: "sound updates for 5.7-rc1" [2] by Takashi. Last two patches are from: "Add support for different DMIC configurations" [3] which focuses on HDAudio with DMIC configuration. Patch: "ASoC: Intel: Skylake: Add alternative topology binary name" of the mentioned series has already been merged to v5.4.y -stable and thus it's not included here. Fixes target mainly Skylake and Kabylake based platforms, released in 2015-2016 period. [1]: https://lore.kernel.org/alsa-devel/20200305145314.32579-1-cezary.rojew...@intel.com/ [2]: https://lore.kernel.org/lkml/s5htv22uso8.wl-ti...@suse.de/ [3]: https://lore.kernel.org/alsa-devel/20200427132727.24942-1-mateusz.gor...@linux.intel.com/ Cezary Rojewski (6): ASoC: Intel: Skylake: Remove superfluous chip initialization ASoC: Intel: Skylake: Select hda configuration permissively ASoC: Intel: Skylake: Enable codec wakeup during chip init ASoC: Intel: Skylake: Shield against no-NHLT configurations ASoC: Intel: Allow for ROM init retry on CNL platforms ASoC: Intel: Skylake: Await purge request ack on CNL Mateusz Gorski (2): ASoC: Intel: Multiple I/O PCM format support for pipe ASoC: Intel: Skylake: Automatic DMIC format configuration according to information from NHLT include/uapi/sound/skl-tplg-interface.h | 2 + sound/soc/intel/skylake/bxt-sst.c | 3 - sound/soc/intel/skylake/cnl-sst.c | 35 -- sound/soc/intel/skylake/skl-nhlt.c | 3 +- sound/soc/intel/skylake/skl-sst-dsp.h | 2 + sound/soc/intel/skylake/skl-topology.c | 159 +++- sound/soc/intel/skylake/skl-topology.h | 1 + sound/soc/intel/skylake/skl.c | 29 ++--- 8 files changed, 204 insertions(+), 30 deletions(-) -- 2.17.1
[PATCH 2/8] ASoC: Intel: Skylake: Select hda configuration permissively
commit a66f88394a78fec9a05fa6e517e9603e8eca8363 upstream.
With _reset_link removed from the probe sequence, codec_mask at the time
skl_find_hda_machine() is invoked will always be 0, so hda machine will
never be chosen. Rather than reorganizing boot flow, be permissive about
invalid mask. codec_mask will be set to proper value during probe_work -
before skl_codec_create() ever gets called.
Signed-off-by: Cezary Rojewski
Reviewed-by: Pierre-Louis Bossart
Link: https://lore.kernel.org/r/20200305145314.32579-3-cezary.rojew...@intel.com
Signed-off-by: Mark Brown
Cc: # 5.4.x
---
sound/soc/intel/skylake/skl.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 861c07417fed..f46b90ccb46f 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -480,13 +480,8 @@ static struct skl_ssp_clk skl_ssp_clks[] = {
static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl,
struct snd_soc_acpi_mach *machines)
{
- struct hdac_bus *bus = skl_to_bus(skl);
struct snd_soc_acpi_mach *mach;
- /* check if we have any codecs detected on bus */
- if (bus->codec_mask == 0)
- return NULL;
-
/* point to common table */
mach = snd_soc_acpi_intel_hda_machines;
--
2.17.1
[PATCH 4/8] ASoC: Intel: Skylake: Shield against no-NHLT configurations
commit 9e6c382f5a6161eb55115fb56614b9827f2e7da3 upstream.
Some configurations expose no NHLT table at all within their
/sys/firmware/acpi/tables. To prevent NULL-dereference errors from
occurring, adjust probe flow and append additional safety checks in
functions involved in NHLT lifecycle.
Signed-off-by: Cezary Rojewski
Reviewed-by: Pierre-Louis Bossart
Link: https://lore.kernel.org/r/20200305145314.32579-5-cezary.rojew...@intel.com
Signed-off-by: Mark Brown
Cc: # 5.4.x
---
sound/soc/intel/skylake/skl-nhlt.c | 3 ++-
sound/soc/intel/skylake/skl.c | 9 +++--
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-nhlt.c
b/sound/soc/intel/skylake/skl-nhlt.c
index 19f328d71f24..d9c8f5cb389e 100644
--- a/sound/soc/intel/skylake/skl-nhlt.c
+++ b/sound/soc/intel/skylake/skl-nhlt.c
@@ -182,7 +182,8 @@ void skl_nhlt_remove_sysfs(struct skl_dev *skl)
{
struct device *dev = &skl->pci->dev;
- sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr);
+ if (skl->nhlt)
+ sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr);
}
/*
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 1a3a3d6a3141..2e5fbd220923 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -632,6 +632,9 @@ static int skl_clock_device_register(struct skl_dev *skl)
struct platform_device_info pdevinfo = {NULL};
struct skl_clk_pdata *clk_pdata;
+ if (!skl->nhlt)
+ return 0;
+
clk_pdata = devm_kzalloc(&skl->pci->dev, sizeof(*clk_pdata),
GFP_KERNEL);
if (!clk_pdata)
@@ -1090,7 +1093,8 @@ static int skl_probe(struct pci_dev *pci,
out_clk_free:
skl_clock_device_unregister(skl);
out_nhlt_free:
- intel_nhlt_free(skl->nhlt);
+ if (skl->nhlt)
+ intel_nhlt_free(skl->nhlt);
out_free:
skl_free(bus);
@@ -1139,7 +1143,8 @@ static void skl_remove(struct pci_dev *pci)
skl_dmic_device_unregister(skl);
skl_clock_device_unregister(skl);
skl_nhlt_remove_sysfs(skl);
- intel_nhlt_free(skl->nhlt);
+ if (skl->nhlt)
+ intel_nhlt_free(skl->nhlt);
skl_free(bus);
dev_set_drvdata(&pci->dev, NULL);
}
--
2.17.1
[PATCH 3/8] ASoC: Intel: Skylake: Enable codec wakeup during chip init
commit e603f11d5df8997d104ab405ff27640b90baffaa upstream. Follow the recommendation set by hda_intel.c and enable HDMI/DP codec wakeup during bus initialization procedure. Disable wakeup once init completes. Signed-off-by: Cezary Rojewski Reviewed-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20200305145314.32579-4-cezary.rojew...@intel.com Signed-off-by: Mark Brown Cc: # 5.4.x --- sound/soc/intel/skylake/skl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index f46b90ccb46f..1a3a3d6a3141 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -129,6 +129,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset) struct hdac_ext_link *hlink; int ret; + snd_hdac_set_codec_wakeup(bus, true); skl_enable_miscbdcge(bus->dev, false); ret = snd_hdac_bus_init_chip(bus, full_reset); @@ -137,6 +138,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset) writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV); skl_enable_miscbdcge(bus->dev, true); + snd_hdac_set_codec_wakeup(bus, false); return ret; } -- 2.17.1
[PATCH 5/8] ASoC: Intel: Allow for ROM init retry on CNL platforms
commit 024aa45f55ccd40704cfdef61b2a8b6d0de9cdd1 upstream.
Due to unconditional initial timeouts, firmware may fail to load during
its initialization. This issue cannot be resolved on driver side as it
is caused by external sources such as CSME but has to be accounted for
nonetheless.
Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl
platform")
Signed-off-by: Cezary Rojewski
Reviewed-by: Pierre-Louis Bossart
Link: https://lore.kernel.org/r/20200305145314.32579-7-cezary.rojew...@intel.com
Signed-off-by: Mark Brown
Cc: # 5.4.x
---
sound/soc/intel/skylake/bxt-sst.c | 2 --
sound/soc/intel/skylake/cnl-sst.c | 15 ++-
sound/soc/intel/skylake/skl-sst-dsp.h | 1 +
3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/sound/soc/intel/skylake/bxt-sst.c
b/sound/soc/intel/skylake/bxt-sst.c
index 92a82e6b5fe6..cdafade8abd6 100644
--- a/sound/soc/intel/skylake/bxt-sst.c
+++ b/sound/soc/intel/skylake/bxt-sst.c
@@ -38,8 +38,6 @@
/* Delay before scheduling D0i3 entry */
#define BXT_D0I3_DELAY 5000
-#define BXT_FW_ROM_INIT_RETRY 3
-
static unsigned int bxt_get_errorcode(struct sst_dsp *ctx)
{
return sst_dsp_shim_read(ctx, BXT_ADSP_ERROR_CODE);
diff --git a/sound/soc/intel/skylake/cnl-sst.c
b/sound/soc/intel/skylake/cnl-sst.c
index 4f64f097e9ae..060e47ae3391 100644
--- a/sound/soc/intel/skylake/cnl-sst.c
+++ b/sound/soc/intel/skylake/cnl-sst.c
@@ -109,7 +109,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx)
{
struct firmware stripped_fw;
struct skl_dev *cnl = ctx->thread_context;
- int ret;
+ int ret, i;
if (!ctx->fw) {
ret = request_firmware(&ctx->fw, ctx->fw_name, ctx->dev);
@@ -131,12 +131,16 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx)
stripped_fw.size = ctx->fw->size;
skl_dsp_strip_extended_manifest(&stripped_fw);
- ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size);
- if (ret < 0) {
- dev_err(ctx->dev, "prepare firmware failed: %d\n", ret);
- goto cnl_load_base_firmware_failed;
+ for (i = 0; i < BXT_FW_ROM_INIT_RETRY; i++) {
+ ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size);
+ if (!ret)
+ break;
+ dev_dbg(ctx->dev, "prepare firmware failed: %d\n", ret);
}
+ if (ret < 0)
+ goto cnl_load_base_firmware_failed;
+
ret = sst_transfer_fw_host_dma(ctx);
if (ret < 0) {
dev_err(ctx->dev, "transfer firmware failed: %d\n", ret);
@@ -158,6 +162,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx)
return 0;
cnl_load_base_firmware_failed:
+ dev_err(ctx->dev, "firmware load failed: %d\n", ret);
release_firmware(ctx->fw);
ctx->fw = NULL;
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h
b/sound/soc/intel/skylake/skl-sst-dsp.h
index cdfec0fca577..067d1ea11cde 100644
--- a/sound/soc/intel/skylake/skl-sst-dsp.h
+++ b/sound/soc/intel/skylake/skl-sst-dsp.h
@@ -67,6 +67,7 @@ struct skl_dev;
#define SKL_FW_INIT0x1
#define SKL_FW_RFW_START 0xf
+#define BXT_FW_ROM_INIT_RETRY 3
#define SKL_ADSPIC_IPC 1
#define SKL_ADSPIS_IPC 1
--
2.17.1
[PATCH 6/8] ASoC: Intel: Skylake: Await purge request ack on CNL
commit 7693cadac86548b30389a6e11d78c38db654f393 upstream.
Each purge request is sent by driver after master core is powered up and
unresetted but before it is unstalled. On unstall, ROM begins processing
the request and initializing environment for FW load. Host should await
ROM's ack before moving forward. Without doing so, ROM init poll may
start too early and false timeouts can occur.
Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl
platform")
Signed-off-by: Cezary Rojewski
Reviewed-by: Pierre-Louis Bossart
Link: https://lore.kernel.org/r/20200305145314.32579-8-cezary.rojew...@intel.com
Signed-off-by: Mark Brown
Cc: # 5.4.x
---
sound/soc/intel/skylake/bxt-sst.c | 1 -
sound/soc/intel/skylake/cnl-sst.c | 20 ++--
sound/soc/intel/skylake/skl-sst-dsp.h | 1 +
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/bxt-sst.c
b/sound/soc/intel/skylake/bxt-sst.c
index cdafade8abd6..38b9d7494083 100644
--- a/sound/soc/intel/skylake/bxt-sst.c
+++ b/sound/soc/intel/skylake/bxt-sst.c
@@ -17,7 +17,6 @@
#include "skl.h"
#define BXT_BASEFW_TIMEOUT 3000
-#define BXT_INIT_TIMEOUT 300
#define BXT_ROM_INIT_TIMEOUT 70
#define BXT_IPC_PURGE_FW 0x01004000
diff --git a/sound/soc/intel/skylake/cnl-sst.c
b/sound/soc/intel/skylake/cnl-sst.c
index 060e47ae3391..c6abcd5aa67b 100644
--- a/sound/soc/intel/skylake/cnl-sst.c
+++ b/sound/soc/intel/skylake/cnl-sst.c
@@ -57,18 +57,34 @@ static int cnl_prepare_fw(struct sst_dsp *ctx, const void
*fwdata, u32 fwsize)
ctx->dsp_ops.stream_tag = stream_tag;
memcpy(ctx->dmab.area, fwdata, fwsize);
+ ret = skl_dsp_core_power_up(ctx, SKL_DSP_CORE0_MASK);
+ if (ret < 0) {
+ dev_err(ctx->dev, "dsp core0 power up failed\n");
+ ret = -EIO;
+ goto base_fw_load_failed;
+ }
+
/* purge FW request */
sst_dsp_shim_write(ctx, CNL_ADSP_REG_HIPCIDR,
CNL_ADSP_REG_HIPCIDR_BUSY | (CNL_IPC_PURGE |
((stream_tag - 1) << CNL_ROM_CTRL_DMA_ID)));
- ret = cnl_dsp_enable_core(ctx, SKL_DSP_CORE0_MASK);
+ ret = skl_dsp_start_core(ctx, SKL_DSP_CORE0_MASK);
if (ret < 0) {
- dev_err(ctx->dev, "dsp boot core failed ret: %d\n", ret);
+ dev_err(ctx->dev, "Start dsp core failed ret: %d\n", ret);
ret = -EIO;
goto base_fw_load_failed;
}
+ ret = sst_dsp_register_poll(ctx, CNL_ADSP_REG_HIPCIDA,
+ CNL_ADSP_REG_HIPCIDA_DONE,
+ CNL_ADSP_REG_HIPCIDA_DONE,
+ BXT_INIT_TIMEOUT, "HIPCIDA Done");
+ if (ret < 0) {
+ dev_err(ctx->dev, "timeout for purge request: %d\n", ret);
+ goto base_fw_load_failed;
+ }
+
/* enable interrupt */
cnl_ipc_int_enable(ctx);
cnl_ipc_op_int_enable(ctx);
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h
b/sound/soc/intel/skylake/skl-sst-dsp.h
index 067d1ea11cde..1df9ef422f61 100644
--- a/sound/soc/intel/skylake/skl-sst-dsp.h
+++ b/sound/soc/intel/skylake/skl-sst-dsp.h
@@ -68,6 +68,7 @@ struct skl_dev;
#define SKL_FW_INIT0x1
#define SKL_FW_RFW_START 0xf
#define BXT_FW_ROM_INIT_RETRY 3
+#define BXT_INIT_TIMEOUT 300
#define SKL_ADSPIC_IPC 1
#define SKL_ADSPIS_IPC 1
--
2.17.1
[PATCH 7/8] ASoC: Intel: Multiple I/O PCM format support for pipe
From: Mateusz Gorski
commit 1b450791d517d4dab9ab6d9a20c8819e3572 upstream.
For pipes supporting multiple input/output formats, kcontrol is
created and selection of pipe input and output configuration
is done based on control set.
If more than one configuration is supported, then this patch
allows user to select configuration of choice
using amixer settings.
Signed-off-by: Mateusz Gorski
Signed-off-by: Pavan K S
Reviewed-by: Cezary Rojewski
Reviewed-by: Pierre-Louis Bossart
Link:
https://lore.kernel.org/r/20200427132727.24942-3-mateusz.gor...@linux.intel.com
Signed-off-by: Mark Brown
Cc: # 5.4.x
---
include/uapi/sound/skl-tplg-interface.h | 1 +
sound/soc/intel/skylake/skl-topology.c | 95 +
sound/soc/intel/skylake/skl-topology.h | 1 +
3 files changed, 97 insertions(+)
diff --git a/include/uapi/sound/skl-tplg-interface.h
b/include/uapi/sound/skl-tplg-interface.h
index 9eee32f5e407..f2711186c81f 100644
--- a/include/uapi/sound/skl-tplg-interface.h
+++ b/include/uapi/sound/skl-tplg-interface.h
@@ -18,6 +18,7 @@
*/
#define SKL_CONTROL_TYPE_BYTE_TLV 0x100
#define SKL_CONTROL_TYPE_MIC_SELECT0x102
+#define SKL_CONTROL_TYPE_MULTI_IO_SELECT 0x103
#define HDA_SST_CFG_MAX900 /* size of copier cfg*/
#define MAX_IN_QUEUE 8
diff --git a/sound/soc/intel/skylake/skl-topology.c
b/sound/soc/intel/skylake/skl-topology.c
index 4b114ece58c6..c9cd6d60d57b 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -579,6 +579,38 @@ static int skl_tplg_unload_pipe_modules(struct skl_dev
*skl,
return ret;
}
+static bool skl_tplg_is_multi_fmt(struct skl_dev *skl, struct skl_pipe *pipe)
+{
+ struct skl_pipe_fmt *cur_fmt;
+ struct skl_pipe_fmt *next_fmt;
+ int i;
+
+ if (pipe->nr_cfgs <= 1)
+ return false;
+
+ if (pipe->conn_type != SKL_PIPE_CONN_TYPE_FE)
+ return true;
+
+ for (i = 0; i < pipe->nr_cfgs - 1; i++) {
+ if (pipe->direction == SNDRV_PCM_STREAM_PLAYBACK) {
+ cur_fmt = &pipe->configs[i].out_fmt;
+ next_fmt = &pipe->configs[i + 1].out_fmt;
+ } else {
+ cur_fmt = &pipe->configs[i].in_fmt;
+ next_fmt = &pipe->configs[i + 1].in_fmt;
+ }
+
+ if (!CHECK_HW_PARAMS(cur_fmt->channels, cur_fmt->freq,
+cur_fmt->bps,
+next_fmt->channels,
+next_fmt->freq,
+next_fmt->bps))
+ return true;
+ }
+
+ return false;
+}
+
/*
* Here, we select pipe format based on the pipe type and pipe
* direction to determine the current config index for the pipeline.
@@ -601,6 +633,14 @@ skl_tplg_get_pipe_config(struct skl_dev *skl, struct
skl_module_cfg *mconfig)
return 0;
}
+ if (skl_tplg_is_multi_fmt(skl, pipe)) {
+ pipe->cur_config_idx = pipe->pipe_config_idx;
+ pipe->memory_pages = pconfig->mem_pages;
+ dev_dbg(skl->dev, "found pipe config idx:%d\n",
+ pipe->cur_config_idx);
+ return 0;
+ }
+
if (pipe->conn_type == SKL_PIPE_CONN_TYPE_NONE) {
dev_dbg(skl->dev, "No conn_type detected, take 0th config\n");
pipe->cur_config_idx = 0;
@@ -1315,6 +1355,56 @@ static int skl_tplg_pga_event(struct snd_soc_dapm_widget
*w,
return 0;
}
+static int skl_tplg_multi_config_set_get(struct snd_kcontrol *kcontrol,
+struct snd_ctl_elem_value *ucontrol,
+bool is_set)
+{
+ struct snd_soc_component *component =
+ snd_soc_kcontrol_component(kcontrol);
+ struct hdac_bus *bus = snd_soc_component_get_drvdata(component);
+ struct skl_dev *skl = bus_to_skl(bus);
+ struct skl_pipeline *ppl;
+ struct skl_pipe *pipe = NULL;
+ struct soc_enum *ec = (struct soc_enum *)kcontrol->private_value;
+ u32 *pipe_id;
+
+ if (!ec)
+ return -EINVAL;
+
+ if (is_set && ucontrol->value.enumerated.item[0] > ec->items)
+ return -EINVAL;
+
+ pipe_id = ec->dobj.private;
+
+ list_for_each_entry(ppl, &skl->ppl_list, node) {
+ if (ppl->pipe->ppl_id == *pipe_id) {
+ pipe = ppl->pipe;
+ break;
+ }
+ }
+ if (!pipe)
+ return -EIO;
+
+ if (is_set)
+ pipe->pipe_config_idx = ucontrol->value.enumerated.item[0];
+ else
+ ucontrol->value.enumerated.item[0] = p
[PATCH 8/8] ASoC: Intel: Skylake: Automatic DMIC format configuration according to information from NHLT
From: Mateusz Gorski
commit 2d744ecf2b98405723a2138a547e5c75009bc4e5 upstream.
Automatically choose DMIC pipeline format configuration depending on
information included in NHLT.
Change the access rights of appropriate kcontrols to read-only in order
to prevent user interference.
Signed-off-by: Mateusz Gorski
Reviewed-by: Cezary Rojewski
Reviewed-by: Pierre-Louis Bossart
Link:
https://lore.kernel.org/r/20200427132727.24942-4-mateusz.gor...@linux.intel.com
Signed-off-by: Mark Brown
Cc: # 5.4.x
---
include/uapi/sound/skl-tplg-interface.h | 1 +
sound/soc/intel/skylake/skl-topology.c | 64 +++--
2 files changed, 62 insertions(+), 3 deletions(-)
diff --git a/include/uapi/sound/skl-tplg-interface.h
b/include/uapi/sound/skl-tplg-interface.h
index f2711186c81f..a93c0decfdd5 100644
--- a/include/uapi/sound/skl-tplg-interface.h
+++ b/include/uapi/sound/skl-tplg-interface.h
@@ -19,6 +19,7 @@
#define SKL_CONTROL_TYPE_BYTE_TLV 0x100
#define SKL_CONTROL_TYPE_MIC_SELECT0x102
#define SKL_CONTROL_TYPE_MULTI_IO_SELECT 0x103
+#define SKL_CONTROL_TYPE_MULTI_IO_SELECT_DMIC 0x104
#define HDA_SST_CFG_MAX900 /* size of copier cfg*/
#define MAX_IN_QUEUE 8
diff --git a/sound/soc/intel/skylake/skl-topology.c
b/sound/soc/intel/skylake/skl-topology.c
index c9cd6d60d57b..aa5833001fde 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -1405,6 +1405,18 @@ static int skl_tplg_multi_config_set(struct snd_kcontrol
*kcontrol,
return skl_tplg_multi_config_set_get(kcontrol, ucontrol, true);
}
+static int skl_tplg_multi_config_get_dmic(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ return skl_tplg_multi_config_set_get(kcontrol, ucontrol, false);
+}
+
+static int skl_tplg_multi_config_set_dmic(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ return skl_tplg_multi_config_set_get(kcontrol, ucontrol, true);
+}
+
static int skl_tplg_tlv_control_get(struct snd_kcontrol *kcontrol,
unsigned int __user *data, unsigned int size)
{
@@ -1949,6 +1961,11 @@ static const struct snd_soc_tplg_kcontrol_ops
skl_tplg_kcontrol_ops[] = {
.get = skl_tplg_multi_config_get,
.put = skl_tplg_multi_config_set,
},
+ {
+ .id = SKL_CONTROL_TYPE_MULTI_IO_SELECT_DMIC,
+ .get = skl_tplg_multi_config_get_dmic,
+ .put = skl_tplg_multi_config_set_dmic,
+ }
};
static int skl_tplg_fill_pipe_cfg(struct device *dev,
@@ -3109,12 +3126,21 @@ static int skl_tplg_control_load(struct
snd_soc_component *cmpnt,
case SND_SOC_TPLG_CTL_ENUM:
tplg_ec = container_of(hdr,
struct snd_soc_tplg_enum_control, hdr);
- if (kctl->access & SNDRV_CTL_ELEM_ACCESS_READWRITE) {
+ if (kctl->access & SNDRV_CTL_ELEM_ACCESS_READ) {
se = (struct soc_enum *)kctl->private_value;
if (tplg_ec->priv.size)
- return skl_init_enum_data(bus->dev, se,
- tplg_ec);
+ skl_init_enum_data(bus->dev, se, tplg_ec);
}
+
+ /*
+* now that the control initializations are done, remove
+* write permission for the DMIC configuration enums to
+* avoid conflicts between NHLT settings and user interaction
+*/
+
+ if (hdr->ops.get == SKL_CONTROL_TYPE_MULTI_IO_SELECT_DMIC)
+ kctl->access = SNDRV_CTL_ELEM_ACCESS_READ;
+
break;
default:
@@ -3584,6 +3610,37 @@ static int skl_manifest_load(struct snd_soc_component
*cmpnt, int index,
return 0;
}
+static void skl_tplg_complete(struct snd_soc_component *component)
+{
+ struct snd_soc_dobj *dobj;
+ struct snd_soc_acpi_mach *mach =
+ dev_get_platdata(component->card->dev);
+ int i;
+
+ list_for_each_entry(dobj, &component->dobj_list, list) {
+ struct snd_kcontrol *kcontrol = dobj->control.kcontrol;
+ struct soc_enum *se =
+ (struct soc_enum *)kcontrol->private_value;
+ char **texts = dobj->control.dtexts;
+ char chan_text[4];
+
+ if (dobj->type != SND_SOC_DOBJ_ENUM ||
+ dobj->control.kcontrol->put !=
+ skl_tplg_multi_config_set_dmic)
+ continue;
+ sprintf(chan_text, "c%d", mach->mach_params.dmic_num);
+
+ for (i = 0; i < se->items; i++) {
+ struc
Re: [PATCH 2/6] ASoC: Intel: Fix use of potentially uninitialized variable
On 2019-08-27 16:17, Amadeusz Sławiński wrote:
From: Amadeusz Sławiński
If ipc->ops.reply_msg_match is NULL, we may end up using uninitialized
mask value.
reported by smatch:
sound/soc/intel/common/sst-ipc.c:266 sst_ipc_reply_find_msg() error:
uninitialized symbol 'mask'.
Signed-off-by: Amadeusz Sławiński
---
sound/soc/intel/common/sst-ipc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/sound/soc/intel/common/sst-ipc.c b/sound/soc/intel/common/sst-ipc.c
index 1186a03a88d6..6068bb697e22 100644
--- a/sound/soc/intel/common/sst-ipc.c
+++ b/sound/soc/intel/common/sst-ipc.c
@@ -223,6 +223,8 @@ struct ipc_message *sst_ipc_reply_find_msg(struct
sst_generic_ipc *ipc,
if (ipc->ops.reply_msg_match != NULL)
header = ipc->ops.reply_msg_match(header, &mask);
+ else
+ mask = (u64)-1;
Please see linux/limits.h and check if this can't be replaced by an
equivalent found there.
if (list_empty(&ipc->rx_list)) {
dev_err(ipc->dev, "error: rx list empty but received 0x%llx\n",
Re: [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids
On 2019-09-27 15:14, Pierre-Louis Bossart wrote: On 9/26/19 9:55 PM, Navid Emamdoost wrote: On Wed, Sep 25, 2019 at 12:05:28PM -0500, Pierre-Louis Bossart wrote: On 9/25/19 11:19 AM, Navid Emamdoost wrote: In snd_skl_parse_uuids if allocation for module->instance_id fails, the allocated memory for module shoulde be released. I changes the allocation for module to use devm_kzalloc to be resource_managed allocation and avoid the release in error path. if you use devm_, don't you need to fix the error path as well then, I see a kfree(uuid) in skl_freeup_uuid_list(). I am not very familiar with this code but the error seems to be that the list_add_tail() is called after the module->instance_id is allocated, so there is a risk that the module allocated earlier is not freed (since it's not yet added to the list). Freeing the module as done in patch 1 works, using devm_ without fixing the error path does not seem correct to me. Good catch, Pierre. Thanks for the feedback, then it's your call if you can accept patch 1 as fix. Cezary, it's really your call. Actually, not the best person to ask about "objective decisions" here as my vision is clouded by changes done internally. This code no longer exists in our internal repo. It's better for host to send MODULE_INFO request rather than understanding firmware binary structure and parse it directly. I'm fine with solution #1 as I guess asking to wait for refactor is not an option. Code deployment is delayed due to range of administrative decisions, some of which should be uncovered on alsa-devel soon enough. Czarek
Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
On 2019-07-18 11:02, Oleksandr Suvorov wrote:
+enum {
+ HP_POWER_EVENT,
+ DAC_POWER_EVENT,
+ ADC_POWER_EVENT,
+ LAST_POWER_EVENT
+};
+
+static u16 mute_mask[] = {
+ SGTL5000_HP_MUTE,
+ SGTL5000_OUTPUTS_MUTE,
+ SGTL5000_OUTPUTS_MUTE
+};
If mute_mask[] is only used within common handler, you may consider
declaring const array within said handler instead (did not check that
myself).
Otherwise, simple comment for the second _OUTPUTS_MUTE should suffice -
its not self explanatory why you doubled that mask.
+
/* sgtl5000 private structure in codec */
struct sgtl5000_priv {
int sysclk; /* sysclk rate */
@@ -137,8 +157,109 @@ struct sgtl5000_priv {
u8 micbias_voltage;
u8 lrclk_strength;
u8 sclk_strength;
+ u16 mute_state[LAST_POWER_EVENT];
};
When I spoke of LAST enum constant, I did not really had this specific
usage in mind.
From design perspective, _LAST_ does not exist and should never be
referred to as "the next option" i.e.: new enum constant.
That is way preferred usage is:
u16 mute_state[ADC_POWER_EVENT+1;
-or-
u16 mute_state[LAST_POWER_EVENT+1];
Maybe I'm just being radical here :)
Czarek
Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
On 2019-07-18 20:42, Cezary Rojewski wrote:
On 2019-07-18 11:02, Oleksandr Suvorov wrote:
+enum {
+ HP_POWER_EVENT,
+ DAC_POWER_EVENT,
+ ADC_POWER_EVENT,
+ LAST_POWER_EVENT
+};
+
+static u16 mute_mask[] = {
+ SGTL5000_HP_MUTE,
+ SGTL5000_OUTPUTS_MUTE,
+ SGTL5000_OUTPUTS_MUTE
+};
If mute_mask[] is only used within common handler, you may consider
declaring const array within said handler instead (did not check that
myself).
Otherwise, simple comment for the second _OUTPUTS_MUTE should suffice -
its not self explanatory why you doubled that mask.
+
/* sgtl5000 private structure in codec */
struct sgtl5000_priv {
int sysclk; /* sysclk rate */
@@ -137,8 +157,109 @@ struct sgtl5000_priv {
u8 micbias_voltage;
u8 lrclk_strength;
u8 sclk_strength;
+ u16 mute_state[LAST_POWER_EVENT];
};
When I spoke of LAST enum constant, I did not really had this specific
usage in mind.
From design perspective, _LAST_ does not exist and should never be
referred to as "the next option" i.e.: new enum constant.
That is way preferred usage is:
u16 mute_state[ADC_POWER_EVENT+1;
-or-
u16 mute_state[LAST_POWER_EVENT+1];
Maybe I'm just being radical here :)
Czarek
Forgive me for double posting. Comment above is targeted towards:
>> +enum {
>> +HP_POWER_EVENT,
>> +DAC_POWER_EVENT,
>> +ADC_POWER_EVENT,
>> +LAST_POWER_EVENT
>> +};
as LAST_POWER_EVENT is not assigned explicitly to ADC_POWER_EVENT and
thus generates implicit "new option" of value 3.
Czarek
Re: [PATCH v2 05/11] ASoC: wcd934x: add playback dapm widgets
On 2019-10-18 02:18, Srinivas Kandagatla wrote:
+static int wcd934x_codec_enable_slim(struct snd_soc_dapm_widget *w,
+struct snd_kcontrol *kc,
+ int event)
+{
+ struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
+ struct wcd934x_codec *wcd = snd_soc_component_get_drvdata(comp);
+ struct wcd_slim_codec_dai_data *dai = &wcd->dai[w->shift];
+
+ switch (event) {
+ case SND_SOC_DAPM_POST_PMU:
+ wcd934x_codec_enable_int_port(dai, comp);
+ break;
+ case SNDRV_PCM_TRIGGER_STOP:
+ break;
Any reason for mentioning _TRIGGER_STOP here?
+ case SND_SOC_DAPM_POST_PMD:
+ kfree(dai->sconfig.chs);
+
+ break;
Comment for kfree depending on _event_ would be advised.
+ }
+
+ return 0;
+}
+
+static void wcd934x_codec_hd2_control(struct snd_soc_component *component,
+ u16 interp_idx, int event)
+{
+ u16 hd2_scale_reg;
+ u16 hd2_enable_reg = 0;
+
+ switch (interp_idx) {
+ case INTERP_HPHL:
+ hd2_scale_reg = WCD934X_CDC_RX1_RX_PATH_SEC3;
+ hd2_enable_reg = WCD934X_CDC_RX1_RX_PATH_CFG0;
+ break;
+ case INTERP_HPHR:
+ hd2_scale_reg = WCD934X_CDC_RX2_RX_PATH_SEC3;
+ hd2_enable_reg = WCD934X_CDC_RX2_RX_PATH_CFG0;
+ break;
+ }
What's the rest of this function for if switch-case does not match?
Without hd2_enable_reg > 0 you might as well return immediately.
+
+ if (hd2_enable_reg && SND_SOC_DAPM_EVENT_ON(event)) {
+ snd_soc_component_update_bits(component, hd2_scale_reg,
+ WCD934X_CDC_RX_PATH_SEC_HD2_ALPHA_MASK,
+ WCD934X_CDC_RX_PATH_SEC_HD2_ALPHA_0P3125);
+ snd_soc_component_update_bits(component, hd2_enable_reg,
+ WCD934X_CDC_RX_PATH_CFG_HD2_EN_MASK,
+ WCD934X_CDC_RX_PATH_CFG_HD2_ENABLE);
+ }
+
+ if (hd2_enable_reg && SND_SOC_DAPM_EVENT_OFF(event)) {
+ snd_soc_component_update_bits(component, hd2_enable_reg,
+ WCD934X_CDC_RX_PATH_CFG_HD2_EN_MASK,
+ WCD934X_CDC_RX_PATH_CFG_HD2_DISABLE);
+ snd_soc_component_update_bits(component, hd2_scale_reg,
+ WCD934X_CDC_RX_PATH_SEC_HD2_ALPHA_MASK,
+ WCD934X_CDC_RX_PATH_SEC_HD2_ALPHA_0P);
+ }
+}
+
+static void wcd934x_codec_hphdelay_lutbypass(struct snd_soc_component *comp,
+u16 interp_idx, int event)
+{
+ u8 hph_dly_mask;
+ u16 hph_lut_bypass_reg = 0;
+ u16 hph_comp_ctrl7 = 0;
+
+ switch (interp_idx) {
+ case INTERP_HPHL:
+ hph_dly_mask = 1;
+ hph_lut_bypass_reg = WCD934X_CDC_TOP_HPHL_COMP_LUT;
+ hph_comp_ctrl7 = WCD934X_CDC_COMPANDER1_CTL7;
+ break;
+ case INTERP_HPHR:
+ hph_dly_mask = 2;
+ hph_lut_bypass_reg = WCD934X_CDC_TOP_HPHR_COMP_LUT;
+ hph_comp_ctrl7 = WCD934X_CDC_COMPANDER2_CTL7;
+ break;
+ default:
+ break;
+ }
'Default' made it here, what was not the case for most of other
switch-case. Keep code consistent would be appreciated.
Moreover, in the following function "wcd934x_config_compander", you do
decide to do all the processing directly within switch-case. I see no
reason why you should not do that here too.
Again, once switch-case fails to find match, the rest of function does
not do much, really.
+
+ if (hph_lut_bypass_reg && SND_SOC_DAPM_EVENT_ON(event)) {
+ snd_soc_component_update_bits(comp, WCD934X_CDC_CLSH_TEST0,
+ hph_dly_mask, 0x0);
+ snd_soc_component_update_bits(comp, hph_lut_bypass_reg,
+ WCD934X_HPH_LUT_BYPASS_MASK,
+ WCD934X_HPH_LUT_BYPASS_ENABLE);
+ }
+
+ if (hph_lut_bypass_reg && SND_SOC_DAPM_EVENT_OFF(event)) {
+ snd_soc_component_update_bits(comp, WCD934X_CDC_CLSH_TEST0,
+ hph_dly_mask, hph_dly_mask);
+ snd_soc_component_update_bits(comp, hph_lut_bypass_reg,
+ WCD934X_HPH_LUT_BYPASS_MASK,
+ WCD934X_HPH_LUT_BYPASS_DISABLE);
+ }
+}
+
+static int wcd934x_config_compander(struct snd_soc_component *comp,
+ int interp_n, int event)
+{
+ struct wcd934x_codec *wcd = dev_get_drvdata(comp->dev);
+ int compander;
+ u16 co
Re: [PATCH v3 6/6] ASoC: sgtl5000: Improve VAG power and mute control
On 2019-07-12 16:56, Oleksandr Suvorov wrote:
+enum {
+ HP_POWER_EVENT,
+ DAC_POWER_EVENT,
+ ADC_POWER_EVENT
+};
+
+struct sgtl5000_mute_state {
+ u16 hp_event;
+ u16 dac_event;
+ u16 adc_event;
+};
+
/* sgtl5000 private structure in codec */
struct sgtl5000_priv {
int sysclk; /* sysclk rate */
@@ -137,8 +156,109 @@ struct sgtl5000_priv {
u8 micbias_voltage;
u8 lrclk_strength;
u8 sclk_strength;
+ struct sgtl5000_mute_state mute_state;
Why not array instead?
u16 mute_state[ADC_POWER_EVENT+1];
-or-
u16 mute_state[LAST_POWER_EVENT+1]; (if you choose to add explicit LAST
enum constant).
Enables simplification, see below.
@@ -170,40 +290,79 @@ static int mic_bias_event(struct snd_soc_dapm_widget *w,
return 0;
}
-/*
- * As manual described, ADC/DAC only works when VAG powerup,
- * So enabled VAG before ADC/DAC up.
- * In power down case, we need wait 400ms when vag fully ramped down.
- */
-static int power_vag_event(struct snd_soc_dapm_widget *w,
- struct snd_kcontrol *kcontrol, int event)
+static void vag_and_mute_control(struct snd_soc_component *component,
+int event, int event_source,
+u16 mute_mask, u16 *mute_reg)
{
- struct snd_soc_component *component =
snd_soc_dapm_to_component(w->dapm);
- const u32 mask = SGTL5000_DAC_POWERUP | SGTL5000_ADC_POWERUP;
-
switch (event) {
- case SND_SOC_DAPM_POST_PMU:
- snd_soc_component_update_bits(component,
SGTL5000_CHIP_ANA_POWER,
- SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
- msleep(400);
+ case SND_SOC_DAPM_PRE_PMU:
+ *mute_reg = mute_output(component, mute_mask);
+ break;
+ case SND_SOC_DAPM_POST_PMU:
+ vag_power_on(component, event_source);
+ restore_output(component, mute_mask, *mute_reg);
break;
-
case SND_SOC_DAPM_PRE_PMD:
- /*
-* Don't clear VAG_POWERUP, when both DAC and ADC are
-* operational to prevent inadvertently starving the
-* other one of them.
-*/
- if ((snd_soc_component_read32(component, SGTL5000_CHIP_ANA_POWER)
&
- mask) != mask) {
- snd_soc_component_update_bits(component,
SGTL5000_CHIP_ANA_POWER,
- SGTL5000_VAG_POWERUP, 0);
- msleep(400);
- }
+ *mute_reg = mute_output(component, mute_mask);
+ vag_power_off(component, event_source);
+ break;
+ case SND_SOC_DAPM_POST_PMD:
+ restore_output(component, mute_mask, *mute_reg);
break;
default:
break;
}
+}
+
+/*
+ * Mute Headphone when power it up/down.
+ * Control VAG power on HP power path.
+ */
+static int headphone_pga_event(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event)
+{
+ struct snd_soc_component *component =
+ snd_soc_dapm_to_component(w->dapm);
+ struct sgtl5000_priv *sgtl5000 =
+ snd_soc_component_get_drvdata(component);
+
+ vag_and_mute_control(component, event, HP_POWER_EVENT,
+SGTL5000_HP_MUTE,
+&sgtl5000->mute_state.hp_event);
+
+ return 0;
+}
+
+/* As manual describes, ADC/DAC powering up/down requires
+ * to mute outputs to avoid pops.
+ * Control VAG power on ADC/DAC power path.
+ */
+static int adc_updown_depop(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event)
+{
+ struct snd_soc_component *component =
+ snd_soc_dapm_to_component(w->dapm);
+ struct sgtl5000_priv *sgtl5000 =
+ snd_soc_component_get_drvdata(component);
+
+ vag_and_mute_control(component, event, ADC_POWER_EVENT,
+SGTL5000_OUTPUTS_MUTE,
+&sgtl5000->mute_state.adc_event);
+
+ return 0;
+}
+
+static int dac_updown_depop(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event)
+{
+ struct snd_soc_component *component =
+ snd_soc_dapm_to_component(w->dapm);
+ struct sgtl5000_priv *sgtl5000 =
+ snd_soc_component_get_drvdata(component);
+
+ vag_and_mute_control(component, event, DAC_POWER_EVENT,
+SGTL5000_OUTPUTS_MUTE,
+&sgtl5000->mute_state.dac_event);
return 0;
}
With array approach you can simplify these 3 callbacks:
- remove local declaration of sgtl5000
- remove the need for "u16 *mute_reg" param for vag_and_mute_control
(you always provide the xxx_event field of mute_state corresponding to
given XXX_POWER_EVENT anyway)
The sgtl5000 local ptr would be
Re: [PATCH v2 3/5] ASoC: core: add support to snd_soc_dai_get_sdw_stream()
On 2019-08-13 10:35, Srinivas Kandagatla wrote:
On platforms which have smart speaker amplifiers connected via
soundwire and modeled as aux devices in ASoC, in such usecases machine
driver should be able to get sdw master stream from dai so that it can
use the runtime stream to setup slave streams.
soundwire already as a set function, get function would provide more
flexibility to above configurations.
Signed-off-by: Srinivas Kandagatla
---
include/sound/soc-dai.h | 10 ++
1 file changed, 10 insertions(+)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index dc48fe081a20..1e01f4a302e0 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -202,6 +202,7 @@ struct snd_soc_dai_ops {
int (*set_sdw_stream)(struct snd_soc_dai *dai,
void *stream, int direction);
+ void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
/*
* DAI digital mute - optional.
* Called by soc-core to minimise any pops.
@@ -410,4 +411,13 @@ static inline int snd_soc_dai_set_sdw_stream(struct
snd_soc_dai *dai,
return -ENOTSUPP;
}
+static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai,
+ int direction)
+{
+ if (dai->driver->ops->get_sdw_stream)
+ return dai->driver->ops->get_sdw_stream(dai, direction);
+ else
+ return ERR_PTR(-ENOTSUPP);
+}
Drop redundant else.
Re: [PATCH v2 3/5] ASoC: core: add support to snd_soc_dai_get_sdw_stream()
On 2019-08-13 18:52, Srinivas Kandagatla wrote:
Thanks for the review,
On 13/08/2019 17:03, Cezary Rojewski wrote:
On 2019-08-13 10:35, Srinivas Kandagatla wrote:
On platforms which have smart speaker amplifiers connected via
soundwire and modeled as aux devices in ASoC, in such usecases machine
driver should be able to get sdw master stream from dai so that it can
use the runtime stream to setup slave streams.
soundwire already as a set function, get function would provide more
flexibility to above configurations.
Signed-off-by: Srinivas Kandagatla
---
+static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai,
+ int direction)
+{
+ if (dai->driver->ops->get_sdw_stream)
+ return dai->driver->ops->get_sdw_stream(dai, direction);
+ else
+ return ERR_PTR(-ENOTSUPP);
+}
Drop redundant else.
Not all the dai drivers would implement this function, I guess else is
not redundant here!
--srini
Eh. By that I meant dropping "else" keyword and reducing indentation for
"return ERR_PTR(-ENOTSUPP);"
Czarek
Re: [RFC PATCH 01/40] soundwire: add debugfs support
On 2019-07-26 01:39, Pierre-Louis Bossart wrote:
+static ssize_t sdw_slave_reg_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct sdw_slave *slave = file->private_data;
+ unsigned int reg;
+ char *buf;
+ ssize_t ret;
+ int i, j;
+
+ buf = kzalloc(RD_BUF, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = scnprintf(buf, RD_BUF, "Register Value\n");
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
+
+ for (i = 0; i < 6; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
In most cases explicit reg macro is used, here it's implicit. Align with
the rest?
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+ ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN);
+ for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+ ret += sdw_sprintf(slave, buf, ret,
+ SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET);
+ for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET;
+ i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
I'd advice to revisit macros declarations first.
There should be SDW_DP0_SAMPLECTRL1_B(bank) declared. In general all
macros for SDW should be "bank-less" (name wise). Additionally,
SDW_BANK_OFFSET(bank) could be provided for convenience i.e.: return 0
for bank0.
Yeah, there might be some speed loss in terms of operation count but in
most cases it is negligible.
Would simplify this entire reg dump greatly.
const array on top with {0, 1} elements and replacing explicit "bank0/1"
strings with "bank%d" gets code size reduced while not losing on
readability.
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n");
+ for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+ for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B0);
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B0);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B1);
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B1);
+
+ for (i = 1; i < 14; i++) {
Explicit valid slave addresses would be preferred.
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i);
+ reg = SDW_DPN_INT(i);
+ for (j = 0; j < 6; j++)
+ ret += sdw_sprintf(slave, buf, ret, reg + j);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+ reg = SDW_DPN_CHANNELEN_B0(i);
+ for (j = 0; j < 9; j++)
+ ret += sdw_sprintf(slave, buf, ret, reg + j);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+ reg = SDW_DPN_CHANNELEN_B1(i);
+ for (j = 0; j < 9; j++)
+ ret += sdw_sprintf(slave, buf, ret, reg + j);
Some sort of MAX_CHANNELS would be nice here too.
+ }
+
+ ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+ kfree(buf);
+
+ return ret;
+}
+
+static const struct file_operations sdw_slave_reg_fops = {
+ .open = simple_open,
+ .read = sdw_slave_reg_read,
+ .llseek = default_llseek,
+};
+
+struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{
+ struct dentry *master;
+ struct dentry *d;
+ char name[32];
+
+ master = slave->bus->debugfs;
+
+ /* create the debugfs slave-name */
+ snprintf(name, sizeof(name), "%s", dev_name(&slave->dev));
+ d = debugfs_create_dir(name, master);
+
+ debugfs_create_file("registers", 0400, d, slave, &sdw_slave_reg_fops);
Pointer returned by _create_file gets completely ignored here. At least
dbg msg would be nice if it fails.
+ return d;
+}
+
Re: [RFC PATCH 04/40] soundwire: intel: add debugfs register dump
On 2019-07-26 01:39, Pierre-Louis Bossart wrote:
+static void intel_debugfs_init(struct sdw_intel *sdw)
+{
+ struct dentry *root = sdw->cdns.bus.debugfs;
+
+ if (!root)
+ return;
+
+ sdw->fs = debugfs_create_dir("intel-sdw", root);
+ if (IS_ERR_OR_NULL(sdw->fs)) {
+ dev_err(sdw->cdns.dev, "debugfs root creation failed\n");
+ sdw->fs = NULL;
+ return;
+ }
+
+ debugfs_create_file("intel-registers", 0400, sdw->fs, sdw,
+ &intel_reg_fops);
+
+ sdw_cdns_debugfs_init(&sdw->cdns, sdw->fs);
+}
I believe there should be dummy equivalent of _init and _exit if debugfs
is not enabled (if these are defined already and I've missed it, please
ignore).
+static void intel_debugfs_exit(struct sdw_intel *sdw)
+{
+ debugfs_remove_recursive(sdw->fs);
+}
Re: [RFC PATCH 03/40] soundwire: cadence_master: align debugfs to 8 digits
On 2019-07-26 01:39, Pierre-Louis Bossart wrote:
SQUASHME
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 91e8bacb83e3..9f611a1fff0a 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -234,7 +234,7 @@ static ssize_t cdns_sprintf(struct sdw_cdns *cdns,
char *buf, size_t pos, unsigned int reg)
{
return scnprintf(buf + pos, RD_BUF - pos,
-"%4x\t%4x\n", reg, cdns_readl(cdns, reg));
+"%4x\t%8x\n", reg, cdns_readl(cdns, reg));
}
static ssize_t cdns_reg_read(struct file *file, char __user *user_buf,
Should just be merged together with the introducing commit. Guess it's
posted unintentionally.
Re: [RFC PATCH 06/40] soundwire: intel: prevent possible dereference in hw_params
On 2019-07-26 01:39, Pierre-Louis Bossart wrote:
This should not happen in production systems but we should test for
all callback arguments before invoking the config_stream callback.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 68832e613b1e..497879dd9c0d 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -509,7 +509,7 @@ static int intel_config_stream(struct sdw_intel *sdw,
struct snd_soc_dai *dai,
struct snd_pcm_hw_params *hw_params, int link_id)
{
- if (sdw->res->ops && sdw->res->ops->config_stream)
+ if (sdw->res->ops && sdw->res->ops->config_stream && sdw->res->arg)
return sdw->res->ops->config_stream(sdw->res->arg,
substream, dai, hw_params, link_id);
Hmm, declaring local for sdw->res should prove useful here after
addition of 4th sdw->res dereference.
Re: [RFC PATCH 09/40] soundwire: cadence_master: fix usage of CONFIG_UPDATE
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
/*
* debugfs
*/
@@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns)
*/
int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
{
- int ret;
-
_cdns_enable_interrupt(cdns);
- ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE,
-CDNS_MCP_CONFIG_UPDATE_BIT);
- if (ret < 0)
- dev_err(cdns->dev, "Config update timedout\n");
- return ret;
+ return 0;
}
EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
Rather than ignoring _cdns_enable_interrupt - despite said func always
returning 0 - simply do: return _cnds_enable_interrupt(cdns) and flag
caller with inline.
Afterwards, one can think if such encapsulation is even required -
remove existing sdw_cdns_enable_interrupt and rename _cnds_enable_interrupt?
Re: [RFC PATCH 09/40] soundwire: cadence_master: fix usage of CONFIG_UPDATE
On 2019-07-26 11:53, Cezary Rojewski wrote:
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
/*
* debugfs
*/
@@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns
*cdns)
*/
int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
{
- int ret;
-
_cdns_enable_interrupt(cdns);
- ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE,
- CDNS_MCP_CONFIG_UPDATE_BIT);
- if (ret < 0)
- dev_err(cdns->dev, "Config update timedout\n");
- return ret;
+ return 0;
}
EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
Rather than ignoring _cdns_enable_interrupt - despite said func always
returning 0 - simply do: return _cnds_enable_interrupt(cdns) and flag
caller with inline.
Afterwards, one can think if such encapsulation is even required -
remove existing sdw_cdns_enable_interrupt and rename
_cnds_enable_interrupt?
Nevermind, I see you simplified it in the next patch..
Re: [RFC PATCH 20/40] soundwire: prototypes for suspend/resume
On 2019-07-26 01:40, Pierre-Louis Bossart wrote: Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/cadence_master.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index c0bf6ff00a44..d375bbfead18 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -165,6 +165,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns); void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root); +int sdw_cdns_suspend(struct sdw_cdns *cdns); +bool sdw_cdns_check_resume_status(struct sdw_cdns *cdns); + int sdw_cdns_get_stream(struct sdw_cdns *cdns, struct sdw_cdns_streams *stream, u32 ch, u32 dir); No commit message, guess it's SQUASHME commit and shouldn't be part of overall series.
Re: [RFC PATCH 23/40] soundwire: stream: fix disable sequence
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
- return do_bank_switch(stream);
+ ret = do_bank_switch(stream);
+ if (ret < 0) {
+ dev_err(bus->dev, "Bank switch failed: %d\n", ret);
+ return ret;
+ }
+
+ /* make sure alternate bank (previous current) is also disabled */
+ list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+ bus = m_rt->bus;
+ /* Disable port(s) */
+ ret = sdw_enable_disable_ports(m_rt, false);
+ if (ret < 0) {
+ dev_err(bus->dev, "Disable port(s) failed: %d\n", ret);
+ return ret;
+ }
+ }
+
+ return 0;
}
/**
While not directly connected to this commit, I see that you do:
link_for_each_entry(m_rt, &stream->master_list, stream_node)
quite often in /stream.c code. Helpful macro would prove useful.
Re: [RFC PATCH 24/40] soundwire: cadence_master: use BIOS defaults for frame shape
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
+static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
+{
+ u32 val;
+ int c;
+ int r;
+
+ r = sdw_find_row_index(n_rows);
+ c = sdw_find_col_index(n_cols);
+
+ val = (r << 3) | c;
+
+ return val;
+}
+
/**
* sdw_cdns_init() - Cadence initialization
* @cdns: Cadence instance
@@ -977,7 +990,9 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
/* Set the default frame shape */
- cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, CDNS_DEFAULT_FRAME_SHAPE);
+ val = cdns_set_default_frame_shape(prop->default_row,
+ prop->default_col);
+ cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
/* Set SSP interval to default value */
cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);
Suggestion:
declare "generic" _get_frame_frame(rows, cols) instead and let it do the
bitwise operations for you. Pretty sure this won't be the only place in
code where reg value for frame_shape needs to be prepared.
Said function could be as simple as:
return (row << 3) | cols;
+inline flag
i.e. could be even a macro..
Otherwise modify _set_default_frame_shape to simply:
return (r << 3) | c
without involving additional local val variable (I don't really see the
point for any locals there though).
Re: [RFC PATCH 29/40] soundwire: intel_init: add kernel module parameter to filter out links
On 2019-07-26 01:40, Pierre-Louis Bossart wrote: @@ -83,6 +87,9 @@ static struct sdw_intel_ctx caps = ioread32(res->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP); caps &= GENMASK(2, 0); + dev_dbg(&adev->dev, "SoundWire links: BIOS count %d hardware caps %d\n", + count, caps); + /* Check HW supported vs property value and use min of two */ count = min_t(u8, caps, count); This message does not look like it belongs to current patch - no link_mask dependency whatsoever. There have been couple "informative" patches in your series, maybe schedule it with them instead (as a separate series)?
Re: [RFC PATCH 31/40] soundwire: intel: move shutdown() callback and don't export symbol
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
+void intel_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct sdw_cdns_dma_data *dma;
+
+ dma = snd_soc_dai_get_dma_data(dai, substream);
+ if (!dma)
+ return;
+
+ snd_soc_dai_set_dma_data(dai, substream, NULL);
+ kfree(dma);
+}
Correct me if I'm wrong, but do we really need to _get_dma_ here?
_set_dma_ seems bulletproof, same for kfree.
Re: [RFC PATCH 32/40] soundwire: intel: add helper for initialization
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
Move code to helper for reuse in power management routines
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index c40ab443e723..215dc81cdf73 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -984,6 +984,15 @@ static struct sdw_master_ops sdw_intel_ops = {
.post_bank_switch = intel_post_bank_switch,
};
+static int intel_init(struct sdw_intel *sdw)
+{
+ /* Initialize shim and controller */
+ intel_link_power_up(sdw);
+ intel_shim_init(sdw);
+
+ return sdw_cdns_init(&sdw->cdns);
+}
Why don't we check polling status for _link_power_up? I've already met
slow starting devices in the past. If polling fails and -EAGAIN is
returned, flow of initialization should react appropriately e.g. poll
till MAX_TIMEOUT of some sort -or- collapse.
Re: [RFC PATCH 33/40] soundwire: intel: Add basic power management support
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
+static int intel_resume(struct device *dev)
+{
+ struct sdw_intel *sdw;
+ int ret;
+
+ sdw = dev_get_drvdata(dev);
+
+ ret = intel_init(sdw);
+ if (ret) {
+ dev_err(dev, "%s failed: %d", __func__, ret);
+ return ret;
+ }
+
+ sdw_cdns_enable_interrupt(&sdw->cdns);
+
+ return ret;
+}
+
+#endif
Suggestion: the local declaration + initialization via dev_get_drvdata()
are usually combined.
Given the upstream declaration of _enable_interrupt, it does return
error code/ success. Given current flow, if function gets to
_enable_interrupt call, ret is already set to 0. Returning
sdw_cds_enable_interrupt() directly would both simplify the definition
and prevent status loss.
Re: [RFC PATCH 27/40] soundwire: Add Intel resource management algorithm
On 2019-07-26 01:40, Pierre-Louis Bossart wrote: This algorithm computes bus parameters like clock frequency, frame shape and port transport parameters based on active stream(s) running on the bus. This implementation is optimal for Intel platforms. Developers can also implement their own .compute_params() callback for specific resource management algorithm. Credits: this patch is based on an earlier internal contribution by Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. All hard-coded values were removed from the initial contribution to use BIOS information instead. FIXME: remove checkpatch report WARNING: Reusing the krealloc arg is almost always a bug + group->rates = krealloc(group->rates, Signed-off-by: Pierre-Louis Bossart Could you specify the requirements and limitations for this algorithm? Last year I written calc for Linux based on Windows (please don't burn me here) equivalent though said requirements/ limitiations might have changed and nothing is valid any longer. I remember that some parts of specification overcomplicated the calculator and due to actual, realtime usecases it could be greatly simplified (that's why I mention that my work is probably no longer valid). However, these details would help me in reviewing your implementation and providing suggestions. And yes, "Frame shape calculator" probably suits this better. Though this might be just a preference thingy : )
Re: [RFC PATCH 00/40] soundwire: updates for 5.4
On 2019-07-26 01:39, Pierre-Louis Bossart wrote: The existing upstream code allows for SoundWire devices to be enumerated and managed by the bus, but streaming is not currently supported. Bard Liao, Rander Wang and I did quite a bit of integration/validation work to close this gap and we now have SoundWire streaming + basic power managemement on Intel CometLake and IceLake reference boards. These changes are still preliminary and should not be merged as is, but it's time to start reviews. While the number of patches is quite large, each of the changes is quite small. SOF driver changes will be submitted shortly as well but are still being validated. ClockStop modes and synchronized playback on multiple links are not supported for now and will likely be part of the next cycle (dependencies on codec drivers and multi-cpu DAI support). Acknowledgements: This work would not have been possible without the support of Slawomir Blauciak and Tomasz Lauda on the SOF side, currently being reviewed, see https://github.com/thesofproject/sof/pull/1638 Comments and feedback welcome! Hello Pierre, This patchset is pretty large - I'd suggest dividing next RFC into segments: debugfs, info, power-management, basic flow corrections and frame shape calculator. Some commits have no messages and others lack additional info - tried to provide feedback wherever I could, though, especially for the last one, it would be vital to post additional info so in-depth feedback can be provided. Maybe nothing for calculator will come up, maybe something will. In general I remember it being an essential part of SDW and one where many bugs where found during the initial verification phase. Thanks for your contribution and have a good day! Czarek
Re: [PATCH 06/17] soundwire: cadence_master: use firmware defaults for frame shape
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 5d9729b4d634..89c55e4bb72c 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -48,6 +48,8 @@
#define CDNS_MCP_SSPSTAT 0xC
#define CDNS_MCP_FRAME_SHAPE 0x10
#define CDNS_MCP_FRAME_SHAPE_INIT 0x14
+#define CDNS_MCP_FRAME_SHAPE_COL_MASK GENMASK(2, 0)
+#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET3
#define CDNS_MCP_CONFIG_UPDATE 0x18
#define CDNS_MCP_CONFIG_UPDATE_BITBIT(0)
@@ -175,7 +177,6 @@
/* Driver defaults */
#define CDNS_DEFAULT_CLK_DIVIDER 0
-#define CDNS_DEFAULT_FRAME_SHAPE 0x30
#define CDNS_DEFAULT_SSP_INTERVAL 0x18
#define CDNS_TX_TIMEOUT 2000
@@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
}
EXPORT_SYMBOL(sdw_cdns_pdi_init);
+static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
+{
+ u32 val;
+ int c;
+ int r;
+
+ r = sdw_find_row_index(n_rows);
+ c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
+
+ val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
+
+ return val;
+}
+
Guess this have been said already, but this function could be simplified
- unless you really want to keep explicit variable declaration. Both "c"
and "r" declarations could be merged into single line while "val" is not
needed at all.
One more thing - is AND bitwise op really needed for cols explicitly? We
know all col values upfront - these are static and declared in global
table nearby. Static declaration takes care of "initial range-check". Is
another one necessary?
Moreover, this is a _get_ and certainly not a _set_ type of function.
I'd even consider renaming it to: "cdns_get_frame_shape" as this is
neither a _set_ nor an explicit initial frame shape setter.
It might be even helpful to split two usages:
#define sdw_frame_shape(col_idx, row_idx) \
((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
u32 cdns_get_frame_shape(u16 rows, u16 cols)
{
u16 c, r;
r = sdw_find_row_index(rows);
c = sdw_find_col_index(cols);
return sdw_frame_shape(c, r);
}
The above may even be simplified into one-liner.
Re: [PATCH 09/17] soundwire: stream: remove unnecessary variable initializations
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
@@ -1493,6 +1493,11 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime
*stream)
}
}
+ if (!bus) {
+ pr_err("Configuration error in %s\n", __func__);
+ return -EINVAL;
+ }
+
This should probably be located in separate path - not at all an
initialization removal.
@@ -1573,6 +1578,11 @@ static int _sdw_enable_stream(struct sdw_stream_runtime
*stream)
}
}
+ if (!bus) {
+ pr_err("Configuration error in %s\n", __func__);
+ return -EINVAL;
+ }
+
Same here.
@@ -1639,13 +1650,14 @@ static int _sdw_disable_stream(struct
sdw_stream_runtime *stream)
ret = do_bank_switch(stream);
if (ret < 0) {
- dev_err(bus->dev, "Bank switch failed: %d\n", ret);
+ pr_err("Bank switch failed: %d\n", ret);
return ret;
}
Here too.
I might have missed something though I bet you got my point.
Re: [alsa-devel] [PATCH 06/17] soundwire: cadence_master: use firmware defaults for frame shape
On 2019-08-06 17:36, Pierre-Louis Bossart wrote:
On 8/6/19 10:27 AM, Cezary Rojewski wrote:
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 5d9729b4d634..89c55e4bb72c 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -48,6 +48,8 @@
#define CDNS_MCP_SSPSTAT 0xC
#define CDNS_MCP_FRAME_SHAPE 0x10
#define CDNS_MCP_FRAME_SHAPE_INIT 0x14
+#define CDNS_MCP_FRAME_SHAPE_COL_MASK GENMASK(2, 0)
+#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET 3
#define CDNS_MCP_CONFIG_UPDATE 0x18
#define CDNS_MCP_CONFIG_UPDATE_BIT BIT(0)
@@ -175,7 +177,6 @@
/* Driver defaults */
#define CDNS_DEFAULT_CLK_DIVIDER 0
-#define CDNS_DEFAULT_FRAME_SHAPE 0x30
#define CDNS_DEFAULT_SSP_INTERVAL 0x18
#define CDNS_TX_TIMEOUT 2000
@@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
}
EXPORT_SYMBOL(sdw_cdns_pdi_init);
+static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
+{
+ u32 val;
+ int c;
+ int r;
+
+ r = sdw_find_row_index(n_rows);
+ c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
+
+ val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
+
+ return val;
+}
+
Guess this have been said already, but this function could be
simplified - unless you really want to keep explicit variable
declaration. Both "c" and "r" declarations could be merged into single
line while "val" is not needed at all.
One more thing - is AND bitwise op really needed for cols explicitly?
We know all col values upfront - these are static and declared in
global table nearby. Static declaration takes care of "initial
range-check". Is another one necessary?
Moreover, this is a _get_ and certainly not a _set_ type of function.
I'd even consider renaming it to: "cdns_get_frame_shape" as this is
neither a _set_ nor an explicit initial frame shape setter.
It might be even helpful to split two usages:
#define sdw_frame_shape(col_idx, row_idx) \
((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
u32 cdns_get_frame_shape(u16 rows, u16 cols)
{
u16 c, r;
r = sdw_find_row_index(rows);
c = sdw_find_col_index(cols);
return sdw_frame_shape(c, r);
}
The above may even be simplified into one-liner.
This is a function used once on startup, there is no real need to
simplify further. The separate variables help add debug traces as needed
and keep the code readable while showing how the values are encoded into
a register.
Eh, I've thought it's gonna be exposed to userspace (via uapi) so it can
be fetched by tests or tools.
In such case - if there is a single usage only - guess function is fine
as is.
Re: [PATCH 08/14] ASoC: Intel: Skylake: Properly cleanup on component removal
On 2019-06-05 15:45, Amadeusz Sławiński wrote:
When we remove component we need to reverse things which were done on
init, this consists of topology cleanup, lists cleanup and releasing
firmware.
Currently cleanup handlers are put in wrong places or otherwise missing.
So add proper component cleanup function and perform cleanups in it.
Signed-off-by: Amadeusz Sławiński
---
sound/soc/intel/skylake/skl-pcm.c | 8 ++--
sound/soc/intel/skylake/skl-topology.c | 15 +++
sound/soc/intel/skylake/skl-topology.h | 2 ++
sound/soc/intel/skylake/skl.c | 2 --
4 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-pcm.c
b/sound/soc/intel/skylake/skl-pcm.c
index 44062806fbed..7e8110c15258 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -1459,8 +1459,12 @@ static int skl_platform_soc_probe(struct
snd_soc_component *component)
static void skl_pcm_remove(struct snd_soc_component *component)
{
- /* remove topology */
- snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL);
+ struct hdac_bus *bus = dev_get_drvdata(component->dev);
+ struct skl *skl = bus_to_skl(bus);
+
+ skl_tplg_exit(component, bus);
+
+ skl_debugfs_exit(skl);
}
static const struct snd_soc_component_driver skl_component = {
diff --git a/sound/soc/intel/skylake/skl-topology.c
b/sound/soc/intel/skylake/skl-topology.c
index 44f3b29a7210..3964262109ac 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -3748,3 +3748,18 @@ int skl_tplg_init(struct snd_soc_component *component,
struct hdac_bus *bus)
return 0;
}
+
+void skl_tplg_exit(struct snd_soc_component *component, struct hdac_bus *bus)
+{
+ struct skl *skl = bus_to_skl(bus);
+ struct skl_pipeline *ppl, *tmp;
+
+ if (!list_empty(&skl->ppl_list))
+ list_for_each_entry_safe(ppl, tmp, &skl->ppl_list, node)
+ list_del(&ppl->node);
+
+ /* clean up topology */
+ snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL);
+
+ release_firmware(skl->tplg);
+}
In debugfs cleanup patch:
[PATCH 07/14] ASoC: Intel: Skylake: Add function to cleanup debugfs
interface
you define skl_debugfs_exit separately - its usage is split and present
in this very patch instead. However, for tplg counterpart -
skl_tplg_exit - you've decided to combine both together. Why not
separate tplg cleanup too?
diff --git a/sound/soc/intel/skylake/skl-topology.h
b/sound/soc/intel/skylake/skl-topology.h
index 82282cac9751..7d32c61c73e7 100644
--- a/sound/soc/intel/skylake/skl-topology.h
+++ b/sound/soc/intel/skylake/skl-topology.h
@@ -471,6 +471,8 @@ void skl_tplg_set_be_dmic_config(struct snd_soc_dai *dai,
struct skl_pipe_params *params, int stream);
int skl_tplg_init(struct snd_soc_component *component,
struct hdac_bus *ebus);
+void skl_tplg_exit(struct snd_soc_component *component,
+ struct hdac_bus *bus);
struct skl_module_cfg *skl_tplg_fe_get_cpr_module(
struct snd_soc_dai *dai, int stream);
int skl_tplg_update_pipe_params(struct device *dev,
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 6d6401410250..e4881ff427ea 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -1119,14 +1119,12 @@ static void skl_remove(struct pci_dev *pci)
struct skl *skl = bus_to_skl(bus);
cancel_work_sync(&skl->probe_work);
- release_firmware(skl->tplg);
pm_runtime_get_noresume(&pci->dev);
/* codec removal, invoke bus_device_remove */
snd_hdac_ext_bus_device_remove(bus);
- skl->debugfs = NULL;
skl_platform_unregister(&pci->dev);
skl_free_dsp(skl);
skl_machine_device_unregister(skl);
Re: [PATCH V2 2/2] ASoC: fsl_esai: recover the channel swap after xrun
On 2019-07-03 08:42, shengjiu.w...@nxp.com wrote:
+static void fsl_esai_reset(unsigned long arg)
+{
+ struct fsl_esai *esai_priv = (struct fsl_esai *)arg;
+ u32 saisr, tfcr, rfcr;
+
+ /* save the registers */
+ regmap_read(esai_priv->regmap, REG_ESAI_TFCR, &tfcr);
+ regmap_read(esai_priv->regmap, REG_ESAI_RFCR, &rfcr);
+
+ /* stop the tx & rx */
+ fsl_esai_trigger_stop(esai_priv, 1);
+ fsl_esai_trigger_stop(esai_priv, 0);
+
+ /* reset the esai, and restore the registers */
+ fsl_esai_init(esai_priv);
+
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_TCR,
+ ESAI_xCR_xPR_MASK,
+ ESAI_xCR_xPR);
+ regmap_update_bits(esai_priv->regmap, REG_ESAI_RCR,
+ ESAI_xCR_xPR_MASK,
+ ESAI_xCR_xPR);
+
+ /* restore registers by regcache_sync */
+ fsl_esai_register_restore(esai_priv);
+
Both _init and _restore may fail given their declaration in 1/2 "ASoC:
fsl_esai: Wrap some operations to be functions" yet here you simply
ignore the return values.
If failure of said functions is permissive, it might be a good place for
a comment.
Czarek
Re: [RFC PATCH 1/6] ASoC: core: add support to snd_soc_dai_get_sdw_stream()
On 2019-06-07 10:56, Srinivas Kandagatla wrote:
On platforms which have smart speaker amplifiers connected via
soundwire and modeled as aux devices in ASoC, in such usecases machine
driver should be able to get sdw master stream from dai so that it can
use the runtime stream to setup slave streams.
soundwire already as a set function, get function would provide more
flexibility to above configurations.
Signed-off-by: Srinivas Kandagatla
---
include/sound/soc-dai.h | 10 ++
1 file changed, 10 insertions(+)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index f5d70041108f..9f90b936fd9a 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -177,6 +177,7 @@ struct snd_soc_dai_ops {
int (*set_sdw_stream)(struct snd_soc_dai *dai,
void *stream, int direction);
+ void *(*get_sdw_stream)(struct snd_soc_dai *dai, int direction);
/*
* DAI digital mute - optional.
* Called by soc-core to minimise any pops.
@@ -385,4 +386,13 @@ static inline int snd_soc_dai_set_sdw_stream(struct
snd_soc_dai *dai,
return -ENOTSUPP;
}
+static inline void *snd_soc_dai_get_sdw_stream(struct snd_soc_dai *dai, int direction)
Exceeds character limit?
+{
+ if (dai->driver->ops->get_sdw_stream)
+ return dai->driver->ops->get_sdw_stream(dai, direction);
+ else
+ return NULL;
set_ equivalent returns -ENOTSUPP instead.
ERR_PTR seems to make more sense here.
+
Unnecessary newline.
+}
+
#endif
Re: [alsa-devel] [RFC PATCH 3/6] soundwire: core: define SDW_MAX_PORT
On 2019-06-07 14:31, Pierre-Louis Bossart wrote: On 6/7/19 3:56 AM, Srinivas Kandagatla wrote: This patch adds SDW_MAX_PORT so that other driver can use it. Signed-off-by: Srinivas Kandagatla --- include/linux/soundwire/sdw.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index aac68e879fae..80ca997e4e5d 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -36,6 +36,7 @@ struct sdw_slave; #define SDW_FRAME_CTRL_BITS 48 #define SDW_MAX_DEVICES 11 +#define SDW_MAX_PORTS 14 That's an ambiguous definition. You can have 16 ports per the SoundWire spec, but DP0 is reserved for control and DP15 is an alias for all ports (same idea as device 15 for broadcast operations but limited to a single device), which leaves 14 ports for audio usages. In the MIPI specs, specifically the DisCo part, the difference is called about with the the DP0 and DPn notations, so this could be SDW_MAX_DPn. Alternatively you could use SDW_MAX_AUDIO_PORTS which is more self-explanatory and does not require in-depth familiarity with the spec. This ambiguity spreads even further. Look at the name of #define below. DP0 is by no means invalid. It's specific and has some optional registers, yes, but that's because of its engagement in BPT. Given the fact SDW does not care about type of data being transported, even "AUDIO" seems misleading here. Though it's still better than no specifier at all. #define SDW_VALID_PORT_RANGE(n) ((n) <= 14 && (n) >= 1) #define SDW_DAI_ID_RANGE_START 100
Re: [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller
On 2019-06-07 10:56, Srinivas Kandagatla wrote: Qualcomm SoundWire Master controller is present in most Qualcomm SoCs either integrated as part of WCD audio codecs via slimbus or as part of SOC I/O. This patchset adds support to a very basic controller which has been tested with WCD934x SoundWire controller connected to WSA881x smart speaker amplifiers. Signed-off-by: Srinivas Kandagatla --- drivers/soundwire/Kconfig | 9 + drivers/soundwire/Makefile | 4 + drivers/soundwire/qcom.c | 983 + 3 files changed, 996 insertions(+) create mode 100644 drivers/soundwire/qcom.c diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig index 53b55b79c4af..f44d4f36dbbb 100644 --- a/drivers/soundwire/Kconfig +++ b/drivers/soundwire/Kconfig @@ -34,4 +34,13 @@ config SOUNDWIRE_INTEL enable this config option to get the SoundWire support for that device. +config SOUNDWIRE_QCOM + tristate "Qualcomm SoundWire Master driver" + select SOUNDWIRE_BUS + depends on SND_SOC + help + SoundWire Qualcomm Master driver. + If you have an Qualcomm platform which has a SoundWire Master then + enable this config option to get the SoundWire support for that + device endif diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index 5817beaca0e1..f4ebfde31372 100644 --- a/drivers/soundwire/Makefile +++ b/drivers/soundwire/Makefile @@ -16,3 +16,7 @@ obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel.o soundwire-intel-init-objs := intel_init.o obj-$(CONFIG_SOUNDWIRE_INTEL) += soundwire-intel-init.o + +#Qualcomm driver +soundwire-qcom-objs := qcom.o +obj-$(CONFIG_SOUNDWIRE_QCOM) += soundwire-qcom.o diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c new file mode 100644 index ..d1722d44d217 --- /dev/null +++ b/drivers/soundwire/qcom.c @@ -0,0 +1,983 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2019, Linaro Limited + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "bus.h" + Pierre already pointed this out - SWR looks odd. During my time with Soundwire I've met SDW and SNDW, but it's the first time I see SWR. +#define SWRM_COMP_HW_VERSION 0x00 +#define SWRM_COMP_CFG_ADDR 0x04 +#define SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK BIT(1) +#define SWRM_COMP_CFG_ENABLE_MSK BIT(0) +#define SWRM_COMP_PARAMS 0x100 +#define SWRM_COMP_PARAMS_DOUT_PORTS_MASK GENMASK(4, 0) +#define SWRM_COMP_PARAMS_DIN_PORTS_MASK GENMASK(9, 5) +#define SWRM_COMP_PARAMS_WR_FIFO_DEPTH GENMASK(14, 10) +#define SWRM_COMP_PARAMS_RD_FIFO_DEPTH GENMASK(19, 15) +#define SWRM_COMP_PARAMS_AUTO_ENUM_SLAVES GENMASK(32. 20) +#define SWRM_INTERRUPT_STATUS 0x200 +#define SWRM_INTERRUPT_STATUS_RMSK GENMASK(16, 0) +#define SWRM_INTERRUPT_STATUS_SLAVE_PEND_IRQ BIT(0) +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED BIT(1) +#define SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS BIT(2) +#define SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET BIT(3) +#define SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW BIT(4) +#define SWRM_INTERRUPT_STATUS_RD_FIFO_UNDERFLOWBIT(5) +#define SWRM_INTERRUPT_STATUS_WR_CMD_FIFO_OVERFLOW BIT(6) +#define SWRM_INTERRUPT_STATUS_CMD_ERRORBIT(7) +#define SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION BIT(8) +#define SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCHBIT(9) +#define SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED BIT(10) +#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED BIT(11) +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED BIT(12) +#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL BIT(13) +#define SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED BIT(14) +#define SWRM_INTERRUPT_STATUS_CLK_STOP_FINISHEDBIT(15) +#define SWRM_INTERRUPT_STATUS_ERROR_PORT_TEST BIT(16) +#define SWRM_INTERRUPT_MASK_ADDR 0x204 +#define SWRM_INTERRUPT_CLEAR 0x208 You seem to shortcut every reg here similarly to how it's done in SDW spec. INTERRUPT is represented by INT there, and by doing so, this define block would look more like a real family. +#define SWRM_CMD_FIFO_WR_CMD 0x300 +#define SWRM_CMD_FIFO_RD_CMD 0x304 +#define SWRM_CMD_FIFO_CMD
Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
On 2019-07-19 09:09, Oleksandr Suvorov wrote:
On Thu, 18 Jul 2019 at 21:49, Cezary Rojewski wrote:
On 2019-07-18 20:42, Cezary Rojewski wrote:
On 2019-07-18 11:02, Oleksandr Suvorov wrote:
+enum {
+HP_POWER_EVENT,
+DAC_POWER_EVENT,
+ADC_POWER_EVENT,
+LAST_POWER_EVENT
+};
+
+static u16 mute_mask[] = {
+SGTL5000_HP_MUTE,
+SGTL5000_OUTPUTS_MUTE,
+SGTL5000_OUTPUTS_MUTE
+};
If mute_mask[] is only used within common handler, you may consider
declaring const array within said handler instead (did not check that
myself).
Otherwise, simple comment for the second _OUTPUTS_MUTE should suffice -
its not self explanatory why you doubled that mask.
Ok, I'll add a comment to explain doubled mask.
+
/* sgtl5000 private structure in codec */
struct sgtl5000_priv {
int sysclk;/* sysclk rate */
@@ -137,8 +157,109 @@ struct sgtl5000_priv {
u8 micbias_voltage;
u8 lrclk_strength;
u8 sclk_strength;
+u16 mute_state[LAST_POWER_EVENT];
};
When I spoke of LAST enum constant, I did not really had this specific
usage in mind.
From design perspective, _LAST_ does not exist and should never be
referred to as "the next option" i.e.: new enum constant.
By its nature, LAST_POWER_EVENT is actually a size of the array, but I
couldn't come up with a better name.
That is way preferred usage is:
u16 mute_state[ADC_POWER_EVENT+1;
-or-
u16 mute_state[LAST_POWER_EVENT+1];
Maybe I'm just being radical here :)
Maybe :) I don't like first variant (ADC_POWER_EVENT+1): somewhen in
future, someone can add a new event to this enum and we've got a
possible situation with "out of array indexing".
Czarek
Forgive me for double posting. Comment above is targeted towards:
>> +enum {
>> +HP_POWER_EVENT,
>> +DAC_POWER_EVENT,
>> +ADC_POWER_EVENT,
>> +LAST_POWER_EVENT
>> +};
as LAST_POWER_EVENT is not assigned explicitly to ADC_POWER_EVENT and
thus generates implicit "new option" of value 3.
So will you be happy with the following variant?
...
ADC_POWER_EVENT,
LAST_POWER_EVENT = ADC_POWER_EVENT,
...
u16 mute_state[LAST_POWER_EVENT+1];
...
It's not about being happy - I'm a happy man in general ;p
As stated already, declaring _LAST_ as the "new option" is misleading
and not advised.
And yeah, [_LAST_ + 1] is usually the one you should go with.
Czarek
Re: [PATCH v6 2/6] ASoC: sgtl5000: Improve VAG power and mute control
On 2019-07-19 12:05, Oleksandr Suvorov wrote:
VAG power control is improved to fit the manual [1]. This patch fixes as
minimum one bug: if customer muxes Headphone to Line-In right after boot,
the VAG power remains off that leads to poor sound quality from line-in.
I.e. after boot:
- Connect sound source to Line-In jack;
- Connect headphone to HP jack;
- Run following commands:
$ amixer set 'Headphone' 80%
$ amixer set 'Headphone Mux' LINE_IN
Change VAG power on/off control according to the following algorithm:
- turn VAG power ON on the 1st incoming event.
- keep it ON if there is any active VAG consumer (ADC/DAC/HP/Line-In).
- turn VAG power OFF when there is the latest consumer's pre-down event
come.
- always delay after VAG power OFF to avoid pop.
- delay after VAG power ON if the initiative consumer is Line-In, this
prevents pop during line-in muxing.
According to the data sheet [1], to avoid any pops/clicks,
the outputs should be muted during input/output
routing changes.
[1] https://www.nxp.com/docs/en/data-sheet/SGTL5000.pdf
Cc: sta...@vger.kernel.org
Fixes: 9b34e6cc3bc2 ("ASoC: Add Freescale SGTL5000 codec support")
Signed-off-by: Oleksandr Suvorov
Reviewed-by: Marcel Ziswiler
Reviewed-by: Fabio Estevam
---
Changes in v6:
- Code optimization
You went crazy with that description (u16 mute_mask[]) :)
Reviewed-by: Cezary Rojewski
Re: [PATCH V2] ASoC: Intel: boards: Use FS as nau8825 sysclk in nau88125_* machine
2) Currently Skylake does not output MCLK/FS when the back-end DAI op hw_param is called, so we cannot switch to MCLK/FS in hw_param. This patch reduces pop by letting nau8825 keep using its internal VCO clock during widget power up sequence, until SNDRV_PCM_TRIGGER_START when MCLK/FS is available. Once device resumes, the system will only enable power sequence for playback without doing hardware parameter, audio format, and PLL configure. In the mean time, the jack detecion sequence has changed PLL parameters and switched to internal clock. Thus, the playback signal distorted without correct PLL parameters. That is why we need to configure the PLL again in SNDRV_PCM_TRIGGER_RESUME case. IIRC the FS can be controlled with the clk_ api with the Skylake driver, as done for some KBL platforms. Or is this not supported by the firmware used by this machine? According to Ben, SKL had limitations in FW for managing the clk's back in the days. Can you point to the other driver you mention so we can cross check? Skylake driver is found within: /sound/soc/intel/skylake directory. "SKL had limitations in FW" - that's misleading. This is neither FW issue nor HW 'limitation'. SKL is an older platform and its goals and design was different than say APL+. Basically, your needs do not align with what's present on SKL hw. Czarek
Re: [PATCH v1 3/4] ASoC: tda7802: Add enable device attribute
On 2019-06-11 19:49, Thomas Preston wrote:
Add a device attribute to control the enable regulator. Write 1 to
enable, 0 to disable (ref-count minus one), or -1 to force disable the
physical pin.
To disable a set of amplifiers wired to the same enable gpio, each
device must be disabled. For example:
echo 0 > /sys/devices/.../device:00/enable
echo 0 > /sys/devices/.../device:01/enable
In an emergency, we can force disable from any device:
echo -1 > /sys/devices/.../device:00/enable
Signed-off-by: Thomas Preston
Cc: Patrick Glaser
Cc: Rob Duncan
Cc: Nate Case
---
sound/soc/codecs/tda7802.c | 65 +-
1 file changed, 64 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
index 38ca52de85f0..62aae011d9f1 100644
--- a/sound/soc/codecs/tda7802.c
+++ b/sound/soc/codecs/tda7802.c
@@ -458,6 +458,42 @@ static struct snd_soc_dai_driver tda7802_dai_driver = {
.ops = &tda7802_dai_ops,
};
+static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct tda7802_priv *tda7802 = dev_get_drvdata(dev);
+ int enabled = regulator_is_enabled(tda7802->enable_reg) ? 1 : 0;
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", enabled);
+}
+
+static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct tda7802_priv *tda7802 = dev_get_drvdata(dev);
+ int err;
+
+ if (sysfs_streq(buf, "1")) {
+ err = regulator_enable(tda7802->enable_reg);
+ if (err < 0)
+ dev_err(dev, "Could not enable (sysfs)\n");
+ } else if (sysfs_streq(buf, "0")) {
+ err = regulator_disable(tda7802->enable_reg);
+ if (err < 0)
+ dev_err(dev, "Could not disable (sysfs)\n");
+ } else if (sysfs_streq(buf, "-1")) {
+ err = regulator_force_disable(tda7802->enable_reg);
+ if (err < 0)
+ dev_err(dev, "Could not force disable (sysfs)\n");
+ } else {
+ return -EINVAL;
+ }
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(enable);
+
/* read device tree or ACPI properties from device */
static int tda7802_read_properties(struct tda7802_priv *tda7802)
{
@@ -493,7 +529,34 @@ static int tda7802_read_properties(struct tda7802_priv
*tda7802)
return err;
}
-static const struct snd_soc_component_driver tda7802_component_driver;
+static int tda7802_probe(struct snd_soc_component *component)
+{
+ struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
+ struct device *dev = &tda7802->i2c->dev;
+ int err;
+
+ dev_dbg(dev, "%s\n", __func__);
Function name alone ain't very informational. Is this intended?
+
+ err = device_create_file(dev, &dev_attr_enable);
+ if (err < 0) {
+ dev_err(dev, "Could not create enable attr\n");
+ return err;
+ }
Regardless of outcome, you'll be returning err here. Consider leaving
error message alone within if-statement. Remove redundant brackets if
you decide to do so.
+
+ return err;
+}
+
+static void tda7802_remove(struct snd_soc_component *component)
+{
+ struct tda7802_priv *tda7802 = snd_soc_component_get_drvdata(component);
+
+ device_remove_file(&tda7802->i2c->dev, &dev_attr_enable);
+}
+
+static const struct snd_soc_component_driver tda7802_component_driver = {
+ .probe = tda7802_probe,
+ .remove = tda7802_remove,
+};
static int tda7802_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
Re: [PATCH v1 4/4] ASoC: tda7802: Add speaker-test sysfs
On 2019-06-11 19:49, Thomas Preston wrote:
Add speaker_test device attribute. When the speaker-test node is read
the hardware speaker test is started.
$ cat /sys/devices/.../device:00/speaker_test
04 04 04 04
Signed-off-by: Thomas Preston
Cc: Patrick Glaser
Cc: Rob Duncan
Cc: Nate Case
---
sound/soc/codecs/tda7802.c | 172 +
1 file changed, 172 insertions(+)
diff --git a/sound/soc/codecs/tda7802.c b/sound/soc/codecs/tda7802.c
index 62aae011d9f1..edfa752c0c9f 100644
--- a/sound/soc/codecs/tda7802.c
+++ b/sound/soc/codecs/tda7802.c
@@ -157,6 +157,7 @@ static const u8 IB3_FORMAT[4][4] = {
#define DUMP_NUM_REGS(DUMP_NUM_BLOCK * 2)
struct tda7802_priv {
+ struct mutex mutex;
struct i2c_client *i2c;
struct regmap *regmap;
struct regulator *enable_reg;
@@ -458,6 +459,159 @@ static struct snd_soc_dai_driver tda7802_dai_driver = {
.ops = &tda7802_dai_ops,
};
+/* The speaker test is triggered by reading a sysfs attribute file attached to
+ * the I2C device. The user space thread that's doing the reading is blocked
+ * until the test completes (or times out). We only permit one test to be in
+ * progress at a time.
+ */
+
+static int speaker_test_start(struct regmap *regmap)
+{
+ int err;
+
+ err = regmap_update_bits(regmap, TDA7802_IB5, IB5_AMPLIFIER_ON, 0);
+ if (err < 0) {
+ dev_err(regmap_get_device(regmap),
+ "Cannot disable amp for speaker test (%d)\n", err);
+ return err;
+ }
+
+ err = regmap_update_bits(regmap, TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0);
+ if (err < 0) {
+ dev_err(regmap_get_device(regmap),
+ "Cannot disable diag mode before speaker test (%d)\n",
+ err);
+ return err;
+ }
+
+ /* Seem to need at least a 15 ms delay here before the rising
+* edge. Otherwise the diagnostics never complete (perhaps
+* they never start).
+*/
+ msleep(20);
+
+ err = regmap_update_bits(regmap, TDA7802_IB4,
+IB4_DIAG_MODE_ENABLE, IB4_DIAG_MODE_ENABLE);
+ if (err < 0) {
+ dev_err(regmap_get_device(regmap),
+ "Cannot enable diag mode for speaker test (%d)\n", err);
+ return err;
+ }
+
+ return 0;
You may want to follow path set by speaker_test_stop: replace "return 0"
with "return err" and have only error msg found within preceding
if-statement.
+}
+
+static int speaker_test_stop(struct regmap *regmap)
+{
+ int err;
+
+ err = regmap_update_bits(regmap, TDA7802_IB4, IB4_DIAG_MODE_ENABLE, 0);
+ if (err < 0)
+ dev_err(regmap_get_device(regmap),
+ "Cannot disable diag mode after speaker test (%d)\n",
+ err);
+ return err;
+}
+
+/* We poll the test completion bit, letting the current thread sleep
+ * until we're done. These values are not critical.
+ */
+#define SPEAKER_TEST_DONE_POLL_PERIOD_US 5000
+#define SPEAKER_TEST_DONE_TIMEOUT_US500
+
+static int speaker_test_check(struct tda7802_priv *tda7802,
+ unsigned int *status)
+{
+ struct regmap *regmap = tda7802->regmap;
+ struct device *dev = &tda7802->i2c->dev;
+ int reg_err = 0, err = 0, i, amp_on;
Both reg_err and err are initialized unnecessarily.
+ unsigned int val;
+
+ reg_err = regulator_enable(tda7802->enable_reg);
+ if (reg_err < 0) {
+ dev_err(dev, "Could not enable (speaker-test).\n");
+ return reg_err;
+ }
+ msleep(ENABLE_DELAY_MS);
+
+ /* we should return amp on state when speaker-test is done */
+ err = regmap_read(regmap, TDA7802_IB5, &_on);
+ if (err < 0) {
+ dev_err(dev, "Could not read amp on state (speaker-test)\n");
+ goto speaker_test_disable;
+ }
+
+ for (i = 0; i < CHANNELS_PER_AMP; ++i)
+ status[i] = 0xff;
+
+ err = speaker_test_start(regmap);
+ if (err < 0)
+ goto speaker_test_restore;
+
+ /* Wait until DB0_STARTUP_DIAG_STATUS is set */
+ err = regmap_read_poll_timeout(regmap, TDA7802_DB0, val,
+ val & DB0_STARTUP_DIAG_STATUS,
+ SPEAKER_TEST_DONE_POLL_PERIOD_US,
+ SPEAKER_TEST_DONE_TIMEOUT_US);
+ if (err < 0) {
+ if (err == -ENODATA)
+ dev_err(dev,
+ "Speaker diagnostic test did not complete\n");
+ speaker_test_stop(regmap);
+ goto speaker_test_restore;
+ }
+
+ err = speaker_test_stop(regmap);
+ if (err < 0)
+ goto speaker_test_restore;
This sequence looks weird and as if it could be simplified but I might
be just plain
Re: [PATCH v7 3/4] ASoC: rt5677: clear interrupts by polarity flip
On 2019-06-14 21:48, Fletcher Woodruff wrote:
From: Ben Zhang
The rt5677 jack detection function has a requirement that the polarity
of an interrupt be flipped after it fires in order to clear the
interrupt.
This patch implements an irq_chip with irq_domain directly instead of
using regmap-irq, so that interrupt source line polarities can be
flipped in the irq handler.
The reason that this patch does not add this feature within regmap-irq
is that future patches will add hotword detection support to this irq
handler. Those patches will require adding additional logic that would
not make sense to have in regmap-irq.
Signed-off-by: Ben Zhang
Signed-off-by: Fletcher Woodruff
---
sound/soc/codecs/rt5677.c | 170 ++
sound/soc/codecs/rt5677.h | 7 +-
2 files changed, 143 insertions(+), 34 deletions(-)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 87a92ba0d040b7..87466ee222ee59 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -23,6 +23,10 @@
#include
#include
#include
+#include
+#include
+#include
+#include
#include
#include
#include
@@ -4620,7 +4624,6 @@ static void rt5677_gpio_config(struct rt5677_priv
*rt5677, unsigned offset,
static int rt5677_to_irq(struct gpio_chip *chip, unsigned offset)
{
struct rt5677_priv *rt5677 = gpiochip_get_data(chip);
- struct regmap_irq_chip_data *data = rt5677->irq_data;
int irq;
if ((rt5677->pdata.jd1_gpio == 1 && offset == RT5677_GPIO1) ||
@@ -4646,7 +4649,7 @@ static int rt5677_to_irq(struct gpio_chip *chip, unsigned
offset)
return -ENXIO;
}
- return regmap_irq_get_virq(data, irq);
+ return irq_create_mapping(rt5677->domain, irq);
}
static const struct gpio_chip rt5677_template_chip = {
@@ -5042,30 +5045,130 @@ static void rt5677_read_device_properties(struct
rt5677_priv *rt5677,
rt5677->pdata.jd3_gpio = val;
}
-static struct regmap_irq rt5677_irqs[] = {
+struct rt5677_irq_desc {
+ unsigned int enable_mask;
+ unsigned int status_mask;
+ unsigned int polarity_mask;
+};
+
+static const struct rt5677_irq_desc rt5677_irq_descs[] = {
[RT5677_IRQ_JD1] = {
- .reg_offset = 0,
- .mask = RT5677_EN_IRQ_GPIO_JD1,
+ .enable_mask = RT5677_EN_IRQ_GPIO_JD1,
+ .status_mask = RT5677_STA_GPIO_JD1,
+ .polarity_mask = RT5677_INV_GPIO_JD1,
},
[RT5677_IRQ_JD2] = {
- .reg_offset = 0,
- .mask = RT5677_EN_IRQ_GPIO_JD2,
+ .enable_mask = RT5677_EN_IRQ_GPIO_JD2,
+ .status_mask = RT5677_STA_GPIO_JD2,
+ .polarity_mask = RT5677_INV_GPIO_JD2,
},
[RT5677_IRQ_JD3] = {
- .reg_offset = 0,
- .mask = RT5677_EN_IRQ_GPIO_JD3,
+ .enable_mask = RT5677_EN_IRQ_GPIO_JD3,
+ .status_mask = RT5677_STA_GPIO_JD3,
+ .polarity_mask = RT5677_INV_GPIO_JD3,
},
};
-static struct regmap_irq_chip rt5677_irq_chip = {
- .name = RT5677_DRV_NAME,
- .irqs = rt5677_irqs,
- .num_irqs = ARRAY_SIZE(rt5677_irqs),
+static irqreturn_t rt5677_irq(int unused, void *data)
+{
+ struct rt5677_priv *rt5677 = data;
+ int ret = 0, i, reg_irq, virq;
+ bool irq_fired = false;
+
+ mutex_lock(&rt5677->irq_lock);
+ /* Read interrupt status */
+ ret = regmap_read(rt5677->regmap, RT5677_IRQ_CTRL1, ®_irq);
+ if (ret) {
+ pr_err("rt5677: failed reading IRQ status: %d\n", ret);
The entire rt5677 makes use of dev_XXX with the exception of.. this very
function. Consider reusing "component" field which is already part of
struct rt5677_priv and removing pr_XXX.
+ goto exit;
+ }
+
+ for (i = 0; i < RT5677_IRQ_NUM; i++) {
+ if (reg_irq & rt5677_irq_descs[i].status_mask) {
+ irq_fired = true;
+ virq = irq_find_mapping(rt5677->domain, i);
+ if (virq)
+ handle_nested_irq(virq);
+
+ /* Clear the interrupt by flipping the polarity of the
+* interrupt source line that fired
+*/
+ reg_irq ^= rt5677_irq_descs[i].polarity_mask;
+ }
+ }
+
+ if (!irq_fired)
+ goto exit;
+
+ ret = regmap_write(rt5677->regmap, RT5677_IRQ_CTRL1, reg_irq);
+ if (ret) {
+ pr_err("rt5677: failed updating IRQ status: %d\n", ret);
Same here.
+ goto exit;
+ }
+exit:
+ mutex_unlock(&rt5677->irq_lock);
+ if (irq_fired)
+ return IRQ_HANDLED;
+ else
+ return IRQ_NONE;
+}
- .num_regs = 1,
- .status_base = RT5677_IRQ_CTRL1,
- .mask_base = RT5677_IRQ_CTRL1,
-
Re: [PATCH RESEND] ASoC: Intel: Skylake: Replace zero-length array with flexible-array
On 2020-05-11 7:46 PM, Gustavo A. R. Silva wrote:
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
sizeof(flexible-array-member) triggers a warning because flexible array
members have incomplete type[1]. There are some instances of code in
which the sizeof operator is being incorrectly/erroneously applied to
zero-length arrays and the result is zero. Such instances may be hiding
some bugs. So, this work (flexible-array member conversions) will also
help to get completely rid of those sorts of issues.
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
sound/soc/intel/atom/sst-atom-controls.h |2 +-
sound/soc/intel/skylake/skl-i2s.h|2 +-
sound/soc/intel/skylake/skl-topology.h |4 ++--
sound/soc/intel/skylake/skl.h|2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
Acked-by: Cezary Rojewski
Thanks,
Czarek
