by Jani Nikula

[permalink] [raw]

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

On Thu, 25 Jun 2015, Shobhit Kumar <[email protected]> wrote:
> On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä
> <[email protected]> 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 <[email protected]>
>>> Cc: Linus Walleij <[email protected]>
>>> Cc: Alexandre Courbot <[email protected]>
>>> Cc: Thierry Reding <[email protected]>
>>> Signed-off-by: Shobhit Kumar <[email protected]>
>>> ---
>>> 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ä <[email protected]>
>>
>> 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ä <[email protected]>
>> 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
>>> [email protected]
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> Intel-gfx mailing list
>> [email protected]
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Jani Nikula, Intel Open Source Technology Center

2015-07-15 08:05:27

by Linus Walleij

[permalink] [raw]

Subject: Re: [v2 1/7] gpiolib: Add support for removing registered consumer lookup table

On Mon, Jun 22, 2015 at 12:54 PM, Shobhit Kumar <[email protected]> wrote:

> In case we unload and load a driver module again that is registering a
> lookup table, without this it will result in multiple entries. Provide
> an option to remove the lookup table on driver unload
>
> v2: Ccing maintainers
> v3: Correct the subject line (Lee jones)
>
> Cc: Samuel Ortiz <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Alexandre Courbot <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Reviewed-by: Alexandre Courbot <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Shobhit Kumar <[email protected]>

Do you plan to take this upstream for the next merge window or
should I just merge this patch?

Yours,
Linus Walleij