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

[Sen Wang <sen@ti.com>]

On 4/2/26 02:07, Thakkar, Devarsh wrote:
> 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,
>>>
>>> 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.
>>
> 
> 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
> 
> 
Understood Devarsh, thank you for the detailed explanation. Deeply 
appreciated.

I also dug more into the datasheet with your comments in mind and wanted 
to gather some feedback on the proposal before sending a proper V3 patch.

Here's my proposal, TL;DR:

- Runtime suspend to use D2 (Sleep) for a quick bringup
- System suspend to use D3 Hot for a complete power-off/on scheme

For the full summary:

(per Datasheet's powerstate, Table 3.8)
D0 	- Full power
D2 	- Quiet Power Down (i2c accessible)
D3 Hot  - Complete Power Down (via HPD or RSEN wkup)
D3 Cold - Complete Power Down (HPD)

Runtime suspend on sii902x should NOT use the same flow as System suspend

- Runtime suspend to use D2 for a quick bringup
- System suspend to use D3 Hot for a complete power-off/on scheme
	+ Hot vs Cold: even though Cold use marginally less power, it can only 
be entered and wkup via HPD event (Hotplug detect), which is not a 
software-controlled event.


In such case, only system suspend needs to cache TPI and audio context, 
reset & initalise the hardware. Whereas runtime suspend should just be 
in a D2 state where all reg values are preserved. Thus providing faster 
response time for runtime resume, especially when we don't have 
autosuspend (yet), and avoids latency for spontaneous events in the DRM 
framework.

Best,
Sen Wang

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