Add Dell laptop driver
cover Add Dell laptop driver
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.
Signature · 1 line
Matthew Garrett | [email protected]
PATCH 1/2 Export SMI call functionality from dcdbas driver
Rename the dcdbas SMI request call to avoid namespacing issues, and then export it so other kernel modules can make use of the functionality.
---
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
drivers/firmware/dcdbas.c +6 -4
@@ -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 ;
drivers/firmware/dcdbas.h +2 -0
@@ -101,5 +101,7 @@ struct apm_cmd { } __attribute__ ((packed)) parameters; } __attribute__ ((packed)); +int dcdbas_smi_request (struct smi_cmd *smi_cmd); +#endif /* _DCDBAS_H_ */
Signature · 1 line
Matthew Garrett | [email protected]
PATCH 2/2 Add Dell laptop driver
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.
---
commit 472d8eed1891ba440f8e6ffde4787f6308390f7d
Author: Matthew Garrett <mjg59@m6300.(none)> Date: Sat Aug 16 21:12:51 2008 +0100
Add Dell laptop driver
drivers/misc/Kconfig +12 -0
@@ -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
drivers/misc/Makefile +1 -0
@@ -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
drivers/misc/dell-laptop.c new file +356 -0
356 lines not on this page · raw message
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 3 lines · On Sat, 16 Aug 2008 21:30:27 +0100 Matthew Garrett <[email protected]> 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 ?
Signature · 3 lines
If you want to reach me at my work email, use [email protected] For development, discussion and tips for power savings, visit http://www.lesswatts.org
Quoted 1 line · Arjan van de Ven
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.
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 23 lines · On Sat, 16 Aug 2008 21:30:27 +0100 Matthew Garrett <[email protected]> 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 <[email protected]>---
commit
472d8eed1891ba440f8e6ffde4787f6308390f7dAuthor: Matthew Garrett <mjg59@m6300.(none)> Date: Sat Aug 16 21:12:51 2008 +0100Add Dell laptop driver
drivers/misc/Kconfig +1
-0@@ -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
drivers/misc/Kconfig config SGI_GRU_DEBUG · line 488
Quoted 10 lines
drivers/misc/Kconfig +10
-0+ 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.
Quoted 37 lines
drivers/misc/Kconfig +0
-0endif # MISC_DEVICESdrivers/misc/Makefile +1
-0@@ -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.odrivers/misc/dell-laptop.c new file +20
-0@@ -0,0 +1,356 @@ +/* +* Driver for Dell laptop extras +* +* Copyright (c) Red Hat <[email protected]> +* +* 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>
drivers/misc/dell-laptop.c line 30
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+#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.
drivers/misc/dell-laptop.c line 44
Quoted 13 lines
drivers/misc/dell-laptop.c new file +13
-0+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' ...
Quoted 23 lines
drivers/misc/dell-laptop.c new file +23
-0+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" ), + },
drivers/misc/dell-laptop.c line 77
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+ }, + { } +}; + +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().
Quoted 5 lines
drivers/misc/dell-laptop.c new file +5
-0+/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least +6 bytes of entry */ + +if (dm->length <17 ) +return ;
drivers/misc/dell-laptop.c line 93
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+ + 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.
Quoted 20 lines
drivers/misc/dell-laptop.c new file +20
-0+ 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) +{
drivers/misc/dell-laptop.c line 124
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+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.
drivers/misc/dell-laptop.c line 131
Quoted 7 lines
drivers/misc/dell-laptop.c new file +7
-0+{ +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.
drivers/misc/dell-laptop.c line 138
Quoted 6 lines
drivers/misc/dell-laptop.c new file +6
-0+ 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?
drivers/misc/dell-laptop.c line 152
Quoted 13 lines
drivers/misc/dell-laptop.c new file +13
-0+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.
Quoted 14 lines
drivers/misc/dell-laptop.c new file +14
-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); +}
drivers/misc/dell-laptop.c line 177
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+ +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.
drivers/misc/dell-laptop.c line 187
Quoted 9 lines
drivers/misc/dell-laptop.c new file +9
-0+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.
Quoted 12 lines
drivers/misc/dell-laptop.c new file +12
-0+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); +}
drivers/misc/dell-laptop.c line 210
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+ +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
drivers/misc/dell-laptop.c line 216
Quoted 6 lines
drivers/misc/dell-laptop.c new file +6
-0+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)
Quoted 12 lines
drivers/misc/dell-laptop.c new file +12
-0+ 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;
drivers/misc/dell-laptop.c line 238
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+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
drivers/misc/dell-laptop.c line 247
Quoted 8 lines
drivers/misc/dell-laptop.c new file +8
-0+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
drivers/misc/dell-laptop.c line 250
Quoted 2 lines
drivers/misc/dell-laptop.c new file +2
-0+ 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.
Quoted 7 lines
drivers/misc/dell-laptop.c new file +7
-0+if (buffer->input[0 ] ==-1 ) { +kfree (buffer); +return -ENODEV ; + } + +dell_send_request (buffer,1 ,1 ); +dell_send_request (buffer,1 ,2 );
drivers/misc/dell-laptop.c line 268
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+ +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
Quoted 27 lines
drivers/misc/dell-laptop.c new file +27
-0+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 ;
drivers/misc/dell-laptop.c line 305
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-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
Quoted 8 lines
drivers/misc/dell-laptop.c new file +8
-0+ 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);
drivers/misc/dell-laptop.c line 323
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+ +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.
Quoted 8 lines
drivers/misc/dell-laptop.c new file +8
-0+ } + + 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); + } +
drivers/misc/dell-laptop.c line 341
Quoted 10 lines
drivers/misc/dell-laptop.c new file +10
-0+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
Quoted 15 lines
drivers/misc/dell-laptop.c new file +15
-0+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 <[email protected]>" ); +MODULE_DESCRIPTION ("Dell laptop driver" ); +MODULE_LICENSE ("GPL" ); +MODULE_ALIAS ("dmi:*svnDellInc.:*:ct8:*" );
Quoted 8 lines · Andrew Morton
drivers/misc/dell-laptop.c line 30
Quoted 6 lines
drivers/misc/dell-laptop.c new file +6
-0+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.
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 5 lines · 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.
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