[PATCH 0/2] Add Dell laptop driver

[PATCH 0/2] Add Dell laptop driver

[Matthew Garrett]

This driver adds backlight and rfkill support for Dell laptops. It uses 
the DCDBAS driver to trigger the system management calls required for 
this, and parses the DMI tables itself in order to find the appropriate 
tokens. In future it should be possible to add LED control, but I don't 
have any appropriate machines right now for testing.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 1/2] Export SMI call functionality from dcdbas driver

[Matthew Garrett]

Rename the dcdbas SMI request call to avoid namespacing issues, and then 
export it so other kernel modules can make use of the functionality.

Signed-off-by: Matthew Garrett <mjg@redhat.com>

---

commit be8f9ba0f636c9eec356976acc178623ec58651e
Author: Matthew Garrett <mjg59@m6300.(none)>
Date:   Sat Aug 16 21:12:09 2008 +0100

    Add support for using the dcdbas framework from inside the kernel

diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 50a071f..762131b 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -238,11 +238,11 @@ static ssize_t host_control_on_shutdown_store(struct device *dev,
 }
 
 /**
- * smi_request: generate SMI request
+ * dcdbas_smi_request: generate SMI request
  *
  * Called with smi_data_lock.
  */
-static int smi_request(struct smi_cmd *smi_cmd)
+int dcdbas_smi_request(struct smi_cmd *smi_cmd)
 {
 	cpumask_t old_mask;
 	int ret = 0;
@@ -279,6 +279,8 @@ out:
 	return ret;
 }
 
+EXPORT_SYMBOL(dcdbas_smi_request);
+
 /**
  * smi_request_store:
  *
@@ -309,14 +311,14 @@ static ssize_t smi_request_store(struct device *dev,
 	switch (val) {
 	case 2:
 		/* Raw SMI */
-		ret = smi_request(smi_cmd);
+		ret = dcdbas_smi_request(smi_cmd);
 		if (!ret)
 			ret = count;
 		break;
 	case 1:
 		/* Calling Interface SMI */
 		smi_cmd->ebx = (u32) virt_to_phys(smi_cmd->command_buffer);
-		ret = smi_request(smi_cmd);
+		ret = dcdbas_smi_request(smi_cmd);
 		if (!ret)
 			ret = count;
 		break;
diff --git a/drivers/firmware/dcdbas.h b/drivers/firmware/dcdbas.h
index 87bc341..ca3cb0a 100644
--- a/drivers/firmware/dcdbas.h
+++ b/drivers/firmware/dcdbas.h
@@ -101,5 +101,7 @@ struct apm_cmd {
 	} __attribute__ ((packed)) parameters;
 } __attribute__ ((packed));
 
+int dcdbas_smi_request(struct smi_cmd *smi_cmd);
+
 #endif /* _DCDBAS_H_ */
 

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 2/2] Add Dell laptop driver

[Matthew Garrett]

This driver provides in-kernel control for the backlight and rfkill 
functionality on Dell laptops. It's based on the publically available 
information in the libsmbios package.

Signed-off-by: Matthew Garrett <mjg@redhat.com>

---

commit 472d8eed1891ba440f8e6ffde4787f6308390f7d
Author: Matthew Garrett <mjg59@m6300.(none)>
Date:   Sat Aug 16 21:12:51 2008 +0100

    Add Dell laptop driver

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index a726f3b..c77eab3 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -475,4 +475,16 @@ config SGI_GRU_DEBUG
 	This option enables addition debugging code for the SGI GRU driver. If
 	you are unsure, say N.
 
+config DELL_LAPTOP
+        tristate "Dell Laptop Extras (EXPERIMENTAL)"
+        depends on X86
+        depends on DCDBAS
+	depends on EXPERIMENTAL
+	depends on BACKLIGHT_CLASS_DEVICE
+	depends on RFKILL
+	default n
+	---help---
+	This driver adds support for rfkill and backlight control to Dell
+	laptops.
+
 endif # MISC_DEVICES
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c6c13f6..865343d 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
 obj-$(CONFIG_SGI_XP)		+= sgi-xp/
 obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
 obj-$(CONFIG_HP_ILO)		+= hpilo.o
+obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
\ No newline at end of file
diff --git a/drivers/misc/dell-laptop.c b/drivers/misc/dell-laptop.c
new file mode 100644
index 0000000..ab09a69
--- /dev/null
+++ b/drivers/misc/dell-laptop.c
@@ -0,0 +1,356 @@
+/*
+ *  Driver for Dell laptop extras
+ *
+ *  Copyright (c) Red Hat <mjg@redhat.com>
+ *
+ *  Based on documentation in the libsmbios package, Copyright (C) 2005 Dell
+ *  Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/backlight.h>
+#include <linux/err.h>
+#include <linux/dmi.h>
+#include <linux/io.h>
+#include <linux/rfkill.h>
+#include "../firmware/dcdbas.h"
+
+struct calling_interface_buffer {
+	u16 class;
+	u16 select;
+	volatile u32 input[4];
+	volatile u32 output[4];
+} __attribute__ ((packed));
+
+struct calling_interface_token {
+	u16 tokenID;
+	u16 location;
+	union {
+		u16 value;
+		u16 stringlength;
+	};
+};
+
+struct calling_interface_structure {
+	u8 type;
+	u8 length;
+	u16 handle;
+	u16 cmdIOAddress;
+	u8 cmdIOCode;
+	u32 supportedCmds;
+	struct calling_interface_token tokens[];
+} __attribute__ ((packed));
+
+static int da_command_address;
+static int da_command_code;
+static int da_num_tokens;
+static struct calling_interface_token *da_tokens;
+
+static struct backlight_device *dell_backlight_device;
+static struct rfkill *wifi_rfkill;
+static struct rfkill *bluetooth_rfkill;
+static struct rfkill *wwan_rfkill;
+
+static struct dmi_system_id __initdata dell_device_table[] = {
+	{
+		.ident = "Dell laptop",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_CHASSIS_TYPE, "8"),
+		},
+	},
+	{ }
+};
+
+static void parse_da_table(const struct dmi_header *dm)
+{
+	/* Final token is a terminator, so we don't want to copy it */
+	int tokens = (dm->length-11)/sizeof(struct calling_interface_token)-1;
+	struct calling_interface_structure *table =
+		(struct calling_interface_structure *)dm;
+
+	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
+	   6 bytes of entry */
+
+	if (dm->length < 17)
+		return;
+
+	da_command_address = table->cmdIOAddress;
+	da_command_code = table->cmdIOCode;
+
+	da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
+			     sizeof(struct calling_interface_token),
+			     GFP_KERNEL);
+
+	memcpy(da_tokens+da_num_tokens, table->tokens,
+	       sizeof(struct calling_interface_token) * tokens);
+
+	da_num_tokens += tokens;
+}
+
+static void find_tokens(const struct dmi_header *dm)
+{
+	switch (dm->type) {
+	case 0xd4: /* Indexed IO */
+		break;
+	case 0xd5: /* Protected Area Type 1 */
+		break;
+	case 0xd6: /* Protected Area Type 2 */
+		break;
+	case 0xda: /* Calling interface */
+		parse_da_table(dm);
+		break;
+	}
+}
+
+static int find_token_location(int tokenid)
+{
+	int i;
+	for (i = 0; i < da_num_tokens; i++) {
+		if (da_tokens[i].tokenID == tokenid)
+			return da_tokens[i].location;
+	}
+	return -1;
+}
+
+static struct calling_interface_buffer *dell_send_request(
+	struct calling_interface_buffer *buffer, int class, int select)
+{
+	struct smi_cmd *command = kzalloc(sizeof(struct smi_cmd), GFP_KERNEL);
+	command->magic = SMI_CMD_MAGIC;
+	command->command_address = da_command_address;
+	command->command_code = da_command_code;
+	command->ebx = virt_to_phys(buffer);
+	command->ecx = 0x42534931;
+
+	buffer->class = class;
+	buffer->select = select;
+
+	dcdbas_smi_request(command);
+
+	kfree(command);
+
+	return buffer;
+}
+
+static int dell_rfkill_set(int radio, enum rfkill_state state)
+{
+	struct calling_interface_buffer *buffer =
+		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
+	int disable = (state == RFKILL_STATE_UNBLOCKED) ? 0 : 1;
+	buffer->input[0] = (1 | (radio<<8) | (disable << 16));
+	dell_send_request(buffer, 17, 11);
+	kfree(buffer);
+	return 0;
+}
+
+static int dell_wifi_set(void *data, enum rfkill_state state)
+{
+	return dell_rfkill_set(1, state);
+}
+
+static int dell_bluetooth_set(void *data, enum rfkill_state state)
+{
+	return dell_rfkill_set(2, state);
+}
+
+static int dell_wwan_set(void *data, enum rfkill_state state)
+{
+	return dell_rfkill_set(3, state);
+}
+
+static int dell_rfkill_get(int bit, enum rfkill_state *state)
+{
+	struct calling_interface_buffer *buffer =
+		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
+	int status;
+	int new_state = RFKILL_STATE_HARD_BLOCKED;
+
+	dell_send_request(buffer, 17, 11);
+	status = buffer->output[1];
+
+	if (status & (1<<16))
+		new_state = RFKILL_STATE_SOFT_BLOCKED;
+
+	if (status & (1<<bit))
+		*state = new_state;
+	else
+		*state = RFKILL_STATE_UNBLOCKED;
+
+	kfree(buffer);
+
+	return 0;
+}
+
+static int dell_wifi_get(void *data, enum rfkill_state *state)
+{
+	return dell_rfkill_get(17, state);
+}
+
+static int dell_bluetooth_get(void *data, enum rfkill_state *state)
+{
+	return dell_rfkill_get(18, state);
+}
+
+static int dell_wwan_get(void *data, enum rfkill_state *state)
+{
+	return dell_rfkill_get(19, state);
+}
+
+static void dell_setup_rfkill(void)
+{
+	struct calling_interface_buffer *buffer =
+		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
+	int status;
+
+	dell_send_request(buffer, 17, 11);
+	status = buffer->output[1];
+
+	if ((status & (1<<2|1<<8)) == (1<<2|1<<8)) {
+		wifi_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_WLAN);
+		wifi_rfkill->name = "dell-wifi";
+		wifi_rfkill->toggle_radio = dell_wifi_set;
+		wifi_rfkill->get_state = dell_wifi_get;
+		rfkill_register(wifi_rfkill);
+	}
+
+	if ((status & (1<<3|1<<9)) == (1<<3|1<<9)) {
+		bluetooth_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_BLUETOOTH);
+		bluetooth_rfkill->name = "dell-bluetooth";
+		bluetooth_rfkill->toggle_radio = dell_bluetooth_set;
+		bluetooth_rfkill->get_state = dell_bluetooth_get;
+		rfkill_register(bluetooth_rfkill);
+	}
+
+	if ((status & (1<<4|1<<10)) == (1<<4|1<<10)) {
+		wwan_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_WWAN);
+		wwan_rfkill->name = "dell-wwan";
+		wwan_rfkill->toggle_radio = dell_wwan_set;
+		wwan_rfkill->get_state = dell_wwan_get;
+		rfkill_register(wwan_rfkill);
+	}
+
+	kfree(buffer);
+}
+
+static int dell_send_intensity(struct backlight_device *bd)
+{
+	int intensity = bd->props.brightness;
+	struct calling_interface_buffer *buffer =
+		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
+
+	buffer->input[0] = find_token_location(0x7d);
+	buffer->input[1] = intensity;
+
+	if (buffer->input[0] == -1) {
+		kfree(buffer);
+		return -ENODEV;
+	}
+
+	dell_send_request(buffer, 1, 1);
+	dell_send_request(buffer, 1, 2);
+
+	kfree(buffer);
+
+	return 0;
+}
+
+static int dell_get_intensity(struct backlight_device *bd)
+{
+	struct calling_interface_buffer *buffer =
+		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
+	int brightness;
+
+	buffer->input[0] = find_token_location(0x7d);
+
+	if (buffer->input[0] == -1) {
+		kfree(buffer);
+		return -ENODEV;
+	}
+
+	dell_send_request(buffer, 0, 1);
+
+	brightness = buffer->output[1];
+
+	kfree(buffer);
+
+	return brightness;
+}
+
+static struct backlight_ops dell_ops = {
+	.get_brightness = dell_get_intensity,
+	.update_status  = dell_send_intensity,
+};
+
+static int __init dell_init(void)
+{
+	struct calling_interface_buffer *buffer;
+	int max_intensity = 0;
+
+	if (!dmi_check_system(dell_device_table))
+		return -ENODEV;
+
+	dmi_walk(find_tokens);
+
+	if (!da_tokens)
+		return -ENODEV;
+
+	buffer = kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
+	buffer->input[0] = find_token_location(0x7d);
+
+	if (buffer->input[0] != -1) {
+		dell_send_request(buffer, 2, 1);
+		max_intensity = buffer->output[3];
+	}
+
+	kfree(buffer);
+
+	if (max_intensity) {
+		dell_backlight_device = backlight_device_register(
+			"dell_backlight",
+			NULL, NULL,
+			&dell_ops);
+
+		if (IS_ERR(dell_backlight_device)) {
+			dell_backlight_device = NULL;
+			goto out;
+		}
+
+		dell_backlight_device->props.max_brightness = max_intensity;
+		dell_backlight_device->props.brightness =
+			dell_get_intensity(dell_backlight_device);
+		backlight_update_status(dell_backlight_device);
+	}
+
+	dell_setup_rfkill();
+
+out:
+	return 0;
+}
+
+static void __exit dell_exit(void)
+{
+	if (dell_backlight_device)
+		backlight_device_unregister(dell_backlight_device);
+	if (wifi_rfkill)
+		rfkill_unregister(wifi_rfkill);
+	if (bluetooth_rfkill)
+		rfkill_unregister(bluetooth_rfkill);
+	if (wwan_rfkill)
+		rfkill_unregister(wwan_rfkill);
+}
+
+module_init(dell_init);
+module_exit(dell_exit);
+
+MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_DESCRIPTION("Dell laptop driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("dmi:*svnDellInc.:*:ct8:*");

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/2] Add Dell laptop driver

