Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752203AbbFZIda (ORCPT ); Fri, 26 Jun 2015 04:33:30 -0400 Received: from mga14.intel.com ([192.55.52.115]:15671 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751579AbbFZIdU convert rfc822-to-8bit (ORCPT ); Fri, 26 Jun 2015 04:33:20 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,683,1427785200"; d="scan'208";a="750836008" From: Jani Nikula To: Shobhit Kumar , Ville =?utf-8?B?U3lyasOkbMOk?= Cc: Shobhit Kumar , Alexandre Courbot , Paul Bolle , Samuel Ortiz , linux-pwm , Povilas Staniulis , intel-gfx , Paul Gortmaker , linux-kernel , dri-devel , linux-gpio , Brain WrecK , Thierry Reding , David Airlie , Daniel Vetter , Lee Jones , Linus Walleij , Chih-Wei Huang Subject: Re: [Intel-gfx] [v2 7/7] drm/i915: Backlight control using CRC PMIC based PWM driver In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <1434970465-12687-1-git-send-email-shobhit.kumar@intel.com> <1434970465-12687-8-git-send-email-shobhit.kumar@intel.com> <20150625084841.GZ5176@intel.com> User-Agent: Notmuch/0.19+112~g77230b0 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Fri, 26 Jun 2015 11:31:17 +0300 Message-ID: <874mlu7twq.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12290 Lines: 313 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(&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 >>> #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 = &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ä >> >> 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ä >> 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 -- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/