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

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

Best regards,
Alexander

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart