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

2015-06-26 Thread Jani Nikula
On Thu, 25 Jun 2015, Shobhit Kumar  wrote:
> On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä
>  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 
>>> Cc: Linus Walleij 
>>> Cc: Alexandre Courbot 
>>> Cc: Thierry Reding 
>>> Signed-off-by: Shobhit Kumar 
>>> ---
>>>  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(_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 
>>>  #include 
>>> +#include 
>>>  #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 = >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 = >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 
>>> 

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

2015-06-25 Thread Shobhit Kumar
On Thu, Jun 25, 2015 at 6:17 PM, Ville Syrjälä
 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ä
>>  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 
>> >> Cc: Linus Walleij 
>> >> Cc: Alexandre Courbot 
>> >> Cc: Thierry Reding 
>> >> Signed-off-by: Shobhit Kumar 
>> >> ---
>> >>  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(_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


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

2015-06-25 Thread Shobhit Kumar
On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä
 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 
>> Cc: Linus Walleij 
>> Cc: Alexandre Courbot 
>> Cc: Thierry Reding 
>> Signed-off-by: Shobhit Kumar 
>> ---
>>  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(_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 
>>  #include 
>> +#include 
>>  #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 = >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 = >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 

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

2015-06-25 Thread Ville Syrjälä
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ä
>  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 
> >> Cc: Linus Walleij 
> >> Cc: Alexandre Courbot 
> >> Cc: Thierry Reding 
> >> Signed-off-by: Shobhit Kumar 
> >> ---
> >>  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(_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.

-- 
Ville Syrjälä
Intel OTC


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

2015-06-25 Thread Ville Syrjälä
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 
> Cc: Linus Walleij 
> Cc: Alexandre Courbot 
> Cc: Thierry Reding 
> Signed-off-by: Shobhit Kumar 
> ---
>  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(_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 
>  #include 
> +#include 
>  #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 = >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 = >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 = >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 = 

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

2015-06-22 Thread Shobhit Kumar
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 
Cc: Linus Walleij 
Cc: Alexandre Courbot 
Cc: Thierry Reding 
Signed-off-by: Shobhit Kumar 
---
 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(_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 
 #include 
+#include 
 #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 = >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 = >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 = >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 = >panel;
+
+   pwm_enable(panel->backlight.pwm);
+   intel_panel_actually_set_backlight(connector, panel->backlight.level);
+}
+
 void intel_panel_enable_backlight(struct intel_connector *connector)