Re: [PATCH] drm/bridge: sii902x: Add Power Management hooks with audio context
From: Devarsh Thakkar
Date: Thu Apr 02 2026 - 03:14:04 EST
Hi Sen,
Thanks for the update.
On 02/04/26 05:03, Sen Wang wrote:
On 4/1/26 09:42, Thakkar, Devarsh wrote:
Hi Sen,Hi Devarsh, Thanks for your review.
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@xxxxxx/ ?
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
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.
I don't think this qualifies to make it an independent patch altogether, as long as goal of the patch is same as the initial one posted, this should be labelled as a V2 with linkage to V1 in changelog. And the follow up patch should be labelled V3.
The changelog should capture the changes under ---:
V1->V2 : What changed
V2->V3 : What changed
along with links for V1 and V2.
This gives the reviewer necessary context on architecture and reasoning behind new changes/approaches even if it means the new patch contains some extra feature which was not present in previous patch.
Regards
Devarsh
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@xxxxxx>
Jayesh Choudhary <j-choudhary@xxxxxx>
Signed-off-by: Sen Wang <sen@xxxxxx>
---
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:Sounds good, I'm assuming PM resume/suspend are atomic but nonetheless need to safeguard it from existing ops.
V1: https://lore.kernel.org/all/e6497541-f3e7-4533- a188-9d422cb34d74@xxxxxx/
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?
I didn't see any difference when I restore these registers but let me double check just to be sure.+ 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.
Understood
Okay, let me analyze this.+ 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 ?
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