Em Wed, 1 Dec 2010 21:05:14 +0100
Corentin Chary <corentin.chary@xxxxxxxxx> escreveu:
> On Wed, Dec 1, 2010 at 5:14 PM, Herton Ronaldo Krzesinski
> <herton@xxxxxxxxxxxxxxx> wrote:
> > Hi,
> >
> > I cut the message just to the comments, here we go:
> >
> > On Wed, 1 Dec 2010 00:22:03 +0100
> > Corentin Chary <corentin.chary@xxxxxxxxx> wrote:
> >> On Tue, Nov 30, 2010 at 6:27 PM, Herton Ronaldo Krzesinski
> >> <herton@xxxxxxxxxxxxxxx> wrote:
> >> > +What: Â Â Â Â Â/sys/devices/platform/shuttle_wmi/brightness_up
> >> > +Date: Â Â Â Â ÂNovember 2010
> >> > +KernelVersion: 2.6.37
> >> > +Contact: Â Â Â "Herton Ronaldo Krzesinski"
> >> > <herton@xxxxxxxxxxxxxxx> +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"
> >> > <herton@xxxxxxxxxxxxxxx> +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"
> >> > <herton@xxxxxxxxxxxxxxx> +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"
> >> > <herton@xxxxxxxxxxxxxxx> +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"
> >> > <herton@xxxxxxxxxxxxxxx> +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"
> >> > <herton@xxxxxxxxxxxxxxx> +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"
> >> > <herton@xxxxxxxxxxxxxxx> +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"
> >> > <herton@xxxxxxxxxxxxxxx> +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
> >> > <herton@xxxxxxxxxxxxxxx>
> >>
> >> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
