Re: [PATCH 3/3] ASoC: es9356-sdca: Modify the Initialization Configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASoC: es9356: missing regcache_cache_only() restore in es9356_sdca_io_init()
--

commit b446d528859126e747930f027bd832991ca056fd
Author: Zhang Yi <zhangyi@everest-semi.com>

ASoC: es9356-sdca: Modify the Initialization Configuration

This patch updates es9356_register_init() to read the chip version from
ES9356_VERSION and apply version-specific register initialization settings,
propagating read errors to es9356_sdca_io_init().

> diff --git a/sound/soc/codecs/es9356.c b/sound/soc/codecs/es9356.c
> index a22e92d1e5ccd..e0df501007ad8 100644
> --- a/sound/soc/codecs/es9356.c
> +++ b/sound/soc/codecs/es9356.c
> @@ -749,58 +749,97 @@ static const struct regmap_config es9356_sdca_regmap = {
>  	.use_single_write = true,
>  };
>  
> -static void es9356_register_init(struct es9356_sdw_priv *es9356)
> +static int es9356_register_init(struct es9356_sdw_priv *es9356)
>  {
> +	unsigned int version;
> +	int ret;
> +
> +	ret = regmap_read(es9356->regmap, ES9356_VERSION, &version);
> +	if (ret < 0) {
> +		pr_err_ratelimited("IO error in %s, ret %d\n", __func__, ret);
> +		return ret;
> +	}

[ ... ]

> @@ -821,11 +860,14 @@ static void es9356_register_init(struct es9356_sdw_priv *es9356)
>  		SDW_SDCA_CTL(FUNC_NUM_UAJ, ES9356_SDCA_ENT_FU36, ES9356_SDCA_CTL_FU_VOLUME, CH_L), ES9356_DEFAULT_VOLUME);
>  	regmap_write(es9356->regmap,
>  		SDW_SDCA_CTL(FUNC_NUM_UAJ, ES9356_SDCA_ENT_FU36, ES9356_SDCA_CTL_FU_VOLUME, CH_R), ES9356_DEFAULT_VOLUME);
> +
> +	return 0;
>  }
>  
>  static int es9356_sdca_io_init(struct device *dev, struct sdw_slave *slave)
>  {
>  	struct es9356_sdw_priv *es9356 = dev_get_drvdata(&slave->dev);
> +	int ret;
>  
>  	if (es9356->hw_init)
>  		return 0;
> @@ -840,7 +882,9 @@ static int es9356_sdca_io_init(struct device *dev, struct sdw_slave *slave)
>  		/* update count of parent 'active' children */
>  		pm_runtime_set_active(&slave->dev);
>  
> -		es9356_register_init(es9356);
> +		ret = es9356_register_init(es9356);
> +		if (ret < 0)
> +			return -EINVAL;
>  	}
>  	pm_runtime_get_noresume(&slave->dev);
>  

[Severity: Medium]
In sound/soc/codecs/es9356.c, es9356_sdca_io_init() disables cache-only mode
before calling es9356_register_init():

sound/soc/codecs/es9356.c:es9356_sdca_io_init() {
	...
	regcache_cache_only(es9356->regmap, false);

	if (es9356->first_hw_init) {
		regcache_cache_bypass(es9356->regmap, true);
	} else {
		/* update count of parent 'active' children */
		pm_runtime_set_active(&slave->dev);

		ret = es9356_register_init(es9356);
		if (ret < 0)
			return -EINVAL;
	}
	...
}

If regmap_read(es9356->regmap, ES9356_VERSION, &version) fails in
es9356_register_init() during es9356_sdw_update_status() ->
es9356_sdca_io_init(), this early return leaves es9356->regmap with
cache-only mode disabled while es9356->hw_init and es9356->first_hw_init
remain false, allowing subsequent mixer control accesses to issue SoundWire
bus transactions to an uninitialized device.

Should this error path restore regcache_cache_only(es9356->regmap, true) and
return ret instead of masking the error code with -EINVAL?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021075136.16155-1-zhangyi@everest-semi.com?part=3