Re: [Generic GPIO patch 3/3] drm/i915: BXT GPIO support for backlight and panel control

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 19 Feb 2016, Deepak M <m.deepak@intel.com> wrote:
> From: Uma Shankar <uma.shankar@intel.com>
>
> Added the BXT GPIO pin configuration and programming logic for
> backlight and panel control.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 46 ++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 7fd1fae..c6e18fe 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -32,6 +32,7 @@
>  #include <linux/slab.h>
>  #include <video/mipi_display.h>
>  #include <asm/intel-mid.h>
> +#include <linux/gpio.h>
>  #include <video/mipi_display.h>
>  #include "i915_drv.h"
>  #include "intel_drv.h"
> @@ -593,6 +594,16 @@ static struct gpio_table gtable[] = {
>  	{ VLV_USB_ULPI_0_REFCLK_GPIOS_43_PCONF0, VLV_USB_ULPI_0_REFCLK_GPIOS_43_PAD, 0}
>  };
>  
> +struct bxt_gpio_table {
> +	u16 gpio_pin;
> +	u16 offset;
> +};
> +
> +static struct bxt_gpio_table bxt_gtable[] = {
> +	{0xC1, 270},
> +	{0x1B, 456}

Where's this magic from?

> +};
> +
>  static inline enum port intel_dsi_seq_port_to_port(u8 port)
>  {
>  	return port ? PORT_C : PORT_A;
> @@ -812,6 +823,39 @@ out:
>  	return 0;
>  }
>  
> +static int bxt_program_gpio(struct intel_dsi *intel_dsi,
> +				const u8 *data, const u8 **cur_data)
> +{
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u8 gpio, action;
> +	u16 function;
> +
> +	/*
> +	 * Skipping the first byte as it is of no
> +	 * interest for android in new version
> +	 */
> +	if (dev_priv->vbt.dsi.seq_version >= 3)
> +		data++;
> +
> +	gpio = *data++;
> +
> +	/* pull up/down */
> +	action = *data++;
> +	function = (bxt_gtable[0].gpio_pin == gpio) ?
> +		bxt_gtable[0].offset :
> +		(bxt_gtable[1].gpio_pin == gpio) ?
> +		bxt_gtable[1].offset : 0;

Don't be lazy, please turn this into a for loop looking at the
table. This is just making it painful for the poor developer who needs
to add one more thing into the table later on.

The smart thing to do would be to abstract this part between vlv/chv/bxt
anyway. There's really no reason to have this duplicated.

> +	if (!function)
> +		return -1;
> +
> +	gpio_request_one(function, GPIOF_DIR_OUT, "MIPI");

This might succeed the first time, but subsequent calls will fail. This
is like allocation. You also need to gpio_free afterwards, but you're
not supposed to do request/free every time you want to set the value.

> +	gpio_set_value(function, action);
> +
> +	*cur_data = data;
> +	return 0;
> +}
> +
>  static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  {
>  	struct drm_device *dev = intel_dsi->base.base.dev;
> @@ -825,6 +869,8 @@ static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data)
>  		ret = chv_program_gpio(intel_dsi, data, &data);
>  	else if (IS_VALLEYVIEW(dev))
>  		ret = vlv_program_gpio(intel_dsi, data, &data);
> +	else if (IS_BROXTON(dev))
> +		ret = bxt_program_gpio(intel_dsi, data, &data);
>  	else
>  		DRM_ERROR("GPIO programming missing for this platform.\n");

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx