Hi Alexander,

On Wed, Oct 13, 2021 at 08:59:22AM +0000, Alexander Stein wrote:

On Tue, Oct 12, 2021 at 10:43 +0200, Laurent Pinchart wrote:

...

On Tue, Oct 12, 2021 at 08:48:43AM +0200, Alexander Stein wrote:

...

VCC needs to be enabled before releasing the enable GPIO.

Reviewed-by: Sam Ravnborg sam@ravnborg.org Signed-off-by: Alexander Stein alexander.stein@ew.tq-group.com

---

drivers/gpu/drm/bridge/ti-sn65dsi83.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 9072342566f3..a6b1fd71dfee 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -33,6 +33,7 @@ #include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h>

#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> @@ -143,6 +144,7 @@ struct sn65dsi83 { struct mipi_dsi_device *dsi; struct drm_bridge *panel_bridge; struct gpio_desc *enable_gpio;

• struct regulator *vcc; int dsi_lanes; bool lvds_dual_link; bool lvds_dual_link_even_odd_swap;

@@ -647,6 +649,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx,

enum sn65dsi83_model model)

...

ctx->panel_bridge = panel_bridge;

• ctx->vcc = devm_regulator_get(dev, "vcc");
• if (IS_ERR(ctx->vcc))

• return dev_err_probe(dev, PTR_ERR(ctx->vcc),
  

• 		     "Failed to get supply 'vcc': %pe\n",
  

• 		     ctx->vcc);
  

• return 0;

}

@@ -691,7 +699,11 @@ static int sn65dsi83_probe(struct i2c_client *client, ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);

• return 0;

• ret = regulator_enable(ctx->vcc);
• if (ret)

• dev_err(dev, "Failed to enable vcc: %i\n", ret);
  

I think this should move to sn65dsi83_atomic_pre_enable() (and similarly for regulator_disable()) as keeping the regulator enabled at all times will cost power.

I get your idea. The thing is that unless 1V8 is provided the bridge is not even accessible on I2C. So any access to sn65dsi83.regmap without the vcc regulator enabled will fail. AFAICS this is not an issue right now, as regmap is only used in sn65dsi83_atomic_enable(), sn65dsi83_atomic_disable() and sn65dsi83_atomic_pre_enable(), so your sugestion would work, but I'm hestitating a bit. The driver then has to ensure all regmap uses are done only when vcc is enabled.

Correct, and that's the usual pattern, drivers need to call pm_runtime_get_sync() before accessing registers. For all you know, even if the power to the bridge is on, the I2C controller it is connected to could be suspended.