Re: [PATCH v2] drm/bridge: ite-it6263: Add suspend/resume support

[Liu Ying]

On Mon, Apr 20, 2026 at 06:15:46AM +0000, Biju Das wrote:
> Hi Liu Ying,

Hi Biju,

> 
>> -----Original Message-----
>> From: Liu Ying <victor.liu@xxxxxxx>
>> Sent: 20 April 2026 03:26
>> Subject: Re: [PATCH v2] drm/bridge: ite-it6263: Add suspend/resume support
>>
>> On Fri, Apr 17, 2026 at 10:49:35AM +0000, Biju Das wrote:
>>> Hi Liu Ying,
>>>
>>> Thanks for the feedback.
>>>
>>>
>>>> -----Original Message-----
>>>> From: Liu Ying <victor.liu@xxxxxxx>
>>>> Sent: 17 April 2026 07:05
>>>> Subject: Re: [PATCH v2] drm/bridge: ite-it6263: Add suspend/resume
>>>> support
>>>>
>>>> Hi Biju,
>>>>
>>>> On Thu, Apr 16, 2026 at 09:29:25AM +0100, Biju wrote:
>>>>> [You don't often get email from biju.das.au@xxxxxxxxx. Learn why
>>>>> this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>>
>>>>> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>>>>>
>>>>> On the RZ/G3L SMARC EVK using PSCI, suspend to RAM powers down the
>>>>> ITE
>>>>> IT6263 chip. The display controller driver's system PM callbacks
>>>>> invoke drm_mode_config_helper_{suspend,resume}, which in turn call
>>>>> the bridge's atomic_{disable,enable} callbacks can handle
>>>>> suspend/resume for the bridge without dedicated PM ops.
>>>>>
>>>>> Introduce it6263_bridge_init() and it6263_bridge_uninit() helpers to
>>>>> consolidate power sequencing, hardware reset, I2C address setup, and
>>>>> LVDS/HDMI configuration. These replace the open-coded init sequence
>>>>> in
>>>>> probe() and are hooked into atomic_enable/atomic_disable
>>>>> respectively, guarded by a powered flag to avoid redundant re-initialisation.
>>>>>
>>>>> Switch from devm_regulator_bulk_get_enable() to
>>>>> devm_regulator_bulk_get() so that regulators can be explicitly
>>>>> enabled and disabled across power cycles. Move reset_gpio and
>>>>> regulator state into the it6263 struct so they are accessible beyond probe time.
>>>>>
>>>>> Add a remove() callback to cleanly power down the bridge on driver
>>>>> unbind via it6263_bridge_uninit().
>>>>>
>>>>> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>>>>> ---
>>>>> v1->v2:
>>>>>  * Dropped system PM callbacks instead using bridge's
>>>>>    atomic_{disable,enable} callbacks to handle suspend/resume.
>>>>> ---
>>>>>  drivers/gpu/drm/bridge/ite-it6263.c | 88
>>>>> ++++++++++++++++++++++++-----
>>>>>  1 file changed, 73 insertions(+), 15 deletions(-)
>>
>> [...]
>>
>>>>> +static int it6263_bridge_init(struct it6263 *it) {
>>>>> +       int ret;
>>>>> +
>>>>> +       ret = regulator_bulk_enable(it->num_supplies, it->supplies);
>>>>> +       if (ret) {
>>>>> +               dev_err(it->dev, "failed to enable power supplies\n");
>>>>> +               return ret;
>>>>> +       }
>>>>> +
>>>>> +       it6263_hw_reset(it->reset_gpio);
>>>>> +
>>>>> +       ret = it6263_lvds_set_i2c_addr(it);
>>>>> +       if (ret) {
>>>>> +               dev_err(it->dev, "failed to set I2C addr\n");
>>>>> +               regulator_bulk_disable(it->num_supplies,
>>>>> + it->supplies);
>>>>
>>>> I know that you call it6263_bridge_init() in probe, probably because
>>>> you want to enable the regulators for hotplug detect after probe(it6263_detect() reads register
>> HDMI_REG_SYS_STATUS to do the detection).
>>>> However, an idea[1] is to wrap the register read operation with
>>>> regulator_bulk_enable() and
>>>> regulator_bulk_disable() in it6263_detect() so that you may drop
>>>> it6263_bridge_init() from probe.  With that,  it6263_bridge_init() is
>>>> now only called from atomic_enable, which means that the
>>>> initialization code can be open-coded and the initialization is
>>>> supposed to be successful(due to the "atomic" nature) hence no need to do the regulator disablement
>> bailout(error message in dmesg is sufficient).
>>>
>>> it6263_detect() still works with regulator_disable(), see the logs below.
>>
>> I guess that it works for you on RZ/G3L SMARC EVK because regulators are already enabled by PSCI before
>> this driver's probe. 
> 
> PSCI does not enable it. The supply to the rails provided by PMIC regulator during system resume
> and it is always on.

Then the PSCI term in commit message doesn't provide any useful information,
so could be dropped.

Since it's always on, can you keep using devm_regulator_bulk_get_enable()
in probe and just move it6263_hw_reset(), it6263_lvds_set_i2c_addr(),
it6263_lvds_config() and it6263_hdmi_config() from probe to atomic_enable?

> 
>> But there could be platforms which use dedicated regulators(like discrete PMICs)
>> for IT6263, which means the regulators are not yet enabled before probe.
> 
> Do you know any platform that does not work the detection after regulator disable()?

No.  But if regulators are not enabled, detection doesn't work for sure.

> 
> Currently we don't have any platforms to test this. If any platforms that has controlled
> regulator we can update the code based on testing.

If we end up with calling regulator_bulk_enable() in atomic_enable and
calling regulator_bulk_disable in atomic_disable, I'd prefer to enable/disable
regulators in detect and edid_read instead of doing nothing, because
detect and edid_read would not work for sure without power on those platforms
with controlled regulators.  If there is any bug, we can fix that.

[...]

>>>>>  static void it6263_bridge_atomic_disable(struct drm_bridge *bridge,
>>>>>                                          struct drm_atomic_state
>>>>> *state)  { @@ -587,6 +626,8 @@ static void
>>>>> it6263_bridge_atomic_disable(struct drm_bridge *bridge,
>>>>>         regmap_write(it->hdmi_regmap, HDMI_REG_PKT_GENERAL_CTRL, 0);
>>>>>         regmap_write(it->hdmi_regmap, HDMI_REG_AFE_DRV_CTRL,
>>>>>                      AFE_DRV_RST | AFE_DRV_PWD);
>>>>> +
>>>>> +       it6263_bridge_uninit(it);
>>>>
>>>> Well, this could effectively disable the regulators and hotplug detection
>>>> won't work then.   So, again, the above idea[1] helps.
>>>
>>> Is it not working on your setup? It works for me.
>>
>> My setup uses always-on regulators, so detect works for me as well even if regulators are not
>> explicitly enabled/disabled in detect callback.  But, as I mentioned above, we need to enable/disable
>> regulators in detect callback (also in edid_read callback) after atomic_disable is done for those
>> platforms which use dedicated regulators.
> 
> On atomic_disable(), we are disabling the regulator. So on, regulator-gpio
> platforms, the detection() won't work after that. In that case, we need to move
> suspend/resume calls from atomic_{enable,disable} to PM callbacks.
> 
> Do you agree?

If you mean system PM callbacks, are you sure that this driver's resume
callback is executed prior to a display controller driver's resume callback
(essentially drm_mode_config_helper_resume() would be called to enable IT6263's
video output if it's the status when suspended) to enable regulators first?
I don't think the order is fixed across all platforms, because the display
controller device can sit before or after the IT6263 device on the dpm_list.
So, I don't want to implement system PM for this driver, at least for now.

> 
> Cheers,
> Biju
> 

-- 
Regards,
Liu Ying