Message-ID: <874mlu7twq.fsf@intel.com>
Date:	Fri, 26 Jun 2015 11:31:17 +0300
From:	Jani Nikula <jani.nikula@...el.com>
To:	Shobhit Kumar <kumar@...bhit.info>,
	Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:	Shobhit Kumar <shobhit.kumar@...el.com>,
	Alexandre Courbot <gnurou@...il.com>,
	Paul Bolle <pebolle@...cali.nl>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	linux-pwm <linux-pwm@...r.kernel.org>,
	Povilas Staniulis <wdmonster@...il.com>,
	intel-gfx <intel-gfx@...ts.freedesktop.org>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	linux-gpio <linux-gpio@...r.kernel.org>,
	Brain WrecK <bloften80@...il.com>,
	Thierry Reding <thierry.reding@...il.com>,
	David Airlie <airlied@...ux.ie>,
	Daniel Vetter <daniel.vetter@...el.com>,
	Lee Jones <lee.jones@...aro.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Chih-Wei Huang <cwhuang@...roid-x86.org>
Subject: Re: [Intel-gfx] [v2 7/7] drm/i915: Backlight control using CRC PMIC based PWM driver

On Thu, 25 Jun 2015, Shobhit Kumar <kumar@...bhit.info> wrote:
> On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä
> <ville.syrjala@...ux.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@...ux.intel.com>
>>> Cc: Linus Walleij <linus.walleij@...aro.org>
>>> Cc: Alexandre Courbot <gnurou@...il.com>
>>> Cc: Thierry Reding <thierry.reding@...il.com>
>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@...el.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@...ux.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@...ux.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@...ts.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@...ts.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists