Re: [PATCH] drm/bridge: sii902x: Add audio context to suspend/resume routine

[Devarsh Thakkar <devarsht@ti.com>]

Hi Sen,

Thanks for the patch.

On 12/02/26 02:40, Sen Wang wrote:
> The sii902x driver has existing suspend/resume handlers that save and
> restore video-related register context (TPI mode and interrupts), but
> these handlers were not saving/restoring audio configuration registers.
> This caused HDMI audio to stop working after system suspend/resume cycles.
> 
> Therefore add audio-related register context to the existing
> suspend/resume handlers when audio context needs to be saved/restored. As
> well as mclk for the sake of power saving, in the case of sii902x being
> the frame producer.
> 
> The audio context is only saved/restored when audio.active is true,
> avoiding unnecessary register access when audio is not in use.
> 
> Tested on TI SK-AM62P-LP board with HDMI audio playback across multiple
> suspend/resume cycles.
> 
> Signed-off-by: Sen Wang <sen@ti.com>
> ---
>   drivers/gpu/drm/bridge/sii902x.c | 91 +++++++++++++++++++++++++++++++-
>   1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 134657041799..fd38a6ae86b2 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -192,6 +192,13 @@ struct sii902x {
>   		struct platform_device *pdev;
>   		struct clk *mclk;
>   		u32 i2s_fifo_sequence[4];
> +		bool active;
> +		/* Audio register context for save/resume */
> +		unsigned int ctx_i2s_input_config;
> +		unsigned int ctx_audio_config_byte2;
> +		unsigned int ctx_audio_config_byte3;
> +		u8 ctx_i2s_stream_header[SII902X_TPI_I2S_STRM_HDR_SIZE];
> +		u8 ctx_audio_infoframe[SII902X_TPI_MISC_INFOFRAME_SIZE];
>   	} audio;
>   };
>   
> @@ -764,6 +771,8 @@ static int sii902x_audio_hw_params(struct device *dev, void *data,
>   	if (ret)
>   		goto out;
>   
> +	sii902x->audio.active = true;
> +
>   	dev_dbg(dev, "%s: hdmi audio enabled\n", __func__);
>   out:
>   	mutex_unlock(&sii902x->mutex);
> @@ -786,6 +795,8 @@ static void sii902x_audio_shutdown(struct device *dev, void *data)
>   	regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
>   		     SII902X_TPI_AUDIO_INTERFACE_DISABLE);
>   
> +	sii902x->audio.active = false;
> +
>   	mutex_unlock(&sii902x->mutex);
>   
>   	clk_disable_unprepare(sii902x->audio.mclk);
> @@ -1081,7 +1092,7 @@ static int __maybe_unused sii902x_resume(struct device *dev)
>   {
>   	struct sii902x *sii902x = dev_get_drvdata(dev);
>   	unsigned int tpi_reg, status;
> -	int ret;
> +	int ret, i;
>   
>   	ret = regmap_read(sii902x->regmap, SII902X_REG_TPI_RQB, &tpi_reg);
>   	if (ret)
> @@ -1109,7 +1120,62 @@ static int __maybe_unused sii902x_resume(struct device *dev)
>   	regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
>   	regmap_write(sii902x->regmap, SII902X_INT_STATUS, status);
>   
> +	/*
> +	 * Restore audio context if audio was active before suspend,
> +	 * in the matching order of sii902x_audio_hw_params()
> +	 * initialization
> +	 */
> +	if (sii902x->audio.active) {
> +		/* Re-enable mclk */
> +		ret = clk_prepare_enable(sii902x->audio.mclk);
> +		if (ret) {
> +			dev_err(dev, "Failed to re-enable mclk: %d\n", ret);
> +			return ret;
> +		}
> +
> +		ret = regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
> +				   sii902x->audio.ctx_audio_config_byte2);
> +		if (ret)
> +			goto err_audio_resume;
> +
> +		ret = regmap_write(sii902x->regmap, SII902X_TPI_I2S_INPUT_CONFIG_REG,
> +				   sii902x->audio.ctx_i2s_input_config);
> +		if (ret)
> +			goto err_audio_resume;
> +
> +		for (i = 0; i < ARRAY_SIZE(sii902x->audio.i2s_fifo_sequence) &&
> +		     sii902x->audio.i2s_fifo_sequence[i]; i++) {
> +			ret = regmap_write(sii902x->regmap,
> +					   SII902X_TPI_I2S_ENABLE_MAPPING_REG,
> +					   sii902x->audio.i2s_fifo_sequence[i]);
> +			if (ret)
> +				goto err_audio_resume;
> +		}
> +
> +		ret = regmap_write(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE3_REG,
> +				   sii902x->audio.ctx_audio_config_byte3);
> +		if (ret)
> +			goto err_audio_resume;
> +
> +		ret = regmap_bulk_write(sii902x->regmap, SII902X_TPI_I2S_STRM_HDR_BASE,
> +					sii902x->audio.ctx_i2s_stream_header,
> +					SII902X_TPI_I2S_STRM_HDR_SIZE);
> +		if (ret)
> +			goto err_audio_resume;
> +
> +		ret = regmap_bulk_write(sii902x->regmap, SII902X_TPI_MISC_INFOFRAME_BASE,
> +					sii902x->audio.ctx_audio_infoframe,
> +					SII902X_TPI_MISC_INFOFRAME_SIZE);
> +		if (ret)
> +			goto err_audio_resume;
> +	}
> +
>   	return 0;
> +
> +err_audio_resume:
> +	clk_disable_unprepare(sii902x->audio.mclk);
> +	dev_err(dev, "Failed to restore audio registers: %d\n", ret);
> +	return ret;
>   }
>   
>   static int __maybe_unused sii902x_suspend(struct device *dev)
> @@ -1122,6 +1188,29 @@ static int __maybe_unused sii902x_suspend(struct device *dev)
>   	regmap_read(sii902x->regmap, SII902X_INT_ENABLE,
>   		    &sii902x->ctx_interrupt);
>   
> +	/*
> +	 * Save audio context if audio is active, and
> +	 * in the matching order of sii902x_audio_hw_params()
> +	 * initialization
> +	 */
> +	if (sii902x->audio.active) {
> +		regmap_read(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
> +			    &sii902x->audio.ctx_audio_config_byte2);
> +		regmap_read(sii902x->regmap, SII902X_TPI_I2S_INPUT_CONFIG_REG,
> +			    &sii902x->audio.ctx_i2s_input_config);
> +		regmap_read(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE3_REG,
> +			    &sii902x->audio.ctx_audio_config_byte3);
> +		regmap_bulk_read(sii902x->regmap, SII902X_TPI_I2S_STRM_HDR_BASE,
> +				 sii902x->audio.ctx_i2s_stream_header,
> +				 SII902X_TPI_I2S_STRM_HDR_SIZE);
> +		regmap_bulk_read(sii902x->regmap, SII902X_TPI_MISC_INFOFRAME_BASE,
> +				 sii902x->audio.ctx_audio_infoframe,
> +				 SII902X_TPI_MISC_INFOFRAME_SIZE);
> +


In resume path you were checking failure for each regmap_write I guess 
similar approach is needed here if going with register reads, so in case 
one or more regmap_read/regmap_bulk_read fails you can return error with 
dev_err message.

But I think even better approach would be to cache the corresponding reg 
values to corresponding struct members during hw_params callback itself 
given all the parameters being used sii902x->audio.ctx do not change 
between hw_params callback and suspend callback as all these params are 
stream related? , If so that would avoid costly i2c operations for 
reading register values which can be skipped in this function.

> +		/* Disable mclk during suspend */
> +		clk_disable_unprepare(sii902x->audio.mclk);
> +	}
> +


audio.active should be cleared here or do we need to call audio_shutdown 
explicitly?

Regards
Devarsh