Em Wed, 1 Dec 2010 21:05:14 +0100 Corentin Chary <[email protected]> escreveu: > On Wed, Dec 1, 2010 at 5:14 PM, Herton Ronaldo Krzesinski > <[email protected]> wrote: > > Hi, > > > > I cut the message just to the comments, here we go: > > > > On Wed, 1 Dec 2010 00:22:03 +0100 > > Corentin Chary <[email protected]> wrote: > >> On Tue, Nov 30, 2010 at 6:27 PM, Herton Ronaldo Krzesinski > >> <[email protected]> wrote: > >> > +What: /sys/devices/platform/shuttle_wmi/brightness_up > >> > +Date: November 2010 > >> > +KernelVersion: 2.6.37 > >> > +Contact: "Herton Ronaldo Krzesinski" > >> > <[email protected]> +Description: > >> > + This is a write only option (accepts any single > >> > value, eg. > >> > + "echo 1 > brightness_up") that is equivalent of > >> > pressing > >> > + fn+<brightness up> function on notebooks. This > >> > option exists > >> > + because of shuttle machines that are notebooks > >> > in desktop form > >> > + factor, and which don't have the notebook > >> > keyboard, thus no > >> > + way to use fn+<brightness up>. > >> > >> Why such files ? Can't we do the same using the backlight device ? > > > > I use the backlight device, but also added this sysfs attribute. > > The reason for it was testing, and also because the acpi video > > interface on Shuttle DA18IE has a bug, if you try to set the > > brightness it writes the value but brightness isn't updated on the > > screen. That's why also I had to made the following workaround on > > backlight support without acpi video on the driver: > > > > /* change brightness by steps, this is a quirk for shuttle > > * machines which don't accept direct write to ec for this */ > > if (wmi_ec_cmd(CMD_READEC, 0, 0, ECRAM_ER0, &val)) > > return -EIO; > > val &= 0x7; > > steps = bd->props.brightness - val; > > while (steps > 0) { > > wmi_ec_cmd(CMD_SCMD, 0, 0, 0x0c, NULL); > > steps--; > > } > > while (steps < 0) { > > wmi_ec_cmd(CMD_SCMD, 0, 0, 0x0b, NULL); > > steps++; > > } > > > > That is, on shuttle DA18IE, the brightness will only be effectively > > set if you send the command with same code as fn+<brightness>, it > > doesn't work writing to address 0x79 directly in EC as the firmware > > and my backlight code does for the other machines. So, if you load > > the module with acpi video support on, you can't change the > > brightness without this. If there is an way that for Shuttle DA18IE > > I could disable acpi video support and always only use the > > backlight class, then I can remove the sysfs attribute. > > If I understand, acpi_backlight=vendor works, but acpi_backlight=video > doesn't, right ? > > If there is no way to make the video module work, then you can > probably blacklist > the shuttles in > drivers/acpi/video_detect.c:acpi_video_get_capabilities. > > How does it work on windows ?
Sorry late response... >From what I remember, Windows has same bug (when using the Windows 7 brightness slider it doesn't update brightness on screen), only the shuttle application which does the brightness down/up stuff works. It's an implementation bug in bios (in their acpi video part). I expect it'll be fixed for the production hardware, but if not perhaps this machine will have the first entry in video_detect.c, and I'll have to keep the workaround in backlight implementation. > > > Shuttle DA18IE is weird because it has an hack on hardware: the LCD > > of the machine is connected on the same VGA output of the machine, > > that is, instead of using the LVDS port, they connect the VGA port, > > and because this you can't have proper display switch on it, it > > works like a VGA splitter, very ugly. This hardware may change, so > > may be we will not have to care any more about this and remove both > > sysfs attribute and quirk, and one reason I don't send the driver > > yet as final version. > > > >> > >> > +What: /sys/devices/platform/shuttle_wmi/cut_lvds > >> > +Date: November 2010 > >> > +KernelVersion: 2.6.37 > >> > +Contact: "Herton Ronaldo Krzesinski" > >> > <[email protected]> +Description: > >> > + This is a write only option. Writing any single > >> > non-zero value > >> > + to it enables main screen output, 0 to disable. > >> > >> Same, could be handled by the backlight device. > > > > I'll remove and see to move to backlight. > > See asus-laptop, it have some blank/unblank support for the backlight > device. > > >> > +What: > >> > /sys/devices/platform/shuttle_wmi/lbar_brightness_down +Date: > >> > November 2010 +KernelVersion: 2.6.37 > >> > +Contact: "Herton Ronaldo Krzesinski" > >> > <[email protected]> +Description: > >> > + This is a write only option (accepts any single > >> > value, eg. > >> > + "echo 1 > lbar_brightness_down"). Decreases one > >> > step of lightbar > >> > + brightness. > >> > + > >> > +What: > >> > /sys/devices/platform/shuttle_wmi/lbar_brightness_up +Date: > >> > November 2010 +KernelVersion: 2.6.37 > >> > +Contact: "Herton Ronaldo Krzesinski" > >> > <[email protected]> +Description: > >> > + This is a write only option (accepts any single > >> > value, eg. > >> > + "echo 1 > lbar_brightness_up"). Increases one > >> > step of lightbar > >> > + brightness. > >> > >> What is the lightbar exactly ? Some kind of led ? Can't you use the > >> led class instead ? > > > > None of shuttle machines I have here have this lightbar, but it > > should be a cosmetic light in the front of the machine at bottom. I > > don't know if it's useful treat is as led, check this pdf: > > http://www.shuttle.eu/fileadmin/resources/download/docs/spec/complete_systems/X5020XA_e.pdf > > > > At page 6, there is a picture with the light bar, it's the item 8. > > > > I don't know if the light bar should be handled by any kernel > > subsystem, or keep as is (just sysfs attributes). > > Looking at the pdf, this should definitly be handled as a led ! I went to implement it as a led, but I have a problem, I don't know where it saves the brightness values (current brightness value), and also don't know what's the maximum brightness. I wrote a script and a debugfs file to read all memory positions from EC by wmi with "CMD_READEC", to make a diff and see if after writing the lightbar up/down commands where values change. But may be without hardware with this lightbar I can't confirm the change, and may be without it values will not change/function will not work. Well, if I can't come up with something, I plan to keep it only at debugfs. > > >> Also the driver seems to reference keyboard brightness, if > >> available, this should be > >> implemented as a led named shuttle::kbd_backlight. > > > > I don't know what's the brightness of keyboard too, that is on > > shuttle documentation without specific explanation, hardware I have > > doesn't have this. > > > >> > + > >> > +What: /sys/devices/platform/shuttle_wmi/volume_down > >> > +Date: November 2010 > >> > +KernelVersion: 2.6.37 > >> > +Contact: "Herton Ronaldo Krzesinski" > >> > <[email protected]> +Description: > >> > + This is a write only option (accepts any single > >> > value, eg. > >> > + "echo 1 > volume_down") that is equivalent of > >> > pressing > >> > + fn+<volume down> function on notebooks. > >> > + > >> > +What: /sys/devices/platform/shuttle_wmi/volume_up > >> > +Date: November 2010 > >> > +KernelVersion: 2.6.37 > >> > +Contact: "Herton Ronaldo Krzesinski" > >> > <[email protected]> +Description: > >> > + This is a write only option (accepts any single > >> > value, eg. > >> > + "echo 1 > volume_up") that is equivalent of > >> > pressing > >> > + fn+<volume up> function on notebooks. > >> > >> Are volume_down and volume_up really needed ? can't alsamixer do > >> the same ? > > > > Yes, I just kept for testing, I will remove it. > > You can put it in debugfs if you want to keep it for debug purpose. > > >> > >> > +What: /sys/devices/platform/shuttle_wmi/webcam > >> > +Date: November 2010 > >> > +KernelVersion: 2.6.37 > >> > +Contact: "Herton Ronaldo Krzesinski" > >> > <[email protected]> +Description: > >> > + Control webcam state. 1 means on, 0 means off. > >> > + > >> > +What: /sys/devices/platform/shuttle_wmi/white_balance > >> > +Date: November 2010 > >> > +KernelVersion: 2.6.37 > >> > +Contact: "Herton Ronaldo Krzesinski" > >> > <[email protected]> +Description: > >> > + This is a write only option (accepts any single > >> > value, eg. > >> > + "echo 1 > white_balance"). Probably triggers an > >> > automatic > >> > + white balance adjustment for lcd, function not > >> > explained in > >> > + shuttle wmi documentation. > >> > >> > >> Here, I don't really understand why you reference "fn+<keys>". We > >> don't really care about the keys right ? What we want is make the > >> feature available for the user. > > > > I use fn+<code> because that's what the firmware/wmi interface > > understands. The 0xEC000200000000<byte command number> command sent > > by wmi (CMD_SCMD) in fact in most cases is the same as sending > > fn+<byte command number> using the keyboard. > > > > So if the notebook keyboard has webcam switch at fn+f9, the command > > we are sending to the bios is: > > 0xEC00020000000009 > > to turn on or off the webcam. The state (on or off) is read using > > CMD_READEC from some address, when it saves state somewhere. > > > > If the sound mute is fn+f4, then the command is 0xEC00020000000004, > > and so on... > > > > The hardware is a notebook without notebook keyboard (it should > > have the same keyboard bios as similar notebook models). There is a > > button on the side of the machine, that sends a key code when > > pressed (in the driver I translate it to KEY_PROG1). This button > > should load an OSD application on the screen, with buttons similar > > to the function buttons on notebook keyboard. When you click the > > button on application, it must write a value > > to /sys/devices/platform/shuttle_wmi/<attribute>, which will call > > the driver that send the command as if it was the keyboard. That's > > why that many sysfs attributes, but some of them like volume aren't > > needed indeed, mixer can be called directly for example, etc. so no > > problem in removing them. > > Ok, now I understand, It's like the existing Win 7 OSD. And I also > understand why the driver looks like this. You should really try to > use generic classes/stuff as > much as possible (backlight, led, rfkill, alsa), and add new sysfs > files when there > is no other choices. > > This way, it will work with the OSD, but it will also work out the box > on a standard > distribution :). Yes, in new version I plan to post soon, in ended moving all irrelevant/testing attributes to debugfs. > > >> > >> Some of the files likes volume_up/volume_down seems to be here for > >> debug purpose. Maybe you could add a simple debugfs interface > >> to send custom commands to the wmi device. > > > > Yes, I'll see here and I'll move the testing attributes I used to > > debugfs, or just a generic fn+number. > > > >> > >> > diff --git a/drivers/platform/x86/Kconfig > >> > b/drivers/platform/x86/Kconfig index faec777..ef84a4d 100644 > >> > --- a/drivers/platform/x86/Kconfig > >> > +++ b/drivers/platform/x86/Kconfig > >> > @@ -639,4 +639,20 @@ config XO1_RFKILL > >> > Support for enabling/disabling the WLAN interface on > >> > the OLPC XO-1 laptop. > >> > > >> > +config SHUTTLE_WMI > >> > + tristate "Shuttle WMI Extras" > >> > + depends on ACPI > >> > + depends on BACKLIGHT_CLASS_DEVICE > >> > + depends on RFKILL > >> > + depends on INPUT > >> > + select ACPI_WMI > >> > + select INPUT_SPARSEKMAP > >> > + ---help--- > >> > + This is a driver for Shuttle machines (mainly for > >> > laptops in desktop > >> > + form factor). It adds controls for wireless, > >> > bluetooth, and 3g control > >> > + radios, webcam switch, backlight controls, among > >> > others. + > >> > + If you have an Shuttle machine with ACPI-WMI interface > >> > say Y or M > >> > + here. > >> > + > >> > >> Add some DA18IE/DA18IM/MA ref here ? > >> > >> > endif # X86_PLATFORM_DEVICES > >> > diff --git a/drivers/platform/x86/Makefile > >> > b/drivers/platform/x86/Makefile index 9950ccc..6a8fa82 100644 > >> > --- a/drivers/platform/x86/Makefile > >> > +++ b/drivers/platform/x86/Makefile > >> > @@ -33,3 +33,4 @@ obj-$(CONFIG_INTEL_IPS) += > >> > intel_ips.o obj-$(CONFIG_GPIO_INTEL_PMIC) += intel_pmic_gpio.o > >> > obj-$(CONFIG_XO1_RFKILL) += xo1-rfkill.o > >> > obj-$(CONFIG_IBM_RTL) += ibm_rtl.o > >> > +obj-$(CONFIG_SHUTTLE_WMI) += shuttle-wmi.o > >> > diff --git a/drivers/platform/x86/shuttle-wmi.c > >> > b/drivers/platform/x86/shuttle-wmi.c new file mode 100644 > >> > index 0000000..389a16d > >> > --- /dev/null > >> > +++ b/drivers/platform/x86/shuttle-wmi.c > >> > @@ -0,0 +1,843 @@ > >> > +/* > >> > + * ACPI-WMI driver for Shuttle DA18IE/DA18IM/MA > >> > + * > >> > + * Copyright (c) 2009 Herton Ronaldo Krzesinski > >> > <[email protected]> > >> > >> 2010 ? > >> > > > > Ops, going to fix these. > > > >> > +static acpi_status wmi_setget_mtd(struct shuttle_cmd *scmd, u32 > >> > **res) +{ > >> > + acpi_status status; > >> > + union acpi_object *obj; > >> > + struct acpi_buffer input; > >> > + static DEFINE_MUTEX(mtd_lock); > >> > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, > >> > NULL }; + > >> > + input.length = sizeof(struct shuttle_cmd); > >> > + scmd->hdr = 0xec00; > >> > + input.pointer = (u8 *) scmd; > >> > + > >> > + mutex_lock(&mtd_lock); > >> > + status = wmi_evaluate_method(SHUTTLE_WMI_SETGET_GUID, 0, > >> > 2, > >> > + &input, &output); > >> > + mutex_unlock(&mtd_lock); > >> > >> I'm not sure why you need a mutex here ? > > > > I need mutex because of the way the vendor implemented things in > > its wmi interface. If I don't serialize the access to it, when > > sending commands in parallel some of them will fail, for example: > > > > # echo 1 > webcam & rfkill unblock <idx> & echo 1 > touchpad > > > > some of the echos or rfkill will fail. This happens because the > > vendor used a common buffer/variable to store parameters in AML > > code, from the DSDT: > > > > Name (AC00, Buffer (0x28) > > ... > > CreateDWordField (AC00, Zero, SAC0) > > CreateDWordField (AC00, 0x04, SAC1) > > ... > > CreateByteField (AC00, Zero, SA00) > > CreateByteField (AC00, One, SA01) > > CreateByteField (AC00, 0x02, SA02) > > CreateByteField (AC00, 0x03, SA03) > > CreateByteField (AC00, 0x04, SA04) > > ... > > Method (WMBC, 3, NotSerialized) > > { > > If (LEqual (Arg1, One)) > > { > > Return (GETC (Arg0)) > > } > > > > If (LEqual (Arg1, 0x02)) > > { > > Return (SETC (Arg0, Arg2)) > > } > > ... > > Method (SETC, 2, NotSerialized) > > { > > If (LEqual (Arg0, Zero)) > > { > > Store (Arg1, AC00) > > OEMF (AC00) > > Return (SAC0) > > } > > ... > > Method (OEMF, 1, NotSerialized) > > { > > If (LEqual (SAC1, 0xEC000300)) > > { > > Store (0x73, DBG8) > > Store ("LS", SAC0) > > Return (SAC0) > > } > > > > If (LEqual (SAC1, 0xEC000000)) > > { > > WKBC (SA00, SA01, SA02, SA03) > > Return (SAC0) > > } > > ... > > > > > > Note that it uses only buffer AC00 to store and read the > > parameters, so parallel calls will overwrite the value in AC00 > > (referenced as SAC?? and SA?? in some places) and some commands > > will fail or have unpredicted behaviour. > > > > Placing the lock prevents the parallel echo/rfkill command I placed > > above as example to fail, all writes/commands are sent successfuly. > > > > I'm attaching here DSDT of both machines I have for you to see the > > entire code. > > Hum ok, if I understand AML correctly, I think the mutex should be in > directly in the DSDT but now that it's done... > > Maybe you should add a comment for the mutex, so nobody will try to > remove it someday. > > >> > >> > + if (ACPI_FAILURE(status)) > >> > + return status; > >> > + > >> > + obj = output.pointer; > >> > + if (obj) { > >> > + if (obj->type == ACPI_TYPE_INTEGER) { > >> > + if (*res) > >> > + **res = obj->integer.value; > >> > + } else > >> > + pr_err("Unsupported object returned > >> > (%s)", __func__); > >> > + kfree(obj); > >> > + } else { > >> > + if (*res) { > >> > + pr_warning("No result from WMI method > >> > (%s)", __func__); > >> > + *res = NULL; > >> > + } > >> > + } > >> > + return AE_OK; > >> > +} > >> > > >> > >> wmi_setget_mtd would probably be simpler like that: > >> > >> static int wmi_setget_mtd(struct shuttle_cmd *scmd, u32 *res). > >> You don't really need to return the acpi_status, you just want to > >> return -1, 0, 1 for wmi_ec_cmd. > > > > Good point, will simplify it. > > > >> > >> > +static int wmi_ec_cmd(unsigned char cmd, unsigned char arg, > >> > + unsigned short param1, unsigned short > >> > param2, > >> > + u32 *res) > >> > +{ > >> > + struct shuttle_cmd scmd = { > >> > + .cmd = cmd, > >> > + .arg = arg, > >> > + .param1 = param1, > >> > + .param2 = param2 > >> > + }; > >> > + > >> > + if (ACPI_FAILURE(wmi_setget_mtd(&scmd, &res))) > >> > + return -1; > >> > + return (res) ? 0 : 1; > >> > +} > >> > + > >> > +struct shuttle_rfkill { > >> > + char *name; > >> > + struct rfkill *rfk; > >> > + enum rfkill_type type; > >> > + unsigned short fn; > >> > + u32 mask; > >> > + u32 list_on[3]; > >> > + u32 list_off[3]; > >> > +}; > >> > + > >> > +static struct shuttle_rfkill shuttle_rfk_list[] = { > >> > + { "shuttle_wlan", NULL, RFKILL_TYPE_WLAN, > >> > + 0x04, 0x80, { 0x08 }, { 0x09 } }, > >> > + { "shuttle_bluetooth", NULL, RFKILL_TYPE_BLUETOOTH, > >> > + 0x0d, 0x20, { 0x0c, 0x29 }, { 0x0d, 0x2a } }, > >> > + { "shuttle_3g", NULL, RFKILL_TYPE_WWAN, > >> > + 0x05, 0x40, { 0x10, 0x29 }, { 0x11, 0x2a } }, > >> > +}; > >> > >> I would rather use simple if/else constructs instead of > >> list_on/list_off, it would probably be more easy to read. > > > > I don't know, I like the array approach as that it's easier to add > > new rfkill types, less hand written code, and more flexible as > > number of notification codes are variable, current we have: > > > > fn+f4 = send command byte == 0x04 == wireless switch > > wmi notification codes: > > - wireless on = 0x08 > > - wireless off = 0x09 > > > > fn+f13? = send command byte == 0xd == bluetooth switch > > wmi notification codes: > > - bluetooth on = 0x0c > > - bluetooth off = 0x0d > > - 3g/bluetooth on = 0x29 > > - 3g/bluetooth off = 0x2a > > > > fn+f5 = send command byte == 0x5 == 3g switch > > wmi notification codes: > > - 3g on = 0x10 > > - 3g off = 0x11 > > - 3g/bluetooth on = 0x29 > > - 3g/bluetooth off = 0x2a > > Then, keep it like this, if nobody else compalins about it, it's > probably ok. I rewrote part of the driver, new version I hope is easier to read/better, this is different now. Soon I hope to post the new version (almost finished, just have to check lightbar/hardware presence stuff and other tests I want to do, other things you pointed I fixed, thanks for the review). > >> > + wmi_ec_cmd(CMD_SCMD, 0, 0, srfk->fn, NULL); > >> > + else > >> > + return 0; > >> > + > >> > + if (wmi_ec_cmd(CMD_READEC, 0, 0, ECRAM_ER1, &val)) > >> > + return -EIO; > >> > + return ((val & srfk->mask) != blocked) ? 0 : -EIO; > >> > +} > >> > + > >> > +static const struct rfkill_ops rfkill_common_ops = { > >> > + .set_block = rfkill_common_set_block, > >> > +}; > >> > + > >> > +static int common_rfkill_init(struct shuttle_rfkill *srfk, > >> > struct device *dev, > >> > + u32 init_val) > >> > +{ > >> > + int rc; > >> > + > >> > + srfk->rfk = rfkill_alloc(srfk->name, dev, srfk->type, > >> > + &rfkill_common_ops, srfk); > >> > + if (!srfk->rfk) > >> > + return -ENOMEM; > >> > + > >> > + rfkill_init_sw_state(srfk->rfk, !(init_val & > >> > srfk->mask)); + > >> > + rc = rfkill_register(srfk->rfk); > >> > + if (rc) { > >> > + rfkill_destroy(srfk->rfk); > >> > + srfk->rfk = NULL; > >> > + return rc; > >> > + } > >> > + > >> > + return 0; > >> > +} > >> > + > >> > +static int shuttle_rfkill_init(struct platform_device *pdev) > >> > +{ > >> > + int rc, i; > >> > + u32 val; > >> > + struct shuttle_rfkill *srfk; > >> > + > >> > + if (wmi_ec_cmd(CMD_READEC, 0, 0, ECRAM_ER1, &val)) > >> > + return -EIO; > >> > + > >> > + for (i = 0; i < ARRAY_SIZE(shuttle_rfk_list); i++) { > >> > + srfk = &shuttle_rfk_list[i]; > >> > + > >> > + /* check that hardware is available (when > >> > missing we can't > >> > + * unblock it), to avoid having rfkill switch > >> > when not needed; > >> > + * after check, reset to initial setting */ > >> > + if (rfkill_common_set_block(srfk, false)) > >> > + continue; > >> > + if (!(val & srfk->mask)) > >> > + rfkill_common_set_block(srfk, true); > >> > >> There is no other solution to guess if the hardware is present ? > >> Because I don't think it's a good idea to toggle every devices on > >> load (may take time, etc...) > > > > Unfortunately there is no other way, there is nothing that I know > > currently which can be used to see if hardware is available or not. > > If I understand correctly the AML code (but it's a mess, so I probably > don't), when > fetching the status for a missing device, is will returns you > something full of ones (0xFFFFFF..) Nope, it returns 0xFFFFFF... when you call the wmi method with invalid command/parameter. What I'll try to do is making an ec memory dump I'm making as for the lightbar case, and do a diff with or without the hardware attached to see if I can get something (dump is slow, takes some time). On the shuttle wmi documentation, it has the "Device no function" event, which happens when you try to enable a non-existent hardware, which is the same as trying to enable/turn on it and it fails, so I don't know if I'll find a bit in memory which has the state of hardware present or not. -- []'s Herton -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
