* [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
@ 2020-07-08 7:11 Yu-Hsuan Hsu
2020-07-08 10:16 ` Enric Balletbo i Serra
0 siblings, 1 reply; 8+ messages in thread
From: Yu-Hsuan Hsu @ 2020-07-08 7:11 UTC (permalink / raw)
To: linux-kernel
Cc: Benson Leung, Enric Balletbo i Serra, Guenter Roeck,
Cheng-Yi Chiang, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Tzung-Bi Shih, Gwendal Grignou, Lee Jones,
Yu-Hsuan Hsu, Yicheng Li, alsa-devel
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>
---
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
2020-07-08 7:11 [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing Yu-Hsuan Hsu
@ 2020-07-08 10:16 ` Enric Balletbo i Serra
2020-07-08 13:28 ` Guenter Roeck
0 siblings, 1 reply; 8+ messages in thread
From: Enric Balletbo i Serra @ 2020-07-08 10:16 UTC (permalink / raw)
To: Yu-Hsuan Hsu, linux-kernel
Cc: Benson Leung, Guenter Roeck, Cheng-Yi Chiang, Liam Girdwood,
Mark Brown, Jaroslav Kysela, Takashi Iwai, Tzung-Bi Shih,
Gwendal Grignou, Lee Jones, Yicheng Li, alsa-devel
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 <yuhsuan@chromium.org>
> ---
> 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?
> platform_set_drvdata(pdev, priv);
>
> ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
2020-07-08 10:16 ` Enric Balletbo i Serra
@ 2020-07-08 13:28 ` Guenter Roeck
2020-07-08 16:17 ` Yu-Hsuan Hsu
0 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2020-07-08 13:28 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: Yu-Hsuan Hsu, linux-kernel, Benson Leung, Guenter Roeck,
Cheng-Yi Chiang, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Tzung-Bi Shih, Gwendal Grignou, Lee Jones,
Yicheng Li, ALSA development
On Wed, Jul 8, 2020 at 3:16 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> 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 <yuhsuan@chromium.org>
> > ---
> > 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
> > platform_set_drvdata(pdev, priv);
> >
> > ret = devm_snd_soc_register_component(dev, &i2s_rx_component_driver,
> >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
2020-07-08 13:28 ` Guenter Roeck
@ 2020-07-08 16:17 ` Yu-Hsuan Hsu
2020-07-08 17:25 ` Guenter Roeck
0 siblings, 1 reply; 8+ messages in thread
From: Yu-Hsuan Hsu @ 2020-07-08 16:17 UTC (permalink / raw)
To: Guenter Roeck
Cc: Enric Balletbo i Serra, linux-kernel, Benson Leung,
Guenter Roeck, Cheng-Yi Chiang, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, Tzung-Bi Shih, Gwendal Grignou,
Lee Jones, Yicheng Li, ALSA development
Guenter Roeck <groeck@google.com> 於 2020年7月8日 週三 下午9:28寫道：
>
> On Wed, Jul 8, 2020 at 3:16 AM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> 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 <yuhsuan@chromium.org>
> > > ---
> > > 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,
> > >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
2020-07-08 16:17 ` Yu-Hsuan Hsu
@ 2020-07-08 17:25 ` Guenter Roeck
2020-07-08 17:51 ` Yu-Hsuan Hsu
0 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2020-07-08 17:25 UTC (permalink / raw)
To: Yu-Hsuan Hsu
Cc: Enric Balletbo i Serra, linux-kernel, Benson Leung,
Guenter Roeck, Cheng-Yi Chiang, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, Tzung-Bi Shih, Gwendal Grignou,
Lee Jones, Yicheng Li, ALSA development
On Wed, Jul 8, 2020 at 9:17 AM Yu-Hsuan Hsu <yuhsuan@chromium.org> wrote:
>
> Guenter Roeck <groeck@google.com> 於 2020年7月8日 週三 下午9:28寫道：
> >
> > On Wed, Jul 8, 2020 at 3:16 AM Enric Balletbo i Serra
> > <enric.balletbo@collabora.com> 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 <yuhsuan@chromium.org>
> > > > ---
> > > > 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.
> 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,
> > > >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
2020-07-08 17:25 ` Guenter Roeck
@ 2020-07-08 17:51 ` Yu-Hsuan Hsu
2020-07-15 17:44 ` Enric Balletbo i Serra
0 siblings, 1 reply; 8+ messages in thread
From: Yu-Hsuan Hsu @ 2020-07-08 17:51 UTC (permalink / raw)
To: Guenter Roeck
Cc: Enric Balletbo i Serra, linux-kernel, Benson Leung,
Guenter Roeck, Cheng-Yi Chiang, Liam Girdwood, Mark Brown,
Jaroslav Kysela, Takashi Iwai, Tzung-Bi Shih, Gwendal Grignou,
Lee Jones, Yicheng Li, ALSA development
Guenter Roeck <groeck@google.com> 於 2020年7月9日 週四 上午1:26寫道：
>
> On Wed, Jul 8, 2020 at 9:17 AM Yu-Hsuan Hsu <yuhsuan@chromium.org> wrote:
> >
> > Guenter Roeck <groeck@google.com> 於 2020年7月8日 週三 下午9:28寫道：
> > >
> > > On Wed, Jul 8, 2020 at 3:16 AM Enric Balletbo i Serra
> > > <enric.balletbo@collabora.com> 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 <yuhsuan@chromium.org>
> > > > > ---
> > > > > 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,
> > > > >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
2020-07-08 17:51 ` Yu-Hsuan Hsu
@ 2020-07-15 17:44 ` Enric Balletbo i Serra
0 siblings, 0 replies; 8+ messages in thread
From: Enric Balletbo i Serra @ 2020-07-15 17:44 UTC (permalink / raw)
To: Yu-Hsuan Hsu, Guenter Roeck
Cc: linux-kernel, Benson Leung, Guenter Roeck, Cheng-Yi Chiang,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Tzung-Bi Shih, Gwendal Grignou, Lee Jones, Yicheng Li,
ALSA development
Hi Yu-Hsuan,
On 8/7/20 19:51, Yu-Hsuan Hsu wrote:
> Guenter Roeck <groeck@google.com> 於 2020年7月9日 週四 上午1:26寫道：
>>
>> On Wed, Jul 8, 2020 at 9:17 AM Yu-Hsuan Hsu <yuhsuan@chromium.org> wrote:
>>>
>>> Guenter Roeck <groeck@google.com> 於 2020年7月8日 週三 下午9:28寫道：
>>>>
>>>> On Wed, Jul 8, 2020 at 3:16 AM Enric Balletbo i Serra
>>>> <enric.balletbo@collabora.com> 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 <yuhsuan@chromium.org>
>>>>>> ---
>>>>>> 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);
>>>>>> +
What about notify the user that he *must* update their firmware?
dev_info(dev, "Missing reset command. Please update your EC firmware.\n")
>>>>>
>>>>> 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.
>
How can you ensure that assuming there is already devices in the field? The
patch should work for both cases, on devices that has the old firmware and on
devices that have the new firmware. You can't guarantee that all the devices
will update their firmware.
I'd suggest notify the user that should update the EC firmware and
fallback/continue when the EC_CODEC_I2S_RX_RESET is not implemented.
>>
>>> 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,
>>>>>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing
@ 2021-01-06 5:05 Yu-Hsuan Hsu
0 siblings, 0 replies; 8+ messages in thread
From: Yu-Hsuan Hsu @ 2021-01-06 5:05 UTC (permalink / raw)
To: linux-kernel
Cc: Benson Leung, Enric Balletbo i Serra, Guenter Roeck,
Cheng-Yi Chiang, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Prashant Malani, Pi-Hsun Shih, Yu-Hsuan Hsu,
Gustavo A . R . Silva, alsa-devel
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>
---
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-06 5:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-08 7:11 [PATCH] ASoC: cros_ec_codec: Reset I2S RX when probing Yu-Hsuan Hsu
2020-07-08 10:16 ` Enric Balletbo i Serra
2020-07-08 13:28 ` Guenter Roeck
2020-07-08 16:17 ` Yu-Hsuan Hsu
2020-07-08 17:25 ` Guenter Roeck
2020-07-08 17:51 ` Yu-Hsuan Hsu
2020-07-15 17:44 ` Enric Balletbo i Serra
2021-01-06 5:05 Yu-Hsuan Hsu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®