[PATCH 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET

[PATCH 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET

[Yu-Hsuan Hsu]

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,
 };
 
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 2/2] ASoC: cros_ec_codec: Reset I2S RX when probing

[Yu-Hsuan Hsu]

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,
-- 
2.29.2.729.g45daf8777d-goog

Re: [PATCH 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 325 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET

[Yu-Hsuan Hsu]

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.

Re: [PATCH 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] ASoC: cros_ec_codec: Reset I2S RX when probing

[Enric Balletbo i Serra]

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,
> 

Re: [PATCH 1/2] cros_ec_commands: Add EC_CODEC_I2S_RX_RESET

[Enric Balletbo i Serra]

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

Re: [PATCH 2/2] ASoC: cros_ec_codec: Reset I2S RX when probing

[Yu-Hsuan Hsu]

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,
> >