[Arjan van de Ven]

On Sat, 16 Aug 2008 21:30:27 +0100
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> This driver provides in-kernel control for the backlight and rfkill 
> functionality on Dell laptops. It's based on the publically available 
> information in the libsmbios package.


how does this interact with the backlight control done via ACPI ?


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 2/2] Add Dell laptop driver

[Matthew Garrett]

On Sat, Aug 16, 2008 at 01:47:52PM -0700, Arjan van de Ven wrote:

> how does this interact with the backlight control done via ACPI ?

Dell's ACPI implementation appears to be implemented on top of this at a 
firmware level, so both are consistent. However, in terms of 
cleanliness, we should wait for Thomas's patches to be merged so this 
driver can disable the backlight support if there's an ACPI backlight 
present. The reason I implemented it is that there's no ACPI backlight 
support on some currently shipping Dells, never mind the older hardware.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/2] Add Dell laptop driver

[Andrew Morton]

On Sat, 16 Aug 2008 21:30:27 +0100 Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> This driver provides in-kernel control for the backlight and rfkill 
> functionality on Dell laptops. It's based on the publically available 
> information in the libsmbios package.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> ---
> 
> commit 472d8eed1891ba440f8e6ffde4787f6308390f7d
> Author: Matthew Garrett <mjg59@m6300.(none)>
> Date:   Sat Aug 16 21:12:51 2008 +0100
> 
>     Add Dell laptop driver
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a726f3b..c77eab3 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -475,4 +475,16 @@ config SGI_GRU_DEBUG
>  	This option enables addition debugging code for the SGI GRU driver. If
>  	you are unsure, say N.
>  
> +config DELL_LAPTOP
> +        tristate "Dell Laptop Extras (EXPERIMENTAL)"
> +        depends on X86
> +        depends on DCDBAS
> +	depends on EXPERIMENTAL
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on RFKILL
> +	default n
> +	---help---
> +	This driver adds support for rfkill and backlight control to Dell
> +	laptops.

whitespace gone wild.

>  endif # MISC_DEVICES
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index c6c13f6..865343d 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
>  obj-$(CONFIG_SGI_XP)		+= sgi-xp/
>  obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
>  obj-$(CONFIG_HP_ILO)		+= hpilo.o
> +obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
> \ No newline at end of file
> diff --git a/drivers/misc/dell-laptop.c b/drivers/misc/dell-laptop.c
> new file mode 100644
> index 0000000..ab09a69
> --- /dev/null
> +++ b/drivers/misc/dell-laptop.c
> @@ -0,0 +1,356 @@
> +/*
> + *  Driver for Dell laptop extras
> + *
> + *  Copyright (c) Red Hat <mjg@redhat.com>
> + *
> + *  Based on documentation in the libsmbios package, Copyright (C) 2005 Dell
> + *  Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/backlight.h>
> +#include <linux/err.h>
> +#include <linux/dmi.h>
> +#include <linux/io.h>
> +#include <linux/rfkill.h>
> +#include "../firmware/dcdbas.h"
> +
> +struct calling_interface_buffer {
> +	u16 class;
> +	u16 select;
> +	volatile u32 input[4];
> +	volatile u32 output[4];
> +} __attribute__ ((packed));

checkpatch will direct you to a fine document.

If the `volatiles' are unavoidable then I'd suggest that both the
changelog and code comments explain the reason(s) for their addition.

> +struct calling_interface_token {
> +	u16 tokenID;
> +	u16 location;
> +	union {
> +		u16 value;
> +		u16 stringlength;
> +	};
> +};
> +
> +struct calling_interface_structure {
> +	u8 type;
> +	u8 length;
> +	u16 handle;

Make the above three members a `struct dmi_header' ...

> +	u16 cmdIOAddress;
> +	u8 cmdIOCode;
> +	u32 supportedCmds;
> +	struct calling_interface_token tokens[];
> +} __attribute__ ((packed));
> +
> +static int da_command_address;
> +static int da_command_code;
> +static int da_num_tokens;
> +static struct calling_interface_token *da_tokens;
> +
> +static struct backlight_device *dell_backlight_device;
> +static struct rfkill *wifi_rfkill;
> +static struct rfkill *bluetooth_rfkill;
> +static struct rfkill *wwan_rfkill;
> +
> +static struct dmi_system_id __initdata dell_device_table[] = {
> +	{
> +		.ident = "Dell laptop",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_CHASSIS_TYPE, "8"),
> +		},
> +	},
> +	{ }
> +};
> +
> +static void parse_da_table(const struct dmi_header *dm)
> +{
> +	/* Final token is a terminator, so we don't want to copy it */
> +	int tokens = (dm->length-11)/sizeof(struct calling_interface_token)-1;
> +	struct calling_interface_structure *table =
> +		(struct calling_interface_structure *)dm;

... and switch this to container_of().

> +	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
> +	   6 bytes of entry */
> +
> +	if (dm->length < 17)
> +		return;
> +
> +	da_command_address = table->cmdIOAddress;
> +	da_command_code = table->cmdIOCode;
> +
> +	da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
> +			     sizeof(struct calling_interface_token),
> +			     GFP_KERNEL);
> +
> +	memcpy(da_tokens+da_num_tokens, table->tokens,
> +	       sizeof(struct calling_interface_token) * tokens);

krealloc() can fail.

> +	da_num_tokens += tokens;
> +}
> +
> +static void find_tokens(const struct dmi_header *dm)
> +{
> +	switch (dm->type) {
> +	case 0xd4: /* Indexed IO */
> +		break;
> +	case 0xd5: /* Protected Area Type 1 */
> +		break;
> +	case 0xd6: /* Protected Area Type 2 */
> +		break;
> +	case 0xda: /* Calling interface */
> +		parse_da_table(dm);
> +		break;
> +	}
> +}
> +
> +static int find_token_location(int tokenid)
> +{
> +	int i;
> +	for (i = 0; i < da_num_tokens; i++) {
> +		if (da_tokens[i].tokenID == tokenid)
> +			return da_tokens[i].location;
> +	}
> +	return -1;
> +}
> +
> +static struct calling_interface_buffer *dell_send_request(
> +	struct calling_interface_buffer *buffer, int class, int select)

static struct calling_interface_buffer *
dell_send_request(struct calling_interface_buffer *buffer, int class,
		  int select)

would be more typical layout, although not vastly better.

