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: 21 April 2026 04:20
> Subject: Re: [PATCH v2] drm/bridge: ite-it6263: Add suspend/resume support
> 
> 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.

OK will drop 'using PSCI'.

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

Agreed.

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

Plan is to do regulator_enable() in probe. I will send next version based on the above discussion.

> 
> [...]
> 
> >>>>>  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?

Maybe register PM Callback LATE_SYSTEM_SLEEP_PM_OPS or NOIRQ_SYSTEM_SLEEP_PM_OPS for this driver
when we have a use case related to controlled regulator. This also depends on i2c/pinctrl driver PM callback
as well(they need to be NOIRQ_SYSTEM_SLEEP_PM_OPS).

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

OK.

Cheers,
Biju