Re: [patch 3/6] backlight and output sysfs support for acpi video driver

[Pavel Machek <pavel@ucw.cz>]

Hi!

> backlight and output sysfs support for acpi video driver

Yes, using generic interfaces is always nice...

> @@ -482,6 +536,134 @@ acpi_video_bus_DOS(struct acpi_video_bus
>  	return status;
>  }
>  
> +
> +/*
> + * copy & paste some code for acpi_pci_data, acpi_pci_data_handler,acpi_pci_data
> + * from pci_bind.c
> + * To-do: write a new API: acpi_pci_get.
> + */

This comment only(?) serves to confuse me...

> +	pathname = kmalloc(ACPI_PATHNAME_MAX, GFP_KERNEL);
> +	if (!pathname)
> +		return -ENOMEM;
> +	memset(pathname, 0, ACPI_PATHNAME_MAX);

kzalloc()? (More than once in this file).

> +	buffer.length = ACPI_PATHNAME_MAX;
> +	buffer.pointer = pathname;
> +
> +	data = kmalloc(sizeof(struct acpi_pci_data), GFP_KERNEL);
> +	if (!data) {
> +		kfree(pathname);
> +		return NULL;
> +	}
> +	memset(data, 0, sizeof(struct acpi_pci_data));
> +
> +	acpi_get_name(device->handle, ACPI_FULL_PATHNAME, &buffer);
> +	printk(KERN_INFO PREFIX "finding PCI device [%s]...\n", pathname);
> +
> +	/*
> +	 * Segment & Bus
> +	 * -------------
> +	 * These are obtained via the parent device's ACPI-PCI context.
> +	 */
> +go_up:
> +	status = acpi_get_data(device->parent->handle, acpi_pci_data_handler,
> +			       (void **)&pdata);
> +	if (ACPI_FAILURE(status) || !pdata || !pdata->bus) {
> +		struct acpi_device *tmp_dev;
> +
> +		tmp_dev = device->parent;
> +		if (tmp_dev->parent && (tmp_dev->parent->handle != ACPI_ROOT_OBJECT)) {
> +			device = tmp_dev;
> +			goto go_up;
> +		}

Could we use plain old loop here?

> +	printk(KERN_INFO PREFIX "data->dev =%p", &data->dev);
> +	printk(KERN_INFO PREFIX "data->dev->dev =%p\n", &data->dev->dev);

I doubt this is useful info for non-debugging. dprintk?
>  
>  
> +	data = acpi_pci_get (device->video->device);
> +        if (!data || !(data->dev)) {

tabs vs. spaces... more than once in this file.

> +		printk(KERN_ERR PREFIX "acpi_video_device:no valid data from acpi_pci_get\n");
> +		return ;

return; looks more natural.

> @@ -1691,8 +1913,9 @@ static int acpi_video_bus_add(struct acp
>  	int result = 0;
>  	acpi_status status = 0;
>  	struct acpi_video_bus *video = NULL;
> +        char proc_dir_name[32];
>  
> -
> +	memset(proc_dir_name, 0, 32);

What about '= { 0, };', instead?

						Pavel
-- 
Thanks for all the (sleeping) penguins.