Re: [PATCH 4/4] drm/panel: jh057n0090: Add regulator support

[Guido Günther]

Hi Sam,
On Tue, Jun 25, 2019 at 11:24:19PM +0200, Sam Ravnborg wrote:
> On Tue, Jun 25, 2019 at 07:05:19PM +0200, Guido Günther wrote:
> > Allow to specify regulators for vcc and iovcc. According to the data
> > sheet the panel wants vcc (2.8V) and iovcc (1.8V) and there's no startup
> > dependency between the two.
> s/jh057n0090/jh057n00900
> 
> > 
> > Signed-off-by: Guido Günther <agx@xxxxxxxxxxx>
> > ---
> >  .../drm/panel/panel-rocktech-jh057n00900.c    | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> > index b8a069055fbc..f8f6f087b9bc 100644
> > --- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> > +++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/media-bus-format.h>
> >  #include <linux/module.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <video/display_timing.h>
> >  #include <video/mipi_display.h>
> >  
> > @@ -47,6 +48,8 @@ struct jh057n {
> >  	struct drm_panel panel;
> >  	struct gpio_desc *reset_gpio;
> >  	struct backlight_device *backlight;
> > +	struct regulator *vcc;
> > +	struct regulator *iovcc;
> >  	bool prepared;
> >  
> >  	struct dentry *debugfs;
> > @@ -160,6 +163,8 @@ static int jh057n_unprepare(struct drm_panel *panel)
> >  		return 0;
> >  
> >  	mipi_dsi_dcs_set_display_off(dsi);
> > +	regulator_disable(ctx->iovcc);
> > +	regulator_disable(ctx->vcc);
> >  	ctx->prepared = false;
> >  
> >  	return 0;
> > @@ -174,6 +179,13 @@ static int jh057n_prepare(struct drm_panel *panel)
> >  		return 0;
> >  
> >  	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Resetting the panel\n");
> > +	ret = regulator_enable(ctx->vcc);
> > +	if (ret < 0)
> > +		return ret;
> > +	ret = regulator_enable(ctx->iovcc);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> >  	usleep_range(20, 40);
> >  	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> > @@ -301,6 +313,13 @@ static int jh057n_probe(struct mipi_dsi_device *dsi)
> >  	if (IS_ERR(ctx->backlight))
> >  		return PTR_ERR(ctx->backlight);
> >  
> > +	ctx->vcc = devm_regulator_get(dev, "vcc");
> > +	if (IS_ERR(ctx->vcc))
> > +		return PTR_ERR(ctx->vcc);
> > +	ctx->iovcc = devm_regulator_get(dev, "iovcc");
> > +	if (IS_ERR(ctx->iovcc))
> > +		return PTR_ERR(ctx->iovcc);
> > +
> Consider to write an error message.
> The regulators are now mandatory, but they be missing in some device
> trees. So it would be good to help them to understand why it fails.

I've fixed this and your other comments in v2.
> 
> With this considered:
> Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>

Thanks! I've not added this yet since I made two more changes:

- also print an error when regulator_enable() fails
- disable vcc if enabling iovcc fails afterwards

Hope this looks sane now.

Cheers,
 -- Guido

> 
> 	Sam
>