[v2,7/7] drm/i915: Backlight control using CRC PMIC based PWM driver
diff mbox

Message ID 1434970465-12687-8-git-send-email-shobhit.kumar@intel.com
State New
Headers show

Commit Message

Kumar, Shobhit June 22, 2015, 10:54 a.m. UTC
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(-)

Comments

Ville Syrjälä June 25, 2015, 8:48 a.m. UTC | #1
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
Shobhit Kumar June 25, 2015, 12:08 p.m. UTC | #2
On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> 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.
>

You are right, its unused, but I thought passing right value still
made sense. Otherwise it makes it look like I am setting up back-light
for invalid pipe, when the real meaning is something like
DONTCARE_PIPE

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

Yeah, missed removing it.

>> +
>>  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?
>

No, VLV designs can actually also use LPSS PWM as well as DISPLAY_PWM.
Infact we had a case where even for eDP, customer design used LPSS. So
this flag in VBT for mipi_config block "pwm_blc" indicate the same for
now. But today this is only PMIC Vs LPSS. There is another update
pending where we have full flexibility in VBT to define PMIC or LPSS
or DISPLAY_PWM to handle the eDP case that I mentioned above.

> And what about CHT?

Its the same as I described above. But then beyond CHT, there is
effort to unify this all as only one SoC PWM.

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

Thanks a lot for testing it out.

Regards
Shobhit

>
>> +                     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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä June 25, 2015, 12:47 p.m. UTC | #3
On Thu, Jun 25, 2015 at 05:38:50PM +0530, Shobhit Kumar wrote:
> On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > 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.
> >
> 
> You are right, its unused, but I thought passing right value still
> made sense. Otherwise it makes it look like I am setting up back-light
> for invalid pipe, when the real meaning is something like
> DONTCARE_PIPE

Well it's not really about the pipe. It's about which set of BLC
registers we're supoosed to use when using the BLC built into the
display engine. And that's only done so that we take over the
hardware state correctly. So INVALID_PIPE is just fine in this case
since the backlight control has nothing to do with the pipe.
Shobhit Kumar June 25, 2015, 1:24 p.m. UTC | #4
On Thu, Jun 25, 2015 at 6:17 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Jun 25, 2015 at 05:38:50PM +0530, Shobhit Kumar wrote:
>> On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > 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.
>> >
>>
>> You are right, its unused, but I thought passing right value still
>> made sense. Otherwise it makes it look like I am setting up back-light
>> for invalid pipe, when the real meaning is something like
>> DONTCARE_PIPE
>
> Well it's not really about the pipe. It's about which set of BLC
> registers we're supoosed to use when using the BLC built into the
> display engine. And that's only done so that we take over the
> hardware state correctly. So INVALID_PIPE is just fine in this case
> since the backlight control has nothing to do with the pipe.
>

Ok, will update.

Regards
Shobhit
Jani Nikula June 26, 2015, 8:31 a.m. UTC | #5
On Thu, 25 Jun 2015, Shobhit Kumar <kumar@shobhit.info> wrote:
> On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> 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.
>>
>
> You are right, its unused, but I thought passing right value still
> made sense. Otherwise it makes it look like I am setting up back-light
> for invalid pipe, when the real meaning is something like
> DONTCARE_PIPE
>
>>>
>>>       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.
>>
>
> Yeah, missed removing it.
>
>>> +
>>>  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?
>>
>
> No, VLV designs can actually also use LPSS PWM as well as DISPLAY_PWM.
> Infact we had a case where even for eDP, customer design used LPSS. So
> this flag in VBT for mipi_config block "pwm_blc" indicate the same for
> now. But today this is only PMIC Vs LPSS. There is another update
> pending where we have full flexibility in VBT to define PMIC or LPSS
> or DISPLAY_PWM to handle the eDP case that I mentioned above.

Side note, we may need to move the hooks from dev_priv->display to
connector->panel in the future.

BR,
Jani.


>
>> And what about CHT?
>
> Its the same as I described above. But then beyond CHT, there is
> effort to unify this all as only one SoC PWM.
>
>>
>> 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.
>
> Thanks a lot for testing it out.
>
> Regards
> Shobhit
>
>>
>>> +                     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
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox

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);
 
 	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
+
 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;
+
+	/* 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) {
+			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;