Skip to contentSkip to conversation

misc,acpi,backlight: MSI S270 Laptop support

PATCH In review Lennart Poettering 3 participants 6 messages Oct 3, 2006 – Oct 4, 2006

Subsystems (2)
ACPI Char and misc drivers
lore
Share & tools
lore

Lennart Poettering

author
#0 lore raw

PATCH 1/1 misc,acpi,backlight: MSI S270 Laptop support

From: Lennart Poettering <[email protected]>

A driver for the special features of MSI S270 laptops (and maybe other MSI laptops). This driver implements a backlight device for controlling LCD brightness (/sys/class/backlight/msi-laptop-bl/). In addition it allows access to the WLAN and Bluetooth states through a platform driver (/sys/devices/platform/msi-laptop-pf/).

Signed-off-by Lennart Poettering <[email protected]>

---

This is the fourth version of this driver. Changes to the previous version are mostly cosmetics: indentation, removal von Subversion $Id$ and minor changes in the commenting style. These changes were requested by Pavel Machek and Andrew Morton.

(Just for reference, the previous version of this patch is available here: http://lkml.org/lkml/2006/8/20/219)

Applies to Len Brown's current linux-acpi tree.

Len, please merge!

Thanks,
Lennart

 MAINTAINERS               |    7 +
 drivers/acpi/ec.c         |    2 
 drivers/misc/Kconfig      |   19 ++
 drivers/misc/Makefile     |    1 
 drivers/misc/msi-laptop.c |  383 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 412 insertions(+), 0 deletions(-)
MAINTAINERS +7 -0
index 23348c0..0d99de9 100644
@@ -1978,6 +1978,13 @@ M:	[email protected]
 L:	[email protected]
 S:	Maintained
 
+MSI LAPTOP SUPPORT
+P:	Lennart Poettering
+M:	[email protected]
+L:	https://tango.0pointer.de/mailman/listinfo/s270-linux
+W:	http://0pointer.de/lennart/tchibo.html
+S:	Maintained
+
 MTRR AND SIMILAR SUPPORT [i386]
 P:	Richard Gooch
 M:	[email protected]
drivers/acpi/ec.c +2 -0
index ae05e8c..e6d4b08 100644
@@ -384,6 +384,8 @@ extern int ec_transaction(u8 command,
 				   wdata_len, rdata, rdata_len);
 }
 
