Re: [Intel-gfx] [v2 7/7] drm/i915: Backlight control using CRC PMIC based PWM driver

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jun 22, 2015 at 04:24:25PM +0530, Shobhit Kumar wrote:
> Use the CRC PWM device in intel_panel.c and add new MIPI backlight
> specififc callbacks
> 
> v2: Modify to use pwm_config callback
> v3: Addressed Jani's comments
>     - Renamed all function as pwm_* instead of vlv_*
>     - Call intel_panel_actually_set_backlight in enable function
>     - Return -ENODEV in case pwm_get fails
>     - in case pwm_config error return error cdoe from pwm_config
>     - Cleanup pwm in intel_panel_destroy_backlight
> 
> 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_drv.h   |  4 ++
>  drivers/gpu/drm/i915/intel_dsi.c   |  6 +++
>  drivers/gpu/drm/i915/intel_panel.c | 95 ++++++++++++++++++++++++++++++++++++--
>  3 files changed, 100 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2afb31a..561c17f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -182,6 +182,10 @@ struct intel_panel {
>  		bool enabled;
>  		bool combination_mode;	/* gen 2/4 only */
>  		bool active_low_pwm;
> +
> +		/* PWM chip */
> +		struct pwm_device *pwm;
> +
>  		struct backlight_device *device;
>  	} backlight;
>  
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index c4db74a..be8722c 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -402,6 +402,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  
>  		intel_dsi_port_enable(encoder);
>  	}
> +
> +	intel_panel_enable_backlight(intel_dsi->attached_connector);
>  }
>  
>  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> @@ -466,6 +468,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	intel_panel_disable_backlight(intel_dsi->attached_connector);
> +
>  	if (is_vid_mode(intel_dsi)) {
>  		/* Send Shutdown command to the panel in LP mode */
>  		for_each_dsi_port(port, intel_dsi->ports)
> @@ -1132,6 +1136,8 @@ void intel_dsi_init(struct drm_device *dev)
>  	}
>  
>  	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
> +	intel_panel_setup_backlight(connector,
> +		(intel_encoder->crtc_mask = (1 << PIPE_A)) ? PIPE_A : PIPE_B);
                                          ^

Whoops. But since the PWM backlight doesn't need the initial pipe for
anything you can actually just pass INVALID_PIPE here.
                
>  
>  	return;
>  
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 7d83527..2aa30db 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -32,8 +32,12 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/moduleparam.h>
> +#include <linux/pwm.h>
>  #include "intel_drv.h"
>  
> +#define CRC_PMIC_PWM_PERIOD_NS	21333
> +#define CRC_PMIC_PWM_STEPS	255

This define appears to be unused.