> +{
> +	struct smi_cmd *command = kzalloc(sizeof(struct smi_cmd), GFP_KERNEL);
> +	command->magic = SMI_CMD_MAGIC;
> +	command->command_address = da_command_address;
> +	command->command_code = da_command_code;
> +	command->ebx = virt_to_phys(buffer);
> +	command->ecx = 0x42534931;

Please put a blank line between end-of-locals and start-of-code. 
Everywhere, all the time.

kzalloc() can fail and must be checked.

> +	buffer->class = class;
> +	buffer->select = select;
> +
> +	dcdbas_smi_request(command);
> +
> +	kfree(command);

Can we make `command' a local?  It's pretty small.  Perhaps
dcdbas_smi_request() (nee smi_request()) has some secret, undocumented
alignment restriction, similar to DMA?

> +	return buffer;
> +}
> +
> +static int dell_rfkill_set(int radio, enum rfkill_state state)
> +{
> +	struct calling_interface_buffer *buffer =
> +		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
> +	int disable = (state == RFKILL_STATE_UNBLOCKED) ? 0 : 1;
> +	buffer->input[0] = (1 | (radio<<8) | (disable << 16));
> +	dell_send_request(buffer, 17, 11);
> +	kfree(buffer);
> +	return 0;
> +}

kzalloc() can fail.

calling_interface_buffer is small enough to be a local.

coding-style.

> +static int dell_wifi_set(void *data, enum rfkill_state state)
> +{
> +	return dell_rfkill_set(1, state);
> +}
> +
> +static int dell_bluetooth_set(void *data, enum rfkill_state state)
> +{
> +	return dell_rfkill_set(2, state);
> +}
> +
> +static int dell_wwan_set(void *data, enum rfkill_state state)
> +{
> +	return dell_rfkill_set(3, state);
> +}
> +
> +static int dell_rfkill_get(int bit, enum rfkill_state *state)
> +{
> +	struct calling_interface_buffer *buffer =
> +		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
> +	int status;
> +	int new_state = RFKILL_STATE_HARD_BLOCKED;
> +
> +	dell_send_request(buffer, 17, 11);
> +	status = buffer->output[1];

kzalloc() can fail.

> +	if (status & (1<<16))
> +		new_state = RFKILL_STATE_SOFT_BLOCKED;
> +
> +	if (status & (1<<bit))
> +		*state = new_state;
> +	else
> +		*state = RFKILL_STATE_UNBLOCKED;
> +
> +	kfree(buffer);

`buffer' is small enough to be a local.

> +	return 0;
> +}
> +
> +static int dell_wifi_get(void *data, enum rfkill_state *state)
> +{
> +	return dell_rfkill_get(17, state);
> +}
> +
> +static int dell_bluetooth_get(void *data, enum rfkill_state *state)
> +{
> +	return dell_rfkill_get(18, state);
> +}
> +
> +static int dell_wwan_get(void *data, enum rfkill_state *state)
> +{
> +	return dell_rfkill_get(19, state);
> +}
> +
> +static void dell_setup_rfkill(void)
> +{
> +	struct calling_interface_buffer *buffer =
> +		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);

ditto, ditto

> +	int status;
> +
> +	dell_send_request(buffer, 17, 11);
> +	status = buffer->output[1];
> +
> +	if ((status & (1<<2|1<<8)) == (1<<2|1<<8)) {

Where does the reader go to find the meaning of these bits?

(code comment needed)

> +		wifi_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_WLAN);
> +		wifi_rfkill->name = "dell-wifi";
> +		wifi_rfkill->toggle_radio = dell_wifi_set;
> +		wifi_rfkill->get_state = dell_wifi_get;
> +		rfkill_register(wifi_rfkill);
> +	}
> +
> +	if ((status & (1<<3|1<<9)) == (1<<3|1<<9)) {
> +		bluetooth_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_BLUETOOTH);
> +		bluetooth_rfkill->name = "dell-bluetooth";
> +		bluetooth_rfkill->toggle_radio = dell_bluetooth_set;
> +		bluetooth_rfkill->get_state = dell_bluetooth_get;
> +		rfkill_register(bluetooth_rfkill);
> +	}
> +
> +	if ((status & (1<<4|1<<10)) == (1<<4|1<<10)) {
> +		wwan_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_WWAN);
> +		wwan_rfkill->name = "dell-wwan";
> +		wwan_rfkill->toggle_radio = dell_wwan_set;
> +		wwan_rfkill->get_state = dell_wwan_get;
> +		rfkill_register(wwan_rfkill);
> +	}

dittoes

> +	kfree(buffer);
> +}
> +
> +static int dell_send_intensity(struct backlight_device *bd)
> +{
> +	int intensity = bd->props.brightness;
> +	struct calling_interface_buffer *buffer =
> +		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);

blah

> +	buffer->input[0] = find_token_location(0x7d);
> +	buffer->input[1] = intensity;

I don't think we really needed local variable `intensity'.  I guess it
has some commentary value.

> +	if (buffer->input[0] == -1) {
> +		kfree(buffer);
> +		return -ENODEV;
> +	}
> +
> +	dell_send_request(buffer, 1, 1);
> +	dell_send_request(buffer, 1, 2);
> +
> +	kfree(buffer);
> +
> +	return 0;
> +}
> +
> +static int dell_get_intensity(struct backlight_device *bd)
> +{
> +	struct calling_interface_buffer *buffer =
> +		kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);

ditto, ditto

> +	int brightness;
> +
> +	buffer->input[0] = find_token_location(0x7d);
> +
> +	if (buffer->input[0] == -1) {
> +		kfree(buffer);
> +		return -ENODEV;
> +	}
> +
> +	dell_send_request(buffer, 0, 1);
> +
> +	brightness = buffer->output[1];
> +
> +	kfree(buffer);
> +
> +	return brightness;
> +}
> +
> +static struct backlight_ops dell_ops = {
> +	.get_brightness = dell_get_intensity,
> +	.update_status  = dell_send_intensity,
> +};
> +
> +static int __init dell_init(void)
> +{
> +	struct calling_interface_buffer *buffer;
> +	int max_intensity = 0;
> +
> +	if (!dmi_check_system(dell_device_table))
> +		return -ENODEV;
> +
> +	dmi_walk(find_tokens);
> +
> +	if (!da_tokens)
> +		return -ENODEV;
> +
> +	buffer = kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);

ditto

> +	buffer->input[0] = find_token_location(0x7d);
> +
> +	if (buffer->input[0] != -1) {
> +		dell_send_request(buffer, 2, 1);
> +		max_intensity = buffer->output[3];
> +	}
> +
> +	kfree(buffer);
> +
> +	if (max_intensity) {
> +		dell_backlight_device = backlight_device_register(
> +			"dell_backlight",
> +			NULL, NULL,
> +			&dell_ops);
> +
> +		if (IS_ERR(dell_backlight_device)) {
> +			dell_backlight_device = NULL;
> +			goto out;

Should propagate PTR_ERR(dell_backlight_device) back to caller.

> +		}
> +
> +		dell_backlight_device->props.max_brightness = max_intensity;
> +		dell_backlight_device->props.brightness =
> +			dell_get_intensity(dell_backlight_device);
> +		backlight_update_status(dell_backlight_device);
> +	}
> +
> +	dell_setup_rfkill();
> +
> +out:
> +	return 0;
> +}
> +
> +static void __exit dell_exit(void)
> +{
> +	if (dell_backlight_device)
> +		backlight_device_unregister(dell_backlight_device);

backlight_device_unregister(NULL) is legal

> +	if (wifi_rfkill)
> +		rfkill_unregister(wifi_rfkill);
> +	if (bluetooth_rfkill)
> +		rfkill_unregister(bluetooth_rfkill);
> +	if (wwan_rfkill)
> +		rfkill_unregister(wwan_rfkill);
> +}
> +
> +module_init(dell_init);
> +module_exit(dell_exit);
> +
> +MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
> +MODULE_DESCRIPTION("Dell laptop driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("dmi:*svnDellInc.:*:ct8:*");

Re: [PATCH 2/2] Add Dell laptop driver

[Matthew Garrett]

On Tue, Aug 19, 2008 at 11:46:37PM -0700, Andrew Morton wrote:
> > +struct calling_interface_buffer {
> > +	u16 class;
> > +	u16 select;
> > +	volatile u32 input[4];
> > +	volatile u32 output[4];
> > +} __attribute__ ((packed));
> 
> checkpatch will direct you to a fine document.

The address of the structure is passed to the BIOS, and it's then 
modified in system management mode. I think volatile is the right thing 
here? I'll fix up the rest.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/2] Add Dell laptop driver

[Michael E Brown]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

On Sat, Aug 16, 2008 at 09:24:52PM +0100, Matthew Garrett wrote:
> This driver adds backlight and rfkill support for Dell laptops. It uses 
> the DCDBAS driver to trigger the system management calls required for 
> this, and parses the DMI tables itself in order to find the appropriate 
> tokens. In future it should be possible to add LED control, but I don't 
> have any appropriate machines right now for testing.

Sorry, I'm *way* late on giving feedback for this (thanks for the cc). Aside
from the other code review comments, I agree with the approach and am glad
somebody finally took the time to write this correctly.

The i8k driver incorrectly hardcodes magic port io data/address. Can somebody
fix i8k to use the code in this module to do it correctly?
--
Michael Brown
libsmbios maintainer

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]