Re: [RFC v5 9/9] drm/i915: Backlight control using CRC PMIC based PWM driver

From: Thierry Reding <thierry.reding@gmail.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Alexandre Courbot <gnurou@gmail.com>,
	Samuel Ortiz <sameo@linux.intel.com>,
	Jani Nikula <jani.nikula@intel.com>,
	Shobhit Kumar <shobhit.kumar@intel.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [RFC v5 9/9] drm/i915: Backlight control using CRC PMIC based PWM driver
Date: Tue, 24 Mar 2015 09:59:38 +0100	[thread overview]
Message-ID: <20150324085937.GC17183@ulmo.nvidia.com> (raw)
In-Reply-To: <20150313143043.GF17419@intel.com>

[-- Attachment #1.1: Type: text/plain, Size: 2111 bytes --]

On Fri, Mar 13, 2015 at 04:30:43PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 12, 2015 at 10:01:33PM +0530, Shobhit Kumar wrote:
> > CC: Samuel Ortiz <sameo@linux.intel.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dsi.c | 25 +++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dsi.h |  3 +++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index 219421c..511446f 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -32,6 +32,7 @@
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <linux/slab.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/pwm.h>
> >  #include "i915_drv.h"
> >  #include "intel_drv.h"
> >  #include "intel_dsi.h"
> > @@ -402,6 +403,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
> >  
> >  		intel_dsi_port_enable(encoder);
> >  	}
> > +
> > +	/* Enable the backlight with default PWM as programmed by BIOS */
> > +	pwm_enable(intel_dsi->pwm);
> > +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 100);
> 
> I suppose we migth want to wrap these in 'if (intel_dsi->pwm)' checks,
> or does the pwm subsystem take care of NULL checks for us?

The PWM core checks for valid PWM devices and will return an error if
you pass in an invalid device. What this is completely missing is any
kind of error checking. But perhaps you don't care about failure here
for this particular platform? It would be useful to debug issues with
black screens and such I can imagine.

Also, though it's admittedly somewhat underdocumented, the typical
sequence of calls should be pwm_config() followed by pwm_enable(). The
reason is that some devices don't support being configured while
enabled. But you may not care about this here since you're always
dealing with a fixed device anyway.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx