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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 26 Jun 2019 12:40:27 +0200
From:   Guido Günther <guido.gunther@...i.sm>
To:     Sam Ravnborg <sam@...nborg.org>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Purism Kernel Team <kernel@...i.sm>
Subject: Re: [PATCH 4/4] drm/panel: jh057n0090: Add regulator support

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@...xcpu.org>
> > ---
> >  .../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@...nborg.org>

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
>