Re: RESEND: [PATCH 3/3] Misc: Add HP WMI laptop extras driver

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 28 May 2008 14:34:25 +0100 Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> This driver adds support for reading and configuring certain information 
> on modern HP laptops with WMI BIOS interfaces. It supports enabling and 
> disabling the ambient light sensor, querying attached displays and hard 
> drive temperature, sending events on docking and querying the state of 
> the dock and toggling the state of the wifi, bluetooth and wwan hardware 
> via rfkill. It also makes the little "(i)" button work on machines that 
> send that via WMI rather than via the keyboard controller.
> 
> ...
>
> --- /dev/null
> +++ b/drivers/misc/hp-wmi.c

I see no Kconfig or Makefile updates.

>
> ...
>
> +static struct key_entry hp_wmi_keymap[] = {
> +	{KE_SW, 0x01, SW_DOCK},
> +	{KE_KEY, 0x02, KEY_BRIGHTNESSUP},
> +	{KE_KEY, 0x03, KEY_BRIGHTNESSDOWN},
> +	{KE_KEY, 0x04, KEY_HELP},
> +	{KE_END, 0}
> +};

Could be made const.  hp_wmi_input_setup() already honours that,
others will need tweaks.

> +static struct input_dev *hp_wmi_input_dev;
> +static struct platform_device *hp_wmi_platform_dev;
> +
> +static struct rfkill *wifi_rfkill;
> +static struct rfkill *bluetooth_rfkill;
> +static struct rfkill *wwan_rfkill;

It will be interesting to see the Kconfig rules for this driver..

> +static struct platform_driver hp_wmi_driver = {
> +	.driver = {
> +		   .name = "hp-wmi",
> +		   .owner = THIS_MODULE,
> +		   },

	.driver = {
		   .name = "hp-wmi",
		   .owner = THIS_MODULE,
	},

would be more conventional layout.

> +	.probe = hp_wmi_bios_setup,
> +	.remove = hp_wmi_bios_remove,
> +};
> +
> +static int hp_wmi_perform_query(int query, int write, int value)
> +{
> +	struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL };

The NULL isn't strictly needed.

> +	struct bios_args args;
> +	struct bios_return bios_return;
> +	acpi_status status;
> +	union acpi_object *obj;
> +
> +	args.signature = 0x55434553;
> +	args.command = write ? 0x2 : 0x1;
> +	args.commandtype = query;
> +	args.datasize = write ? 0x4 : 0;
> +	args.data = value;

Could have populated args with `= { .name = value, ... }'?

> +	input.length = sizeof(struct bios_args);
> +	input.pointer = &args;

And `input', perhaps.

> +	status = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, 0x3, &input, &output);
> +
> +	obj = output.pointer;
> +
> +	if (!obj || obj->type != ACPI_TYPE_BUFFER)
> +		return -EINVAL;
> +
> +	bios_return = *((struct bios_return *)obj->buffer.pointer);
> +	if (bios_return.return_code > 0)
> +		return bios_return.return_code * -1;
> +	else
> +		return bios_return.value;
> +}
> +
>
> ...
>
> +static int hp_wmi_setkeycode(struct input_dev *dev, int scancode, int keycode)
> +{
> +	struct key_entry *key;
> +
> +	int old_keycode;

Unneeded newline.

> +	if (keycode < 0 || keycode > KEY_MAX)
> +		return -EINVAL;
> +
> +	key = hp_wmi_get_entry_by_scancode(scancode);
> +	if (key && key->type == KE_KEY) {
> +		old_keycode = key->keycode;
> +		key->keycode = keycode;
> +		set_bit(keycode, dev->keybit);
> +		if (!hp_wmi_get_entry_by_keycode(old_keycode))
> +			clear_bit(old_keycode, dev->keybit);
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>
> ...
>
> +static int __init hp_wmi_bios_setup(struct platform_device *device)
> +{
> +	device_create_file(&device->dev, &dev_attr_display);
> +	device_create_file(&device->dev, &dev_attr_hddtemp);
> +	device_create_file(&device->dev, &dev_attr_als);
> +	device_create_file(&device->dev, &dev_attr_dock);

wham, four new warnings.

Please check and suitably handle errors.