> +
>  void
>  intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
>  		       struct drm_display_mode *adjusted_mode)
> @@ -544,6 +548,15 @@ static u32 bxt_get_backlight(struct intel_connector *connector)
>  	return I915_READ(BXT_BLC_PWM_DUTY1);
>  }
>  
> +static u32 pwm_get_backlight(struct intel_connector *connector)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +	int duty_ns;
> +
> +	duty_ns = pwm_get_duty_cycle(panel->backlight.pwm);
> +	return DIV_ROUND_UP(duty_ns * 100, CRC_PMIC_PWM_PERIOD_NS);
> +}
> +
>  static u32 intel_panel_get_backlight(struct intel_connector *connector)
>  {
>  	struct drm_device *dev = connector->base.dev;
> @@ -632,6 +645,14 @@ static void bxt_set_backlight(struct intel_connector *connector, u32 level)
>  	I915_WRITE(BXT_BLC_PWM_DUTY1, level);
>  }
>  
> +static void pwm_set_backlight(struct intel_connector *connector, u32 level)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +	int duty_ns = DIV_ROUND_UP(level * CRC_PMIC_PWM_PERIOD_NS, 100);
> +
> +	pwm_config(panel->backlight.pwm, duty_ns, CRC_PMIC_PWM_PERIOD_NS);
> +}
> +
>  static void
>  intel_panel_actually_set_backlight(struct intel_connector *connector, u32 level)
>  {
> @@ -769,6 +790,16 @@ static void bxt_disable_backlight(struct intel_connector *connector)
>  	I915_WRITE(BXT_BLC_PWM_CTL1, tmp & ~BXT_BLC_PWM_ENABLE);
>  }
>  
> +static void pwm_disable_backlight(struct intel_connector *connector)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +
> +	/* Disable the backlight */
> +	pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS);
> +	usleep_range(2000, 3000);
> +	pwm_disable(panel->backlight.pwm);
> +}
> +
>  void intel_panel_disable_backlight(struct intel_connector *connector)
>  {
>  	struct drm_device *dev = connector->base.dev;
> @@ -1002,6 +1033,14 @@ static void bxt_enable_backlight(struct intel_connector *connector)
>  	I915_WRITE(BXT_BLC_PWM_CTL1, pwm_ctl | BXT_BLC_PWM_ENABLE);
>  }
>  
> +static void pwm_enable_backlight(struct intel_connector *connector)
> +{
> +	struct intel_panel *panel = &connector->panel;
> +
> +	pwm_enable(panel->backlight.pwm);
> +	intel_panel_actually_set_backlight(connector, panel->backlight.level);
> +}
> +
>  void intel_panel_enable_backlight(struct intel_connector *connector)
>  {
>  	struct drm_device *dev = connector->base.dev;
> @@ -1378,6 +1417,40 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  	return 0;
>  }
>  
> +static int pwm_setup_backlight(struct intel_connector *connector,
> +			       enum pipe pipe)
> +{
> +	struct drm_device *dev = connector->base.dev;
> +	struct intel_panel *panel = &connector->panel;
> +	int retval = -1;

No need to initialize it.

> +
> +	/* Get the PWM chip for backlight control */
> +	panel->backlight.pwm = pwm_get(dev->dev, "pwm_backlight");
> +	if (IS_ERR(panel->backlight.pwm)) {
> +		DRM_ERROR("Failed to own the pwm chip\n");
> +		panel->backlight.pwm = NULL;
> +		return -ENODEV;
> +	}
> +
> +	retval = pwm_config(panel->backlight.pwm, CRC_PMIC_PWM_PERIOD_NS,
> +			    CRC_PMIC_PWM_PERIOD_NS);
> +	if (retval < 0) {
> +		DRM_ERROR("Failed to configure the pwm chip\n");
> +		pwm_put(panel->backlight.pwm);
> +		panel->backlight.pwm = NULL;
> +		return retval;
> +	}
> +
> +	panel->backlight.min = 0; /* 0% */
> +	panel->backlight.max = 100; /* 100% */
> +	panel->backlight.level = DIV_ROUND_UP(
> +				 pwm_get_duty_cycle(panel->backlight.pwm) * 100,
> +				 CRC_PMIC_PWM_PERIOD_NS);
> +	panel->backlight.enabled = panel->backlight.level != 0;
> +
> +	return 0;
> +}
> +
>  int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
>  {
>  	struct drm_device *dev = connector->dev;
> @@ -1421,6 +1494,10 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
>  	struct intel_connector *intel_connector = to_intel_connector(connector);
>  	struct intel_panel *panel = &intel_connector->panel;
>  
> +	/* dispose of the pwm */
> +	if (panel->backlight.pwm)
> +		pwm_put(panel->backlight.pwm);
> +
>  	panel->backlight.present = false;
>  }
>  
> @@ -1448,11 +1525,19 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev)
>  		dev_priv->display.set_backlight = pch_set_backlight;
>  		dev_priv->display.get_backlight = pch_get_backlight;
>  	} else if (IS_VALLEYVIEW(dev)) {
> -		dev_priv->display.setup_backlight = vlv_setup_backlight;
> -		dev_priv->display.enable_backlight = vlv_enable_backlight;
> -		dev_priv->display.disable_backlight = vlv_disable_backlight;
> -		dev_priv->display.set_backlight = vlv_set_backlight;
> -		dev_priv->display.get_backlight = vlv_get_backlight;
> +		if (dev_priv->vbt.has_mipi) {

Do all VLV DSI desins use the PMIC for backlight, or is there
something more specific in VBT we could look at?

And what about CHT?

Othwerwise things seem reasonable, so with the the
intel_panel_setup_backlight() pipe thing fixed this patch is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I also gave the entire series a go on my FFRD8 and it appears to work
just fine, so you can also add
Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
to all the patches if you want.

> +			dev_priv->display.setup_backlight = pwm_setup_backlight;
> +			dev_priv->display.enable_backlight = pwm_enable_backlight;
> +			dev_priv->display.disable_backlight = pwm_disable_backlight;
> +			dev_priv->display.set_backlight = pwm_set_backlight;
> +			dev_priv->display.get_backlight = pwm_get_backlight;
> +		} else {
> +			dev_priv->display.setup_backlight = vlv_setup_backlight;
> +			dev_priv->display.enable_backlight = vlv_enable_backlight;
> +			dev_priv->display.disable_backlight = vlv_disable_backlight;
> +			dev_priv->display.set_backlight = vlv_set_backlight;
> +			dev_priv->display.get_backlight = vlv_get_backlight;
> +		}
>  	} else if (IS_GEN4(dev)) {
>  		dev_priv->display.setup_backlight = i965_setup_backlight;
>  		dev_priv->display.enable_backlight = i965_enable_backlight;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel