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

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