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

[Biju Das]

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(-)
> 
> "suspend/resume" in subject makes people think that this patch probably adds runtime PM or system PM
> support.  To avoid this, can you change the subject to something like:
> "drm/bridge: ite-it6263: Support power cycle in runtime"
> ?
> 
> >
> > diff --git a/drivers/gpu/drm/bridge/ite-it6263.c
> > b/drivers/gpu/drm/bridge/ite-it6263.c
> > index 4f3ebb7af4d4..1954bb11f7f4 100644
> > --- a/drivers/gpu/drm/bridge/ite-it6263.c
> > +++ b/drivers/gpu/drm/bridge/ite-it6263.c
> > @@ -200,9 +200,13 @@ struct it6263 {
> >         struct regmap *lvds_regmap;
> >         struct drm_bridge bridge;
> >         struct drm_bridge *next_bridge;
> > +       struct gpio_desc *reset_gpio;
> > +       struct regulator_bulk_data *supplies;
> 
> I would move it6263_supplies[] on top of struct it6263 definition and use 'struct regulator_bulk_data
> supplies[ARRAY_SIZE(it6263_supplies)];' here, so that you may drop devm_kcalloc() for the supplies
> array in probe.

OK. Good point.

> 
> > +       unsigned int num_supplies;
> 
> The above new supplies array has a known size, so this can be dropped and you may get the number of
> supplies via ARRAY_SIZE(it->supplies).

Agreed.

> 
> >         int lvds_data_mapping;
> >         bool lvds_dual_link;
> >         bool lvds_link12_swap;
> > +       bool powered;
> >  };
> >
> >  static inline struct it6263 *bridge_to_it6263(struct drm_bridge
> > *bridge) @@ -578,6 +582,41 @@ static int it6263_read_edid(void *data, u8 *buf, unsigned int block,
> size_t len)
> >         return 0;
> >  }
> >
> > +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.


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


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

> 
> >  }
> >
> >  static void it6263_bridge_atomic_enable(struct drm_bridge *bridge, @@
> > -603,6 +644,9 @@ static void it6263_bridge_atomic_enable(struct drm_bridge *bridge,
> >         bool pclk_high;
> >         int i, ret;
> >
> > +       if (!it->powered)
> > +               it6263_bridge_init(it);
> > +
> >         connector = drm_atomic_get_new_connector_for_encoder(state,
> >                                                              bridge->encoder);
> >         crtc = drm_atomic_get_new_connector_state(state,
> > connector)->crtc; @@ -840,7 +884,6 @@ static const struct
> > drm_bridge_funcs it6263_bridge_funcs = {  static int
> > it6263_probe(struct i2c_client *client)  {
> >         struct device *dev = &client->dev;
> > -       struct gpio_desc *reset_gpio;
> >         struct it6263 *it;
> >         int ret;
> >
> > @@ -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.

Cheers,
Biju