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

[Devarsh Thakkar <devarsht@ti.com>]

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


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