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

[Liu Ying]

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.  But there could be
platforms which use dedicated regulators(like discrete PMICs) for IT6263,
which means the regulators are not yet enabled before probe.

> 
> 
>>
>>> +               return ret;
>>> +       }
>>> +
>>> +       it6263_lvds_config(it);
>>> +       it6263_hdmi_config(it);
>>> +
>>> +       it->powered = true;
>>
>> If you drop it6263_bridge_init() from probe, I think 'powered' flag can be dropped too.
>>
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int it6263_bridge_uninit(struct it6263 *it) {
>>> +       regulator_bulk_disable(it->num_supplies, it->supplies);
>>> +       it->powered = false;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>  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.

> 
> 
> root@smarc-rzg3l:~# [   33.512618] ####it6263_detect####
> [   44.008621] ####it6263_detect####
> [   54.504623] ####it6263_detect####
> [   65.000602] ####it6263_detect####
> [   65.227743] ####it6263_detect####
> [   65.233322] ####it6263_bridge_atomic_disable####
> [   75.240637] ####it6263_detect####
> [   85.480628] ####it6263_detect####
> [   95.720662] ####it6263_detect####
> [  105.960640] ####it6263_detect####
> [  116.200647] ####it6263_detect####
> [  126.440635] ####it6263_detect####
> [  127.048981] ####it6263_detect####
> [  127.517962] ####it6263_bridge_atomic_enable####

[...]

>>> @@ -858,13 +901,21 @@ static int it6263_probe(struct i2c_client *client)
>>>                 return dev_err_probe(dev, PTR_ERR(it->hdmi_regmap),
>>>                                      "failed to init I2C regmap for
>>> HDMI\n");
>>>
>>> -       reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>>> -       if (IS_ERR(reset_gpio))
>>> -               return dev_err_probe(dev, PTR_ERR(reset_gpio),
>>> +       it->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>>> +       if (IS_ERR(it->reset_gpio))
>>> +               return dev_err_probe(dev, PTR_ERR(it->reset_gpio),
>>>                                      "failed to get reset gpio\n");
>>>
>>> -       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it6263_supplies),
>>> -                                            it6263_supplies);
>>> +       it->num_supplies = ARRAY_SIZE(it6263_supplies);
>>> +       it->supplies = devm_kcalloc(dev, it->num_supplies,
>>> +                                   sizeof(*it->supplies), GFP_KERNEL);
>>> +       if (!it->supplies)
>>> +               return -ENOMEM;
>>> +
>>> +       for (unsigned int i = 0; i < it->num_supplies; i++)
>>
>> Nit: I would define i together with the other local variables at the beginning of this function.
> 
> "i" is used here only. For me it is better than putting at the top.
> 
> I got feedback from other subsystem maintainer to use unsigned here
> as the scope is within for loop.

Ack.

> 
> Cheers,
> Biju

-- 
Regards,
Liu Ying