[PATCH v7 2/2] ASoC: cros_ec_codec: Reset I2S RX when probing
It is not guaranteed that I2S RX is disabled when the kernel booting.
For example, if the kernel crashes while it is enabled, it will keep
enabled until the next time EC reboots. Reset I2S RX when probing to
fix this issue.
Signed-off-by: Yu-Hsuan Hsu
---
Updated the info message.
sound/soc/codecs/cros_ec_codec.c | 12
1 file changed, 12 insertions(+)
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index f33a2a9654e7..c4772f82485a 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -1011,6 +1011,18 @@ static int cros_ec_codec_platform_probe(struct
platform_device *pdev)
}
priv->ec_capabilities = r.capabilities;
+ /* Reset EC codec i2s rx. */
+ p.cmd = EC_CODEC_I2S_RX_RESET;
+ ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
+ (uint8_t *)&p, sizeof(p), NULL, 0);
+ if (ret == -ENOPROTOOPT) {
+ dev_info(dev,
+"Missing reset command. Please update EC firmware.\n");
+ } else if (ret) {
+ dev_err(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret);
+ return ret;
+ }
+
platform_set_drvdata(pdev, priv);
ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
--
2.30.0.296.g2bfb1c46d8-goog
[PATCH v7 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET
Add the new command EC_CODEC_I2S_RX_RESET in ec_codec_i2s_rx_subcmd,
which is used for resetting the EC codec.
Signed-off-by: Yu-Hsuan Hsu
---
include/linux/platform_data/cros_ec_commands.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/platform_data/cros_ec_commands.h
b/include/linux/platform_data/cros_ec_commands.h
index 86376779ab31..95889ada83a3 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4600,6 +4600,7 @@ enum ec_codec_i2s_rx_subcmd {
EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
EC_CODEC_I2S_RX_SET_BCLK = 0x4,
+ EC_CODEC_I2S_RX_RESET = 0x5,
EC_CODEC_I2S_RX_SUBCMD_COUNT,
};
--
2.30.0.296.g2bfb1c46d8-goog
[PATCH v6 2/2] ASoC: cros_ec_codec: Reset I2S RX when probing
It is not guaranteed that I2S RX is disabled when the kernel booting.
For example, if the kernel crashes while it is enabled, it will keep
enabled until the next time EC reboots. Reset I2S RX when probing to
fix this issue.
Signed-off-by: Yu-Hsuan Hsu
---
Returns the error code when it fails to reset.
sound/soc/codecs/cros_ec_codec.c | 12
1 file changed, 12 insertions(+)
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index f33a2a9654e7..40e437aa1d55 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -1011,6 +1011,18 @@ static int cros_ec_codec_platform_probe(struct
platform_device *pdev)
}
priv->ec_capabilities = r.capabilities;
+ /* Reset EC codec i2s rx. */
+ p.cmd = EC_CODEC_I2S_RX_RESET;
+ ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
+ (uint8_t *)&p, sizeof(p), NULL, 0);
+ if (ret == -ENOPROTOOPT) {
+ dev_info(dev,
+"Command not found. Please update the EC firmware.\n");
+ } else if (ret) {
+ dev_err(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret);
+ return ret;
+ }
+
platform_set_drvdata(pdev, priv);
ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
--
2.30.0.284.gd98b1dd5eaa7-goog
[PATCH v6 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET
Add the new command EC_CODEC_I2S_RX_RESET in ec_codec_i2s_rx_subcmd,
which is used for resetting the EC codec.
Signed-off-by: Yu-Hsuan Hsu
---
include/linux/platform_data/cros_ec_commands.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/platform_data/cros_ec_commands.h
b/include/linux/platform_data/cros_ec_commands.h
index 86376779ab31..95889ada83a3 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4600,6 +4600,7 @@ enum ec_codec_i2s_rx_subcmd {
EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
EC_CODEC_I2S_RX_SET_BCLK = 0x4,
+ EC_CODEC_I2S_RX_RESET = 0x5,
EC_CODEC_I2S_RX_SUBCMD_COUNT,
};
--
2.30.0.284.gd98b1dd5eaa7-goog
[PATCH v5] ASoC: cros_ec_codec: Reset I2S RX when probing
It is not guaranteed that I2S RX is disabled when the kernel booting.
For example, if the kernel crashes while it is enabled, it will keep
enabled until the next time EC reboots. Reset I2S RX when probing to
fix this issue.
Signed-off-by: Yu-Hsuan Hsu
---
This patch checks the return value. If it is -ENOPROTOOPT
(EC_RES_INVALID_VERSION), it will ask clients to update EC firmware.
Previous patches
v1:
https://patchwork.kernel.org/project/alsa-devel/patch/20200708071117.3070707-1-yuhs...@chromium.org/
v2:
https://patchwork.kernel.org/project/alsa-devel/patch/20200716170914.3623060-1-yuhs...@chromium.org/
v3:
https://patchwork.kernel.org/project/alsa-devel/patch/20210106050559.1459027-1-yuhs...@chromium.org/
v4:
https://patchwork.kernel.org/project/alsa-devel/patch/20210107085942.2891525-2-yuhs...@chromium.org/
include/linux/platform_data/cros_ec_commands.h | 1 +
sound/soc/codecs/cros_ec_codec.c | 11 +++
2 files changed, 12 insertions(+)
diff --git a/include/linux/platform_data/cros_ec_commands.h
b/include/linux/platform_data/cros_ec_commands.h
index 86376779ab31..95889ada83a3 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4600,6 +4600,7 @@ enum ec_codec_i2s_rx_subcmd {
EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
EC_CODEC_I2S_RX_SET_BCLK = 0x4,
+ EC_CODEC_I2S_RX_RESET = 0x5,
EC_CODEC_I2S_RX_SUBCMD_COUNT,
};
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index f33a2a9654e7..d35c57724b45 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -1011,6 +1011,17 @@ static int cros_ec_codec_platform_probe(struct
platform_device *pdev)
}
priv->ec_capabilities = r.capabilities;
+ /* Reset EC codec i2s rx. */
+ p.cmd = EC_CODEC_I2S_RX_RESET;
+ ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
+ (uint8_t *)&p, sizeof(p), NULL, 0);
+ if (ret == -ENOPROTOOPT) {
+ dev_info(dev,
+"Command not found. Please update the EC firmware.\n");
+ } else if (ret) {
+ dev_err(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret);
+ }
+
platform_set_drvdata(pdev, priv);
ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
--
2.30.0.284.gd98b1dd5eaa7-goog
Re: [PATCH 2/2] ASoC: cros_ec_codec: Reset I2S RX when probing
Enric Balletbo i Serra 於 2021年1月13日 週三 上午12:34寫道： > > Hi Yu-Hsuan, > > Thank you for the patch. > > On 7/1/21 9:59, Yu-Hsuan Hsu wrote: > > It is not guaranteed that I2S RX is disabled when the kernel booting. > > For example, if the kernel crashes while it is enabled, it will keep > > enabled until the next time EC reboots. Reset I2S RX when probing to > > fix this issue. > > > > Signed-off-by: Yu-Hsuan Hsu > > If I am not mistaken this is the four version of this patchset (see [1]). > Please > prefix your patches with the proper version and maintain a changelog for them, > otherwise makes difficult to follow all the discussions already done. > > [1] > v1: https://lkml.org/lkml/2020/7/8/173 > v2: > https://mailman.alsa-project.org/pipermail/alsa-devel/2020-July/170933.html > v3: > https://patchwork.kernel.org/project/alsa-devel/patch/20210106050559.1459027-1-yuhs...@chromium.org/ > v4: https://patchwork.kernel.org/project/alsa-devel/list/?series=410441 Sorry that I forgot to add version. Will add v5 in the next patch. Thanks! > > > --- > > sound/soc/codecs/cros_ec_codec.c | 7 +++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/sound/soc/codecs/cros_ec_codec.c > > b/sound/soc/codecs/cros_ec_codec.c > > index f33a2a9654e7..28b3e2c48c86 100644 > > --- a/sound/soc/codecs/cros_ec_codec.c > > +++ b/sound/soc/codecs/cros_ec_codec.c > > @@ -1011,6 +1011,13 @@ static int cros_ec_codec_platform_probe(struct > > platform_device *pdev) > > } > > priv->ec_capabilities = r.capabilities; > > > > + /* Reset EC codec i2s rx. */ > > + p.cmd = EC_CODEC_I2S_RX_RESET; > > + ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX, > > +(uint8_t *)&p, sizeof(p), NULL, 0); > > + if (ret) > > + dev_warn(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret); > > + > > My comment in the first version is still valid, I guess. This command was > introduced later and with an old firmware I suspect this message will appear > on > every boot, right? So, to solve the issue and get rid of this warn you're > forced > to upgrade the firmware. Would make sense to handle returned error value to > warn > when the firmware needs to be updated and error and break when is really an > error? > > We have mapped ec error codes to linux error codes. So, it should be possible > now. Oh, I didn't notice it. Thanks for the remind. I will work on it. > > Thanks, > Enric > > > platform_set_drvdata(pdev, priv); > > > > ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver, > >
Re: [PATCH 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET
Mark Brown 於 2021年1月7日 週四 下午9:55寫道： > > On Thu, Jan 07, 2021 at 04:59:41PM +0800, Yu-Hsuan Hsu wrote: > > Add the new command EC_CODEC_I2S_RX_RESET in ec_codec_i2s_rx_subcmd, > > which is used for resetting the EC codec. > > I think the request was to sync over all the commands that are supported > in the EC rather than just split this one addition into a separate > patch. Got it. However, after running make_linux_ec_commands_h.sh to create the new cros_ec_commands.h, I found there are lots of difference (1092 insertions(+), 66 deletions(-)). In addition, there are also some redefined variables(most are in ./include/linux/usb/pd.h) causing the compile error. It seems not easy to sync cros_ec_commands.h. I'm afraid of breaking something. Does anyone have any suggestion? Thanks.
[PATCH 2/2] ASoC: cros_ec_codec: Reset I2S RX when probing
It is not guaranteed that I2S RX is disabled when the kernel booting. For example, if the kernel crashes while it is enabled, it will keep enabled until the next time EC reboots. Reset I2S RX when probing to fix this issue. Signed-off-by: Yu-Hsuan Hsu --- sound/soc/codecs/cros_ec_codec.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c index f33a2a9654e7..28b3e2c48c86 100644 --- a/sound/soc/codecs/cros_ec_codec.c +++ b/sound/soc/codecs/cros_ec_codec.c @@ -1011,6 +1011,13 @@ static int cros_ec_codec_platform_probe(struct platform_device *pdev) } priv->ec_capabilities = r.capabilities; + /* Reset EC codec i2s rx. */ + p.cmd = EC_CODEC_I2S_RX_RESET; + ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX, + (uint8_t *)&p, sizeof(p), NULL, 0); + if (ret) + dev_warn(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret); + platform_set_drvdata(pdev, priv); ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver, -- 2.29.2.729.g45daf8777d-goog
[PATCH 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET
Add the new command EC_CODEC_I2S_RX_RESET in ec_codec_i2s_rx_subcmd,
which is used for resetting the EC codec.
Signed-off-by: Yu-Hsuan Hsu
---
include/linux/platform_data/cros_ec_commands.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/platform_data/cros_ec_commands.h
b/include/linux/platform_data/cros_ec_commands.h
index 86376779ab31..95889ada83a3 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4600,6 +4600,7 @@ enum ec_codec_i2s_rx_subcmd {
EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
EC_CODEC_I2S_RX_SET_BCLK = 0x4,
+ EC_CODEC_I2S_RX_RESET = 0x5,
EC_CODEC_I2S_RX_SUBCMD_COUNT,
};
--
2.29.2.729.g45daf8777d-goog
[PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
It is not guaranteed that I2S RX is disabled when the kernel booting.
For example, if the kernel crashes while it is enabled, it will keep
enabled until the next time EC reboots. Reset I2S RX when probing to
fix this issue.
Signed-off-by: Yu-Hsuan Hsu
---
include/linux/platform_data/cros_ec_commands.h | 1 +
sound/soc/codecs/cros_ec_codec.c | 7 +++
2 files changed, 8 insertions(+)
diff --git a/include/linux/platform_data/cros_ec_commands.h
b/include/linux/platform_data/cros_ec_commands.h
index 86376779ab31..95889ada83a3 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4600,6 +4600,7 @@ enum ec_codec_i2s_rx_subcmd {
EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
EC_CODEC_I2S_RX_SET_BCLK = 0x4,
+ EC_CODEC_I2S_RX_RESET = 0x5,
EC_CODEC_I2S_RX_SUBCMD_COUNT,
};
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index f33a2a9654e7..28b3e2c48c86 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -1011,6 +1011,13 @@ static int cros_ec_codec_platform_probe(struct
platform_device *pdev)
}
priv->ec_capabilities = r.capabilities;
+ /* Reset EC codec i2s rx. */
+ p.cmd = EC_CODEC_I2S_RX_RESET;
+ ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
+ (uint8_t *)&p, sizeof(p), NULL, 0);
+ if (ret)
+ dev_warn(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret);
+
platform_set_drvdata(pdev, priv);
ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
--
2.29.2.729.g45daf8777d-goog
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Pierre-Louis Bossart 於 2020年8月13日 週四 下午8:57寫道： > > > > On 8/13/20 3:45 AM, Takashi Iwai wrote: > > On Thu, 13 Aug 2020 10:36:57 +0200, > > Yu-Hsuan Hsu wrote: > >> > >> Lu, Brent 於 2020年8月13日 週四 下午3:55寫道： > >>> > >>>>>> > >>>>>> CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large > >>>>>> buffer as possible. So the period size is an arbitrary number in > >>>>>> different platforms. Atom SST platform happens to be 256, and CML > >>>>>> SOF platform is 1056 for example. > >>>>> > >>>>> ok, but earlier in this thread it was mentioned that values such as > >>>>> 432 are not suitable. the statement above seems to mean the period > >>>>> actual value is a "don't care", so I don't quite see why this specific > >>>>> patch2 restricting the value to 240 is necessary. Patch1 is needed for > >>>>> sure, > >>>>> Patch2 is where Takashi and I are not convinced. > >>>> > >>>> I have downloaded the patch1 but it does not work. After applying patch1, > >>>> the default period size changes to 320. However, it also has the same > >>>> issue > >>>> with period size 320. (It can be verified by aplay.) > >>> > >>> The period_size is related to the audio latency so it's decided by > >>> application > >>> according to the use case it's running. That's why there are concerns > >>> about > >>> patch 2 and also you cannot find similar constraints in other machine > >>> driver. > >> You're right. However, the problem here is the provided period size > >> does not work. Like 256, setting the period size to 320 also makes > >> users have big latency in the DSP ring buffer. > >> > >> localhost ~ # aplay -Dhw:1,0 --period-size=320 --buffer-size=640 > >> /dev/zero -d 1 -f dat --test-position > >> Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 > >> Hz, Stereo > >> Suspicious buffer position (1 total): avail = 0, delay = 2640, buffer = 640 > >> Suspicious buffer position (2 total): avail = 0, delay = 2640, buffer = 640 > >> Suspicious buffer position (3 total): avail = 0, delay = 2720, buffer = 640 > >> ... > > > > It means that the delay value returned from the driver is bogus. > > I suppose it comes pcm_delay value calculated in sst_calc_tstamp(), > > but haven't followed the code closely yet. Maybe checking the debug > > outputs can help to trace what's going wrong. > > the problem is really that we add a constraint that the period size be a > multiple of 1ms, and it's not respected. 320 samples is not a valid > choice, I don't get how it ends-up being selected? there's a glitch in > the matrix here. > > Oh sorry that I applied the wrong patch. With the correct patch, the default period size is 432. With period size 432, running aplay with --test-position does not show any errors. However, by cat `/proc/asound/card1/pcm0p/sub0/status`. We can see the delay is around 3000. Here are all period sizes I have tried. All buffer sizes are set to 2 * period size. period size: 192, delay is a negative number. Not sure what happened. period size: 240, delay is fixed at 960 period size: 288, delay is around 27XX period size: 336, delay is around 27XX period size: 384, delay is around 24XX (no errors from aplay) period size: 432, delay is around 30XX (no errors from aplay) period size: 480, delay is fixed at 3120 (no errors from aplay) period size: 524, delay is around 31XX (no errors from aplay) Not sure why the delay is around 50ms except for the period size 240. Is it normal? Thanks, Yu-Hsuan
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Lu, Brent 於 2020年8月13日 週四 下午3:55寫道： > > > > > > > > > CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large > > > > buffer as possible. So the period size is an arbitrary number in > > > > different platforms. Atom SST platform happens to be 256, and CML > > > > SOF platform is 1056 for example. > > > > > > ok, but earlier in this thread it was mentioned that values such as > > > 432 are not suitable. the statement above seems to mean the period > > > actual value is a "don't care", so I don't quite see why this specific > > > patch2 restricting the value to 240 is necessary. Patch1 is needed for > > > sure, > > > Patch2 is where Takashi and I are not convinced. > > > > I have downloaded the patch1 but it does not work. After applying patch1, > > the default period size changes to 320. However, it also has the same issue > > with period size 320. (It can be verified by aplay.) > > The period_size is related to the audio latency so it's decided by application > according to the use case it's running. That's why there are concerns about > patch 2 and also you cannot find similar constraints in other machine driver. You're right. However, the problem here is the provided period size does not work. Like 256, setting the period size to 320 also makes users have big latency in the DSP ring buffer. localhost ~ # aplay -Dhw:1,0 --period-size=320 --buffer-size=640 /dev/zero -d 1 -f dat --test-position Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo Suspicious buffer position (1 total): avail = 0, delay = 2640, buffer = 640 Suspicious buffer position (2 total): avail = 0, delay = 2640, buffer = 640 Suspicious buffer position (3 total): avail = 0, delay = 2720, buffer = 640 ... > > Another problem is the buffer size. Too large buffer is not just wasting > memories. > It also creates problems to memory allocator since continuous pages are not > always there. Using a small period_count like 2 or 4 should be sufficient for > audio > data transfer. > > buffer_size = period_size * period_count * 100 / sample_rate; > snd_pcm_hw_params_set_buffer_time_near(mPcmDevice, params, &buffer_size, > NULL); > > And one more problem here: you need to decide period_size and period_count > first in order to calculate the buffer size... It's a good point. I will bring it up to our team and see whether we can use the smaller buffer size. Thanks! > > > Regards, > Brent Thanks, Yu-Hsuan
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Pierre-Louis Bossart 於 2020年8月13日 週四 上午12:38寫道： > > > > On 8/12/20 11:08 AM, Lu, Brent wrote: > >>> > >>> I also wonder what's really missing, too :) > >>> > >>> BTW, I took a look back at the thread, and CRAS seems using a very > >>> large buffer, namely: > >>> [ 52.434791] sound pcmC1D0p: PERIOD_SIZE [240:240] > >>> [ 52.434802] sound pcmC1D0p: BUFFER_SIZE [204480:204480] > >> > >> yes, that's 852 periods and 4.260 seconds. Never seen such values :-) > > > > CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large > > buffer as possible. So the period size is an arbitrary number in different > > platforms. Atom SST platform happens to be 256, and CML SOF platform > > is 1056 for example. > > ok, but earlier in this thread it was mentioned that values such as 432 > are not suitable. the statement above seems to mean the period actual > value is a "don't care", so I don't quite see why this specific patch2 > restricting the value to 240 is necessary. Patch1 is needed for sure, > Patch2 is where Takashi and I are not convinced. I have downloaded the patch1 but it does not work. After applying patch1, the default period size changes to 320. However, it also has the same issue with period size 320. (It can be verified by aplay.)
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Takashi Iwai 於 2020年8月12日 週三 下午2:58寫道： > > On Wed, 12 Aug 2020 08:53:42 +0200, > Yu-Hsuan Hsu wrote: > > > > Takashi Iwai 於 2020年8月12日 週三 下午2:14寫道： > > > > > > On Wed, 12 Aug 2020 05:09:58 +0200, > > > Yu-Hsuan Hsu wrote: > > > > > > > > Mark Brown 於 2020年8月12日 週三 上午1:22寫道： > > > > > > > > > > On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote: > > > > > > > > > > > > constraint logic needs to know about this DSP limitation - it > > > > > > > seems like > > > > > > > none of this is going to change without something new going into > > > > > > > the > > > > > > > mix? We at least need a new question to ask about the DSP > > > > > > > firmware I > > > > > > > think. > > > > > > > > > > > I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu > > > > > > kernel 5.4, > > > > > > and I see no issues with the 240 sample period. Same with 432, 960, > > > > > > 9600, > > > > > > etc. > > > > > > > > > > > I also tried just for fun what happens with 256 samples, and I > > > > > > don't see any > > > > > > underflows thrown either, so I am wondering what exactly the > > > > > > problem is? > > > > > > Something's not adding up. I would definitively favor multiple of > > > > > > 1ms > > > > > > periods, since it's the only case that was productized, but there's > > > > > > got to > > > > > > me something a side effect of how CRAS programs the hw_params. > > > > > > > > > > Is it something that goes wrong with longer playbacks possibly (eg, > > > > > someone watching a feature film or something)? > > > > > > > > Thanks for testing! > > > > > > > > After doing some experiments, I think I can identify the problem more > > > > precisely. > > > > 1. aplay can not reproduce this issue because it writes samples > > > > immediately when there are some space in the buffer. However, you can > > > > add --test-position to see how the delay grows with period size 256. > > > > > aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f > > > > > dat --test-position > > > > Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 > > > > Hz, Stereo > > > > Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = > > > > 512 > > > > Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = > > > > 512 > > > > Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = > > > > 512 > > > > ... > > > > > > Isn't this about the alignment of the buffer size against the period > > > size, not the period size itself? i.e. in the example above, the > > > buffer size isn't a multiple of period size, and DSP can't handle if > > > the position overlaps the buffer size in a half way. > > > > > > If that's the problem (and it's an oft-seen restriction), the right > > > constraint is > > > snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); > > > > > > > > > Takashi > > Oh sorry for my typo. The issue happens no matter what buffer size is > > set. Actually, even if I want to set 480, it will change to 512 > > automatically. > > Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer > > = 512 <-this one is the buffer size > > OK, then it means that the buffer size alignment is already in place. > > And this large delay won't happen if you use period size 240? > > > Takashi Yes! If I set the period size to 240, it will not print "Suspicious buffer position ..." Yu-Hsuan > > > > > 2. Since many samples are moved to DSP(delay), the measured rate of > > > > the ring-buffer is high. (I measured it by alsa_conformance_test, > > > > which only test the sampling rate in the ring buffer of kernel not > > > > DSP) > > > > > > > > 3. Since CRAS writes samples with a fixed frequency, this behavior > > > > will take all samples from the ring buffer, which is seen as underrun > > > > by CRAS. (It seems that it is not a real underrun because that avail > > > > does not larger than buffer size. Maybe CRAS should also take dalay > > > > into account.) > > > > > > > > 4. In spite of it is not a real underrun, the large delay is still a > > > > big problem. Can we apply the constraint to fix it? Or any better > > > > idea? > > > > > > > > Thanks, > > > > Yu-Hsuan > > > > > >
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Takashi Iwai 於 2020年8月12日 週三 下午2:14寫道： > > On Wed, 12 Aug 2020 05:09:58 +0200, > Yu-Hsuan Hsu wrote: > > > > Mark Brown 於 2020年8月12日 週三 上午1:22寫道： > > > > > > On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote: > > > > > > > > constraint logic needs to know about this DSP limitation - it seems > > > > > like > > > > > none of this is going to change without something new going into the > > > > > mix? We at least need a new question to ask about the DSP firmware I > > > > > think. > > > > > > > I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel > > > > 5.4, > > > > and I see no issues with the 240 sample period. Same with 432, 960, > > > > 9600, > > > > etc. > > > > > > > I also tried just for fun what happens with 256 samples, and I don't > > > > see any > > > > underflows thrown either, so I am wondering what exactly the problem is? > > > > Something's not adding up. I would definitively favor multiple of 1ms > > > > periods, since it's the only case that was productized, but there's got > > > > to > > > > me something a side effect of how CRAS programs the hw_params. > > > > > > Is it something that goes wrong with longer playbacks possibly (eg, > > > someone watching a feature film or something)? > > > > Thanks for testing! > > > > After doing some experiments, I think I can identify the problem more > > precisely. > > 1. aplay can not reproduce this issue because it writes samples > > immediately when there are some space in the buffer. However, you can > > add --test-position to see how the delay grows with period size 256. > > > aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat > > > --test-position > > Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 > > Hz, Stereo > > Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512 > > Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512 > > Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512 > > ... > > Isn't this about the alignment of the buffer size against the period > size, not the period size itself? i.e. in the example above, the > buffer size isn't a multiple of period size, and DSP can't handle if > the position overlaps the buffer size in a half way. > > If that's the problem (and it's an oft-seen restriction), the right > constraint is > snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); > > > Takashi Oh sorry for my typo. The issue happens no matter what buffer size is set. Actually, even if I want to set 480, it will change to 512 automatically. Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512 <-this one is the buffer size > > > 2. Since many samples are moved to DSP(delay), the measured rate of > > the ring-buffer is high. (I measured it by alsa_conformance_test, > > which only test the sampling rate in the ring buffer of kernel not > > DSP) > > > > 3. Since CRAS writes samples with a fixed frequency, this behavior > > will take all samples from the ring buffer, which is seen as underrun > > by CRAS. (It seems that it is not a real underrun because that avail > > does not larger than buffer size. Maybe CRAS should also take dalay > > into account.) > > > > 4. In spite of it is not a real underrun, the large delay is still a > > big problem. Can we apply the constraint to fix it? Or any better > > idea? > > > > Thanks, > > Yu-Hsuan > >
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Mark Brown 於 2020年8月12日 週三 上午1:22寫道： > > On Tue, Aug 11, 2020 at 11:54:38AM -0500, Pierre-Louis Bossart wrote: > > > > constraint logic needs to know about this DSP limitation - it seems like > > > none of this is going to change without something new going into the > > > mix? We at least need a new question to ask about the DSP firmware I > > > think. > > > I just tested aplay -Dhw: on a Cyan Chromebook with the Ubuntu kernel 5.4, > > and I see no issues with the 240 sample period. Same with 432, 960, 9600, > > etc. > > > I also tried just for fun what happens with 256 samples, and I don't see any > > underflows thrown either, so I am wondering what exactly the problem is? > > Something's not adding up. I would definitively favor multiple of 1ms > > periods, since it's the only case that was productized, but there's got to > > me something a side effect of how CRAS programs the hw_params. > > Is it something that goes wrong with longer playbacks possibly (eg, > someone watching a feature film or something)? Thanks for testing! After doing some experiments, I think I can identify the problem more precisely. 1. aplay can not reproduce this issue because it writes samples immediately when there are some space in the buffer. However, you can add --test-position to see how the delay grows with period size 256. > aplay -Dhw:1,0 --period-size=256 --buffer-size=480 /dev/zero -d 1 -f dat > --test-position Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo Suspicious buffer position (1 total): avail = 0, delay = 2064, buffer = 512 Suspicious buffer position (2 total): avail = 0, delay = 2064, buffer = 512 Suspicious buffer position (3 total): avail = 0, delay = 2096, buffer = 512 ... 2. Since many samples are moved to DSP(delay), the measured rate of the ring-buffer is high. (I measured it by alsa_conformance_test, which only test the sampling rate in the ring buffer of kernel not DSP) 3. Since CRAS writes samples with a fixed frequency, this behavior will take all samples from the ring buffer, which is seen as underrun by CRAS. (It seems that it is not a real underrun because that avail does not larger than buffer size. Maybe CRAS should also take dalay into account.) 4. In spite of it is not a real underrun, the large delay is still a big problem. Can we apply the constraint to fix it? Or any better idea? Thanks, Yu-Hsuan
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Takashi Iwai 於 2020年8月11日 週二 下午4:39寫道：
>
> On Tue, 11 Aug 2020 10:25:22 +0200,
> Yu-Hsuan Hsu wrote:
> >
> > Takashi Iwai 於 2020年8月11日 週二 下午3:43寫道：
> > >
> > > On Tue, 11 Aug 2020 04:29:24 +0200,
> > > Yu-Hsuan Hsu wrote:
> > > >
> > > > Lu, Brent 於 2020年8月11日 週二 上午10:17寫道：
> > > > >
> > > > > >
> > > > > > Sorry for the late reply. CRAS does not set the period size when
> > > > > > using it.
> > > > > > The default period size is 256, which consumes the samples
> > > > > > quickly(about 49627
> > > > > > fps when the rate is 48000 fps) at the beginning of the playback.
> > > > > > Since CRAS write samples with the fixed frequency, it triggers
> > > > > > underruns
> > > > > > immidiately.
> > > > > >
> > > > > > According to Brent, the DSP is using 240 period regardless the
> > > > > > hw_param. If the
> > > > > > period size is 256, DSP will read 256 samples each time but only
> > > > > > consume 240
> > > > > > samples until the ring buffer of DSP is full. This behavior makes
> > > > > > the samples in
> > > > > > the ring buffer of kernel consumed quickly. (Not sure whether the
> > > > > > explanation is
> > > > > > correct. Need Brent to confirm it.)
> > > > > >
> > > > > > Unfortunately, we can not change the behavior of DSP. After some
> > > > > > experiments,
> > > > > > we found that the issue can be fixed if we set the period size to
> > > > > > 240. With the
> > > > > > same frequency as the DSP, the samples are consumed stably. Because
> > > > > > everyone
> > > > > > can trigger this issue when using the driver without setting the
> > > > > > period size, we
> > > > > > think it is a general issue that should be fixed in the kernel.
> > > > >
> > > > > I check the code and just realized CRAS does nothing but request
> > > > > maximum buffer
> > > > > size. As I know the application needs to decide the buffer time and
> > > > > period time so
> > > > > ALSA could generate a hw_param structure with proper period size
> > > > > instead of using
> > > > > fixed constraint in machine driver because driver has no idea about
> > > > > the latency you
> > > > > want.
> > > > >
> > > > > You can use snd_pcm_hw_params_set_buffer_time_near() and
> > > > > snd_pcm_hw_params_set_period_time_near() to get a proper
> > > > > configuration of
> > > > > buffer and period parameters according to the latency requirement. In
> > > > > the CRAS
> > > > > code, there is a UCM variable to support this: DmaPeriodMicrosecs. I
> > > > > tested it on
> > > > > Celes and it looks quite promising. It seems to me that adding
> > > > > constraint in machine
> > > > > driver is not necessary.
> > > > >
> > > > > SectionDevice."Speaker".0 {
> > > > > Value {
> > > > > PlaybackPCM "hw:chtrt5650,0"
> > > > > DmaPeriodMicrosecs "5000"
> > > > > ...
> > > > >
> > > > > [ 52.434761] sound pcmC1D0p: hw_param
> > > > > [ 52.434767] sound pcmC1D0p: ACCESS 0x1
> > > > > [ 52.434770] sound pcmC1D0p: FORMAT 0x4
> > > > > [ 52.434772] sound pcmC1D0p: SUBFORMAT 0x1
> > > > > [ 52.434776] sound pcmC1D0p: SAMPLE_BITS [16:16]
> > > > > [ 52.434779] sound pcmC1D0p: FRAME_BITS [32:32]
> > > > > [ 52.434782] sound pcmC1D0p: CHANNELS [2:2]
> > > > > [ 52.434785] sound pcmC1D0p: RATE [48000:48000]
> > > > > [ 52.434788] sound pcmC1D0p: PERIOD_TIME [5000:5000]
> > > > > [ 52.434791] sound pcmC1D0p: PERIOD_SIZE [240:240]
> > > > > [ 52.434794] sound pcmC1D0p: PERIOD_BYTES [960:960]
> > > > > [ 52.434797] sound pcmC1D0p: PERIODS [852:852]
> > > > > [ 52.434799] sound pcmC1D0p: BUFFER_TIME [426:426]
> > > > > [ 52.434802] sound pcmC1D0p: BUFFER_SIZE [204480:204480]
> > > &g
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Takashi Iwai 於 2020年8月11日 週二 下午3:43寫道：
>
> On Tue, 11 Aug 2020 04:29:24 +0200,
> Yu-Hsuan Hsu wrote:
> >
> > Lu, Brent 於 2020年8月11日 週二 上午10:17寫道：
> > >
> > > >
> > > > Sorry for the late reply. CRAS does not set the period size when using
> > > > it.
> > > > The default period size is 256, which consumes the samples
> > > > quickly(about 49627
> > > > fps when the rate is 48000 fps) at the beginning of the playback.
> > > > Since CRAS write samples with the fixed frequency, it triggers underruns
> > > > immidiately.
> > > >
> > > > According to Brent, the DSP is using 240 period regardless the
> > > > hw_param. If the
> > > > period size is 256, DSP will read 256 samples each time but only
> > > > consume 240
> > > > samples until the ring buffer of DSP is full. This behavior makes the
> > > > samples in
> > > > the ring buffer of kernel consumed quickly. (Not sure whether the
> > > > explanation is
> > > > correct. Need Brent to confirm it.)
> > > >
> > > > Unfortunately, we can not change the behavior of DSP. After some
> > > > experiments,
> > > > we found that the issue can be fixed if we set the period size to 240.
> > > > With the
> > > > same frequency as the DSP, the samples are consumed stably. Because
> > > > everyone
> > > > can trigger this issue when using the driver without setting the period
> > > > size, we
> > > > think it is a general issue that should be fixed in the kernel.
> > >
> > > I check the code and just realized CRAS does nothing but request maximum
> > > buffer
> > > size. As I know the application needs to decide the buffer time and
> > > period time so
> > > ALSA could generate a hw_param structure with proper period size instead
> > > of using
> > > fixed constraint in machine driver because driver has no idea about the
> > > latency you
> > > want.
> > >
> > > You can use snd_pcm_hw_params_set_buffer_time_near() and
> > > snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
> > > buffer and period parameters according to the latency requirement. In the
> > > CRAS
> > > code, there is a UCM variable to support this: DmaPeriodMicrosecs. I
> > > tested it on
> > > Celes and it looks quite promising. It seems to me that adding constraint
> > > in machine
> > > driver is not necessary.
> > >
> > > SectionDevice."Speaker".0 {
> > > Value {
> > > PlaybackPCM "hw:chtrt5650,0"
> > > DmaPeriodMicrosecs "5000"
> > > ...
> > >
> > > [ 52.434761] sound pcmC1D0p: hw_param
> > > [ 52.434767] sound pcmC1D0p: ACCESS 0x1
> > > [ 52.434770] sound pcmC1D0p: FORMAT 0x4
> > > [ 52.434772] sound pcmC1D0p: SUBFORMAT 0x1
> > > [ 52.434776] sound pcmC1D0p: SAMPLE_BITS [16:16]
> > > [ 52.434779] sound pcmC1D0p: FRAME_BITS [32:32]
> > > [ 52.434782] sound pcmC1D0p: CHANNELS [2:2]
> > > [ 52.434785] sound pcmC1D0p: RATE [48000:48000]
> > > [ 52.434788] sound pcmC1D0p: PERIOD_TIME [5000:5000]
> > > [ 52.434791] sound pcmC1D0p: PERIOD_SIZE [240:240]
> > > [ 52.434794] sound pcmC1D0p: PERIOD_BYTES [960:960]
> > > [ 52.434797] sound pcmC1D0p: PERIODS [852:852]
> > > [ 52.434799] sound pcmC1D0p: BUFFER_TIME [426:426]
> > > [ 52.434802] sound pcmC1D0p: BUFFER_SIZE [204480:204480]
> > > [ 52.434805] sound pcmC1D0p: BUFFER_BYTES [817920:817920]
> > > [ 52.434808] sound pcmC1D0p: TICK_TIME [0:0]
> > >
> > > Regards,
> > > Brent
> > Hi Brent,
> >
> > Yes, I know we can do it to fix the issue as well. As I mentioned
> > before, we wanted to fix it in kernel because it is a real issue,
> > isn't it? Basically, a driver should work with any param it supports.
> > But in this case, everyone can trigger underrun if he or she does not
> > the period size to 240. If you still think it's not necessary, I can
> > modify UCM to make CRAS set the appropriate period size.
>
> How does it *not* work if you set other than period size 240, more
> exactly?
>
> The hw_constraint to a fixed period size must be really an exception.
> If you look at other drivers, you won't find any other doing such.
> It already indicates that something is wrong.
>
> Usually the fixed period size comes from the hardware limitation and
> defined in snd_pcm_hardware. Or, sometimes it's an alignment issue.
> If you need more than that, you should doubt what's really not
> working.
>
>
> Takashi
Thank Takashi,
As I mentioned before, if the period size is set to 256, the measured
rate of sample-consuming will be around 49627 fps. It causes underrun
because the rate we set is 48000 fps. This behavior also happen on the
other period rate except for 240.
Thanks,
Yu-Hsuan
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Lu, Brent 於 2020年8月11日 週二 上午10:17寫道：
>
> >
> > Sorry for the late reply. CRAS does not set the period size when using it.
> > The default period size is 256, which consumes the samples quickly(about
> > 49627
> > fps when the rate is 48000 fps) at the beginning of the playback.
> > Since CRAS write samples with the fixed frequency, it triggers underruns
> > immidiately.
> >
> > According to Brent, the DSP is using 240 period regardless the hw_param. If
> > the
> > period size is 256, DSP will read 256 samples each time but only consume 240
> > samples until the ring buffer of DSP is full. This behavior makes the
> > samples in
> > the ring buffer of kernel consumed quickly. (Not sure whether the
> > explanation is
> > correct. Need Brent to confirm it.)
> >
> > Unfortunately, we can not change the behavior of DSP. After some
> > experiments,
> > we found that the issue can be fixed if we set the period size to 240. With
> > the
> > same frequency as the DSP, the samples are consumed stably. Because everyone
> > can trigger this issue when using the driver without setting the period
> > size, we
> > think it is a general issue that should be fixed in the kernel.
>
> I check the code and just realized CRAS does nothing but request maximum
> buffer
> size. As I know the application needs to decide the buffer time and period
> time so
> ALSA could generate a hw_param structure with proper period size instead of
> using
> fixed constraint in machine driver because driver has no idea about the
> latency you
> want.
>
> You can use snd_pcm_hw_params_set_buffer_time_near() and
> snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
> buffer and period parameters according to the latency requirement. In the CRAS
> code, there is a UCM variable to support this: DmaPeriodMicrosecs. I tested
> it on
> Celes and it looks quite promising. It seems to me that adding constraint in
> machine
> driver is not necessary.
>
> SectionDevice."Speaker".0 {
> Value {
> PlaybackPCM "hw:chtrt5650,0"
> DmaPeriodMicrosecs "5000"
> ...
>
> [ 52.434761] sound pcmC1D0p: hw_param
> [ 52.434767] sound pcmC1D0p: ACCESS 0x1
> [ 52.434770] sound pcmC1D0p: FORMAT 0x4
> [ 52.434772] sound pcmC1D0p: SUBFORMAT 0x1
> [ 52.434776] sound pcmC1D0p: SAMPLE_BITS [16:16]
> [ 52.434779] sound pcmC1D0p: FRAME_BITS [32:32]
> [ 52.434782] sound pcmC1D0p: CHANNELS [2:2]
> [ 52.434785] sound pcmC1D0p: RATE [48000:48000]
> [ 52.434788] sound pcmC1D0p: PERIOD_TIME [5000:5000]
> [ 52.434791] sound pcmC1D0p: PERIOD_SIZE [240:240]
> [ 52.434794] sound pcmC1D0p: PERIOD_BYTES [960:960]
> [ 52.434797] sound pcmC1D0p: PERIODS [852:852]
> [ 52.434799] sound pcmC1D0p: BUFFER_TIME [426:426]
> [ 52.434802] sound pcmC1D0p: BUFFER_SIZE [204480:204480]
> [ 52.434805] sound pcmC1D0p: BUFFER_BYTES [817920:817920]
> [ 52.434808] sound pcmC1D0p: TICK_TIME [0:0]
>
> Regards,
> Brent
Hi Brent,
Yes, I know we can do it to fix the issue as well. As I mentioned
before, we wanted to fix it in kernel because it is a real issue,
isn't it? Basically, a driver should work with any param it supports.
But in this case, everyone can trigger underrun if he or she does not
the period size to 240. If you still think it's not necessary, I can
modify UCM to make CRAS set the appropriate period size.
Thanks,
Yu-Hsuan
>
> >
> > Thanks,
> > Yu-Hsuan
Re: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
Pierre-Louis Bossart 於 2020年8月10日 週一 下午11:03寫道： > > > > On 8/6/20 11:41 AM, Lu, Brent wrote: > >> > >> I don't get this. If the platform driver already stated 240 and 960 > >> samples why > >> would 432 be chosen? Doesn't this mean the constraint is not applied? > > > > Hi Pierre, > > > > Sorry for late reply. I used following constraints in V3 patch so any > > period which > > aligns 1ms would be accepted. > > > > + /* > > + * Make sure the period to be multiple of 1ms to align the > > + * design of firmware. Apply same rule to buffer size to make > > + * sure alsa could always find a value for period size > > + * regardless the buffer size given by user space. > > + */ > > + snd_pcm_hw_constraint_step(substream->runtime, 0, > > +SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 48); > > + snd_pcm_hw_constraint_step(substream->runtime, 0, > > +SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 48); > > 432 samples is 9ms, I don't have a clue why/how CRAS might ask for this > value. > > It'd be a bit odd to add constraints just for the purpose of letting > userspace select a sensible value. Sorry for the late reply. CRAS does not set the period size when using it. The default period size is 256, which consumes the samples quickly(about 49627 fps when the rate is 48000 fps) at the beginning of the playback. Since CRAS write samples with the fixed frequency, it triggers underruns immidiately. According to Brent, the DSP is using 240 period regardless the hw_param. If the period size is 256, DSP will read 256 samples each time but only consume 240 samples until the ring buffer of DSP is full. This behavior makes the samples in the ring buffer of kernel consumed quickly. (Not sure whether the explanation is correct. Need Brent to confirm it.) Unfortunately, we can not change the behavior of DSP. After some experiments, we found that the issue can be fixed if we set the period size to 240. With the same frequency as the DSP, the samples are consumed stably. Because everyone can trigger this issue when using the driver without setting the period size, we think it is a general issue that should be fixed in the kernel. Thanks, Yu-Hsuan
Re: [PATCH v2] ASoC: cros_ec_codec: Reset I2S RX when probing
Guenter Roeck 於 2020年7月17日 週五 下午10:32寫道：
>
> On Thu, Jul 16, 2020 at 10:47 AM Enric Balletbo i Serra
> wrote:
> >
> > Hi,
> >
> > On 16/7/20 19:23, Guenter Roeck wrote:
> > > On Thu, Jul 16, 2020 at 10:09 AM Yu-Hsuan Hsu
> > > wrote:
> > >>
> > >> It is not guaranteed that I2S RX is disabled when the kernel booting.
> > >> For example, if the kernel crashes while it is enabled, it will keep
> > >> enabled until the next time EC reboots. Reset I2S RX when probing to
> > >> fix this issue.
> > >>
> > >> Signed-off-by: Yu-Hsuan Hsu
> > >> ---
> > >> drivers/platform/chrome/cros_ec_proto.c| 7 ++-
> > >> include/linux/platform_data/cros_ec_commands.h | 1 +
> > >> sound/soc/codecs/cros_ec_codec.c | 9 +
> > >> 3 files changed, 16 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/platform/chrome/cros_ec_proto.c
> > >> b/drivers/platform/chrome/cros_ec_proto.c
> > >> index 3e745e0fe092c..2c60690d7147c 100644
> > >> --- a/drivers/platform/chrome/cros_ec_proto.c
> > >> +++ b/drivers/platform/chrome/cros_ec_proto.c
> > >> @@ -572,7 +572,12 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device
> > >> *ec_dev,
> > >> return -ENOTSUPP;
> > >> } else if (msg->result != EC_RES_SUCCESS) {
> > >> dev_dbg(ec_dev->dev, "Command result (err: %d)\n",
> > >> msg->result);
> > >> - return -EPROTO;
> > >> + switch (msg->result) {
> > >> + case EC_RES_INVALID_PARAM:
> > >> + return -EINVAL;
> > >
> > > As we have learned, this may impact other callers of
> > > cros_ec_cmd_xfer_status() which only accept -EPROTO as error return
> > > value. In addition to that, the code is odd:
> > >
> > > if (msg->result == EC_RES_INVALID_VERSION) {
> > > ...
> > > } else if (msg->result != EC_RES_SUCCESS) {
> > > switch (msg->result) {
> > >
> > > }
> > > }
> > >
> >
> > Ack, this is odd.
> >
> > > I really dislike the notion of changing error return values of
> > > cros_ec_cmd_xfer_status() one by one. That can only cause ongoing
> > > trouble with callers expecting specific error return codes (as we have
> > > already seen).
> > >
> >
> > Hmm, that's a good point. Ok.
> >
> > Let's apply the Guenter's patch that maps the errors *and* fix the callers
> > of
> > cros_ec_cmd_xfer_status which only accept -EPROTO (there are few).
> >
> > Yu-Hsuan, can you take care of this and send a patch series with all the
> > required patches? If not, I can work on this next week.
> >
>
> I can look into it as well. Let me know - I don't want to duplicate work.
>
> Guenter
Hi Guenter,
Really thanks for your assistance! Could you help me on those patches?
Since you wrote that patch, I think it should be the most efficient
way to make them merged.
Thanks,
Yu-Hsuan
>
> > Thanks,
> > Enric
> >
> > > Guenter
> > >
> > >> + default:
> > >> + return -EPROTO;
> > >> + }
> > >> }
> > >>
> > >> return ret;
> > >> diff --git a/include/linux/platform_data/cros_ec_commands.h
> > >> b/include/linux/platform_data/cros_ec_commands.h
> > >> index 69210881ebac8..11ce917ca924c 100644
> > >> --- a/include/linux/platform_data/cros_ec_commands.h
> > >> +++ b/include/linux/platform_data/cros_ec_commands.h
> > >> @@ -4598,6 +4598,7 @@ enum ec_codec_i2s_rx_subcmd {
> > >> EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
> > >> EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
> > >> EC_CODEC_I2S_RX_SET_BCLK = 0x4,
> > >> + EC_CODEC_I2S_RX_RESET = 0x5,
> > >> EC_CODEC_I2S_RX_SUBCMD_COUNT,
> > >> };
> > >>
> > >> diff --git a/sound/soc/codecs/cros_ec_codec.c
> > >> b/sound/soc/codecs/cros_ec_codec.c
> > >> index f23956cf4ed84..b5ff30b7f1aa8 100644
> > >> --- a/sound/soc/codecs/cros_ec_codec.c
> > >> +++ b/sound/soc/codecs/cros_ec_codec.c
> > >> @@ -1034,6 +1034,15 @@ static int cros_ec_codec_platform_probe(struct
> > >> platform_device *pdev)
> > >> }
> > >> priv->ec_capabilities = r.capabilities;
> > >>
> > >> + /* Reset EC codec I2S RX. */
> > >> + p.cmd = EC_CODEC_I2S_RX_RESET;
> > >> + ret = send_ec_host_command(priv->ec_device,
> > >> EC_CMD_EC_CODEC_I2S_RX,
> > >> + (uint8_t *)&p, sizeof(p), NULL, 0);
> > >> + if (ret == -EINVAL)
> > >> + dev_info(dev, "Missing reset command. Please update your
> > >> EC firmware.\n");
> > >> + else if (ret)
> > >> + dev_err(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret);
> > >> +
> > >> platform_set_drvdata(pdev, priv);
> > >>
> > >> ret = devm_snd_soc_register_component(dev,
> > >> &i2s_rx_component_driver,
> > >> --
> > >> 2.27.0.389.gc38d7665816-goog
> > >>
[PATCH v2] ASoC: cros_ec_codec: Reset I2S RX when probing
It is not guaranteed that I2S RX is disabled when the kernel booting.
For example, if the kernel crashes while it is enabled, it will keep
enabled until the next time EC reboots. Reset I2S RX when probing to
fix this issue.
Signed-off-by: Yu-Hsuan Hsu
---
drivers/platform/chrome/cros_ec_proto.c| 7 ++-
include/linux/platform_data/cros_ec_commands.h | 1 +
sound/soc/codecs/cros_ec_codec.c | 9 +
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c
b/drivers/platform/chrome/cros_ec_proto.c
index 3e745e0fe092c..2c60690d7147c 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -572,7 +572,12 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
return -ENOTSUPP;
} else if (msg->result != EC_RES_SUCCESS) {
dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
- return -EPROTO;
+ switch (msg->result) {
+ case EC_RES_INVALID_PARAM:
+ return -EINVAL;
+ default:
+ return -EPROTO;
+ }
}
return ret;
diff --git a/include/linux/platform_data/cros_ec_commands.h
b/include/linux/platform_data/cros_ec_commands.h
index 69210881ebac8..11ce917ca924c 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4598,6 +4598,7 @@ enum ec_codec_i2s_rx_subcmd {
EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
EC_CODEC_I2S_RX_SET_BCLK = 0x4,
+ EC_CODEC_I2S_RX_RESET = 0x5,
EC_CODEC_I2S_RX_SUBCMD_COUNT,
};
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index f23956cf4ed84..b5ff30b7f1aa8 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -1034,6 +1034,15 @@ static int cros_ec_codec_platform_probe(struct
platform_device *pdev)
}
priv->ec_capabilities = r.capabilities;
+ /* Reset EC codec I2S RX. */
+ p.cmd = EC_CODEC_I2S_RX_RESET;
+ ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
+ (uint8_t *)&p, sizeof(p), NULL, 0);
+ if (ret == -EINVAL)
+ dev_info(dev, "Missing reset command. Please update your EC
firmware.\n");
+ else if (ret)
+ dev_err(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret);
+
platform_set_drvdata(pdev, priv);
ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
--
2.27.0.389.gc38d7665816-goog
Re: [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
Guenter Roeck 於 2020年7月9日 週四 上午1:26寫道：
>
> On Wed, Jul 8, 2020 at 9:17 AM Yu-Hsuan Hsu wrote:
> >
> > Guenter Roeck 於 2020年7月8日 週三 下午9:28寫道：
> > >
> > > On Wed, Jul 8, 2020 at 3:16 AM Enric Balletbo i Serra
> > > wrote:
> > > >
> > > > Hi Yu-Hsuan,
> > > >
> > > > Thank you for your patch.
> > > >
> > > > On 8/7/20 9:11, Yu-Hsuan Hsu wrote:
> > > > > It is not guaranteed that I2S RX is disabled when the kernel booting.
> > > > > For example, if the kernel crashes while it is enabled, it will keep
> > > > > enabled until the next time EC reboots. Reset I2S RX when probing to
> > > > > fix this issue.
> > > > >
> > > > > Signed-off-by: Yu-Hsuan Hsu
> > > > > ---
> > > > > include/linux/platform_data/cros_ec_commands.h | 1 +
> > > > > sound/soc/codecs/cros_ec_codec.c | 7 +++
> > > > > 2 files changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/include/linux/platform_data/cros_ec_commands.h
> > > > > b/include/linux/platform_data/cros_ec_commands.h
> > > > > index 69210881ebac8..11ce917ca924c 100644
> > > > > --- a/include/linux/platform_data/cros_ec_commands.h
> > > > > +++ b/include/linux/platform_data/cros_ec_commands.h
> > > > > @@ -4598,6 +4598,7 @@ enum ec_codec_i2s_rx_subcmd {
> > > > > EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
> > > > > EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
> > > > > EC_CODEC_I2S_RX_SET_BCLK = 0x4,
> > > > > + EC_CODEC_I2S_RX_RESET = 0x5,
> > > >
> > > > Is this a new command not available in the firmware that is already in
> > > > the field?
> > > >
> > > >
> > > > > EC_CODEC_I2S_RX_SUBCMD_COUNT,
> > > > > };
> > > > >
> > > > > diff --git a/sound/soc/codecs/cros_ec_codec.c
> > > > > b/sound/soc/codecs/cros_ec_codec.c
> > > > > index 8d45c628e988e..5495214e73e68 100644
> > > > > --- a/sound/soc/codecs/cros_ec_codec.c
> > > > > +++ b/sound/soc/codecs/cros_ec_codec.c
> > > > > @@ -1034,6 +1034,13 @@ static int cros_ec_codec_platform_probe(struct
> > > > > platform_device *pdev)
> > > > > }
> > > > > priv->ec_capabilities = r.capabilities;
> > > > >
> > > > > + /* Reset EC codec I2S RX. */
> > > > > + p.cmd = EC_CODEC_I2S_RX_RESET;
> > > > > + ret = send_ec_host_command(priv->ec_device,
> > > > > EC_CMD_EC_CODEC_I2S_RX,
> > > > > +(uint8_t *)&p, sizeof(p), NULL, 0);
> > > > > + if (ret)
> > > > > + dev_err(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret);
> > > > > +
> > > >
> > > > With an old firmware I suspect this message will appear on every boot,
> > > > right?
> > > > So, to solve the issue and get rid of this error you're forced to
> > > > upgrade the
> > > > firmware. Is that true?
> > > >
> > >
> > > It might possibly make more sense to fail this silently and to send
> > > EC_CODEC_I2S_RX_DISABLE as backup if it is not supported (-ENOTSUPP
> > > can possibly be used as trigger if the call returns it).
> > >
> > > Also, I don't accept dev_err() if the error is ignored for patches in
> > > my scope of responsibility.
> > >
> > > Guenter
> > Thanks for the suggestion. Our plan is to upstream this patch first.
> > And then we will merge it into the kernel after the firmware is
> > updated. Is it feasible? (I'm not sure whether there is the better way
> > if I want to update EC and the kernel at the same time.)
> >
> > I think calling EC_CODEC_I2S_RX_DISABLE does not make sense because it
> > checks the value of i2s_rx_enabled first. If i2s_rx_enabled is false,
> > it will skip the function. However, we don't need to reset while the
> > i2s_rx_enabled is already false.
> >
> Exactly my point. If i2s_rx_enabled is false, nothing needs to be
> done, and it doesn't hurt if the EC does nothing. If i2s_rx_enabled is
> true, it needs to be set to false, which is accomplished by sending
> EC_CODEC_I2S_RX_DISABLE.
Sorry my bad. If i2s_rx_enabled is false, it will skip and return
EC_RES_BUSY. And then we may need to handle one more error. I think it
may become too complicated to handle those errors. Could we just merge
this change after the firmware updates? So that we don't need to worry
about the unsupported command.
>
> > In addition, since it is a sub-command, it will return
> > EC_RES_INVALID_PARAM but not ENOTSUPP if the command is not supported.
> > And then EC_RES_INVALID_PARAM will turn into -EPROTO finally so it's
> > difficult to do other operators basing on the return value.
> >
>
> You might have to convince Enric to permit another error code to
> translate EC_RES_INVALID_PARAM. After all, that would meet his
> requirement that the error code must be used in the kernel to accept a
> translation.
>
> Guenter
>
> > Thanks,
> > Yu-Hsuan
> >
> > >
> > > > > platform_set_drvdata(pdev, priv);
> > > > >
> > > > > ret = devm_snd_soc_register_component(dev,
> > > > > &i2s_rx_component_driver,
> > > > >
Re: [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
Guenter Roeck 於 2020年7月8日 週三 下午9:28寫道：
>
> On Wed, Jul 8, 2020 at 3:16 AM Enric Balletbo i Serra
> wrote:
> >
> > Hi Yu-Hsuan,
> >
> > Thank you for your patch.
> >
> > On 8/7/20 9:11, Yu-Hsuan Hsu wrote:
> > > It is not guaranteed that I2S RX is disabled when the kernel booting.
> > > For example, if the kernel crashes while it is enabled, it will keep
> > > enabled until the next time EC reboots. Reset I2S RX when probing to
> > > fix this issue.
> > >
> > > Signed-off-by: Yu-Hsuan Hsu
> > > ---
> > > include/linux/platform_data/cros_ec_commands.h | 1 +
> > > sound/soc/codecs/cros_ec_codec.c | 7 +++
> > > 2 files changed, 8 insertions(+)
> > >
> > > diff --git a/include/linux/platform_data/cros_ec_commands.h
> > > b/include/linux/platform_data/cros_ec_commands.h
> > > index 69210881ebac8..11ce917ca924c 100644
> > > --- a/include/linux/platform_data/cros_ec_commands.h
> > > +++ b/include/linux/platform_data/cros_ec_commands.h
> > > @@ -4598,6 +4598,7 @@ enum ec_codec_i2s_rx_subcmd {
> > > EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
> > > EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
> > > EC_CODEC_I2S_RX_SET_BCLK = 0x4,
> > > + EC_CODEC_I2S_RX_RESET = 0x5,
> >
> > Is this a new command not available in the firmware that is already in the
> > field?
> >
> >
> > > EC_CODEC_I2S_RX_SUBCMD_COUNT,
> > > };
> > >
> > > diff --git a/sound/soc/codecs/cros_ec_codec.c
> > > b/sound/soc/codecs/cros_ec_codec.c
> > > index 8d45c628e988e..5495214e73e68 100644
> > > --- a/sound/soc/codecs/cros_ec_codec.c
> > > +++ b/sound/soc/codecs/cros_ec_codec.c
> > > @@ -1034,6 +1034,13 @@ static int cros_ec_codec_platform_probe(struct
> > > platform_device *pdev)
> > > }
> > > priv->ec_capabilities = r.capabilities;
> > >
> > > + /* Reset EC codec I2S RX. */
> > > + p.cmd = EC_CODEC_I2S_RX_RESET;
> > > + ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
> > > +(uint8_t *)&p, sizeof(p), NULL, 0);
> > > + if (ret)
> > > + dev_err(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret);
> > > +
> >
> > With an old firmware I suspect this message will appear on every boot,
> > right?
> > So, to solve the issue and get rid of this error you're forced to upgrade
> > the
> > firmware. Is that true?
> >
>
> It might possibly make more sense to fail this silently and to send
> EC_CODEC_I2S_RX_DISABLE as backup if it is not supported (-ENOTSUPP
> can possibly be used as trigger if the call returns it).
>
> Also, I don't accept dev_err() if the error is ignored for patches in
> my scope of responsibility.
>
> Guenter
Thanks for the suggestion. Our plan is to upstream this patch first.
And then we will merge it into the kernel after the firmware is
updated. Is it feasible? (I'm not sure whether there is the better way
if I want to update EC and the kernel at the same time.)
I think calling EC_CODEC_I2S_RX_DISABLE does not make sense because it
checks the value of i2s_rx_enabled first. If i2s_rx_enabled is false,
it will skip the function. However, we don't need to reset while the
i2s_rx_enabled is already false.
In addition, since it is a sub-command, it will return
EC_RES_INVALID_PARAM but not ENOTSUPP if the command is not supported.
And then EC_RES_INVALID_PARAM will turn into -EPROTO finally so it's
difficult to do other operators basing on the return value.
Thanks,
Yu-Hsuan
>
> > > platform_set_drvdata(pdev, priv);
> > >
> > > ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
> > >
[PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
It is not guaranteed that I2S RX is disabled when the kernel booting.
For example, if the kernel crashes while it is enabled, it will keep
enabled until the next time EC reboots. Reset I2S RX when probing to
fix this issue.
Signed-off-by: Yu-Hsuan Hsu
---
include/linux/platform_data/cros_ec_commands.h | 1 +
sound/soc/codecs/cros_ec_codec.c | 7 +++
2 files changed, 8 insertions(+)
diff --git a/include/linux/platform_data/cros_ec_commands.h
b/include/linux/platform_data/cros_ec_commands.h
index 69210881ebac8..11ce917ca924c 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4598,6 +4598,7 @@ enum ec_codec_i2s_rx_subcmd {
EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH = 0x2,
EC_CODEC_I2S_RX_SET_DAIFMT = 0x3,
EC_CODEC_I2S_RX_SET_BCLK = 0x4,
+ EC_CODEC_I2S_RX_RESET = 0x5,
EC_CODEC_I2S_RX_SUBCMD_COUNT,
};
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index 8d45c628e988e..5495214e73e68 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -1034,6 +1034,13 @@ static int cros_ec_codec_platform_probe(struct
platform_device *pdev)
}
priv->ec_capabilities = r.capabilities;
+ /* Reset EC codec I2S RX. */
+ p.cmd = EC_CODEC_I2S_RX_RESET;
+ ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
+ (uint8_t *)&p, sizeof(p), NULL, 0);
+ if (ret)
+ dev_err(dev, "failed to EC_CODEC_I2S_RESET: %d\n", ret);
+
platform_set_drvdata(pdev, priv);
ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
--
2.27.0.383.g050319c2ae-goog
Re: [PATCH v2] ASoC: cros_ec_codec: Log results when EC commands fail
Yu-Hsuan Hsu 於 2020年7月4日 週六 下午7:34寫道：
>
> Guenter Roeck 於 2020年7月4日 週六 上午3:28寫道：
> >
> > On Fri, Jul 3, 2020 at 12:11 PM Yu-Hsuan Hsu wrote:
> > >
> > > Guenter Roeck 於 2020年7月3日 週五 下午11:58寫道：
> > > >
> > > > On Fri, Jul 3, 2020 at 3:56 AM Enric Balletbo i Serra
> > > > wrote:
> > > > >
> > > > > Hi Yu-Hsuan,
> > > > >
> > > > > On 3/7/20 11:40, Yu-Hsuan Hsu wrote:
> > > > > > Enric Balletbo i Serra 於 2020年7月3日
> > > > > > 週五 下午5:19寫道：
> > > > > >>
> > > > > >> Hi Yu-Hsuan,
> > > > > >>
> > > > > >> On 3/7/20 10:48, Yu-Hsuan Hsu wrote:
> > > > > >>> Enric Balletbo i Serra 於 2020年7月3日
> > > > > >>> 週五 下午4:38寫道：
> > > > > >>>>
> > > > > >>>> Hi Yu-Hsuan,
> > > > > >>>>
> > > > > >>>> Thank you for your patch
> > > > > >>>>
> > > > > >>>> On 3/7/20 9:19, Yu-Hsuan Hsu wrote:
> > > > > >>>>> Log results of failed EC commands to identify a problem more
> > > > > >>>>> easily.
> > > > > >>>>>
> > > > > >>>>> Replace cros_ec_cmd_xfer_status with cros_ec_cmd_xfer because
> > > > > >>>>> the result
> > > > > >>>>> has already been checked in this function. The wrapper is not
> > > > > >>>>> needed.
> > > > > >>>>>
> > > > > >>>>
> > > > > >>>> Nack, we did an effort to remove all public users of
> > > > > >>>> cros_ec_cmd_xfer() in
> > > > > >>>> favour of cros_ec_cmd_xfer_status() and you are reintroducing
> > > > > >>>> again. You can do
> > > > > >>>> the same but using cros_ec_cmd_xfer_status(). In fact, your
> > > > > >>>> patch will not build
> > > > > >>>> on top of the upcoming changes.
> > > > > >>> Thanks! But I have a question about implementing it. Does it look
> > > > > >>> like
> > > > > >>> the one below?
> > > > > >>> ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > > > > >>> if (ret < 0) {
> > > > > >>
> > > > > >> In this case will already print an error.
> > > > > >>
> > > > > >> What are you trying to achieve?
> > > > > >>
> > > > > >> If the only reason is of this patch is print a message you should
> > > > > >> either, or
> > > > > >> enable dynamic printk and enable dev_dbg or event better use the
> > > > > >> kernel trace
> > > > > >> functionality. There is no need to be more verbose.
> > > > > >>
> > > > > >> Example:
> > > > > >> $ echo 1 > /sys/kernel/debug/tracing/events/cros_ec/enable
> > > > > >> $ cat /sys/kernel/debug/tracing/trace
> > > > > >>
> > > > > >> 369.416372: cros_ec_request_start: version: 0, command:
> > > > > >> EC_CMD_USB_PD_POWER_INFO
> > > > > >> 369.420528: cros_ec_request_done: version: 0, command:
> > > > > >> EC_CMD_USB_PD_POWER_INFO, ec result: EC_RES_SUCCESS, retval: 16
> > > > > >>
> > > > > >> Cheers,
> > > > > >> Enric
> > > > > >>
> > > > > > Thank Enric,
> > > > > >
> > > > > > The situation is that some users encountered errors on ChromeBook.
> > > > >
> > > > > And, aren't you able to reproduce the issue?
> > > > >
> > > > >
> > > > > > From their feedback reports, we only get the message like
> > > > > > 'cros-ec-codec GOOG0013:00: ASoC: Failed to set DAI format: -71'.
> > > > > > We know that -71 is -EPROTO but it is not clear enough for us to
> > > > > > find
> > > > > > out the root cause. That's why we want the detail of the result.
> > > > >
> > > > >
> > >
Re: [PATCH v2] ASoC: cros_ec_codec: Log results when EC commands fail
Guenter Roeck 於 2020年7月4日 週六 上午3:28寫道：
>
> On Fri, Jul 3, 2020 at 12:11 PM Yu-Hsuan Hsu wrote:
> >
> > Guenter Roeck 於 2020年7月3日 週五 下午11:58寫道：
> > >
> > > On Fri, Jul 3, 2020 at 3:56 AM Enric Balletbo i Serra
> > > wrote:
> > > >
> > > > Hi Yu-Hsuan,
> > > >
> > > > On 3/7/20 11:40, Yu-Hsuan Hsu wrote:
> > > > > Enric Balletbo i Serra 於 2020年7月3日 週五
> > > > > 下午5:19寫道：
> > > > >>
> > > > >> Hi Yu-Hsuan,
> > > > >>
> > > > >> On 3/7/20 10:48, Yu-Hsuan Hsu wrote:
> > > > >>> Enric Balletbo i Serra 於 2020年7月3日
> > > > >>> 週五 下午4:38寫道：
> > > > >>>>
> > > > >>>> Hi Yu-Hsuan,
> > > > >>>>
> > > > >>>> Thank you for your patch
> > > > >>>>
> > > > >>>> On 3/7/20 9:19, Yu-Hsuan Hsu wrote:
> > > > >>>>> Log results of failed EC commands to identify a problem more
> > > > >>>>> easily.
> > > > >>>>>
> > > > >>>>> Replace cros_ec_cmd_xfer_status with cros_ec_cmd_xfer because the
> > > > >>>>> result
> > > > >>>>> has already been checked in this function. The wrapper is not
> > > > >>>>> needed.
> > > > >>>>>
> > > > >>>>
> > > > >>>> Nack, we did an effort to remove all public users of
> > > > >>>> cros_ec_cmd_xfer() in
> > > > >>>> favour of cros_ec_cmd_xfer_status() and you are reintroducing
> > > > >>>> again. You can do
> > > > >>>> the same but using cros_ec_cmd_xfer_status(). In fact, your patch
> > > > >>>> will not build
> > > > >>>> on top of the upcoming changes.
> > > > >>> Thanks! But I have a question about implementing it. Does it look
> > > > >>> like
> > > > >>> the one below?
> > > > >>> ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > > > >>> if (ret < 0) {
> > > > >>
> > > > >> In this case will already print an error.
> > > > >>
> > > > >> What are you trying to achieve?
> > > > >>
> > > > >> If the only reason is of this patch is print a message you should
> > > > >> either, or
> > > > >> enable dynamic printk and enable dev_dbg or event better use the
> > > > >> kernel trace
> > > > >> functionality. There is no need to be more verbose.
> > > > >>
> > > > >> Example:
> > > > >> $ echo 1 > /sys/kernel/debug/tracing/events/cros_ec/enable
> > > > >> $ cat /sys/kernel/debug/tracing/trace
> > > > >>
> > > > >> 369.416372: cros_ec_request_start: version: 0, command:
> > > > >> EC_CMD_USB_PD_POWER_INFO
> > > > >> 369.420528: cros_ec_request_done: version: 0, command:
> > > > >> EC_CMD_USB_PD_POWER_INFO, ec result: EC_RES_SUCCESS, retval: 16
> > > > >>
> > > > >> Cheers,
> > > > >> Enric
> > > > >>
> > > > > Thank Enric,
> > > > >
> > > > > The situation is that some users encountered errors on ChromeBook.
> > > >
> > > > And, aren't you able to reproduce the issue?
> > > >
> > > >
> > > > > From their feedback reports, we only get the message like
> > > > > 'cros-ec-codec GOOG0013:00: ASoC: Failed to set DAI format: -71'.
> > > > > We know that -71 is -EPROTO but it is not clear enough for us to find
> > > > > out the root cause. That's why we want the detail of the result.
> > > >
> > > >
> > > > If I am not mistaken this ends calling i2s_rx_set_daifmt() into the EC
> > > > firmware,
> > > > if the result is -EPROTO that means is not returning EC_RES_SUCCESS, so
> > > > there
> > > > are few options:
> > > >
> > > > if (i2s_rx_enabled)
> > > > return EC_RES_BUSY;
> > > >
> > > > if (daifmt >= EC_CODEC_I2S_RX_DAIFMT_COUNT)
>
Re: [PATCH v2] ASoC: cros_ec_codec: Log results when EC commands fail
Guenter Roeck 於 2020年7月3日 週五 下午11:58寫道：
>
> On Fri, Jul 3, 2020 at 3:56 AM Enric Balletbo i Serra
> wrote:
> >
> > Hi Yu-Hsuan,
> >
> > On 3/7/20 11:40, Yu-Hsuan Hsu wrote:
> > > Enric Balletbo i Serra 於 2020年7月3日 週五
> > > 下午5:19寫道：
> > >>
> > >> Hi Yu-Hsuan,
> > >>
> > >> On 3/7/20 10:48, Yu-Hsuan Hsu wrote:
> > >>> Enric Balletbo i Serra 於 2020年7月3日 週五
> > >>> 下午4:38寫道：
> > >>>>
> > >>>> Hi Yu-Hsuan,
> > >>>>
> > >>>> Thank you for your patch
> > >>>>
> > >>>> On 3/7/20 9:19, Yu-Hsuan Hsu wrote:
> > >>>>> Log results of failed EC commands to identify a problem more easily.
> > >>>>>
> > >>>>> Replace cros_ec_cmd_xfer_status with cros_ec_cmd_xfer because the
> > >>>>> result
> > >>>>> has already been checked in this function. The wrapper is not needed.
> > >>>>>
> > >>>>
> > >>>> Nack, we did an effort to remove all public users of
> > >>>> cros_ec_cmd_xfer() in
> > >>>> favour of cros_ec_cmd_xfer_status() and you are reintroducing again.
> > >>>> You can do
> > >>>> the same but using cros_ec_cmd_xfer_status(). In fact, your patch will
> > >>>> not build
> > >>>> on top of the upcoming changes.
> > >>> Thanks! But I have a question about implementing it. Does it look like
> > >>> the one below?
> > >>> ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > >>> if (ret < 0) {
> > >>
> > >> In this case will already print an error.
> > >>
> > >> What are you trying to achieve?
> > >>
> > >> If the only reason is of this patch is print a message you should
> > >> either, or
> > >> enable dynamic printk and enable dev_dbg or event better use the kernel
> > >> trace
> > >> functionality. There is no need to be more verbose.
> > >>
> > >> Example:
> > >> $ echo 1 > /sys/kernel/debug/tracing/events/cros_ec/enable
> > >> $ cat /sys/kernel/debug/tracing/trace
> > >>
> > >> 369.416372: cros_ec_request_start: version: 0, command:
> > >> EC_CMD_USB_PD_POWER_INFO
> > >> 369.420528: cros_ec_request_done: version: 0, command:
> > >> EC_CMD_USB_PD_POWER_INFO, ec result: EC_RES_SUCCESS, retval: 16
> > >>
> > >> Cheers,
> > >> Enric
> > >>
> > > Thank Enric,
> > >
> > > The situation is that some users encountered errors on ChromeBook.
> >
> > And, aren't you able to reproduce the issue?
> >
> >
> > > From their feedback reports, we only get the message like
> > > 'cros-ec-codec GOOG0013:00: ASoC: Failed to set DAI format: -71'.
> > > We know that -71 is -EPROTO but it is not clear enough for us to find
> > > out the root cause. That's why we want the detail of the result.
> >
> >
> > If I am not mistaken this ends calling i2s_rx_set_daifmt() into the EC
> > firmware,
> > if the result is -EPROTO that means is not returning EC_RES_SUCCESS, so
> > there
> > are few options:
> >
> > if (i2s_rx_enabled)
> > return EC_RES_BUSY;
> >
> > if (daifmt >= EC_CODEC_I2S_RX_DAIFMT_COUNT)
> > return EC_RES_INVALID_PARAM;
> >
> > if (audio_codec_i2s_rx_set_daifmt(daifmt) != EC_SUCCESS)
> > return EC_RES_ERROR;
> >
> > > Because the situation happens on users' side, it is not possible for
> > > them to enable kernel trace (ChromeOS does not allow users to touch
> > > kernel).
> > >
> >
> > Are you sure that when you know the error code you'll find the root cause
> > (without adding more prints)? There is only three possibilities? You can't
> > start
> > adding prints just to debug a user issue because you don't allow to be more
> > verbose. I understand that might help you but is not the way to go.
Hi Enric and Guenter,
Thanks for your inspiring comments.
I'm not sure whether we will find the root cause if I know the error
code. But I think it's not a point.
We wanted to add this error log because we found that the current one
is not enough. Since it is a real error, it would be better if we ca
Re: [PATCH v2] ASoC: cros_ec_codec: Log results when EC commands fail
Enric Balletbo i Serra 於 2020年7月3日 週五 下午5:19寫道：
>
> Hi Yu-Hsuan,
>
> On 3/7/20 10:48, Yu-Hsuan Hsu wrote:
> > Enric Balletbo i Serra 於 2020年7月3日 週五
> > 下午4:38寫道：
> >>
> >> Hi Yu-Hsuan,
> >>
> >> Thank you for your patch
> >>
> >> On 3/7/20 9:19, Yu-Hsuan Hsu wrote:
> >>> Log results of failed EC commands to identify a problem more easily.
> >>>
> >>> Replace cros_ec_cmd_xfer_status with cros_ec_cmd_xfer because the result
> >>> has already been checked in this function. The wrapper is not needed.
> >>>
> >>
> >> Nack, we did an effort to remove all public users of cros_ec_cmd_xfer() in
> >> favour of cros_ec_cmd_xfer_status() and you are reintroducing again. You
> >> can do
> >> the same but using cros_ec_cmd_xfer_status(). In fact, your patch will not
> >> build
> >> on top of the upcoming changes.
> > Thanks! But I have a question about implementing it. Does it look like
> > the one below?
> > ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > if (ret < 0) {
>
> In this case will already print an error.
>
> What are you trying to achieve?
>
> If the only reason is of this patch is print a message you should either, or
> enable dynamic printk and enable dev_dbg or event better use the kernel trace
> functionality. There is no need to be more verbose.
>
> Example:
> $ echo 1 > /sys/kernel/debug/tracing/events/cros_ec/enable
> $ cat /sys/kernel/debug/tracing/trace
>
> 369.416372: cros_ec_request_start: version: 0, command:
> EC_CMD_USB_PD_POWER_INFO
> 369.420528: cros_ec_request_done: version: 0, command:
> EC_CMD_USB_PD_POWER_INFO, ec result: EC_RES_SUCCESS, retval: 16
>
> Cheers,
> Enric
>
Thank Enric,
The situation is that some users encountered errors on ChromeBook.
>From their feedback reports, we only get the message like
'cros-ec-codec GOOG0013:00: ASoC: Failed to set DAI format: -71'.
We know that -71 is -EPROTO but it is not clear enough for us to find
out the root cause. That's why we want the detail of the result.
Because the situation happens on users' side, it is not possible for
them to enable kernel trace (ChromeOS does not allow users to touch
kernel).
The other way we thought is changing dev_dbg to dev_err in
cros_ec_cmd_xfer_status. But we are not sure whether it is also an
error for other usages.
> > if (ret == -EPROTO)
> > dev_err(..., msg->result)
> > goto error;
> > }
> > I'm not sure whether it makes sense to check ret == -EPROTO here.
> >
> >>
> >>> Signed-off-by: Yu-Hsuan Hsu
> >>> ---
> >>> sound/soc/codecs/cros_ec_codec.c | 9 -
> >>> 1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/sound/soc/codecs/cros_ec_codec.c
> >>> b/sound/soc/codecs/cros_ec_codec.c
> >>> index 8d45c628e988e..a4ab62f59efa6 100644
> >>> --- a/sound/soc/codecs/cros_ec_codec.c
> >>> +++ b/sound/soc/codecs/cros_ec_codec.c
> >>> @@ -90,10 +90,17 @@ static int send_ec_host_command(struct cros_ec_device
> >>> *ec_dev, uint32_t cmd,
> >>> if (outsize)
> >>> memcpy(msg->data, out, outsize);
> >>>
> >>> - ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> >>> + ret = cros_ec_cmd_xfer(ec_dev, msg);
> >>> if (ret < 0)
> >>> goto error;
> >>>
> >>> + if (msg->result != EC_RES_SUCCESS) {
> >>> + dev_err(ec_dev->dev, "Command %d failed: %d\n", cmd,
> >>> + msg->result);
> >>> + ret = -EPROTO;
> >>> + goto error;
> >>> + }
> >>> +
> >>> if (insize)
> >>> memcpy(in, msg->data, insize);
> >>>
> >>>
Re: [PATCH v2] ASoC: cros_ec_codec: Log results when EC commands fail
Enric Balletbo i Serra 於 2020年7月3日 週五 下午4:38寫道：
>
> Hi Yu-Hsuan,
>
> Thank you for your patch
>
> On 3/7/20 9:19, Yu-Hsuan Hsu wrote:
> > Log results of failed EC commands to identify a problem more easily.
> >
> > Replace cros_ec_cmd_xfer_status with cros_ec_cmd_xfer because the result
> > has already been checked in this function. The wrapper is not needed.
> >
>
> Nack, we did an effort to remove all public users of cros_ec_cmd_xfer() in
> favour of cros_ec_cmd_xfer_status() and you are reintroducing again. You can
> do
> the same but using cros_ec_cmd_xfer_status(). In fact, your patch will not
> build
> on top of the upcoming changes.
Thanks! But I have a question about implementing it. Does it look like
the one below?
ret = cros_ec_cmd_xfer_status(ec_dev, msg);
if (ret < 0) {
if (ret == -EPROTO)
dev_err(..., msg->result)
goto error;
}
I'm not sure whether it makes sense to check ret == -EPROTO here.
>
> > Signed-off-by: Yu-Hsuan Hsu
> > ---
> > sound/soc/codecs/cros_ec_codec.c | 9 -
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/codecs/cros_ec_codec.c
> > b/sound/soc/codecs/cros_ec_codec.c
> > index 8d45c628e988e..a4ab62f59efa6 100644
> > --- a/sound/soc/codecs/cros_ec_codec.c
> > +++ b/sound/soc/codecs/cros_ec_codec.c
> > @@ -90,10 +90,17 @@ static int send_ec_host_command(struct cros_ec_device
> > *ec_dev, uint32_t cmd,
> > if (outsize)
> > memcpy(msg->data, out, outsize);
> >
> > - ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > + ret = cros_ec_cmd_xfer(ec_dev, msg);
> > if (ret < 0)
> > goto error;
> >
> > + if (msg->result != EC_RES_SUCCESS) {
> > + dev_err(ec_dev->dev, "Command %d failed: %d\n", cmd,
> > + msg->result);
> > + ret = -EPROTO;
> > + goto error;
> > + }
> > +
> > if (insize)
> > memcpy(in, msg->data, insize);
> >
> >
Re: [PATCH v2] ASoC: cros_ec_codec: Log results when EC commands fail
Tzung-Bi Shih 於 2020年7月3日 週五 下午3:32寫道：
>
> On Fri, Jul 3, 2020 at 3:19 PM Yu-Hsuan Hsu wrote:
> > Log results of failed EC commands to identify a problem more easily.
> >
> > Replace cros_ec_cmd_xfer_status with cros_ec_cmd_xfer because the result
> > has already been checked in this function. The wrapper is not needed.
>
> Alternatively, you can still use cros_ec_cmd_xfer_status( ). I guess
> it is okay to have 2 logs for an error.
>
> > diff --git a/sound/soc/codecs/cros_ec_codec.c
> > b/sound/soc/codecs/cros_ec_codec.c
> > index 8d45c628e988e..a4ab62f59efa6 100644
> > --- a/sound/soc/codecs/cros_ec_codec.c
> > +++ b/sound/soc/codecs/cros_ec_codec.c
> > @@ -90,10 +90,17 @@ static int send_ec_host_command(struct cros_ec_device
> > *ec_dev, uint32_t cmd,
> > if (outsize)
> > memcpy(msg->data, out, outsize);
> >
> > - ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > + ret = cros_ec_cmd_xfer(ec_dev, msg);
> > if (ret < 0)
> I am thinking of if it is a better solution to print msg->result here.
The problem is the msg->result is not always meaningful.
In cros_ec_cmd_xfer_status, we know that the msg->result is meaningful
only when ret == 0. Therefore, we can not print the msg->result
directly here.
In addition, adding a conditional operator here to check whether ret
is -EPROTO is not a good way, either.
We should consider the situation that cros_ec_cmd_xfer may return
-EPROTO directly.
>
> > goto error;
> >
> > + if (msg->result != EC_RES_SUCCESS) {
> > + dev_err(ec_dev->dev, "Command %d failed: %d\n", cmd,
> > + msg->result);
> > + ret = -EPROTO;
> > + goto error;
> > + }
> So that you don't need this block.
[PATCH v2] ASoC: cros_ec_codec: Log results when EC commands fail
Log results of failed EC commands to identify a problem more easily.
Replace cros_ec_cmd_xfer_status with cros_ec_cmd_xfer because the result
has already been checked in this function. The wrapper is not needed.
Signed-off-by: Yu-Hsuan Hsu
---
sound/soc/codecs/cros_ec_codec.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index 8d45c628e988e..a4ab62f59efa6 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -90,10 +90,17 @@ static int send_ec_host_command(struct cros_ec_device
*ec_dev, uint32_t cmd,
if (outsize)
memcpy(msg->data, out, outsize);
- ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+ ret = cros_ec_cmd_xfer(ec_dev, msg);
if (ret < 0)
goto error;
+ if (msg->result != EC_RES_SUCCESS) {
+ dev_err(ec_dev->dev, "Command %d failed: %d\n", cmd,
+ msg->result);
+ ret = -EPROTO;
+ goto error;
+ }
+
if (insize)
memcpy(in, msg->data, insize);
--
2.27.0.212.ge8ba1cc988-goog
[PATCH] ASoC: cros_ec_codec: Log results when EC commands fail
Log results of failed EC commands to identify a problem more easily.
Signed-off-by: Yu-Hsuan Hsu
---
sound/soc/codecs/cros_ec_codec.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index 8d45c628e988e..a4ab62f59efa6 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -90,10 +90,17 @@ static int send_ec_host_command(struct cros_ec_device
*ec_dev, uint32_t cmd,
if (outsize)
memcpy(msg->data, out, outsize);
- ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+ ret = cros_ec_cmd_xfer(ec_dev, msg);
if (ret < 0)
goto error;
+ if (msg->result != EC_RES_SUCCESS) {
+ dev_err(ec_dev->dev, "Command %d failed: %d\n", cmd,
+ msg->result);
+ ret = -EPROTO;
+ goto error;
+ }
+
if (insize)
memcpy(in, msg->data, insize);
--
2.27.0.212.ge8ba1cc988-goog
Re: [PATCH] ASoC: rockchip: add format and rate constraints on rk3399
Mark Brown 於 2020年6月30日 週二 下午6:40寫道： > > On Tue, Jun 30, 2020 at 05:16:15PM +0800, Yu-Hsuan Hsu wrote: > > S8 and S24 formats does not work on this machine driver so force to use > > S16_LE instead. > > > In addition, add constraint to limit the max value of rate because the > > rate higher than 96000(172000, 192000) is not stable either. > > What is the source of these restrictions - are they due to the component > devices? If they are then the component devices ought to be setting > suitable constraints themselves. I'm not sure why it happened. But I guess it is not the issues on those components because they work normally on other machine drivers. Because this driver is old and lacks maintenance, I think we can just filter some unstable formats. Actually, those formats are very rarely used as well.
[PATCH] ASoC: rockchip: add format and rate constraints on rk3399
S8 and S24 formats does not work on this machine driver so force to use
S16_LE instead.
In addition, add constraint to limit the max value of rate because the
rate higher than 96000(172000, 192000) is not stable either.
Signed-off-by: Yu-Hsuan Hsu
---
sound/soc/rockchip/rk3399_gru_sound.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/sound/soc/rockchip/rk3399_gru_sound.c
b/sound/soc/rockchip/rk3399_gru_sound.c
index f45e5aaa4b302..9539b0d024fed 100644
--- a/sound/soc/rockchip/rk3399_gru_sound.c
+++ b/sound/soc/rockchip/rk3399_gru_sound.c
@@ -219,19 +219,32 @@ static int rockchip_sound_dmic_hw_params(struct
snd_pcm_substream *substream,
return 0;
}
+static int rockchip_sound_startup(struct snd_pcm_substream *substream)
+{
+ struct snd_pcm_runtime *runtime = substream->runtime;
+
+ runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
+ return snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_RATE,
+ 8000, 96000);
+}
+
static const struct snd_soc_ops rockchip_sound_max98357a_ops = {
+ .startup = rockchip_sound_startup,
.hw_params = rockchip_sound_max98357a_hw_params,
};
static const struct snd_soc_ops rockchip_sound_rt5514_ops = {
+ .startup = rockchip_sound_startup,
.hw_params = rockchip_sound_rt5514_hw_params,
};
static const struct snd_soc_ops rockchip_sound_da7219_ops = {
+ .startup = rockchip_sound_startup,
.hw_params = rockchip_sound_da7219_hw_params,
};
static const struct snd_soc_ops rockchip_sound_dmic_ops = {
+ .startup = rockchip_sound_startup,
.hw_params = rockchip_sound_dmic_hw_params,
};
--
2.27.0.212.ge8ba1cc988-goog
[PATCH v2] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
On KBL platform, the microphone is attached to external codec(rt5514) instead of PCH. However, TDM slot between PCH and codec is 16 bits only. In order to avoid setting wrong format, we should add a constraint to force to use 16 bits format forever. Signed-off-by: Yu-Hsuan Hsu --- I have updated the commit message. Please see whether it is clear enough. Thanks. sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index 74dda8784f1a01..67b276a65a8d2d 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct snd_pcm_substream *substream) snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, dmic_constraints); + runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE; + snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16); + return snd_pcm_hw_constraint_list(substream->runtime, 0, SNDRV_PCM_HW_PARAM_RATE, &constraints_rates); } -- 2.23.0.351.gc4317032e6-goog
Re: [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
Pierre-Louis Bossart 於 2019年9月14日 週六 上午1:28寫道： > > please don't top-post on public mailing lists, thanks. > > On 9/13/19 9:45 AM, Yu-Hsuan Hsu wrote: > > Thanks for the review! If I'm not mistaken, the microphone is attached > > to external codec(rt5514) instead of PCH on Kabylake platform. So there > > should be a TDM between DMICs and PCH. We can see in the > > kabylake_ssp0_hw_params function, there are some operations about > > setting tdm slot_width to 16 bits. Therefore, I think it only supports > > S16_LE format for DMICs. Is it correct? > > Ah yes, ok. we have other machine drivers where dmic refers to the PCH > attached case, thanks for the precision. > > I am still not clear on the problem, you are adding this constraint to a > front-end, so in theory the copier element in the firmware should take > care of converting from 16-bits recorded on the TDM link to the 24 bits > used by the application. Is this not the case? is this patch based on an > actual error and if yes can you share more information to help check > where the problem happens, topology maybe? If we use 24 bits format on that device to record, the audio samples it returns are still in 16 bits. So the rate we measured is only the half of the expected rate. It's a real problem. Apart from the rate, the audio samples are also wrong if we still decode them with 24 bits format. Therefore, the better fix is to add a constraint to remove 24bits support. By the way, we found this issue by "ALSA conformance test", which is a new tool to verify audio drivers. (https://chromium.googlesource.com/chromiumos/platform/audiotest/+/master/alsa_conformance_test.md) > > > > > Pierre-Louis Bossart > <mailto:pierre-louis.boss...@linux.intel.com>> 於 2019年9月12日 週四 下 > > 午9:02寫道： > > > > On 9/11/19 9:27 PM, Yu-Hsuan Hsu wrote: > > > 24 bits recording from DMIC is not supported for KBL platform because > > > the TDM slot between PCH and codec is 16 bits only. We should add a > > > constraint to remove that unsupported format. > > > > Humm, when you use DMICs they are directly connected to the PCH with a > > standard 1-bit PDM. There is no notion of TDM or slot. > > > > It could very well be that the firmware/topology only support 16 bit (I > > vaguely recall another case where 24 bits was added), but the > > description in the commit message would need to be modified to make the > > reason for this change clearer. > > > > > > > > Signed-off-by: Yu-Hsuan Hsu > <mailto:yuhs...@chromium.org>> > > > --- > > > sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c > > b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c > > > index 74dda8784f1a01..67b276a65a8d2d 100644 > > > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c > > > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c > > > @@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct > > snd_pcm_substream *substream) > > > snd_pcm_hw_constraint_list(runtime, 0, > > SNDRV_PCM_HW_PARAM_CHANNELS, > > > dmic_constraints); > > > > > > + runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE; > > > + snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16); > > > + > > > return snd_pcm_hw_constraint_list(substream->runtime, 0, > > > SNDRV_PCM_HW_PARAM_RATE, &constraints_rates); > > > } > > > > > >
Re: [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
Pierre-Louis Bossart 於 2019年9月12日 週四 下午9:02寫道： > > On 9/11/19 9:27 PM, Yu-Hsuan Hsu wrote: > > 24 bits recording from DMIC is not supported for KBL platform because > > the TDM slot between PCH and codec is 16 bits only. We should add a > > constraint to remove that unsupported format. > > Humm, when you use DMICs they are directly connected to the PCH with a > standard 1-bit PDM. There is no notion of TDM or slot. > > It could very well be that the firmware/topology only support 16 bit (I > vaguely recall another case where 24 bits was added), but the > description in the commit message would need to be modified to make the > reason for this change clearer. (I sent it again because the previous email contains HTML subpart. Sorry for the inconvenience.) Thanks for the review! If I'm not mistaken, the microphone is attached to external codec(rt5514) instead of PCH on Kabylake platform. So there should be a TDM between DMICs and PCH. We can see in the kabylake_ssp0_hw_params function, there are some operations about setting tdm slot_width to 16 bits. Therefore, I think it only supports S16_LE format for DMICs. Is it correct? > > > > > > > Signed-off-by: Yu-Hsuan Hsu > > --- > > sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c > > b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c > > index 74dda8784f1a01..67b276a65a8d2d 100644 > > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c > > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c > > @@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct > > snd_pcm_substream *substream) > > snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, > > dmic_constraints); > > > > + runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE; > > + snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16); > > + > > return snd_pcm_hw_constraint_list(substream->runtime, 0, > > SNDRV_PCM_HW_PARAM_RATE, &constraints_rates); > > } > > >
[PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
24 bits recording from DMIC is not supported for KBL platform because the TDM slot between PCH and codec is 16 bits only. We should add a constraint to remove that unsupported format. Signed-off-by: Yu-Hsuan Hsu --- sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index 74dda8784f1a01..67b276a65a8d2d 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct snd_pcm_substream *substream) snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, dmic_constraints); + runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE; + snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16); + return snd_pcm_hw_constraint_list(substream->runtime, 0, SNDRV_PCM_HW_PARAM_RATE, &constraints_rates); } -- 2.23.0.162.g0b9fbb3734-goog
[PATCH v6] ASoC: max98090: remove 24-bit format support if RJ is 0
The supported formats are S16_LE and S24_LE now. However, S24_LE is
not supported when TDM is 0 and it is not in the right justified mode.
We should remove 24-bit format in that situation to avoid triggering
error.
Signed-off-by: Yu-Hsuan Hsu
---
Changed the order of the conditional.
Remove the snd_pcm_hw_constraint_msbits function.
Use removing 24 bits format instead of fixing at 16 bits format.
sound/soc/codecs/max98090.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 7619ea31ab50..9fbb4c31bcf1 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -1909,6 +1909,24 @@ static int max98090_configure_dmic(struct max98090_priv
*max98090,
return 0;
}
+static int max98090_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_component *component = dai->component;
+ struct max98090_priv *max98090 =
snd_soc_component_get_drvdata(component);
+ unsigned int fmt = max98090->dai_fmt;
+
+ /*
+* When TDM = 0, remove 24-bit format support if it is not in right
+* justified mode.
+*/
+ if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_RIGHT_J &&
+ !max98090->tdm_slots)
+ substream->runtime->hw.formats &= ~SNDRV_PCM_FMTBIT_S24_LE;
+
+ return 0;
+}
+
static int max98090_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
@@ -2316,6 +2334,7 @@ EXPORT_SYMBOL_GPL(max98090_mic_detect);
#define MAX98090_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
static const struct snd_soc_dai_ops max98090_dai_ops = {
+ .startup = max98090_dai_startup,
.set_sysclk = max98090_dai_set_sysclk,
.set_fmt = max98090_dai_set_fmt,
.set_tdm_slot = max98090_set_tdm_slot,
--
2.22.0.410.gd8fdbe21b5-goog
[PATCH v5] ASoC: max98090: remove 24-bit format support if RJ is 0
The supported formats are S16_LE and S24_LE now. However, S24_LE is
not supported when TDM is 0 and it is not in the right justified mode.
We should remove 24-bit format in that situation to avoid triggering
error.
Signed-off-by: Yu-Hsuan Hsu
---
The datasheet said that when TDM=0 and RJ=0, S24_LE is not supported.
So I added a constraint to check TDM. Please take a look. Thanks!
sound/soc/codecs/max98090.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 7619ea31ab50..d118cf80b6b2 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -1909,6 +1909,26 @@ static int max98090_configure_dmic(struct max98090_priv
*max98090,
return 0;
}
+static int max98090_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_component *component = dai->component;
+ struct max98090_priv *max98090 =
snd_soc_component_get_drvdata(component);
+ unsigned int fmt = max98090->dai_fmt;
+
+ /*
+* When TDM = 0, remove 24-bit format support if it is not in right
+* justified mode.
+*/
+ if (!max98090->tdm_slots &&
+ (fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_RIGHT_J) {
+ substream->runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
+ snd_pcm_hw_constraint_msbits(substream->runtime, 0, 16, 16);
+ }
+
+ return 0;
+}
+
static int max98090_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
@@ -2316,6 +2336,7 @@ EXPORT_SYMBOL_GPL(max98090_mic_detect);
#define MAX98090_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
static const struct snd_soc_dai_ops max98090_dai_ops = {
+ .startup = max98090_dai_startup,
.set_sysclk = max98090_dai_set_sysclk,
.set_fmt = max98090_dai_set_fmt,
.set_tdm_slot = max98090_set_tdm_slot,
--
2.22.0.410.gd8fdbe21b5-goog
[PATCH v4] ASoC: max98090: remove 24-bit format support if RJ is 0
The supported formats are S16_LE and S24_LE now. However, by datasheet
of max98090, S24_LE is only supported when it is in the right justified
mode. We should remove 24-bit format if it is not in that mode to avoid
triggering error.
Signed-off-by: Yu-Hsuan Hsu
---
Remove Change-Id.
sound/soc/codecs/max98090.c | 16
1 file changed, 16 insertions(+)
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 7619ea31ab50..ada8c25e643d 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -1909,6 +1909,21 @@ static int max98090_configure_dmic(struct max98090_priv
*max98090,
return 0;
}
+static int max98090_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_component *component = dai->component;
+ struct max98090_priv *max98090 =
snd_soc_component_get_drvdata(component);
+ unsigned int fmt = max98090->dai_fmt;
+
+ /* Remove 24-bit format support if it is not in right justified mode. */
+ if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_RIGHT_J) {
+ substream->runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
+ snd_pcm_hw_constraint_msbits(substream->runtime, 0, 16, 16);
+ }
+ return 0;
+}
+
static int max98090_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
@@ -2316,6 +2331,7 @@ EXPORT_SYMBOL_GPL(max98090_mic_detect);
#define MAX98090_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
static const struct snd_soc_dai_ops max98090_dai_ops = {
+ .startup = max98090_dai_startup,
.set_sysclk = max98090_dai_set_sysclk,
.set_fmt = max98090_dai_set_fmt,
.set_tdm_slot = max98090_set_tdm_slot,
--
2.22.0.rc1.311.g5d7573a151-goog
[PATCH v3] ASoC: max98090: remove 24-bit format support if RJ is 0
The supported formats are S16_LE and S24_LE now. However, by datasheet
of max98090, S24_LE is only supported when it is in the right justified
mode. We should remove 24-bit format if it is not in that mode to avoid
triggering error.
Change-Id: I304882e0b67974f8fe4c2a47c61a41a04635b2df
Signed-off-by: Yu-Hsuan Hsu
---
Fix compile error.
codec->dai => dai
runtime => substream->runtime
sound/soc/codecs/max98090.c | 16
1 file changed, 16 insertions(+)
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 7619ea31ab50..ada8c25e643d 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -1909,6 +1909,21 @@ static int max98090_configure_dmic(struct max98090_priv
*max98090,
return 0;
}
+static int max98090_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_component *component = dai->component;
+ struct max98090_priv *max98090 =
snd_soc_component_get_drvdata(component);
+ unsigned int fmt = max98090->dai_fmt;
+
+ /* Remove 24-bit format support if it is not in right justified mode. */
+ if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_RIGHT_J) {
+ substream->runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
+ snd_pcm_hw_constraint_msbits(substream->runtime, 0, 16, 16);
+ }
+ return 0;
+}
+
static int max98090_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
@@ -2316,6 +2331,7 @@ EXPORT_SYMBOL_GPL(max98090_mic_detect);
#define MAX98090_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
static const struct snd_soc_dai_ops max98090_dai_ops = {
+ .startup = max98090_dai_startup,
.set_sysclk = max98090_dai_set_sysclk,
.set_fmt = max98090_dai_set_fmt,
.set_tdm_slot = max98090_set_tdm_slot,
--
2.22.0.rc1.311.g5d7573a151-goog
[PATCH v2] ASoC: max98090: remove 24-bit format support if RJ is 0
The supported formats are S16_LE and S24_LE now. However, by datasheet
of max98090, S24_LE is only supported when it is in the right justified
mode. We should remove 24-bit format if it is not in that mode to avoid
triggering error.
Signed-off-by: Yu-Hsuan Hsu
---
sound/soc/codecs/max98090.c | 16
1 file changed, 16 insertions(+)
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 7619ea31ab50..a6c2cb89767c 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -1909,6 +1909,21 @@ static int max98090_configure_dmic(struct max98090_priv
*max98090,
return 0;
}
+static int max98090_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_component *component = codec_dai->component;
+ struct max98090_priv *max98090 =
snd_soc_component_get_drvdata(component);
+ unsigned int fmt = max98090->dai_fmt;
+
+ /* Remove 24-bit format support if it is not in right justified mode. */
+ if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_RIGHT_J) {
+ runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
+ snd_pcm_hw_constraint_msbits(substream->runtime, 0, 16, 16);
+ }
+ return 0;
+}
+
static int max98090_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
@@ -2316,6 +2331,7 @@ EXPORT_SYMBOL_GPL(max98090_mic_detect);
#define MAX98090_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
static const struct snd_soc_dai_ops max98090_dai_ops = {
+ .startup = max98090_dai_startup,
.set_sysclk = max98090_dai_set_sysclk,
.set_fmt = max98090_dai_set_fmt,
.set_tdm_slot = max98090_set_tdm_slot,
--
2.22.0.rc1.311.g5d7573a151-goog
[PATCH] ASoC: max98090: remove 24-bit format support if RJ is 0
The supported formats are S16_LE and S24_LE now. However, by datasheet
of max98090, S24_LE is only supported when it is in the right justified
mode. We should remove 24-bit format if it is not in that mode to avoid
triggering error.
Signed-off-by: Yu-Hsuan Hsu
---
sound/soc/codecs/max98090.c | 16
1 file changed, 16 insertions(+)
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 7619ea31ab50..4cbfb0b95404 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -1909,6 +1909,21 @@ static int max98090_configure_dmic(struct max98090_priv
*max98090,
return 0;
}
+static int max98090_dai_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_soc_component *component = codec_dai->component;
+ struct max98090_priv *max98090 =
snd_soc_component_get_drvdata(component);
+ unsigned int fmt = max98090->dai_fmt;
+
+ /* Remove 24-bit format support if RJ is 1. */
+ if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_RIGHT_J) {
+ runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
+ snd_pcm_hw_constraint_msbits(substream->runtime, 0, 16, 16);
+ }
+ return 0;
+}
+
static int max98090_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
@@ -2316,6 +2331,7 @@ EXPORT_SYMBOL_GPL(max98090_mic_detect);
#define MAX98090_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
static const struct snd_soc_dai_ops max98090_dai_ops = {
+ .startup = max98090_dai_startup,
.set_sysclk = max98090_dai_set_sysclk,
.set_fmt = max98090_dai_set_fmt,
.set_tdm_slot = max98090_set_tdm_slot,
--
2.22.0.rc1.311.g5d7573a151-goog
[PATCH] ASoC: max98090: remove 24-bit format support
Remove 24-bit format support because it doesn't work now. We can revert
this change after it really supports.
(https://patchwork.kernel.org/patch/10783561/)
Signed-off-by: Yu-Hsuan Hsu
---
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 7619ea31ab50..b25b7efa9118 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -2313,7 +2313,7 @@ int max98090_mic_detect(struct snd_soc_component
*component,
EXPORT_SYMBOL_GPL(max98090_mic_detect);
#define MAX98090_RATES SNDRV_PCM_RATE_8000_96000
-#define MAX98090_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE)
+#define MAX98090_FORMATS SNDRV_PCM_FMTBIT_S16_LE
static const struct snd_soc_dai_ops max98090_dai_ops = {
.set_sysclk = max98090_dai_set_sysclk,
--
2.21.0.1020.gf2820cf01a-goog
[PATCH] ASoC: da7219: Update the support rate list
If we want to set rate to 64000 on da7219, it fails and returns
"snd_pcm_hw_params: Invalid argument".
We should remove 64000 from support rate list because it is not
available.
Signed-off-by: Yu-Hsuan Hsu
---
sound/soc/codecs/da7219.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 5f5fa3416af3..7497457cf3d4 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -1658,20 +1658,26 @@ static const struct snd_soc_dai_ops da7219_dai_ops = {
#define DA7219_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
+#define DA7219_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\
+ SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
+ SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
+ SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
+ SNDRV_PCM_RATE_96000)
+
static struct snd_soc_dai_driver da7219_dai = {
.name = "da7219-hifi",
.playback = {
.stream_name = "Playback",
.channels_min = 1,
.channels_max = DA7219_DAI_CH_NUM_MAX,
- .rates = SNDRV_PCM_RATE_8000_96000,
+ .rates = DA7219_RATES,
.formats = DA7219_FORMATS,
},
.capture = {
.stream_name = "Capture",
.channels_min = 1,
.channels_max = DA7219_DAI_CH_NUM_MAX,
- .rates = SNDRV_PCM_RATE_8000_96000,
+ .rates = DA7219_RATES,
.formats = DA7219_FORMATS,
},
.ops = &da7219_dai_ops,
--
2.21.0.593.g511ec345e18-goog
