AW: (EXT) Re: [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support

[Alexander Stein]

Hello Laurent,

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 at ravnborg.org>
> > Signed-off-by: Alexander Stein <alexander.stein at 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.

Best regards,
Alexander