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

[Sen Wang <sen@ti.com>]

On 4/1/26 09:42, Thakkar, Devarsh wrote:
> 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
> 
Hi Devarsh, Thanks for your review.

This new patch encompasses more features than the previous patch to
warrant it being a separate patch. But nonetheless my apologies for
not stating the indirection in my commit message.

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

Okay I'll attach it to the commit message for v2 patch.

> I guess this still does not support runtime suspend/resume ?
> 
No it doesn't, I don't know if full suspend/resume is necessarily the 
correct solution for runtime as well, besides there's also ways to do it 
in mode_set/atomic callbacks. Not well-versed in DRM framework to draw 
the conclusion here.

Runtime PM should ideally decouple sound and video to maximize the power 
saving. Let me take a deeper look and see if this chip has features that 
benefits from runtime PM.

> 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?
> 
Sounds good, I'm assuming PM resume/suspend are atomic but nonetheless 
need to safeguard it from existing ops.
>> +		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 */
> 
> ```
> 
I didn't see any difference when I restore these registers but let me 
double check just to be sure.
>> +	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.
> 

Understood
>> +		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
> 
Okay, let me analyze this.
>> +		/*
>> +		 * 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 ?
> 

I can add what we have for runtime, although I need to investigate this 
is actually makes a difference.

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

Thanks for your review Devarsh, let me investigate and follow-up with a 
v2 patch.

Best regards,
Sen Wang