[Intel-gfx] [RFC Patch 1/3] DRM/I915: parse child device from VBT

[Jesse Barnes]

On Fri,  7 Aug 2009 10:55:17 +0800
yakui.zhao at intel.com wrote:

> From: Zhao Yakui <yakui.zhao at intel.com>
> 
> Parse the child device from VBT.

Can you make the changelog a bit more verbose?  What types of child
devices are available?  On what ranges of chipsets?  Just a little bit
of description for someone browsing the history...

>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +		dev_priv->child_dev = NULL;
> +		dev_priv->child_dev_num = 0;

Not needed, the dev_priv is allocated and zeroed already.

  
> +static void
> +parse_device_mapping_from_vbt(struct drm_i915_private *dev_priv,
> +		       struct bdb_header *bdb)

The _vbt suffix is redundant since the whole file deals with VBT
parsing.  So you can just call it "parse_child_devices" or something
similar.

> +{
> +	struct bdb_general_definitions *p_defs;
> +	struct child_device_config *p_child, *child_dev_ptr;
> +	int i, child_device_num, count;
> +	u16	block_size, *block_ptr;
> +
> +	p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
> +	if (!p_defs) {
> +		DRM_DEBUG("No general definition block is found\n");
> +		return;
> +	}

Eric may have already mentioned this, but we don't do Hungarian
notation in the kernel, so you can drop the "p_" prefixes to pointer
types.  We should probably clean up parse_sdvo_Device_mapping at some
point too (maybe this code was initially copy & pasted from that?).

> +	/* judge whether the size of child device meets the
> requirements.
> +	 * If the child device size obtained from general definition
> block
> +	 * is different with sizeof(struct child_device_config),
> skip the
> +	 * parsing of sdvo device info
> +	 */
> +	if (p_defs->child_dev_size != sizeof(*p_child)) {
> +		/* different child dev size . Ignore it */
> +		DRM_DEBUG("different child size is found.
> Invalid.\n");
> +		return;
> +	}

The message should be "Unsupported child device size, ignoring.\n".

> +	/* get the block size of general definitions */
> +	block_ptr = (u16 *)((char *)p_defs - 2);
> +	block_size = *block_ptr;
> +	/* get the number of child device */
> +	child_device_num = (block_size - sizeof(*p_defs)) /
> +				sizeof(*p_child);
> +	count = 0;
> +	/* get the number of child device that is present */
> +	for (i = 0; i < child_device_num; i++) {
> +		p_child = &(p_defs->devices[i]);
> +		if (!p_child->device_type) {
> +			/* skip the device block if device type is
> invalid */
> +			continue;
> +		}
> +		count++;
> +	}
> +	if (!count) {
> +		DRM_DEBUG("no child dev is parsed from VBT \n");
> +		return;
> +	}

More wordsmithing: "VBT contains no child devices.\n"

>  /**
>   * intel_init_bios - initialize VBIOS settings & find VBT
>   * @dev: DRM device
> @@ -365,6 +428,7 @@
>  	parse_lfp_panel_data(dev_priv, bdb);
>  	parse_sdvo_panel_data(dev_priv, bdb);
>  	parse_sdvo_device_mapping(dev_priv, bdb);
> +	parse_device_mapping_from_vbt(dev_priv, bdb);
>  	parse_driver_features(dev_priv, bdb);
>  
>  	pci_unmap_rom(pdev, bios);

Do we know how reliable the child device structures are?  Should we
limit the parsing to G4x devices?

-- 
Jesse Barnes, Intel Open Source Technology Center