Re: [PATCH 2/2] misc: Add dell-laptop driver

Re: [PATCH 2/2] misc: Add dell-laptop driver

[Andrew Morton]

On Thu, 27 Nov 2008 16:34:44 +0000
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> Add a driver for controling Dell-specific backlight and rfkill interfaces.
> This driver makes use of the dcdbas interface to the Dell firmware to allow
> the backlight and rfkill interfaces on Dell systems to be driven through the
> standardised sysfs interfaces.
>     
>
> ...
>
> +struct calling_interface_buffer {
> +	u16 class;
> +	u16 select;
> +	volatile u32 input[4];
> +	volatile u32 output[4];
> +} __attribute__ ((packed));

We have that little __packed helper for this.

I don't think it adds a ton of value personally, but the __attribute__
syntax is a bit of a mouthful, and consistency is nice.  I've suggested
that Andy add a checkpatch rule for open-coded use of the various
helpers which are defined in include/linux/compiler-gcc.h.

> +struct calling_interface_token {
> +	u16 tokenID;
> +	u16 location;
> +	union {
> +		u16 value;
> +		u16 stringlength;
> +	};
> +};
> +
> +struct calling_interface_structure {
> +	struct dmi_header header;
> +	u16 cmdIOAddress;
> +	u8 cmdIOCode;
> +	u32 supportedCmds;
> +	struct calling_interface_token tokens[];
> +} __attribute__ ((packed));

ditto.

