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

["Kumar, Shobhit" <shobhit.kumar@linux.intel.com>]

On 2/26/2015 3:43 PM, Alexandre Courbot wrote:
> 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.

Thanks for confirming the bad feeling that I already had with this patch 
as I knew I was not requesting gpio. Was already starting to look into 
the directions you pointed out. Will send corrected implementation soon 
as suggested.

Regards
Shobhit

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx