[Intel-gfx] [DRM/I915]: Check the LID device to decide whether the LVDS should be initialized

[yakui]

On Thu, 2009-07-02 at 09:46 +0800, Eric Anholt wrote:
> From 8f5a8ebda01c505117c93b74eca1b5a49e11b430 Mon Sep 17 00:00:00 2001
> From: Eric Anholt <eric at anholt.net>
> Date: Wed, 1 Jul 2009 18:37:03 -0700
> Subject: [PATCH] drm/i915: Clean up the ACPI LID-based detection code.
> 
> Signed-off-by: Eric Anholt <eric at anholt.net>
> ---
>  drivers/gpu/drm/i915/intel_lvds.c |   69 ++++++++++++++++---------------------
>  1 files changed, 30 insertions(+), 39 deletions(-)
> 
> On Tue, 2009-06-30 at 10:22 +0800, yakui wrote: 
> > On Fri, 2009-06-26 at 09:46 +0800, Zhao, Yakui wrote:
> > > From: Zhao Yakui <yakui.zhao at intel.com>
> > Hi, Eric
> >     How about this patch?
> >     Thanks.
> 
> This code still looks like it needs work -- bad hungarian notation,
> missing whitespace, and gratuitous comments explaining exactly the logic
> of the code you were just reading without enlightening as to why.  How
> about this patch squashed into yours, or on top of it? 
Your changes look OK to me. And the code style is better than mine.

It is enough to squash it into one patch.

Thanks.
   Yakui

> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 2812524..2d0426d 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -788,24 +788,21 @@ static const struct dmi_system_id intel_no_lvds[] = {
>  
>  	{ }	/* terminating entry */
>  };
> +
>  #ifdef CONFIG_ACPI
>  /*
> - * check_lid_device -- check whether it is ACPI LID device.
> + * check_lid_device -- check whether it @handle is an ACPI LID device.
>   * @handle: ACPI device handle
>   * @level : depth in the ACPI namespace tree
>   * @context: the number of LID device when we find the device
>   * @rv: a return value to fill if desired (Not use)
> - *
> - * check whether it is a LID device by comparing the HID. If it is,
> - * increase the number of LID device.
>   */
>  static acpi_status
>  check_lid_device(acpi_handle handle, u32 level, void *context,
> -			void **retyurn_value)
> +			void **return_value)
>  {
> -#define		ACPI_HID_LID		"PNP0C0D"
>  	struct acpi_device *acpi_dev;
> -	int *p_lid = (int *)context;
> +	int *lid_present = context;
>  
>  	acpi_dev = NULL;
>  	/* Get the acpi device for device handle */
> @@ -813,49 +810,44 @@ check_lid_device(acpi_handle handle, u32 level, void *context,
>  		/* If there is no ACPI device for handle, return */
>  		return AE_OK;
>  	}
> -	if (!strncmp(acpi_device_hid(acpi_dev), ACPI_HID_LID, 7)) {
> -		/*
> -		 * compare the device HID with "PNP0C0D". If it is equal, the
> -		 * LID device is found. Increase the count
> -		 */
> -		(*p_lid)++;
> -	}
> +
> +	if (!strncmp(acpi_device_hid(acpi_dev), "PNP0C0D", 7))
> +		*lid_present = 1;
> +
>  	return AE_OK;
>  }
> +
>  /**
>   * check whether there exists the ACPI LID device by enumerating the ACPI
>   * device tree.
> - * If ACPI is disabled, there is no ACPI device tree. one is returned.
> - * If the LID device is found, it will return one.
> - * If no LID device is found, it will return  zero.
>   */
>  static int intel_lid_present(void)
>  {
> -	int lid_count = 0;
> +	int lid_present = 0;
>  
>  	if (acpi_disabled) {
> -		/*
> -		 * if ACPI is disabled, there is no ACPI device tree. And
> -		 * we don't know whether there exists the LID device.
> -		 * In such case we will return 1.
> +		/* If ACPI is disabled, there is no ACPI device tree to
> +		 * check, so assume the LID device would have been present.
>  		 */
>  		return 1;
>  	}
>  
>  	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
>  				ACPI_UINT32_MAX,
> -				check_lid_device, &lid_count, NULL);
> -
> -	if (!lid_count) {
> -		/* No LID device is not found. Return zero */
> -		return 0;
> -	}
> +				check_lid_device, &lid_present, NULL);
>  
> -	return 1;
> +	return lid_present;
>  }
>  #else
> -static inline int intel_lid_present(void) { return 1; }
> +static int intel_lid_present(void)
> +{
> +	/* In the absence of ACPI built in, assume that the LID device would
> +	 * have been present.
> +	 */
> +	return 1;
> +}
>  #endif
> +
>  /**
>   * intel_lvds_init - setup LVDS connectors on this device
>   * @dev: drm device
> @@ -879,16 +871,15 @@ void intel_lvds_init(struct drm_device *dev)
>  	if (dmi_check_system(intel_no_lvds))
>  		return;
>  
> -	if (!intel_lid_present()) {
> -		/* If there is no LID device, we can think that there is
> -		 * no LVDS output device. In such case it is unnecessary to
> -		 * create the LVDS output device.
> -		 * But maybe on some boxes there is no LVDS device while the
> -		 * LID device is found. If so, it had better be added to
> -		 * the quirk list.
> -		 */
> +	/* Assume that any device without an ACPI LID device also doesn't
> +	 * have an integrated LVDS.  We would be better off parsing the BIOS
> +	 * to get a reliable indicator, but that code isn't written yet.
> +	 *
> +	 * In the case of all-in-one desktops using LVDS that we've seen,
> +	 * they're using SDVO LVDS.
> +	 */
> +	if (!intel_lid_present())
>  		return;
> -	}
>  
>  	if (IS_IGDNG(dev)) {
>  		if ((I915_READ(PCH_LVDS) & LVDS_DETECTED) == 0)
> -- 
> 1.6.3.1
>