> +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[] = {

this can be made const.

> +	{
> +		.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;

lol.

> +	struct calling_interface_structure *table =
> +		container_of(dm, struct calling_interface_structure, header);
> +
> +	/* 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);
> +
> +	if (!da_tokens)
> +		return;
> +
> +	memcpy(da_tokens+da_num_tokens, table->tokens,
> +	       sizeof(struct calling_interface_token) * tokens);
> +
> +	da_num_tokens += tokens;
> +}

OK.  No locking is needed for updates to the global state?

>
> ...
>
> +static int __init dell_init(void)
> +{
> +	struct calling_interface_buffer buffer;
> +	int max_intensity = 0;
> +	int ret;
> +
> +	if (!dmi_check_system(dell_device_table))
> +		return -ENODEV;
> +
> +	dmi_walk(find_tokens);
> +
> +	if (!da_tokens)
> +		return -ENODEV;
> +
> +	ret = dell_setup_rfkill();
> +
> +	if (ret) {
> +		printk(KERN_WARNING "dell-laptop: Unable to setup rfkill\n");
> +		return ret;
> +	}
> +
> +#ifdef CONFIG_ACPI
> +	if (acpi_video_backlight_support())
> +		return 0;
> +#endif

Do we need the ifdefs here?  It looks like include/linux/acpi.h tries
to provide a suitable 0-returning stub?

If the ifdefs are needed, then perhaps include/linux/acpi.h needs fixing?

> +	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
> +	buffer.input[0] = find_token_location(BRIGHTNESS_TOKEN);
> +
> +	if (buffer.input[0] != -1) {
> +		dell_send_request(&buffer, 0, 2);
> +		max_intensity = buffer.output[3];
> +	}
> +
> +	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;
> +			ret = PTR_ERR(dell_backlight_device);

The above two statements are in the wrong order.

> +			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);
> +	}
> +
> +	return 0;
> +out:
> +	if (wifi_rfkill)
> +		rfkill_unregister(wifi_rfkill);
> +	if (bluetooth_rfkill)
> +		rfkill_unregister(bluetooth_rfkill);
> +	if (wwan_rfkill)
> +		rfkill_unregister(wwan_rfkill);
> +	return ret;
> +}
> +

Re: [PATCH 2/2] misc: Add dell-laptop driver

[Matthew Garrett]

On Tue, Dec 02, 2008 at 11:50:29AM -0800, Andrew Morton wrote:
> > +struct calling_interface_buffer {
> > +	u16 class;
> > +	u16 select;
> > +	volatile u32 input[4];
> > +	volatile u32 output[4];
> > +} __attribute__ ((packed));
> 
> We have that little __packed helper for this.

Ok. I've switched to that, although the __attribute__ notation seems to 
be the dominant form in the kernel right now.

> > +static struct dmi_system_id __initdata dell_device_table[] = {
> 
> this can be made const.

Done.

> > +	da_num_tokens += tokens;
> > +}
> 
> OK.  No locking is needed for updates to the global state?

The parse function is called once during module init, before the driver 
is registered. On the other hand, that makes me realise that we leak it 
if something else fails, so I've fixed that up as well.

> > +#ifdef CONFIG_ACPI
> > +	if (acpi_video_backlight_support())
> > +		return 0;
> > +#endif
> 
> Do we need the ifdefs here?  It looks like include/linux/acpi.h tries
> to provide a suitable 0-returning stub?

They're protected by #ifdef CONFIG_ACPI_VIDEO, but it looks like acpi.h 
is empty if CONFIG_ACPI isn't set?

> > +			dell_backlight_device = NULL;
> > +			ret = PTR_ERR(dell_backlight_device);
> 
> The above two statements are in the wrong order.

Good catch, thanks.

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

Re: [PATCH 2/2] misc: Add dell-laptop driver

[Andrew Morton]

On Tue, 2 Dec 2008 20:05:45 +0000
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> > > +#ifdef CONFIG_ACPI
> > > +	if (acpi_video_backlight_support())
> > > +		return 0;
> > > +#endif
> > 
> > Do we need the ifdefs here?  It looks like include/linux/acpi.h tries
> > to provide a suitable 0-returning stub?
> 
> They're protected by #ifdef CONFIG_ACPI_VIDEO, but it looks like acpi.h 
> is empty if CONFIG_ACPI isn't set?

No, acpi.h has a _few_ ACPI=n stubs:

: static inline int early_acpi_boot_init(void)
: {
: 	return 0;
: }
: static inline int acpi_boot_init(void)
: {
: 	return 0;
: }
: 
: static inline int acpi_boot_table_init(void)
: {
: 	return 0;
: }
: 
: static inline int acpi_mps_check(void)
: {
: 	return 0;
: }
: 
: static inline int acpi_check_resource_conflict(struct resource *res)
: {
: 	return 0;
: }
: 
: static inline int acpi_check_region(resource_size_t start, resource_size_t n,
: 				    const char *name)
: {
: 	return 0;
: }
: 
: static inline int acpi_check_mem_region(resource_size_t start,
: 					resource_size_t n, const char *name)
: {
: 	return 0;
: }

So it seems that acpi_video_backlight_support() should be added to this
list.  Except it already has a stub, in the wrong place.  This whole block:

: #if defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE)
: 
: extern long acpi_video_get_capabilities(acpi_handle graphics_dev_handle);
: extern long acpi_is_video_device(struct acpi_device *device);
: extern int acpi_video_backlight_support(void);
: extern int acpi_video_display_switch_support(void);
: 
: #else
: 
: static inline long acpi_video_get_capabilities(acpi_handle graphics_dev_handle)
: {
: 	return 0;
: }
: 
: static inline long acpi_is_video_device(struct acpi_device *device)
: {
: 	return 0;
: }
: 
: static inline int acpi_video_backlight_support(void)
: {
: 	return 0;
: }
: 
: static inline int acpi_video_display_switch_support(void)
: {
: 	return 0;
: }
: 
: #endif /* defined(CONFIG_ACPI_VIDEO) || defined(CONFIG_ACPI_VIDEO_MODULE) */
: 

needs to be moved outside the `#ifdef CONFIG_ACPI' altogether.  Then you
can drop the ifdef.

Re: [PATCH 2/2] misc: Add dell-laptop driver

[Matthew Garrett]

On Tue, Dec 02, 2008 at 12:22:22PM -0800, Andrew Morton wrote:

> No, acpi.h has a _few_ ACPI=n stubs:

Ah, yes. I'll send a patch for that to Len.

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

[PATCH 2/2 update] misc: Add dell-laptop driver

[Matthew Garrett]

misc: Add dell-laptop driver
    
Add a driver for controling Dell-specific backlight and rfkill interfaces.
This driver makes use of the dcdbas interface to the Dell firmware to allow
the backlight and rfkill interfaces on Dell systems to be driven through the
standardised sysfs interfaces.
    
Signed-off-by: Matthew Garrett <mjg@redhat.com>

---

Contains Andrew's suggested updates, plus a fix for a leak on the init 
error path.

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index fee7304..3949a1c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -498,6 +498,18 @@ 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.
+
 source "drivers/misc/c2port/Kconfig"
 
 endif # MISC_DEVICES
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 817f7f5..39043f8 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -32,4 +32,5 @@ 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
 obj-$(CONFIG_C2PORT)		+= c2port/
diff --git a/drivers/misc/dell-laptop.c b/drivers/misc/dell-laptop.c
new file mode 100644
index 0000000..4d33a20
--- /dev/null
+++ b/drivers/misc/dell-laptop.c
@@ -0,0 +1,436 @@
+/*
+ *  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 <linux/power_supply.h>
+#include <linux/acpi.h>
+#include "../firmware/dcdbas.h"
+
+#define BRIGHTNESS_TOKEN 0x7d
+
+/* This structure will be modified by the firmware when we enter
+ * system management mode, hence the volatiles */
+
+struct calling_interface_buffer {
+	u16 class;
+	u16 select;
+	volatile u32 input[4];
+	volatile u32 output[4];
+} __packed;
+
+struct calling_interface_token {
+	u16 tokenID;
+	u16 location;
+	union {
+		u16 value;
+		u16 stringlength;
+	};
+};
+
+struct calling_interface_structure {
+	struct dmi_header header;
+	u16 cmdIOAddress;
+	u8 cmdIOCode;
+	u32 supportedCmds;
+	struct calling_interface_token tokens[];
+} __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 const 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 =
+		container_of(dm, struct calling_interface_structure, header);
+
+	/* 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);
+
+	if (!da_tokens)
+		return;
+
+	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;
+
+	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);
+
+	return buffer;
+}
+
+/* Derived from information in DellWirelessCtl.cpp:
+   Class 17, select 11 is radio control. It returns an array of 32-bit values.
+
+   result[0]: return code
+   result[1]:
+     Bit 0:      Hardware switch supported
+     Bit 1:      Wifi locator supported
+     Bit 2:      Wifi is supported
+     Bit 3:      Bluetooth is supported
+     Bit 4:      WWAN is supported
+     Bit 5:      Wireless keyboard supported
+     Bits 6-7:   Reserved
+     Bit 8:      Wifi is installed
+     Bit 9:      Bluetooth is installed
+     Bit 10:     WWAN is installed
+     Bits 11-15: Reserved
+     Bit 16:     Hardware switch is on
+     Bit 17:     Wifi is blocked
+     Bit 18:     Bluetooth is blocked
+     Bit 19:     WWAN is blocked
+     Bits 20-31: Reserved
+   result[2]: NVRAM size in bytes
+   result[3]: NVRAM format version number
+*/
+
+static int dell_rfkill_set(int radio, enum rfkill_state state)
+{
+	struct calling_interface_buffer buffer;
+	int disable = (state == RFKILL_STATE_UNBLOCKED) ? 0 : 1;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer.input[0] = (1 | (radio<<8) | (disable << 16));
+	dell_send_request(&buffer, 17, 11);
+
+	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;
+	int status;
+	int new_state = RFKILL_STATE_HARD_BLOCKED;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	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;
+
+	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 int dell_setup_rfkill(void)
+{
+	struct calling_interface_buffer buffer;
+	int status;
+	int ret;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	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);
+		if (!wifi_rfkill)
+			goto err_wifi;
+		wifi_rfkill->name = "dell-wifi";
+		wifi_rfkill->toggle_radio = dell_wifi_set;
+		wifi_rfkill->get_state = dell_wifi_get;
+		ret = rfkill_register(wifi_rfkill);
+		if (ret)
+			goto err_wifi;
+	}
+
+	if ((status & (1<<3|1<<9)) == (1<<3|1<<9)) {
+		bluetooth_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_BLUETOOTH);
+		if (!bluetooth_rfkill)
+			goto err_bluetooth;
+		bluetooth_rfkill->name = "dell-bluetooth";
+		bluetooth_rfkill->toggle_radio = dell_bluetooth_set;
+		bluetooth_rfkill->get_state = dell_bluetooth_get;
+		ret = rfkill_register(bluetooth_rfkill);
+		if (ret)
+			goto err_bluetooth;
+	}
+
+	if ((status & (1<<4|1<<10)) == (1<<4|1<<10)) {
+		wwan_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_WWAN);
+		if (!wwan_rfkill)
+			goto err_wwan;
+		wwan_rfkill->name = "dell-wwan";
+		wwan_rfkill->toggle_radio = dell_wwan_set;
+		wwan_rfkill->get_state = dell_wwan_get;
+		ret = rfkill_register(wwan_rfkill);
+		if (ret)
+			goto err_wwan;
+	}
+
+	return 0;
+err_wwan:
+	if (wwan_rfkill)
+		rfkill_free(wwan_rfkill);
+	if (bluetooth_rfkill) {
+		rfkill_unregister(bluetooth_rfkill);
+		bluetooth_rfkill = NULL;
+	}
+err_bluetooth:
+	if (bluetooth_rfkill)
+		rfkill_free(bluetooth_rfkill);
+	if (wifi_rfkill) {
+		rfkill_unregister(wifi_rfkill);
+		wifi_rfkill = NULL;
+	}
+err_wifi:
+	if (wifi_rfkill)
+		rfkill_free(wifi_rfkill);
+
+	return ret;
+}
+
+static int dell_send_intensity(struct backlight_device *bd)
+{
+	struct calling_interface_buffer buffer;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer.input[0] = find_token_location(BRIGHTNESS_TOKEN);
+	buffer.input[1] = bd->props.brightness;
+
+	if (buffer.input[0] == -1)
+		return -ENODEV;
+
+	if (power_supply_is_system_supplied() > 0)
+		dell_send_request(&buffer, 1, 2);
+	else
+		dell_send_request(&buffer, 1, 1);
+
+	return 0;
+}
+
+static int dell_get_intensity(struct backlight_device *bd)
+{
+	struct calling_interface_buffer buffer;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer.input[0] = find_token_location(BRIGHTNESS_TOKEN);
+
+	if (buffer.input[0] == -1)
+		return -ENODEV;
+
+	if (power_supply_is_system_supplied() > 0)
+		dell_send_request(&buffer, 0, 2);
+	else
+		dell_send_request(&buffer, 0, 1);
+
+	return buffer.output[1];
+}
+
+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;
+	int ret;
+
+	if (!dmi_check_system(dell_device_table))
+		return -ENODEV;
+
+	dmi_walk(find_tokens);
+
+	if (!da_tokens)  {
+		printk(KERN_INFO "dell-laptop: Unable to find dmi tokens\n");
+		return -ENODEV;
+	}
+
+	ret = dell_setup_rfkill();
+
+	if (ret) {
+		printk(KERN_WARNING "dell-laptop: Unable to setup rfkill\n");
+		goto out;
+	}
+
+#ifdef CONFIG_ACPI
+	/* In the event of an ACPI backlight being available, don't
+	 * register the platform controller.
+	 */
+	if (acpi_video_backlight_support())
+		return 0;
+#endif
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer.input[0] = find_token_location(BRIGHTNESS_TOKEN);
+
+	if (buffer.input[0] != -1) {
+		dell_send_request(&buffer, 0, 2);
+		max_intensity = buffer.output[3];
+	}
+
+	if (max_intensity) {
+		dell_backlight_device = backlight_device_register(
+			"dell_backlight",
+			NULL, NULL,
+			&dell_ops);
+
+		if (IS_ERR(dell_backlight_device)) {
+			ret = PTR_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);
+	}
+
+	return 0;
+out:
+	if (wifi_rfkill)
+		rfkill_unregister(wifi_rfkill);
+	if (bluetooth_rfkill)
+		rfkill_unregister(bluetooth_rfkill);
+	if (wwan_rfkill)
+		rfkill_unregister(wwan_rfkill);
+	kfree(da_tokens);
+	return ret;
+}
+
+static void __exit dell_exit(void)
+{
+	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 update] misc: Add dell-laptop driver

[Alan Jenkins]

Matthew Garrett wrote:
> misc: Add dell-laptop driver
>     
> Add a driver for controling Dell-specific backlight and rfkill interfaces.
> This driver makes use of the dcdbas interface to the Dell firmware to allow
> the backlight and rfkill interfaces on Dell systems to be driven through the
> standardised sysfs interfaces.
>     
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
>
> ---
>
> Contains Andrew's suggested updates, plus a fix for a leak on the init 
> error path.
>   

I see you don't call rfkill_set_default(). Do Dell firmwares support
persistent rfkill state?

My ill-informed guess is that it shouldn't hurt to call it. If the
firmware is sane, it will initialize the rfkill to a useful state.
Otherwise, the radio(s) would be unusable without the platform driver.
But do feel free to ignore me, if you suspect the firmware of insanity :).

Regards
Alan

Re: [PATCH 2/2 update] misc: Add dell-laptop driver

[none]

Matthew Garrett wrote:
> misc: Add dell-laptop driver
>     
> Add a driver for controling Dell-specific backlight and rfkill interfaces.
> This driver makes use of the dcdbas interface to the Dell firmware to allow
> the backlight and rfkill interfaces on Dell systems to be driven through the
> standardised sysfs interfaces.
>     
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
>
> ---
>
> Contains Andrew's suggested updates, plus a fix for a leak on the init 
> error path.
>   

Sorry, I should have mentioned this in my first message:

> +	if ((status & (1<<2|1<<8)) == (1<<2|1<<8)) {
> +		wifi_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_WLAN);
> +		if (!wifi_rfkill)
> +			goto err_wifi;
> +		wifi_rfkill->name = "dell-wifi";
> +		wifi_rfkill->toggle_radio = dell_wifi_set;
> +		wifi_rfkill->get_state = dell_wifi_get;
>   

The rfkill doc (and code) say that you also need to initialize
wifi_rfkill->state to the current state of the hardware.

> +		ret = rfkill_register(wifi_rfkill);
> +		if (ret)
> +			goto err_wifi;
> +	}
> +

Regards
Alan

Re: [PATCH 2/2 update] misc: Add dell-laptop driver

[Henrique de Moraes Holschuh]

(I feel weird replying to "none" :-) )

On Wed, 03 Dec 2008, none wrote:
> > +		wifi_rfkill->get_state = dell_wifi_get;
> 
> The rfkill doc (and code) say that you also need to initialize
> wifi_rfkill->state to the current state of the hardware.

I may have screwed up and ommited from the docs the fact that it is not
strictly necessary to do so if you provide rfkill->get_state.  So that is
not a serious problem with the patch at all.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 2/2 update] misc: Add dell-laptop driver

[Len Brown]

I didn't notice a MAINTAINER entry for this new driver.

-- 
-Len Brown
Intel Open Source Technology Center

Re: [PATCH 2/2 update] misc: Add dell-laptop driver

[Matthew Garrett]

Add a driver for controling Dell-specific backlight and rfkill 
interfaces. This driver makes use of the dcdbas interface to the Dell 
firmware to allow the backlight and rfkill interfaces on Dell systems to 
be driven through the standardised sysfs interfaces.
    
Signed-off-by: Matthew Garrett <mjg@redhat.com>

---

Includes a MAINTAINERS entry.

diff --git a/MAINTAINERS b/MAINTAINERS
index 618c1ef..2de9dca 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1360,6 +1360,11 @@ P:	Maciej W. Rozycki
 M:	macro@linux-mips.org
 S:	Maintained
 
+DELL LAPTOP DRIVER
+P:	Matthew Garrett
+M:	mjg59@srcf.ucam.org
+S:	Maintained
+
 DELL LAPTOP SMM DRIVER
 P:	Massimo Dal Zotto
 M:	dz@debian.org
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index fee7304..3949a1c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -498,6 +498,18 @@ 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.
+
 source "drivers/misc/c2port/Kconfig"
 
 endif # MISC_DEVICES
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 817f7f5..39043f8 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -32,4 +32,5 @@ 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
 obj-$(CONFIG_C2PORT)		+= c2port/
diff --git a/drivers/misc/dell-laptop.c b/drivers/misc/dell-laptop.c
new file mode 100644
index 0000000..4d33a20
--- /dev/null
+++ b/drivers/misc/dell-laptop.c
@@ -0,0 +1,436 @@
+/*
+ *  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 <linux/power_supply.h>
+#include <linux/acpi.h>
+#include "../firmware/dcdbas.h"
+
+#define BRIGHTNESS_TOKEN 0x7d
+
+/* This structure will be modified by the firmware when we enter
+ * system management mode, hence the volatiles */
+
+struct calling_interface_buffer {
+	u16 class;
+	u16 select;
+	volatile u32 input[4];
+	volatile u32 output[4];
+} __packed;
+
+struct calling_interface_token {
+	u16 tokenID;
+	u16 location;
+	union {
+		u16 value;
+		u16 stringlength;
+	};
+};
+
+struct calling_interface_structure {
+	struct dmi_header header;
+	u16 cmdIOAddress;
+	u8 cmdIOCode;
+	u32 supportedCmds;
+	struct calling_interface_token tokens[];
+} __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 const 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 =
+		container_of(dm, struct calling_interface_structure, header);
+
+	/* 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);
+
+	if (!da_tokens)
+		return;
+
+	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;
+
+	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);
+
+	return buffer;
+}
+
+/* Derived from information in DellWirelessCtl.cpp:
+   Class 17, select 11 is radio control. It returns an array of 32-bit values.
+
+   result[0]: return code
+   result[1]:
+     Bit 0:      Hardware switch supported
+     Bit 1:      Wifi locator supported
+     Bit 2:      Wifi is supported
+     Bit 3:      Bluetooth is supported
+     Bit 4:      WWAN is supported
+     Bit 5:      Wireless keyboard supported
+     Bits 6-7:   Reserved
+     Bit 8:      Wifi is installed
+     Bit 9:      Bluetooth is installed
+     Bit 10:     WWAN is installed
+     Bits 11-15: Reserved
+     Bit 16:     Hardware switch is on
+     Bit 17:     Wifi is blocked
+     Bit 18:     Bluetooth is blocked
+     Bit 19:     WWAN is blocked
+     Bits 20-31: Reserved
+   result[2]: NVRAM size in bytes
+   result[3]: NVRAM format version number
+*/
+
+static int dell_rfkill_set(int radio, enum rfkill_state state)
+{
+	struct calling_interface_buffer buffer;
+	int disable = (state == RFKILL_STATE_UNBLOCKED) ? 0 : 1;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer.input[0] = (1 | (radio<<8) | (disable << 16));
+	dell_send_request(&buffer, 17, 11);
+
+	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;
+	int status;
+	int new_state = RFKILL_STATE_HARD_BLOCKED;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	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;
+
+	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 int dell_setup_rfkill(void)
+{
+	struct calling_interface_buffer buffer;
+	int status;
+	int ret;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	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);
+		if (!wifi_rfkill)
+			goto err_wifi;
+		wifi_rfkill->name = "dell-wifi";
+		wifi_rfkill->toggle_radio = dell_wifi_set;
+		wifi_rfkill->get_state = dell_wifi_get;
+		ret = rfkill_register(wifi_rfkill);
+		if (ret)
+			goto err_wifi;
+	}
+
+	if ((status & (1<<3|1<<9)) == (1<<3|1<<9)) {
+		bluetooth_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_BLUETOOTH);
+		if (!bluetooth_rfkill)
+			goto err_bluetooth;
+		bluetooth_rfkill->name = "dell-bluetooth";
+		bluetooth_rfkill->toggle_radio = dell_bluetooth_set;
+		bluetooth_rfkill->get_state = dell_bluetooth_get;
+		ret = rfkill_register(bluetooth_rfkill);
+		if (ret)
+			goto err_bluetooth;
+	}
+
+	if ((status & (1<<4|1<<10)) == (1<<4|1<<10)) {
+		wwan_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_WWAN);
+		if (!wwan_rfkill)
+			goto err_wwan;
+		wwan_rfkill->name = "dell-wwan";
+		wwan_rfkill->toggle_radio = dell_wwan_set;
+		wwan_rfkill->get_state = dell_wwan_get;
+		ret = rfkill_register(wwan_rfkill);
+		if (ret)
+			goto err_wwan;
+	}
+
+	return 0;
+err_wwan:
+	if (wwan_rfkill)
+		rfkill_free(wwan_rfkill);
+	if (bluetooth_rfkill) {
+		rfkill_unregister(bluetooth_rfkill);
+		bluetooth_rfkill = NULL;
+	}
+err_bluetooth:
+	if (bluetooth_rfkill)
+		rfkill_free(bluetooth_rfkill);
+	if (wifi_rfkill) {
+		rfkill_unregister(wifi_rfkill);
+		wifi_rfkill = NULL;
+	}
+err_wifi:
+	if (wifi_rfkill)
+		rfkill_free(wifi_rfkill);
+
+	return ret;
+}
+
+static int dell_send_intensity(struct backlight_device *bd)
+{
+	struct calling_interface_buffer buffer;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer.input[0] = find_token_location(BRIGHTNESS_TOKEN);
+	buffer.input[1] = bd->props.brightness;
+
+	if (buffer.input[0] == -1)
+		return -ENODEV;
+
+	if (power_supply_is_system_supplied() > 0)
+		dell_send_request(&buffer, 1, 2);
+	else
+		dell_send_request(&buffer, 1, 1);
+
+	return 0;
+}
+
+static int dell_get_intensity(struct backlight_device *bd)
+{
+	struct calling_interface_buffer buffer;
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer.input[0] = find_token_location(BRIGHTNESS_TOKEN);
+
+	if (buffer.input[0] == -1)
+		return -ENODEV;
+
+	if (power_supply_is_system_supplied() > 0)
+		dell_send_request(&buffer, 0, 2);
+	else
+		dell_send_request(&buffer, 0, 1);
+
+	return buffer.output[1];
+}
+
+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;
+	int ret;
+
+	if (!dmi_check_system(dell_device_table))
+		return -ENODEV;
+
+	dmi_walk(find_tokens);
+
+	if (!da_tokens)  {
+		printk(KERN_INFO "dell-laptop: Unable to find dmi tokens\n");
+		return -ENODEV;
+	}
+
+	ret = dell_setup_rfkill();
+
+	if (ret) {
+		printk(KERN_WARNING "dell-laptop: Unable to setup rfkill\n");
+		goto out;
+	}
+
+#ifdef CONFIG_ACPI
+	/* In the event of an ACPI backlight being available, don't
+	 * register the platform controller.
+	 */
+	if (acpi_video_backlight_support())
+		return 0;
+#endif
+
+	memset(&buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer.input[0] = find_token_location(BRIGHTNESS_TOKEN);
+
+	if (buffer.input[0] != -1) {
+		dell_send_request(&buffer, 0, 2);
+		max_intensity = buffer.output[3];
+	}
+
+	if (max_intensity) {
+		dell_backlight_device = backlight_device_register(
+			"dell_backlight",
+			NULL, NULL,
+			&dell_ops);
+
+		if (IS_ERR(dell_backlight_device)) {
+			ret = PTR_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);
+	}
+
+	return 0;
+out:
+	if (wifi_rfkill)
+		rfkill_unregister(wifi_rfkill);
+	if (bluetooth_rfkill)
+		rfkill_unregister(bluetooth_rfkill);
+	if (wwan_rfkill)
+		rfkill_unregister(wwan_rfkill);
+	kfree(da_tokens);
+	return ret;
+}
+
+static void __exit dell_exit(void)
+{
+	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 update] misc: Add dell-laptop driver

[Sven Wegener]

On Wed, 3 Dec 2008, Matthew Garrett wrote:

> Add a driver for controling Dell-specific backlight and rfkill 
> interfaces. This driver makes use of the dcdbas interface to the Dell 
> firmware to allow the backlight and rfkill interfaces on Dell systems to 
> be driven through the standardised sysfs interfaces.

> +static void parse_da_table(const struct dmi_header *dm)

__init

> +static void find_tokens(const struct dmi_header *dm)

__init

> +{
> +	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;
> +	}

This could be a simple if (dm->type == 0xda), it looks like the switch
is abused as documentation.

> +/* Derived from information in DellWirelessCtl.cpp:
> +   Class 17, select 11 is radio control. It returns an array of 32-bit values.
> +
> +   result[0]: return code
> +   result[1]:
> +     Bit 0:      Hardware switch supported
> +     Bit 1:      Wifi locator supported
> +     Bit 2:      Wifi is supported
> +     Bit 3:      Bluetooth is supported
> +     Bit 4:      WWAN is supported
> +     Bit 5:      Wireless keyboard supported
> +     Bits 6-7:   Reserved
> +     Bit 8:      Wifi is installed
> +     Bit 9:      Bluetooth is installed
> +     Bit 10:     WWAN is installed
> +     Bits 11-15: Reserved
> +     Bit 16:     Hardware switch is on
> +     Bit 17:     Wifi is blocked
> +     Bit 18:     Bluetooth is blocked
> +     Bit 19:     WWAN is blocked
> +     Bits 20-31: Reserved
> +   result[2]: NVRAM size in bytes
> +   result[3]: NVRAM format version number

Could these bit numbers be defines with declarative names, e.g define
DELL_WIFI_BLOCKED (1<<17)? It would make reading the code that just uses
plain numbers easier.

> +	memset(&buffer, 0, sizeof(struct calling_interface_buffer));

sizeof(buffer)

Likewise for other sizeof() occurrences that follow.

> +static int dell_setup_rfkill(void)

__init

Sven

Re: [PATCH 2/2 update] misc: Add dell-laptop driver

[Alan Jenkins]

I still think you need to initialise wifi_rfkill->state before you call
rfkill_register().

It seems an easy mistake, probably the rfkill core should test for it.

Alan

(Sorry about the insane From: header in my previous mail)

Re: [PATCH 2/2 update] misc: Add dell-laptop driver

[Henrique de Moraes Holschuh]

On Thu, 04 Dec 2008, Alan Jenkins wrote:
> I still think you need to initialise wifi_rfkill->state before you call
> rfkill_register().
> 
> It seems an easy mistake, probably the rfkill core should test for it.

Well, yes, I could add that paranoia to the core.  Added to the todo
list.

But the core will re-init rfkill->state from rfkill->get_state(), so
it is not a serious problem at all (since the drive provides
rfkill->get_state().

The reason it is recommended to always init the thing, is that if you
later move to a purely event-driven rfkill interface, you won't break
things.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh