Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 4/16/21 1:55 PM, Mark Brown wrote: On Fri, Apr 16, 2021 at 11:47:01AM -0500, Pierre-Louis Bossart wrote: On 4/16/21 11:31 AM, Mark Brown wrote: Not really written down that I can think of. I think the next steps that I can think of right now are unfortunately bigger and harder ones, mainly working out a way to represent digital configuration as a graph that can be attached to/run in parallel with DAPM other people might have some better ideas though. Sorry, I appreciate that this isn't super helpful :/ I see a need for this in our future SoundWire/SDCA work. So far I was planning to model the entities as 'widgets' and lets DAPM propagate activation information for power management, however there are also bits of information in 'Clusters' (number of channels and spatial relationships) that could change dynamically and would be interesting to propagate across entities, so that when we get a stream of data on the bus we know what it is. Yes, I was thinking along similar lines last time I looked at it - I was using the term digital domains. You'd need some impedence matching objects for things like SRCs, and the ability to flag which configuration matters within a domain (eg, a lot of things will covert to the maximum supported bit width for internal operation so bit width only matters on external interfaces) but I think for a first pass we can get away with forcing everything other than what DPCM has as front ends into static configurations. You lost me on the last sentence. did you mean "forcing everything into static configurations except for what DPCM has as front-ends"? It may already be too late for static configurations, Intel, NXP and others have started to enable cases where the dailink configuration varies. FWIW both the USB and SDCA class document are very careful with the notion of constraints and whether an entity is implemented in the analog or digital domains. There are 'clock sources' that may be used in specific terminals but no notion of explicit SRC in the graph to leave implementers a lot of freedom. There is a notion of 'Usage' that describes e.g. FullBand or WideBand without defining what the representation is. The bit width is also not described except where necessary (history buffers and external bus-facing interfaces). Like you said it's mostly the boundaries of the domains that matter.
Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 4/16/21 11:31 AM, Mark Brown wrote: On Fri, Apr 16, 2021 at 04:03:05PM +, codrin.ciubota...@microchip.com wrote: Thank you for the links! So basically the machine driver disappears and all the components will be visible in user-space. Not entirely - you still need something to say how they're wired together but it'll be a *lot* simpler for anything that currently used DPCM. If there is a list with the 'steps' or tasks to achieve this? I can try to pitch in. Not really written down that I can think of. I think the next steps that I can think of right now are unfortunately bigger and harder ones, mainly working out a way to represent digital configuration as a graph that can be attached to/run in parallel with DAPM other people might have some better ideas though. Sorry, I appreciate that this isn't super helpful :/ I see a need for this in our future SoundWire/SDCA work. So far I was planning to model the entities as 'widgets' and lets DAPM propagate activation information for power management, however there are also bits of information in 'Clusters' (number of channels and spatial relationships) that could change dynamically and would be interesting to propagate across entities, so that when we get a stream of data on the bus we know what it is. when we discussed the multi-configuration support for BT offload, it also became apparent that we don't fully control the sample rate changes between FE and BE, we only control the start and ends. I fully agree that the division between front- and back-ends is becoming limiting and DPCM is not only complicated but difficult to stretch further.
Re: [PATCH v1] ASoC: Intel: kbl_da7219_max98927: Fix kabylake_ssp_fixup function
On 4/15/21 7:43 AM, Lukasz Majczak wrote:
kabylake_ssp_fixup function uses snd_soc_dpcm to identify the
codecs DAIs. The HW parameters are changed based on the codec DAI of the
stream. The earlier approach to get snd_soc_dpcm was using container_of()
macro on snd_pcm_hw_params.
The structures have been modified over time and snd_soc_dpcm does not have
snd_pcm_hw_params as a reference but as a copy. This causes the current
driver to crash when used.
This patch changes the way snd_soc_dpcm is extracted. snd_soc_pcm_runtime
holds 2 dpcm instances (one for playback and one for capture). 2 codecs
on the SSP are dmic (capture) and speakers (playback). Based on the
stream direction, snd_soc_dpcm is extracted from snd_soc_pcm_runtime.
Tested for all use cases of the driver.
Based on similar fix in kbl_rt5663_rt5514_max98927.c
from Harsha Priya and
Vamshi Krishna Gopal
Cc: # 5.4+
Signed-off-by: Lukasz Majczak
---
Hi,
This is basically a cherry-pick of this change:
https://patchwork.kernel.org/project/alsa-devel/patch/1595432147-11166-1-git-send-email-harshapriy...@intel.com/
just applied to the kbl_da7219_max98927.
Best regards,
Lukasz
Acked-by: Pierre-Louis Bossart
sound/soc/intel/boards/kbl_da7219_max98927.c | 38 +++-
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c
b/sound/soc/intel/boards/kbl_da7219_max98927.c
index 9dfe5bd9180d..4b7b4a044f81 100644
--- a/sound/soc/intel/boards/kbl_da7219_max98927.c
+++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
@@ -284,11 +284,33 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime
*rtd,
struct snd_interval *chan = hw_param_interval(params,
SNDRV_PCM_HW_PARAM_CHANNELS);
struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
- struct snd_soc_dpcm *dpcm = container_of(
- params, struct snd_soc_dpcm, hw_params);
- struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link;
- struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;
+ struct snd_soc_dpcm *dpcm, *rtd_dpcm = NULL;
+ /*
+* The following loop will be called only for playback stream
+* In this platform, there is only one playback device on every SSP
+*/
+ for_each_dpcm_fe(rtd, SNDRV_PCM_STREAM_PLAYBACK, dpcm) {
+ rtd_dpcm = dpcm;
+ break;
+ }
+
+ /*
+* This following loop will be called only for capture stream
+* In this platform, there is only one capture device on every SSP
+*/
+ for_each_dpcm_fe(rtd, SNDRV_PCM_STREAM_CAPTURE, dpcm) {
+ rtd_dpcm = dpcm;
+ break;
+ }
+
+ if (!rtd_dpcm)
+ return -EINVAL;
+
+ /*
+* The above 2 loops are mutually exclusive based on the stream
direction,
+* thus rtd_dpcm variable will never be overwritten
+*/
/*
* Topology for kblda7219m98373 & kblmax98373 supports only S24_LE,
* where as kblda7219m98927 & kblmax98927 supports S16_LE by default.
@@ -311,9 +333,9 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime
*rtd,
/*
* The ADSP will convert the FE rate to 48k, stereo, 24 bit
*/
- if (!strcmp(fe_dai_link->name, "Kbl Audio Port") ||
- !strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") ||
- !strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) {
+ if (!strcmp(rtd_dpcm->fe->dai_link->name, "Kbl Audio Port") ||
+ !strcmp(rtd_dpcm->fe->dai_link->name, "Kbl Audio Headset Playback")
||
+ !strcmp(rtd_dpcm->fe->dai_link->name, "Kbl Audio Capture Port")) {
rate->min = rate->max = 48000;
chan->min = chan->max = 2;
snd_mask_none(fmt);
@@ -324,7 +346,7 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime
*rtd,
* The speaker on the SSP0 supports S16_LE and not S24_LE.
* thus changing the mask here
*/
- if (!strcmp(be_dai_link->name, "SSP0-Codec"))
+ if (!strcmp(rtd_dpcm->be->dai_link->name, "SSP0-Codec"))
snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
return 0;
Re: [PATCH v2 2/3] soundwire: Intel: introduce DMI quirks for HP Spectre x360 Convertible
On 4/13/21 11:08 PM, Vinod Koul wrote: On 12-04-21, 14:37, Dave Hansen wrote: On 3/1/21 11:51 PM, Bard Liao wrote: +++ b/drivers/soundwire/dmi-quirks.c @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2021 Intel Corporation. It looks like this is already in intel-next, so this may be moot. But, is there a specific reason this is dual licensed? If so, can you please include information about the license choice in the cover letter of any future version? The soundwire module from Intel and core soundwire core was always dual licensed, so it kind of followed that.. If there is no specific reason for this contribution to be dual licensed, please make it GPL-2.0 only. This module, I would say NO. Unless someone from Intel disagree.. Pierre/Bard..? If all agree I dont see a reason why this cant be updated to GPL only. The initial version of those quirks was contributed as a change to drivers/soundwire/slave.c, which is dual-licensed. the code was split to a different file and the dual-license followed. I am personally favorable to keeping the code as is, the quirks are just referring to low-level hardware descriptors that are not aligned with DevID hardware registers in external SoundWire devices. If enumeration was handled at a lower level, e.g. in DSP firmware the same information would be quite useful. That said, it's been agreed with Dave that moving forward all new contributions from Intel with a dual-license would include an explicit statement in the commit message as to why it was selected over plain vanilla GPL-2.0-only.
Re: [PATCH] ASoC: Intel: Handle device properties with software node API
On 4/13/21 9:05 AM, Heikki Krogerus wrote: On Tue, Apr 13, 2021 at 03:20:45PM +0300, Heikki Krogerus wrote: On Mon, Apr 12, 2021 at 03:36:20PM -0500, Pierre-Louis Bossart wrote: I took the code and split it in two for BYT/CHT (modified to remove devm_) and SoundWire parts (added as is). https://github.com/thesofproject/linux/pull/2810 Both cases result in a refcount error on device_remove_sof when removing the platform device. I don't understand the code well enough to figure out what happens, but it's likely a case of the software node being removed twice? Right. Because you are injecting the node to an already existing device, the node does not get linked with the device in sysfs. That would increment the reference count in a normal case. It all happens in the function software_node_notify(). Driver core calls it when a device is added and also when it's removed. In this case it is only called when it's removed. I think the best way to handle this now is to simply not decrementing the ref count when you add the properties, so don't call fwnode_handle_put() there (but add a comment explaining why you are not calling it). No, sorry... That's just too hacky. Let's not do that after all. We can also fix this in the software node code. I'm attaching a patch that should make it possible to inject the software nodes also afterwards safely. This is definitely also not without its problems, but we can go with that if it works. Let me know. I tested manually on bytcr w/ RT5640 and used the SOF CI farm to test the SoundWire cases, I don't see any issues with your patch. The refcount issue is gone and the module load/unload cycles don't report any problems. Would you queue it for 5.13-rc1, or is this too late already? For a better solution you would call device_reprobe() after you have injected the software node, but before that you need to modify device_reprobe() so it calls device_platform_notify() (which it really should call in any case). But this should probable be done later, separately. thanks, P.S. Have you guys considered the possibility of describing the connections between all these components by using one of the methods that we now have for that in kernel, for example device graph? It can now be used also with software nodes (OF graph and ACPI device graph are of course already fully supported). I must admit I've never heard of a 'device graph'. Any pointers or APIs you can think of? It's a good comment since we are planning to rework the SOF clients and machine driver handling.
Re: [PATCH] ASoC: Intel: Handle device properties with software node API
Hi Heikki,
diff --git a/sound/soc/intel/boards/bytcht_es8316.c
b/sound/soc/intel/boards/bytcht_es8316.c
index 06df2d46d910b..4a9817a95928c 100644
--- a/sound/soc/intel/boards/bytcht_es8316.c
+++ b/sound/soc/intel/boards/bytcht_es8316.c
@@ -544,7 +544,7 @@ static int snd_byt_cht_es8316_mc_probe(struct
platform_device *pdev)
props[cnt++] =
PROPERTY_ENTRY_BOOL("everest,jack-detect-inverted");
if (cnt) {
- ret = device_add_properties(codec_dev, props);
+ ret = device_create_managed_software_node(codec_dev, props,
NULL);
I don't think this is correct. This is using the codec_dev device, but this
property is created in the machine driver - different device. I think the
proper fix is to remove the property in the machine driver .remove()
callback, as done below for the SoundWire case, and not to rely on devm_
with the wrong device.
there was a thread between July and October on this in
https://github.com/thesofproject/linux/pull/2306/
It seems that we need to restart this work.
Heikki, do you mind if I take your patches (keeping you as the author) and
rework+test them with the SOF tree + CI ?
OK by me (though, I'm not sure about the author part after that).
I took the code and split it in two for BYT/CHT (modified to remove
devm_) and SoundWire parts (added as is).
https://github.com/thesofproject/linux/pull/2810
Both cases result in a refcount error on device_remove_sof when removing
the platform device. I don't understand the code well enough to figure
out what happens, but it's likely a case of the software node being
removed twice?
kernel: [ 872.695132] refcount_t: underflow; use-after-free.
kernel: [ 872.695153] WARNING: CPU: 7 PID: 16086 at lib/refcount.c:28
refcount_warn_saturate+0xa6/0xf0
kernel: [ 872.695222] CPU: 7 PID: 16086 Comm: rmmod Not tainted
5.12.0-rc4-pr2810-5522-default #439c50f6
kernel: [ 872.695225] Hardware name: Intel Corporation Tiger Lake
Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS
TGLSFWI1.R00.3264.A00.2006251828 06/25/2020
kernel: [ 872.695226] RIP: 0010:refcount_warn_saturate+0xa6/0xf0
< snip>
kernel: [ 872.695244] Call Trace:
kernel: [ 872.695248] sof_sdw_rt711_exit+0x2d/0x40 [snd_soc_sof_sdw]
kernel: [ 872.695253] mc_remove+0xa8/0xe0 [snd_soc_sof_sdw]
kernel: [ 872.695256] ? rt711_rtd_init+0x170/0x170 [snd_soc_sof_sdw]
kernel: [ 872.695259] platform_remove+0x1a/0x40
kernel: [ 872.695264] device_release_driver_internal+0xf1/0x1d0
kernel: [ 872.695267] bus_remove_device+0xed/0x160
kernel: [ 872.695269] device_del+0x187/0x3e0
kernel: [ 872.695272] platform_device_del.part.0+0xe/0x60
kernel: [ 872.695274] platform_device_unregister+0x17/0x30
kernel: [ 872.695277] snd_sof_device_remove+0x53/0xf0 [snd_sof]
kernel: [ 872.695283] sof_pci_remove+0x15/0x40 [snd_sof_pci]
kernel: [ 872.695286] pci_device_remove+0x36/0xa0
kernel: [ 872.695290] device_release_driver_internal+0xf1/0x1d0
kernel: [ 872.695292] driver_detach+0x42/0x90
kernel: [ 872.695294] bus_remove_driver+0x56/0xd0
kernel: [ 872.695296] pci_unregister_driver+0x36/0x80
kernel: [ 872.695299] __x64_sys_delete_module+0x18f/0x250
Re: [PATCH] ASoC: Intel: bytcr_wm5102: remove useless variable
On 4/9/21 1:08 AM, Jiapeng Chong wrote:
Fix the following gcc warning:
sound/soc/intel/boards/bytcr_wm5102.c:216:40: warning:
‘byt_wm5102_dai_params’ defined but not used.
Reported-by: Abaci Robot
Signed-off-by: Jiapeng Chong
Thanks for the patch.
Acked-by: Pierre-Louis Bossart
---
sound/soc/intel/boards/bytcr_wm5102.c | 8
1 file changed, 8 deletions(-)
diff --git a/sound/soc/intel/boards/bytcr_wm5102.c
b/sound/soc/intel/boards/bytcr_wm5102.c
index f38850e..fd584e3 100644
--- a/sound/soc/intel/boards/bytcr_wm5102.c
+++ b/sound/soc/intel/boards/bytcr_wm5102.c
@@ -213,14 +213,6 @@ static int byt_wm5102_init(struct snd_soc_pcm_runtime
*runtime)
return 0;
}
-static const struct snd_soc_pcm_stream byt_wm5102_dai_params = {
- .formats = SNDRV_PCM_FMTBIT_S16_LE,
- .rate_min = 48000,
- .rate_max = 48000,
- .channels_min = 2,
- .channels_max = 2,
-};
-
static int byt_wm5102_codec_fixup(struct snd_soc_pcm_runtime *rtd,
struct snd_pcm_hw_params *params)
{
Re: [PATCH] ASoC: amd: Add support for ALC1015P codec in acp3x machine driver
static const struct acpi_device_id acp3x_audio_acpi_match[] = {
{ "AMDI5682", (unsigned long)_5682},
{ "AMDI1015", (unsigned long)_1015},
+ { "AMDP1015", (unsigned long)_1015p},
This isn't a valid ACPI ID. AMDP does not exist in
...
There was a similar issue with Intel platforms using this part, we had
to use a different HID.
Is it okay if i use "AMDI1016" for ALC1015P?
That's valid, though obviously you might regret that later on if someone
releases a CODEC with a 1016 name (equally well ACPI being what it is
there's no good options for naming).
wish granted, the 1016 already exists :-)
you may want to align with what we did for Intel and use the same HID:
RTL1015
As per RTK team inputs, "RTL1015" ACPI HID is in use for RT1015p codec
driver.
RTK team suggested us to use "RTL1015A" as ACPI HID.
Let us know, if we can use "RTL1015A" as an ACPI HID?
Not if you want to be compliant. The part ID remains pegged to 4 hex
digits, no matter what the vendor ID representation is.
The only solution I can suggest is using the PCI ID 0x1002, ie.
AMDI1015 -> AMD platform with RT1015
10021015 -> AMD platform with RT1015p
Note that it's not only ACPI's fault, other standards also only allow
for 16 bits for part IDs, we'd have the same issue with SoundWire.
Re: [PATCH 1/2] soundwire: add macro to selectively change error levels
On 4/1/21 3:56 PM, Greg KH wrote: On Thu, Apr 01, 2021 at 01:43:53PM -0500, Pierre-Louis Bossart wrote: My bigger issue with this is that this macro is crazy. Why do you need debugging here at all for this type of thing? That's what ftrace is for, do not sprinkle code with "we got this return value from here!" all over the place like what this does. We are not sprinkling the code all over the place with any new logs, they exist already in the SoundWire code and this patch helps filter them out. See e.g. patch 2/2 - dev_err(>dev, - "Clk Stop type =%d failed: %d\n", type, ret); + sdw_dev_dbg_or_err(>dev, ret != -ENODATA, + "Clk Stop mode %d type =%d failed: %d\n", + mode, type, ret); You just added a debug log for no reason. The number of logs is lower when dynamic debug is not enabled, and equal when it is. there's no addition. The previous behavior was unconditional dev_err that everyone sees. Now it's dev_err ONLY when the code is NOT -ENODATA, and dev_dgb otherwise, meaning it will seen ONLY be seen IF dynamic debug is enabled for drivers/soundwire/bus.c Allow me to use another example from patch2: - if (ret == -ENODATA) - dev_dbg(bus->dev, - "ClockStopNow Broadcast msg ignored %d", ret); - else - dev_err(bus->dev, - "ClockStopNow Broadcast msg failed %d", ret); + sdw_dev_dbg_or_err(bus->dev, ret != -ENODATA, + "ClockStopNow Broadcast msg failed %d\n", ret); There's no new log, is there? No, but that is not what you showed above which was just an error message being replaced with both a debug and an error message. either debug or error message, not both. Just drop the debug messages, they are pointless, right? That's the primary debug tool used with our friends at RedHat and Canonical, and that includes remote debug where we don't have access to the plaforms. We also have quite a few Bugzilla or github reports from community users who can provide the logs of alsa-info and dmesg, but that's about it. Those debug messages is what we get as feedback and test reports, so we absolutely need them to be 'to the point'. Maybe to reassure you on the scope of the changes I am suggesting here, there is a total of *13* occurrences of dev_dbg() in the SoundWire bus code, and they were added in very specific branches where something goes boink to help folks like Bard and I figure out what sequence led to the problem. I think it's the same on Qualcomm platforms. In these examples related to the clock stop/restart, a message will be generated during pm_runtime suspend/resume sequences and only when unexpected behavior is detected, so the total bandwidth used by these messages is minimal. It has to be that way, we are currently debugging cases where we see those odd behaviors after thousands of suspend/resume cycles, the last thing we want is to be swamped with "pointless" messages. It's not at all like we are reporting "hello, i have this error code", it's rather "this error code should not happen in this sequence". in 99% of the cases, the error code is actually not very useful, it's where the error occurs that is priceless for debug.
Re: [PATCH 1/2] soundwire: add macro to selectively change error levels
My bigger issue with this is that this macro is crazy. Why do you need debugging here at all for this type of thing? That's what ftrace is for, do not sprinkle code with "we got this return value from here!" all over the place like what this does. We are not sprinkling the code all over the place with any new logs, they exist already in the SoundWire code and this patch helps filter them out. See e.g. patch 2/2 - dev_err(>dev, - "Clk Stop type =%d failed: %d\n", type, ret); + sdw_dev_dbg_or_err(>dev, ret != -ENODATA, + "Clk Stop mode %d type =%d failed: %d\n", + mode, type, ret); You just added a debug log for no reason. The number of logs is lower when dynamic debug is not enabled, and equal when it is. there's no addition. The previous behavior was unconditional dev_err that everyone sees. Now it's dev_err ONLY when the code is NOT -ENODATA, and dev_dgb otherwise, meaning it will seen ONLY be seen IF dynamic debug is enabled for drivers/soundwire/bus.c Allow me to use another example from patch2: - if (ret == -ENODATA) - dev_dbg(bus->dev, - "ClockStopNow Broadcast msg ignored %d", ret); - else - dev_err(bus->dev, - "ClockStopNow Broadcast msg failed %d", ret); + sdw_dev_dbg_or_err(bus->dev, ret != -ENODATA, + "ClockStopNow Broadcast msg failed %d\n", ret); There's no new log, is there? If that still gives you a heartburn, I would still like a macro that filters out dev_err so that we don't report an error when it's recoverable or harmless, and don't have spaghetti code as above.
Re: [PATCH 3/7] PM: runtime: remove kernel-doc warnings
On 4/1/21 8:40 AM, Rafael J. Wysocki wrote: On Thu, Apr 1, 2021 at 1:26 AM Pierre-Louis Bossart wrote: remove make W=1 warnings drivers/base/power/runtime.c:926: warning: Function parameter or member 'timer' not described in 'pm_suspend_timer_fn' drivers/base/power/runtime.c:926: warning: Excess function parameter 'data' description in 'pm_suspend_timer_fn' Signed-off-by: Pierre-Louis Bossart --- drivers/base/power/runtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index fe1dad68aee4..1fc1a992f90c 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -951,7 +951,7 @@ static void pm_runtime_work(struct work_struct *work) /** * pm_suspend_timer_fn - Timer function for pm_schedule_suspend(). - * @data: Device pointer passed by pm_schedule_suspend(). + * @timer: hrtimer used by pm_schedule_suspend(). * * Check if the time is right and queue a suspend request. */ -- I can apply this along with the [4-5/7]. Do you want me to do that? Works for me. I wasn't sure by looking at the MAINTAINERS file which files in drivers/base/ are maintained by whom, so sent the patches as a single set.
Re: [PATCH v2] soundwire: qcom: wait for fifo space to be available before read/write
On 4/1/21 4:00 AM, Srinivas Kandagatla wrote: If we write registers very fast we can endup in a situation where some of the writes will be dropped without any notice. So wait for the fifo space to be available before reading/writing the soundwire registers. Out of curiosity, do you actually need to do a check in the read case as well? The commit message talks about writes getting dropped, is the opposite also a problem?
Re: [PATCH] soundwire: intel_init: test link->cdns
On 4/1/21 2:21 AM, Vinod Koul wrote: On 31-03-21, 09:02, Bard Liao wrote: intel_link_probe() could return error and dev_get_drvdata() will return null in such case. So we have to test link->cdns after link->cdns = dev_get_drvdata(>auxdev.dev); Otherwise, we will meet the "kernel NULL pointer dereference" error. This fails to apply for me probably a dependency on the auxiliary bus transition?
Re: [PATCH 1/2] soundwire: add macro to selectively change error levels
On 4/1/21 2:24 AM, Vinod Koul wrote:
On 31-03-21, 09:13, Bard Liao wrote:
From: Pierre-Louis Bossart
We sometimes discard -ENODATA when reporting errors and lose all
traces of issues in the console log, add a macro to add use dev_dbg()
in such cases.
Signed-off-by: Pierre-Louis Bossart
Reviewed-by: Rander Wang
Reviewed-by: Guennadi Liakhovetski
Signed-off-by: Bard Liao
---
drivers/soundwire/bus.h | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 40354469860a..8370216f95d4 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -227,4 +227,12 @@ int sdw_bwrite_no_pm_unlocked(struct sdw_bus *bus, u16
dev_num, u32 addr, u8 val
void sdw_clear_slave_status(struct sdw_bus *bus, u32 request);
int sdw_slave_modalias(const struct sdw_slave *slave, char *buf, size_t size);
+#define sdw_dev_dbg_or_err(dev, is_err, fmt, ...) \
+ do {\
+ if (is_err) \
+ dev_err(dev, fmt, __VA_ARGS__); \
+ else\
+ dev_dbg(dev, fmt, __VA_ARGS__); \
+ } while (0)
I see a variant in sof code and now here, why not add in a
dev_dbg_or_err() and use everywhere?
Good point, I hesitated back and forth on specific v. generic macro.
The main reason why I added this macro for SoundWire is that quite a few
subsystems have their own debug functions (DRM, ACPI, etc), and I wasn't
sure if there was any appetite to add more options in
include/linux/dev_printk.h. SOF also uses a different format due to history.
If at the end of the day SoundWire and SOF are the only users the value
of a common macro is limited.
But it's true that the macro could be used by others.
I really have no opinion here and will follow the consensus.
Re: [PATCH 1/2] soundwire: add macro to selectively change error levels
+#define sdw_dev_dbg_or_err(dev, is_err, fmt, ...) \
+ do {\
+ if (is_err) \
+ dev_err(dev, fmt, __VA_ARGS__); \
+ else\
+ dev_dbg(dev, fmt, __VA_ARGS__); \
+ } while (0)
I see a variant in sof code and now here, why not add in a
dev_dbg_or_err() and use everywhere?
Good point, I hesitated back and forth on specific v. generic macro.
The main reason why I added this macro for SoundWire is that quite a few
subsystems have their own debug functions (DRM, ACPI, etc), and I wasn't
sure if there was any appetite to add more options in
include/linux/dev_printk.h. SOF also uses a different format due to history.
It is better if those other subsystems move to using the common kernel
debug functions. Historically they were all separate, there is no good
reason for them to be that way today.
So please do not create custom subsystem debug macros like this just for
this tiny set of drivers.
My bigger issue with this is that this macro is crazy. Why do you need
debugging here at all for this type of thing? That's what ftrace is
for, do not sprinkle code with "we got this return value from here!" all
over the place like what this does.
We are not sprinkling the code all over the place with any new logs,
they exist already in the SoundWire code and this patch helps filter
them out. See e.g. patch 2/2
- dev_err(>dev,
- "Clk Stop type =%d failed: %d\n", type, ret);
+ sdw_dev_dbg_or_err(>dev, ret != -ENODATA,
+ "Clk Stop mode %d type =%d failed:
%d\n",
+ mode, type, ret);
If you see all my recent patches they were precisely trying to avoid
polluting the console logs with too much information that is irrelevant
from most users, and making sure that when a log is provided it's
uniquely identifiable.
There are similar macros where -EPROBE_DEFER is ignored.
This addresses a very SoundWire-specific case where if we see a -ENODATA
error code (Command Ignored), we ignore it and don't report it by
default. We still have a rare set of cases where this -ENODATA code
shows up unexpectedly, possibly due to problematic reset sequences, and
we want developers to help track them down what causes this sequence
using dynamic debug.
I am not arguing about ftrace v. dynamic debug, and that's also partly
why I didn't feel comfortable expanding the generic set of debug functions.
[PATCH 7/7] devcoredump: fix kernel-doc warning
remove make W=1 warnings
drivers/base/devcoredump.c:208: warning:
Function parameter or member 'data' not described in
'devcd_free_sgtable'
drivers/base/devcoredump.c:208: warning:
Excess function parameter 'table' description in 'devcd_free_sgtable'
drivers/base/devcoredump.c:225: warning:
expecting prototype for devcd_read_from_table(). Prototype was for
devcd_read_from_sgtable() instead
Signed-off-by: Pierre-Louis Bossart
---
drivers/base/devcoredump.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
index 352de5d41466..8eec0e0ddff7 100644
--- a/drivers/base/devcoredump.c
+++ b/drivers/base/devcoredump.c
@@ -202,7 +202,7 @@ static int devcd_match_failing(struct device *dev, const
void *failing)
* NOTE: if two tables allocated with devcd_alloc_sgtable and then chained
* using the sg_chain function then that function should be called only once
* on the chained table
- * @table: pointer to sg_table to free
+ * @data: pointer to sg_table to free
*/
static void devcd_free_sgtable(void *data)
{
@@ -210,7 +210,7 @@ static void devcd_free_sgtable(void *data)
}
/**
- * devcd_read_from_table - copy data from sg_table to a given buffer
+ * devcd_read_from_sgtable - copy data from sg_table to a given buffer
* and return the number of bytes read
* @buffer: the buffer to copy the data to it
* @buf_len: the length of the buffer
--
2.25.1
[PATCH 6/7] platform-msi: fix kernel-doc warnings
remove make W=1 warnings drivers/base/platform-msi.c:336: warning: Function parameter or member 'is_tree' not described in '__platform_msi_create_device_domain' drivers/base/platform-msi.c:336: warning: expecting prototype for platform_msi_create_device_domain(). Prototype was for __platform_msi_create_device_domain() instead Signed-off-by: Pierre-Louis Bossart --- drivers/base/platform-msi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c index 2c1e2e0c1a59..0b72b134a304 100644 --- a/drivers/base/platform-msi.c +++ b/drivers/base/platform-msi.c @@ -316,10 +316,11 @@ void *platform_msi_get_host_data(struct irq_domain *domain) } /** - * platform_msi_create_device_domain - Create a platform-msi domain + * __platform_msi_create_device_domain - Create a platform-msi domain * * @dev: The device generating the MSIs * @nvec: The number of MSIs that need to be allocated + * @is_tree: flag to indicate tree hierarchy * @write_msi_msg: Callback to write an interrupt message for @dev * @ops: The hierarchy domain operations to use * @host_data: Private data associated to this domain -- 2.25.1
[PATCH 3/7] PM: runtime: remove kernel-doc warnings
remove make W=1 warnings drivers/base/power/runtime.c:926: warning: Function parameter or member 'timer' not described in 'pm_suspend_timer_fn' drivers/base/power/runtime.c:926: warning: Excess function parameter 'data' description in 'pm_suspend_timer_fn' Signed-off-by: Pierre-Louis Bossart --- drivers/base/power/runtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index fe1dad68aee4..1fc1a992f90c 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -951,7 +951,7 @@ static void pm_runtime_work(struct work_struct *work) /** * pm_suspend_timer_fn - Timer function for pm_schedule_suspend(). - * @data: Device pointer passed by pm_schedule_suspend(). + * @timer: hrtimer used by pm_schedule_suspend(). * * Check if the time is right and queue a suspend request. */ -- 2.25.1
[PATCH 4/7] PM: wakeup: fix kernel-doc warnings and fix typos
Remove make W=1 warnings and fit 'Itereates' typos
drivers/base/power/wakeup.c:403: warning: wrong kernel-doc identifier on line:
* device_wakeup_arm_wake_irqs(void)
drivers/base/power/wakeup.c:419: warning: wrong kernel-doc identifier on line:
* device_wakeup_disarm_wake_irqs(void)
drivers/base/power/wakeup.c:537: warning: Function parameter or member
'enable' not described in 'device_set_wakeup_enable'
drivers/base/power/wakeup.c:592: warning: expecting prototype for
wakup_source_activate(). Prototype was for wakeup_source_activate()
instead
drivers/base/power/wakeup.c:697: warning: expecting prototype for
wakup_source_deactivate(). Prototype was for
wakeup_source_deactivate() instead
drivers/base/power/wakeup.c:795: warning: Function parameter or member
't' not described in 'pm_wakeup_timer_fn'
drivers/base/power/wakeup.c:795: warning: Excess function parameter
'data' description in 'pm_wakeup_timer_fn'
drivers/base/power/wakeup.c:1027: warning: Function parameter or
member 'set' not described in 'pm_wakep_autosleep_enabled'
drivers/base/power/wakeup.c:1027: warning: Excess function parameter
'enabled' description in 'pm_wakep_autosleep_enabled'
Signed-off-by: Pierre-Louis Bossart
---
drivers/base/power/wakeup.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 92073ac68473..f0b37c188514 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -400,9 +400,9 @@ void device_wakeup_detach_irq(struct device *dev)
}
/**
- * device_wakeup_arm_wake_irqs(void)
+ * device_wakeup_arm_wake_irqs -
*
- * Itereates over the list of device wakeirqs to arm them.
+ * Iterates over the list of device wakeirqs to arm them.
*/
void device_wakeup_arm_wake_irqs(void)
{
@@ -416,9 +416,9 @@ void device_wakeup_arm_wake_irqs(void)
}
/**
- * device_wakeup_disarm_wake_irqs(void)
+ * device_wakeup_disarm_wake_irqs -
*
- * Itereates over the list of device wakeirqs to disarm them.
+ * Iterates over the list of device wakeirqs to disarm them.
*/
void device_wakeup_disarm_wake_irqs(void)
{
@@ -532,6 +532,7 @@ EXPORT_SYMBOL_GPL(device_init_wakeup);
/**
* device_set_wakeup_enable - Enable or disable a device to wake up the system.
* @dev: Device to handle.
+ * @enable: enable/disable flag
*/
int device_set_wakeup_enable(struct device *dev, bool enable)
{
@@ -581,7 +582,7 @@ static bool wakeup_source_not_registered(struct
wakeup_source *ws)
*/
/**
- * wakup_source_activate - Mark given wakeup source as active.
+ * wakeup_source_activate - Mark given wakeup source as active.
* @ws: Wakeup source to handle.
*
* Update the @ws' statistics and, if @ws has just been activated, notify the
PM
@@ -686,7 +687,7 @@ static inline void update_prevent_sleep_time(struct
wakeup_source *ws,
#endif
/**
- * wakup_source_deactivate - Mark given wakeup source as inactive.
+ * wakeup_source_deactivate - Mark given wakeup source as inactive.
* @ws: Wakeup source to handle.
*
* Update the @ws' statistics and notify the PM core that the wakeup source has
@@ -785,7 +786,7 @@ EXPORT_SYMBOL_GPL(pm_relax);
/**
* pm_wakeup_timer_fn - Delayed finalization of a wakeup event.
- * @data: Address of the wakeup source object associated with the event source.
+ * @t: timer list
*
* Call wakeup_source_deactivate() for the wakeup source whose address is
stored
* in @data if it is currently active and its timer has not been canceled and
@@ -1021,7 +1022,7 @@ bool pm_save_wakeup_count(unsigned int count)
#ifdef CONFIG_PM_AUTOSLEEP
/**
* pm_wakep_autosleep_enabled - Modify autosleep_enabled for all wakeup
sources.
- * @enabled: Whether to set or to clear the autosleep_enabled flags.
+ * @set: Whether to set or to clear the autosleep_enabled flags.
*/
void pm_wakep_autosleep_enabled(bool set)
{
--
2.25.1
[PATCH 5/7] PM: clk: remove kernel-doc warning
Remove make W=1 warning: drivers/base/power/clock_ops.c:148: warning: expecting prototype for pm_clk_enable(). Prototype was for __pm_clk_enable() instead Signed-off-by: Pierre-Louis Bossart --- drivers/base/power/clock_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c index 84d5acb6301b..0251f3e6e61d 100644 --- a/drivers/base/power/clock_ops.c +++ b/drivers/base/power/clock_ops.c @@ -140,7 +140,7 @@ static void pm_clk_op_unlock(struct pm_subsys_data *psd, unsigned long *flags) } /** - * pm_clk_enable - Enable a clock, reporting any errors + * __pm_clk_enable - Enable a clock, reporting any errors * @dev: The device for the given clock * @ce: PM clock entry corresponding to the clock. */ -- 2.25.1
[PATCH 2/7] driver core: attribute_container: remove kernel-doc warnings
Remove make W=1 warnings drivers/base/attribute_container.c:471: warning: Function parameter or member 'cont' not described in 'attribute_container_add_class_device_adapter' drivers/base/attribute_container.c:471: warning: Function parameter or member 'dev' not described in 'attribute_container_add_class_device_adapter' drivers/base/attribute_container.c:471: warning: Function parameter or member 'classdev' not described in 'attribute_container_add_class_device_adapter' Signed-off-by: Pierre-Louis Bossart --- drivers/base/attribute_container.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/base/attribute_container.c b/drivers/base/attribute_container.c index f7bd0f4db13d..9c00d203d61e 100644 --- a/drivers/base/attribute_container.c +++ b/drivers/base/attribute_container.c @@ -461,6 +461,10 @@ attribute_container_add_class_device(struct device *classdev) /** * attribute_container_add_class_device_adapter - simple adapter for triggers * + * @cont: the container to register. + * @dev: the generic device to activate the trigger for + * @classdev: the class device to add + * * This function is identical to attribute_container_add_class_device except * that it is designed to be called from the triggers */ -- 2.25.1
[PATCH 1/7] driver core: remove kernel-doc warnings
remove make W=1 warning: drivers/base/core.c:1670: warning: Function parameter or member 'flags' not described in 'fw_devlink_create_devlink' drivers/base/core.c:1670: warning: Function parameter or member 'con' not described in 'fw_devlink_create_devlink' drivers/base/core.c:1670: warning: Function parameter or member 'sup_handle' not described in 'fw_devlink_create_devlink' drivers/base/core.c:1670: warning: Function parameter or member 'flags' not described in 'fw_devlink_create_devlink' drivers/base/core.c:1763: warning: Function parameter or member 'dev' not described in '__fw_devlink_link_to_consumers' drivers/base/core.c:1844: warning: Function parameter or member 'dev' not described in '__fw_devlink_link_to_suppliers' drivers/base/core.c:1844: warning: Function parameter or member 'fwnode' not described in '__fw_devlink_link_to_suppliers' Signed-off-by: Pierre-Louis Bossart --- drivers/base/core.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/base/core.c b/drivers/base/core.c index de518178ac36..ad0d26f04215 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -1647,8 +1647,9 @@ static int fw_devlink_relax_cycle(struct device *con, void *sup) /** * fw_devlink_create_devlink - Create a device link from a consumer to fwnode - * @con - Consumer device for the device link - * @sup_handle - fwnode handle of supplier + * @con: consumer device for the device link + * @sup_handle: fwnode handle of supplier + * @flags: devlink flags * * This function will try to create a device link between the consumer device * @con and the supplier device represented by @sup_handle. @@ -1744,7 +1745,7 @@ static int fw_devlink_create_devlink(struct device *con, /** * __fw_devlink_link_to_consumers - Create device links to consumers of a device - * @dev - Device that needs to be linked to its consumers + * @dev: Device that needs to be linked to its consumers * * This function looks at all the consumer fwnodes of @dev and creates device * links between the consumer device and @dev (supplier). @@ -1814,8 +1815,8 @@ static void __fw_devlink_link_to_consumers(struct device *dev) /** * __fw_devlink_link_to_suppliers - Create device links to suppliers of a device - * @dev - The consumer device that needs to be linked to its suppliers - * @fwnode - Root of the fwnode tree that is used to create device links + * @dev: The consumer device that needs to be linked to its suppliers + * @fwnode: Root of the fwnode tree that is used to create device links * * This function looks at all the supplier fwnodes of fwnode tree rooted at * @fwnode and creates device links between @dev (consumer) and all the -- 2.25.1
[PATCH 0/7] drivers/base: remove kernel-doc warnings
Trivial fixes to reduce the number of warnings with make W=1
After this patchset, there are still a number of false positives
reported, I left them as is.
drivers/base/power/runtime.c:347: warning: Excess function parameter
'dev' description in '__rpm_callback'
drivers/base/platform.c:1545:20: warning: no previous prototype for
‘early_platform_cleanup’ [-Wmissing-prototypes]
1545 | void __weak __init early_platform_cleanup(void) { }
|^~
drivers/base/attribute_container.c:304: warning: Function parameter or
member 'fn' not described in 'attribute_container_device_trigger_safe'
drivers/base/attribute_container.c:304: warning: Function parameter or
member 'undo' not described in
'attribute_container_device_trigger_safe'
drivers/base/attribute_container.c:357: warning: Function parameter or
member 'fn' not described in 'attribute_container_device_trigger'
drivers/base/module.c: In function ‘module_add_driver’:
drivers/base/module.c:36:6: warning: variable ‘no_warn’ set but not
used [-Wunused-but-set-variable]
36 | int no_warn;
| ^~~
Pierre-Louis Bossart (7):
driver core: remove kernel-doc warnings
driver core: attribute_container: remove kernel-doc warnings
PM: runtime: remove kernel-doc warnings
PM: wakeup: fix kernel-doc warnings and fix typos
PM: clk: remove kernel-doc warning
platform-msi: fix kernel-doc warnings
devcoredump: fix kernel-doc warning
drivers/base/attribute_container.c | 4
drivers/base/core.c| 11 ++-
drivers/base/devcoredump.c | 4 ++--
drivers/base/platform-msi.c| 3 ++-
drivers/base/power/clock_ops.c | 2 +-
drivers/base/power/runtime.c | 2 +-
drivers/base/power/wakeup.c| 17 +
7 files changed, 25 insertions(+), 18 deletions(-)
base-commit: 7a43c78d0573e00456b033e2b9a895b89464
--
2.25.1
Re: [PATCH] soundwire: qcom: wait for fifo space to be available before read/write
+static int swrm_wait_for_rd_fifo_avail(struct qcom_swrm_ctrl *swrm)
+{
+ u32 fifo_outstanding_cmd, value;
+ u8 fifo_retry_count = SWR_OVERFLOW_RETRY_COUNT;
+
+ /* Check for fifo underflow during read */
+ swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, );
+ fifo_outstanding_cmd = FIELD_GET(SWRM_RD_CMD_FIFO_CNT_MASK, value);
+
+/* Check number of outstanding commands in fifo before read */
+ if (fifo_outstanding_cmd)
+ return 0;
+
+ do {
+ usleep_range(500, 510);
+ swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, );
+ fifo_outstanding_cmd = FIELD_GET(SWRM_RD_CMD_FIFO_CNT_MASK,
value);
+ if (fifo_outstanding_cmd > 0)
+ break;
+ } while (fifo_retry_count--);
+
+ if (fifo_outstanding_cmd == 0) {
+ dev_err_ratelimited(swrm->dev, "%s err read underflow\n",
__func__);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static int swrm_wait_for_wr_fifo_avail(struct qcom_swrm_ctrl *swrm)
+{
+ u32 fifo_outstanding_cmd, value;
+ u8 fifo_retry_count = SWR_OVERFLOW_RETRY_COUNT;
+
+ /* Check for fifo overflow during write */
+ swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, );
+ fifo_outstanding_cmd = FIELD_GET(SWRM_WR_CMD_FIFO_CNT_MASK, value);
+
+ /* Check number of outstanding commands in fifo before write */
+ if (fifo_outstanding_cmd != swrm->wr_fifo_depth)
+ return 0;
+
+ do {
+ usleep_range(500, 510);
+ swrm->reg_read(swrm, SWRM_CMD_FIFO_STATUS, );
+ fifo_outstanding_cmd = FIELD_GET(SWRM_WR_CMD_FIFO_CNT_MASK,
value);
+ if (fifo_outstanding_cmd < swrm->wr_fifo_depth)
+ break;
+ } while (fifo_retry_count--);
+
+ if (fifo_outstanding_cmd == swrm->wr_fifo_depth) {
+ dev_err_ratelimited(swrm->dev, "%s err write overflow\n",
__func__);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
nit-pick: you could merge the prologue and loop body with a
while(fifo_retry_count--) and put the usleep_range() at the end of the loop.
Re: [PATCH V2] soundwire: qcom: use signed variable for error return
On 3/31/21 10:55 AM, Vinod Koul wrote: We get warning of using a unsigned variable being compared to less than zero. The comparison is correct as it checks for errors from previous call to qcom_swrm_get_alert_slave_dev_num(), so we should use a signed variable here. While at it, drop the superfluous initialization as well drivers/soundwire/qcom.c: qcom_swrm_irq_handler() warn: impossible condition '(devnum < 0) => (0-255 < 0)' Reported-by: kernel test robot Signed-off-by: Vinod Koul Reviewed-by: Pierre-Louis Bossart --- drivers/soundwire/qcom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index b08ecb9b418c..ec86c4e53fdb 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -428,7 +428,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) struct qcom_swrm_ctrl *swrm = dev_id; u32 value, intr_sts, intr_sts_masked, slave_status; u32 i; - u8 devnum = 0; + int devnum; int ret = IRQ_HANDLED; swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, _sts);
Re: [PATCH] soundwire: qcom: use signed variable for error return
On 3/31/21 2:21 AM, Vinod Koul wrote: We get warning for using a unsigned variable being compared to less than zero. The comparison is correct as it checks for errors from previous call to qcom_swrm_get_alert_slave_dev_num(), so we should use a signed variable instead. drivers/soundwire/qcom.c: qcom_swrm_irq_handler() warn: impossible condition '(devnum < 0) => (0-255 < 0)' Reported-by: kernel test robot Signed-off-by: Vinod Koul --- drivers/soundwire/qcom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index b08ecb9b418c..55ed133c6704 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -428,7 +428,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) struct qcom_swrm_ctrl *swrm = dev_id; u32 value, intr_sts, intr_sts_masked, slave_status; u32 i; - u8 devnum = 0; + s8 devnum = 0; it's not great to store negative error codes with s8. That works in this specific case because the function only returns -EINVAL. We actually have zero occurrences of s8 in the drivers/soundwire/ code. int ret = IRQ_HANDLED; swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, _sts);
Re: [PATCH] ASoC: amd: Add support for ALC1015P codec in acp3x machine driver
On 3/30/21 10:35 AM, Mark Brown wrote:
On Tue, Mar 30, 2021 at 09:12:11PM +0530, Mukunda,Vijendar wrote:
On 3/30/21 7:52 PM, Pierre-Louis Bossart wrote:
static const struct acpi_device_id acp3x_audio_acpi_match[] = {
{ "AMDI5682", (unsigned long)_5682},
{ "AMDI1015", (unsigned long)_1015},
+ { "AMDP1015", (unsigned long)_1015p},
This isn't a valid ACPI ID. AMDP does not exist in
...
There was a similar issue with Intel platforms using this part, we had
to use a different HID.
Is it okay if i use "AMDI1016" for ALC1015P?
That's valid, though obviously you might regret that later on if someone
releases a CODEC with a 1016 name (equally well ACPI being what it is
there's no good options for naming).
wish granted, the 1016 already exists :-)
you may want to align with what we did for Intel and use the same HID:
RTL1015
Re: [PATCH] ASoC: amd: Add support for ALC1015P codec in acp3x machine driver
static const struct acpi_device_id acp3x_audio_acpi_match[] = {
{ "AMDI5682", (unsigned long)_5682},
{ "AMDI1015", (unsigned long)_1015},
+ { "AMDP1015", (unsigned long)_1015p},
This isn't a valid ACPI ID. AMDP does not exist in
https://uefi.org/acpi_id_list
There was a similar issue with Intel platforms using this part, we had
to use a different HID.
[RFC PATCH 0/2] ASoC: remove cppchecks warnings on lm49453 and da732x
There are the last two patches in the cleanups, this time I am not sure what the code does and what the proper fix might be. Feedback welcome. Pierre-Louis Bossart (2): ASoC: lm49453: fix useless assignment before return ASoC: da732x: simplify code sound/soc/codecs/da732x.c | 17 ++--- sound/soc/codecs/da732x.h | 12 sound/soc/codecs/lm49453.c | 2 -- 3 files changed, 10 insertions(+), 21 deletions(-) -- 2.25.1
[RFC PATCH 2/2] ASoC: da732x: simplify code
cppcheck reports a false positive:
sound/soc/codecs/da732x.c:1161:25: warning: Either the condition
'indiv<0' is redundant or there is division by zero at line
1161. [zerodivcond]
fref = (da732x->sysclk / indiv);
^
sound/soc/codecs/da732x.c:1158:12: note: Assuming that condition
'indiv<0' is not redundant
if (indiv < 0)
^
sound/soc/codecs/da732x.c:1161:25: note: Division by zero
fref = (da732x->sysclk / indiv);
^
The code is awfully convoluted/confusing and can be simplified with a
single variable and the BIT macro.
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/da732x.c | 17 ++---
sound/soc/codecs/da732x.h | 12
2 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/sound/soc/codecs/da732x.c b/sound/soc/codecs/da732x.c
index d43ee7159ae0..42d6a3fc3af5 100644
--- a/sound/soc/codecs/da732x.c
+++ b/sound/soc/codecs/da732x.c
@@ -168,30 +168,25 @@ static const struct reg_default da732x_reg_cache[] = {
static inline int da732x_get_input_div(struct snd_soc_component *component,
int sysclk)
{
int val;
- int ret;
if (sysclk < DA732X_MCLK_10MHZ) {
- val = DA732X_MCLK_RET_0_10MHZ;
- ret = DA732X_MCLK_VAL_0_10MHZ;
+ val = DA732X_MCLK_VAL_0_10MHZ;
} else if ((sysclk >= DA732X_MCLK_10MHZ) &&
(sysclk < DA732X_MCLK_20MHZ)) {
- val = DA732X_MCLK_RET_10_20MHZ;
- ret = DA732X_MCLK_VAL_10_20MHZ;
+ val = DA732X_MCLK_VAL_10_20MHZ;
} else if ((sysclk >= DA732X_MCLK_20MHZ) &&
(sysclk < DA732X_MCLK_40MHZ)) {
- val = DA732X_MCLK_RET_20_40MHZ;
- ret = DA732X_MCLK_VAL_20_40MHZ;
+ val = DA732X_MCLK_VAL_20_40MHZ;
} else if ((sysclk >= DA732X_MCLK_40MHZ) &&
(sysclk <= DA732X_MCLK_54MHZ)) {
- val = DA732X_MCLK_RET_40_54MHZ;
- ret = DA732X_MCLK_VAL_40_54MHZ;
+ val = DA732X_MCLK_VAL_40_54MHZ;
} else {
return -EINVAL;
}
snd_soc_component_write(component, DA732X_REG_PLL_CTRL, val);
- return ret;
+ return val;
}
static void da732x_set_charge_pump(struct snd_soc_component *component, int
state)
@@ -1158,7 +1153,7 @@ static int da732x_set_dai_pll(struct snd_soc_component
*component, int pll_id,
if (indiv < 0)
return indiv;
- fref = (da732x->sysclk / indiv);
+ fref = da732x->sysclk / BIT(indiv);
div_hi = freq_out / fref;
frac_div = (u64)(freq_out % fref) * 8192ULL;
do_div(frac_div, fref);
diff --git a/sound/soc/codecs/da732x.h b/sound/soc/codecs/da732x.h
index c5af17ee1516..c2f784c3f359 100644
--- a/sound/soc/codecs/da732x.h
+++ b/sound/soc/codecs/da732x.h
@@ -48,14 +48,10 @@
#defineDA732X_MCLK_20MHZ 2000
#defineDA732X_MCLK_40MHZ 4000
#defineDA732X_MCLK_54MHZ 5400
-#defineDA732X_MCLK_RET_0_10MHZ 0
-#defineDA732X_MCLK_VAL_0_10MHZ 1
-#defineDA732X_MCLK_RET_10_20MHZ1
-#defineDA732X_MCLK_VAL_10_20MHZ2
-#defineDA732X_MCLK_RET_20_40MHZ2
-#defineDA732X_MCLK_VAL_20_40MHZ4
-#defineDA732X_MCLK_RET_40_54MHZ3
-#defineDA732X_MCLK_VAL_40_54MHZ8
+#defineDA732X_MCLK_VAL_0_10MHZ 0
+#defineDA732X_MCLK_VAL_10_20MHZ1
+#defineDA732X_MCLK_VAL_20_40MHZ2
+#defineDA732X_MCLK_VAL_40_54MHZ3
#defineDA732X_DAI_ID1 0
#defineDA732X_DAI_ID2 1
#defineDA732X_SRCCLK_PLL 0
--
2.25.1
[RFC PATCH 1/2] ASoC: lm49453: fix useless assignment before return
Cppcheck warning: sound/soc/codecs/lm49453.c:1210:11: style: Variable 'pll_clk' is assigned a value that is never used. [unreadVariable] pll_clk = BIT(4); ^ FIXME: What is the correct fix? /* fll clk slection */ pll_clk = BIT(4); return 0; is the assignment redundant or the 'return 0' a mistake? Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/lm49453.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/sound/soc/codecs/lm49453.c b/sound/soc/codecs/lm49453.c index eb3dd0bd80d9..fb0fb23537e7 100644 --- a/sound/soc/codecs/lm49453.c +++ b/sound/soc/codecs/lm49453.c @@ -1206,8 +1206,6 @@ static int lm49453_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id, break; case 48000: case 32576: - /* fll clk slection */ - pll_clk = BIT(4); return 0; default: return -EINVAL; -- 2.25.1
[PATCH 17/17] ASoC: ux500: mop500: align function prototype
cppcheck warning: sound/soc/ux500/mop500_ab8500.c:360:60: style:inconclusive: Function 'mop500_ab8500_machine_init' argument 1 names different: declaration 'runtime' definition 'rtd'. [funcArgNamesDifferent] int mop500_ab8500_machine_init(struct snd_soc_pcm_runtime *rtd) ^ sound/soc/ux500/mop500_ab8500.h:16:60: note: Function 'mop500_ab8500_machine_init' argument 1 names different: declaration 'runtime' definition 'rtd'. int mop500_ab8500_machine_init(struct snd_soc_pcm_runtime *runtime); ^ sound/soc/ux500/mop500_ab8500.c:360:60: note: Function 'mop500_ab8500_machine_init' argument 1 names different: declaration 'runtime' definition 'rtd'. int mop500_ab8500_machine_init(struct snd_soc_pcm_runtime *rtd) ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/ux500/mop500_ab8500.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/ux500/mop500_ab8500.h b/sound/soc/ux500/mop500_ab8500.h index 99cfd972ea7a..8138a4e9aaf5 100644 --- a/sound/soc/ux500/mop500_ab8500.h +++ b/sound/soc/ux500/mop500_ab8500.h @@ -13,7 +13,7 @@ extern struct snd_soc_ops mop500_ab8500_ops[]; -int mop500_ab8500_machine_init(struct snd_soc_pcm_runtime *runtime); +int mop500_ab8500_machine_init(struct snd_soc_pcm_runtime *rtd); void mop500_ab8500_remove(struct snd_soc_card *card); #endif -- 2.25.1
[PATCH 15/17] ASoC: ti: omap-mcsp: remove duplicate test
cppcheck warning:
sound/soc/ti/omap-mcbsp.c:379:11: style: The if condition is the same
as the previous if condition [duplicateCondition]
if (mcbsp->irq) {
^
sound/soc/ti/omap-mcbsp.c:376:11: note: First condition
if (mcbsp->irq)
^
sound/soc/ti/omap-mcbsp.c:379:11: note: Second condition
if (mcbsp->irq) {
^
Keeping two separate tests was probably intentional for clarity, but
since this generates warnings we might as well make cppcheck happy so
that we have fewer warnings.
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/ti/omap-mcbsp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/ti/omap-mcbsp.c b/sound/soc/ti/omap-mcbsp.c
index 6025b30bbe77..db47981768c5 100644
--- a/sound/soc/ti/omap-mcbsp.c
+++ b/sound/soc/ti/omap-mcbsp.c
@@ -373,10 +373,9 @@ static void omap_mcbsp_free(struct omap_mcbsp *mcbsp)
MCBSP_WRITE(mcbsp, WAKEUPEN, 0);
/* Disable interrupt requests */
- if (mcbsp->irq)
+ if (mcbsp->irq) {
MCBSP_WRITE(mcbsp, IRQEN, 0);
- if (mcbsp->irq) {
free_irq(mcbsp->irq, (void *)mcbsp);
} else {
free_irq(mcbsp->rx_irq, (void *)mcbsp);
--
2.25.1
[PATCH 16/17] ASoC: ux500: mop500: rename shadowing variable
cppcheck warning:
sound/soc/ux500/mop500.c:143:23: style: Local variable 'mop500_card'
shadows outer variable [shadowVariable]
struct snd_soc_card *mop500_card = platform_get_drvdata(pdev);
^
sound/soc/ux500/mop500.c:54:28: note: Shadowed declaration
static struct snd_soc_card mop500_card = {
^
sound/soc/ux500/mop500.c:143:23: note: Shadow variable
struct snd_soc_card *mop500_card = platform_get_drvdata(pdev);
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/ux500/mop500.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/ux500/mop500.c b/sound/soc/ux500/mop500.c
index cdae1190b930..4f41bb0ab2b0 100644
--- a/sound/soc/ux500/mop500.c
+++ b/sound/soc/ux500/mop500.c
@@ -140,12 +140,12 @@ static int mop500_probe(struct platform_device *pdev)
static int mop500_remove(struct platform_device *pdev)
{
- struct snd_soc_card *mop500_card = platform_get_drvdata(pdev);
+ struct snd_soc_card *card = platform_get_drvdata(pdev);
pr_debug("%s: Enter.\n", __func__);
- snd_soc_unregister_card(mop500_card);
- mop500_ab8500_remove(mop500_card);
+ snd_soc_unregister_card(card);
+ mop500_ab8500_remove(card);
mop500_of_node_put();
return 0;
--
2.25.1
[PATCH 13/17] ASoC: tegra: tegra20_das: align function prototypes
)
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/tegra/tegra20_das.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/tegra/tegra20_das.h b/sound/soc/tegra/tegra20_das.h
index d22abc4d08e6..18e832ded73a 100644
--- a/sound/soc/tegra/tegra20_das.h
+++ b/sound/soc/tegra/tegra20_das.h
@@ -95,7 +95,7 @@ struct tegra20_das {
* dap_id: DAP to connect: TEGRA20_DAS_DAP_ID_*
* dac_sel: DAC to connect to: TEGRA20_DAS_DAP_SEL_DAC*
*/
-extern int tegra20_das_connect_dap_to_dac(int dap_id, int dac_sel);
+extern int tegra20_das_connect_dap_to_dac(int dap, int dac);
/*
* Connect a DAP to another DAP
@@ -105,7 +105,7 @@ extern int tegra20_das_connect_dap_to_dac(int dap_id, int
dac_sel);
* sdata1rx: Is this DAP's SDATA1 pin RX (1) or TX (0)
* sdata2rx: Is this DAP's SDATA2 pin RX (1) or TX (0)
*/
-extern int tegra20_das_connect_dap_to_dap(int dap_id, int other_dap_sel,
+extern int tegra20_das_connect_dap_to_dap(int dap, int otherdap,
int master, int sdata1rx,
int sdata2rx);
@@ -115,6 +115,6 @@ extern int tegra20_das_connect_dap_to_dap(int dap_id, int
other_dap_sel,
* dac_id: DAC ID to connect: TEGRA20_DAS_DAC_ID_*
* dap_sel: DAP to receive input from: TEGRA20_DAS_DAC_SEL_DAP*
*/
-extern int tegra20_das_connect_dac_to_dap(int dac_id, int dap_sel);
+extern int tegra20_das_connect_dac_to_dap(int dac, int dap);
#endif
--
2.25.1
[PATCH 14/17] ASoC: ti: omap-abe-twl6040: remove useless assignment
cppcheck warning: sound/soc/ti/omap-abe-twl6040.c:173:10: style: Variable 'ret' is assigned a value that is never used. [unreadVariable] int ret = 0; ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/ti/omap-abe-twl6040.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/ti/omap-abe-twl6040.c b/sound/soc/ti/omap-abe-twl6040.c index 16ea039ff865..91cc9a4f44d7 100644 --- a/sound/soc/ti/omap-abe-twl6040.c +++ b/sound/soc/ti/omap-abe-twl6040.c @@ -170,7 +170,7 @@ static int omap_abe_twl6040_init(struct snd_soc_pcm_runtime *rtd) struct snd_soc_card *card = rtd->card; struct abe_twl6040 *priv = snd_soc_card_get_drvdata(card); int hs_trim; - int ret = 0; + int ret; /* * Configure McPDM offset cancellation based on the HSOTRIM value from -- 2.25.1
[PATCH 12/17] ASoC: tegra: tegra20_das: clarify expression
cppcheck warning: sound/soc/tegra/tegra20_das.c:64:60: style: Boolean result is used in bitwise operation. Clarify expression with parentheses. [clarifyCondition] reg = otherdap << TEGRA20_DAS_DAP_CTRL_SEL_DAP_CTRL_SEL_P | ^ sound/soc/tegra/tegra20_das.c:65:61: style: Boolean result is used in bitwise operation. Clarify expression with parentheses. [clarifyCondition] !!sdata2rx << TEGRA20_DAS_DAP_CTRL_SEL_DAP_SDATA2_TX_RX_P | ^ sound/soc/tegra/tegra20_das.c:66:61: style: Boolean result is used in bitwise operation. Clarify expression with parentheses. [clarifyCondition] !!sdata1rx << TEGRA20_DAS_DAP_CTRL_SEL_DAP_SDATA1_TX_RX_P | ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/tegra/tegra20_das.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sound/soc/tegra/tegra20_das.c b/sound/soc/tegra/tegra20_das.c index 79dba878d854..69c651274c12 100644 --- a/sound/soc/tegra/tegra20_das.c +++ b/sound/soc/tegra/tegra20_das.c @@ -61,10 +61,10 @@ int tegra20_das_connect_dap_to_dap(int dap, int otherdap, int master, addr = TEGRA20_DAS_DAP_CTRL_SEL + (dap * TEGRA20_DAS_DAP_CTRL_SEL_STRIDE); - reg = otherdap << TEGRA20_DAS_DAP_CTRL_SEL_DAP_CTRL_SEL_P | - !!sdata2rx << TEGRA20_DAS_DAP_CTRL_SEL_DAP_SDATA2_TX_RX_P | - !!sdata1rx << TEGRA20_DAS_DAP_CTRL_SEL_DAP_SDATA1_TX_RX_P | - !!master << TEGRA20_DAS_DAP_CTRL_SEL_DAP_MS_SEL_P; + reg = (otherdap << TEGRA20_DAS_DAP_CTRL_SEL_DAP_CTRL_SEL_P) | + (!!sdata2rx << TEGRA20_DAS_DAP_CTRL_SEL_DAP_SDATA2_TX_RX_P) | + (!!sdata1rx << TEGRA20_DAS_DAP_CTRL_SEL_DAP_SDATA1_TX_RX_P) | + (!!master << TEGRA20_DAS_DAP_CTRL_SEL_DAP_MS_SEL_P); tegra20_das_write(addr, reg); -- 2.25.1
[PATCH 10/17] ASoC: stm: stm32_adfsdm: fix snprintf format string
cppcheck warning:
sound/soc/stm/stm32_adfsdm.c:120:2: warning: %d in format
string (no. 1) requires 'int' but the argument type is 'unsigned
int'. [invalidPrintfArgType_sint]
snprintf(str_freq, sizeof(str_freq), "%d\n", freq);
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/stm/stm32_adfsdm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/stm/stm32_adfsdm.c b/sound/soc/stm/stm32_adfsdm.c
index 47fae8dd20b4..e6078f50e508 100644
--- a/sound/soc/stm/stm32_adfsdm.c
+++ b/sound/soc/stm/stm32_adfsdm.c
@@ -117,7 +117,7 @@ static int stm32_adfsdm_set_sysclk(struct snd_soc_dai *dai,
int clk_id,
/* Set IIO frequency if CODEC is master as clock comes from SPI_IN */
- snprintf(str_freq, sizeof(str_freq), "%d\n", freq);
+ snprintf(str_freq, sizeof(str_freq), "%u\n", freq);
size = iio_write_channel_ext_info(priv->iio_ch, "spi_clk_freq",
str_freq, sizeof(str_freq));
if (size != sizeof(str_freq)) {
--
2.25.1
[PATCH 11/17] ASoC: sunxi: sun8i-codec: clarify expression
cppcheck warning:
sound/soc/sunxi/sun8i-codec.c:488:28: style: Clarify calculation
precedence for '%' and '?'. [clarifyCalculation]
return sample_rate % 4000 ? 22579200 : 24576000;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/sunxi/sun8i-codec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
index 460924fc173f..518bfb724a5b 100644
--- a/sound/soc/sunxi/sun8i-codec.c
+++ b/sound/soc/sunxi/sun8i-codec.c
@@ -485,7 +485,7 @@ static int sun8i_codec_get_lrck_div_order(unsigned int
slots,
static unsigned int sun8i_codec_get_sysclk_rate(unsigned int sample_rate)
{
- return sample_rate % 4000 ? 22579200 : 24576000;
+ return (sample_rate % 4000) ? 22579200 : 24576000;
}
static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
--
2.25.1
[PATCH 07/17] ASoC: pxa: remove useless assignment
cppcheck warning:
sound/soc/pxa/mmp-pcm.c:207:10: style: Variable 'ret' is assigned a
value that is never used. [unreadVariable]
int ret = 0, stream;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/pxa/mmp-pcm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/pxa/mmp-pcm.c b/sound/soc/pxa/mmp-pcm.c
index 53fc49e32fbc..5d520e18e512 100644
--- a/sound/soc/pxa/mmp-pcm.c
+++ b/sound/soc/pxa/mmp-pcm.c
@@ -204,7 +204,7 @@ static int mmp_pcm_new(struct snd_soc_component *component,
{
struct snd_pcm_substream *substream;
struct snd_pcm *pcm = rtd->pcm;
- int ret = 0, stream;
+ int ret, stream;
for (stream = 0; stream < 2; stream++) {
substream = pcm->streams[stream].substream;
--
2.25.1
[PATCH 08/17] ASoC: sti: sti_uniperif: add missing error check
cppcheck warning: sound/soc/sti/sti_uniperif.c:490:6: style: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment] ret = devm_snd_soc_register_component(>dev, ^ sound/soc/sti/sti_uniperif.c:486:6: note: ret is assigned ret = sti_uniperiph_cpu_dai_of(node, priv); ^ sound/soc/sti/sti_uniperif.c:490:6: note: ret is overwritten ret = devm_snd_soc_register_component(>dev, ^ sti_uniperiph_cpu_dai_of() can return -EINVAL which seems like a good-enough reason to bail. Signed-off-by: Pierre-Louis Bossart --- sound/soc/sti/sti_uniperif.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/soc/sti/sti_uniperif.c b/sound/soc/sti/sti_uniperif.c index 7b9169f04d6e..67315d9b352d 100644 --- a/sound/soc/sti/sti_uniperif.c +++ b/sound/soc/sti/sti_uniperif.c @@ -484,6 +484,8 @@ static int sti_uniperiph_probe(struct platform_device *pdev) priv->pdev = pdev; ret = sti_uniperiph_cpu_dai_of(node, priv); + if (ret < 0) + return ret; dev_set_drvdata(>dev, priv); -- 2.25.1
[PATCH 09/17] ASoC: sti: uniperif: align function prototypes
cppcheck warning:
sound/soc/sti/uniperif_player.c:1049:24: style:inconclusive: Function
'uni_player_init' argument 2 names different: declaration 'uni_player'
definition 'player'. [funcArgNamesDifferent]
struct uniperif *player)
^
sound/soc/sti/uniperif.h:1375:24: note: Function 'uni_player_init'
argument 2 names different: declaration 'uni_player' definition
'player'.
struct uniperif *uni_player);
^
sound/soc/sti/uniperif_player.c:1049:24: note: Function
'uni_player_init' argument 2 names different: declaration 'uni_player'
definition 'player'.
struct uniperif *player)
^
sound/soc/sti/uniperif_reader.c:411:24: style:inconclusive: Function
'uni_reader_init' argument 2 names different: declaration 'uni_reader'
definition 'reader'. [funcArgNamesDifferent]
struct uniperif *reader)
^
sound/soc/sti/uniperif.h:1380:24: note: Function 'uni_reader_init'
argument 2 names different: declaration 'uni_reader' definition
'reader'.
struct uniperif *uni_reader);
^
sound/soc/sti/uniperif_reader.c:411:24: note: Function
'uni_reader_init' argument 2 names different: declaration 'uni_reader'
definition 'reader'.
struct uniperif *reader)
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/sti/uniperif.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sti/uniperif.h b/sound/soc/sti/uniperif.h
index a16adeb7c1e9..2a5de328501c 100644
--- a/sound/soc/sti/uniperif.h
+++ b/sound/soc/sti/uniperif.h
@@ -1372,12 +1372,12 @@ static __maybe_unused const struct snd_pcm_hardware
uni_tdm_hw = {
/* uniperiph player*/
int uni_player_init(struct platform_device *pdev,
- struct uniperif *uni_player);
+ struct uniperif *player);
int uni_player_resume(struct uniperif *player);
/* uniperiph reader */
int uni_reader_init(struct platform_device *pdev,
- struct uniperif *uni_reader);
+ struct uniperif *reader);
/* common */
int sti_uniperiph_dai_set_fmt(struct snd_soc_dai *dai,
--
2.25.1
[PATCH 04/17] ASoC: bcm: cygnus_ssp: remove useless initialization
Cppcheck warning: sound/soc/bcm/cygnus-ssp.c:1364:6: style: Redundant initialization for 'err'. The initialized value is overwritten before it is read. [redundantInitialization] err = devm_snd_soc_register_component(dev, _ssp_component, ^ sound/soc/bcm/cygnus-ssp.c:1313:10: note: err is initialized int err = -EINVAL; ^ sound/soc/bcm/cygnus-ssp.c:1364:6: note: err is overwritten err = devm_snd_soc_register_component(dev, _ssp_component, ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/bcm/cygnus-ssp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/bcm/cygnus-ssp.c b/sound/soc/bcm/cygnus-ssp.c index 6e634b448293..fea296b41a43 100644 --- a/sound/soc/bcm/cygnus-ssp.c +++ b/sound/soc/bcm/cygnus-ssp.c @@ -1310,7 +1310,7 @@ static int cygnus_ssp_probe(struct platform_device *pdev) struct device_node *child_node; struct resource *res; struct cygnus_audio *cygaud; - int err = -EINVAL; + int err; int node_count; int active_port_count; -- 2.25.1
[PATCH 02/17] ASoC: atmel: fix shadowed variable
Fix cppcheck warning:
sound/soc/atmel/atmel-classd.c:51:14: style: Local variable 'pwm_type'
shadows outer variable [shadowVariable]
const char *pwm_type;
^
sound/soc/atmel/atmel-classd.c:226:27: note: Shadowed declaration
static const char * const pwm_type[] = {
^
sound/soc/atmel/atmel-classd.c:51:14: note: Shadow variable
const char *pwm_type;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/atmel/atmel-classd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/atmel/atmel-classd.c b/sound/soc/atmel/atmel-classd.c
index b1a28a9382fb..6023369e0f1a 100644
--- a/sound/soc/atmel/atmel-classd.c
+++ b/sound/soc/atmel/atmel-classd.c
@@ -48,7 +48,7 @@ static struct atmel_classd_pdata *atmel_classd_dt_init(struct
device *dev)
{
struct device_node *np = dev->of_node;
struct atmel_classd_pdata *pdata;
- const char *pwm_type;
+ const char *pwm_type_s;
int ret;
if (!np) {
@@ -60,8 +60,8 @@ static struct atmel_classd_pdata *atmel_classd_dt_init(struct
device *dev)
if (!pdata)
return ERR_PTR(-ENOMEM);
- ret = of_property_read_string(np, "atmel,pwm-type", _type);
- if ((ret == 0) && (strcmp(pwm_type, "diff") == 0))
+ ret = of_property_read_string(np, "atmel,pwm-type", _type_s);
+ if ((ret == 0) && (strcmp(pwm_type_s, "diff") == 0))
pdata->pwm_type = CLASSD_MR_PWMTYP_DIFF;
else
pdata->pwm_type = CLASSD_MR_PWMTYP_SINGLE;
--
2.25.1
[PATCH 00/17] ASoC: remove cppcheck warnings for multiple SOCs
Trivial cleanups to make cppcheck less verbose. There should be no functionality change, except for the 'sti_uniperif' patch where an error check was added. Pierre-Louis Bossart (17): ASoC: amd: renoir: acp3x-pdm-dma: remove unnecessary assignments ASoC: atmel: fix shadowed variable ASoC: atmel: atmel-i2s: remove useless initialization ASoC: bcm: cygnus_ssp: remove useless initialization ASoC: meson: axg-tdmin: remove useless assignment ASoC: meson: axg-tdmout: remove useless assignment ASoC: pxa: remove useless assignment ASoC: sti: sti_uniperif: add missing error check ASoC: sti: uniperif: align function prototypes ASoC: stm: stm32_adfsdm: fix snprintf format string ASoC: sunxi: sun8i-codec: clarify expression ASoC: tegra: tegra20_das: clarify expression ASoC: tegra: tegra20_das: align function prototypes ASoC: ti: omap-abe-twl6040: remove useless assignment ASoC: ti: omap-mcsp: remove duplicate test ASoC: ux500: mop500: rename shadowing variable ASoC: ux500: mop500: align function prototype sound/soc/amd/renoir/acp3x-pdm-dma.c | 2 -- sound/soc/atmel/atmel-classd.c | 6 +++--- sound/soc/atmel/atmel-i2s.c | 2 +- sound/soc/bcm/cygnus-ssp.c | 2 +- sound/soc/meson/axg-tdmin.c | 2 +- sound/soc/meson/axg-tdmout.c | 2 +- sound/soc/pxa/mmp-pcm.c | 2 +- sound/soc/sti/sti_uniperif.c | 2 ++ sound/soc/sti/uniperif.h | 4 ++-- sound/soc/stm/stm32_adfsdm.c | 2 +- sound/soc/sunxi/sun8i-codec.c| 2 +- sound/soc/tegra/tegra20_das.c| 8 sound/soc/tegra/tegra20_das.h| 6 +++--- sound/soc/ti/omap-abe-twl6040.c | 2 +- sound/soc/ti/omap-mcbsp.c| 3 +-- sound/soc/ux500/mop500.c | 6 +++--- sound/soc/ux500/mop500_ab8500.h | 2 +- 17 files changed, 27 insertions(+), 28 deletions(-) -- 2.25.1
[PATCH 06/17] ASoC: meson: axg-tdmout: remove useless assignment
cppcheck complains about potential null pointer dereference but it's
rather an unnecessary assignment to NULL before walking through a
list.
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/meson/axg-tdmout.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/meson/axg-tdmout.c b/sound/soc/meson/axg-tdmout.c
index 3ceabddae629..22d519fc07b2 100644
--- a/sound/soc/meson/axg-tdmout.c
+++ b/sound/soc/meson/axg-tdmout.c
@@ -55,7 +55,7 @@ static const struct regmap_config axg_tdmout_regmap_cfg = {
static struct snd_soc_dai *
axg_tdmout_get_be(struct snd_soc_dapm_widget *w)
{
- struct snd_soc_dapm_path *p = NULL;
+ struct snd_soc_dapm_path *p;
struct snd_soc_dai *be;
snd_soc_dapm_widget_for_each_sink_path(w, p) {
--
2.25.1
[PATCH 05/17] ASoC: meson: axg-tdmin: remove useless assignment
cppcheck complains about potential null pointer dereference but it's
rather an unnecessary assignment to NULL before walking through a
list.
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/meson/axg-tdmin.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/meson/axg-tdmin.c b/sound/soc/meson/axg-tdmin.c
index b4faf9d5c1aa..49b613a1faf2 100644
--- a/sound/soc/meson/axg-tdmin.c
+++ b/sound/soc/meson/axg-tdmin.c
@@ -57,7 +57,7 @@ static const struct snd_kcontrol_new axg_tdmin_in_mux =
static struct snd_soc_dai *
axg_tdmin_get_be(struct snd_soc_dapm_widget *w)
{
- struct snd_soc_dapm_path *p = NULL;
+ struct snd_soc_dapm_path *p;
struct snd_soc_dai *be;
snd_soc_dapm_widget_for_each_source_path(w, p) {
--
2.25.1
[PATCH 03/17] ASoC: atmel: atmel-i2s: remove useless initialization
Cppcheck complains: sound/soc/atmel/atmel-i2s.c:628:6: style: Redundant initialization for 'err'. The initialized value is overwritten before it is read. [redundantInitialization] err = devm_request_irq(>dev, irq, atmel_i2s_interrupt, 0, ^ sound/soc/atmel/atmel-i2s.c:598:10: note: err is initialized int err = -ENXIO; ^ sound/soc/atmel/atmel-i2s.c:628:6: note: err is overwritten err = devm_request_irq(>dev, irq, atmel_i2s_interrupt, 0, ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/atmel/atmel-i2s.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/atmel/atmel-i2s.c b/sound/soc/atmel/atmel-i2s.c index 7c6187e41f2b..584656cc7d3c 100644 --- a/sound/soc/atmel/atmel-i2s.c +++ b/sound/soc/atmel/atmel-i2s.c @@ -595,7 +595,7 @@ static int atmel_i2s_probe(struct platform_device *pdev) struct regmap *regmap; void __iomem *base; int irq; - int err = -ENXIO; + int err; unsigned int pcm_flags = 0; unsigned int version; -- 2.25.1
[PATCH 01/17] ASoC: amd: renoir: acp3x-pdm-dma: remove unnecessary assignments
cppcheck warning:
sound/soc/amd/renoir/acp3x-pdm-dma.c:132:17: style: Variable
'pdm_dma_enable' is assigned a value that is never
used. [unreadVariable]
pdm_dma_enable = 0x00;
^
sound/soc/amd/renoir/acp3x-pdm-dma.c:156:18: style: Variable
'pdm_dma_enable' is assigned a value that is never
used. [unreadVariable]
pdm_dma_enable = 0x00;
^
indeed those values are never used because the timeout is reset.
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/amd/renoir/acp3x-pdm-dma.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/soc/amd/renoir/acp3x-pdm-dma.c
b/sound/soc/amd/renoir/acp3x-pdm-dma.c
index 7b14d9a81b97..1acd20439399 100644
--- a/sound/soc/amd/renoir/acp3x-pdm-dma.c
+++ b/sound/soc/amd/renoir/acp3x-pdm-dma.c
@@ -129,7 +129,6 @@ static int start_pdm_dma(void __iomem *acp_base)
enable_pdm_clock(acp_base);
rn_writel(pdm_enable, acp_base + ACP_WOV_PDM_ENABLE);
rn_writel(pdm_dma_enable, acp_base + ACP_WOV_PDM_DMA_ENABLE);
- pdm_dma_enable = 0x00;
timeout = 0;
while (++timeout < ACP_COUNTER) {
pdm_dma_enable = rn_readl(acp_base + ACP_WOV_PDM_DMA_ENABLE);
@@ -153,7 +152,6 @@ static int stop_pdm_dma(void __iomem *acp_base)
if (pdm_dma_enable & 0x01) {
pdm_dma_enable = 0x02;
rn_writel(pdm_dma_enable, acp_base + ACP_WOV_PDM_DMA_ENABLE);
- pdm_dma_enable = 0x00;
timeout = 0;
while (++timeout < ACP_COUNTER) {
pdm_dma_enable = rn_readl(acp_base +
--
2.25.1
Re: [PATCH] soundwire: intel: move to auxiliary bus
On 3/24/21 10:36 AM, Greg KH wrote: On Wed, Mar 24, 2021 at 09:55:01AM -0500, Pierre-Louis Bossart wrote: Note at this point it would mean an API change and impact the existing Nvidia/Mellanox code, we are using the same sequence as them THere is no "stable api" in the kernel, so if something has to change, that's fine, we can change the users at the same time, not an issue. What I meant is that this requires consensus to make a change, and so far I haven't seen any burning desire from the contributors to revisit the 2-step sequence. I will however modify the code in this patch to implement a SoundWire 'linkdev' register/unregister function, it'll be much easier to review and maintain, and will follow the same pattern as the mlx5 code (all errors and domain-specific initializations handled in the same function). Draft code being tested is at https://github.com/thesofproject/linux/pull/2809
Re: [PATCH] soundwire: intel: move to auxiliary bus
On 3/24/21 5:50 AM, Vinod Koul wrote:
On 23-03-21, 12:29, Pierre-Louis Bossart wrote:
Thanks Greg and Vinod for the reviews
-static int intel_master_probe(struct platform_device *pdev)
+static int intel_link_probe(struct auxiliary_device *auxdev, const struct
auxiliary_device_id *id)
{
- struct device *dev = >dev;
+ struct device *dev = >dev;
+ struct sdw_intel_link_dev *ldev =
auxiliary_dev_to_sdw_intel_link_dev(auxdev);
Do we need another abstractions for resources here, why not aux dev
creation set the resources required and we skip this step...
Not sure what resources you are referring to?
Resources in the sdw_intel_link_dev which are sdw_intel_link_res. They
should be resources for auxdev and if you do that lets you get rid of
this abstraction.
Sorry Vinod, I am not following your line of thought. We must be talking
of different things or having a different understanding of what the
auxiliary device is.
The auxiliary device is deliberately minimal by design and does not
contain domain-specific information/resources/pointers/pdata as the
platform_device does. You extend it by defining a larger structure that
contains an auxiliary device and whatever domain-specific
fields/structures/domains are needed, then use container_of to access it.
It's not just Intel doing this, the first example from Mellanox uses the
same pattern, albeit with a single pointer instead of the structure we used.
see e.g.
https://elixir.bootlin.com/linux/latest/source/include/linux/mlx5/driver.h#L545
So I am not sure what you mean by 'rid of this abstraction' when this
abstraction is pretty much the way things were designed?
Maybe an example of what sort of structure you had in mind would help?
this is just a container_of() and the documented way of extending the auxbus
(see
https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#example-usage)
struct sdw_intel_link_dev {
struct auxiliary_device auxdev;
struct sdw_intel_link_res link_res;
};
#define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
struct sdw_intel *sdw;
struct sdw_cdns *cdns;
struct sdw_bus *bus;
@@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device
*pdev)
cdns = >cdns;
bus = >bus;
- sdw->instance = pdev->id;
- sdw->link_res = dev_get_platdata(dev);
+ sdw->instance = auxdev->id;
so auxdev has id and still we pass id as argument :( Not sure if folks
can fix this now
That's odd, yeah, it should be fixed.
I think we are talking about different things?
this is defined in mod_devicetable.h:
struct auxiliary_device_id {
char name[AUXILIARY_NAME_SIZE];
kernel_ulong_t driver_data;
};
and used for the driver probe:
ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table,
auxdev));
but there is a separate id:
struct auxiliary_device {
struct device dev;
const char *name;
u32 id;
};
which is set during the device initialization in intel_init.c
/* we don't use an IDA since we already have a link ID */
auxdev->id = link_id;
In the auxiliary bus design, the parent has to take care of managing this
id, be it with an IDA or as we do here with a physical link ID that is
unique.
Aha, maybe both of them should not be 'id' to avoid this confusion!
the function definition follows the expected prototype
struct auxiliary_driver {
int (*probe)(struct auxiliary_device *,
const struct auxiliary_device_id *id);
we can rename the argument to e.g. dev_id if that helps. Suggestions
welcome.
That also reminds me that we have duplicate info:
+ sdw->instance = auxdev->id;
+ bus->link_id = auxdev->id;
drop the local driver instance and use bus->link_id please
if you are referring to sdw->instance, it could probably be removed, but
that would need to be a separate cleanup changing cadence_master.c as
well. this patch only changes pdev->id with auxdev->id and provides only
the transition from platform device to auxiliary device.
Re: [PATCH] soundwire: intel: move to auxiliary bus
Note that the auxiliary bus API has separate init and add steps, which
requires more attention in the error unwinding paths. The main loop
needs to deal with kfree() and auxiliary_device_uninit() for the
current iteration before jumping to the common label which releases
everything allocated in prior iterations.
The init/add steps can be moved together in the aux bus code if that
makes this usage simpler. Please do that instead.
IIRC the two steps were separated during the auxbus reviews to allow the
parent to call kfree() on an init failure, and auxiliary_device_uninit()
afterwards.
https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device
With a single auxbus_register(), the parent wouldn't know whether to use
kfree() or auxiliary_device_uinit() when an error is returned, would it?
It should, you know the difference when you call device_register() vs.
device_initialize()/device_add(), for what to do, right?
Should be no difference here either :)
sorry, not following.
with the regular devices, the errors can only happen on the second "add"
stage.
int device_register(struct device *dev)
{
device_initialize(dev);
return device_add(dev);
}
that's not what is currently implemented for the auxiliary bus
the current flow is
ldev = kzalloc(..)
some inits
ret = auxiliary_device_init(>auxdev)
if (ret < 0) {
kfree(ldev);
goto err1;
}
ret = auxiliary_device_add(>auxdev)
if (ret < 0)
auxiliary_device_uninit(>auxdev)
goto err2;
}
...
err2:
err1:
How would I convert this to
ldev = kzalloc(..)
some inits
ret = auxiliary_device_register()
if (ret) {
kfree(ldev) or not?
unit or not?
}
IIRC during reviews there was an ask that the parent and name be checked,
and that's why the code added the two checks below:
int auxiliary_device_init(struct auxiliary_device *auxdev)
{
struct device *dev = >dev;
if (!dev->parent) {
pr_err("auxiliary_device has a NULL dev->parent\n");
return -EINVAL;
}
if (!auxdev->name) {
pr_err("auxiliary_device has a NULL name\n");
return -EINVAL;
}
dev->bus = _bus_type;
device_initialize(>dev);
return 0;
}
does this clarify the sequence?
Yes, thanks, but I don't know the answer to your question, sorry. This
feels more complex than it should be, but I do not have the time at the
moment to look into it, sorry.
Try getting the authors of this code to fix it up :)
We can try to check why those two tests were added before initialize(),
I don't fully recall these details
If we could move these tests after device_initialize() then we could add
a _register function.
Note at this point it would mean an API change and impact the existing
Nvidia/Mellanox code, we are using the same sequence as them
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/dev.c#L262
Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
I am using hw_params_fixup, but it's not enough. The first thing I do is to not add the BE HW constraint rules in runtime->hw_constraints, because this will potentially affect the FE HW params. If the FE does sampling rate conversion, for example, applying the sampling rate constrain rules from a BE codec on FE is useless. The second thing I do is to apply these BE HW constraint rules to the BE HW params. It's true that the BE HW params can be fine tuned via hw_params_fixup (topology, device-tree or even static parameters) and set in such a way that avoid the BE HW constraints, so we could ignore the constraint rules added by their drivers. It's not every elegant and running the BE HW constraint rules for the fixup BE HW params can be a sanity check. Also, I am thinking that if the FE does no conversion (be_hw_params_fixup missing) and more than one BEs are available, applying the HW constraint rules on the same set of BE HW params could rule out the incompatible BE DAI links and start the compatible ones only. I am not sure this is a real usecase. Even after a second cup of coffee I was not able to follow why the hw_params_fixup was not enough? That paragraph is rather dense. And to be frank I don't fully understand the problem statement above: "separate the FE HW constraints from the BE HW constraints". We have existing solutions with a DSP-based SRC adjusting FE rates to what is required by the BE dailink. Maybe it would help to show examples of what you can do today and what you cannot, so that we are on the same wavelength on what the limitations are and how to remove them?
Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 3/23/21 6:43 AM, Codrin Ciubotariu wrote:
HW constraints are needed to set limitations for HW parameters used to
configure the DAIs. All DAIs on the same link must agree upon the HW
parameters, so the parameters are affected by the DAIs' features and
their limitations. In case of DPCM, the FE DAIs can be used to perform
different kind of conversions, such as format or rate changing, bringing
the audio stream to a configuration supported by all the DAIs of the BE's
link. For this reason, the limitations of the BE DAIs are not always
important for the HW parameters between user-space and FE, only for the
paratemers between FE and BE DAI links. This brings us to this patch-set,
which aims to separate the FE HW constraints from the BE HW constraints.
This way, the FE can be used to perform more efficient HW conversions, on
the initial audio stream parameters, to parameters supported by the BE
DAIs.
To achieve this, the first thing needed is to detect whether a HW
constraint rule is enforced by a FE or a BE DAI. This means that
snd_pcm_hw_rule_add() needs to be able to differentiate between the two
type of DAIs. For this, the runtime pointer to struct snd_pcm_runtime is
replaced with a pointer to struct snd_pcm_substream, to be able to reach
substream->pcm->internal to differentiate between FE and BE DAIs.
This change affects many sound drivers (and one gpu drm driver).
All these changes are included in the first patch, to have a better
overview of the implications created by this change.
The second patch adds a new struct snd_pcm_hw_constraints under struct
snd_soc_dpcm_runtime, which is used to store the HW constraint rules
added by the BE DAIs. This structure is initialized with a subset of the
runtime constraint rules which does not include the rules that affect
the buffer or period size. snd_pcm_hw_rule_add() will add the BE rules
to the new struct snd_pcm_hw_constraints.
The third and last patch will apply the BE rule constraints, after the
fixup callback. If the fixup HW parameters do not respect the BE
constraint rules, the rules will exit with an error. The FE mask and
interval constraints are copied to the BE ones, to satisfy the
dai_link->dpcm_merged_* flags. The dai_link->dpcm_merged_* flags are
used to know if the FE does format or sampling rate conversion.
I tested with ad1934 and wm8731 codecs as BEs, with a not-yet-mainlined
ASRC as FE, that can also do format conversion. I realize that the
change to snd_pcm_hw_rule_add() has a big impact, even though all the
drivers use snd_pcm_hw_rule_add() with substream->runtime, so passing
substream instead of runtime is not that risky.
can you use the BE hw_params_fixup instead?
That's what we use for SOF.
The FE hw_params are propagated to the BE, and then the BE can update
the hw_params based on its own limitations and pass the result
downstream, e.g. to a codec.
I'll copy below my understanding of the flow, which we discussed
recently in the SOF team:
my understanding is that we start with the front-end hw_params as the
basis for the back-end hw_params.
static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
int ret, stream = substream->stream;
mutex_lock_nested(>card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
memcpy(>dpcm[stream].hw_params, params,
sizeof(struct snd_pcm_hw_params));
ret = dpcm_be_dai_hw_params(fe, stream);
<<< the BE is handled first.
if (ret < 0) {
dev_err(fe->dev,"ASoC: hw_params BE failed %d\n", ret);
goto out;
}
dev_dbg(fe->dev, "ASoC: hw_params FE %s rate %d chan %x fmt %d\n",
fe->dai_link->name, params_rate(params),
params_channels(params), params_format(params));
/* call hw_params on the frontend */
ret = soc_pcm_hw_params(substream, params);
then each BE will be configured
int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
{
struct snd_soc_dpcm *dpcm;
int ret;
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
struct snd_pcm_substream *be_substream =
snd_soc_dpcm_get_substream(be, stream);
/* is this op for this BE ? */
if (!snd_soc_dpcm_be_can_update(fe, be, stream))
continue;
/* copy params for each dpcm */
memcpy(>hw_params, >dpcm[stream].hw_params,
sizeof(struct snd_pcm_hw_params));
/* perform any hw_params fixups */
ret = snd_soc_link_be_hw_params_fixup(be, >hw_params);
The fixup is the key, in SOF this is where we are going to look for
information from the topology.
/* fixup the BE DAI link to match any values from topology */
int sof_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd, struct
snd_pcm_hw_params *params)
{
struct
Re: [PATCH] soundwire: intel: move to auxiliary bus
On 3/23/21 1:32 PM, Greg KH wrote:
On Tue, Mar 23, 2021 at 01:04:49PM -0500, Pierre-Louis Bossart wrote:
Note that the auxiliary bus API has separate init and add steps, which
requires more attention in the error unwinding paths. The main loop
needs to deal with kfree() and auxiliary_device_uninit() for the
current iteration before jumping to the common label which releases
everything allocated in prior iterations.
The init/add steps can be moved together in the aux bus code if that
makes this usage simpler. Please do that instead.
IIRC the two steps were separated during the auxbus reviews to allow the
parent to call kfree() on an init failure, and auxiliary_device_uninit()
afterwards.
https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device
With a single auxbus_register(), the parent wouldn't know whether to use
kfree() or auxiliary_device_uinit() when an error is returned, would it?
It should, you know the difference when you call device_register() vs.
device_initialize()/device_add(), for what to do, right?
Should be no difference here either :)
sorry, not following.
with the regular devices, the errors can only happen on the second "add"
stage.
int device_register(struct device *dev)
{
device_initialize(dev);
return device_add(dev);
}
that's not what is currently implemented for the auxiliary bus
the current flow is
ldev = kzalloc(..)
some inits
ret = auxiliary_device_init(>auxdev)
if (ret < 0) {
kfree(ldev);
goto err1;
}
ret = auxiliary_device_add(>auxdev)
if (ret < 0)
auxiliary_device_uninit(>auxdev)
goto err2;
}
...
err2:
err1:
How would I convert this to
ldev = kzalloc(..)
some inits
ret = auxiliary_device_register()
if (ret) {
kfree(ldev) or not?
unit or not?
}
IIRC during reviews there was an ask that the parent and name be
checked, and that's why the code added the two checks below:
int auxiliary_device_init(struct auxiliary_device *auxdev)
{
struct device *dev = >dev;
if (!dev->parent) {
pr_err("auxiliary_device has a NULL dev->parent\n");
return -EINVAL;
}
if (!auxdev->name) {
pr_err("auxiliary_device has a NULL name\n");
return -EINVAL;
}
dev->bus = _bus_type;
device_initialize(>dev);
return 0;
}
does this clarify the sequence?
Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Minor comments below.
On 3/22/21 4:38 AM, Perry Yuan wrote:
From: Perry Yuan
add support for Dell privacy driver for the Dell units equipped
hardware privacy design, which protect users privacy of audio and
camera from hardware level. Once the audio or camera privacy mode
activated, any applications will not get any audio or video stream
when user pressed ctrl+F4 hotkey, audio privacy mode will be
enabled, micmute led will be also changed accordingly
The micmute led is fully controlled by hardware & EC(embedded controller)
and camera mute hotkey is Ctrl+F9. Currently design only emmits
typo: emits
SW_CAMERA_LENS_COVER event while the camera lens shutter will be
changed by EC & hw(hadware) control
typo: hardware
*The flow is like this:
1) User presses key. HW does stuff with this key (timeout timer is started)
2) WMI event is emitted from BIOS to kernel
3) WMI event is received by dell-privacy
4) KEY_MICMUTE emitted from dell-privacy
5) Userland picks up key and modifies kcontrol for SW mute
6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
ledtrig_audio_set(LED_AUDIO_MICMUTE,
rt715->micmute_led ? LED_ON :LED_OFF);
7) If "LED" is set to on dell-privacy notifies EC,
and timeout is cancelled, HW mic mute activated.
what happens if there is timeout? You have an explicit description of
the timer handling in the success case, but not what happens on a timeout...
diff --git a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
new file mode 100644
index ..20c15a51ec38
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
@@ -0,0 +1,32 @@
+What:
/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported
+Date: Feb 2021
+KernelVersion: 5.12
5.13 now?
+static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct privacy_acpi_priv *priv = privacy_acpi;
+ acpi_status status;
+ acpi_handle handle;
+ static char *acpi_method = (char *)"ECAK";
+
+ handle = ec_get_handle();
+ if (!handle)
+ return -EIO;
+ if (!acpi_has_method(handle, acpi_method))
+ return -EIO;
+ status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
+ if (ACPI_FAILURE(status)) {
+ dev_err(priv->dev, "Error setting privacy EC ack value: %s\n",
+ acpi_format_exception(status));
+ return -EIO;
+ }
+ pr_debug("set dell privacy micmute ec ack event done\n");
can we use dev_dbg() here? Same for all occurrences of pr_debug and
pr_err below, it's cleaner and easier to filter.
+ return 0;
+}
+
+static int dell_privacy_acpi_remove(struct platform_device *pdev)
+{
+ struct privacy_acpi_priv *priv = dev_get_drvdata(privacy_acpi->dev);
+
+ led_classdev_unregister(>cdev);
+
+ return 0;
+}
+/*
+ * Pressing the mute key activates a time delayed circuit to physically cut
+ * off the mute. The LED is in the same circuit, so it reflects the true
+ * state of the HW mute. The reason for the EC "ack" is so that software
+ * can first invoke a SW mute before the HW circuit is cut off. Without SW
+ * cutting this off first does not affect the time delayed muting or status
+ * of the LED but there is a possibility of a "popping" noise.
+ *
+ * If the EC receives the SW ack, the circuit will be activated before the
+ * delay completed.
+ *
+ * Exposing as an LED device allows the codec drivers notification path to
+ * EC ACK to work
+ */
+static int dell_privacy_leds_setup(struct device *dev)
+{
+ struct privacy_acpi_priv *priv = dev_get_drvdata(dev);
+ int ret = 0;
useless init
+
+ priv->cdev.name = "dell-privacy::micmute";
+ priv->cdev.max_brightness = 1;
+ priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
+ priv->cdev.default_trigger = "audio-micmute";
+ priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+ ret = devm_led_classdev_register(dev, >cdev);
+ if (ret)
+ return ret;
+ return 0;
+}
+
+static int dell_privacy_acpi_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ platform_set_drvdata(pdev, privacy_acpi);
+ privacy_acpi->dev = >dev;
+ privacy_acpi->platform_device = pdev;
+
+ ret = dell_privacy_leds_setup(>dev);
+ if (ret)
+ return -EIO;
any reason to hard-code -EIO, woud 'return ret' be enough?
+
+ return 0;
+}
+
+static struct platform_driver dell_privacy_platform_drv = {
+ .driver = {
+ .name = PRIVACY_PLATFORM_NAME,
+ },
+ .probe = dell_privacy_acpi_probe,
+ .remove = dell_privacy_acpi_remove,
+};
+
+int __init dell_privacy_acpi_init(void)
is the __init necessary? You call this routine
Re: [PATCH] soundwire: add slave device to linked list after device_register()
Hi Vinod,
We currently add the slave device to a linked list before
device_register(), and then remove it if device_register() fails.
It's not clear why this sequence was necessary, this patch moves the
linked list management to after the device_register().
Maybe add a comment :-)
The reason here is that before calling device_register() we need to be
ready and completely initialized. device_register is absolutely the last
step in the flow, always.
The probe of the device can happen and before you get a chance to
add to list, many number of things can happen.. So adding after is not a
very good idea :)
Can you describe that 'many number of things' in the SoundWire context?
While you are correct in general on the use of device_register(), in
this specific case the device registration and the possible Slave driver
probe if there's a match doesn't seem to use this linked list.
This sdw_slave_add() routine is called while parsing ACPI/DT tables and
at this point the bus isn't functional. This sequence only deals with
device registration and driver probe, the actual activation and
enumeration happen much later. What does matter is that by the time all
ACPI/DT devices have been scanned all initialization is complete. The
last part of the flow is not the device_register() at the individual
peripheral level.
Even for the Qualcomm case, the steps are different:
ret = sdw_bus_master_add(>bus, dev, dev->fwnode);
if (ret) {
dev_err(dev, "Failed to register Soundwire controller (%d)\n",
ret);
goto err_clk;
}
qcom_swrm_init(ctrl); <<< that's where the bus is functional
Note that we are not going to lay on the tracks for this, this sequence
was tagged by static analysis tools who don't understand that
put_device() actually frees memory by way of the .release callback, but
that led us to ask ourselves whether this sequence was actually necessary.
Re: [PATCH] soundwire: intel: move to auxiliary bus
Note that the auxiliary bus API has separate init and add steps, which requires more attention in the error unwinding paths. The main loop needs to deal with kfree() and auxiliary_device_uninit() for the current iteration before jumping to the common label which releases everything allocated in prior iterations. The init/add steps can be moved together in the aux bus code if that makes this usage simpler. Please do that instead. IIRC the two steps were separated during the auxbus reviews to allow the parent to call kfree() on an init failure, and auxiliary_device_uninit() afterwards. https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device With a single auxbus_register(), the parent wouldn't know whether to use kfree() or auxiliary_device_uinit() when an error is returned, would it?
Re: [PATCH] soundwire: intel: move to auxiliary bus
Thanks Greg and Vinod for the reviews
-static int intel_master_probe(struct platform_device *pdev)
+static int intel_link_probe(struct auxiliary_device *auxdev, const struct
auxiliary_device_id *id)
{
- struct device *dev = >dev;
+ struct device *dev = >dev;
+ struct sdw_intel_link_dev *ldev =
auxiliary_dev_to_sdw_intel_link_dev(auxdev);
Do we need another abstractions for resources here, why not aux dev
creation set the resources required and we skip this step...
Not sure what resources you are referring to?
this is just a container_of() and the documented way of extending the
auxbus (see
https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#example-usage)
struct sdw_intel_link_dev {
struct auxiliary_device auxdev;
struct sdw_intel_link_res link_res;
};
#define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \
container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev)
struct sdw_intel *sdw;
struct sdw_cdns *cdns;
struct sdw_bus *bus;
@@ -1346,14 +1347,14 @@ static int intel_master_probe(struct platform_device
*pdev)
cdns = >cdns;
bus = >bus;
- sdw->instance = pdev->id;
- sdw->link_res = dev_get_platdata(dev);
+ sdw->instance = auxdev->id;
so auxdev has id and still we pass id as argument :( Not sure if folks
can fix this now
That's odd, yeah, it should be fixed.
I think we are talking about different things?
this is defined in mod_devicetable.h:
struct auxiliary_device_id {
char name[AUXILIARY_NAME_SIZE];
kernel_ulong_t driver_data;
};
and used for the driver probe:
ret = auxdrv->probe(auxdev, auxiliary_match_id(auxdrv->id_table,
auxdev));
but there is a separate id:
struct auxiliary_device {
struct device dev;
const char *name;
u32 id;
};
which is set during the device initialization in intel_init.c
/* we don't use an IDA since we already have a link ID */
auxdev->id = link_id;
In the auxiliary bus design, the parent has to take care of managing
this id, be it with an IDA or as we do here with a physical link ID that
is unique.
in short, I don't see how I could change the code given the differences
in concepts?
Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
As you suggested,I should add the alignment change in another patch.
But if i keep the old alignment, the code will be very odd.
Seems like that I have to change the below code to new alignment in this
patch.
if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) { <<--- changed back
if (!privacy_valid)
has_privacy = true;
else
has_privacy = false;
if (!has_privacy) {
micmute_led_cdev.brightness <<--- new alignment
...
}
...
}
I don't get the point, sorry. The code above doesn't seem properly
indented or there were spurious tab/spaces conversions?
Re: [PATCH] ASoC: Intel: Handle device properties with software node API
On 3/22/21 6:06 AM, Heikki Krogerus wrote:
The function device_add_properties() is going to be removed.
Replacing it with software node API equivalents.
Signed-off-by: Heikki Krogerus
---
Hi,
This patch depends on a fix from mainline, available in v5.12-rc4:
2a92c90f2ecc ("software node: Fix device_add_software_node()")
Cheers,
---
sound/soc/intel/boards/bytcht_es8316.c | 2 +-
sound/soc/intel/boards/bytcr_rt5640.c | 2 +-
sound/soc/intel/boards/bytcr_rt5651.c | 2 +-
sound/soc/intel/boards/sof_sdw_rt711.c | 20 +++-
sound/soc/intel/boards/sof_sdw_rt711_sdca.c | 20 +++-
5 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/sound/soc/intel/boards/bytcht_es8316.c
b/sound/soc/intel/boards/bytcht_es8316.c
index 06df2d46d910b..4a9817a95928c 100644
--- a/sound/soc/intel/boards/bytcht_es8316.c
+++ b/sound/soc/intel/boards/bytcht_es8316.c
@@ -544,7 +544,7 @@ static int snd_byt_cht_es8316_mc_probe(struct
platform_device *pdev)
props[cnt++] =
PROPERTY_ENTRY_BOOL("everest,jack-detect-inverted");
if (cnt) {
- ret = device_add_properties(codec_dev, props);
+ ret = device_create_managed_software_node(codec_dev, props,
NULL);
I don't think this is correct. This is using the codec_dev device, but
this property is created in the machine driver - different device. I
think the proper fix is to remove the property in the machine driver
.remove() callback, as done below for the SoundWire case, and not to
rely on devm_ with the wrong device.
there was a thread between July and October on this in
https://github.com/thesofproject/linux/pull/2306/
It seems that we need to restart this work.
Heikki, do you mind if I take your patches (keeping you as the author)
and rework+test them with the SOF tree + CI ?
if (ret) {
put_device(codec_dev);
return ret;
diff --git a/sound/soc/intel/boards/bytcr_rt5640.c
b/sound/soc/intel/boards/bytcr_rt5640.c
index 59d6d47c8d829..661dad81e5bce 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -918,7 +918,7 @@ static int byt_rt5640_add_codec_device_props(const char
*i2c_dev_name)
if (byt_rt5640_quirk & BYT_RT5640_JD_NOT_INV)
props[cnt++] =
PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted");
- ret = device_add_properties(i2c_dev, props);
+ ret = device_create_managed_software_node(i2c_dev, props, NULL);
put_device(i2c_dev);
return ret;
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c
b/sound/soc/intel/boards/bytcr_rt5651.c
index 148b7b1bd3e8c..4cb6ef4c3a3d9 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -547,7 +547,7 @@ static int byt_rt5651_add_codec_device_props(struct device
*i2c_dev)
if (byt_rt5651_quirk & BYT_RT5651_JD_NOT_INV)
props[cnt++] =
PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted");
- return device_add_properties(i2c_dev, props);
+ return device_create_managed_software_node(i2c_dev, props, NULL);
}
static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
diff --git a/sound/soc/intel/boards/sof_sdw_rt711.c
b/sound/soc/intel/boards/sof_sdw_rt711.c
index 04074c09dded9..b7c635c0fadd5 100644
--- a/sound/soc/intel/boards/sof_sdw_rt711.c
+++ b/sound/soc/intel/boards/sof_sdw_rt711.c
@@ -24,19 +24,29 @@
static int rt711_add_codec_device_props(const char *sdw_dev_name)
{
struct property_entry props[MAX_NO_PROPS] = {};
+ struct fwnode_handle *fwnode;
struct device *sdw_dev;
int ret;
+ if (!SOF_RT711_JDSRC(sof_sdw_quirk))
+ return 0;
+
sdw_dev = bus_find_device_by_name(_bus_type, NULL, sdw_dev_name);
if (!sdw_dev)
return -EPROBE_DEFER;
- if (SOF_RT711_JDSRC(sof_sdw_quirk)) {
- props[0] = PROPERTY_ENTRY_U32("realtek,jd-src",
- SOF_RT711_JDSRC(sof_sdw_quirk));
+ props[0] = PROPERTY_ENTRY_U32("realtek,jd-src",
+ SOF_RT711_JDSRC(sof_sdw_quirk));
+
+ fwnode = fwnode_create_software_node(props, NULL);
+ if (IS_ERR(fwnode)) {
+ put_device(sdw_dev);
+ return PTR_ERR(fwnode);
}
- ret = device_add_properties(sdw_dev, props);
+ ret = device_add_software_node(sdw_dev, to_software_node(fwnode));
+
+ fwnode_handle_put(fwnode);
put_device(sdw_dev);
return ret;
@@ -144,7 +154,7 @@ int sof_sdw_rt711_exit(struct device *dev, struct
snd_soc_dai_link *dai_link)
if (!sdw_dev)
return -EINVAL;
- device_remove_properties(sdw_dev);
+ device_remove_software_node(sdw_dev);
put_device(sdw_dev);
return 0;
diff --git a/sound/soc/intel/boards/sof_sdw_rt711_sdca.c
Re: [PATCH v3 4/7] ASoC: codecs: wcd938x: add basic controls
+static int wcd938x_ear_pa_put_gain(struct snd_kcontrol *kcontrol,
+ struct snd_ctl_elem_value *ucontrol)
+{
+ struct snd_soc_component *component =
snd_soc_kcontrol_component(kcontrol);
+ struct wcd938x_sdw_priv *wcd = snd_soc_component_get_drvdata(component);
+ struct wcd938x_priv *wcd938x = wcd->wcd938x;
+
+ if (wcd938x->comp1_enable) {
+ dev_err(component->dev, "Can not set EAR PA Gain, compander1 is
enabled\n");
+ return -EINVAL;
+ }
+
+ snd_soc_component_write_field(component, WCD938X_ANA_EAR_COMPANDER_CTL,
+ WCD938X_EAR_GAIN_MASK,
+ ucontrol->value.integer.value[0]);
+
+ return 0;
that goes back to my other comments, the earpiece is connected to the RX
interface, so what component would be used to set the gain here? the TX
one? But what tells you this component is active and ready to support
commands?
Re: [PATCH v3 0/7] ASoC: codecs: add wcd938x support
On 3/19/21 4:29 AM, Srinivas Kandagatla wrote: This patchset adds support for Qualcomm WCD938X codec. Qualcomm WCD9380/WCD9385 Codec is a standalone Hi-Fi audio codec IC connected over SoundWire. This device has two SoundWire devices, RX and TX respectively supporting 4 x ADCs, ClassH, Ear, Aux PA, 2xHPH, 7 x TX diff inputs, 8 DMICs and MBHC. Even though this device has two SoundWire devices, only tx device has access to main codec Control/Status Registers! That part is a new concept we haven't seen so far with SoundWire support, and I added a number of comments in the patches. It would really help if you could add more explanations on how regmap/pm_runtime/gpios/regulators/interrupts are supposed to work with such a functional split. Thanks! This patchset along with other SoundWire patches on the list have been tested on SM8250 MTP device. Am planning to send support for MBHC once this driver gets accepted! Thanks, srini Many thanks for reviewing v2. Changes since v2: - fixed dt_binding_check error Srinivas Kandagatla (7): ASoC: dt-bindings: wcd938x: add bindings for wcd938x ASoC: codecs: wcd-clsh: add new version support ASoC: codecs: wcd938x: add basic driver ASoC: codecs: wcd938x: add basic controls ASoC: codecs: wcd938x: add playback dapm widgets ASoC: codecs: wcd938x: add capture dapm widgets ASoC: codecs: wcd938x: add audio routing .../bindings/sound/qcom,wcd938x.yaml | 165 + sound/soc/codecs/Kconfig |9 + sound/soc/codecs/Makefile |2 + sound/soc/codecs/wcd-clsh-v2.c| 350 +- sound/soc/codecs/wcd-clsh-v2.h| 16 + sound/soc/codecs/wcd938x-sdw.c| 291 ++ sound/soc/codecs/wcd938x.c| 3623 + sound/soc/codecs/wcd938x.h| 676 +++ 8 files changed, 5122 insertions(+), 10 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/qcom,wcd938x.yaml create mode 100644 sound/soc/codecs/wcd938x-sdw.c create mode 100644 sound/soc/codecs/wcd938x.c create mode 100644 sound/soc/codecs/wcd938x.h
Re: [PATCH v3 7/7] ASoC: codecs: wcd938x: add audio routing
On 3/19/21 4:29 AM, Srinivas Kandagatla wrote:
This patch adds audio routing for both playback and capture.
Signed-off-by: Srinivas Kandagatla
---
sound/soc/codecs/wcd938x.c | 97 ++
1 file changed, 97 insertions(+)
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index 31e3cf729568..0f801920ebac 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -3153,6 +3153,99 @@ static const struct snd_soc_dapm_widget
wcd938x_rx_dapm_widgets[] = {
};
+static const struct snd_soc_dapm_route wcd938x_rx_audio_map[] = {
+ {"IN1_HPHL", NULL, "VDD_BUCK"},
+ {"IN1_HPHL", NULL, "CLS_H_PORT"},
+
+ {"RX1", NULL, "IN1_HPHL"},
+ {"RX1", NULL, "RXCLK"},
+ {"RDAC1", NULL, "RX1"},
+ {"HPHL_RDAC", "Switch", "RDAC1"},
+ {"HPHL PGA", NULL, "HPHL_RDAC"},
+ {"HPHL", NULL, "HPHL PGA"},
+
+ {"IN2_HPHR", NULL, "VDD_BUCK"},
+ {"IN2_HPHR", NULL, "CLS_H_PORT"},
+ {"RX2", NULL, "IN2_HPHR"},
+ {"RDAC2", NULL, "RX2"},
+ {"RX2", NULL, "RXCLK"},
+ {"HPHR_RDAC", "Switch", "RDAC2"},
+ {"HPHR PGA", NULL, "HPHR_RDAC"},
+ {"HPHR", NULL, "HPHR PGA"},
+
+ {"IN3_AUX", NULL, "VDD_BUCK"},
+ {"IN3_AUX", NULL, "CLS_H_PORT"},
+ {"RX3", NULL, "IN3_AUX"},
+ {"RDAC4", NULL, "RX3"},
+ {"RX3", NULL, "RXCLK"},
+ {"AUX_RDAC", "Switch", "RDAC4"},
+ {"AUX PGA", NULL, "AUX_RDAC"},
+ {"AUX", NULL, "AUX PGA"},
+
+ {"RDAC3_MUX", "RX3", "RX3"},
+ {"RDAC3_MUX", "RX1", "RX1"},
+ {"RDAC3", NULL, "RDAC3_MUX"},
+ {"EAR_RDAC", "Switch", "RDAC3"},
+ {"EAR PGA", NULL, "EAR_RDAC"},
+ {"EAR", NULL, "EAR PGA"},
+};
+
+static const struct snd_soc_dapm_route wcd938x_audio_map[] = {
+ {"ADC1_OUTPUT", NULL, "ADC1_MIXER"},
+ {"ADC1_MIXER", "Switch", "ADC1 REQ"},
+ {"ADC1 REQ", NULL, "ADC1"},
+ {"ADC1", NULL, "AMIC1"},
+
+ {"ADC2_OUTPUT", NULL, "ADC2_MIXER"},
+ {"ADC2_MIXER", "Switch", "ADC2 REQ"},
+ {"ADC2 REQ", NULL, "ADC2"},
+ {"ADC2", NULL, "HDR12 MUX"},
+ {"HDR12 MUX", "NO_HDR12", "ADC2 MUX"},
+ {"HDR12 MUX", "HDR12", "AMIC1"},
+ {"ADC2 MUX", "INP3", "AMIC3"},
+ {"ADC2 MUX", "INP2", "AMIC2"},
+
+ {"ADC3_OUTPUT", NULL, "ADC3_MIXER"},
+ {"ADC3_MIXER", "Switch", "ADC3 REQ"},
+ {"ADC3 REQ", NULL, "ADC3"},
+ {"ADC3", NULL, "HDR34 MUX"},
+ {"HDR34 MUX", "NO_HDR34", "ADC3 MUX"},
+ {"HDR34 MUX", "HDR34", "AMIC5"},
+ {"ADC3 MUX", "INP4", "AMIC4"},
+ {"ADC3 MUX", "INP6", "AMIC6"},
+
+ {"ADC4_OUTPUT", NULL, "ADC4_MIXER"},
+ {"ADC4_MIXER", "Switch", "ADC4 REQ"},
+ {"ADC4 REQ", NULL, "ADC4"},
+ {"ADC4", NULL, "ADC4 MUX"},
+ {"ADC4 MUX", "INP5", "AMIC5"},
+ {"ADC4 MUX", "INP7", "AMIC7"},
+
+ {"DMIC1_OUTPUT", NULL, "DMIC1_MIXER"},
+ {"DMIC1_MIXER", "Switch", "DMIC1"},
+
+ {"DMIC2_OUTPUT", NULL, "DMIC2_MIXER"},
+ {"DMIC2_MIXER", "Switch", "DMIC2"},
+
+ {"DMIC3_OUTPUT", NULL, "DMIC3_MIXER"},
+ {"DMIC3_MIXER", "Switch", "DMIC3"},
+
+ {"DMIC4_OUTPUT", NULL, "DMIC4_MIXER"},
+ {"DMIC4_MIXER", "Switch", "DMIC4"},
+
+ {"DMIC5_OUTPUT", NULL, "DMIC5_MIXER"},
+ {"DMIC5_MIXER", "Switch", "DMIC5"},
+
+ {"DMIC6_OUTPUT", NULL, "DMIC6_MIXER"},
+ {"DMIC6_MIXER", "Switch", "DMIC6"},
+
+ {"DMIC7_OUTPUT", NULL, "DMIC7_MIXER"},
+ {"DMIC7_MIXER", "Switch", "DMIC7"},
+
+ {"DMIC8_OUTPUT", NULL, "DMIC8_MIXER"},
+ {"DMIC8_MIXER", "Switch", "DMIC8"},
+};
And last comment that shows I am at a loss on how this is supposed to
work: how would sidetone be handled? This is functionality that needs
both playback and capture to be working, but if they are split into two
separate spaces with only the TX handling commands then what happens?
+
static int wcd938x_get_micb_vout_ctl_val(u32 micb_mv)
{
/* min micbias voltage is 1V and maximum is 2.85V */
@@ -3332,6 +3425,8 @@ static const struct snd_soc_component_driver
soc_codec_dev_wcd938x_sdw_rx = {
.num_controls = ARRAY_SIZE(wcd938x_rx_snd_controls),
.dapm_widgets = wcd938x_rx_dapm_widgets,
.num_dapm_widgets = ARRAY_SIZE(wcd938x_rx_dapm_widgets),
+ .dapm_routes = wcd938x_rx_audio_map,
+ .num_dapm_routes = ARRAY_SIZE(wcd938x_rx_audio_map),
};
static const struct snd_soc_component_driver soc_codec_dev_wcd938x_sdw_tx = {
@@ -3341,6 +3436,8 @@ static const struct snd_soc_component_driver
soc_codec_dev_wcd938x_sdw_tx = {
.num_controls = ARRAY_SIZE(wcd938x_snd_controls),
.dapm_widgets = wcd938x_dapm_widgets,
.num_dapm_widgets = ARRAY_SIZE(wcd938x_dapm_widgets),
+ .dapm_routes = wcd938x_audio_map,
+ .num_dapm_routes = ARRAY_SIZE(wcd938x_audio_map),
};
static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_priv *wcd)
Re: [PATCH v3 3/7] ASoC: codecs: wcd938x: add basic driver
On 3/19/21 4:29 AM, Srinivas Kandagatla wrote:
This patch adds basic SoundWire codec driver to support for
WCD938X TX and RX devices.
It took me a while to figure out that you are adding support for a codec
that has 2 Slave interfaces internally, one for TX and one for RX dais.
Each of the interfaces will appear as a separate Linux device, even
though they are physically in the same hardware component.
That perfectly legit from a SoundWire standpoint, but I wonder how you
are going to handle pm_runtime and regmap access if the two parts are
joined at the hip for imp-def register access (described in the cover
letter as "Even though this device has two SoundWire devices, only tx
device has access to main codec Control/Status Registers!").
I was clearly unable to figure out how regmap/gpios/regulator were
handled between the two TX and TX parts.
See more below.
diff --git a/sound/soc/codecs/wcd938x-sdw.c b/sound/soc/codecs/wcd938x-sdw.c
new file mode 100644
index ..ca29793b0972
--- /dev/null
+++ b/sound/soc/codecs/wcd938x-sdw.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0
GPL-2.0-only for consistency with the other additions below?
+static int wcd9380_probe(struct sdw_slave *pdev,
+const struct sdw_device_id *id)
+{
+ struct device *dev = >dev;
+ struct wcd938x_sdw_priv *wcd;
+ const char *dir = "rx";
+ int ret;
+
+ wcd = devm_kzalloc(dev, sizeof(*wcd), GFP_KERNEL);
+ if (!wcd)
+ return -ENOMEM;
+
+ of_property_read_string(dev->of_node, "direction", );
+ if (!strcmp(dir, "tx"))
+ wcd->is_tx = true;
+ else
+ wcd->is_tx = false;
+
+
extra line
+ ret = of_property_read_variable_u32_array(dev->of_node,
"qcom,port-mapping",
+ wcd->port_map,
+ WCD938X_MAX_TX_SWR_PORTS,
+ WCD938X_MAX_SWR_PORTS);
+ if (ret)
+ dev_info(dev, "Static Port mapping not specified\n");
+
+ wcd->sdev = pdev;
+ dev_set_drvdata(dev, wcd);
+ ret = wcd938x_init(wcd);
+ if (ret)
+ return ret;
+
+ pdev->prop.scp_int1_mask = SDW_SCP_INT1_IMPL_DEF |
+ SDW_SCP_INT1_BUS_CLASH |
+ SDW_SCP_INT1_PARITY;
+ pdev->prop.lane_control_support = true;
+ if (wcd->is_tx) {
+ pdev->prop.source_ports = GENMASK(WCD938X_MAX_SWR_PORTS, 0);
+ pdev->prop.src_dpn_prop = wcd938x_dpn_prop;
+ wcd->ch_info = _sdw_tx_ch_info[0];
+ pdev->prop.wake_capable = true;
+ } else {
+ pdev->prop.sink_ports = GENMASK(WCD938X_MAX_SWR_PORTS, 0);
+ pdev->prop.sink_dpn_prop = wcd938x_dpn_prop;
+ wcd->ch_info = _sdw_rx_ch_info[0];
+ }
+
+ if (wcd->is_tx)
+ return wcd938x_register_component(wcd, dev, _tx_dai);
+ else
+ return wcd938x_register_component(wcd, dev, _rx_dai);
+
+}
+
+static const struct sdw_device_id wcd9380_slave_id[] = {
+ SDW_SLAVE_ENTRY(0x0217, 0x10d, 0),
does this mean you cannot determine the functionality of the device by
looking at the devId registers?
I would have expected each interface to have a different part ID to show
that they are different...such tricks would not work in the ACPI world
at the moment, the expectation was really that different part numbers
are unique enough to know what to expect.
+ {},
+};
+MODULE_DEVICE_TABLE(sdw, wcd9380_slave_id);
+
+static struct sdw_driver wcd9380_codec_driver = {
+ .probe = wcd9380_probe,
+ .ops = _slave_ops,
+ .id_table = wcd9380_slave_id,
+ .driver = {
+ .name = "wcd9380-codec",
+ }
+};
+module_sdw_driver(wcd9380_codec_driver);
[...]
+static bool wcd938x_readable_register(struct device *dev, unsigned int reg)
+{
+ bool ret;
+
+ ret = wcd938x_readonly_register(dev, reg);
+ if (!ret)
+ return wcd938x_rdwr_register(dev, reg);
+
+ return ret;
+}
+
+static bool wcd938x_writeable_register(struct device *dev, unsigned int reg)
+{
+ return wcd938x_rdwr_register(dev, reg);
+}
+
+static bool wcd938x_volatile_register(struct device *dev, unsigned int reg)
+{
+ if (reg <= WCD938X_BASE_ADDRESS)
+ return 0;
+
+ if (reg == WCD938X_DIGITAL_SWR_TX_CLK_RATE)
+ return true;
+
+ if (wcd938x_readonly_register(dev, reg))
+ return true;
+
+ return false;
+}
this part is quite confusing, you mentioned that only the TX interface
has access to registers, but here you seem to expose regmap registers
for both TX and RX?
+
+static struct regmap_config wcd938x_regmap_config = {
+ .name = "wcd938x_csr",
+ .reg_bits = 32,
+
Re: [PATCH v3 2/7] ASoC: codecs: wcd-clsh: add new version support
+static void wcd_clsh_v3_set_hph_mode(struct snd_soc_component *component,
+ int mode)
+{
+ u8 val = 0;
initialization not needed.
+
+ switch (mode) {
+ case CLS_H_NORMAL:
+ val = 0x00;
+ break;
+ case CLS_AB:
+ case CLS_H_ULP:
+ val = 0x0C;
+ break;
+ case CLS_AB_HIFI:
+ case CLS_H_HIFI:
+ val = 0x08;
+ break;
+ case CLS_H_LP:
+ case CLS_H_LOHIFI:
+ case CLS_AB_LP:
+ case CLS_AB_LOHIFI:
+ val = 0x04;
+ break;
+ default:
+ dev_err(component->dev, "%s:Invalid mode %d\n", __func__, mode);
+ return;
+ };
+
+ snd_soc_component_update_bits(component, WCD9XXX_ANA_HPH, 0x0C, val);
+}
+
+
extra line
+void wcd_clsh_set_hph_mode(struct wcd_clsh_ctrl *ctrl, int mode)
+{
+ struct snd_soc_component *comp = ctrl->comp;
+
+ if (ctrl->codec_version >= WCD937X)
+ wcd_clsh_v3_set_hph_mode(comp, mode);
+ else
+ wcd_clsh_v2_set_hph_mode(comp, mode);
+
+}
+
static void wcd_clsh_set_flyback_current(struct snd_soc_component *comp,
int mode)
{
@@ -289,6 +388,130 @@ static void wcd_clsh_set_buck_regulator_mode(struct
snd_soc_component *comp,
WCD9XXX_A_ANA_RX_REGULATOR_MODE_CLS_H);
}
+static void wcd_clsh_v3_set_buck_regulator_mode(struct snd_soc_component *component,
+ int mode)
+{
+ snd_soc_component_update_bits(component, WCD9XXX_ANA_RX_SUPPLIES,
+ 0x02, 0x00);
+}
+
+static inline void wcd_clsh_v3_set_flyback_mode(struct snd_soc_component
*component,
+ int mode)
+{
+ if (mode == CLS_H_HIFI || mode == CLS_H_LOHIFI ||
+ mode == CLS_AB_HIFI || mode == CLS_AB_LOHIFI) {
+ snd_soc_component_update_bits(component,
+ WCD9XXX_ANA_RX_SUPPLIES,
+ 0x04, 0x04);
+ snd_soc_component_update_bits(component,
+ WCD9XXX_FLYBACK_VNEG_CTRL_4,
+ 0xF0, 0x80);
+ } else {
+ snd_soc_component_update_bits(component,
+ WCD9XXX_ANA_RX_SUPPLIES,
+ 0x04, 0x00); /* set to Default */
+ snd_soc_component_update_bits(component,
+ WCD9XXX_FLYBACK_VNEG_CTRL_4,
+ 0xF0, 0x70);
+ }
+}
+
+static inline void wcd_clsh_v3_force_iq_ctl(struct snd_soc_component
*component,
+int mode, bool enable)
+{
+ if (enable) {
+ snd_soc_component_update_bits(component,
+ WCD9XXX_FLYBACK_VNEGDAC_CTRL_2,
+ 0xE0, 0xA0);
+ /* 100usec delay is needed as per HW requirement */
+ usleep_range(100, 110);
+ snd_soc_component_update_bits(component,
+ WCD9XXX_CLASSH_MODE_3,
+ 0x02, 0x02);
+ snd_soc_component_update_bits(component,
+ WCD9XXX_CLASSH_MODE_2,
+ 0xFF, 0x1C);
+ if (mode == CLS_H_LOHIFI || mode == CLS_AB_LOHIFI) {
+ snd_soc_component_update_bits(component,
+ WCD9XXX_HPH_NEW_INT_PA_MISC2,
+ 0x20, 0x20);
+ snd_soc_component_update_bits(component,
+ WCD9XXX_RX_BIAS_HPH_LOWPOWER,
+ 0xF0, 0xC0);
+ snd_soc_component_update_bits(component,
+ WCD9XXX_HPH_PA_CTL1,
+ 0x0E, 0x02);
+ }
+ } else {
+ snd_soc_component_update_bits(component,
+ WCD9XXX_HPH_NEW_INT_PA_MISC2,
+ 0x20, 0x00);
+ snd_soc_component_update_bits(component,
+ WCD9XXX_RX_BIAS_HPH_LOWPOWER,
+ 0xF0, 0x80);
+ snd_soc_component_update_bits(component,
+ WCD9XXX_HPH_PA_CTL1,
+ 0x0E, 0x06);
+ }
+}
do you need the inline for the two functions above?
Re: [PATCH v3] ASoC: Intel: sof_rt5682: Add ALC1015Q-VB speaker amp support
On 3/17/21 8:21 PM, Jack Yu wrote: This patch adds jsl_rt5682_rt1015p which supports the RT5682 headset codec and ALC1015Q-VB speaker amplifier combination on JasperLake platform. This driver also supports ALC1015Q-CG if running in auto-mode. Following table shows the audio interface support of the two amplifiers. | ALC1015Q-CG | ALC1015Q-VB = I2C | Yes | No Auto-mode | 48K, 64fs | 16k, 32fs | 48k, 32fs | 48k, 64fs Signed-off-by: Brent Lu The code is looks fine, but Jack Yu added a separate patch that makes RTL1019 equivalent to RTL1015, so should this patch also handle the RTL1019 case? For rt1019 non-i2c mode (auto mode), it uses the sdb pin to enable amp, the same as rt1015 non-i2c mode, therefore we propose rt1019(auto mode) to use rt1015p instead of adding a new driver for it. ok, that's fine. Acked-by: Pierre-Louis Bossart
Re: [PATCH v3] ASoC: Intel: sof_rt5682: Add ALC1015Q-VB speaker amp support
On 3/17/21 6:08 AM, Brent Lu wrote: This patch adds jsl_rt5682_rt1015p which supports the RT5682 headset codec and ALC1015Q-VB speaker amplifier combination on JasperLake platform. This driver also supports ALC1015Q-CG if running in auto-mode. Following table shows the audio interface support of the two amplifiers. | ALC1015Q-CG | ALC1015Q-VB = I2C | Yes | No Auto-mode | 48K, 64fs | 16k, 32fs | 48k, 32fs | 48k, 64fs Signed-off-by: Brent Lu The code is looks fine, but Jack Yu added a separate patch that makes RTL1019 equivalent to RTL1015, so should this patch also handle the RTL1019 case?
Re: [PATCH v3] ASoC: amd: add support for rt5682 codec in machine driver
On 3/16/21 8:37 AM, Mukunda,Vijendar wrote:
On 15/03/21 9:30 pm, Pierre-Louis Bossart wrote:
+static int rt5682_clk_enable(struct snd_pcm_substream *substream)
+{
+ int ret;
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+
+ /*
+ * Set wclk to 48000 because the rate constraint of this driver is
+ * 48000. ADAU7002 spec: "The ADAU7002 requires a BCLK rate that is
+ * minimum of 64x the LRCLK sample rate." RT5682 is the only clk
+ * source so for all codecs we have to limit bclk to 64X lrclk.
+ */
+ clk_set_rate(rt5682_dai_wclk, 48000);
+ clk_set_rate(rt5682_dai_bclk, 48000 * 64);
+ ret = clk_prepare_enable(rt5682_dai_bclk);
+ if (ret < 0) {
+ dev_err(rtd->dev, "can't enable master clock %d\n", ret);
+ return ret;
+ }
+ return ret;
+}
Out of curiosity, is there a reason why you use clk_prepare_enable()
for the bclk but not for the wclk?These changes were shared by codec
vendor as an initial patch.
We should use clk_prepare_enable() for wclk not for bclk.
We will update and share the new patch.
Well the question remains: if you have two clocks and only enable one,
why do you need to get two clocks.
Also this patch was modeled after the da7219 case, where the same open
applies.
Re: [PATCH v2] ASoC: Intel: sof_rt5682: Add ALC1015Q-VB speaker amp support
On 3/16/21 4:46 AM, Brent Lu wrote:
This patch adds jsl_rt5682_rt1015p which supports the RT5682 headset
codec and ALC1015Q-VB speaker amplifier combination on JasperLake
platform.
This driver also applies for ALC1015Q-CG running in auto-mode.
Signed-off-by: Brent Lu
---
sound/soc/intel/boards/Kconfig| 1 +
sound/soc/intel/boards/sof_realtek_common.c | 99 +++
sound/soc/intel/boards/sof_realtek_common.h | 7 ++
sound/soc/intel/boards/sof_rt5682.c | 19 +++-
.../intel/common/soc-acpi-intel-jsl-match.c | 13 +++
5 files changed, 137 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index d1d28129a32b..58379393b8e4 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -457,6 +457,7 @@ config SND_SOC_INTEL_SOF_RT5682_MACH
select SND_SOC_MAX98373_I2C
select SND_SOC_RT1011
select SND_SOC_RT1015
+ select SND_SOC_RT1015P
select SND_SOC_RT5682_I2C
select SND_SOC_DMIC
select SND_SOC_HDAC_HDMI
diff --git a/sound/soc/intel/boards/sof_realtek_common.c
b/sound/soc/intel/boards/sof_realtek_common.c
index f3cf73c620ba..5dd0eb438aa3 100644
--- a/sound/soc/intel/boards/sof_realtek_common.c
+++ b/sound/soc/intel/boards/sof_realtek_common.c
@@ -7,6 +7,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -136,3 +137,101 @@ void sof_rt1011_codec_conf(struct snd_soc_card *card)
card->codec_conf = rt1011_codec_confs;
card->num_configs = ARRAY_SIZE(rt1011_codec_confs);
}
+
+/*
+ * rt1015: i2c mode driver for ALC1015 and ALC1015Q
+ * rt1015p: auto-mode driver for ALC1015, ALC1015Q, and ALC1015Q-VB
+ */
+static const struct snd_soc_dapm_route rt1015p_1dev_dapm_routes[] = {
+ /* speaker */
+ { "Left Spk", NULL, "Speaker" },
+ { "Right Spk", NULL, "Speaker" },
+};
+
+static const struct snd_soc_dapm_route rt1015p_2dev_dapm_routes[] = {
+ /* speaker */
+ { "Left Spk", NULL, "Left Speaker" },
+ { "Right Spk", NULL, "Right Speaker" },
+};
I am confused by these routes...
is this a result of using the codec confs below only when there are 2
amps with their own enable pin?
You still have 2 amps even in the 1dev case, so I want to make sure the
code has enough comments so that we don't lose track of the design.
The rest of the code looks fine.
+static struct snd_soc_codec_conf rt1015p_codec_confs[] = {
+ {
+ .dlc = COMP_CODEC_CONF(RT1015P_DEV0_NAME),
+ .name_prefix = "Left",
+ },
+ {
+ .dlc = COMP_CODEC_CONF(RT1015P_DEV1_NAME),
+ .name_prefix = "Right",
+ },
+};
+
+static struct snd_soc_dai_link_component rt1015p_dai_link_components[] = {
+ {
+ .name = RT1015P_DEV0_NAME,
+ .dai_name = RT1015P_CODEC_DAI,
+ },
+ {
+ .name = RT1015P_DEV1_NAME,
+ .dai_name = RT1015P_CODEC_DAI,
+ },
+};
+
+static int rt1015p_get_num_codecs(void)
+{
+ static int dev_num;
+
+ if (dev_num)
+ return dev_num;
+
+ if (!acpi_dev_present("RTL1015", "1", -1))
+ dev_num = 1;
+ else
+ dev_num = 2;
+
+ return dev_num;
+}
+
+static int rt1015p_hw_params(struct snd_pcm_substream *substream,
+struct snd_pcm_hw_params *params)
+{
+ /* reserved for debugging purpose */
+
+ return 0;
+}
+
+static const struct snd_soc_ops rt1015p_ops = {
+ .hw_params = rt1015p_hw_params,
+};
+
+static int rt1015p_init(struct snd_soc_pcm_runtime *rtd)
+{
+ struct snd_soc_card *card = rtd->card;
+ int ret;
+
+ if (rt1015p_get_num_codecs() == 1)
+ ret = snd_soc_dapm_add_routes(>dapm,
rt1015p_1dev_dapm_routes,
+
ARRAY_SIZE(rt1015p_1dev_dapm_routes));
+ else
+ ret = snd_soc_dapm_add_routes(>dapm,
rt1015p_2dev_dapm_routes,
+
ARRAY_SIZE(rt1015p_2dev_dapm_routes));
+ if (ret)
+ dev_err(rtd->dev, "Speaker map addition failed: %d\n", ret);
+ return ret;
+}
+
+void sof_rt1015p_dai_link(struct snd_soc_dai_link *link)
+{
+ link->codecs = rt1015p_dai_link_components;
+ link->num_codecs = rt1015p_get_num_codecs();
+ link->init = rt1015p_init;
+ link->ops = _ops;
+}
+
+void sof_rt1015p_codec_conf(struct snd_soc_card *card)
+{
+ if (rt1015p_get_num_codecs() == 1)
+ return;
+
+ card->codec_conf = rt1015p_codec_confs;
+ card->num_configs = ARRAY_SIZE(rt1015p_codec_confs);
+}
diff --git a/sound/soc/intel/boards/sof_realtek_common.h
b/sound/soc/intel/boards/sof_realtek_common.h
index 87cb3812b926..cb0b49b2855c 100644
--- a/sound/soc/intel/boards/sof_realtek_common.h
+++ b/sound/soc/intel/boards/sof_realtek_common.h
Re: [PATCH v4 0/5] soundwire: add static port map support
On 3/15/21 11:56 AM, Srinivas Kandagatla wrote: In some cases, SoundWire device ports are statically mapped to Controller ports during design, however there is no way to expose this information to the controller. Controllers like Qualcomm ones use this info to setup static bandwidth parameters for those ports. A generic port allocation is not possible in this cases! This patch adds a new member m_port_map to SoundWire device so that it can populate the static master port map and share it with controller to be able to setup correct bandwidth parameters. As a user of this feature this patchset also adds new bindings for wsa881x smart speaker which has 4 ports which are statically mapped to the 3 output and 1 input port of the controller. Tested it on DB845c and SM8250 MTP. thanks, srini Reviewed-by: Pierre-Louis Bossart Changes since v3: - updated kernel doc for more clarity on m_port_map Srinivas Kandagatla (5): soundwire: add static port mapping support soundwire: qcom: update port map allocation bit mask soundwire: qcom: add static port map support ASoC: dt-bindings: wsa881x: add bindings for port mapping ASoC: codecs: wsa881x: add static port map support .../bindings/sound/qcom,wsa881x.yaml | 9 ++ drivers/soundwire/qcom.c | 31 +++ include/linux/soundwire/sdw.h | 2 ++ sound/soc/codecs/wsa881x.c| 7 + 4 files changed, 43 insertions(+), 6 deletions(-)
Re: [PATCH v3] ASoC: amd: add support for rt5682 codec in machine driver
+static int rt5682_clk_enable(struct snd_pcm_substream *substream)
+{
+ int ret;
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+
+ /*
+* Set wclk to 48000 because the rate constraint of this driver is
+* 48000. ADAU7002 spec: "The ADAU7002 requires a BCLK rate that is
+* minimum of 64x the LRCLK sample rate." RT5682 is the only clk
+* source so for all codecs we have to limit bclk to 64X lrclk.
+*/
+ clk_set_rate(rt5682_dai_wclk, 48000);
+ clk_set_rate(rt5682_dai_bclk, 48000 * 64);
+ ret = clk_prepare_enable(rt5682_dai_bclk);
+ if (ret < 0) {
+ dev_err(rtd->dev, "can't enable master clock %d\n", ret);
+ return ret;
+ }
+ return ret;
+}
Out of curiosity, is there a reason why you use clk_prepare_enable() for
the bclk but not for the wclk?
Re: [PATCH] ASoC: Intel: sof_rt5682: Add rt1015p speaker amp support
I am not a big fan of the code partition you've selected.
+void sof_rt1015p_set_share_en_spk(void)
+{
+ /* Two amps share one en pin so there is only one device in acpi
+* table
+*/
+ rt1015p_quirk |= RT1015P_SHARE_EN_SPK;
+}
This is a function now used in the machine driver, see below [1]
This adds a new interface between machine and realtek common code, which
we are trying to move to a module with well-defined APIs.
+void sof_rt1015p_dai_link(struct snd_soc_dai_link *link)
+{
+ link->codecs = rt1015p_dai_link_components;
+ if (rt1015p_quirk & RT1015P_SHARE_EN_SPK)
+ link->num_codecs = 1;
+ else
+ link->num_codecs = ARRAY_SIZE(rt1015p_dai_link_components);
+ link->init = rt1015p_init;
+ link->ops = _ops;
+}
+
+void sof_rt1015p_codec_conf(struct snd_soc_card *card)
+{
+ if (rt1015p_quirk & RT1015P_SHARE_EN_SPK)
+ return;
+
+ card->codec_conf = rt1015p_codec_confs;
+ card->num_configs = ARRAY_SIZE(rt1015p_codec_confs);
+}
could we not handle this quirk inside one of the two dai_link or
codec_conf functions above?
The machine driver does not care about this RT1015P_SHARE_EN_SPK quirk,
it's only used in those two functions so should be set in this scope.
There's no need to make it visible to the machine driver, is there?
+ /* setup amp quirk if any */
+ if (sof_rt5682_quirk & SOF_RT1015P_SPEAKER_AMP_PRESENT) {
+ /* There may be only one device instance if two amps
+* sharing one en pin
+*/
+ if (!acpi_dev_present("RTL1015", "1", -1)) {
+ dev_dbg(>dev, "Device %s not exist\n",
+ RT1015P_DEV1_NAME);
+ sof_rt1015p_set_share_en_spk();
+ }
+ }
+
[1]
I see no problem using the _UID (Unique ID) to check if a second
amplifier is present or not. This seems to follow the ACPI spec Section
6.1.12, as long as "the _UID must be unique across all devices with
either a common _HID or _CID"
+ else if (sof_rt5682_quirk & SOF_RT1015P_SPEAKER_AMP_PRESENT)
+ sof_rt1015p_codec_conf(_audio_card_rt5682);
[PATCH 21/23] ASoC: tas2770: remove useless initialization
cppcheck warning:
sound/soc/codecs/tas2770.c:109:10: style: Variable 'ret' is assigned a
value that is never used. [unreadVariable]
int ret = 0;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/tas2770.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tas2770.c b/sound/soc/codecs/tas2770.c
index 15fca5109e14..781bf9cc4faa 100644
--- a/sound/soc/codecs/tas2770.c
+++ b/sound/soc/codecs/tas2770.c
@@ -106,7 +106,7 @@ static int tas2770_codec_suspend(struct snd_soc_component
*component)
static int tas2770_codec_resume(struct snd_soc_component *component)
{
struct tas2770_priv *tas2770 = snd_soc_component_get_drvdata(component);
- int ret = 0;
+ int ret;
if (tas2770->sdz_gpio) {
gpiod_set_value_cansleep(tas2770->sdz_gpio, 1);
--
2.25.1
[PATCH 20/23] ASoC: tas2562: remove warning on return value
cppcheck warning: sound/soc/codecs/tas2562.c:530:9: warning: Identical condition and return expression 'ret', return value is always 0 [identicalConditionAfterEarlyExit] return ret; ^ sound/soc/codecs/tas2562.c:525:6: note: If condition 'ret' is true, the function will return/exit if (ret) ^ sound/soc/codecs/tas2562.c:530:9: note: Returning identical expression 'ret' return ret; ^ Fix with return 0 Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/tas2562.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/codecs/tas2562.c b/sound/soc/codecs/tas2562.c index ba23f9f07f04..10302552195e 100644 --- a/sound/soc/codecs/tas2562.c +++ b/sound/soc/codecs/tas2562.c @@ -526,7 +526,7 @@ static int tas2562_volume_control_put(struct snd_kcontrol *kcontrol, tas2562->volume_lvl = ucontrol->value.integer.value[0]; - return ret; + return 0; } /* Digital Volume Control. From 0 dB to -110 dB in 1 dB steps */ -- 2.25.1
[PATCH 23/23] ASoC: tscs454: remove useless test on PLL disable
cppcheck warning:
sound/soc/codecs/tscs454.c:730:37: style: Same value in both branches
of ternary operator. [duplicateValueTernary]
val = pll1 ? FV_PLL1CLKEN_DISABLE : FV_PLL2CLKEN_DISABLE;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/tscs454.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tscs454.c b/sound/soc/codecs/tscs454.c
index 1bafc9d1101c..43220bb36701 100644
--- a/sound/soc/codecs/tscs454.c
+++ b/sound/soc/codecs/tscs454.c
@@ -727,7 +727,12 @@ static int pll_power_event(struct snd_soc_dapm_widget *w,
if (enable)
val = pll1 ? FV_PLL1CLKEN_ENABLE : FV_PLL2CLKEN_ENABLE;
else
- val = pll1 ? FV_PLL1CLKEN_DISABLE : FV_PLL2CLKEN_DISABLE;
+ /*
+* FV_PLL1CLKEN_DISABLE and FV_PLL2CLKEN_DISABLE are
+* identical zero vzalues, there is no need to test
+* the PLL index
+*/
+ val = FV_PLL1CLKEN_DISABLE;
ret = snd_soc_component_update_bits(component, R_PLLCTL, msk, val);
if (ret < 0) {
--
2.25.1
[PATCH 19/23] ASoC: tas2562: remove useless assignment
cppcheck throws a warning:
sound/soc/codecs/tas2562.c:203:4: style: Assignment of function
parameter has no effect outside the function. [uselessAssignmentArg]
tx_mask &= ~(1 << right_slot);
^
This assignment seems to come from a copy/paste but the value is
indeed not used. Let's remove it.
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/tas2562.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/sound/soc/codecs/tas2562.c b/sound/soc/codecs/tas2562.c
index 19965fabe949..ba23f9f07f04 100644
--- a/sound/soc/codecs/tas2562.c
+++ b/sound/soc/codecs/tas2562.c
@@ -200,7 +200,6 @@ static int tas2562_set_dai_tdm_slot(struct snd_soc_dai *dai,
right_slot = left_slot;
} else {
right_slot = __ffs(tx_mask);
- tx_mask &= ~(1 << right_slot);
}
}
--
2.25.1
[PATCH 22/23] ASoC: tlv320dac33: clarify expression
cppcheck warning: sound/soc/codecs/tlv320dac33.c:1074:43: style: Clarify calculation precedence for '%' and '?'. [clarifyCalculation] (dac33->alarm_threshold % period_size ? ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/tlv320dac33.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c index d905e03aaec7..48572d66cb3b 100644 --- a/sound/soc/codecs/tlv320dac33.c +++ b/sound/soc/codecs/tlv320dac33.c @@ -1071,7 +1071,7 @@ static void dac33_calculate_times(struct snd_pcm_substream *substream, */ dac33->nsample = period_size * ((dac33->alarm_threshold / period_size) + - (dac33->alarm_threshold % period_size ? +((dac33->alarm_threshold % period_size) ? 1 : 0)); else if (period_size > nsample_limit) dac33->nsample = nsample_limit; -- 2.25.1
[PATCH 18/23] ASoC: sti-sas: remove unused struct members
cppcheck warnings:
sound/soc/codecs/sti-sas.c:54:25: style: struct member
'sti_dac_audio::field' is never used. [unusedStructMember]
struct regmap_field **field;
^
sound/soc/codecs/sti-sas.c:55:24: style: struct member
'sti_dac_audio::rst' is never used. [unusedStructMember]
struct reset_control *rst;
^
sound/soc/codecs/sti-sas.c:61:25: style: struct member
'sti_spdif_audio::field' is never used. [unusedStructMember]
struct regmap_field **field;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/sti-sas.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/sound/soc/codecs/sti-sas.c b/sound/soc/codecs/sti-sas.c
index ec9933b054ad..ffdf7e559515 100644
--- a/sound/soc/codecs/sti-sas.c
+++ b/sound/soc/codecs/sti-sas.c
@@ -51,14 +51,11 @@ static const struct reg_default stih407_sas_reg_defaults[]
= {
struct sti_dac_audio {
struct regmap *regmap;
struct regmap *virt_regmap;
- struct regmap_field **field;
- struct reset_control *rst;
int mclk;
};
struct sti_spdif_audio {
struct regmap *regmap;
- struct regmap_field **field;
int mclk;
};
--
2.25.1
[PATCH 14/23] ASoC: mt6359: remove useless assignment
cppcheck warning:
sound/soc/codecs/mt6359.c:242:19: style: Variable 'stage' is assigned a value
that is never used. [unreadVariable]
int i = 0, stage = 0;
^
sound/soc/codecs/mt6359.c:260:19: style: Variable 'stage' is assigned a value
that is never used. [unreadVariable]
int i = 0, stage = 0;
^
sound/soc/codecs/mt6359.c:274:8: style: Variable 'i' is assigned a value that
is never used. [unreadVariable]
int i = 0, stage = 0;
^
sound/soc/codecs/mt6359.c:274:19: style: Variable 'stage' is assigned a value
that is never used. [unreadVariable]
int i = 0, stage = 0;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/mt6359.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/mt6359.c b/sound/soc/codecs/mt6359.c
index 6f4b1da52082..b909b36582b7 100644
--- a/sound/soc/codecs/mt6359.c
+++ b/sound/soc/codecs/mt6359.c
@@ -239,7 +239,7 @@ static void zcd_disable(struct mt6359_priv *priv)
static void hp_main_output_ramp(struct mt6359_priv *priv, bool up)
{
- int i = 0, stage = 0;
+ int i, stage;
int target = 7;
/* Enable/Reduce HPL/R main output stage step by step */
@@ -257,7 +257,7 @@ static void hp_main_output_ramp(struct mt6359_priv *priv,
bool up)
static void hp_aux_feedback_loop_gain_ramp(struct mt6359_priv *priv, bool up)
{
- int i = 0, stage = 0;
+ int i, stage;
int target = 0xf;
/* Enable/Reduce HP aux feedback loop gain step by step */
--
2.25.1
[PATCH 13/23] ASoC: mt6358: remove useless initializations
cppcheck warnings:
sound/soc/codecs/mt6358.c:334:19: style: Variable 'stage' is assigned
a value that is never used. [unreadVariable]
int i = 0, stage = 0;
^
sound/soc/codecs/mt6358.c:350:19: style: Variable 'stage' is assigned
a value that is never used. [unreadVariable]
int i = 0, stage = 0;
^
185/930 files checked 25% done
Checking sound/soc/codecs/mt6359.c ...
sound/soc/codecs/mt6359.c:274:8: style: Variable 'i' is assigned a
value that is never used. [unreadVariable]
int i = 0, stage = 0;
^
sound/soc/codecs/mt6359.c:274:19: style: Variable 'stage' is assigned
a value that is never used. [unreadVariable]
int i = 0, stage = 0;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/mt6358.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/mt6358.c b/sound/soc/codecs/mt6358.c
index 1f39d5998cf6..9b263a9a669d 100644
--- a/sound/soc/codecs/mt6358.c
+++ b/sound/soc/codecs/mt6358.c
@@ -331,7 +331,7 @@ static void hp_zcd_disable(struct mt6358_priv *priv)
static void hp_main_output_ramp(struct mt6358_priv *priv, bool up)
{
- int i = 0, stage = 0;
+ int i, stage;
int target = 7;
/* Enable/Reduce HPL/R main output stage step by step */
@@ -347,7 +347,7 @@ static void hp_main_output_ramp(struct mt6358_priv *priv,
bool up)
static void hp_aux_feedback_loop_gain_ramp(struct mt6358_priv *priv, bool up)
{
- int i = 0, stage = 0;
+ int i, stage;
/* Reduce HP aux feedback loop gain step by step */
for (i = 0; i <= 0xf; i++) {
--
2.25.1
[PATCH 12/23] ASoC: max98090: remove useless assignment
cppcheck warning:
sound/soc/codecs/max98090.c:1835:16: style: Variable 'test_diff' is
assigned a value that is never used. [unreadVariable]
int test_diff = INT_MAX;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/max98090.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 06276ff5f8a3..bc30a1dc7530 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -1832,7 +1832,7 @@ static const struct dmic_table dmic_table[] = { /* One
for each pclk freq. */
static int max98090_find_divisor(int target_freq, int pclk)
{
int current_diff = INT_MAX;
- int test_diff = INT_MAX;
+ int test_diff;
int divisor_index = 0;
int i;
--
2.25.1
[PATCH 15/23] ASoC: nau8825: remove useless assignment
cppcheck warning:
sound/soc/codecs/nau8825.c:2113:10: style: Variable 'ret' is assigned
a value that is never used. [unreadVariable]
int ret = 0;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/nau8825.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
index e19db30c457b..67de0e49ccf4 100644
--- a/sound/soc/codecs/nau8825.c
+++ b/sound/soc/codecs/nau8825.c
@@ -2111,7 +2111,7 @@ static int nau8825_set_pll(struct snd_soc_component
*component, int pll_id, int
static int nau8825_mclk_prepare(struct nau8825 *nau8825, unsigned int freq)
{
- int ret = 0;
+ int ret;
nau8825->mclk = devm_clk_get(nau8825->dev, "mclk");
if (IS_ERR(nau8825->mclk)) {
--
2.25.1
[PATCH 17/23] ASoC: sigmadsp: align function prototype
cppcheck warning: sound/soc/codecs/sigmadsp.c:736:60: style:inconclusive: Function 'sigmadsp_setup' argument 2 names different: declaration 'rate' definition 'samplerate'. [funcArgNamesDifferent] int sigmadsp_setup(struct sigmadsp *sigmadsp, unsigned int samplerate) ^ sound/soc/codecs/sigmadsp.h:62:60: note: Function 'sigmadsp_setup' argument 2 names different: declaration 'rate' definition 'samplerate'. int sigmadsp_setup(struct sigmadsp *sigmadsp, unsigned int rate); ^ sound/soc/codecs/sigmadsp.c:736:60: note: Function 'sigmadsp_setup' argument 2 names different: declaration 'rate' definition 'samplerate'. int sigmadsp_setup(struct sigmadsp *sigmadsp, unsigned int samplerate) ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/sigmadsp.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/codecs/sigmadsp.h b/sound/soc/codecs/sigmadsp.h index e3c9656e006d..d63b8c366efb 100644 --- a/sound/soc/codecs/sigmadsp.h +++ b/sound/soc/codecs/sigmadsp.h @@ -59,7 +59,7 @@ struct sigmadsp *devm_sigmadsp_init_i2c(struct i2c_client *client, int sigmadsp_attach(struct sigmadsp *sigmadsp, struct snd_soc_component *component); -int sigmadsp_setup(struct sigmadsp *sigmadsp, unsigned int rate); +int sigmadsp_setup(struct sigmadsp *sigmadsp, unsigned int samplerate); void sigmadsp_reset(struct sigmadsp *sigmadsp); #endif -- 2.25.1
[PATCH 11/23] ASoC: hdmi-codec: remove unused spk_mask member
fix cppcheck warning:
sound/soc/codecs/hdmi-codec.c:25:16: style: struct member
'hdmi_codec_channel_map_table::spk_mask' is never
used. [unusedStructMember]
unsigned long spk_mask; /* speaker position bit mask */
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/hdmi-codec.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index 83e74ddccf59..1567ba196ab9 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -22,7 +22,6 @@
struct hdmi_codec_channel_map_table {
unsigned char map; /* ALSA API channel map position */
- unsigned long spk_mask; /* speaker position bit mask */
};
/*
--
2.25.1
[PATCH 16/23] ASoC: pcm1681: remove useless assignment
cppcheck warning:
sound/soc/codecs/pcm1681.c:87:8: style: Variable 'i' is assigned a
value that is never used. [unreadVariable]
int i = 0, val = -1, enable = 0;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/pcm1681.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c
index 07ed8fded471..5b78e9299c95 100644
--- a/sound/soc/codecs/pcm1681.c
+++ b/sound/soc/codecs/pcm1681.c
@@ -84,7 +84,7 @@ static const int pcm1681_deemph[] = { 44100, 48000, 32000 };
static int pcm1681_set_deemph(struct snd_soc_component *component)
{
struct pcm1681_private *priv = snd_soc_component_get_drvdata(component);
- int i = 0, val = -1, enable = 0;
+ int i, val = -1, enable = 0;
if (priv->deemph) {
for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++) {
--
2.25.1
[PATCH 07/23] ASoC: da7219-aad: remove useless initialization
cppcheck warning: sound/soc/codecs/da7219-aad.c:118:22: style: Variable 'ret' is assigned a value that is never used. [unreadVariable] int report = 0, ret = 0; ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/da7219-aad.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/codecs/da7219-aad.c b/sound/soc/codecs/da7219-aad.c index 48081d71c22c..7998fdd3b378 100644 --- a/sound/soc/codecs/da7219-aad.c +++ b/sound/soc/codecs/da7219-aad.c @@ -115,7 +115,7 @@ static void da7219_aad_hptest_work(struct work_struct *work) __le16 tonegen_freq_hptest; u8 pll_srm_sts, pll_ctrl, gain_ramp_ctrl, accdet_cfg8; - int report = 0, ret = 0; + int report = 0, ret; /* Lock DAPM, Kcontrols affected by this test and the PLL */ snd_soc_dapm_mutex_lock(dapm); -- 2.25.1
[PATCH 10/23] ASoC: hdmi-codec: remove useless initialization
Fix cppcheck warning:
sound/soc/codecs/hdmi-codec.c:745:5: style: Redundant initialization
for 'cf'. The initialized value is overwritten before it is
read. [redundantInitialization]
cf = dai->playback_dma_data;
^
sound/soc/codecs/hdmi-codec.c:738:31: note: cf is initialized
struct hdmi_codec_daifmt *cf = dai->playback_dma_data;
^
sound/soc/codecs/hdmi-codec.c:745:5: note: cf is overwritten
cf = dai->playback_dma_data;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/hdmi-codec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index 422539f933de..83e74ddccf59 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -735,7 +735,7 @@ static int hdmi_codec_set_jack(struct snd_soc_component
*component,
static int hdmi_dai_spdif_probe(struct snd_soc_dai *dai)
{
- struct hdmi_codec_daifmt *cf = dai->playback_dma_data;
+ struct hdmi_codec_daifmt *cf;
int ret;
ret = hdmi_dai_probe(dai);
--
2.25.1
[PATCH 09/23] ASoC: hdac_hdmi: align function arguments
cppcheck warning: sound/soc/codecs/hdac_hdmi.c:1882:54: style:inconclusive: Function 'hdac_hdmi_jack_init' argument 2 names different: declaration 'pcm' definition 'device'. [funcArgNamesDifferent] int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device, ^ sound/soc/codecs/hdac_hdmi.h:5:54: note: Function 'hdac_hdmi_jack_init' argument 2 names different: declaration 'pcm' definition 'device'. int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int pcm, ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/hdac_hdmi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/codecs/hdac_hdmi.h b/sound/soc/codecs/hdac_hdmi.h index 4fa2fc9ee893..493fa3b4ef75 100644 --- a/sound/soc/codecs/hdac_hdmi.h +++ b/sound/soc/codecs/hdac_hdmi.h @@ -2,7 +2,7 @@ #ifndef __HDAC_HDMI_H__ #define __HDAC_HDMI_H__ -int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int pcm, +int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device, struct snd_soc_jack *jack); int hdac_hdmi_jack_port_init(struct snd_soc_component *component, -- 2.25.1
[PATCH 08/23] ASoC: hdac_hdmi: remove useless initializations
Cppcheck complains a lot about possible null pointer dereferences but
it's again a case of useless initializations to NULL.
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/hdac_hdmi.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 2c1305bf0572..66408a98298b 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -523,7 +523,7 @@ static struct hdac_hdmi_port *hdac_hdmi_get_port_from_cvt(
struct hdac_hdmi_cvt *cvt)
{
struct hdac_hdmi_pcm *pcm;
- struct hdac_hdmi_port *port = NULL;
+ struct hdac_hdmi_port *port;
int ret, i;
list_for_each_entry(pcm, >pcm_list, head) {
@@ -713,7 +713,7 @@ static struct hdac_hdmi_pcm *hdac_hdmi_get_pcm(struct
hdac_device *hdev,
struct hdac_hdmi_port *port)
{
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
- struct hdac_hdmi_pcm *pcm = NULL;
+ struct hdac_hdmi_pcm *pcm;
struct hdac_hdmi_port *p;
list_for_each_entry(pcm, >pcm_list, head) {
@@ -900,7 +900,7 @@ static int hdac_hdmi_set_pin_port_mux(struct snd_kcontrol
*kcontrol,
struct hdac_hdmi_port *port = w->priv;
struct hdac_device *hdev = dev_to_hdac_dev(dapm->dev);
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
- struct hdac_hdmi_pcm *pcm = NULL;
+ struct hdac_hdmi_pcm *pcm;
const char *cvt_name = e->texts[ucontrol->value.enumerated.item[0]];
ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol);
@@ -1693,7 +1693,7 @@ static void hdac_hdmi_eld_notify_cb(void *aptr, int port,
int pipe)
{
struct hdac_device *hdev = aptr;
struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
- struct hdac_hdmi_pin *pin = NULL;
+ struct hdac_hdmi_pin *pin;
struct hdac_hdmi_port *hport = NULL;
struct snd_soc_component *component = hdmi->component;
int i;
@@ -1958,7 +1958,7 @@ static int hdmi_codec_probe(struct snd_soc_component
*component)
struct hdac_device *hdev = hdmi->hdev;
struct snd_soc_dapm_context *dapm =
snd_soc_component_get_dapm(component);
- struct hdac_ext_link *hlink = NULL;
+ struct hdac_ext_link *hlink;
int ret;
hdmi->component = component;
@@ -2227,7 +2227,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
{
struct hdac_device *hdev = dev_to_hdac_dev(dev);
struct hdac_bus *bus = hdev->bus;
- struct hdac_ext_link *hlink = NULL;
+ struct hdac_ext_link *hlink;
dev_dbg(dev, "Enter: %s\n", __func__);
@@ -2263,7 +2263,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
{
struct hdac_device *hdev = dev_to_hdac_dev(dev);
struct hdac_bus *bus = hdev->bus;
- struct hdac_ext_link *hlink = NULL;
+ struct hdac_ext_link *hlink;
dev_dbg(dev, "Enter: %s\n", __func__);
--
2.25.1
[PATCH 06/23] ASoC: cx2070x: remove duplicate else branch
cppcheck warning:
sound/soc/codecs/cx2072x.c:1436:10: style:inconclusive: Found
duplicate branches for 'if' and 'else'. [duplicateBranch]
} else if (type & 0x4) {
^
sound/soc/codecs/cx2072x.c:1439:5: note: Found duplicate branches for
'if' and 'else'.
} else {
^
sound/soc/codecs/cx2072x.c:1436:10: note: Found duplicate branches for
'if' and 'else'.
} else if (type & 0x4) {
^
The last two branches do the same thing and can be collapsed together.
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/cx2072x.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/cx2072x.c b/sound/soc/codecs/cx2072x.c
index d2d004654c9b..d924e3528029 100644
--- a/sound/soc/codecs/cx2072x.c
+++ b/sound/soc/codecs/cx2072x.c
@@ -1430,11 +1430,11 @@ static int cx2072x_jack_status_check(void *data)
state |= SND_JACK_HEADSET;
if (type & 0x2)
state |= SND_JACK_BTN_0;
- } else if (type & 0x4) {
- /* Nokia headset */
- state |= SND_JACK_HEADPHONE;
} else {
- /* Headphone */
+ /*
+* Nokia headset (type & 0x4) and
+* regular Headphone
+*/
state |= SND_JACK_HEADPHONE;
}
}
--
2.25.1
[PATCH 02/23] ASoC: ad1836: remove useless return
Cppcheck warning: sound/soc/codecs/ad1836.c:311:9: warning: Identical condition and return expression 'ret', return value is always 0 [identicalConditionAfterEarlyExit] return ret; ^ sound/soc/codecs/ad1836.c:308:6: note: If condition 'ret' is true, the function will return/exit if (ret) ^ sound/soc/codecs/ad1836.c:311:9: note: Returning identical expression 'ret' return ret; ^ Likely copy/paste between adc and dac cases. Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/ad1836.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/sound/soc/codecs/ad1836.c b/sound/soc/codecs/ad1836.c index a46152560294..08a5651bed9f 100644 --- a/sound/soc/codecs/ad1836.c +++ b/sound/soc/codecs/ad1836.c @@ -305,8 +305,6 @@ static int ad1836_probe(struct snd_soc_component *component) return ret; ret = snd_soc_dapm_add_routes(dapm, ad183x_adc_routes, num_adcs); - if (ret) - return ret; return ret; } -- 2.25.1
[PATCH 05/23] ASoC: cx2070x: remove useless assignment
Cppcheck warning: sound/soc/codecs/cx2072x.c:830:26: style: Variable 'reg1.r.rx_data_one_line' is reassigned a value before the old one has been used. [redundantAssignment] reg1.r.rx_data_one_line = 1; ^ sound/soc/codecs/cx2072x.c:782:26: note: reg1.r.rx_data_one_line is assigned reg1.r.rx_data_one_line = 1; ^ sound/soc/codecs/cx2072x.c:830:26: note: reg1.r.rx_data_one_line is overwritten reg1.r.rx_data_one_line = 1; ^ sound/soc/codecs/cx2072x.c:831:26: style: Variable 'reg1.r.tx_data_one_line' is reassigned a value before the old one has been used. [redundantAssignment] reg1.r.tx_data_one_line = 1; ^ sound/soc/codecs/cx2072x.c:783:26: note: reg1.r.tx_data_one_line is assigned reg1.r.tx_data_one_line = 1; ^ sound/soc/codecs/cx2072x.c:831:26: note: reg1.r.tx_data_one_line is overwritten reg1.r.tx_data_one_line = 1; ^ Likely copy/paste. Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/cx2072x.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/sound/soc/codecs/cx2072x.c b/sound/soc/codecs/cx2072x.c index 8ab22815c2c9..d2d004654c9b 100644 --- a/sound/soc/codecs/cx2072x.c +++ b/sound/soc/codecs/cx2072x.c @@ -827,9 +827,6 @@ static int cx2072x_config_i2spcm(struct cx2072x_priv *cx2072x) } regdbt2.r.i2s_bclk_invert = is_bclk_inv; - reg1.r.rx_data_one_line = 1; - reg1.r.tx_data_one_line = 1; - /* Configures the BCLK output */ bclk_rate = cx2072x->sample_rate * frame_len; reg5.r.i2s_pcm_clk_div_chan_en = 0; -- 2.25.1
[PATCH 03/23] ASoC: adau1977: remove useless return
Cppcheck warning: sound/soc/codecs/adau1977.c:242:9: warning: Identical condition and return expression 'ret', return value is always 0 [identicalConditionAfterEarlyExit] return ret; ^ sound/soc/codecs/adau1977.c:239:6: note: If condition 'ret' is true, the function will return/exit if (ret) ^ sound/soc/codecs/adau1977.c:242:9: note: Returning identical expression 'ret' return ret; ^ Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/adau1977.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/sound/soc/codecs/adau1977.c b/sound/soc/codecs/adau1977.c index 8260f49caa24..e347a48131d1 100644 --- a/sound/soc/codecs/adau1977.c +++ b/sound/soc/codecs/adau1977.c @@ -236,8 +236,6 @@ static int adau1977_reset(struct adau1977 *adau1977) ret = regmap_write(adau1977->regmap, ADAU1977_REG_POWER, ADAU1977_POWER_RESET); regcache_cache_bypass(adau1977->regmap, false); - if (ret) - return ret; return ret; } -- 2.25.1
[PATCH 04/23] ASoC: cros_ec_codec: remove null pointer dereference warning
Cppcheck complains of a possible issue: sound/soc/codecs/cros_ec_codec.c:98:10: warning: Possible null pointer dereference: in [nullPointer] memcpy(in, msg->data, insize); ^ sound/soc/codecs/cros_ec_codec.c:162:34: note: Calling function 'send_ec_host_command', 5th argument 'NULL' value is 0 (uint8_t *), sizeof(p), NULL, 0); ^ sound/soc/codecs/cros_ec_codec.c:98:10: note: Null pointer dereference memcpy(in, msg->data, insize); ^ In practice the access to the pointer is protected by another argument, but this is likely to fool other static analysis tools. Add a test to avoid doing the memcpy if the pointer is NULL or the size is zero. Signed-off-by: Pierre-Louis Bossart --- sound/soc/codecs/cros_ec_codec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c index c4772f82485a..a201d652aca2 100644 --- a/sound/soc/codecs/cros_ec_codec.c +++ b/sound/soc/codecs/cros_ec_codec.c @@ -94,7 +94,7 @@ static int send_ec_host_command(struct cros_ec_device *ec_dev, uint32_t cmd, if (ret < 0) goto error; - if (insize) + if (in && insize) memcpy(in, msg->data, insize); ret = 0; -- 2.25.1
[PATCH 00/23] ASoC: codecs: remove cppcheck warnings
Lots of small fixes in various codec drivers that should have no functional impact. Pierre-Louis Bossart (23): ASoC: ab8500-codec: remove useless structure ASoC: ad1836: remove useless return ASoC: adau1977: remove useless return ASoC: cros_ec_codec: remove null pointer dereference warning ASoC: cx2070x: remove useless assignment ASoC: cx2070x: remove duplicate else branch ASoC: da7219-aad: remove useless initialization ASoC: hdac_hdmi: remove useless initializations ASoC: hdac_hdmi: align function arguments ASoC: hdmi-codec: remove useless initialization ASoC: hdmi-codec: remove unused spk_mask member ASoC: max98090: remove useless assignment ASoC: mt6358: remove useless initializations ASoC: mt6359: remove useless assignment ASoC: nau8825: remove useless assignment ASoC: pcm1681: remove useless assignment ASoC: sigmadsp: align function prototype ASoC: sti-sas: remove unused struct members ASoC: tas2562: remove useless assignment ASoC: tas2562: remove warning on return value ASoC: tas2770: remove useless initialization ASoC: tlv320dac33: clarify expression ASoC: tscs454: remove useless test on PLL disable sound/soc/codecs/ab8500-codec.c | 7 --- sound/soc/codecs/ad1836.c| 2 -- sound/soc/codecs/adau1977.c | 2 -- sound/soc/codecs/cros_ec_codec.c | 2 +- sound/soc/codecs/cx2072x.c | 11 --- sound/soc/codecs/da7219-aad.c| 2 +- sound/soc/codecs/hdac_hdmi.c | 14 +++--- sound/soc/codecs/hdac_hdmi.h | 2 +- sound/soc/codecs/hdmi-codec.c| 3 +-- sound/soc/codecs/max98090.c | 2 +- sound/soc/codecs/mt6358.c| 4 ++-- sound/soc/codecs/mt6359.c| 4 ++-- sound/soc/codecs/nau8825.c | 2 +- sound/soc/codecs/pcm1681.c | 2 +- sound/soc/codecs/sigmadsp.h | 2 +- sound/soc/codecs/sti-sas.c | 3 --- sound/soc/codecs/tas2562.c | 3 +-- sound/soc/codecs/tas2770.c | 2 +- sound/soc/codecs/tlv320dac33.c | 2 +- sound/soc/codecs/tscs454.c | 7 ++- 20 files changed, 32 insertions(+), 46 deletions(-) -- 2.25.1
[PATCH 01/23] ASoC: ab8500-codec: remove useless structure
Cppcheck warnings:
sound/soc/codecs/ab8500-codec.c:117:20: style: struct member
'ab8500_codec_drvdata_dbg::vaud' is never used. [unusedStructMember]
struct regulator *vaud;
^
sound/soc/codecs/ab8500-codec.c:118:20: style: struct member
'ab8500_codec_drvdata_dbg::vamic1' is never used. [unusedStructMember]
struct regulator *vamic1;
^
sound/soc/codecs/ab8500-codec.c:119:20: style: struct member
'ab8500_codec_drvdata_dbg::vamic2' is never used. [unusedStructMember]
struct regulator *vamic2;
^
sound/soc/codecs/ab8500-codec.c:120:20: style: struct member
'ab8500_codec_drvdata_dbg::vdmic' is never used. [unusedStructMember]
struct regulator *vdmic;
^
The structure is never used, remove.
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/codecs/ab8500-codec.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index c95f007cede1..5525e1ccab76 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -113,13 +113,6 @@ enum amic_idx {
AMIC_IDX_2
};
-struct ab8500_codec_drvdata_dbg {
- struct regulator *vaud;
- struct regulator *vamic1;
- struct regulator *vamic2;
- struct regulator *vdmic;
-};
-
/* Private data for AB8500 device-driver */
struct ab8500_codec_drvdata {
struct regmap *regmap;
--
2.25.1
Re: [PATCH v4 0/9] soundwire: qcom: various improvements
On 3/12/21 6:00 AM, Srinivas Kandagatla wrote: Thanks for reviewing v3 of this patchset! During testing SoundWire controller on SM8250 MTP, we found few issues like all the interrupts are not handled, all transport parameters are not read from device tree. Patch to add Auto Enumeration supported by the controller is also part of this series. Other major issue was register read/writes which was interrupt based was an overhead and puts lot of limitation on context it can be used from. With previous approach number of interrupts generated after enumeration are around 130: $ cat /proc/interrupts | grep soundwire 21: 130 0 0 0 0 0 0 0 GICv3 234 Edge soundwire after this patch they are just 3 interrupts $ cat /proc/interrupts | grep soundwire 21: 3 0 0 0 0 0 0 0 GICv3 234 Edge soundwire So this patchset add various improvements to the existing driver to address above issues. Tested it on SM8250 MTP with 2x WSA881x speakers, HeadPhones on WCD938x via lpass-rx-macro and Analog MICs via lpass-tx-macro. Also tested on DragonBoard DB845c with 2xWSA881x speakers. LGTM, for the series Reviewed-by: Pierre-Louis Bossart Changes since v3: - Fixed setting assigned bit during autoenumeration Srinivas Kandagatla (9): dt-bindings: soundwire: qcom: clarify data port bus parameters soundwire: qcom: add support to missing transport params soundwire: qcom: set continue execution flag for ignored commands soundwire: qcom: start the clock during initialization soundwire: qcom: update register read/write routine soundwire: qcom: add support to new interrupts soundwire: export sdw_compare_devid() and sdw_extract_slave_id() soundwire: qcom: add auto enumeration support soundwire: qcom: wait for enumeration to be complete in probe .../bindings/soundwire/qcom,sdw.txt | 20 + drivers/soundwire/bus.c | 4 +- drivers/soundwire/qcom.c | 529 ++ include/linux/soundwire/sdw.h | 2 + 4 files changed, 442 insertions(+), 113 deletions(-)
Re: [PATCH v3 1/5] soundwire: add static port mapping support
On 3/12/21 5:39 AM, Srinivas Kandagatla wrote:
Some of the SoundWire device ports are statically mapped to Controller
ports during design, however there is no way to expose this information
to the controller. Controllers like Qualcomm ones use this info to setup
static bandwidth parameters for those ports.
A generic port allocation is not possible in this cases!
So this patch adds a new member m_port_map to struct sdw_slave to expose
this static map.
Signed-off-by: Srinivas Kandagatla
---
include/linux/soundwire/sdw.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index d08039d65825..b032d6ac0b39 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -614,6 +614,7 @@ struct sdw_slave_ops {
* @debugfs: Slave debugfs
* @node: node for bus list
* @port_ready: Port ready completion flag for each Slave port
+ * @m_port_map: static Master port map for each Slave port0 to port14
did you mean port1..port14?
DP0 is a special case that's not supposed to be used for audio transport
but rather extended control and command?
* @dev_num: Current Device Number, values can be 0 or dev_num_sticky
* @dev_num_sticky: one-time static Device Number assigned by Bus
* @probed: boolean tracking driver state
@@ -645,6 +646,7 @@ struct sdw_slave {
#endif
struct list_head node;
struct completion port_ready[SDW_MAX_PORTS];
+ unsigned int m_port_map[SDW_MAX_PORTS];
enum sdw_clk_stop_mode curr_clk_stop_mode;
u16 dev_num;
u16 dev_num_sticky;
[PATCH 3/4] ASoC: mediatek: mt2701: rename shadowed array
cppcheck warning:
sound/soc/mediatek/mt2701/mt2701-afe-pcm.c:406:36: style: Local
variable 'memif_data' shadows outer variable [shadowVariable]
const struct mtk_base_memif_data *memif_data;
^
sound/soc/mediatek/mt2701/mt2701-afe-pcm.c:977:41: note: Shadowed
declaration
static const struct mtk_base_memif_data memif_data[MT2701_MEMIF_NUM] = {
^
sound/soc/mediatek/mt2701/mt2701-afe-pcm.c:406:36: note: Shadow
variable
const struct mtk_base_memif_data *memif_data;
^
sound/soc/mediatek/mt2701/mt2701-afe-pcm.c:431:36: style: Local
variable 'memif_data' shadows outer variable [shadowVariable]
const struct mtk_base_memif_data *memif_data;
^
sound/soc/mediatek/mt2701/mt2701-afe-pcm.c:977:41: note: Shadowed
declaration
static const struct mtk_base_memif_data memif_data[MT2701_MEMIF_NUM] = {
^
sound/soc/mediatek/mt2701/mt2701-afe-pcm.c:431:36: note: Shadow
variable
const struct mtk_base_memif_data *memif_data;
^
Signed-off-by: Pierre-Louis Bossart
---
sound/soc/mediatek/mt2701/mt2701-afe-pcm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
b/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
index d5cffe7a7e15..bc3d0466472b 100644
--- a/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
+++ b/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
@@ -974,7 +974,7 @@ static const struct snd_soc_component_driver
mt2701_afe_pcm_dai_component = {
.resume = mtk_afe_resume,
};
-static const struct mtk_base_memif_data memif_data[MT2701_MEMIF_NUM] = {
+static const struct mtk_base_memif_data memif_data_array[MT2701_MEMIF_NUM] = {
{
.name = "DL1",
.id = MT2701_MEMIF_DL1,
@@ -1366,7 +1366,7 @@ static int mt2701_afe_pcm_dev_probe(struct
platform_device *pdev)
return -ENOMEM;
for (i = 0; i < afe->memif_size; i++) {
- afe->memif[i].data = _data[i];
+ afe->memif[i].data = _data_array[i];
afe->memif[i].irq_usage = -1;
}
--
2.25.1
