Re: [ACPI] [PATCH] ACPI based Memory Hotplug Driver Patch

[Bjorn H. <bjo...@hp...>]

Nice work!

On Thursday 09 September 2004 4:54 am, S, Naveen B wrote:
> +acpi_memory_get_device_resources(struct acpi_memory_device *mem_device) 
> +{
> +	acpi_status status;
> +	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};	
> +	struct acpi_resource *resource = NULL;
> +	struct acpi_resource_address16 *address16 = NULL;
> +	struct acpi_resource_address32 *address32 = NULL;
> +	struct acpi_resource_address64 *address64 = NULL;

You can use acpi_resource_to_address64() to keep from having to write
the same code for address16, address32, and address64.

> +	status = acpi_get_current_resources(mem_device->handle, &buffer);
> +	if (ACPI_FAILURE(status))
> +		return_VALUE(-EINVAL);
> +
> +	resource = (struct acpi_resource *) buffer.pointer;
> +		
> +	switch (resource->id) {
> +	case ACPI_RSTYPE_ADDRESS16:

I'm not sure you should assume the range will be the only item in _CRS
(i.e., you may need to iterate over all items using acpi_walk_resources()).

> +static int 
> +acpi_memory_check_device(struct acpi_memory_device *mem_device)

Could you just use acpi_bus_get_status() instead of defining this?

> +static int 
> +acpi_memory_powerdown_device(struct acpi_memory_device *mem_device)
> +{
> + ...
> +	/* Evalute _STA to check if the device is disabled */
> +	status = acpi_evaluate_integer(mem_device->handle, "_STA", 
> +		NULL, &current_status);

acpi_bus_get_status() again?

> +static acpi_status is_memory_device(acpi_handle handle)

(Different indent style).  Seems like this should check _CID as well
as _HID?  Could it leverage acpi_match_ids() somehow?