| Message ID | 20210107085942.2891525-2-yuhsuan@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET | expand |
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-1-yuhsuan@chromium.org/ 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-1-yuhsuan@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, > >
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,
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(+)