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

[Biju Das]

Hi Liu Ying,

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

> 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()?

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.

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

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?

Cheers,
Biju