[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 yuhsuan@chromium.org --- 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, };
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 yuhsuan@chromium.org --- 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,
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 yuhsuan@chromium.org
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... v4: https://patchwork.kernel.org/project/alsa-devel/list/?series=410441
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.
Thanks, Enric
platform_set_drvdata(pdev, priv);
ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
Enric Balletbo i Serra enric.balletbo@collabora.com 於 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 yuhsuan@chromium.org
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... 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,
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.
Mark Brown broonie@kernel.org 於 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.
On Fri, Jan 08, 2021 at 12:57:51PM +0800, Yu-Hsuan Hsu wrote:
Mark Brown broonie@kernel.org 於 2021年1月7日 週四 下午9:55寫道：
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.
TBH that seems like a big enough change to split out from this and done as a separate series, I'd be perfectly happy to apply your original change. I guess part of doing that sync up should ideally be to refactor things so that it can be done mechanically in future.
On Mon, Jan 11, 2021 at 4:42 PM Mark Brown broonie@kernel.org wrote:
On Fri, Jan 08, 2021 at 12:57:51PM +0800, Yu-Hsuan Hsu wrote:
Mark Brown broonie@kernel.org 於 2021年1月7日 週四 下午9:55寫道：
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.
TBH that seems like a big enough change to split out from this and done as a separate series, I'd be perfectly happy to apply your original change. I guess part of doing that sync up should ideally be to refactor things so that it can be done mechanically in future.
Being able to do it mechanically was the idea for introducing the script. Unfortunately it doesn't help to have such a script if people don't use it.
Guenter
On Mon, Jan 11, 2021 at 05:52:31PM -0800, Guenter Roeck wrote:
On Mon, Jan 11, 2021 at 4:42 PM Mark Brown broonie@kernel.org wrote:
TBH that seems like a big enough change to split out from this and done as a separate series, I'd be perfectly happy to apply your original change. I guess part of doing that sync up should ideally be to refactor things so that it can be done mechanically in future.
Being able to do it mechanically was the idea for introducing the script. Unfortunately it doesn't help to have such a script if people don't use it.
Looking at the issues Yu-Hsuan mentions and briefly at the code I guess there's some updates needed with the script for namespacing around at least pd_ to avoid the need for hand massaging things, that'll put people off using the script.
Hi,
On 12/1/21 15:10, Mark Brown wrote:
On Mon, Jan 11, 2021 at 05:52:31PM -0800, Guenter Roeck wrote:
On Mon, Jan 11, 2021 at 4:42 PM Mark Brown broonie@kernel.org wrote:
TBH that seems like a big enough change to split out from this and done as a separate series, I'd be perfectly happy to apply your original change. I guess part of doing that sync up should ideally be to refactor things so that it can be done mechanically in future.
Being able to do it mechanically was the idea for introducing the script. Unfortunately it doesn't help to have such a script if people don't use it.
Looking at the issues Yu-Hsuan mentions and briefly at the code I guess there's some updates needed with the script for namespacing around at least pd_ to avoid the need for hand massaging things, that'll put people off using the script.
I even didn't know about that script :-(, or forget about it, assuming the files are async again, I think I'm fine on only introduce that line of code instead of a full sync (and lets think how we can do better work on this and solve in the chrome-platform tree). I have some comments on patch 2, though.
Cheers, Enric
participants (4)
-
Enric Balletbo i Serra -
Guenter Roeck -
Mark Brown -
Yu-Hsuan Hsu