Re: [PATCH 2/2] drm/i915: Use the CRC gpio_chip for panel enable/disable

[Alexandre Courbot <gnurou@gmail.com>]

On Wed, Feb 18, 2015 at 9:18 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> The CRC (Crystal Cove) PMIC, controls the panel enable and disable
> signals for BYT for dsi panels. This is indicated in the VBT fields. Use
> that to initialize and use GPIO based control for these signals.
>
> 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_dsi.c | 35 +++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_dsi.h | 11 +++++++++++
>  2 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index c8c8b24..6b56ca0 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_panel.h>
>  #include <drm/drm_mipi_dsi.h>
>  #include <linux/slab.h>
> +#include <linux/gpio.h>
>  #include "i915_drv.h"
>  #include "intel_drv.h"
>  #include "intel_dsi.h"
> @@ -415,6 +416,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>
>         DRM_DEBUG_KMS("\n");
>
> +       /* Panel Enable over CRC PMIC if needed */
> +       if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC)
> +               gpio_set_value_cansleep(
> +                               intel_dsi->crc_base + GPIO_PANEL_EN, 1);
> +
> +       msleep(intel_dsi->panel_on_delay);
> +
>         /* Disable DPOunit clock gating, can stall pipe
>          * and we need DPLL REFA always enabled */
>         tmp = I915_READ(DPLL(pipe));
> @@ -432,8 +440,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>         /* put device in ready state */
>         intel_dsi_device_ready(encoder);
>
> -       msleep(intel_dsi->panel_on_delay);
> -
>         drm_panel_prepare(intel_dsi->panel);
>
>         for_each_dsi_port(port, intel_dsi->ports)
> @@ -576,6 +582,11 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
>
>         msleep(intel_dsi->panel_off_delay);
>         msleep(intel_dsi->panel_pwr_cycle_delay);
> +
> +       /* Panel Disable over CRC PMIC if needed */
> +       if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC)
> +               gpio_set_value_cansleep(
> +                               intel_dsi->crc_base + GPIO_PANEL_EN, 0);
>  }
>
>  static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
> @@ -977,6 +988,12 @@ static const struct drm_connector_funcs intel_dsi_connector_funcs = {
>         .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>
> +static int match_gpio_chip_by_label(struct gpio_chip *chip,
> +                                               void *data)
> +{
> +       return !strcmp(chip->label, data);
> +}
> +
>  void intel_dsi_init(struct drm_device *dev)
>  {
>         struct intel_dsi *intel_dsi;
> @@ -1070,6 +1087,20 @@ void intel_dsi_init(struct drm_device *dev)
>                 goto err;
>         }
>
> +       /*
> +        * In case of BYT with CRC PMIC, we need to use GPIO for
> +        * Panel control. Store the GPIO base
> +        */
> +       if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) {
> +               struct gpio_chip *gpio;
> +               gpio = gpiochip_find(GPIO_CHIP_NAME, match_gpio_chip_by_label);
> +               if (!gpio) {
> +                       printk("Failed to find crc gpio chip\n");
> +                       intel_dsi->crc_base = 0;
> +               } else
> +                       intel_dsi->crc_base = gpio->base;
> +       }

This looks terribly wrong - you lookup a particular GPIO chip by name,
use a forged GPIO number without even requesting it, and are using
deprecated functions.

Please use the GPIO descriptor interface when adding new GPIO code,
see Documentation/gpio/consumer.h. The integer-based GPIO interface is
considered deprecated and should not be used for new code.

Using gpiod_* functions will prevent you from doing the other mistakes
I mentioned, since it forces you to request your GPIO properly and
will not allow you to forge a descriptor.

See also Documentation/gpio/board.txt for how you can associate GPIOs
to your device's functions depending on which firmware your platform
is using.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx