Re: [PATCH] drm/bridge: sii902x: Add Power Management hooks with audio context

[Devarsh Thakkar <devarsht@ti.com>]

Hi Sen,

On 01/04/26 07:07, Sen Wang wrote:

Sorry but this is not very clear, is this a V2 to 
https://lore.kernel.org/all/e6497541-f3e7-4533-a188-9d422cb34d74@ti.com/ ?

In that case, the subject should mention it as PATCH v2 along with 
changelog as documented in kernel patch guidelines : 
https://docs.kernel.org/process/submitting-patches.html

> Add suspend and resume hooks. On suspend, save TPI mode, interrupt
> enable, and audio register context (when active). On resume, detect
> power loss by comparing the saved TPI value; if changed, reset the
> device and restore TPI mode and interrupts. Restore audio registers
> and re-enable mclk if audio was active before suspend.
> 
> Audio register values are read back during suspend rather than cached
> during hw_params, as the sii902x requires a specific register write
> sequence during initialization that must be preserved on resume.
> 
> Based on initial PM hooks implementation by:
>    Aradhya Bhatia <a-bhatia1@ti.com>
>    Jayesh Choudhary <j-choudhary@ti.com>
> 
> Signed-off-by: Sen Wang <sen@ti.com>
> ---
> Tested on TI SK-AM62P-LP board with HDMI audio playback across multiple
> suspend/resume cycles.

Could you please also share the test logs ? Did you verify that both 
audio/video context resume back seamlessly afer resuming from system 
suspend ?

I guess this still does not support runtime suspend/resume ?

And assuming this is v2, here you should mention changelog and v1 link:
V1: https://lore.kernel.org/all/e6497541-f3e7-4533-a188-9d422cb34d74@ti.com/
> 
>   drivers/gpu/drm/bridge/sii902x.c | 165 +++++++++++++++++++++++++++++++
>   1 file changed, 165 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 12497f5ce4ff..8da8ca22ac99 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -179,6 +179,8 @@ struct sii902x {
>   	struct gpio_desc *reset_gpio;
>   	struct i2c_mux_core *i2cmux;
>   	u32 bus_width;
> +	unsigned int ctx_tpi;
> +	unsigned int ctx_interrupt;
>   
>   	/*
>   	 * Mutex protects audio and video functions from interfering
> @@ -189,6 +191,13 @@ struct sii902x {
>   		struct platform_device *pdev;
>   		struct clk *mclk;
>   		u32 i2s_fifo_sequence[4];
> +		bool active;
> +		/* Audio register context for suspend/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;
>   };
>   
> @@ -755,6 +764,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);
> @@ -777,6 +788,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);
> @@ -1069,6 +1082,157 @@ static const struct drm_bridge_timings default_sii902x_timings = {
>   		 | DRM_BUS_FLAG_DE_HIGH,
>   };
>   
> +static int sii902x_resume(struct device *dev)
> +{
> +	struct sii902x *sii902x = dev_get_drvdata(dev);
> +	unsigned int tpi_reg, status;
> +	int ret, i;
> +
> +	ret = regmap_read(sii902x->regmap, SII902X_REG_TPI_RQB, &tpi_reg);
> +	if (ret)
> +		return ret;
> +
> +	if (tpi_reg != sii902x->ctx_tpi) {
> +		/*
> +		 * TPI register context has changed. SII902X power supply
> +		 * device has been turned off and on.
> +		 */
> +		sii902x_reset(sii902x);
> +
> +		/* Configure the device to enter TPI mode. */
> +		ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
> +		if (ret)
> +			return ret;
> +
> +		/* Re-enable the interrupts */
> +		regmap_write(sii902x->regmap, SII902X_INT_ENABLE,
> +			     sii902x->ctx_interrupt);
> +	}
> +
> +	/* Clear all pending interrupts */
> +	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) {

audio.active should be protected with mutex lock as done elsewhere in 
the driver?

> +		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;
> +	}
> +

Do we need to restore the indirect registers status too with the 
constants that were used ?

/* Decode Level 0 Packets */ 

regmap_write(regmap, SII902X_IND_SET_PAGE, 0x02);  /* 0xBC */ 

regmap_write(regmap, SII902X_IND_OFFSET,  0x24);  /* 0xBD */ 

regmap_write(regmap, SII902X_IND_VALUE,   0x02);  /* 0xBE */ 

