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

[Shobhit Kumar <kumar@shobhit.info>]

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