+EXPORT_SYMBOL(ec_transaction);
+
 static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 {
 	int result;
drivers/misc/Kconfig +19 -0
index 7fc692a..d25b0db 100644
@@ -28,5 +28,24 @@ config IBM_ASM
 
 	  If unsure, say N.
 
+config MSI_LAPTOP
+        tristate "MSI Laptop Extras"
+        depends on X86
+        depends on ACPI_EC
+        depends on BACKLIGHT_CLASS_DEVICE
+        ---help---
+          This is a driver for laptops built by MSI (MICRO-STAR 
+	  INTERNATIONAL):
+	      
+	      MSI MegaBook S270 (MS-1013)
+	      Cytron/TCM/Medion/Tchibo MD96100/SAM2000
+	   
+	  It adds support for Bluetooth, WLAN and LCD brightness control.
+
+          More information about this driver is available at
+          <http://0pointer.de/lennart/tchibo.html>.
+
+          If you have an MSI S270 laptop, say Y or M here.
+
 endmenu
drivers/misc/Makefile +1 -0
index 19c2b85..1328c5f 100644
@@ -5,3 +5,4 @@ obj- := misc.o	# Dummy rule to force bui
 
 obj-$(CONFIG_IBM_ASM)	+= ibmasm/
 obj-$(CONFIG_HDPU_FEATURES)	+= hdpuftrs/
+obj-$(CONFIG_MSI_LAPTOP)     += msi-laptop.o
drivers/misc/msi-laptop.c new file +383 -0

383 lines not on this page · raw message

Signature · 1 line
1.4.1.1
#1 lore raw
Quoted 10 lines · On Tue, 3 Oct 2006 03:10:56 +0200 Lennart Poettering <[email protected]>…

From: Lennart Poettering <[email protected]>

A driver for the special features of MSI S270 laptops (and maybe other MSI laptops). This driver implements a backlight device for controlling LCD brightness (/sys/class/backlight/msi-laptop-bl/). In addition it allows access to the WLAN and Bluetooth states through a platform driver (/sys/devices/platform/msi-laptop-pf/).

...

err, this driver has only about fifteen tab characters in it. The whole thing is using lean-on-the-spacebar instead of hard tabs.

fix, please??

Review

Quoted 2 lines
(quoted diff) +2 -0
+static ssize_t store_lcd_level(
+                struct device *dev, struct device_attribute *attr, const char *buf, size_t count) {

No, kernel-style is

static ssize_t store_lcd_level(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{

or thereabouts.

#2 lore raw
In reply to Lennart Poettering

Hi Lennart,

drivers/misc/msi-laptop.c line 331

Quoted 7 lines · Lennart Poettering
drivers/misc/msi-laptop.c new file +7 -0
+        
+        msipf_device = platform_device_register_simple("msi-laptop-pf", -1, NULL, 0);
+	if (IS_ERR(msipf_device)) {
+		ret = PTR_ERR(msipf_device);
+		goto fail_platform_driver;
+	}
+

Please do not use platform_device_register_simple, use platform_device_alloc and platform_device_add instead (_simple will go away some day).

drivers/misc/msi-laptop.c line 373

Quoted 9 lines
drivers/misc/msi-laptop.c new file +9 -0
+        ret = sysfs_create_group(&msipf_device->dev.kobj, &msipf_attribute_group);
+        if (ret)
+                goto fail_platform_device;
+
+
+        /* Enable automatic brightness control again */
+        if (auto_brightness != 2)
+                set_auto_brightness(1);     
+

What happens if auto_brightness is 2 but userspace messed up with it through device's sysfs attribute? Overall brightness controll interface (module vs. per-device) needs to be tightened up.

Signature · 1 line
Dmitry
author
#3 lore raw
Quoted 4 lines · Dmitry Torokhov
Quoted 4 lines
drivers/misc/msi-laptop.c new file +4 -0
+        ret = sysfs_create_group(&msipf_device->dev.kobj, &msipf_attribute_group);
+        if (ret)
+                goto fail_platform_device;
+
Quoted 8 lines

drivers/misc/msi-laptop.c line 373

Quoted 5 lines
drivers/misc/msi-laptop.c new file +5 -0
+
+        /* Enable automatic brightness control again */
+        if (auto_brightness != 2)
+                set_auto_brightness(1);     
+

What happens if auto_brightness is 2 but userspace messed up with it through device's sysfs attribute?

If auto_brightness is 2 we assume that the user doesn't want the module to fiddle with the automatic brightness control automatically. So we don't do it, neither when loading nor when unloading the module. However, if the user wants to fiddle with the setting through sysfs he may do so and we will not reset his changes when unloading the module. This allows the user to do something like this to disable the brightness control without having the the driver loaded the whole time:

  modprobe msi-laptop auto_brightness=2 && echo 0 > /sys/devices/platform/msi-laptop-pf/auto_brightness && modprobe -r msi-laptop

If auto_brightness is 1 or 0, we do as requested but reset the control to the bootup default when unloading. (i.e. enable it again)

Sounds like a reasonable policy to me, doesn't it to you?

Quoted 2 lines

Overall brightness controll interface (module vs. per-device) needs to be tightened up.

Hmm, I am sorry? I don't understand what you mean?

There's only a single device of this type available in a system at maximum. Hence "per-device" and "per-module" is mostly the same here.

Lennart

Signature · 2 lines
Lennart Poettering; lennart [at] poettering [dot] net
ICQ# 11060553; GPG 0x1A015CC4; http://0pointer.net/lennart/
#4 lore raw
Quoted 24 lines · Lennart Poettering
Quoted 4 lines · Dmitry Torokhov
Quoted 4 lines
drivers/misc/msi-laptop.c new file +4 -0
+        ret = sysfs_create_group(&msipf_device->dev.kobj, &msipf_attribute_group);
+        if (ret)
+                goto fail_platform_device;
+
Quoted 8 lines

drivers/misc/msi-laptop.c line 373

Quoted 5 lines
drivers/misc/msi-laptop.c new file +5 -0
+
+        /* Enable automatic brightness control again */
+        if (auto_brightness != 2)
+                set_auto_brightness(1);     
+

What happens if auto_brightness is 2 but userspace messed up with it through device's sysfs attribute?

If auto_brightness is 2 we assume that the user doesn't want the module to fiddle with the automatic brightness control automatically. So we don't do it, neither when loading nor when unloading the module. However, if the user wants to fiddle with the setting through sysfs he may do so and we will not reset his changes when unloading the module. This allows the user to do something like this to disable the brightness control without having the the driver loaded the whole time:

Quoted 4 lines
  modprobe msi-laptop auto_brightness=2 && echo 0 > /sys/devices/platform/msi-laptop-pf/auto_brightness && modprobe -r msi-laptop

If auto_brightness is 1 or 0, we do as requested but reset the control to the bootup default when unloading. (i.e. enable it again)

Normally drivers clean up after themselves as if they were never loaded, taht is why I questioned partial cleanup.

Signature · 1 line
Dmitry
author
#5 lore raw
Quoted 9 lines · Dmitry Torokhov
Quoted 9 lines

If auto_brightness is 2 we assume that the user doesn't want the module to fiddle with the automatic brightness control automatically. So we don't do it, neither when loading nor when unloading the module. However, if the user wants to fiddle with the setting through sysfs he may do so and we will not reset his changes when unloading the module. This allows the user to do something like this to disable the brightness control without having the the driver loaded the whole time:

Quoted 7 lines
Quoted 4 lines
  modprobe msi-laptop auto_brightness=2 && echo 0 > /sys/devices/platform/msi-laptop-pf/auto_brightness && modprobe -r msi-laptop

If auto_brightness is 1 or 0, we do as requested but reset the control to the bootup default when unloading. (i.e. enable it again)

Normally drivers clean up after themselves as if they were never loaded, taht is why I questioned partial cleanup.

Sure. But "normally" the user wouldn't bother to pass auto_brightness=2 to the module in which case we *do* "clean up" after ourselves and reenable automatic brightness control on module unload.

The special behaviour when auto_brightness=2 is passed is merely a special feature for those who need it. It was useful to me and was trivial to implement, and hence I did it.

Lennart

Signature · 2 lines
Lennart Poettering; lennart [at] poettering [dot] net
ICQ# 11060553; GPG 0x1A015CC4; http://0pointer.net/lennart/