```

> +	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 sii902x_suspend(struct device *dev)
> +{
> +	struct sii902x *sii902x = dev_get_drvdata(dev);
> +	int ret;
> +

Have you reviewed Table 3.8 of the datasheet ?

I think we should probably be utilizing different operating modes during 
suspend/resume cycles.

For e.g. with system suspend go to D3 cold state which is absolute 
minimum power.

For runtime suspend thought, have to be little careful as the D state 
should be chosen such that it does not have a too high resume latency.
If D3 doesn't have too high then probably use the same else use D2.

> +	ret = regmap_read(sii902x->regmap, SII902X_REG_TPI_RQB,
> +			  &sii902x->ctx_tpi);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(sii902x->regmap, SII902X_INT_ENABLE,
> +			  &sii902x->ctx_interrupt);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Save audio context if audio is active, in the matching order
> +	 * of sii902x_audio_hw_params() initialization.
> +	 */
> +	if (sii902x->audio.active) {

Here too, I think mutex protection required while accessing this flag.

> +		ret = regmap_read(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE2_REG,
> +				  &sii902x->audio.ctx_audio_config_byte2);
> +		if (ret)
> +			goto err_audio_suspend;
> +
> +		ret = regmap_read(sii902x->regmap, SII902X_TPI_I2S_INPUT_CONFIG_REG,
> +				  &sii902x->audio.ctx_i2s_input_config);
> +		if (ret)
> +			goto err_audio_suspend;
> +
> +		ret = regmap_read(sii902x->regmap, SII902X_TPI_AUDIO_CONFIG_BYTE3_REG,
> +				  &sii902x->audio.ctx_audio_config_byte3);
> +		if (ret)
> +			goto err_audio_suspend;
> +
> +		ret = regmap_bulk_read(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_suspend;
> +
> +		ret = regmap_bulk_read(sii902x->regmap, SII902X_TPI_MISC_INFOFRAME_BASE,
> +				       sii902x->audio.ctx_audio_infoframe,
> +				       SII902X_TPI_MISC_INFOFRAME_SIZE);
> +		if (ret)
> +			goto err_audio_suspend;
> +

In the previous revision, my comment was to skip  register reads for 
restoring the context and instead populate the context in hw_params itself :

something as below :
@@ -710,18 +717,36 @@ static int sii902x_audio_hw_params(struct device 
*dev, void *data,
         ret = regmap_write(sii902x->regmap, 

                            SII902X_TPI_AUDIO_CONFIG_BYTE2_REG, 

                            config_byte2_reg); 

-       if (ret < 0) 

+       if (ret < 0) 

                 goto out; 

+       sii902x->audio.ctx_audio_config_byte2 = config_byte2_reg; 

  

         ret = regmap_write(sii902x->regmap, 
SII902X_TPI_I2S_INPUT_CONFIG_REG,
                            i2s_config_reg); 

         if (ret) 

                 goto out; 

+       sii902x->audio.ctx_i2s_input_config = i2s_config_reg; 

  

         for (i = 0; i < ARRAY_SIZE(sii902x->audio.i2s_fifo_sequence) && 

                     sii902x->audio.i2s_fifo_sequence[i]; i++) 

                 regmap_write(sii902x->regmap, 

                              SII902X_TPI_I2S_ENABLE_MAPPING_REG,

and likewise and then you don't have to do reg reads in resume back

> +		/*
> +		 * audio.active is kept true so that resume restores the audio
> +		 * context. sii902x_audio_shutdown() clears it when the stream
> +		 * is explicitly closed.
> +		 */
> +		clk_disable_unprepare(sii902x->audio.mclk);
> +	}
> +
> +	return 0;
> +
> +err_audio_suspend:
> +	dev_err(dev, "Failed to save audio context: %d\n", ret);
> +	return ret;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(sii902x_pm_ops, sii902x_suspend, sii902x_resume);
> +

Can we add runtime suspend/resume hooks too ?

>   static int sii902x_init(struct sii902x *sii902x)
>   {
>   	struct device *dev = &sii902x->i2c->dev;
> @@ -1247,6 +1411,7 @@ static struct i2c_driver sii902x_driver = {
>   	.remove = sii902x_remove,
>   	.driver = {
>   		.name = "sii902x",
> +		.pm = pm_sleep_ptr(&sii902x_pm_ops),
>   		.of_match_table = sii902x_dt_ids,
>   	},
>   	.id_table = sii902x_i2c_ids,

Regards
Devarsh