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

[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ä
> <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> >> Cc: Alexandre Courbot <gnurou@xxxxxxxxx>
> >> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@xxxxxxxxx>
> >> ---
> >>  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.

-- 
Ville Syrjälä
Intel OTC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/