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

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

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

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@bp.renesas.com>
---
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(-)

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;
+	unsigned int num_supplies;
 	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);
+		return ret;
+	}
+
+	it6263_lvds_config(it);
+	it6263_hdmi_config(it);
+
+	it->powered = true;
+
+	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);
 }
 
 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++)
+		it->supplies[i].supply = it6263_supplies[i];
+
+	ret = devm_regulator_bulk_get(dev, it->num_supplies, it->supplies);
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to get power supplies\n");
 
@@ -872,12 +923,6 @@ static int it6263_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	it6263_hw_reset(reset_gpio);
-
-	ret = it6263_lvds_set_i2c_addr(it);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to set I2C addr\n");
-
 	it->lvds_i2c = devm_i2c_new_dummy_device(dev, client->adapter,
 						 LVDS_INPUT_CTRL_I2C_ADDR);
 	if (IS_ERR(it->lvds_i2c))
@@ -890,8 +935,9 @@ static int it6263_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(it->lvds_regmap),
 				     "failed to init I2C regmap for LVDS\n");
 
-	it6263_lvds_config(it);
-	it6263_hdmi_config(it);
+	ret = it6263_bridge_init(it);
+	if (ret)
+		return ret;
 
 	i2c_set_clientdata(client, it);
 
@@ -903,7 +949,18 @@ static int it6263_probe(struct i2c_client *client)
 	it->bridge.vendor = "ITE";
 	it->bridge.product = "IT6263";
 
-	return devm_drm_bridge_add(dev, &it->bridge);
+	ret = devm_drm_bridge_add(dev, &it->bridge);
+	if (ret)
+		it6263_bridge_uninit(it);
+
+	return ret;
+}
+
+static void it6263_remove(struct i2c_client *i2c)
+{
+	struct it6263 *it = i2c_get_clientdata(i2c);
+
+	it6263_bridge_uninit(it);
 }
 
 static const struct of_device_id it6263_of_match[] = {
@@ -920,6 +977,7 @@ MODULE_DEVICE_TABLE(i2c, it6263_i2c_ids);
 
 static struct i2c_driver it6263_driver = {
 	.probe = it6263_probe,
+	.remove = it6263_remove,
 	.driver = {
 		.name = "it6263",
 		.of_match_table = it6263_of_match,
-- 
2.43.0

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

[Liu Ying]

Hi Biju,

On Thu, Apr 16, 2026 at 09:29:25AM +0100, Biju wrote:
> [You don't often get email from biju.das.au@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> From: Biju Das <biju.das.jz@bp.renesas.com>
> 
> 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@bp.renesas.com>
> ---
> 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.

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

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

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

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

> +               it->supplies[i].supply = it6263_supplies[i];
> +
> +       ret = devm_regulator_bulk_get(dev, it->num_supplies, it->supplies);
>         if (ret)
>                 return dev_err_probe(dev, ret, "failed to get power supplies\n");
> 
> @@ -872,12 +923,6 @@ static int it6263_probe(struct i2c_client *client)
>         if (ret)
>                 return ret;
> 
> -       it6263_hw_reset(reset_gpio);
> -
> -       ret = it6263_lvds_set_i2c_addr(it);
> -       if (ret)
> -               return dev_err_probe(dev, ret, "failed to set I2C addr\n");
> -
>         it->lvds_i2c = devm_i2c_new_dummy_device(dev, client->adapter,
>                                                  LVDS_INPUT_CTRL_I2C_ADDR);
>         if (IS_ERR(it->lvds_i2c))
> @@ -890,8 +935,9 @@ static int it6263_probe(struct i2c_client *client)
>                 return dev_err_probe(dev, PTR_ERR(it->lvds_regmap),
>                                      "failed to init I2C regmap for LVDS\n");
> 
> -       it6263_lvds_config(it);
> -       it6263_hdmi_config(it);
> +       ret = it6263_bridge_init(it);
> +       if (ret)
> +               return ret;
> 
>         i2c_set_clientdata(client, it);
> 
> @@ -903,7 +949,18 @@ static int it6263_probe(struct i2c_client *client)
>         it->bridge.vendor = "ITE";
>         it->bridge.product = "IT6263";
> 
> -       return devm_drm_bridge_add(dev, &it->bridge);
> +       ret = devm_drm_bridge_add(dev, &it->bridge);
> +       if (ret)
> +               it6263_bridge_uninit(it);
> +
> +       return ret;
> +}
> +
> +static void it6263_remove(struct i2c_client *i2c)
> +{
> +       struct it6263 *it = i2c_get_clientdata(i2c);
> +
> +       it6263_bridge_uninit(it);
>  }
> 
>  static const struct of_device_id it6263_of_match[] = {
> @@ -920,6 +977,7 @@ MODULE_DEVICE_TABLE(i2c, it6263_i2c_ids);
> 
>  static struct i2c_driver it6263_driver = {
>         .probe = it6263_probe,
> +       .remove = it6263_remove,
>         .driver = {
>                 .name = "it6263",
>                 .of_match_table = it6263_of_match,
> --
> 2.43.0
> 

-- 
Regards,
Liu Ying

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@nxp.com>
> 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@gmail.com. Learn why this
> > is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > From: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > 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@bp.renesas.com>
> > ---
> > 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

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@nxp.com>
>> 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@gmail.com. Learn why this
>>> is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> From: Biju Das <biju.das.jz@bp.renesas.com>
>>>
>>> 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@bp.renesas.com>
>>> ---
>>> 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

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

[Biju Das]

Hi Liu Ying,

> -----Original Message-----
> From: Liu Ying <victor.liu@nxp.com>
> 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@nxp.com>
> >> 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@gmail.com. Learn why
> >>> this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >>>
> >>> From: Biju Das <biju.das.jz@bp.renesas.com>
> >>>
> >>> 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@bp.renesas.com>
> >>> ---
> >>> 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

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@nxp.com>
>> 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@nxp.com>
>>>> 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@gmail.com. Learn why
>>>>> this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>>>
>>>>> From: Biju Das <biju.das.jz@bp.renesas.com>
>>>>>
>>>>> 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@bp.renesas.com>
>>>>> ---
>>>>> 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

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

[Biju Das]

Hi Liu Ying,

> -----Original Message-----
> From: Liu Ying <victor.liu@nxp.com>
> 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@nxp.com>
> >> 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@nxp.com>
> >>>> 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@gmail.com. Learn why
> >>>>> this is important at https://aka.ms/LearnAboutSenderIdentification
> >>>>> ]
> >>>>>
> >>>>> From: Biju Das <biju.das.jz@bp.renesas.com>
> >>>>>
> >>>>> 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@bp.renesas.com>
> >>>>> ---
> >>>>> 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